From 1b584a813ee0ffef5c8b6faa99e4e6009bcafb6a Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 21 Sep 2022 15:05:16 -0400 Subject: [PATCH 1/8] feat!: add mango (maple) support for course creation assignment receiver --- openedx_demo_plugin/receivers.py | 25 +++++++++++++++++++------ openedx_demo_plugin/settings/common.py | 2 ++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/openedx_demo_plugin/receivers.py b/openedx_demo_plugin/receivers.py index 5fc6eb8..8ee6266 100644 --- a/openedx_demo_plugin/receivers.py +++ b/openedx_demo_plugin/receivers.py @@ -9,9 +9,11 @@ from openedx_events.learning.data import UserData try: - from common.djangoapps.student.api import get_access_role_by_role_name + from cms.djangoapps.course_creators.models import CourseCreator + from organizations.api import get_organization_by_short_name except ImportError: - get_access_role_by_role_name = object + get_organization_by_short_name = object + CourseCreator = object User = get_user_model() @@ -24,10 +26,21 @@ def assign_org_course_access_to_user(user: UserData, **kwargs): OPEN_EDX_VISITOR_ORG setting if exists. If doesn't exist, then acts like a noop. """ - visitor_org = getattr(settings, "OPEN_EDX_VISITOR_ORG", None) - if not visitor_org: + visitor_org_short_name = getattr(settings, "OPEN_EDX_VISITOR_ORG", None) + if not visitor_org_short_name: return + visitor_org = get_organization_by_short_name(visitor_org_short_name) registered_user = User.objects.get(username=user.pii.username) - org_content_creator_role = get_access_role_by_role_name("org_course_creator_group") - org_content_creator_role(org=visitor_org).add_users(registered_user) + course_creator = CourseCreator( + user=registered_user, + state=CourseCreator.GRANTED, + all_organizations=False, + ) + + # In order to add course creator permissions programmatically, we must attach + # to the user just registered. So the post_add signals receivers like + # `course_creator_organizations_changed_callback` can run checks over instance.admin. + course_creator.admin = User.objects.get(username=settings.COURSE_CREATOR_ADMIN_ID) + course_creator.save() + course_creator.organizations.add(visitor_org.get("id")) diff --git a/openedx_demo_plugin/settings/common.py b/openedx_demo_plugin/settings/common.py index 6231922..9c153e2 100644 --- a/openedx_demo_plugin/settings/common.py +++ b/openedx_demo_plugin/settings/common.py @@ -24,3 +24,5 @@ def plugin_settings(settings): ] } } + if "cms.djangoapps.course_creators" not in settings.INSTALLED_APPS: + settings.INSTALLED_APPS = settings.INSTALLED_APPS + ["cms.djangoapps.course_creators"] From 92a3cab00f3d36cb4c57290ca8c86652f56040a7 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 21 Sep 2022 15:50:15 -0400 Subject: [PATCH 2/8] fix: add error management when user not found --- openedx_demo_plugin/receivers.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/openedx_demo_plugin/receivers.py b/openedx_demo_plugin/receivers.py index 8ee6266..b696602 100644 --- a/openedx_demo_plugin/receivers.py +++ b/openedx_demo_plugin/receivers.py @@ -4,6 +4,8 @@ For a detailed description on events receivers definitions please refer to the hooks official documentation. """ +import logging + from django.conf import settings from django.contrib.auth import get_user_model from openedx_events.learning.data import UserData @@ -16,6 +18,7 @@ CourseCreator = object User = get_user_model() +log = logging.getLogger(__name__) def assign_org_course_access_to_user(user: UserData, **kwargs): @@ -41,6 +44,10 @@ def assign_org_course_access_to_user(user: UserData, **kwargs): # In order to add course creator permissions programmatically, we must attach # to the user just registered. So the post_add signals receivers like # `course_creator_organizations_changed_callback` can run checks over instance.admin. - course_creator.admin = User.objects.get(username=settings.COURSE_CREATOR_ADMIN_ID) + try: + course_creator.admin = User.objects.get(username=settings.COURSE_CREATOR_ADMIN_ID) + except User.DoestNotExist: + log.exception("User with username specified in COURSE_CREATOR_ADMIN_ID does not exist.") + return course_creator.save() course_creator.organizations.add(visitor_org.get("id")) From 57a09cf044b1534fb01b78324f966210c9a446ac Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 21 Sep 2022 15:54:26 -0400 Subject: [PATCH 3/8] fix: add log management to receiver --- openedx_demo_plugin/receivers.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/openedx_demo_plugin/receivers.py b/openedx_demo_plugin/receivers.py index b696602..c7f432f 100644 --- a/openedx_demo_plugin/receivers.py +++ b/openedx_demo_plugin/receivers.py @@ -31,7 +31,9 @@ def assign_org_course_access_to_user(user: UserData, **kwargs): """ visitor_org_short_name = getattr(settings, "OPEN_EDX_VISITOR_ORG", None) if not visitor_org_short_name: + log.info("No OPEN_EDX_VISITOR_ORG provided, terminating course creation assignment.") return + visitor_org = get_organization_by_short_name(visitor_org_short_name) registered_user = User.objects.get(username=user.pii.username) @@ -41,6 +43,11 @@ def assign_org_course_access_to_user(user: UserData, **kwargs): all_organizations=False, ) + course_creator_admin_id = getattr(settings, "COURSE_CREATOR_ADMIN_ID", None) + if not course_creator_admin_id: + log.info("No COURSE_CREATOR_ADMIN_ID provided, terminating course creation assignment.") + return + # In order to add course creator permissions programmatically, we must attach # to the user just registered. So the post_add signals receivers like # `course_creator_organizations_changed_callback` can run checks over instance.admin. From 03e55a011ffe61e397219ca57745d5e55f607663 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 21 Sep 2022 17:01:21 -0400 Subject: [PATCH 4/8] test: rewrite test for latest changes --- openedx_demo_plugin/tests/test_receivers.py | 75 +++++++++++++++++++-- 1 file changed, 68 insertions(+), 7 deletions(-) diff --git a/openedx_demo_plugin/tests/test_receivers.py b/openedx_demo_plugin/tests/test_receivers.py index c32e399..b1978c1 100644 --- a/openedx_demo_plugin/tests/test_receivers.py +++ b/openedx_demo_plugin/tests/test_receivers.py @@ -5,6 +5,7 @@ """ from unittest.mock import patch +from django.conf import settings from django.contrib.auth import get_user_model from django.test import TestCase, override_settings from openedx_events.data import EventsMetadata @@ -16,6 +17,9 @@ User = get_user_model() +@override_settings( + OPEN_EDX_VISITOR_ORG="Public", COURSE_CREATOR_ADMIN_ID="dummy-staff-user", +) class RegistrationCompletedReceiverTest(TestCase): """ Tests the registration receiver assigns the correct permissions. @@ -40,25 +44,80 @@ def setUp(self): minorversion=0, ) self.registered_user = User.objects.create(username=self.user.pii.username) + self.staff = User.objects.create(username=settings.COURSE_CREATOR_ADMIN_ID, is_staff=True) - @patch("openedx_demo_plugin.receivers.get_access_role_by_role_name") - def test_receiver_called_after_event(self, get_access_role_by_role_name): + @patch("openedx_demo_plugin.receivers.CourseCreator") + @patch("openedx_demo_plugin.receivers.get_organization_by_short_name") + def test_receiver_called_after_event(self, get_organization_by_short_name, course_creator): """ Test that assign_org_course_access_to_user is called the correct information after sending STUDENT_REGISTRATION_COMPLETED event. """ - org_content_creator_role = get_access_role_by_role_name("org_course_creator_group") + get_organization_by_short_name.return_value = { + "id": 1, + } STUDENT_REGISTRATION_COMPLETED.connect(assign_org_course_access_to_user) STUDENT_REGISTRATION_COMPLETED.send_event( user=self.user, ) - org_content_creator_role(org="Public").add_users.assert_called_with(self.registered_user) + get_organization_by_short_name.assert_called_with(settings.OPEN_EDX_VISITOR_ORG) + course_creator.assert_called_with( + user=self.registered_user, + state=course_creator.GRANTED, + all_organizations=False, + ) + course_creator.organizations.add.assert_called_with(settings.OPEN_EDX_VISITOR_ORG) + + @override_settings(COURSE_CREATOR_ADMIN_ID="non-existent-user") + @patch("openedx_demo_plugin.receivers.CourseCreator") + @patch("openedx_demo_plugin.receivers.get_organization_by_short_name") + def test_unexistent_course_creator_staff(self, get_organization_by_short_name, course_creator): + """ + Test that stops when the user associated with COURSE_CREATOR_ADMIN_ID does not exist + after sending STUDENT_REGISTRATION_COMPLETED event. + """ + STUDENT_REGISTRATION_COMPLETED.connect(assign_org_course_access_to_user) + + STUDENT_REGISTRATION_COMPLETED.send_event( + user=self.user, + ) + + get_organization_by_short_name.assert_called_with(settings.OPEN_EDX_VISITOR_ORG) + course_creator.assert_called_with( + user=self.registered_user, + state=course_creator.GRANTED, + all_organizations=False, + ) + course_creator.organizations.add.assert_not_called() + + @override_settings(COURSE_CREATOR_ADMIN_ID=None) + @patch("openedx_demo_plugin.receivers.CourseCreator") + @patch("openedx_demo_plugin.receivers.get_organization_by_short_name") + def test_not_specified_course_creator_id(self, get_organization_by_short_name, course_creator): + """ + Test that stops when COURSE_CREATOR_ADMIN_ID is not specified before sending STUDENT_REGISTRATION_COMPLETED + event. + """ + STUDENT_REGISTRATION_COMPLETED.connect(assign_org_course_access_to_user) + + STUDENT_REGISTRATION_COMPLETED.send_event( + user=self.user, + ) + + get_organization_by_short_name.assert_called_with(settings.OPEN_EDX_VISITOR_ORG) + course_creator.assert_called_with( + user=self.registered_user, + state=course_creator.GRANTED, + all_organizations=False, + ) + course_creator.organizations.add.assert_not_called() @override_settings(OPEN_EDX_VISITOR_ORG=None) - @patch("openedx_demo_plugin.receivers.get_access_role_by_role_name") - def test_receiver_noop(self, get_access_role_by_role_name): + @patch("openedx_demo_plugin.receivers.CourseCreator") + @patch("openedx_demo_plugin.receivers.get_organization_by_short_name") + def test_receiver_noop(self, get_organization_by_short_name, course_creator): """ Test that when OPEN_EDX_VISITOR_ORG is not defined then the receiver acts as a noop. @@ -69,4 +128,6 @@ def test_receiver_noop(self, get_access_role_by_role_name): user=self.user, ) - get_access_role_by_role_name.return_value.assert_not_called() + get_organization_by_short_name.return_value.assert_not_called() + course_creator.assert_not_called() + course_creator.organizations.add.assert_not_called() From f5bb40704ef32d746ca44083958f66998202b20d Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 21 Sep 2022 17:04:44 -0400 Subject: [PATCH 5/8] fix: use organization id while checking for calls --- openedx_demo_plugin/tests/test_receivers.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openedx_demo_plugin/tests/test_receivers.py b/openedx_demo_plugin/tests/test_receivers.py index b1978c1..8aa2191 100644 --- a/openedx_demo_plugin/tests/test_receivers.py +++ b/openedx_demo_plugin/tests/test_receivers.py @@ -53,8 +53,9 @@ def test_receiver_called_after_event(self, get_organization_by_short_name, cours Test that assign_org_course_access_to_user is called the correct information after sending STUDENT_REGISTRATION_COMPLETED event. """ + organization_id = 1 get_organization_by_short_name.return_value = { - "id": 1, + "id": organization_id, } STUDENT_REGISTRATION_COMPLETED.connect(assign_org_course_access_to_user) @@ -68,7 +69,7 @@ def test_receiver_called_after_event(self, get_organization_by_short_name, cours state=course_creator.GRANTED, all_organizations=False, ) - course_creator.organizations.add.assert_called_with(settings.OPEN_EDX_VISITOR_ORG) + course_creator.organizations.add.assert_called_with(organization_id) @override_settings(COURSE_CREATOR_ADMIN_ID="non-existent-user") @patch("openedx_demo_plugin.receivers.CourseCreator") From 44afc9aed633900f2f7e484d677775ba988b5627 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 21 Sep 2022 17:06:25 -0400 Subject: [PATCH 6/8] fix: add missing return_value for tests --- openedx_demo_plugin/tests/test_receivers.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/openedx_demo_plugin/tests/test_receivers.py b/openedx_demo_plugin/tests/test_receivers.py index 8aa2191..56a4273 100644 --- a/openedx_demo_plugin/tests/test_receivers.py +++ b/openedx_demo_plugin/tests/test_receivers.py @@ -69,7 +69,7 @@ def test_receiver_called_after_event(self, get_organization_by_short_name, cours state=course_creator.GRANTED, all_organizations=False, ) - course_creator.organizations.add.assert_called_with(organization_id) + course_creator.return_value.organizations.add.assert_called_with(organization_id) @override_settings(COURSE_CREATOR_ADMIN_ID="non-existent-user") @patch("openedx_demo_plugin.receivers.CourseCreator") @@ -91,7 +91,7 @@ def test_unexistent_course_creator_staff(self, get_organization_by_short_name, c state=course_creator.GRANTED, all_organizations=False, ) - course_creator.organizations.add.assert_not_called() + course_creator.return_value.organizations.add.assert_not_called() @override_settings(COURSE_CREATOR_ADMIN_ID=None) @patch("openedx_demo_plugin.receivers.CourseCreator") @@ -113,7 +113,7 @@ def test_not_specified_course_creator_id(self, get_organization_by_short_name, c state=course_creator.GRANTED, all_organizations=False, ) - course_creator.organizations.add.assert_not_called() + course_creator.return_value.organizations.add.assert_not_called() @override_settings(OPEN_EDX_VISITOR_ORG=None) @patch("openedx_demo_plugin.receivers.CourseCreator") @@ -131,4 +131,4 @@ def test_receiver_noop(self, get_organization_by_short_name, course_creator): get_organization_by_short_name.return_value.assert_not_called() course_creator.assert_not_called() - course_creator.organizations.add.assert_not_called() + course_creator.return_value.organizations.add.assert_not_called() From c3c03c47dd4441c2e854bca4d9738f1c4c5efc30 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 21 Sep 2022 20:11:04 -0400 Subject: [PATCH 7/8] fix: use variable instead of getting from settings --- openedx_demo_plugin/receivers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_demo_plugin/receivers.py b/openedx_demo_plugin/receivers.py index c7f432f..469e540 100644 --- a/openedx_demo_plugin/receivers.py +++ b/openedx_demo_plugin/receivers.py @@ -52,7 +52,7 @@ def assign_org_course_access_to_user(user: UserData, **kwargs): # to the user just registered. So the post_add signals receivers like # `course_creator_organizations_changed_callback` can run checks over instance.admin. try: - course_creator.admin = User.objects.get(username=settings.COURSE_CREATOR_ADMIN_ID) + course_creator.admin = User.objects.get(username=course_creator_admin_id) except User.DoestNotExist: log.exception("User with username specified in COURSE_CREATOR_ADMIN_ID does not exist.") return From 533e6da476057b95510cf25714c8281036c1d26d Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 21 Sep 2022 20:13:35 -0400 Subject: [PATCH 8/8] refactor: reorganize code to not check unnecessary stuff --- openedx_demo_plugin/receivers.py | 11 +++++------ openedx_demo_plugin/tests/test_receivers.py | 8 ++------ 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/openedx_demo_plugin/receivers.py b/openedx_demo_plugin/receivers.py index 469e540..c246078 100644 --- a/openedx_demo_plugin/receivers.py +++ b/openedx_demo_plugin/receivers.py @@ -34,7 +34,10 @@ def assign_org_course_access_to_user(user: UserData, **kwargs): log.info("No OPEN_EDX_VISITOR_ORG provided, terminating course creation assignment.") return - visitor_org = get_organization_by_short_name(visitor_org_short_name) + course_creator_admin_id = getattr(settings, "COURSE_CREATOR_ADMIN_ID", None) + if not course_creator_admin_id: + log.info("No COURSE_CREATOR_ADMIN_ID provided, terminating course creation assignment.") + return registered_user = User.objects.get(username=user.pii.username) course_creator = CourseCreator( @@ -43,14 +46,10 @@ def assign_org_course_access_to_user(user: UserData, **kwargs): all_organizations=False, ) - course_creator_admin_id = getattr(settings, "COURSE_CREATOR_ADMIN_ID", None) - if not course_creator_admin_id: - log.info("No COURSE_CREATOR_ADMIN_ID provided, terminating course creation assignment.") - return - # In order to add course creator permissions programmatically, we must attach # to the user just registered. So the post_add signals receivers like # `course_creator_organizations_changed_callback` can run checks over instance.admin. + visitor_org = get_organization_by_short_name(visitor_org_short_name) try: course_creator.admin = User.objects.get(username=course_creator_admin_id) except User.DoestNotExist: diff --git a/openedx_demo_plugin/tests/test_receivers.py b/openedx_demo_plugin/tests/test_receivers.py index 56a4273..b5cb33d 100644 --- a/openedx_demo_plugin/tests/test_receivers.py +++ b/openedx_demo_plugin/tests/test_receivers.py @@ -107,12 +107,8 @@ def test_not_specified_course_creator_id(self, get_organization_by_short_name, c user=self.user, ) - get_organization_by_short_name.assert_called_with(settings.OPEN_EDX_VISITOR_ORG) - course_creator.assert_called_with( - user=self.registered_user, - state=course_creator.GRANTED, - all_organizations=False, - ) + get_organization_by_short_name.assert_not_called() + course_creator.assert_not_called() course_creator.return_value.organizations.add.assert_not_called() @override_settings(OPEN_EDX_VISITOR_ORG=None)