MA-722 Render xBlock API - #8240
Conversation
nasthagiri
changed the title
88430c1 to
3875427
Compare
d112ab4 to
ef84032
Compare
There was a problem hiding this comment.
Note to reviewers: get_opt_course_with_access was removed since there were no callers. Must be an old function.
1d30ece to
1efddd5
Compare
There was a problem hiding this comment.
Note to reviewers: Checking for enrollment was originally done in can_load_mobile in access.py. But now, all enrollment checks are consolidated in get_course_with_access.
|
I'd also suggest bringing in @cpennington and @simultech (Andrew Decker from UQ), since they're exploring some refactoring around the courseware template. |
|
There are currently LTI failures caused by merge conflicts with the latest |
871d90c to
0b68e5e
Compare
3b914ad to
d80f1d8
Compare
There was a problem hiding this comment.
Why disable_staff_debug_info?
There was a problem hiding this comment.
When viewing just an xBlock does it make sense to display other LMS-view related data, like "Submission History" and "Staff Debug Info"?
For Mobile, we don't want this information. It currently renders a UX-unfriendly pop-up with debug information.
May I assume the same requirements for LTI as well?
There was a problem hiding this comment.
I would have thought it would be convenient, particularly in the LTI use case, since the staff may not interact regularly with the LMS. @mcgachey: any thoughts on this?
There was a problem hiding this comment.
With the way we have things set up now, everybody's going to be viewing content as a student, so it doesn't make a lot of difference for that. My only major concern if we ever did have people use staff access would be that if a staff member viewed a chromeless template, is there a danger that they could escape into areas of the platform that we don't want them to access (at least in the LTI iframe), potentially ending up with recursive chrome?
There was a problem hiding this comment.
yeah, good point. Given that, we may want to keep it safe and not expose staff information for now.
|
Done with my first pass. |
|
Once the feature flag name is changed and you've confirmed that this doesn't explode access check or modulestore queries on your local dev, I'm 👍 |
edb4200 to
e5a12cf
Compare
|
👍 |
1 similar comment
|
👍 |
|
One nitpick comment, otherwise 👍 |
e5a12cf to
d240785
Compare
…tent Change https://github.com/edx/edx-platform/pull/8240 refactored the LTI provider template rendering code and introduced an issue where a user was required to be enrolled in a course before that course's content could be accessed over LTI. According to the LTI design spec: https://docs.google.com/document/d/185hdPvIxcKtiDOLjb4sTGovA_WYXWz5Cd79gCzQwBms we delegate access control over LTI content to the LTI consumer, rather than requiring that users enroll in edX courses explicitly (and that admins keep edX and LTI provider enrollemnts consistent when students add or drop courses). This change fixes the immediate issue, which is disrupting the LTI Provider pilot currently running at Harvard.
|
👍 Looks like this will work well with the chromeless refactoring. |
7 participants
https://openedx.atlassian.net/browse/MA-722
This PR encompasses the following:
render_xblockendpoint under a feature flag for now.render_coursewaremethod in favor of a commonrender_xblockendpoint.TestRenderXBlockMixin.has_accessso all enrollment checks are consolidated inget_course_with_access, instead of the responsibility shared between multiple places. This removes the need forcan_load_forum.disable_staff_debug_infoparameter to disable rendering of Staff debug information instaff_problem_info.html.Reviewers: