Use DB configured canvas assignments in launches

lms/views/basic_lti_launch.py

@@ -91,16 +91,19 @@ def store_lti_data(self):
             )
 
     @view_config(canvas_file=True)
-    def canvas_file_basic_lti_launch(self):
+    def legacy_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"]
@@ -163,6 +166,48 @@ 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.
+            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

@@ -32,10 +32,15 @@ def db_configured_assignment_launch_view(context, request):
 
     def __call__(self, context, request):
         assignment_svc = request.find_service(name="assignment")
+        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, context.resource_link_id)
+            assignment_svc.exists(
+                tool_consumer_instance_guid,
+                context.resource_link_id,
+                ext_lti_assignment_id,
+            )
             == self.value
         )
 
@@ -158,6 +163,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::
@@ -167,10 +174,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):
@@ -202,8 +215,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/views/basic_lti_launch_test.py

@@ -17,25 +17,46 @@
 )
 
 
-def canvas_file_basic_lti_launch_caller(context, pyramid_request):
+def legacy_canvas_file_basic_lti_launch_caller(context, pyramid_request):
     """
-    Call BasicLTILaunchViews.canvas_file_basic_lti_launch().
+    Call BasicLTILaunchViews.legacy_canvas_file_basic_lti_launch().
 
     Set up the appropriate conditions and then call
-    BasicLTILaunchViews.canvas_file_basic_lti_launch(), and return whatever
-    BasicLTILaunchViews.canvas_file_basic_lti_launch() returns.
+    BasicLTILaunchViews.legacy_canvas_file_basic_lti_launch(), and return whatever
+    BasicLTILaunchViews.legacy_canvas_file_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
+    # legacy_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 file_id param is always present when canvas_file_basic_lti_launch()
+    # The file_id param is always present when legacy_canvas_file_basic_lti_launch()
     # is called. The canvas_file=True view predicate ensures this.
     pyramid_request.params["file_id"] = "TEST_FILE_ID"
 
     views = BasicLTILaunchViews(context, pyramid_request)
 
-    return views.canvas_file_basic_lti_launch()
+    return views.legacy_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):
@@ -220,7 +241,7 @@ def test_it_does_not_call_grading_info_upsert_if_canvas(
 
     @pytest.fixture(
         params=[
-            canvas_file_basic_lti_launch_caller,
+            legacy_canvas_file_basic_lti_launch_caller,
             db_configured_basic_lti_launch_caller,
             blackboard_copied_basic_lti_launch_caller,
             brightspace_copied_basic_lti_launch_caller,
@@ -251,7 +272,7 @@ def test_it_records_the_course_in_the_DB(
 
     @pytest.fixture(
         params=[
-            canvas_file_basic_lti_launch_caller,
+            legacy_canvas_file_basic_lti_launch_caller,
             db_configured_basic_lti_launch_caller,
             blackboard_copied_basic_lti_launch_caller,
             brightspace_copied_basic_lti_launch_caller,
@@ -274,7 +295,7 @@ def view_caller(self, request):
 @pytest.mark.usefixtures("is_canvas")
 class TestCanvasFileBasicLTILaunch:
     def test_it(self, context, pyramid_request, assignment_service):
-        canvas_file_basic_lti_launch_caller(context, pyramid_request)
+        legacy_canvas_file_basic_lti_launch_caller(context, pyramid_request)
 
         course_id = pyramid_request.params["custom_canvas_course_id"]
         file_id = pyramid_request.params["file_id"]
@@ -288,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

@@ -58,7 +58,7 @@ def test_it(
         resource_link_id_history_exists,
         context,
     ):
-        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.
@@ -107,17 +107,23 @@ 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, context, pyramid_request
+    ):
+        pyramid_request.params = {"canvas_file": 22}
         predicate = CanvasFile(value, mock.sentinel.config)
 
-        assert predicate(mock.sentinel.context, request) is expected
+        assert predicate(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, context
+    ):
+        assignment_service.exists.return_value = expected
+
         predicate = CanvasFile(value, mock.sentinel.config)
 
-        assert predicate(mock.sentinel.context, DummyRequest()) is expected
+        assert predicate(context, pyramid_request) is expected
 
 
 class TestVitalSourceBook:
@@ -129,25 +135,34 @@ 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, context):
-        request = DummyRequest(params={"url": "https://example.com"})
+    def test_when_assignment_is_url_configured(
+        self, value, expected, context, pyramid_request
+    ):
+        pyramid_request.params = {"url": "https://example.com"}
+
         predicate = URLConfigured(value, mock.sentinel.config)
 
-        assert predicate(context, request) is expected
+        assert predicate(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, context):
+    def test_when_assignment_is_not_url_configured(
+        self, value, expected, context, pyramid_request, assignment_service
+    ):
+        assignment_service.exists.return_value = expected
+
         predicate = URLConfigured(value, mock.sentinel.config)
 
-        assert predicate(context, DummyRequest()) is expected
+        assert predicate(context, pyramid_request) is expected
 
 
 class TestConfigured: