From f251b0405a85ac2bbd628e55ac2687546fc0e730 Mon Sep 17 00:00:00 2001 From: Alex Jordan Date: Mon, 18 Nov 2024 14:58:23 -0800 Subject: [PATCH] feedback from PR#2617 --- lib/WeBWorK/Authen/LTIAdvanced/SubmitGrade.pm | 8 ++-- .../Authen/LTIAdvantage/SubmitGrade.pm | 24 ++++++----- lib/WeBWorK/ContentGenerator/GatewayQuiz.pm | 2 +- lib/WeBWorK/Utils/ProblemProcessing.pm | 10 +++-- lib/WeBWorK/Utils/Sets.pm | 43 +++++++++---------- 5 files changed, 45 insertions(+), 42 deletions(-) diff --git a/lib/WeBWorK/Authen/LTIAdvanced/SubmitGrade.pm b/lib/WeBWorK/Authen/LTIAdvanced/SubmitGrade.pm index 14eb40d6be..a68e591e1d 100644 --- a/lib/WeBWorK/Authen/LTIAdvanced/SubmitGrade.pm +++ b/lib/WeBWorK/Authen/LTIAdvanced/SubmitGrade.pm @@ -138,13 +138,13 @@ async sub submit_set_grade ($self, $userID, $setID) { my $db = $c->{db}; my $user = $db->getUser($userID); - return unless $user; - - my $score = can_submit_LMS_score($db, $ce, $userID, $setID); - return unless ($score || !$self->{post_processing_mode} && $ce->{LTIGradeOnSubmit} eq 'homework_always'); + return 0 unless $user; my $userSet = $db->getMergedSet($userID, $setID); + my $score = can_submit_LMS_score($db, $ce, $userID, $userSet); + return 0 unless ($score || !$self->{post_processing_mode} && $ce->{LTIGradeOnSubmit} eq 'homework_always'); + $self->warning("Submitting grade for user $userID and set $setID.") if $ce->{debug_lti_grade_passback} || $self->{post_processing_mode}; $self->warning('lis_source_did is not available for this set.') diff --git a/lib/WeBWorK/Authen/LTIAdvantage/SubmitGrade.pm b/lib/WeBWorK/Authen/LTIAdvantage/SubmitGrade.pm index fb3614e6a2..df91eb7656 100644 --- a/lib/WeBWorK/Authen/LTIAdvantage/SubmitGrade.pm +++ b/lib/WeBWorK/Authen/LTIAdvantage/SubmitGrade.pm @@ -217,13 +217,13 @@ async sub submit_set_grade ($self, $userID, $setID) { my $db = $c->{db}; my $user = $db->getUser($userID); - return unless $user; - - my $score = can_submit_LMS_score($db, $ce, $userID, $setID); - return unless ($score || !$self->{post_processing_mode} && $ce->{LTIGradeOnSubmit} eq 'homework_always'); + return 0 unless $user; my $userSet = $db->getMergedSet($userID, $setID); + my $score = can_submit_LMS_score($db, $ce, $userID, $userSet); + return 0 unless ($score || !$self->{post_processing_mode} && $ce->{LTIGradeOnSubmit} eq 'homework_always'); + $self->warning("Submitting grade for user $userID and set $setID."); $self->warning('LMS user id is not available for this user.') unless $user->lis_source_did; $self->warning('LMS lineitem is not available for this set.') unless $userSet->lis_source_did; @@ -271,17 +271,19 @@ async sub submit_grade ($self, $LMSuserID, $lineitem, $scoreGiven, $scoreMaximum return 0; } - my $priorData = decode_json($response->body); - my $priorScore = @$priorData - && $priorData->[0]{resultMaximum} ? $priorData->[0]{resultScore} / $priorData->[0]{resultMaximum} : 0; + my $priorData = decode_json($response->body); + my $priorScore = + (@$priorData && $priorData->[0]{resultMaximum} && defined $priorData->[0]{resultScore}) + ? $priorData->[0]{resultScore} / $priorData->[0]{resultMaximum} + : 0; my $score = $scoreMaximum ? $scoreGiven / $scoreMaximum : 0; - # we want to update the LMS score if the difference is significant, - # or if the new score is 1 but the LMS score was not 1 (but possibly insignificantly different) - # or if the new score is 0 and the LMS score was empty and it is past the SendScoresAfterDate + # We want to update the LMS score if the difference is significant, + # or if the new score is 1 but the LMS score was not 1 but possibly insignificantly different, + # or if the new score is 0 and the LMS score was empty and it is past the SendScoresAfterDate. if (abs($score - $priorScore) < 0.001 && ($score != 1 || $priorScore == 1) - && ($score != 0 || @$priorData && $priorData->[0]{resultScore} ne '' || $nullEqualsZero)) + && ($score != 0 || @$priorData && defined $priorData->[0]{resultScore} || $nullEqualsZero)) { $self->warning( "LMS grade will NOT be updated as the grade has not significantly changed. Old score: $priorScore, New score: $score." diff --git a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm index 5d5516f537..5e20bf4909 100644 --- a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm +++ b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm @@ -1031,7 +1031,7 @@ async sub pre_header_initialize ($c) { && ($ce->{LTIGradeOnSubmit} && $ce->{LTIGradeMode} eq 'homework' || $ce->{LTIGradeOnSubmit} && $ce->{LTIGradeMode} eq 'course' - && can_submit_LMS_score($db, $ce, $effectiveUserID, $setID)) + && can_submit_LMS_score($db, $ce, $effectiveUserID, $db->getMergedSet($effectiveUserID, $setID))) ) { my $grader = $ce->{LTI}{ $ce->{LTIVersion} }{grader}->new($c); diff --git a/lib/WeBWorK/Utils/ProblemProcessing.pm b/lib/WeBWorK/Utils/ProblemProcessing.pm index dd6a8c752f..70c699fafb 100644 --- a/lib/WeBWorK/Utils/ProblemProcessing.pm +++ b/lib/WeBWorK/Utils/ProblemProcessing.pm @@ -253,10 +253,12 @@ async sub process_and_log_answer ($c) { if ($ce->{LTIGradeMode}) { my $LMSname = $ce->{LTI}{ $ce->{LTIVersion} }{LMS_name}; my $LTIGradeResult = -1; - if ($ce->{LTIGradeOnSubmit} && $ce->{LTIGradeMode} eq 'homework' - || $ce->{LTIGradeOnSubmit} - && $ce->{LTIGradeMode} eq 'course' - && can_submit_LMS_score($db, $ce, $problem->user_id, $problem->set_id)) + if ( + $ce->{LTIGradeOnSubmit} && $ce->{LTIGradeMode} eq 'homework' + || $ce->{LTIGradeOnSubmit} && $ce->{LTIGradeMode} eq 'course' && can_submit_LMS_score( + $db, $ce, $problem->user_id, $db->getMergedSet($problem->user_id, $problem->set_id) + ) + ) { $LTIGradeResult = 0; my $grader = $ce->{LTI}{ $ce->{LTIVersion} }{grader}->new($c); diff --git a/lib/WeBWorK/Utils/Sets.pm b/lib/WeBWorK/Utils/Sets.pm index dfc7da414c..84e2771cf3 100644 --- a/lib/WeBWorK/Utils/Sets.pm +++ b/lib/WeBWorK/Utils/Sets.pm @@ -147,18 +147,17 @@ sub grade_gateway ($db, $setName, $studentName) { } } -sub set_attempted ($db, $userID, $setID) { - my $userSet = $db->getMergedSet($userID, $setID); +sub set_attempted ($db, $userID, $userSet) { + my $setID = $userSet->set_id; if ($userSet->assignment_type() =~ /gateway/) { my @versionNums = $db->listSetVersions($userID, $setID); - # it counts as "attempted" if there is more than one version + # It counts as "attempted" if there is more than one version. return 1 if (1 < @versionNums); - # if there is one version, check for an attempted problem - # there could also be no actual attempted problems, but something like an - # achievement item has awarded credit for one exercise somewhere in the test + # If there is one version, check for an attempted problem. There could also + # be no actual attempted problems, but a score was manually overridden. if (@versionNums) { my @problemNums = $db->listUserProblems($userID, $setID); my $problem = $db->getMergedProblemVersion($userID, $setID, $versionNums[0], $problemNums[0]); @@ -170,7 +169,7 @@ sub set_attempted ($db, $userID, $setID) { return 0; } - # if there are no versions + # If there are no versions, the test was not attempted. return 0; } else { my @problemNums = $db->listUserProblems($userID, $setID); @@ -185,27 +184,29 @@ sub set_attempted ($db, $userID, $setID) { sub earliest_gateway_date ($db, $ce, $userSet) { my @versionNums = $db->listSetVersions($userSet->user_id, $userSet->set_id); - # if there are no versions, use the template's date + # If there are no versions, use the template's date. return get_LTISendScoresAfterDate($userSet, $ce) unless (@versionNums); - # otherwise, use the earliest date among versions - my $earliest_date = - get_LTISendScoresAfterDate($db->getSetVersion($userSet->user_id, $userSet->set_id, $versionNums[0]), $ce); + # Otherwise, use the earliest date among versions. + my $earliest_date = -1; for my $i (@versionNums) { my $versionedSetDate = get_LTISendScoresAfterDate($db->getSetVersion($userSet->user_id, $userSet->set_id, $i), $ce); - $earliest_date = $versionedSetDate if ($versionedSetDate < $earliest_date); + $earliest_date = $versionedSetDate if $earliest_date == -1 || $versionedSetDate < $earliest_date; } return $earliest_date; } sub grade_all_sets ($db, $ce, $studentName) { - my @setIDs = $db->listUserSets($studentName); + my @setIDs = $db->listUserSets($studentName); + my @userSetIDs = map { [ $studentName, $_ ] } @setIDs; + my @userSets = $db->getMergedSets(@userSetIDs); + my $courseTotalRight = 0; my $courseTotal = 0; - for my $setID (@setIDs) { - my $score = can_submit_LMS_score($db, $ce, $studentName, $setID); + for my $userSet (@userSets) { + my $score = can_submit_LMS_score($db, $ce, $studentName, $userSet); my $totalRight = $score->{totalRight}; my $total = $score->{total}; my $criticalDate = $score->{criticalDate} // ''; @@ -213,7 +214,7 @@ sub grade_all_sets ($db, $ce, $studentName) { if ($ce->{LTISendScoresAfterDate} eq 'never' || $criticalDate && before($criticalDate)) { next if ($ce->{LTISendGradesEarlyThreshold} eq 'attempted' - && !set_attempted($db, $studentName, $setID)); + && !set_attempted($db, $studentName, $userSet)); next if ($ce->{LTISendGradesEarlyThreshold} ne 'attempted' && $total > 0 @@ -247,16 +248,14 @@ sub get_LTISendScoresAfterDate ($set, $ce) { } } -# Checks if the set is past the LTISendScoresAfterDate or has met the LTISendGradesEarlyThreshold +# Checks if the set is past the LTISendScoresAfterDate or has met the LTISendGradesEarlyThreshold. # Returns a reference to hash with keys totalRight, total, score, criticalDate if the set has met # either condition and undef if not. -sub can_submit_LMS_score ($db, $ce, $userID, $setID) { - my $userSet = $db->getMergedSet($userID, $setID); - +sub can_submit_LMS_score ($db, $ce, $userID, $userSet) { my $totalRight; my $total; if ($userSet->assignment_type() =~ /gateway/) { - ($totalRight, $total) = grade_gateway($db, $setID, $userID); + ($totalRight, $total) = grade_gateway($db, $userSet->set_id, $userID); } else { ($totalRight, $total) = (grade_set($db, $userSet, $userID))[ 0, 1 ]; } @@ -275,7 +274,7 @@ sub can_submit_LMS_score ($db, $ce, $userID, $setID) { } return $return - if ($ce->{LTISendGradesEarlyThreshold} eq 'attempted' && set_attempted($db, $userID, $setID) + if ($ce->{LTISendGradesEarlyThreshold} eq 'attempted' && set_attempted($db, $userID, $userSet) || $score >= $ce->{LTISendGradesEarlyThreshold}); }