Only upsert LMSUser on launches

lms/security.py

@@ -363,7 +363,7 @@ def get_lti_user(request) -> LTIUser | None:
     if lti_user:
         # Make a record of the user for analytics so we can map from the
         # LTI users and the corresponding user in H
-        request.find_service(UserService).upsert_user(lti_user, request.lti_params)
+        request.find_service(UserService).upsert_user(lti_user)
 
         # Attach useful information to sentry in case we get an exception further down the line
         sentry_sdk.set_tag("application_instance_id", lti_user.application_instance_id)

lms/services/user.py

@@ -35,7 +35,7 @@ def __init__(self, db, h_authority: str):
         self._db = db
         self._h_authority = h_authority
 
-    def upsert_user(self, lti_user: LTIUser, lti_params: LTIParams) -> User:
+    def upsert_user(self, lti_user: LTIUser) -> User:
         """Store a record of having seen a particular user."""
 
         # Note! - Storing a user in our DB currently has an implication for
@@ -65,13 +65,11 @@ def upsert_user(self, lti_user: LTIUser, lti_params: LTIParams) -> User:
             # We are only storing emails for teachers now.
             user.email = lti_user.email
 
-        self._upsert_lms_user(user, lti_params)
         return user
 
-    def _upsert_lms_user(self, user: User, lti_params: LTIParams) -> LMSUser:
+    def upsert_lms_user(self, user: User, lti_params: LTIParams) -> LMSUser:
         """Upsert LMSUser based on a User object."""
         self._db.flush()  # Make sure User has hit the DB on the current transaction
-
         lms_user = bulk_upsert(
             self._db,
             LMSUser,

lms/views/lti/basic_launch.py

@@ -20,7 +20,7 @@
 from lms.models import Assignment
 from lms.product.plugin.misc import MiscPlugin
 from lms.security import Permissions
-from lms.services import LTIGradingService, VitalSourceService
+from lms.services import LTIGradingService, UserService, VitalSourceService
 from lms.services.assignment import AssignmentService
 from lms.validation import BasicLTILaunchSchema, ConfigureAssignmentSchema
 
@@ -51,6 +51,11 @@ def __init__(self, context, request) -> None:
         self.request.find_service(name="application_instance").update_from_lti_params(
             self.request.lti_user.application_instance, self.request.lti_params
         )
+        # Keep a record of every LMS user in the DB
+        # While request.user gets updated on every request we only need/want to update LMSUser on launches
+        request.find_service(UserService).upsert_lms_user(
+            request.user, request.lti_params
+        )
         self.course = self._record_course()
 
     @view_config(

lms/views/lti/deep_linking.py

@@ -49,7 +49,7 @@
 from lms.events import LTIEvent
 from lms.product.plugin.misc import MiscPlugin
 from lms.security import Permissions
-from lms.services import JWTService
+from lms.services import JWTService, UserService
 from lms.validation import DeepLinkingLTILaunchSchema
 from lms.validation._base import JSONPyramidRequestSchema
 
@@ -67,6 +67,10 @@ def deep_linking_launch(context, request):
     request.find_service(name="application_instance").update_from_lti_params(
         request.lti_user.application_instance, request.lti_params
     )
+    # Keep a record of every LMS user in the DB
+    # While request.user gets updated on every request we only need/want to update LMSUser on launches
+    request.find_service(UserService).upsert_lms_user(request.user, request.lti_params)
+
     course = request.find_service(name="course").get_from_launch(
         request.product.family, request.lti_params
     )

tests/unit/lms/security_test.py

@@ -651,9 +651,7 @@ def test_it(self, pyramid_request, user_service, lti_user, sentry_sdk):
         result = get_lti_user(pyramid_request)
 
         assert result == lti_user
-        user_service.upsert_user.assert_called_once_with(
-            result, pyramid_request.lti_params
-        )
+        user_service.upsert_user.assert_called_once_with(result)
         sentry_sdk.set_tag.assert_called_once_with(
             "application_instance_id", lti_user.application_instance_id
         )

tests/unit/lms/services/user_test.py

@@ -13,8 +13,8 @@
 
 class TestUserService:
     @pytest.mark.usefixtures("user_is_instructor")
-    def test_upsert_user(self, service, lti_user, db_session, pyramid_request):
-        user = service.upsert_user(lti_user, pyramid_request.lti_params)
+    def test_upsert_user(self, service, lti_user, db_session):
+        user = service.upsert_user(lti_user)
 
         saved_user = db_session.query(User).order_by(User.id.desc()).first()
         assert saved_user == Any.instance_of(User).with_attrs(
@@ -31,43 +31,50 @@ def test_upsert_user(self, service, lti_user, db_session, pyramid_request):
             }
         )
         assert saved_user == user
-        self.assert_lms_user(db_session, user, pyramid_request.lti_params)
 
     @pytest.mark.usefixtures("user_is_learner")
     def test_upsert_user_doesnt_save_email_for_students(
-        self, service, lti_user, db_session, pyramid_request
+        self, service, lti_user, db_session
     ):
-        service.upsert_user(lti_user, pyramid_request.lti_params)
+        service.upsert_user(lti_user)
 
         saved_user = db_session.query(User).order_by(User.id.desc()).first()
         assert saved_user.roles == lti_user.roles
         assert not saved_user.email
-        self.assert_lms_user(db_session, saved_user, pyramid_request.lti_params)
 
     @pytest.mark.usefixtures("user")
-    def test_upsert_user_with_an_existing_user(
-        self, service, lti_user, db_session, pyramid_request
-    ):
-        user = service.upsert_user(lti_user, pyramid_request.lti_params)
+    def test_upsert_user_with_an_existing_user(self, service, lti_user, db_session):
+        user = service.upsert_user(lti_user)
 
         saved_user = db_session.get(User, user.id)
         assert saved_user.id == user.id
         assert saved_user.roles == lti_user.roles
         assert user == saved_user
-        self.assert_lms_user(db_session, saved_user, pyramid_request.lti_params)
 
     @pytest.mark.usefixtures("user")
     def test_upsert_user_doesnt_save_email_for_existing_students(
-        self, service, lti_user, db_session, pyramid_request
+        self, service, lti_user, db_session
     ):
         lti_user.roles = "Student"
 
-        service.upsert_user(lti_user, pyramid_request.lti_params)
+        service.upsert_user(lti_user)
 
         saved_user = db_session.query(User).order_by(User.id.desc()).first()
         assert saved_user.roles == lti_user.roles
         assert not saved_user.email
-        self.assert_lms_user(db_session, saved_user, pyramid_request.lti_params)
+
+    def test_upsert_lms_user(self, service, lti_user, pyramid_request, db_session):
+        user = service.upsert_user(lti_user)
+        lms_user = service.upsert_lms_user(user, pyramid_request.lti_params)
+
+        lms_user = db_session.scalars(
+            select(LMSUser).where(LMSUser.h_userid == user.h_userid)
+        ).one()
+
+        assert lms_user.display_name == user.display_name
+        assert lms_user.email == user.email
+        assert lms_user.updated == user.updated
+        assert lms_user.lti_v13_user_id == pyramid_request.lti_params.v13.get("sub")
 
     def test_get(self, user, service):
         db_user = service.get(user.application_instance, user.user_id)
@@ -176,18 +183,6 @@ def test_get_users_by_instructor_h_userid(
 
         assert db_session.scalars(query).all() == [student_in_assigment]
 
-    def assert_lms_user(self, db_session, user, lti_params):
-        """Assert the corresponding LMSUser to user exists in the DB with the same attributes."""
-
-        lms_user = db_session.scalars(
-            select(LMSUser).where(LMSUser.h_userid == user.h_userid)
-        ).one()
-
-        assert lms_user.display_name == user.display_name
-        assert lms_user.email == user.email
-        assert lms_user.updated == user.updated
-        assert lms_user.lti_v13_user_id == lti_params.v13.get("sub")
-
     @pytest.fixture
     def course(self, application_instance, db_session):
         course = factories.Course(application_instance=application_instance)

tests/unit/lms/views/lti/basic_launch_test.py

@@ -20,6 +20,7 @@
     "lti_role_service",
     "grouping_service",
     "misc_plugin",
+    "user_service",
 )
 class TestBasicLaunchViews:
     def test___init___(

tests/unit/lms/views/lti/deep_linking_test.py

@@ -18,6 +18,7 @@
 from tests import factories
 
 
+@pytest.mark.usefixtures("user_service")
 class TestDeepLinkingFieldsRequestSchema:
     @pytest.mark.parametrize(
         "payload",
@@ -81,6 +82,7 @@ def test_it(
         application_instance_service,
         course_service,
         misc_plugin,
+        user_service,
     ):
         deep_linking_launch(context, pyramid_request)
 
@@ -90,6 +92,9 @@ def test_it(
         course_service.get_from_launch.assert_called_once_with(
             pyramid_request.product.family, pyramid_request.lti_params
         )
+        user_service.upsert_lms_user.assert_called_once_with(
+            pyramid_request.user, pyramid_request.lti_params
+        )
         lti_h_service.sync.assert_called_once_with(
             [course_service.get_from_launch.return_value], pyramid_request.params
         )