diff --git a/lms/services/assignment.py b/lms/services/assignment.py index f1291a5b09..43088a6969 100644 --- a/lms/services/assignment.py +++ b/lms/services/assignment.py @@ -1,5 +1,7 @@ from functools import lru_cache +from sqlalchemy.orm.exc import MultipleResultsFound, NoResultFound + from lms.models import Assignment @@ -10,54 +12,199 @@ def __init__(self, db): self._db = db @lru_cache(maxsize=128) - def get(self, tool_consumer_instance_guid, resource_link_id): + def get( + self, + tool_consumer_instance_guid, + resource_link_id=None, + ext_lti_assignment_id=None, + ): + """Get an assignment using using resource_link_id, ext_lti_assignment_id or both.""" + + if not any([resource_link_id, ext_lti_assignment_id]): + raise ValueError( + "Can't get an assignment without neither resource_link_id or ext_lti_assignment_id" + ) + + 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. + 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 + # ext_lti_assignment_id but no resource_link_id. + return self._get_for_canvas_assignment_config( + tool_consumer_instance_guid, ext_lti_assignment_id + ) + + # Non-Canvas assignment launches always have a resource_link_id and never + # have an ext_lti_assignment_id. + return self._get_by_resource_link_id( + 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, + resource_link_id=None, + ext_lti_assignment_id=None, + ) -> bool: + try: + 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: + return True + + def set_document_url( # pylint:disable=too-many-arguments + self, + document_url, + tool_consumer_instance_guid, + resource_link_id=None, + ext_lti_assignment_id=None, + extra=None, + ): + """ + Save the given document_url in an existing assignment or create new one. + + Set the document_url for the assignment that matches + tool_consumer_instance_guid and resource_link_id/ext_lti_assignment_id + or create a new one if none exist on the DB. + + Any existing document URL for this assignment will be overwritten. + """ + try: + assignment = self.get( + tool_consumer_instance_guid, resource_link_id, ext_lti_assignment_id + ) + 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) + + self._clear_cache() + return assignment + + def _get_by_resource_link_id(self, tool_consumer_instance_guid, resource_link_id): return ( self._db.query(Assignment) .filter_by( tool_consumer_instance_guid=tool_consumer_instance_guid, resource_link_id=resource_link_id, ) - .one_or_none() + .one() ) - def get_document_url(self, tool_consumer_instance_guid, resource_link_id): - """ - Return the matching document URL or None. + def _get_for_canvas_assignment_config( + self, tool_consumer_instance_guid, ext_lti_assignment_id + ): + return ( + self._db.query(Assignment) + .filter_by( + tool_consumer_instance_guid=tool_consumer_instance_guid, + ext_lti_assignment_id=ext_lti_assignment_id, + ) + .one() + ) - Return the document URL for the assignment with the given - tool_consumer_instance_guid and resource_link_id, or None. - """ - assignment = self.get(tool_consumer_instance_guid, resource_link_id) + @staticmethod + def _update_extra(old_extra, new_extra): + new_extra = new_extra if new_extra else {} + if not old_extra: + return new_extra - return assignment.document_url if assignment else None + 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 - def set_document_url( - self, tool_consumer_instance_guid, resource_link_id, document_url - ): + return new_extra + + def merge_canvas_assignments(self, old_assignment, new_assignment): """ - Save the given document_url. + Merge two Canvas assignments into one and return the merged assignment. - Set the document_url for the assignment that matches - tool_consumer_instance_guid and resource_link_id. Any existing document - URL for this assignment will be overwritten. + Merge old_assignment into new_assignment, delete old_assignment, and return the updated + new_assignment. """ - assignment = self.get(tool_consumer_instance_guid, resource_link_id) + new_extra = self._update_extra(old_assignment.extra, new_assignment.extra) - if assignment: - assignment.document_url = document_url - else: - self._db.add( - Assignment( - document_url=document_url, - resource_link_id=resource_link_id, - tool_consumer_instance_guid=tool_consumer_instance_guid, - ) - ) + 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() + + new_assignment.extra = new_extra + new_assignment.resource_link_id = old_assignment.resource_link_id + + self._clear_cache() + return new_assignment - # 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.) + 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): diff --git a/lms/views/basic_lti_launch.py b/lms/views/basic_lti_launch.py index 2f59c413a5..bb18a67404 100644 --- a/lms/views/basic_lti_launch.py +++ b/lms/views/basic_lti_launch.py @@ -113,12 +113,14 @@ def canvas_file_basic_lti_launch(self): # module item configuration. As a result of this we can rely on this # being around in future code. self.assignment_service.set_document_url( - self.request.params["tool_consumer_instance_guid"], - resource_link_id, # 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}", + 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) @@ -162,9 +164,9 @@ def db_configured_basic_lti_launch(self): # here we can safely assume that the document_url exists. tool_consumer_instance_guid = self.request.params["tool_consumer_instance_guid"] resource_link_id = self.request.params["resource_link_id"] - document_url = self.assignment_service.get_document_url( + document_url = self.assignment_service.get( tool_consumer_instance_guid, resource_link_id - ) + ).document_url return self.basic_lti_launch(document_url) @view_config(blackboard_copied=True) @@ -206,12 +208,12 @@ def course_copied_basic_lti_launch(self, original_resource_link_id): tool_consumer_instance_guid = self.request.params["tool_consumer_instance_guid"] resource_link_id = self.request.params["resource_link_id"] - document_url = self.assignment_service.get_document_url( + document_url = self.assignment_service.get( tool_consumer_instance_guid, original_resource_link_id - ) + ).document_url self.assignment_service.set_document_url( - tool_consumer_instance_guid, resource_link_id, document_url + document_url, tool_consumer_instance_guid, resource_link_id ) return self.basic_lti_launch(document_url) @@ -309,9 +311,9 @@ def configure_assignment(self): document_url = self.request.parsed_params["document_url"] self.assignment_service.set_document_url( + document_url, self.request.parsed_params["tool_consumer_instance_guid"], self.request.parsed_params["resource_link_id"], - document_url, ) self.context.js_config.add_document_url(document_url) diff --git a/lms/views/predicates/_lti_launch.py b/lms/views/predicates/_lti_launch.py index ea0f31a359..71f76cb147 100644 --- a/lms/views/predicates/_lti_launch.py +++ b/lms/views/predicates/_lti_launch.py @@ -35,14 +35,11 @@ def __call__(self, context, request): resource_link_id = request.params.get("resource_link_id") tool_consumer_instance_guid = request.params.get("tool_consumer_instance_guid") - has_document_url = bool( - assignment_svc.get_document_url( - tool_consumer_instance_guid, resource_link_id - ) + return ( + assignment_svc.exists(tool_consumer_instance_guid, resource_link_id) + == self.value ) - return has_document_url == self.value - class _CourseCopied(Base, ABC): """ @@ -111,10 +108,9 @@ def __call__(self, context, request): # Look for the document URL of the previous assignment that # this one was copied from. assignment_service = request.find_service(name="assignment") - previous_document_url = assignment_service.get_document_url( + is_newly_copied = assignment_service.exists( tool_consumer_instance_guid, original_resource_link_id ) - is_newly_copied = bool(previous_document_url) return is_newly_copied == self.value diff --git a/tests/unit/lms/services/assignment_test.py b/tests/unit/lms/services/assignment_test.py index 5144dff7de..05a776e35f 100644 --- a/tests/unit/lms/services/assignment_test.py +++ b/tests/unit/lms/services/assignment_test.py @@ -1,51 +1,149 @@ +import uuid from unittest.mock import sentinel import pytest +from sqlalchemy.orm.exc import NoResultFound +from lms.models import Assignment from lms.services.assignment import AssignmentService, factory from tests import factories class TestAssignmentService: - def test_get(self, svc, assignment): + def test_get_without_ids(self, svc, assignment): + with pytest.raises(ValueError): + assignment = svc.get( + assignment.tool_consumer_instance_guid, + resource_link_id=None, + ext_lti_assignment_id=None, + ) + + def test_get_raises_NoResultFound_if_theres_no_matching_assignment(self, svc): + with pytest.raises(NoResultFound): + svc.get("TOOL_CONSUMER_INSTANCE_GUID", "RESOURCE_LINK_ID") + + def test_get_raises_NoResultFound_if_theres_no_matching_assignment_with_two_ids( + self, svc + ): + with pytest.raises(NoResultFound): + svc.get( + "TOOL_CONSUMER_INSTANCE_GUID", + "RESOURCE_LINK_ID", + "EXT_LTI_ASSIGNMENT_ID", + ) + + def test_get_by_resource_link_id_only(self, svc, assignment): retrieved_assignment = svc.get( assignment.tool_consumer_instance_guid, assignment.resource_link_id ) assert retrieved_assignment == assignment - def test_get_returns_None_if_theres_no_matching_assignment(self, svc): + def test_get_by_ext_lti_id_only(self, svc, assignment_canvas_not_launched): retrieved_assignment = svc.get( - "TOOL_CONSUMER_INSTANCE_GUID", "RESOURCE_LINK_ID" + assignment_canvas_not_launched.tool_consumer_instance_guid, + ext_lti_assignment_id=assignment_canvas_not_launched.ext_lti_assignment_id, ) - assert retrieved_assignment is None + assert retrieved_assignment == assignment_canvas_not_launched - def test_get_document_url_returns_the_document_url(self, svc, assignment): - document_url = svc.get_document_url( - assignment.tool_consumer_instance_guid, assignment.resource_link_id + def test_get_by_both_ids_no_results(self, svc): + with pytest.raises(NoResultFound): + svc.get( + "tool_consumer_instance_guid", + resource_link_id="RESOURCE_LINK_ID", + ext_lti_assignment_id="ext_lti_assignment_id", + ) + + def test_get_by_both_ids_not_launched(self, svc, assignment_canvas_not_launched): + assert assignment_canvas_not_launched.resource_link_id is None + + retrieved_assignment = svc.get( + assignment_canvas_not_launched.tool_consumer_instance_guid, + resource_link_id="RESOURCE_LINK_ID", + ext_lti_assignment_id=assignment_canvas_not_launched.ext_lti_assignment_id, ) + assert retrieved_assignment == assignment_canvas_not_launched - assert document_url == assignment.document_url + def test_get_by_both_ids_launched(self, svc, assignment_canvas): + retrieved_assignment = svc.get( + assignment_canvas.tool_consumer_instance_guid, + resource_link_id=assignment_canvas.resource_link_id, + ext_lti_assignment_id=assignment_canvas.ext_lti_assignment_id, + ) + assert retrieved_assignment == assignment_canvas + + @pytest.mark.parametrize( + "old_extra,new_extra", + [ + ({}, {}), + ({"somedata": "not copied"}, {}), + ({"canvas_file_mappings": {1: 2}}, {"canvas_file_mappings": {1: 2}}), + ], + ) + def test_merge_canvas_assignments( + self, + old_extra, + new_extra, + svc, + assignment, + assignment_canvas_not_launched, + db_session, + ): + # Make both assignments belong to the same school + assignment.tool_consumer_instance_guid = ( + assignment_canvas_not_launched.tool_consumer_instance_guid + ) + assignment.extra = old_extra + db_session.flush() + assert db_session.query(Assignment).count() == 3 + 2 # noise + fixtures - def test_get_document_url_returns_None_if_theres_no_matching_document_url( - self, svc + merged_assignment = svc.merge_canvas_assignments( + assignment, assignment_canvas_not_launched + ) + + # We merged both into the newest one, the one with a non-null ext_lti_assignment_id + assert merged_assignment.id == assignment_canvas_not_launched.id + assert merged_assignment.resource_link_id == assignment.resource_link_id + assert merged_assignment.extra == new_extra + assert db_session.query(Assignment).count() == 3 + 1 # Deleted one assignment + + def test_exist_with_no_assignment(self, svc): + assert not svc.exists("TOOL_CONSUMER_INSTANCE_GUID", "RESOURCE_LINK_ID") + + def test_exists_with_assignment(self, svc, assignment): + assert svc.exists( + assignment.tool_consumer_instance_guid, assignment.resource_link_id + ) + + def test_exist_with_duplicates( + self, + svc, + db_session, + assignment, + assignment_canvas_not_launched, ): - document_url = svc.get_document_url( - "TOOL_CONSUMER_INSTANCE_GUID", "RESOURCE_LINK_ID" + # Make both assignments belong to the same school + assignment.tool_consumer_instance_guid = ( + assignment_canvas_not_launched.tool_consumer_instance_guid ) + db_session.flush() - assert document_url is None + assert svc.exists( + assignment.tool_consumer_instance_guid, + assignment.resource_link_id, + assignment_canvas_not_launched.ext_lti_assignment_id, + ) def test_set_document_url_saves_the_document_url(self, svc): svc.set_document_url( + "NEW_DOCUMENT_URL", "TOOL_CONSUMER_INSTANCE_GUID", "RESOURCE_LINK_ID", - "NEW_DOCUMENT_URL", ) assert ( - svc.get_document_url("TOOL_CONSUMER_INSTANCE_GUID", "RESOURCE_LINK_ID") + svc.get("TOOL_CONSUMER_INSTANCE_GUID", "RESOURCE_LINK_ID").document_url == "NEW_DOCUMENT_URL" ) @@ -53,17 +151,37 @@ def test_set_document_url_overwrites_an_existing_document_url( self, svc, assignment ): svc.set_document_url( + "NEW_DOCUMENT_URL", assignment.tool_consumer_instance_guid, assignment.resource_link_id, - "NEW_DOCUMENT_URL", ) assert assignment.document_url == "NEW_DOCUMENT_URL" + def test_set_document_url_with_extra(self, svc, assignment): + svc.set_document_url( + "NEW_DOCUMENT_URL", + assignment.tool_consumer_instance_guid, + assignment.resource_link_id, + extra={"some": "value"}, + ) + + assert assignment.extra["some"] == "value" + @pytest.fixture def assignment(self): return factories.Assignment() + @pytest.fixture + def assignment_canvas_not_launched(self): + return factories.Assignment( + resource_link_id=None, ext_lti_assignment_id=str(uuid.uuid4()) + ) + + @pytest.fixture + def assignment_canvas(self): + return factories.Assignment(ext_lti_assignment_id=str(uuid.uuid4())) + @pytest.fixture(autouse=True) def noise(self): factories.Assignment.create_batch(size=3) diff --git a/tests/unit/lms/views/basic_lti_launch_test.py b/tests/unit/lms/views/basic_lti_launch_test.py index 65ab3e7f31..6e385ba0a5 100644 --- a/tests/unit/lms/views/basic_lti_launch_test.py +++ b/tests/unit/lms/views/basic_lti_launch_test.py @@ -286,9 +286,11 @@ def test_it(self, context, pyramid_request, assignment_service): file_id = pyramid_request.params["file_id"] assignment_service.set_document_url.assert_called_once_with( - pyramid_request.params["tool_consumer_instance_guid"], - pyramid_request.params["resource_link_id"], document_url=f"canvas://file/course/{course_id}/file_id/{file_id}", + tool_consumer_instance_guid=pyramid_request.params[ + "tool_consumer_instance_guid" + ], + resource_link_id=pyramid_request.params["resource_link_id"], ) @@ -304,7 +306,7 @@ def test_it_adds_the_document_url( db_configured_basic_lti_launch_caller(context, pyramid_request) context.js_config.add_document_url.assert_called_once_with( - assignment_service.get_document_url.return_value + assignment_service.get.return_value.document_url ) @@ -331,7 +333,7 @@ def test_it_copies_the_assignment_settings_and_adds_the_document_url( # It gets the original assignment settings # from the DB. - assignment_service.get_document_url.assert_called_once_with( + assignment_service.get.assert_called_once_with( pyramid_request.params["tool_consumer_instance_guid"], pyramid_request.params[param_name], ) @@ -339,14 +341,14 @@ def test_it_copies_the_assignment_settings_and_adds_the_document_url( # It copies the assignment settings to the new resource_link_id in the # DB. assignment_service.set_document_url.assert_called_once_with( + assignment_service.get.return_value.document_url, pyramid_request.params["tool_consumer_instance_guid"], pyramid_request.params["resource_link_id"], - assignment_service.get_document_url.return_value, ) # It adds the document URL to the JavaScript config. context.js_config.add_document_url.assert_called_once_with( - assignment_service.get_document_url.return_value + assignment_service.get.return_value.document_url ) @@ -371,9 +373,9 @@ def test_it_saves_the_assignments_document_url_to_the_db( configure_assignment_caller(context, pyramid_request) assignment_service.set_document_url.assert_called_once_with( + pyramid_request.parsed_params["document_url"], pyramid_request.parsed_params["tool_consumer_instance_guid"], pyramid_request.parsed_params["resource_link_id"], - pyramid_request.parsed_params["document_url"], ) def test_it_enables_frontend_grading(self, context, pyramid_request): diff --git a/tests/unit/lms/views/predicates/_lti_launch_test.py b/tests/unit/lms/views/predicates/_lti_launch_test.py index 0d1889d3b9..9b15af70ed 100644 --- a/tests/unit/lms/views/predicates/_lti_launch_test.py +++ b/tests/unit/lms/views/predicates/_lti_launch_test.py @@ -21,8 +21,10 @@ class TestDBConfigured: @pytest.mark.parametrize("value,expected", [(True, True), (False, False)]) def test_when_theres_a_matching_assignment_config_in_the_db( - self, pyramid_request, value, expected + self, pyramid_request, assignment_service, value, expected ): + assignment_service.exists.return_value = True + predicate = DBConfigured(value, mock.sentinel.config) result = predicate(mock.sentinel.context, pyramid_request) @@ -33,7 +35,8 @@ def test_when_theres_a_matching_assignment_config_in_the_db( def test_when_theres_no_matching_assignment_config_in_the_db( self, assignment_service, pyramid_request, value, expected ): - assignment_service.get_document_url.return_value = None + assignment_service.exists.return_value = False + predicate = DBConfigured(value, mock.sentinel.config) result = predicate(mock.sentinel.context, pyramid_request) @@ -53,22 +56,20 @@ def test_it( resource_link_id_exists, resource_link_id_history_exists, ): - def get_document_url(_, resource_link_id): - document_url = None - + def exists(_, resource_link_id): 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. - document_url = mock.sentinel.document_url + return True if resource_link_id == pyramid_request.params[PredicateClass.param_name]: if resource_link_id_history_exists: # The database has a document_url for the resource_link_id_history. - document_url = mock.sentinel.previous_document_url + return True - return document_url + return False - assignment_service.get_document_url.side_effect = get_document_url + assignment_service.exists.side_effect = exists # If there's no Assignment for resource_link_id in the DB # but there *is* a Assignment for resource_link_id_history @@ -166,7 +167,7 @@ def test_when_assignment_is_canvas_file(self, pyramid_request, value, expected): def test_when_assignment_is_db_configured( self, pyramid_request, assignment_service, value, expected ): - assignment_service.get_document_url.return_value = mock.sentinel.document_url + assignment_service.exists.return_value = True predicate = Configured(value, mock.sentinel.config) @@ -185,7 +186,7 @@ def test_when_assignment_is_vitalsource_book( def test_when_assignment_is_unconfigured( self, assignment_service, pyramid_request, value, expected ): - assignment_service.get_document_url.return_value = None + assignment_service.exists.return_value = False predicate = Configured(value, mock.sentinel.config) @@ -203,7 +204,7 @@ def pyramid_request(self, pyramid_request): def assignment_service(self, assignment_service): # Make sure that the assignment is *not* DB-configured by default in # these tests. - assignment_service.get_document_url.return_value = None + assignment_service.exists.return_value = False return assignment_service