Use DB configured canvas assignments in launches

Take and store groupset for group assignments
Allow nulls for groupset param
Split canvas specific logic in .get to private helpers
Fix for canvas assignments in new API endpoint
Model changes for ext_lti_assignment_id

lms/resources/_js_config/__init__.py

@@ -87,6 +87,14 @@ def add_document_url(self, document_url):
                     _query={"document_url": document_url},
                 ),
             }
+
+        elif document_url.startswith("canvas://"):
+            self.add_canvas_file_id(
+                self._request.params["custom_canvas_course_id"],
+                self._request.params["resource_link_id"],
+                self._request.params["file_id"],
+            )
+
         else:
             self._config["viaUrl"] = via_url(self._request, document_url)
             self._add_canvas_speedgrader_settings(document_url=document_url)

lms/services/assignment.py

@@ -1,6 +1,6 @@
 from functools import lru_cache
 
-from sqlalchemy.orm.exc import NoResultFound
+from sqlalchemy.orm.exc import MultipleResultsFound, NoResultFound
 
 from lms.models import Assignment
 
@@ -28,9 +28,14 @@ def get(
         if all([resource_link_id, ext_lti_assignment_id]):
             # When launching an assignment in Canvas there are both resource_link_id and
             # ext_lti_assignment_id launch params.
-            return self._get_for_canvas_launch(
+            assignments = self.get_for_canvas_launch(
                 tool_consumer_instance_guid, resource_link_id, ext_lti_assignment_id
             )
+            if len(assignments) > 1:
+                raise MultipleResultsFound(
+                    "Multiple assignments found. Should merge_canvas_assignments have been called"
+                )
+            return assignments[0]
 
         if not resource_link_id:
             # When creating or editing an assignment Canvas launches us with an
@@ -45,6 +50,44 @@ def get(
             tool_consumer_instance_guid, resource_link_id
         )
 
+    @lru_cache(maxsize=128)
+    def get_for_canvas_launch(
+        self,
+        tool_consumer_instance_guid,
+        resource_link_id,
+        ext_lti_assignment_id,
+    ):
+        """Get a canvas assignment by both resource_link_id and ext_lti_assignment_id."""
+        assert resource_link_id and ext_lti_assignment_id
+
+        assignments = (
+            self._db.query(Assignment)
+            .filter(
+                Assignment.tool_consumer_instance_guid == tool_consumer_instance_guid,
+                (
+                    (
+                        (Assignment.resource_link_id == resource_link_id)
+                        & (Assignment.ext_lti_assignment_id == ext_lti_assignment_id)
+                    )
+                    | (
+                        (Assignment.resource_link_id == resource_link_id)
+                        & (Assignment.ext_lti_assignment_id.is_(None))
+                    )
+                    | (
+                        (Assignment.resource_link_id.is_(None))
+                        & (Assignment.ext_lti_assignment_id == ext_lti_assignment_id)
+                    )
+                ),
+            )
+            .order_by(Assignment.resource_link_id.asc())
+            .all()
+        )
+
+        if not assignments:
+            raise NoResultFound()
+
+        return assignments
+
     def exists(
         self,
         tool_consumer_instance_guid,
@@ -55,6 +98,9 @@ def exists(
             self.get(
                 tool_consumer_instance_guid, resource_link_id, ext_lti_assignment_id
             )
+        except MultipleResultsFound:
+            # Merge needed but it exists
+            return True
         except (NoResultFound, ValueError):
             return False
         else:
@@ -84,21 +130,16 @@ def set_document_url(  # pylint:disable=too-many-arguments
         except NoResultFound:
             assignment = Assignment(
                 tool_consumer_instance_guid=tool_consumer_instance_guid,
+                document_url=document_url,
                 resource_link_id=resource_link_id,
                 ext_lti_assignment_id=ext_lti_assignment_id,
             )
             self._db.add(assignment)
 
         assignment.document_url = document_url
+        assignment.extra = self._update_extra(assignment.extra, extra)
 
-        if extra:
-            assignment.extra = extra
-
-        # Clear the cache (@lru_cache) on self.get because we've changed the
-        # contents of the DB. (Python's @lru_cache doesn't have a way to remove
-        # just one key from the cache, you have to clear the entire cache.)
-        self.get.cache_clear()
-
+        self._clear_cache()
         return assignment
 
     def _get_by_resource_link_id(self, tool_consumer_instance_guid, resource_link_id):
@@ -111,72 +152,6 @@ def _get_by_resource_link_id(self, tool_consumer_instance_guid, resource_link_id
             .one()
         )
 
-    def _get_for_canvas_launch(
-        self,
-        tool_consumer_instance_guid,
-        resource_link_id,
-        ext_lti_assignment_id,
-    ):
-        """Get a canvas assignment by both resource_link_id and ext_lti_assignment_id."""
-        assert resource_link_id and ext_lti_assignment_id
-
-        assignments = (
-            self._db.query(Assignment)
-            .filter(
-                Assignment.tool_consumer_instance_guid == tool_consumer_instance_guid,
-                (
-                    (
-                        (Assignment.resource_link_id == resource_link_id)
-                        & (Assignment.ext_lti_assignment_id == ext_lti_assignment_id)
-                    )
-                    | (
-                        (Assignment.resource_link_id == resource_link_id)
-                        & (Assignment.ext_lti_assignment_id.is_(None))
-                    )
-                    | (
-                        (Assignment.resource_link_id.is_(None))
-                        & (Assignment.ext_lti_assignment_id == ext_lti_assignment_id)
-                    )
-                ),
-            )
-            .order_by(Assignment.resource_link_id.asc())
-            .all()
-        )
-
-        if not assignments:
-            raise NoResultFound()
-
-        if len(assignments) == 2:
-            # We found two assignments: one with the matching resource_link_id and no ext_lti_assignment_id
-            # and one with the matching ext_lti_assignment_id and no resource_link_id.
-            #
-            # This happens because legacy code used to store Canvas assignments in the DB with a
-            # resource_link_id and no ext_lti_assignment_id, see https://github.com/hypothesis/lms/pull/2780
-            #
-            # Whereas current code stores Canvas assignments during content-item-selection with an
-            # ext_lti_assignment_id and no resource_link_id.
-            #
-            # We need to merge the two assignments into one.
-            # order is guaranteed by the query's `order by`
-            old_assignment, new_assignment = assignments
-
-            assert not old_assignment.ext_lti_assignment_id
-            assert not new_assignment.resource_link_id
-
-            assignment = self._merge_canvas_assignments(old_assignment, new_assignment)
-        else:
-            assignment = assignments[0]
-
-        if not assignment.resource_link_id:
-            # We found an assignment with an ext_lti_assignment_id but no resource_link_id.
-            # This happens the first time a new Canvas assignment is launched: the assignment got created
-            # during content-item-selection with an ext_lti_assignment_id but no resource_link_id,
-            # and then the first time the assignment is launched we add the resource_link_id.
-            assignment.resource_link_id = resource_link_id
-
-        # We found an assignment with the matching resource_link_id and ext_lti_assignment_id.kk
-        return assignment
-
     def _get_for_canvas_assignment_config(
         self, tool_consumer_instance_guid, ext_lti_assignment_id
     ):
@@ -189,29 +164,48 @@ def _get_for_canvas_assignment_config(
             .one()
         )
 
-    def _merge_canvas_assignments(self, old_assignment, new_assignment):
+    @staticmethod
+    def _update_extra(old_extra, new_extra):
+        new_extra = new_extra if new_extra else {}
+        if not old_extra:
+            return new_extra
+
+        old_extra = dict(old_extra)
+        if old_canvas_file_mappings := old_extra.get("canvas_file_mappings"):
+            new_extra["canvas_file_mappings"] = old_canvas_file_mappings
+
+        return new_extra
+
+    def merge_canvas_assignments(self, old_assignment, new_assignment):
         """
         Merge two Canvas assignments into one and return the merged assignment.
 
         Merge old_assignment into new_assignment, delete old_assignment, and return the updated
         new_assignment.
         """
-        old_extra = dict(old_assignment.extra)
-        # Legacy Canvas assignments in the DB were only ever saved with
-        # `"canvas_file_mappings"` or nothing in the `extra` column.
-        assert list(old_extra.keys()) in ([], ["canvas_file_mappings"])
+        new_extra = self._update_extra(old_assignment.extra, new_assignment.extra)
 
         self._db.delete(old_assignment)
         # Flushing early so the `resource_link_id` constraints doesn't
         # conflict between the deleted record and new_assignment .
         self._db.flush()
 
-        if old_canvas_file_mappings := old_extra.get("canvas_file_mappings"):
-            new_assignment.extra["canvas_file_mappings"] = old_canvas_file_mappings
-
+        new_assignment.extra = new_extra
         new_assignment.resource_link_id = old_assignment.resource_link_id
+
+        self._clear_cache()
         return new_assignment
 
+    def _clear_cache(self):
+        """
+        Clear the cache (@lru_cache) because we've changed the contents of the DB.
+
+        Python's @lru_cache doesn't have a way to remove
+        just one key from the cache, you have to clear the entire cache.)
+        """
+        self.get.cache_clear()
+        self.get_for_canvas_launch.cache_clear()
+
 
 def factory(_context, request):
     return AssignmentService(db=request.db)

lms/views/api/canvas/assignments.py

@@ -44,7 +44,7 @@ def create(self):
         assignment = self.assignment_service.set_document_url(
             self.application_instance.tool_consumer_instance_guid,
             url,
-            ext_lti_assignment_id=self.request.json_body.get("ext_lti_assignment_id"),
+            ext_lti_assignment_id=params["ext_lti_assignment_id"],
             extra=extra,
         )
         return {"ext_lti_assignment_id": assignment.ext_lti_assignment_id}

lms/views/basic_lti_launch.py

@@ -94,36 +94,35 @@ def store_lti_data(self):
     @view_config(canvas_file=True)
     def canvas_file_basic_lti_launch(self):
         """
-        Respond to a Canvas file assignment launch.
+        Respond to a Canvas file assignment launch which is not db_configured.
 
         Canvas file assignment launch requests have a ``file_id`` request
         parameter, which is the Canvas instance's ID for the file. To display
         the assignment we have to use this ``file_id`` to get a download URL
         for the file from the Canvas API. We then pass that download URL to
         Via. We have to re-do this file-ID-for-download-URL exchange on every
         single launch because Canvas's download URLs are temporary.
-        """
 
+        Note that this only apply to assignments configured but not yet launched
+        after canvas assignments are also DB configured see: js_config._create_assignment_api
+        """
         course_id = self.request.params["custom_canvas_course_id"]
         file_id = self.request.params["file_id"]
         resource_link_id = self.request.params["resource_link_id"]
 
-        # Normally this would be done during `configure_assignment()` but
-        # Canvas skips that step. We are doing this to ensure that there is a
-        # module item configuration. As a result of this we can rely on this
-        # being around in future code.
+        # This URL is mostly for show. We just want to ensure that a module
+        # configuration exists. If we're going to do that we might as well
+        # make sure this URL is meaningful.
+        document_url = f"canvas://file/course/{course_id}/file_id/{file_id}"
+
+        # Newly configured canvas assignments make it to the database
+        # but for exiting ones that never got launched we keep this call to persist them.
         self.assignment_service.set_document_url(
             self.request.params["tool_consumer_instance_guid"],
-            # This URL is mostly for show. We just want to ensure that a module
-            # configuration exists. If we're going to do that we might as well
-            # make sure this URL is meaningful.
-            document_url=f"canvas://file/course/{course_id}/file_id/{file_id}",
+            document_url=document_url,
             resource_link_id=resource_link_id,
         )
-
-        self.context.js_config.add_canvas_file_id(course_id, resource_link_id, file_id)
-
-        return self.basic_lti_launch(grading_supported=False)
+        return self.basic_lti_launch(document_url=document_url, grading_supported=False)
 
     @view_config(vitalsource_book=True)
     def vitalsource_lti_launch(self):
@@ -167,6 +166,55 @@ def db_configured_basic_lti_launch(self):
         ).document_url
         return self.basic_lti_launch(document_url)
 
+    @view_config(db_configured=True, request_param="ext_lti_assignment_id")
+    def canvas_db_configured_basic_lti_launch(self):
+        """Respond to a Canvas DB-configured assignment launch."""
+        tool_consumer_instance_guid = self.request.params["tool_consumer_instance_guid"]
+        resource_link_id = self.request.params["resource_link_id"]
+        ext_lti_assignment_id = self.request.params["ext_lti_assignment_id"]
+        if self.context.is_canvas:
+            assignments = self.assignment_service.get_for_canvas_launch(
+                tool_consumer_instance_guid, resource_link_id, ext_lti_assignment_id
+            )
+
+            if len(assignments) == 2:
+                # We found two assignments: one with the matching resource_link_id and no ext_lti_assignment_id
+                # and one with the matching ext_lti_assignment_id and no resource_link_id.
+                #
+                # This happens because legacy code used to store Canvas assignments in the DB with a
+                # resource_link_id and no ext_lti_assignment_id, see https://github.com/hypothesis/lms/pull/2780
+                #
+                # Whereas current code stores Canvas assignments during content-item-selection with an
+                # ext_lti_assignment_id and no resource_link_id.
+                #
+                # We need to merge the two assignments into one.
+
+                # order is guaranteed by the query's `order by`
+                old_assignment, new_assignment = assignments
+
+                assert not old_assignment.ext_lti_assignment_id
+                assert not new_assignment.resource_link_id
+
+                assignment = self.assignment_service.merge_canvas_assignments(
+                    old_assignment, new_assignment
+                )
+            else:
+                assignment = assignments[0]
+
+            if not assignment.resource_link_id:
+                # We found an assignment with an ext_lti_assignment_id but no resource_link_id.
+                # This happens the first time a new Canvas assignment is launched: the assignment got created
+                # during content-item-selection with an ext_lti_assignment_id but no resource_link_id,
+                # and then the first time the assignment is launched we add the resource_link_id.
+                assignment.resource_link_id = resource_link_id
+
+        else:
+            assignment = self.assignment_service.get(
+                tool_consumer_instance_guid, resource_link_id
+            ).document_url
+
+        return self.basic_lti_launch(assignment.document_url)
+
     @view_config(blackboard_copied=True)
     def blackboard_copied_basic_lti_launch(self):
         """