From 15492030e074caa28e9c2758cc54498a9d760924 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Tue, 10 Sep 2024 16:51:28 +0200 Subject: [PATCH 1/3] New dahboard endpoint to sync grades of a given async --- lms/routes.py | 4 + lms/services/dashboard.py | 4 +- lms/services/lti_grading/_v13.py | 52 +++++++++-- lms/views/dashboard/api/grading.py | 86 +++++++++++++++++++ .../lms/services/lti_grading/_v13_test.py | 28 ++++++ .../lms/views/dashboard/api/grading_test.py | 72 ++++++++++++++++ 6 files changed, 236 insertions(+), 10 deletions(-) create mode 100644 lms/views/dashboard/api/grading.py create mode 100644 tests/unit/lms/views/dashboard/api/grading_test.py diff --git a/lms/routes.py b/lms/routes.py index 0d68b66f69..8692208672 100644 --- a/lms/routes.py +++ b/lms/routes.py @@ -284,6 +284,10 @@ def includeme(config): # noqa: PLR0915 config.add_route( "api.dashboard.assignment", r"/api/dashboard/assignments/{assignment_id}" ) + config.add_route( + "api.dashboard.assignments.grading.sync", + "/api/dashboard/assignments/{assignment_id}/grading/sync", + ) config.add_route("api.dashboard.assignments", "/api/dashboard/assignments") config.add_route( "api.dashboard.course.assignments.metrics", diff --git a/lms/services/dashboard.py b/lms/services/dashboard.py index 7aca589eef..f12e8915f3 100644 --- a/lms/services/dashboard.py +++ b/lms/services/dashboard.py @@ -1,8 +1,8 @@ from pyramid.httpexceptions import HTTPNotFound, HTTPUnauthorized from sqlalchemy import select +from lms.models import Assignment, Organization from lms.models.dashboard_admin import DashboardAdmin -from lms.models.organization import Organization from lms.security import Permissions from lms.services.organization import OrganizationService @@ -21,7 +21,7 @@ def __init__( self._course_service = course_service self._organization_service = organization_service - def get_request_assignment(self, request): + def get_request_assignment(self, request) -> Assignment: """Get and authorize an assignment for the given request.""" assigment_id = request.matchdict.get( "assignment_id" diff --git a/lms/services/lti_grading/_v13.py b/lms/services/lti_grading/_v13.py index 9e799c1237..9657cd8fe9 100644 --- a/lms/services/lti_grading/_v13.py +++ b/lms/services/lti_grading/_v13.py @@ -83,15 +83,38 @@ def read_result(self, grading_id) -> GradingResult: def get_score_maximum(self, resource_link_id) -> float | None: return self._read_grading_configuration(resource_link_id).get("scoreMaximum") + def sync_grade( # noqa: PLR0913 + self, + lti_registration: LTIRegistration, + lis_outcome_service_url: str, + grade_timestamp: datetime, + user_grading_id: str, + score: float, + ): + """ + Send a grade to the LMS. + + This is very similar to `record_result` but not scoped to the request context, + taking all the necessary information as parameters. + """ + payload = self._record_score_payload( + score, user_grading_id, grade_timestamp.isoformat() + ) + return self._ltia_service.request( + lti_registration, + "POST", + self._service_url(lis_outcome_service_url, "/scores"), + scopes=self.LTIA_SCOPES, + json=payload, + headers={"Content-Type": "application/vnd.ims.lis.v1.score+json"}, + ) + def record_result(self, grading_id, score=None, pre_record_hook=None, comment=None): - payload = { - "scoreMaximum": 1, - "scoreGiven": score, - "userId": grading_id, - "timestamp": datetime.now(timezone.utc).isoformat(), - "activityProgress": "Completed", - "gradingProgress": "FullyGraded", - } + payload = self._record_score_payload( + score, + grading_id, + datetime.now(timezone.utc).isoformat(), + ) if comment: payload["comment"] = self._misc_plugin.format_grading_comment_for_lms( comment @@ -183,6 +206,19 @@ def _read_grading_configuration(self, resource_link_id) -> dict: return {} + @staticmethod + def _record_score_payload( + score: float | None, user_grading_id: str, timestamp: str + ): + return { + "scoreMaximum": 1, + "scoreGiven": score, + "userId": user_grading_id, + "timestamp": timestamp, + "activityProgress": "Completed", + "gradingProgress": "FullyGraded", + } + @staticmethod def _service_url(base_url, endpoint): """ diff --git a/lms/views/dashboard/api/grading.py b/lms/views/dashboard/api/grading.py new file mode 100644 index 0000000000..34c9a335c8 --- /dev/null +++ b/lms/views/dashboard/api/grading.py @@ -0,0 +1,86 @@ +import logging +from datetime import datetime + +from marshmallow import Schema, fields +from pyramid.view import view_config +from sqlalchemy import select + +from lms.models import ( + ApplicationInstance, + Grouping, + LMSUser, + LTIRegistration, +) +from lms.security import Permissions +from lms.services import LTIAHTTPService +from lms.services.dashboard import DashboardService +from lms.services.lti_grading.factory import LTI13GradingService +from lms.validation._base import JSONPyramidRequestSchema + +LOG = logging.getLogger(__name__) + + +class _GradeSchema(Schema): + h_userid = fields.Str(required=True) + grade = fields.Float(required=True) + + +class AutoGradeSyncSchema(JSONPyramidRequestSchema): + grades = fields.List(fields.Nested(_GradeSchema), required=True) + + +class DashboardGradingViews: + def __init__(self, request) -> None: + self.request = request + self.db = request.db + self.dashboard_service: DashboardService = request.find_service( + name="dashboard" + ) + + @view_config( + route_name="api.dashboard.assignments.grading.sync", + request_method="POST", + renderer="json", + permission=Permissions.GRADE_ASSIGNMENT, + schema=AutoGradeSyncSchema, + ) + def auto_grading_sync(self): + assignment = self.dashboard_service.get_request_assignment(self.request) + assert assignment.lis_outcome_service_url, "Assignment without grading URL" + lti_registration = self.db.scalars( + select(LTIRegistration) + .join(ApplicationInstance) + .join(Grouping) + .where(Grouping.id == assignment.course_id) + .order_by(LTIRegistration.updated.desc()) + ).first() + assert lti_registration, "No LTI registraion for LTI1.3 assignment" + + sync_h_user_ids = [g["h_userid"] for g in self.request.parsed_params["grades"]] + + sync_lms_users = self.db.execute( + select(LMSUser.h_userid, LMSUser.lti_v13_user_id).where( + LMSUser.h_userid.in_(sync_h_user_ids) + ) + ).all() + # Organize the data in a dict h_userid -> lti_user_id for easier access + sync_lms_users_by_h_userid = {r[0]: r[1] for r in sync_lms_users} + + grading_service = LTI13GradingService( + ltia_service=self.request.find_service(LTIAHTTPService), + line_item_url=None, + line_item_container_url=None, + product_family=None, # type: ignore + misc_plugin=None, # type: ignore + lti_registration=None, # type: ignore + ) + # Use the same timestamp for all grades of the same sync + grade_sync_time_stamp = datetime.now() + for grade in self.request.parsed_params["grades"]: + grading_service.sync_grade( + lti_registration, + assignment.lis_outcome_service_url, + grade_sync_time_stamp, + sync_lms_users_by_h_userid[grade["h_userid"]], + grade["grade"], + ) diff --git a/tests/unit/lms/services/lti_grading/_v13_test.py b/tests/unit/lms/services/lti_grading/_v13_test.py index d4a6b5f9e8..f8a08bb3cb 100644 --- a/tests/unit/lms/services/lti_grading/_v13_test.py +++ b/tests/unit/lms/services/lti_grading/_v13_test.py @@ -1,3 +1,4 @@ +from datetime import datetime from unittest.mock import Mock, sentinel import pytest @@ -136,6 +137,33 @@ def test_get_score_maximum_no_line_item(self, svc, ltia_http_service): assert not svc.get_score_maximum(sentinel.resource_link_id) + def test_sync_grade(self, svc, ltia_http_service, lti_registration): + response = svc.sync_grade( + lti_registration, + "LIS_OUTCOME_SERVICE_URL", + datetime(2022, 4, 4), + sentinel.user_id, + sentinel.grade, + ) + + payload = { + "scoreMaximum": 1, + "scoreGiven": sentinel.grade, + "userId": sentinel.user_id, + "timestamp": "2022-04-04T00:00:00", + "activityProgress": "Completed", + "gradingProgress": "FullyGraded", + } + ltia_http_service.request.assert_called_once_with( + lti_registration, + "POST", + "LIS_OUTCOME_SERVICE_URL/scores", + scopes=svc.LTIA_SCOPES, + json=payload, + headers={"Content-Type": "application/vnd.ims.lis.v1.score+json"}, + ) + assert response == ltia_http_service.request.return_value + @freeze_time("2022-04-04") @pytest.mark.parametrize("comment", [sentinel.comment, None]) def test_record_result( diff --git a/tests/unit/lms/views/dashboard/api/grading_test.py b/tests/unit/lms/views/dashboard/api/grading_test.py new file mode 100644 index 0000000000..c49d9df8da --- /dev/null +++ b/tests/unit/lms/views/dashboard/api/grading_test.py @@ -0,0 +1,72 @@ +from datetime import datetime + +import pytest +from freezegun import freeze_time + +from lms.views.dashboard.api.grading import DashboardGradingViews +from tests import factories + + +@pytest.mark.usefixtures("dashboard_service", "ltia_http_service") +class TestDashboardGradingViews: + @freeze_time("2022-06-21 12:00:00") + def test_auto_grading_sync( + self, + lti_v13_application_instance, + pyramid_request, + views, + DashboardGradingView, + ltia_http_service, + dashboard_service, + assignment, + ): + dashboard_service.get_request_assignment.return_value = assignment + + views.auto_grading_sync() + + dashboard_service.get_request_assignment.assert_called_once_with( + pyramid_request + ) + DashboardGradingView.assert_called_once_with( + ltia_service=ltia_http_service, + line_item_url=None, + line_item_container_url=None, + product_family=None, + misc_plugin=None, + lti_registration=None, + ) + DashboardGradingView.return_value.sync_grade.assert_called_once_with( + lti_v13_application_instance.lti_registration, + "LIS_OUTCOME_SERVICE_URL", + datetime(2022, 6, 21, 12, 0, 0), + "LTI_V13_USER_ID", + 1, + ) + + @pytest.fixture + def assignment(self, lti_v13_application_instance, db_session): + course = factories.Course(application_instance=lti_v13_application_instance) + assignment = factories.Assignment( + lis_outcome_service_url="LIS_OUTCOME_SERVICE_URL", course=course + ) + db_session.flush() + return assignment + + @pytest.fixture + def lms_user(self): + return factories.LMSUser(lti_v13_user_id="LTI_V13_USER_ID") + + @pytest.fixture + def pyramid_request(self, pyramid_request, lms_user): + pyramid_request.parsed_params = { + "grades": [{"h_userid": lms_user.h_userid, "grade": 1}] + } + return pyramid_request + + @pytest.fixture + def views(self, pyramid_request): + return DashboardGradingViews(pyramid_request) + + @pytest.fixture + def DashboardGradingView(self, patch): + return patch("lms.views.dashboard.api.grading.LTI13GradingService") From 5b10d03b1bc6175d8e5f6d87bb38adf3977d5158 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Wed, 11 Sep 2024 14:09:24 +0200 Subject: [PATCH 2/3] Expose grade syncing endpoint to the FE --- lms/js_config_types.py | 3 +++ lms/resources/_js_config/__init__.py | 3 +++ tests/unit/lms/resources/_js_config/__init___test.py | 1 + 3 files changed, 7 insertions(+) diff --git a/lms/js_config_types.py b/lms/js_config_types.py index 28a4207632..ae564ee5ba 100644 --- a/lms/js_config_types.py +++ b/lms/js_config_types.py @@ -118,6 +118,9 @@ class DashboardRoutes(TypedDict): students: str """Paginated endpoint to fetch students""" + assignment_grades_sync: str + """Sync grades for a given assignment""" + class User(TypedDict): is_staff: bool diff --git a/lms/resources/_js_config/__init__.py b/lms/resources/_js_config/__init__.py index 3294ede0cf..1ae3d9b4e8 100644 --- a/lms/resources/_js_config/__init__.py +++ b/lms/resources/_js_config/__init__.py @@ -283,6 +283,9 @@ def enable_dashboard_mode(self, token_lifetime_seconds: int) -> None: "api.dashboard.assignments" ), students=self._to_frontend_template("api.dashboard.students"), + assignment_grades_sync=self._to_frontend_template( + "api.dashboard.assignments.grading.sync" + ), ), ), } diff --git a/tests/unit/lms/resources/_js_config/__init___test.py b/tests/unit/lms/resources/_js_config/__init___test.py index 1e0985d071..acbc0bcba0 100644 --- a/tests/unit/lms/resources/_js_config/__init___test.py +++ b/tests/unit/lms/resources/_js_config/__init___test.py @@ -735,6 +735,7 @@ def test_it(self, js_config, lti_user, bearer_token_schema): "courses": "/api/dashboard/courses", "assignments": "/api/dashboard/assignments", "students": "/api/dashboard/students", + "assignment_grades_sync": "/api/dashboard/assignments/:assignment_id/grading/sync", }, } From d9d3665ba338300b88fe5dbe6a2cb9bf1084a525 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Mon, 16 Sep 2024 10:26:25 +0200 Subject: [PATCH 3/3] Add a feature flag for the sync grade side of auto grading This will allow us to expose the configuring and calculating side of auto grading while we work on the syncing part. --- lms/js_config_types.py | 3 +++ lms/models/application_instance.py | 1 + lms/resources/_js_config/__init__.py | 4 ++++ .../admin/application_instance/show.html.jinja2 | 1 + .../lms/resources/_js_config/__init___test.py | 16 +++++++++++++++- 5 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lms/js_config_types.py b/lms/js_config_types.py index ae564ee5ba..ff68f2ad6e 100644 --- a/lms/js_config_types.py +++ b/lms/js_config_types.py @@ -130,3 +130,6 @@ class User(TypedDict): class DashboardConfig(TypedDict): user: User routes: DashboardRoutes + + auto_grading_sync_enabled: bool + """Whether or nor the opotion to sync grades back to the LMS is enabled.""" diff --git a/lms/models/application_instance.py b/lms/models/application_instance.py index a1de0f882b..2a08402ce3 100644 --- a/lms/models/application_instance.py +++ b/lms/models/application_instance.py @@ -56,6 +56,7 @@ class ApplicationSettings(JSONSettings): JSONSetting("hypothesis", "instructor_email_digests_enabled", asbool), JSONSetting("hypothesis", "lti_13_sourcedid_for_grading", asbool), JSONSetting("hypothesis", "auto_grading_enabled", asbool), + JSONSetting("hypothesis", "auto_grading_sync_enabled", asbool), ) diff --git a/lms/resources/_js_config/__init__.py b/lms/resources/_js_config/__init__.py index 1ae3d9b4e8..f3132a02bd 100644 --- a/lms/resources/_js_config/__init__.py +++ b/lms/resources/_js_config/__init__.py @@ -287,6 +287,10 @@ def enable_dashboard_mode(self, token_lifetime_seconds: int) -> None: "api.dashboard.assignments.grading.sync" ), ), + auto_grading_sync_enabled=self._lti_user + and self._application_instance.settings.get( + "hypothesis", "auto_grading_sync_enabled", False + ), ), } ) diff --git a/lms/templates/admin/application_instance/show.html.jinja2 b/lms/templates/admin/application_instance/show.html.jinja2 index 7820e10a78..607c737423 100644 --- a/lms/templates/admin/application_instance/show.html.jinja2 +++ b/lms/templates/admin/application_instance/show.html.jinja2 @@ -115,6 +115,7 @@ {{ settings_checkbox('Enable instructor email digests', 'hypothesis', 'instructor_email_digests_enabled') }} {{ settings_checkbox("Use alternative parameter for LTI1.3 grading", "hypothesis", "lti_13_sourcedid_for_grading", default=False) }} {{ settings_checkbox("Enable auto-grading", "hypothesis", "auto_grading_enabled", default=False) }} + {{ settings_checkbox("Enable auto-grading grade sync", "hypothesis", "auto_grading_sync_enabled", default=False) }}
Canvas settings diff --git a/tests/unit/lms/resources/_js_config/__init___test.py b/tests/unit/lms/resources/_js_config/__init___test.py index acbc0bcba0..f292922473 100644 --- a/tests/unit/lms/resources/_js_config/__init___test.py +++ b/tests/unit/lms/resources/_js_config/__init___test.py @@ -713,7 +713,18 @@ def LTIEvent(self, patch): class TestEnableDashboardMode: - def test_it(self, js_config, lti_user, bearer_token_schema): + @pytest.mark.parametrize("auto_grading_sync_setting", [True, False]) + def test_it( + self, + js_config, + lti_user, + bearer_token_schema, + auto_grading_sync_setting, + application_instance, + ): + application_instance.settings.set( + "hypothesis", "auto_grading_sync_enabled", auto_grading_sync_setting + ) js_config.enable_dashboard_mode(token_lifetime_seconds=100) config = js_config.asdict() @@ -737,6 +748,7 @@ def test_it(self, js_config, lti_user, bearer_token_schema): "students": "/api/dashboard/students", "assignment_grades_sync": "/api/dashboard/assignments/:assignment_id/grading/sync", }, + "auto_grading_sync_enabled": auto_grading_sync_setting, } def test_user_when_staff(self, js_config, pyramid_request_staff_member, context): @@ -748,6 +760,8 @@ def test_user_when_staff(self, js_config, pyramid_request_staff_member, context) "is_staff": True, "display_name": "staff@example.com", } + # Grade syncing always disabled for staff + assert not config["dashboard"]["auto_grading_sync_enabled"] @pytest.fixture def pyramid_request_staff_member(self, pyramid_config, pyramid_request):