Skip to content

Commit

Permalink
Store the Name and Roles service URL in LMSCourse
Browse files Browse the repository at this point in the history 
Store LTI1.3 context_memberships_url in LMSCourse when available.

This will allow us to comunicate with the service outside the launch
context.

See: https://www.imsglobal.org/spec/lti-nrps/v2p0#lti-1-3-integration
  • Loading branch information
@marcospri
marcospri committed Aug 27, 2024
1 parent 737f031 commit 8f8de6c
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 72 deletions.
28 changes: 17 additions & 11 deletions lms/services/course.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
LMSCourseApplicationInstance,
LMSCourseMembership,
LMSUser,
LTIParams,
LTIRole,
Organization,
RoleScope,
Expand Down Expand Up @@ -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,
)
Expand Down Expand Up @@ -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(
[
{
Expand All @@ -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,
Expand All @@ -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,
Expand Down
104 changes: 43 additions & 61 deletions tests/unit/lms/services/course_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
LMSCourse,
LMSCourseApplicationInstance,
LMSCourseMembership,
LTIParams,
RoleScope,
RoleType,
)
Expand Down Expand Up @@ -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,
)
Expand All @@ -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

Expand All @@ -168,34 +142,30 @@ 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,
authority_provided_id="authority_original_context_id",
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,
)
Expand All @@ -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,
}
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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(
[
Expand Down Expand Up @@ -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):
Expand Down

0 comments on commit 8f8de6c

Please sign in to comment.