diff --git a/bases/rsptx/admin_server_api/routers/lti1p3.py b/bases/rsptx/admin_server_api/routers/lti1p3.py index 8eee30c42..a528a1c74 100644 --- a/bases/rsptx/admin_server_api/routers/lti1p3.py +++ b/bases/rsptx/admin_server_api/routers/lti1p3.py @@ -238,7 +238,7 @@ async def login_or_create_user( user = await create_user(new_user) rslogger.info(f"LTI1p3 - Created {user.username} ({user.id})") except Exception as e: - HTTPException(status_code=400, detail=f"Error creating user '{e}'") + raise HTTPException(status_code=400, detail=f"Error creating user '{e}'") else: # have user, make sure their course_id/course_name are updated await update_user( diff --git a/bases/rsptx/assignment_server_api/routers/grader.py b/bases/rsptx/assignment_server_api/routers/grader.py index 5c8b1e849..fe6375918 100644 --- a/bases/rsptx/assignment_server_api/routers/grader.py +++ b/bases/rsptx/assignment_server_api/routers/grader.py @@ -44,7 +44,6 @@ recompute_totals_for, ) - router = APIRouter( prefix="/instructor/grader", tags=["grader"], @@ -653,7 +652,9 @@ async def upsert_grade( recomputed = [] for assignment in await _assignments_to_recompute(course, payload): try: - await recompute_totals_for(course, assignment, [payload.sid]) + await recompute_totals_for( + course, assignment, [payload.sid], instructor_triggered=True + ) recomputed.append(assignment.id) except Exception as e: # pragma: no cover - defensive # A failed roll-up must not lose the grade the instructor just typed. @@ -766,7 +767,13 @@ async def regrade_run( which_to_grade_override=payload.which_to_grade_override, ) report = await regrade_batch( - course, sids, questions, assignment, options, dry_run=False + course, + sids, + questions, + assignment, + options, + dry_run=False, + instructor_triggered=True, ) rslogger.info( f"Regrade run by {user.username} assignment={assignment.id} " @@ -802,7 +809,9 @@ async def recompute_totals( } sids = [s for s in payload.sids if s not in instructor_ids] - processed = await recompute_totals_for(course, assignment, sids) + processed = await recompute_totals_for( + course, assignment, sids, instructor_triggered=True + ) rslogger.info( f"Recompute totals by {user.username} assignment={assignment.id} " f"students={processed}" @@ -930,7 +939,12 @@ async def set_manual_assignment_total( grade = await set_manual_total( student.id, assignment.id, course.course_name, payload.score, True ) - await attempt_lti1p3_score_update(student.id, assignment.id, payload.score) + await attempt_lti1p3_score_update( + student.id, + assignment.id, + payload.score, + instructor_triggered=True, + ) rslogger.info( f"Manual total set by {user.username} assignment={assignment.id} " f"sid={payload.sid} score={payload.score}" @@ -950,7 +964,9 @@ async def set_manual_assignment_total( await set_manual_total( student.id, assignment.id, course.course_name, preserved, False ) - await recompute_totals_for(course, assignment, [payload.sid]) + await recompute_totals_for( + course, assignment, [payload.sid], instructor_triggered=True + ) recomputed = await fetch_grade(student.id, assignment.id) rslogger.info( f"Manual total reverted by {user.username} assignment={assignment.id} " diff --git a/bases/rsptx/rsmanage/core.py b/bases/rsptx/rsmanage/core.py index d8ce0831b..2cb30c14a 100644 --- a/bases/rsptx/rsmanage/core.py +++ b/bases/rsptx/rsmanage/core.py @@ -1157,7 +1157,12 @@ async def fixtotals( for a in assignments: assignments_scanned += 1 changes = await recompute_totals_detail( - c, a, sids, dry_run=dry_run, only_existing=not create_missing + c, + a, + sids, + dry_run=dry_run, + only_existing=not create_missing, + instructor_triggered=True, ) students_scanned += len(changes) total_manual += sum(1 for ch in changes if ch.skipped_manual) diff --git a/components/rsptx/grading_helpers/regrade.py b/components/rsptx/grading_helpers/regrade.py index ca5abc02f..7e0ba84aa 100644 --- a/components/rsptx/grading_helpers/regrade.py +++ b/components/rsptx/grading_helpers/regrade.py @@ -35,7 +35,6 @@ PEER_SCORE_SENTINEL, ) - MANUAL_COMMENT = "autograded" @@ -315,6 +314,7 @@ async def _recompute_total_for_user( course_name: str, dry_run: bool = False, only_existing: bool = False, + instructor_triggered: bool = False, ) -> TotalChange: """Roll the student's ``question_grades`` up into their ``grades`` row. @@ -370,7 +370,9 @@ async def _recompute_total_for_user( manual_total=False, ) await upsert_grade(new_grade) - await attempt_lti1p3_score_update(user.id, assignment.id, total) + await attempt_lti1p3_score_update( + user.id, assignment.id, total, instructor_triggered=instructor_triggered + ) return change @@ -380,6 +382,7 @@ async def recompute_totals_detail( sids: Optional[List[str]] = None, dry_run: bool = False, only_existing: bool = False, + instructor_triggered: bool = False, ) -> List[TotalChange]: """Recompute assignment totals for the given students and report what moved. @@ -407,6 +410,7 @@ async def recompute_totals_detail( course.course_name, dry_run=dry_run, only_existing=only_existing, + instructor_triggered=instructor_triggered, ) ) except Exception as e: # pragma: no cover - defensive @@ -418,6 +422,7 @@ async def recompute_totals_for( course: CoursesValidator, assignment: AssignmentValidator, sids: Optional[List[str]] = None, + instructor_triggered: bool = False, ) -> int: """Recompute assignment totals (and push LTI 1.3 scores) for the given students. When ``sids`` is empty/None every student in the course is @@ -426,7 +431,11 @@ async def recompute_totals_for( This is used by the manual multi-grade flow, where individual grades are written through ``POST /grade`` (which does not itself recompute totals). """ - return len(await recompute_totals_detail(course, assignment, sids)) + return len( + await recompute_totals_detail( + course, assignment, sids, instructor_triggered=instructor_triggered + ) + ) async def regrade_batch( @@ -436,6 +445,7 @@ async def regrade_batch( assignment: AssignmentValidator, options: RegradeOptions, dry_run: bool = False, + instructor_triggered: bool = False, ) -> RegradeReport: """Run a re-grade over the student x question matrix. @@ -481,7 +491,10 @@ async def regrade_batch( if user is not None: try: await _recompute_total_for_user( - user, assignment, course.course_name + user, + assignment, + course.course_name, + instructor_triggered=instructor_triggered, ) except Exception as e: # pragma: no cover - defensive rslogger.error(f"recompute totals failed sid={sid}: {e}") diff --git a/components/rsptx/lti1p3/core.py b/components/rsptx/lti1p3/core.py index 76c9b84c5..4e60516c5 100644 --- a/components/rsptx/lti1p3/core.py +++ b/components/rsptx/lti1p3/core.py @@ -20,31 +20,6 @@ from rsptx.lti1p3.pylti1p3.service_connector import ServiceConnector from rsptx.lti1p3.pylti1p3.assignments_grades import AssignmentsGradesService -# ================================ -# Notes re LTI 1.3 Implementation -# ================================ -# -# Pathways for grades to get sent to LTI 1.3 platforms: -# 1. User does activity in book -# - grade_submission or score_reading_page call compute_total_score -# - compute_total_score calls attempt_lti1p3_score_update -# - scores are not pushed if assignment is not released -# 2. Instructor releases grades in grading interface -# - api call made to /runestone/admin/releasegrades -# - releasegrades calls attempt_lti1p3_score_updates if the grades are now released -# 3. Instructor presses send lti grades in grading interface -# - api call made to /runestone/admin/push_lti_grades -# - push_lti_grades calls attempt_lti1p3_score_updates -# 4. One of the following: -# - Relase Grade to LTI button pressed in LTI_ONLY mode -# - Assignment launched -# - peer.py send_lti_scores is called -# - Student hits calculate self grade button on assignment -# They call _try_to_send_lti_grade -# - _try_to_send_lti_grade calls attempt_lti1p3_score_update if is 1.3 - -# ================================ - def get_assignment_score_resource_id(course, assignment): """ @@ -93,13 +68,33 @@ def time_now() -> str: """ Get current time formatted the way LTI spec expects it. """ - return ( - datetime.datetime.now(datetime.timezone.utc).isoformat().replace("+00:00", "Z") - ) + return _format_lti_timestamp(datetime.datetime.now(datetime.timezone.utc)) + + +def _format_lti_timestamp(value: datetime.datetime) -> str: + if value.tzinfo is None: + value = value.replace(tzinfo=datetime.timezone.utc) + else: + value = value.astimezone(datetime.timezone.utc) + return value.isoformat().replace("+00:00", "Z") + + +def _submitted_at_for_score( + rs_assignment: Assignment, score_timestamp: str, instructor_triggered: bool +) -> str: + if not instructor_triggered: + return score_timestamp + if rs_assignment.duedate is None: + return score_timestamp + return _format_lti_timestamp(rs_assignment.duedate - datetime.timedelta(minutes=1)) async def attempt_lti1p3_score_update( - rs_user_id: int, rs_assign_id: int, score: float, force: bool = False + rs_user_id: int, + rs_assign_id: int, + score: float, + force: bool = False, + instructor_triggered: bool = False, ): """ Attempt to send a score update to any linked LTI 1.3 tools for a given user and assignment. @@ -109,6 +104,7 @@ async def attempt_lti1p3_score_update( :param rs_assign_id: The Runestone assignment id :param score: The score to send :param force: If True, will send the score even if the grades are not yet released in RS or the course is set to not auto-update grades + :param instructor_triggered: If True, report submission.submittedAt as just before the assignment deadline. """ rslogger.debug("LTI1p3 - attempt_lti1p3_score_update") lti_assign = await fetch_lti1p3_grading_data_for_assignment(rs_assign_id) @@ -120,17 +116,23 @@ async def attempt_lti1p3_score_update( (await fetch_lti1p3_user(rs_user_id, lti_assign.lti1p3_course.id), score) ] await _send_lti1p3_score_updates( - lti_assign=lti_assign, updates=updates, force=force + lti_assign=lti_assign, + updates=updates, + force=force, + instructor_triggered=instructor_triggered, ) -async def attempt_lti1p3_score_updates(rs_assign_id: int, force: bool = False): +async def attempt_lti1p3_score_updates( + rs_assign_id: int, force: bool = False, instructor_triggered: bool = False +): """ Attempt to send a score update to any linked LTI 1.3 tools for a given assignment. Will return early if no LTI 1.3 data is found for the assignment. :param rs_assign_id: The Runestone assignment id :param force: If True, will send the score even if the grades are not yet released in RS or the course is set to not auto-update grades + :param instructor_triggered: If True, report submission.submittedAt as just before the assignment deadline. """ rslogger.debug("LTI1p3 - attempt_lti1p3_score_updates") lti_assign = await fetch_lti1p3_grading_data_for_assignment(rs_assign_id) @@ -148,7 +150,10 @@ async def attempt_lti1p3_score_updates(rs_assign_id: int, force: bool = False): # updates = [(u, grades_dict.get(u.rs_user_id)) for u in all_users if u.rs_user_id in grades_dict] await _send_lti1p3_score_updates( - lti_assign=lti_assign, updates=updates, force=force + lti_assign=lti_assign, + updates=updates, + force=force, + instructor_triggered=instructor_triggered, ) @@ -156,6 +161,7 @@ async def _send_lti1p3_score_updates( lti_assign: Lti1p3Assignment, updates: List[Tuple[Lti1p3User, int]], force: bool = False, + instructor_triggered: bool = False, ): """ Attempt to send a set of 1+ updates to any linked LTI 1.3 tools for a given assignment. @@ -163,6 +169,7 @@ async def _send_lti1p3_score_updates( :param lti_assign: The Lti1p3Assignment object - must have the LTI 1.3 course and rs assignment linked. LTIcourse should have rs_course and lti_config linked. :param updates: List of tuples (Lti1p3User, score) to send :param force: If True, will send the score even if the grades are not yet released in RS or the course is set to not auto-update grades + :param instructor_triggered: If True, set submission.submittedAt just before the assignment deadline so LMS late policies don't mark instructor-entered grades late. """ rslogger.debug(f"LTI1p3 - _send_lti1p3_score_updates {updates}") @@ -263,14 +270,19 @@ async def _send_lti1p3_score_updates( score = max_score # Send the grade + score_timestamp = time_now() + submitted_at = _submitted_at_for_score( + rs_assignment, score_timestamp, instructor_triggered + ) g = ( Grade() .set_score_given(score) .set_score_maximum(max_score) .set_user_id(lti_user.lti_user_id) - .set_timestamp(time_now()) + .set_timestamp(score_timestamp) .set_activity_progress("Completed") .set_grading_progress("FullyGraded") + .set_extra_claims({"submission": {"submittedAt": submitted_at}}) ) try: _ = await ags.put_grade(g, line_item) diff --git a/test/components/rsptx/grading_helpers/test_regrade_batch.py b/test/components/rsptx/grading_helpers/test_regrade_batch.py index ed8ac517e..2a806b21a 100644 --- a/test/components/rsptx/grading_helpers/test_regrade_batch.py +++ b/test/components/rsptx/grading_helpers/test_regrade_batch.py @@ -161,6 +161,7 @@ async def test_rollup_uses_the_graded_course(): assert fetch_scores.await_args.args == (assignment.id, "testcourse", "student1") assert upsert.await_args.args[0].score == 7.0 assert lti.await_args.args[2] == 7.0 + assert lti.await_args.kwargs == {"instructor_triggered": False} # _effective_deadline @@ -324,6 +325,18 @@ async def test_recompute_totals_for_still_returns_the_processed_count(): assert processed == 1 +async def test_recompute_totals_for_forwards_instructor_triggered_flag(): + fu, fg, fs, up, lti = _patch_rollup( + SimpleNamespace(score=0, manual_total=False), [5] + ) + with fu, fg, fs, up, lti as lti_mock: + await regrade.recompute_totals_for( + _course(), _assignment(), ["student1"], instructor_triggered=True + ) + + assert lti_mock.await_args.kwargs == {"instructor_triggered": True} + + async def test_only_existing_skips_students_with_no_grade_row(): """A bulk repair should not materialise a 0 for a student who never had a total -- that would push a fresh zero to the LMS for a non-submitter.""" diff --git a/test/components/rsptx/lti1p3/test_score_updates.py b/test/components/rsptx/lti1p3/test_score_updates.py index d2c3a471a..b03f215c7 100644 --- a/test/components/rsptx/lti1p3/test_score_updates.py +++ b/test/components/rsptx/lti1p3/test_score_updates.py @@ -7,6 +7,8 @@ assignment's points (which the LMS rejects with a 422). """ +import datetime +import json from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock, patch @@ -21,10 +23,10 @@ def _lti_user(rs_user_id=1, lti_user_id="lms-user-1"): return SimpleNamespace(rs_user_id=rs_user_id, lti_user_id=lti_user_id) -def _lti_assign(points=10, released=True): +def _lti_assign(points=10, released=True, duedate=None): rs_course = SimpleNamespace(id=1, course_name="course1", timezone="UTC") rs_assignment = SimpleNamespace( - id=7, name="Homework 1", duedate=None, points=points, released=released + id=7, name="Homework 1", duedate=duedate, points=points, released=released ) return SimpleNamespace( lti_lineitem_id="https://lms.example/lineitems/1", @@ -35,7 +37,13 @@ def _lti_assign(points=10, released=True): ) -async def _send(updates, lti_assign=None, show_points="true"): +async def _send( + updates, + lti_assign=None, + show_points="true", + return_payloads=False, + instructor_triggered=False, +): """Drive _send_lti1p3_score_updates and report the grades it sent. Returns the list of (user_id, score_given, score_maximum) actually handed to @@ -45,13 +53,16 @@ async def _send(updates, lti_assign=None, show_points="true"): sent = [] async def fake_put_grade(grade, line_item): - sent.append( - ( - grade.get_user_id(), - grade.get_score_given(), - grade.get_score_maximum(), + if return_payloads: + sent.append(json.loads(grade.get_value())) + else: + sent.append( + ( + grade.get_user_id(), + grade.get_score_given(), + grade.get_score_maximum(), + ) ) - ) return {} ags = MagicMock() @@ -69,11 +80,35 @@ async def fake_put_grade(grade, line_item): patch.object(core, "ServiceConnector"), patch.object(core, "AssignmentsGradesService", return_value=ags), ): - await core._send_lti1p3_score_updates(lti_assign, updates) + await core._send_lti1p3_score_updates( + lti_assign, updates, instructor_triggered=instructor_triggered + ) return sent +async def test_instructor_triggered_score_sets_submitted_at_before_deadline(): + duedate = datetime.datetime(2026, 1, 2, 12, 0, 0) + + sent = await _send( + [(_lti_user(), 5)], + lti_assign=_lti_assign(duedate=duedate), + return_payloads=True, + instructor_triggered=True, + ) + + assert sent[0]["submission"] == {"submittedAt": "2026-01-02T11:59:00Z"} + + +async def test_student_triggered_score_sets_submitted_at_to_timestamp(): + sent = await _send( + [(_lti_user(), 5)], + return_payloads=True, + ) + + assert sent[0]["submission"] == {"submittedAt": sent[0]["timestamp"]} + + async def test_a_user_with_no_lti_mapping_is_skipped_not_fatal(): """fetch_lti1p3_user returns None for a student who never launched through the LMS. That used to raise and abandon everyone after them in the batch."""