Sync grades back to the LMS for auto grading #6672

marcospri · 2024-09-10T15:41:57Z

For:

Backend: Participation grading #6647

Basic API endpoint to support the first version of the "Sync grades" button.

Testing

Based on this assignment https://hypothesis.instructure.com/courses/319/assignments/7296

use a request like:

curl 'http://localhost:8001/api/dashboard/assignments/121/grading/sync' -X POST \
  -H 'Accept: */*' \
  -H "Content-Type: application/json" \
  -H 'Authorization: XXXXXXX' \
  --data '{"grades": [{"h_userid": "acct:[email protected]", "grade": 0.7}]}'

with a token from an instructor (it won't work for staff members).

Check the grade over: https://hypothesis.instructure.com/courses/319/assignments/7296

marcospri · 2024-09-10T15:48:20Z

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(


New API endpoint

marcospri · 2024-09-10T15:48:41Z

lms/services/dashboard.py

@@ -1,8 +1,8 @@
 from pyramid.httpexceptions import HTTPNotFound, HTTPUnauthorized
 from sqlalchemy import select

+from lms.models import Assignment, Organization


Adding some type annos around here to get autocompletion.

marcospri · 2024-09-10T15:53:24Z

lms/services/lti_grading/_v13.py

@@ -83,15 +83,32 @@ 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


New method to sync an individual grade.

We don't use the existing record_result because:

We'd need to backport now any changes here for the LTI1.1 as the share a common interface.

That method is more complex as it deals with comments (that might need to be formatted before), "pre_record_hook", to change the shape of the payload...

I reckon the simple method is the best solution right now.

marcospri · 2024-09-11T08:46:25Z

lms/views/dashboard/api/grading.py

+        permission=Permissions.GRADE_ASSIGNMENT,
+        schema=AutoGradeSyncSchema,
+    )
+    def auto_grading_sync(self):


This is an endpoint that would need many changes before is ready for the MVP:

Don't start a sync if there's already one running.

Run the actual sync of grades asynchronously on a celery task.

I also image we'll need a GET endpoint to fetch progress (state of the last one?) of any running sync.

marcospri · 2024-09-11T08:46:46Z

lms/views/dashboard/api/grading.py

+    )
+    def auto_grading_sync(self):
+        assignment = self.dashboard_service.get_request_assignment(self.request)
+        assert assignment.lis_outcome_service_url, "Assignment wihtout grading URL"


asserts for the type checker.

marcospri · 2024-09-11T09:52:07Z

lms/views/dashboard/api/grading.py

+            ltia_service=self.request.find_service(LTIAHTTPService),
+            line_item_url=None,
+            line_item_container_url=None,
+            product_family=None,  # type: ignore


This shouldn't be None in the grading service but to refactor that we have to refactor the v11 version as well as they share an interface. I'll have to do that once we have the MVP.

Perhaps it makes sense to optionally inject a grading service in the constructor, and fall back to this if not provided. That way you can still use this at runtime, but pass a mock in tests.

Oh I see, but views are "built" by the framework itself.

This sticks out like a shore thumb so I reckon the right fix is to refactor the service to take this as parameters in v11 and v13.

Crated a note on the main issue to keep track of that.

but views are "built" by the framework itself.

Oh, I didn't think about that. Let's keep it as is for now and apply your suggested improvements afterwards.

marcospri · 2024-09-11T09:53:39Z

lms/views/dashboard/api/grading.py

+
+
+class AutoGradeSyncSchema(JSONPyramidRequestSchema):
+    grades = fields.List(fields.Nested(_GradeSchema), required=True)


The endpoint takes the grades from the frontend. I reckon this makes sense because:

It saves the BE to doing double work of calculating the grades again.

It guarantees that what you see in the dashboard is what we are going to send to the LMS

I was thinking about this as well, and I also think it makes sense.

It would even allow to eventually let instructors adjust the pre-calculated grade before syncing, right from the dashboard, if at some point we want to allow that.

acelaya

I think we should add the new endpoint to the DashboardRoutes definition. Other than that, this looks good.

acelaya · 2024-09-11T10:16:42Z

lms/views/dashboard/api/grading.py

+
+
+class AutoGradeSyncSchema(JSONPyramidRequestSchema):
+    grades = fields.List(fields.Nested(_GradeSchema), required=True)


I was thinking about this as well, and I also think it makes sense.

It would even allow to eventually let instructors adjust the pre-calculated grade before syncing, right from the dashboard, if at some point we want to allow that.

acelaya · 2024-09-11T10:19:56Z

lms/views/dashboard/api/grading.py

+            ltia_service=self.request.find_service(LTIAHTTPService),
+            line_item_url=None,
+            line_item_container_url=None,
+            product_family=None,  # type: ignore


Perhaps it makes sense to optionally inject a grading service in the constructor, and fall back to this if not provided. That way you can still use this at runtime, but pass a mock in tests.

This will allow us to expose the configuring and calculating side of auto grading while we work on the syncing part.

marcospri force-pushed the sync-grades-dashbaord branch from 6ed9f3b to c783b22 Compare September 10, 2024 15:58

marcospri force-pushed the lms-user-v13-id-code branch 2 times, most recently from 486e72b to 1f65d03 Compare September 11, 2024 08:33

Base automatically changed from lms-user-v13-id-code to main September 11, 2024 08:37

marcospri force-pushed the sync-grades-dashbaord branch 3 times, most recently from 91aa414 to 2f1ebc0 Compare September 11, 2024 09:51

marcospri commented Sep 11, 2024

View reviewed changes

marcospri requested a review from acelaya September 11, 2024 10:01

marcospri changed the title ~~[WIP] Sync grades back to the LMS for auto grading~~ Sync grades back to the LMS for auto grading Sep 11, 2024

acelaya approved these changes Sep 11, 2024

View reviewed changes

marcospri mentioned this pull request Sep 11, 2024

Backend: Participation grading #6647

Closed

2 tasks

marcospri added 2 commits September 16, 2024 09:43

New dahboard endpoint to sync grades of a given async

1549203

Expose grade syncing endpoint to the FE

5b10d03

marcospri force-pushed the sync-grades-dashbaord branch from fe680b6 to 5b10d03 Compare September 16, 2024 07:43

Add a feature flag for the sync grade side of auto grading

d9d3665

This will allow us to expose the configuring and calculating side of auto grading while we work on the syncing part.

marcospri force-pushed the sync-grades-dashbaord branch from 9f04f16 to d9d3665 Compare September 16, 2024 12:42

marcospri merged commit a02651c into main Sep 19, 2024
9 checks passed

marcospri deleted the sync-grades-dashbaord branch September 19, 2024 08:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync grades back to the LMS for auto grading #6672

Sync grades back to the LMS for auto grading #6672

marcospri commented Sep 10, 2024 •

edited

Loading

marcospri Sep 10, 2024

marcospri Sep 10, 2024

marcospri Sep 10, 2024

marcospri Sep 11, 2024

marcospri Sep 11, 2024

marcospri Sep 11, 2024

acelaya Sep 11, 2024

marcospri Sep 11, 2024

acelaya Sep 11, 2024

marcospri Sep 11, 2024

acelaya Sep 11, 2024

acelaya left a comment

acelaya Sep 11, 2024

acelaya Sep 11, 2024



		class AutoGradeSyncSchema(JSONPyramidRequestSchema):
		grades = fields.List(fields.Nested(_GradeSchema), required=True)

Sync grades back to the LMS for auto grading #6672

Sync grades back to the LMS for auto grading #6672

Conversation

marcospri commented Sep 10, 2024 • edited Loading

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcospri commented Sep 10, 2024 •

edited

Loading