diff --git a/lms/services/course.py b/lms/services/course.py index db53e69881..55b69e8770 100644 --- a/lms/services/course.py +++ b/lms/services/course.py @@ -17,6 +17,7 @@ LMSCourseApplicationInstance, LMSCourseMembership, LMSUser, + LTIParams, LTIRole, Organization, RoleScope, @@ -75,8 +76,7 @@ def get_from_launch(self, product_family: Family, lti_params) -> Course: historical_course = self._get_copied_from_course(lti_params) return self.upsert_course( - context_id=lti_params["context_id"], - name=lti_params["context_title"], + lti_params=lti_params, extra=extra, copied_from=historical_course, ) @@ -253,24 +253,25 @@ def get_by_context_id(self, context_id, raise_on_missing=False) -> Course | None return query.one_or_none() - def upsert_course( # noqa: PLR0913 + def upsert_course( self, - context_id, - name, - extra, + lti_params: LTIParams, + extra: dict, settings=None, copied_from: Grouping | None = None, ) -> Course: """ Create or update a course based on the provided values. - :param context_id: The course id from LTI params - :param name: The name of the course + :param lti_params: Parameters from the LTI launch :param extra: Additional LMS specific values :param settings: A dict of settings for the course :param copied_from: A reference to the course this one was copied from """ + context_id = lti_params["context_id"] + name = lti_params["context_title"] + course = self._grouping_service.upsert_groupings( [ { @@ -284,13 +285,17 @@ def upsert_course( # noqa: PLR0913 copied_from=copied_from, )[0] - self._upsert_lms_course(course) + self._upsert_lms_course(course, lti_params) return course - def _upsert_lms_course(self, course: Course) -> LMSCourse: + def _upsert_lms_course(self, course: Course, lti_params: LTIParams) -> LMSCourse: """Upsert LMSCourse based on a Course object.""" self._db.flush() # Make sure Course has hit the DB on the current transaction + lti_context_membership_url = lti_params.v13.get( + "https://purl.imsglobal.org/spec/lti-nrps/claim/namesroleservice", {} + ).get("context_memberships_url") + lms_course = bulk_upsert( self._db, LMSCourse, @@ -300,10 +305,11 @@ def _upsert_lms_course(self, course: Course) -> LMSCourse: "lti_context_id": course.lms_id, "h_authority_provided_id": course.authority_provided_id, "name": course.lms_name, + "lti_context_memberships_url": lti_context_membership_url, } ], index_elements=["h_authority_provided_id"], - update_columns=["updated", "name"], + update_columns=["updated", "name", "lti_context_memberships_url"], ).one() bulk_upsert( self._db, diff --git a/tests/unit/lms/services/course_test.py b/tests/unit/lms/services/course_test.py index 6a4500c191..d39a616ce1 100644 --- a/tests/unit/lms/services/course_test.py +++ b/tests/unit/lms/services/course_test.py @@ -12,6 +12,7 @@ LMSCourse, LMSCourseApplicationInstance, LMSCourseMembership, + LTIParams, RoleScope, RoleType, ) @@ -61,71 +62,49 @@ def test_get_by_context_id_with_no_match_and_raise_on_missing(self, svc): svc.get_by_context_id("NO MATCH", raise_on_missing=True) def test_get_from_launch_when_existing( - self, svc, get_by_context_id, upsert_course, product + self, svc, get_by_context_id, upsert_course, product, lti_params ): course = get_by_context_id.return_value = factories.Course( extra={"existing": "extra"} ) - course = svc.get_from_launch( - product, - lti_params={ - "context_id": sentinel.context_id, - "context_title": sentinel.context_title, - }, - ) + course = svc.get_from_launch(product, lti_params=lti_params) get_by_context_id.assert_called_once_with( sentinel.context_id, ) upsert_course.assert_called_once_with( - context_id=sentinel.context_id, - name=sentinel.context_title, + lti_params=lti_params, extra={"existing": "extra"}, copied_from=None, ) assert course == upsert_course.return_value def test_get_from_launch_when_new( - self, svc, get_by_context_id, upsert_course, product + self, svc, get_by_context_id, upsert_course, product, lti_params ): get_by_context_id.return_value = None - course = svc.get_from_launch( - product, - lti_params={ - "context_id": sentinel.context_id, - "context_title": sentinel.context_title, - }, - ) + course = svc.get_from_launch(product, lti_params=lti_params) get_by_context_id.assert_called_once_with(sentinel.context_id) upsert_course.assert_called_once_with( - context_id=sentinel.context_id, - name=sentinel.context_title, + lti_params=lti_params, extra={}, copied_from=None, ) assert course == upsert_course.return_value def test_get_from_launch_when_new_and_canvas( - self, svc, upsert_course, get_by_context_id + self, svc, upsert_course, get_by_context_id, lti_params ): get_by_context_id.return_value = None - course = svc.get_from_launch( - Product.Family.CANVAS, - lti_params={ - "context_id": sentinel.context_id, - "context_title": sentinel.context_title, - "custom_canvas_course_id": sentinel.canvas_id, - }, - ) + course = svc.get_from_launch(Product.Family.CANVAS, lti_params=lti_params) get_by_context_id.assert_called_once_with(sentinel.context_id) upsert_course.assert_called_once_with( - context_id=sentinel.context_id, - name=sentinel.context_title, + lti_params=lti_params, extra={"canvas": {"custom_canvas_course_id": sentinel.canvas_id}}, copied_from=None, ) @@ -138,21 +117,16 @@ def test_get_from_launch_when_new_and_historical_course_doesnt_exists( "authority_new_context_id", "authority_original_context_id", ] + lti_params = { + "context_id": "new_context_id", + "context_title": sentinel.context_title, + "custom_Context.id.history": "original_context_id", + } - course = svc.get_from_launch( - product, - lti_params={ - "context_id": "new_context_id", - "context_title": sentinel.context_title, - "custom_Context.id.history": "original_context_id", - }, - ) + course = svc.get_from_launch(product, lti_params=lti_params) upsert_course.assert_called_once_with( - context_id="new_context_id", - name=sentinel.context_title, - extra={}, - copied_from=None, + lti_params=lti_params, extra={}, copied_from=None ) assert course == upsert_course.return_value @@ -168,6 +142,11 @@ def test_get_from_launch_when_new_and_historical_course_exists( "authority_new_context_id", "authority_original_context_id", ] + lti_params = { + "context_id": "new_context_id", + "context_title": sentinel.context_title, + "custom_Context.id.history": "original_context_id", + } historical_course = factories.Course( application_instance=application_instance, @@ -175,27 +154,18 @@ def test_get_from_launch_when_new_and_historical_course_exists( lms_id="original_context_id", ) - course = svc.get_from_launch( - product, - lti_params={ - "context_id": "new_context_id", - "context_title": sentinel.context_title, - "custom_Context.id.history": "original_context_id", - }, - ) + course = svc.get_from_launch(product, lti_params=lti_params) upsert_course.assert_called_once_with( - context_id="new_context_id", - name=sentinel.context_title, - extra={}, - copied_from=historical_course, + lti_params=lti_params, extra={}, copied_from=historical_course ) assert course == upsert_course.return_value - def test_upsert_course(self, svc, grouping_service, bulk_upsert, db_session): + def test_upsert_course( + self, svc, grouping_service, bulk_upsert, db_session, lti_params + ): course = svc.upsert_course( - context_id=sentinel.context_id, - name=sentinel.name, + lti_params=lti_params, extra=sentinel.extra, settings=sentinel.settings, ) @@ -204,7 +174,7 @@ def test_upsert_course(self, svc, grouping_service, bulk_upsert, db_session): [ { "lms_id": sentinel.context_id, - "lms_name": sentinel.name, + "lms_name": sentinel.context_title, "extra": sentinel.extra, "settings": sentinel.settings, } @@ -225,10 +195,11 @@ def test_upsert_course(self, svc, grouping_service, bulk_upsert, db_session): "lti_context_id": course.lms_id, "h_authority_provided_id": course.authority_provided_id, "name": course.lms_name, + "lti_context_memberships_url": None, } ], index_elements=["h_authority_provided_id"], - update_columns=["updated", "name"], + update_columns=["updated", "name", "lti_context_memberships_url"], ), call().one(), call( @@ -255,6 +226,7 @@ def test_upsert_course_sets_canvas_sections_enabled_based_on_legacy_rows( canvas_sections_enabled, grouping_service, bulk_upsert, + lti_params, ): db_session.add( CourseGroupsExportedFromH( @@ -267,7 +239,7 @@ def test_upsert_course_sets_canvas_sections_enabled_based_on_legacy_rows( "canvas", "sections_enabled", canvas_sections_enabled ) - svc.upsert_course("context_id", "new course name", {}) + svc.upsert_course(lti_params=lti_params, extra={}) grouping_service.upsert_groupings.assert_called_once_with( [ @@ -551,6 +523,16 @@ def upsert_course(self, svc): def bulk_upsert(self, patch): return patch("lms.services.course.bulk_upsert") + @pytest.fixture + def lti_params(self): + return LTIParams( + { + "context_id": sentinel.context_id, + "context_title": sentinel.context_title, + "custom_canvas_course_id": sentinel.canvas_id, + } + ) + class TestCourseServiceFactory: def test_it(self, pyramid_request, grouping_service, CourseService):