-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MA-1919 making mobile handout links accommodate jump to id's and cour… #11602
Conversation
handouts_html = replace_course_urls( | ||
handouts_html, | ||
course_key=course.id | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this issue pre-exists, but..
I wonder if there's a good way to refactor this code so it is shared between LMS Web and Mobile API code. Otherwise, if it's updated in LMS, developers won't know to update it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There probably should be a fairly straightforward way to do that. The modules are coming from the same place, after all.
frag = Fragment(content=unicode(handouts_html)) | ||
for wrapper in course_handouts_module.system.wrappers: | ||
frag = wrapper(block=course_handouts_module, view=None, frag=frag, context=None) | ||
handouts_html = frag.content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Per DRY, can we have a common helper for L87-90 and L51-54?
Looks good. Just a few comments. |
# check that we start with relative static assets | ||
handouts_usage_key = self.course.id.make_usage_key('course_info', 'handouts') | ||
underlying_handouts = self.store.get_item(handouts_usage_key) | ||
underlying_handouts.data = "<a href=\"/jump_to_id/identifier\">Intracourse Link</a>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a relative URL, why does it start with a leading slash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the style of URL we allow instructors to use when they are creating courses. For example, MIT's "Minds and Machines" course uses:
<ol>
<a href="/jump_to_id/2b02305f3c494aa0b7f0d43aa0778fa9">Discussion Forum Guidelines</a>
</ol>
<ol>
<a href="/jump_to_id/095f84e31ecf485bb14b51e7b7991db8">Class Schedule</a>
</ol>
https://studio.edx.org/course_info/course-v1:MITx+24.09x+3T2015#
👍 Once tests pass. |
👍 |
Not sure how that got closed. |
7c6a964
to
75a26b6
Compare
MA-1919 making mobile handout links accommodate jump to id's and cour…
@nasthagiri @jcdyer
Fixes https://openedx.atlassian.net/browse/MA-1919