From 9a738a402bf623f31ca38caccf65f052948098f3 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Thu, 7 Nov 2024 11:26:18 +0100 Subject: [PATCH] Base group set reads on the new LMSGroupSet table --- lms/product/canvas/_plugin/grouping.py | 4 +- lms/product/plugin/course_copy.py | 8 +-- lms/services/group_set.py | 59 +++++++------------ tests/factories/__init__.py | 1 + tests/factories/lms_group_set.py | 6 ++ .../product/canvas/_plugin/grouping_test.py | 18 +++--- .../lms/product/plugin/course_copy_test.py | 44 +++++++------- tests/unit/lms/services/group_set_test.py | 53 ++++++++--------- 8 files changed, 92 insertions(+), 101 deletions(-) create mode 100644 tests/factories/lms_group_set.py diff --git a/lms/product/canvas/_plugin/grouping.py b/lms/product/canvas/_plugin/grouping.py index 9680384e51..a3a584071f 100644 --- a/lms/product/canvas/_plugin/grouping.py +++ b/lms/product/canvas/_plugin/grouping.py @@ -89,9 +89,9 @@ def get_groups_for_instructor(self, _svc, course, group_set_id): except CanvasAPIError as canvas_api_error: group_set_name = None if group_set := self._group_set_service.find_group_set( - course.application_instance, group_set_id=group_set_id + course.application_instance, lms_id=group_set_id ): - group_set_name = group_set["name"] + group_set_name = group_set.name raise GroupError( ErrorCodes.GROUP_SET_NOT_FOUND, diff --git a/lms/product/plugin/course_copy.py b/lms/product/plugin/course_copy.py index bd4090516a..f0f097afad 100644 --- a/lms/product/plugin/course_copy.py +++ b/lms/product/plugin/course_copy.py @@ -93,7 +93,7 @@ def find_matching_group_set_in_course(self, course, group_set_id): # Get the original group set from the DB group_set = self._group_set_service.find_group_set( - application_instance=course.application_instance, group_set_id=group_set_id + application_instance=course.application_instance, lms_id=group_set_id ) if not group_set: # If we haven't found it could that either: @@ -107,12 +107,12 @@ def find_matching_group_set_in_course(self, course, group_set_id): # or another user might have done it before for us. if new_group_set := self._group_set_service.find_group_set( application_instance=course.application_instance, - name=group_set["name"], + name=group_set.name, context_id=course.lms_id, ): # We found a match, store it to save the search for next time - course.set_mapped_group_set_id(group_set_id, new_group_set["id"]) - return new_group_set["id"] + course.set_mapped_group_set_id(group_set_id, new_group_set.lms_id) + return new_group_set.lms_id # No match return None diff --git a/lms/services/group_set.py b/lms/services/group_set.py index c42db94052..ce6f536281 100644 --- a/lms/services/group_set.py +++ b/lms/services/group_set.py @@ -2,16 +2,12 @@ from sqlalchemy import Text, column, func, select, union -from lms.models import Course, Grouping, LMSGroupSet +from lms.models import Course, LMSCourse, LMSCourseApplicationInstance, LMSGroupSet from lms.services.upsert import bulk_upsert class GroupSetDict(TypedDict): - """ - Group sets are a collection of student groups. - - We store them in Course.extra - """ + """Group sets are a collection of student groups.""" id: str name: str @@ -21,9 +17,9 @@ class GroupSetService: def __init__(self, db): self._db = db - def store_group_sets(self, course, group_sets: list[dict]): + def store_group_sets(self, course: Course, group_sets: list[dict]): """ - Store this course's available group sets. + Store a course's available group sets. We keep record of these for bookkeeping and as the basics to dealt with groups while doing course copy. @@ -49,44 +45,31 @@ def store_group_sets(self, course, group_sets: list[dict]): ) def find_group_set( - self, application_instance, group_set_id=None, name=None, context_id=None - ) -> GroupSetDict | None: - """ - Find the first matching group set in this course. - - Group sets are stored as part of Course.extra, this method allows to query and filter them. - - :param context_id: Match only group sets of courses with this ID - :param name: Filter courses by name - :param group_set_id: Filter courses by ID - """ - group_set = ( - func.jsonb_to_recordset(Course.extra["group_sets"]) - .table_valued( - column("id", Text), column("name", Text), joins_implicitly=True + self, application_instance, lms_id=None, name=None, context_id=None + ) -> LMSGroupSet | None: + """Find the first matching group set with the passed filters.""" + + query = ( + select(LMSGroupSet) + .join(LMSCourse) + .join(LMSCourseApplicationInstance) + .where( + LMSCourseApplicationInstance.application_instance_id + == application_instance.id ) - .render_derived(with_types=True) ) - - query = self._db.query(Grouping.id, group_set.c.id, group_set.c.name).filter( - Grouping.application_instance == application_instance - ) - if context_id: - query = query.filter(Grouping.lms_id == context_id) + query = query.where(LMSCourse.lti_context_id == context_id) - if group_set_id: - query = query.filter(group_set.c.id == group_set_id) + if lms_id: + query = query.where(LMSGroupSet.lms_id == str(lms_id)) if name: - query = query.filter( - func.lower(func.trim(group_set.c.name)) == func.lower(func.trim(name)) + query = query.where( + func.lower(func.trim(LMSGroupSet.name)) == func.lower(func.trim(name)) ) - if group_set := query.first(): - return {"id": group_set.id, "name": group_set.name} - - return None + return self._db.scalars(query).first() def factory(_context, request): diff --git a/tests/factories/__init__.py b/tests/factories/__init__.py index 9a09eea392..2462c8017d 100644 --- a/tests/factories/__init__.py +++ b/tests/factories/__init__.py @@ -37,6 +37,7 @@ LMSCourseApplicationInstance, LMSCourseMembership, ) +from tests.factories.lms_group_set import LMSGroupSet from tests.factories.lms_user import LMSUser from tests.factories.lti_registration import LTIRegistration from tests.factories.lti_role import LTIRole, LTIRoleOverride diff --git a/tests/factories/lms_group_set.py b/tests/factories/lms_group_set.py new file mode 100644 index 0000000000..2c9a3ee875 --- /dev/null +++ b/tests/factories/lms_group_set.py @@ -0,0 +1,6 @@ +from factory import make_factory +from factory.alchemy import SQLAlchemyModelFactory + +from lms import models + +LMSGroupSet = make_factory(models.LMSGroupSet, FACTORY_CLASS=SQLAlchemyModelFactory) diff --git a/tests/unit/lms/product/canvas/_plugin/grouping_test.py b/tests/unit/lms/product/canvas/_plugin/grouping_test.py index 03124bf2d7..b2796e0c1c 100644 --- a/tests/unit/lms/product/canvas/_plugin/grouping_test.py +++ b/tests/unit/lms/product/canvas/_plugin/grouping_test.py @@ -110,9 +110,9 @@ def test_get_groups_for_instructor( assert canvas_api_client.group_category_groups.return_value == api_groups def test_get_groups_for_instructor_group_set_not_found( - self, grouping_service, canvas_api_client, course, plugin, course_service + self, grouping_service, canvas_api_client, course, plugin, group_set_service ): - course_service.find_group_set.return_value = None + group_set_service.find_group_set.return_value = None canvas_api_client.group_category_groups.side_effect = CanvasAPIError() with pytest.raises(GroupError) as err: @@ -120,8 +120,8 @@ def test_get_groups_for_instructor_group_set_not_found( grouping_service, course, sentinel.group_set_id ) - course_service.find_group_set.assert_called_once_with( - group_set_id=sentinel.group_set_id + group_set_service.find_group_set.assert_called_once_with( + course.application_instance, lms_id=sentinel.group_set_id ) assert err.value.error_code == ErrorCodes.GROUP_SET_NOT_FOUND assert err.value.details == { @@ -130,9 +130,11 @@ def test_get_groups_for_instructor_group_set_not_found( } def test_get_groups_for_instructor_group_set_not_found_with_original_name( - self, grouping_service, canvas_api_client, course, plugin, course_service + self, grouping_service, canvas_api_client, course, plugin, group_set_service ): - course_service.find_group_set.return_value = {"name": sentinel.name} + group_set_service.find_group_set.return_value = factories.LMSGroupSet( + name=sentinel.name + ) canvas_api_client.group_category_groups.side_effect = CanvasAPIError() with pytest.raises(GroupError) as err: @@ -140,8 +142,8 @@ def test_get_groups_for_instructor_group_set_not_found_with_original_name( grouping_service, course, sentinel.group_set_id ) - course_service.find_group_set.assert_called_once_with( - group_set_id=sentinel.group_set_id + group_set_service.find_group_set.assert_called_once_with( + course.application_instance, lms_id=sentinel.group_set_id ) assert err.value.error_code == ErrorCodes.GROUP_SET_NOT_FOUND assert err.value.details == { diff --git a/tests/unit/lms/product/plugin/course_copy_test.py b/tests/unit/lms/product/plugin/course_copy_test.py index e153fb5b96..8d01291b41 100644 --- a/tests/unit/lms/product/plugin/course_copy_test.py +++ b/tests/unit/lms/product/plugin/course_copy_test.py @@ -122,7 +122,7 @@ def store_new_course_files(self): class TestCourseCopyGroupsHelper: @pytest.mark.parametrize("raising", [True, False]) def test_find_matching_group_in_course( - self, helper, grouping_plugin, raising, course_service, course + self, helper, grouping_plugin, raising, course, group_set_service ): if raising: grouping_plugin.get_group_sets.side_effect = ExternalRequestError @@ -132,17 +132,18 @@ def test_find_matching_group_in_course( ) grouping_plugin.get_group_sets.assert_called_once_with(course) - course_service.find_group_set.assert_any_call( - group_set_id=sentinel.group_set_id + group_set_service.find_group_set.assert_any_call( + course.application_instance, lms_id=sentinel.group_set_id ) - course_service.find_group_set.assert_called_with( - name=course_service.find_group_set.return_value["name"], + group_set_service.find_group_set.assert_called_with( + application_instance=course.application_instance, + name=group_set_service.find_group_set.return_value.name, context_id=course.lms_id, ) course.set_mapped_group_set_id( - sentinel.group_set_id, course_service.find_group_set.return_value["id"] + sentinel.group_set_id, group_set_service.find_group_set.return_value.lms_id ) - assert new_group_set_id == course_service.find_group_set.return_value["id"] + assert new_group_set_id == group_set_service.find_group_set.return_value.lms_id def test_find_matching_file_raises_OAuth2TokenError(self, helper, grouping_plugin): grouping_plugin.get_group_sets.side_effect = OAuth2TokenError @@ -153,25 +154,25 @@ def test_find_matching_file_raises_OAuth2TokenError(self, helper, grouping_plugi ) def test_find_matching_group_in_course_no_stored_group_from_original_course( - self, helper, grouping_plugin, course_service, course + self, helper, grouping_plugin, course, group_set_service ): - course_service.find_group_set.return_value = None + group_set_service.find_group_set.return_value = None new_group_set_id = helper.find_matching_group_set_in_course( course, sentinel.group_set_id ) grouping_plugin.get_group_sets.assert_called_once_with(course) - course_service.find_group_set.assert_any_call( - group_set_id=sentinel.group_set_id + group_set_service.find_group_set.assert_any_call( + course.application_instance, lms_id=sentinel.group_set_id ) assert not new_group_set_id def test_find_matching_group_in_course_no_stored_group_from_new_course( - self, helper, grouping_plugin, course_service, course + self, helper, grouping_plugin, group_set_service, course ): - course_service.find_group_set.side_effect = [ - course_service.find_group_set.return_value, + group_set_service.find_group_set.side_effect = [ + group_set_service.find_group_set.return_value, None, ] @@ -180,16 +181,17 @@ def test_find_matching_group_in_course_no_stored_group_from_new_course( ) grouping_plugin.get_group_sets.assert_called_once_with(course) - course_service.find_group_set.assert_any_call( - group_set_id=sentinel.group_set_id + group_set_service.find_group_set.assert_any_call( + course.application_instance, lms_id=sentinel.group_set_id ) - course_service.find_group_set.assert_called_with( - name=course_service.find_group_set.return_value["name"], + group_set_service.find_group_set.assert_called_with( + application_instance=course.application_instance, + name=group_set_service.find_group_set.return_value.name, context_id=course.lms_id, ) assert not new_group_set_id - @pytest.mark.usefixtures("course_service", "grouping_plugin") + @pytest.mark.usefixtures("course_service", "grouping_plugin", "group_set_service") def test_factory(self, pyramid_request): plugin = CourseCopyGroupsHelper.factory(sentinel.context, pyramid_request) @@ -202,5 +204,5 @@ def course(self): return create_autospec(Course, spec_set=True, instance=True) @pytest.fixture - def helper(self, course_service, grouping_plugin): - return CourseCopyGroupsHelper(course_service, grouping_plugin) + def helper(self, group_set_service, grouping_plugin): + return CourseCopyGroupsHelper(group_set_service, grouping_plugin) diff --git a/tests/unit/lms/services/group_set_test.py b/tests/unit/lms/services/group_set_test.py index 2daacf97a1..9d7c70fe71 100644 --- a/tests/unit/lms/services/group_set_test.py +++ b/tests/unit/lms/services/group_set_test.py @@ -20,8 +20,7 @@ class TestGroupSetService: ({"id": 1111, "name": "name"}, {"id": "1111", "name": "name"}), ], ) - def test_set_group_sets(self, group_set, expected, svc, db_session): - course = factories.Course(extra={}, lms_course=factories.LMSCourse()) + def test_set_group_sets(self, group_set, expected, svc, db_session, course): db_session.flush() svc.store_group_sets(course, [group_set]) @@ -35,15 +34,15 @@ def test_set_group_sets(self, group_set, expected, svc, db_session): == group_set["name"] ) - @pytest.mark.usefixtures("course_with_group_sets") + @pytest.mark.usefixtures("group_sets") @pytest.mark.parametrize( "params", ( - {"context_id": "context_id", "group_set_id": "ID", "name": "NAME"}, + {"context_id": "context_id", "lms_id": "ID", "name": "NAME"}, {"context_id": "context_id", "name": "NAME"}, {"context_id": "context_id", "name": "name"}, {"context_id": "context_id", "name": "NAME "}, - {"context_id": "context_id", "group_set_id": "ID"}, + {"context_id": "context_id", "lms_id": "ID"}, ), ) def test_find_group_set(self, svc, params, application_instance): @@ -51,16 +50,16 @@ def test_find_group_set(self, svc, params, application_instance): application_instance=application_instance, **params ) - assert group_set["id"] == "ID" - assert group_set["name"] == "NAME" + assert group_set.lms_id == "ID" + assert group_set.name == "NAME" - @pytest.mark.usefixtures("course_with_group_sets") + @pytest.mark.usefixtures("group_sets") @pytest.mark.parametrize( "params", ( - {"context_id": "context_id", "group_set_id": "NOID", "name": "NAME"}, - {"context_id": "context_id", "group_set_id": "ID", "name": "NONAME"}, - {"context_id": "no_context_id", "group_set_id": "ID", "name": "NAME"}, + {"context_id": "context_id", "lms_id": "NOID", "name": "NAME"}, + {"context_id": "context_id", "lms_id": "ID", "name": "NONAME"}, + {"context_id": "no_context_id", "lms_id": "ID", "name": "NAME"}, ), ) def test_find_group_set_no_matches(self, svc, params, application_instance): @@ -68,7 +67,7 @@ def test_find_group_set_no_matches(self, svc, params, application_instance): application_instance=application_instance, **params ) - @pytest.mark.usefixtures("course_with_group_sets") + @pytest.mark.usefixtures("group_sets") def test_find_group_set_returns_first_result(self, svc, application_instance): assert svc.find_group_set(application_instance) @@ -78,25 +77,23 @@ def svc(self, db_session): @pytest.fixture def course(self, application_instance): - return factories.Course( - application_instance=application_instance, lms_id="context_id" + course = factories.Course( + application_instance=application_instance, + lms_id="context_id", + lms_course=factories.LMSCourse(lti_context_id="context_id"), ) + factories.LMSCourseApplicationInstance( + lms_course=course.lms_course, application_instance=application_instance + ) + return course @pytest.fixture - def course_with_group_sets(self, course): - course.extra = { - "group_sets": [ - { - "id": "ID", - "name": "NAME", - }, - { - "id": "NOT MATCHING ID NOISE", - "name": "NOT MATCHING NAME NOISE", - }, - ] - } - return course + def group_sets(self, course, db_session): + factories.LMSGroupSet(name="NAME", lms_id="ID", lms_course=course.lms_course) + factories.LMSGroupSet( + name="NOT MATCHING", lms_id="NOT MATCHING", lms_course=course.lms_course + ) + db_session.flush() class TestFactory: