Base group set reads on the new LMSGroupSet table

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,

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"]
+            return new_group_set.lms_id
 
         # No match
         return None

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):

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

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)

tests/unit/lms/product/canvas/_plugin/grouping_test.py

@@ -110,18 +110,18 @@ 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:
             plugin.get_groups_for_instructor(
                 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,18 +130,20 @@ 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:
             plugin.get_groups_for_instructor(
                 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 == {

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)

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,40 +34,40 @@ 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):
         group_set = svc.find_group_set(
             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):
         assert not svc.find_group_set(
             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: