Switch from predicate to explicit request_param in the view

lms/services/assignment.py

@@ -140,11 +140,7 @@ def set_document_url(  # pylint:disable=too-many-arguments
         assignment.document_url = document_url
         assignment.extra = self._update_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):
@@ -198,10 +194,19 @@ def merge_canvas_assignments(self, old_assignment, new_assignment):
         new_assignment.extra = new_extra
         new_assignment.resource_link_id = old_assignment.resource_link_id
 
-        # Clear the cache, any subsequent call to get should return the now merged assignment
-        self.get.cache_clear()
+        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/basic_lti_launch.py

@@ -144,7 +144,7 @@ def vitalsource_lti_launch(self):
         )
         return {}
 
-    @view_config(db_configured=True, is_canvas=False)
+    @view_config(db_configured=True)
     def db_configured_basic_lti_launch(self):
         """
         Respond to a DB-configured assignment launch.
@@ -171,41 +171,46 @@ def canvas_db_configured_basic_lti_launch(self):
         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
+            )
 
-        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
 
-        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
+            assignment = self.assignment_service.get(
+                tool_consumer_instance_guid, resource_link_id
+            ).document_url
 
         return self.basic_lti_launch(assignment.document_url)
 

lms/views/predicates/__init__.py

@@ -12,15 +12,13 @@
     CanvasFile,
     Configured,
     DBConfigured,
-    IsCanvas,
     URLConfigured,
     VitalSourceBook,
 )
 
 
 def includeme(config):
     for view_predicate_factory in (
-        IsCanvas,
         DBConfigured,
         BlackboardCopied,
         BrightspaceCopied,

lms/views/predicates/_lti_launch.py

@@ -61,22 +61,6 @@ def __call__(self, context, request):
         return is_canvas is self.value
 
 
-class IsCanvas(Base):
-    """Check if the current launch comes from Canvas."""
-
-    name = "is_canvas"
-
-    def __call__(self, context, request):
-        is_canvas = False
-        if request.params.get("tool_consumer_info_product_family_code") == "canvas":
-            is_canvas = True
-
-        if "custom_canvas_course_id" in request.params:
-            is_canvas = True
-
-        return is_canvas is self.value
-
-
 class _CourseCopied(Base, ABC):
     """
     Allow invoking an LTI launch view for newly course-copied assignments.

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

@@ -8,7 +8,6 @@
     CanvasFile,
     Configured,
     DBConfigured,
-    IsCanvas,
     URLConfigured,
     VitalSourceBook,
 )
@@ -20,7 +19,6 @@ def test_includeme_adds_the_view_predicates():
     includeme(config)
 
     assert config.add_view_predicate.call_args_list == [
-        mock.call("is_canvas", IsCanvas),
         mock.call("db_configured", DBConfigured),
         mock.call("blackboard_copied", BlackboardCopied),
         mock.call("brightspace_copied", BrightspaceCopied),

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

@@ -10,7 +10,6 @@
     CanvasFile,
     Configured,
     DBConfigured,
-    IsCanvas,
     URLConfigured,
     VitalSourceBook,
 )
@@ -19,27 +18,6 @@
 pytestmark = pytest.mark.usefixtures("assignment_service")
 
 
-class TestIsCanvas:
-    @pytest.mark.parametrize(
-        "params,value,expected",
-        [
-            ({}, True, False),
-            ({}, False, True),
-            ({"tool_consumer_info_product_family_code": "other"}, True, False),
-            ({"tool_consumer_info_product_family_code": "canvas"}, True, True),
-            ({"custom_canvas_course_id": "any value"}, True, True),
-        ],
-    )
-    def test_when_family_code_is(self, pyramid_request, params, value, expected):
-        pyramid_request.params = params
-
-        predicate = IsCanvas(value, mock.sentinel.config)
-
-        result = predicate(mock.sentinel.context, pyramid_request)
-
-        assert result is expected
-
-
 class TestDBConfigured:
     @pytest.mark.parametrize("value,expected", [(True, True), (False, False)])
     def test_when_theres_a_matching_assignment_config_in_the_db(