Skip to content
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

feat: give data researcher view access to other learners progress page #179

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions futurex_openedx_extensions/helpers/apps.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""helpers Django application initialization"""
from __future__ import annotations

from bridgekeeper import perms
from django.apps import AppConfig


Expand Down Expand Up @@ -29,8 +30,19 @@ class HelpersConfig(AppConfig):

def ready(self) -> None:
"""Connect handlers to send notifications about discussions."""
from lms.djangoapps.course_home_api.permissions import ( # pylint: disable=import-outside-toplevel
CAN_MASQUARADE_LEARNER_PROGRESS,
)
from lms.djangoapps.courseware.rules import ( # pylint: disable=import-outside-toplevel
HasAccessRule,
HasRolesRule,
)

from futurex_openedx_extensions.helpers import \
custom_roles # pylint: disable=unused-import, import-outside-toplevel
from futurex_openedx_extensions.helpers import \
monkey_patches # pylint: disable=unused-import, import-outside-toplevel
from futurex_openedx_extensions.helpers import signals # pylint: disable=unused-import, import-outside-toplevel
if CAN_MASQUARADE_LEARNER_PROGRESS in perms:
del perms[CAN_MASQUARADE_LEARNER_PROGRESS]
Comment on lines +46 to +47
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this line is needed. I'm presuming perms is a dictionary that we can set to without deleting, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this it was giving error that permission already existed.
image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thank you both!!

perms[CAN_MASQUARADE_LEARNER_PROGRESS] = HasAccessRule('staff') | HasRolesRule('data_researcher')
4 changes: 4 additions & 0 deletions test_utils/edx_platform_mocks_shared/bridgekeeper.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
"""Mock bridgekeeper permission map"""


perms = {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
"""Mocked course api permissions"""

CAN_MASQUARADE_LEARNER_PROGRESS = 'fake_name'
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
"""Mocked bridgekeepr lms rules"""


class Rule:
"""Mocked Base class for rules."""
def check(self, user, instance=None):
"""Check if a user satisfies this rule."""
raise NotImplementedError()

def __or__(self, other):
return True


class HasRolesRule(Rule): # pylint: disable=too-few-public-methods
"""Mocked HasRolesRule"""
def __init__(self, *roles):
self.roles = roles

def check(self, user=None, instance=None):
return True


class HasAccessRule(Rule): # pylint: disable=too-few-public-methods
"""Mocked HasAccessRule"""

def __init__(self, *roles):
self.roles = roles

def check(self, user=None, instance=None):
return True