Fix for static path parsing #1396
Conversation
|
@cdodge This is the issue we discussed in email the other day where a url with query params gets mangled. Would you mind taking a look please. |
|
Note that although the PR is pointing to a failed test, it isn't for this PR. It seems someone stole my hash :) Anyway, the correct test run for this PR is at https://jenkins.testeng.edx.org/job/edx-all-tests-manual-commit/18/ @wedaly is aware of the issue. |
There was a problem hiding this comment.
Would it be possible to put this 'query string parameter aware-ness' inside the .convert_legacy_static_url_with_course_id rather than the replace_static_url? It seems generic enough to be useful other places.
|
Thanks for the bug fix submission. |
|
@chrisndodge moved the code per your suggestion. Thanks. |
|
@chrisndodge Is this good to go? |
|
@dcadams sorry for the radio silence - been heads down on some work. Looking again... |
There was a problem hiding this comment.
I haven't used urlparse before, but is there another way to reference the params element besides the [4] index. It's not very readable. Can you do urlparse(path).params instead?
There was a problem hiding this comment.
Short answer: yes, you can.
|
@chrisndodge @cpennington Thank you for the comments. Please take another look. |
There was a problem hiding this comment.
I don't think you need the two cases, now. Couldn't you just take the loc_url, and then urljoin the rest of url_parse (minus the path) onto it? Then, if query is empty, it should work the same as the old code, and if query isn't empty, it'll do the right thing.
When a url contains a query parameter it gets incorrectly parsed when replacing the /static portion. This fix handles urls with query parameters.
|
@cpennington good idea. Would you mind taking another look. |
|
👍 |
* Implements ga_optional openedx#1239 (openedx#1393) * Fix has_terminated on course-overview. openedx#1324 (openedx#1391) * Version up CourseOverview to regenerate CourseOverview. * Add send_mail option to ContractAuth(biz) openedx#1322 (openedx#1392) * Revert "Revert "Merge pull request openedx#1298 from hachiyanagi-ks/develop/dogwood/…" (openedx#1394) * Version up django and ora2 openedx#1375 (openedx#1396) * Version up to django==1.8.16 openedx#1375 * Version up ora2 to gacco/eucalyptus==1.1.5 openedx#1375 * Add format file openedx#1428 (openedx#1465) * Text correction of scoring by instructor openedx#1467
When a url contains a query parameter it gets incorrectly parsed
when replacing the /static portion.
This fix introduces a special case for urls with query parameters.
Fix for JIRA issue: LMS-1377