Skip to content

Conversation

@omarkhan
Copy link
Contributor

The dashboard report image download can currently fail in 2 different ways:

  • If the image is from a different domain and it has been cached by the browser, it won't be converted into a data URL due to cross-domain security issues. A javascript SecurityError is thrown, and nothing happens.
  • If the image url starts with // (no explicit protocol) or the url is relative (e.g. /static/...), and it hasn't yet been cached by the browser, it will fail to show up in the downloaded report.

This pull request fixes both issues, by:

  • Not trying to create a data URL for cross-domain requests
  • Expanding relative urls and urls without an explicit protocol

This pull request also extends the integration tests to test report downloads.

@Kelketek
Copy link
Member

@omarkhan Test failing here, but it may just need to be rerun.

@omarkhan
Copy link
Contributor Author

@Kelketek the tests are passing now, yay

@Kelketek
Copy link
Member

👍

@omarkhan
Copy link
Contributor Author

@bradenmacdonald do you want to review this one as well?

@bradenmacdonald
Copy link
Member

@omarkhan Not necessary, thanks. I will review the edx-release hash update PR though.

@bradenmacdonald
Copy link
Member

@omarkhan Actually never mind I'm not going to review any of these PRs. I was only going to review this master PR if it was a merge from edx-release to master, which can be tricky`, but you just implemented it separately on each branch so that's fine.

@omarkhan
Copy link
Contributor Author

@bradenmacdonald do I need to get thumbs up from @tobz on this PR as well?

@bradenmacdonald
Copy link
Member

@omarkhan No, the master branch of Problem Builder is maintained by OpenCraft alone. EdX review is only required for the edx-release branch.

@omarkhan
Copy link
Contributor Author

@bradenmacdonald thanks that's what I figured, just wanted to make sure before merging

@omarkhan omarkhan force-pushed the omar/report-download branch from 6118a36 to 0ef0a74 Compare March 17, 2016 02:04
omarkhan added a commit that referenced this pull request Mar 17, 2016
Fix dashboard report image download
@omarkhan omarkhan merged commit e01af34 into master Mar 17, 2016
@omarkhan omarkhan deleted the omar/report-download branch March 17, 2016 02:25
@omarkhan omarkhan restored the omar/report-download branch March 17, 2016 06:22
itsjeyd added a commit to open-craft/openedx-platform that referenced this pull request Mar 24, 2016
Associated PRs:

- open-craft/problem-builder#107 (Fix dashboard report image download)
- open-craft/problem-builder#108 (Add instructions for enabling dashboard in studio)
- open-craft/problem-builder#109 (Use dashboard image from master branch in tests)
- open-craft/problem-builder#111 (StepBuilder: Fix inconsistent feedback for MCQs)
@xitij2000 xitij2000 deleted the omar/report-download branch December 13, 2019 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants