Support for masquerade #17
Conversation
Ivanca
commented
Oct 25, 2018
•
Ivanca
commented
|
@felipemontoya review needed. |
requirements.txt
Outdated
| web-fragments==0.2.2 | ||
| lxml==3.8.0 |
There was a problem hiding this comment.
this dependency is no longer needed
seb_openedx/middleware.py
Outdated
| active_comps = get_enabled_permission_classes() | ||
| for permission in active_comps: | ||
| if masquerade and masquerade.role != 'staff' and permission == AlwaysAllowStaff: |
There was a problem hiding this comment.
This is strange, we have all the logic to prevent doing things like permission == AlwaysAllowStaff and here we are.
I propose the following, lets add an optional kwarg to the permission().check() signature, so that we can send the masquerade object. Then we can conduct this logic in the AlwaysAllowStaff directly
seb_openedx/middleware.py
Outdated
| if access_denied: | ||
| html = six.text_type(self.handle_access_denied(request, view_func, view_args, view_kwargs, course_key)) | ||
| if masquerade: | ||
| html += masquerade_html |
There was a problem hiding this comment.
Why are we concatenating here?
Would it be possible to sent the rendering context to the handle_access_denied function (or have it in self.context) and only add this masquerade_html so that the renderer can put it in the correct place in the html?
There was a problem hiding this comment.
@felipemontoya The renderer at SebCoursewareIndex.as_view() is outside our control so there is no clean way to 'put it in the correct place in the html', meaning the JS positioning code is needed.
What I can do is include the "preview_menu.html" on the template in order to keep the code cleaner.
seb_openedx/middleware.py
Outdated
|
|
||
| return None | ||
|
|
||
| def supports_preview_menu(self, request): | ||
| """ if request view support preview_menu or not (not fully accurate) """ |
There was a problem hiding this comment.
I would not say this is "not fully accurate", it is more like: seb-openedx only supports masquerade on this and that views.
| """ if request view support preview_menu or not (not fully accurate) """ | |
| """ Calculates if request view supports the preview_menu or not in this context. Not a perfect match to openedx's internal support""" |
seb_openedx/middleware.py
Outdated
| 'staff_access': request.user.is_staff, | ||
| 'masquerade': masquerade, | ||
| } | ||
| js_reposition = render_to_string('seb-reposition-wrapper-preview-menu.html', {}) |
There was a problem hiding this comment.
If we do the context change to the renderer, we no longer need this
seb_openedx/middleware.py
Outdated
| 'masquerade': masquerade, | ||
| } | ||
| js_reposition = render_to_string('seb-reposition-wrapper-preview-menu.html', {}) | ||
| masquerade_html = render_to_string('preview_menu.html', context) + js_reposition |
There was a problem hiding this comment.
Concatenating pieces of HTML is always strange when we have full scale renderers. This won't be necessary
seb_openedx/middleware.py
Outdated
| @@ -110,7 +144,7 @@ def courseware_error_response(self, request, *view_args, **view_kwargs): | |||
| html = Fragment() | |||
| html.add_content(render_to_string('seb-403-error-message.html', {})) | |||
| SebCoursewareIndex.set_context_fragment(html) | |||
| return SebCoursewareIndex.as_view()(request, *view_args, **view_kwargs) | |||
| return force_text(SebCoursewareIndex.as_view()(request, *view_args, **view_kwargs).content) | |||
There was a problem hiding this comment.
I believe this wont be necessary after the proposed refactor
Ivanca
force-pushed
the
icc/support-for-masquerade
branch
3 times, most recently
from
7704f22
to
66f576e
Compare
Ivanca
force-pushed
the
icc/support-for-masquerade
branch
from
66f576e
to
4399a9c
Compare
| course_key_string = view_kwargs.get('course_key_string') or view_kwargs.get('course_id') | ||
| course_key = CourseKey.from_string(course_key_string) if course_key_string else None | ||
|
|
||
| if self.get_view_path(request) == 'courseware.masquerade': |
Ivanca
force-pushed
the
icc/support-for-masquerade
branch
from
fdcfe9b
to
d7eeecb
Compare
