Skip to content

Fix dashboard report image download (edx-release hotfix) - #106

Merged
omarkhan merged 2 commits into
edx-releasefrom
omar/edx-release/report-download
Mar 14, 2016
Merged

Fix dashboard report image download (edx-release hotfix)#106
omarkhan merged 2 commits into
edx-releasefrom
omar/edx-release/report-download

Conversation

@omarkhan

Copy link
Copy Markdown
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

@omarkhan
omarkhan force-pushed the omar/edx-release/report-download branch from 0f201e0 to 74f8cda Compare March 11, 2016 08:08
@omarkhan

Copy link
Copy Markdown
Contributor Author

The tests are failing because the version of xblock-sdk this is using does not include the version of bok choy that allows us to test screenshots. @bradenmacdonald shall I just remove the tests from this hotfix PR and put them in a PR against master?

@omarkhan
omarkhan force-pushed the omar/edx-release/report-download branch from e8f8faa to 415845d Compare March 11, 2016 11:43
"""
self.assertScreenshot('image', 'dashboard-image', threshold=3000)

@check_dashboard_and_report(SIMPLE_DASHBOARD, image='//courses.edx.org/c4x/HarvardX/GSE3x/asset/swoop-bg.png')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If your test relies on an an external image, it should be hosted on GitHub.

@bradenmacdonald

Copy link
Copy Markdown
Member

@omarkhan Sure, if you want to use screenshot tests, you can include them only in the master version.

For the screenshots, is it possible to include some colorization in one of the segments of the image? That would be a more comprehensive test. (See screenshot I attached to the OC ticket).

@omarkhan
omarkhan force-pushed the omar/edx-release/report-download branch from a8b91d5 to db51593 Compare March 14, 2016 02:33
@omarkhan

Copy link
Copy Markdown
Contributor Author

@bradenmacdonald I have removed the tests from this PR, but the existing tests are failing on circleci. I did some digging and it looks like none of the test setup (installing test requirements, using the run_tests.py wrapper script) is happening on circleci. Is there a reason we are using circleci as well as travis? Should I fix the circleci tests or just drop them?

@omarkhan omarkhan changed the title Fix dashboard report image download Fix dashboard report image download (edx-release hotfix) Mar 14, 2016
@omarkhan

Copy link
Copy Markdown
Contributor Author

@bradenmacdonald I have opened another PR for master that includes tests, including a test for the colorized overlays: #107

@bradenmacdonald

Copy link
Copy Markdown
Member

@omarkhan We use CircleCI on master and Travis on edx-release. Eventually we will move both to CircleCI.

@omarkhan

Copy link
Copy Markdown
Contributor Author

@bradenmacdonald then I guess this is ready to go, once I get your +1

@bradenmacdonald

Copy link
Copy Markdown
Member

@omarkhan Nice fix. Code LGTM. Haven't tested it. @Kelketek is your reviewer and will give you +1.

@Kelketek

Copy link
Copy Markdown
Member

👍

@omarkhan
omarkhan force-pushed the omar/edx-release/report-download branch from db51593 to 85f9a26 Compare March 14, 2016 16:38
@omarkhan

Copy link
Copy Markdown
Contributor Author

@tobz this is ready for upstream review, can you take a look? Thanks!

@tobz

tobz commented Mar 14, 2016

Copy link
Copy Markdown

LGTM. 👍

omarkhan added a commit that referenced this pull request Mar 14, 2016
Fix dashboard report image download (edx-release hotfix)
@omarkhan
omarkhan merged commit fe0e825 into edx-release Mar 14, 2016
@omarkhan
omarkhan deleted the omar/edx-release/report-download branch March 14, 2016 17:15
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