feat: give data researcher view access to other learners progress page #179
Conversation
tehreem-sadat
changed the title
tehreem-sadat
force-pushed
the
tehreem/oveeride_perm_for_progress_page_access
branch
2 times, most recently
from
8520c0f to
5f3a908
Compare
tehreem-sadat
force-pushed
the
tehreem/oveeride_perm_for_progress_page_access
branch
from
5f3a908 to
b8dc3ff
Compare
tehreem-sadat
force-pushed
the
tehreem/oveeride_perm_for_progress_page_access
branch
from
b8dc3ff to
5b07eae
Compare
| if CAN_MASQUARADE_LEARNER_PROGRESS in perms: | ||
| del perms[CAN_MASQUARADE_LEARNER_PROGRESS] |
There was a problem hiding this comment.
I don't think this line is needed. I'm presuming perms is a dictionary that we can set to without deleting, right?
There was a problem hiding this comment.
Without this it was giving error that permission already existed.
There was a problem hiding this comment.
yes, this is explicitly prevented in PermissionMap. I searched and I found that there was no technical reason behind it. It's just to prevent accidental change of the permission. So, a del statement is needed which will alarm reviewers as it did here!
There was a problem hiding this comment.
Makes sense, thank you both!!
There was a problem hiding this comment.
Thank you @tehreem-sadat , I'll keep it in request-change mode to test it along with nelc/edx-platform#51
| if CAN_MASQUARADE_LEARNER_PROGRESS in perms: | ||
| del perms[CAN_MASQUARADE_LEARNER_PROGRESS] |
There was a problem hiding this comment.
yes, this is explicitly prevented in PermissionMap. I searched and I found that there was no technical reason behind it. It's just to prevent accidental change of the permission. So, a del statement is needed which will alarm reviewers as it did here!
Related issue:: #151
Dependent edx-platform PR: nelc/edx-platform#51