Use DB configured canvas assignments in launches

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/views/basic_lti_launch.py

@@ -45,7 +45,7 @@ def __init__(self, context, request):
             self.request.params
         )
 
-    def basic_lti_launch(self, document_url=None, grading_supported=True):
+    def basic_lti_launch(self, document_url, grading_supported=True):
         """Do a basic LTI launch with the given document_url."""
         self.sync_lti_data_to_h()
         self.store_lti_data()
@@ -54,8 +54,7 @@ def basic_lti_launch(self, document_url=None, grading_supported=True):
         if grading_supported:
             self.context.js_config.maybe_enable_grading()
 
-        if document_url is not None:
-            self.context.js_config.add_document_url(document_url)
+        self.context.js_config.add_document_url(document_url)
 
         return {}
 
@@ -94,38 +93,40 @@ 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(
             # 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,
             tool_consumer_instance_guid=self.request.params[
                 "tool_consumer_instance_guid"
             ],
             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):
@@ -169,6 +170,50 @@ 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"]
+
+        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
+
+        return self.basic_lti_launch(assignment.document_url)
+
     @view_config(blackboard_copied=True)
     def blackboard_copied_basic_lti_launch(self):
         """

lms/views/predicates/_lti_launch.py

@@ -33,10 +33,13 @@ def db_configured_assignment_launch_view(context, request):
     def __call__(self, context, request):
         assignment_svc = request.find_service(name="assignment")
         resource_link_id = request.params.get("resource_link_id")
+        ext_lti_assignment_id = request.params.get("ext_lti_assignment_id")
         tool_consumer_instance_guid = request.params.get("tool_consumer_instance_guid")
 
         return (
-            assignment_svc.exists(tool_consumer_instance_guid, resource_link_id)
+            assignment_svc.exists(
+                tool_consumer_instance_guid, resource_link_id, ext_lti_assignment_id
+            )
             == self.value
         )
 
@@ -159,6 +162,8 @@ class CanvasFile(Base):
     """
     Allow invoking an LTI launch view only for Canvas file assignments.
 
+    Newer Canvas file assignment are already present in the DB so they behave like DB Configured ones.
+
     Pass ``canvas_file=True`` to a view config to allow invoking the view only
     for Canvas file assignments, or ``canvas_file=False`` to allow it only for
     other types of assignment. For example::
@@ -168,10 +173,16 @@ def canvas_file_assignment_launch_view(context, request):
             ...
     """
 
+    def __init__(self, value, config):
+        super().__init__(value, config)
+        self.db_configured = DBConfigured(True, config)
+
     name = "canvas_file"
 
     def __call__(self, context, request):
-        return ("canvas_file" in request.params) == self.value
+        return ("canvas_file" in request.params) == self.value and self.db_configured(
+            context, request
+        ) != self.value
 
 
 class VitalSourceBook(Base):
@@ -203,8 +214,14 @@ def url_configured_assignment_launch_view(context, request):
 
     name = "url_configured"
 
+    def __init__(self, value, config):
+        super().__init__(value, config)
+        self.db_configured = DBConfigured(True, config)
+
     def __call__(self, context, request):
-        return ("url" in request.params) == self.value
+        return ("url" in request.params) == self.value and self.db_configured(
+            context, request
+        ) != self.value
 
 
 class Configured(Base):

tests/unit/lms/resources/_js_config/__init___test.py

@@ -194,6 +194,22 @@ def test_it_adds_the_viaUrl_api_config_for_Blackboard_documents(self, js_config)
             "path": "/api/blackboard/courses/test_course_id/via_url?document_url=blackboard%3A%2F%2Fcontent-resource%2Fxyz123",
         }
 
+    def test_it_adds_the_viaUrl_api_config_for_Canvas_documents(
+        self, js_config, pyramid_request
+    ):
+        course_id, file_id = "125", "100"
+        pyramid_request.params["custom_canvas_course_id"] = course_id
+        pyramid_request.params["file_id"] = file_id
+
+        js_config.add_document_url(
+            f"canvas://file/course/{course_id}/file_id/{file_id}"
+        )
+
+        assert js_config.asdict()["api"]["viaUrl"] == {
+            "authUrl": "http://example.com/api/canvas/oauth/authorize",
+            "path": "/api/canvas/courses/125/assignments/TEST_RESOURCE_LINK_ID/files/100/via_url",
+        }
+
     def test_it_sets_the_document_url(self, js_config, submission_params):
         js_config.add_document_url("example_document_url")
 

tests/unit/lms/views/basic_lti_launch_test.py

@@ -38,6 +38,27 @@ def canvas_file_basic_lti_launch_caller(context, pyramid_request):
     return views.canvas_file_basic_lti_launch()
 
 
+def canvas_db_configured_basic_lti_launch_caller(context, pyramid_request):
+    """
+    Call BasicLTILaunchViews.db_configured_basic_lti_launch().
+
+    Set up the appropriate conditions and then call
+    BasicLTILaunchViews.canvas_db_configured_basic_lti_launch(), and return whatever
+    BasicLTILaunchViews.canvas_db_configured_basic_lti_launch() returns.
+    """
+    # The custom_canvas_course_id param is always present when
+    # canvas_file_basic_lti_launch() is called: Canvas always includes this
+    # param because we request it in our config.xml.
+    pyramid_request.params["custom_canvas_course_id"] = "TEST_COURSE_ID"
+    # The ext_lti_assignment_id param is always present for canvas launches.
+    # The `request_param="ext_lti_assignment_id"` on the view ensures this.
+    pyramid_request.params["ext_lti_assignment_id"] = "TEXT_EXT_LTI_ASSIGNMENT_ID"
+
+    views = BasicLTILaunchViews(context, pyramid_request)
+
+    return views.canvas_db_configured_basic_lti_launch()
+
+
 def db_configured_basic_lti_launch_caller(context, pyramid_request):
     """
     Call BasicLTILaunchViews.db_configured_basic_lti_launch().
@@ -276,12 +297,6 @@ class TestCanvasFileBasicLTILaunch:
     def test_it(self, context, pyramid_request, assignment_service):
         canvas_file_basic_lti_launch_caller(context, pyramid_request)
 
-        context.js_config.add_canvas_file_id.assert_called_once_with(
-            pyramid_request.params["custom_canvas_course_id"],
-            pyramid_request.params["resource_link_id"],
-            pyramid_request.params["file_id"],
-        )
-
         course_id = pyramid_request.params["custom_canvas_course_id"]
         file_id = pyramid_request.params["file_id"]
 
@@ -294,6 +309,31 @@ def test_it(self, context, pyramid_request, assignment_service):
         )
 
 
+@pytest.mark.usefixtures("is_canvas")
+class TestCanvasDBConfiguredBasicLTILaunch:
+    def test_it_sets_resource_id(self, context, pyramid_request, assignment_service):
+        assignment = factories.Assignment(resource_link_id=None)
+        assignment_service.get_for_canvas_launch.return_value = [assignment]
+
+        canvas_db_configured_basic_lti_launch_caller(context, pyramid_request)
+
+        assert assignment.resource_link_id == pyramid_request.params["resource_link_id"]
+
+    def test_it_merges_duplicates(self, context, pyramid_request, assignment_service):
+        old_assignment = factories.Assignment(ext_lti_assignment_id=None)
+        new_assignment = factories.Assignment(resource_link_id=None)
+        assignment_service.get_for_canvas_launch.return_value = [
+            old_assignment,
+            new_assignment,
+        ]
+
+        canvas_db_configured_basic_lti_launch_caller(context, pyramid_request)
+
+        assignment_service.merge_canvas_assignments.assert_called_once_with(
+            old_assignment, new_assignment
+        )
+
+
 class TestDBConfiguredBasicLTILaunch:
     def test_it_enables_frontend_grading(self, context, pyramid_request):
         db_configured_basic_lti_launch_caller(context, pyramid_request)

tests/unit/lms/views/predicates/_lti_launch_test.py

@@ -56,7 +56,7 @@ def test_it(
         resource_link_id_exists,
         resource_link_id_history_exists,
     ):
-        def exists(_, resource_link_id):
+        def exists(_, resource_link_id, _resource_link_id_exists=None):
             if resource_link_id == pyramid_request.params["resource_link_id"]:
                 if resource_link_id_exists:
                     # The database already has a document_url for the resource_link_id.
@@ -105,17 +105,21 @@ def pyramid_request(self, PredicateClass, pyramid_request):
 
 class TestCanvasFile:
     @pytest.mark.parametrize("value,expected", [(True, True), (False, False)])
-    def test_when_assignment_is_canvas_file(self, value, expected):
-        request = DummyRequest(params={"canvas_file": 22})
+    def test_when_assignment_is_canvas_file(self, value, expected, pyramid_request):
+        pyramid_request.params = {"canvas_file": 22}
         predicate = CanvasFile(value, mock.sentinel.config)
 
-        assert predicate(mock.sentinel.context, request) is expected
+        assert predicate(mock.sentinel.context, pyramid_request) is expected
 
     @pytest.mark.parametrize("value,expected", [(True, False), (False, True)])
-    def test_when_assignment_is_not_canvas_file(self, value, expected):
+    def test_when_assignment_is_not_canvas_file(
+        self, value, expected, pyramid_request, assignment_service
+    ):
+        assignment_service.exists.return_value = expected
+
         predicate = CanvasFile(value, mock.sentinel.config)
 
-        assert predicate(mock.sentinel.context, DummyRequest()) is expected
+        assert predicate(mock.sentinel.context, pyramid_request) is expected
 
 
 class TestVitalSourceBook:
@@ -127,25 +131,31 @@ def test_when_assignment_is_vitalsource_book(self, value, expected):
         assert predicate(mock.sentinel.context, request) is expected
 
     @pytest.mark.parametrize("value,expected", [(True, False), (False, True)])
-    def test_when_assignment_is_not_vitalsource_book(self, value, expected):
+    def test_when_assignment_is_not_vitalsource_book(
+        self, value, expected, pyramid_request
+    ):
         predicate = VitalSourceBook(value, mock.sentinel.config)
 
-        assert predicate(mock.sentinel.context, DummyRequest()) is expected
+        assert predicate(mock.sentinel.context, pyramid_request) is expected
 
 
 class TestURLConfigured:
     @pytest.mark.parametrize("value,expected", [(True, True), (False, False)])
-    def test_when_assignment_is_url_configured(self, value, expected):
-        request = DummyRequest(params={"url": "https://example.com"})
+    def test_when_assignment_is_url_configured(self, value, expected, pyramid_request):
+        pyramid_request.params = {"url": "https://example.com"}
         predicate = URLConfigured(value, mock.sentinel.config)
 
-        assert predicate(mock.sentinel.context, request) is expected
+        assert predicate(mock.sentinel.context, pyramid_request) is expected
 
     @pytest.mark.parametrize("value,expected", [(True, False), (False, True)])
-    def test_when_assignment_is_not_url_configured(self, value, expected):
+    def test_when_assignment_is_not_url_configured(
+        self, value, expected, pyramid_request, assignment_service
+    ):
+        assignment_service.exists.return_value = expected
+
         predicate = URLConfigured(value, mock.sentinel.config)
 
-        assert predicate(mock.sentinel.context, DummyRequest()) is expected
+        assert predicate(mock.sentinel.context, pyramid_request) is expected
 
 
 class TestConfigured: