Parse urls with query params for ^/static - #3394
Conversation
|
@jbau please. |
|
@dcadams this seems good. |
|
@sarina can you give this a review, or schedule it for review, or bounce it to someone? Let me give you some context. Basically we have things breaking on our instance b/c the "/static" URL expansion happens on the path part of URLs, but not within query parameters. The issue here is that this is inconsistent with the behavior in studio's HTML editor, which will collapse tagging @cahrens as an FYI, and she can confirm the studio behavior. |
There was a problem hiding this comment.
I don't think you need re.sub here (you can just slice the string), but I don't feel strongly about making this change. Others might though.
Actually, I don't see the subbing out of "/static" in the code above your addition. is it really necessary?
|
Here is a very brief description of what Studio is doing. In the TinyMCE Editor for HTML components, we rewrite /static -> c4x notation when loading in the Editor, and we do the reverse on save. This is necessary so that links and images appear correctly in the WYSIWYG editor (in the code editor accessible from the toolbar, we show only the /static notation, as that is what is actually persisted). The rewrite code we use is very simplistic, so it converts all c4x links to /static on Save. We should not be storing absolute links to the courseware in mongo because that is not portable (import into a new course will not work as the links will not refer to the new course). |
|
@jbau I'm unfamiliar with this area of the codebase. @singingwolfboy has been triaging PRs, I'm hoping he can help identify a reviewer for this case. |
|
Thank you @cahrens I was just looking at the raw HTML editor. We are not in a bind right now as we haven't pulled in the original editor change yet. I believe this PR will still be beneficial in preventing cross-course linking of assets. |
|
I agree-- I do think this PR is the right thing to do. |
|
@singingwolfboy any chance you can schedule a look at this? |
There was a problem hiding this comment.
You could use a loop like for query_name, query_value in orig_query, and then use query_value instead of query_item[1], which I think would make the operation clearer.
|
@dianakhuang could you schedule a review please. |
There was a problem hiding this comment.
We prefer the indentation style listed here: https://github.com/edx/edx-platform/wiki/Python-Guidelines#breaking-long-lines
|
👍 |
|
🎋 🍊 ⛵ once the indentation style thing is fixed |
|
FYI @dcadams is on vacation today. He'll be back in tomorrow 5/28 |
Query params with a value that starts with /static should have their values converted to the full location. Query params with a value that does not start with /static should be left unchanged.
|
Indentation fixed, re-based, all tests passing. |
Parse urls with query params for ^/static
8 participants
Query params with a value that starts with /static should have their values converted to the full location.
Query params with a value that does not start with /static should be left unchanged.