Skip to content

Fix for BLD-465: e-reader error when popping out window. - #1856

Merged
valera-rozuvan merged 1 commit into
masterfrom
valera/fix_e_reader_error_new_page
Dec 9, 2013
Merged

Fix for BLD-465: e-reader error when popping out window.#1856
valera-rozuvan merged 1 commit into
masterfrom
valera/fix_e_reader_error_new_page

Conversation

@valera-rozuvan

Copy link
Copy Markdown
Contributor

BLD-465

Description of BLD-465
The attached error is seen when attempting to pop out the e-reader in a different window - happens about 50% of the time. Can be seen in 2.03x here: https://courses.edx.org/courses/MITx/2.03x/3T2013/courseware/Course_Text/Textbook/# . Also noted in 4.605x course.

Reviewers

NOTE
Original work was going on in https://github.com/edx/edx-platform/pull/1811 . Create a separate PR so as not to block @olmar .

@ghost ghost assigned singingwolfboy Dec 5, 2013
@auraz

auraz commented Dec 5, 2013

Copy link
Copy Markdown
Contributor

👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Presumably, some view function needs to call this handle_ajax function, and return the response as a HttpResponse instance. How does the caller know if this function returned an error message? (If it did, the HttpResponse should have some sort of error status code, rather than 200.) It might be better to have this function raise an exception, which the caller can catch.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this function will raise Exception, it will be raised in line 594 if module_render.py, so server will generate 500 error. Is it what you want?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps a new error type should be added to xmodule/exceptions.py to account for invalid requests, which xmodules could raise to indicate that the request is invalid. However, that seems outside the scope of this pull request, and anyway, I don't think we want to spend more time and effort developing xmodules, so it doesn't seem worthwhile. Since xmodule doesn't support what I was looking for, this is fine.

@jmclaus

jmclaus commented Dec 6, 2013

Copy link
Copy Markdown

@valera-rozuvan 👍 When Jenkins agrees.

@singingwolfboy

Copy link
Copy Markdown
Contributor

👍 once the tests pass. You can try rebasing your branch onto the latest master to see if that gets the tests to pass.

Moving work on BLD-465 from PR 1811.
Fixing missing import clause in Python.
Addressing DB's comment.

BLD-465.
valera-rozuvan added a commit that referenced this pull request Dec 9, 2013
Fix for BLD-465: e-reader error when popping out window.
@valera-rozuvan
valera-rozuvan merged commit 3730eb3 into master Dec 9, 2013
@valera-rozuvan
valera-rozuvan deleted the valera/fix_e_reader_error_new_page branch December 9, 2013 11:03
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