From b45b5ae335bf77699b15737b936e73c0f1ab5c15 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Thu, 13 Feb 2025 15:42:53 -0600 Subject: [PATCH] Actually handle failure to find a problem from a grouping set gracefully. Actually, this handles any failure to generate a new test version gracefully. This wraps the set version assignment call in an `eval` and catches any exceptions that occur in the process including issues from an instructor not setting up grouping sets correctly. If any exceptions do occur the user is shown the usual invalid set screen with a message stating, "Unable to generate a valid test version. This is usually caused by invalid usage of grouping sets or a database error. Please speak to your instructor to fix the error. A system administrator can obtain more details on this error from the logs." The actual exception is logged to the webwork2 application log file (logs/webwork2.log) so an instructor can ask a system administrator for details of the error. Note this does not add to the general webwork2 issue of showing exceptions in the user interface. This is one way to resolve issue #2669 and probably the best way without massively revising the GatewayQuiz module. --- lib/WeBWorK/ContentGenerator/GatewayQuiz.pm | 107 +++++++++++--------- lib/WeBWorK/Utils/Instructor.pm | 48 ++++----- 2 files changed, 77 insertions(+), 78 deletions(-) diff --git a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm index 33ff9ed9b1..783c2f8622 100644 --- a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm +++ b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm @@ -550,55 +550,68 @@ async sub pre_header_initialize ($c) { ) ) { - # Assign the set, get the right name, version number, etc., and redefine the $set and $problem for the - # remainder of this method. + # Attempt to assign the set. my $setTmpl = $db->getUserSet($effectiveUserID, $setID); - assignSetVersionToUser($db, $effectiveUserID, $setTmpl); - $setVersionNumber++; - - # Get a clean version of the set and merged version to use in the rest of the routine. - my $cleanSet = $db->getSetVersion($effectiveUserID, $setID, $setVersionNumber); - $set = $db->getMergedSetVersion($effectiveUserID, $setID, $setVersionNumber); - $set->visible(1); - - $problem = $db->getMergedProblemVersion($effectiveUserID, $setID, $setVersionNumber, $setPNum[0]); - - # Convert the floating point value from Time::HiRes to an integer for use below. Truncate toward 0. - my $timeNowInt = int($c->submitTime); - - # Set up creation time, and open and due dates. - my $ansOffset = $set->answer_date - $set->due_date; - $set->version_creation_time($timeNowInt); - $set->open_date($timeNowInt); - # Figure out the due date, taking into account the time limit cap. - my $dueTime = - $timeLimit == 0 || ($set->time_limit_cap && $c->submitTime + $timeLimit > $set->due_date) - ? $set->due_date - : $timeNowInt + $timeLimit; - - $set->due_date($dueTime); - $set->answer_date($set->due_date + $ansOffset); - $set->version_last_attempt_time(0); - - # Put this new info into the database. Put back the data needed for the version, and leave blank any - # information that should be inherited from the user set or global set. Set the data which determines - # if a set is open, because a set version should not reopen after it's complete. - $cleanSet->version_creation_time($set->version_creation_time); - $cleanSet->open_date($set->open_date); - $cleanSet->due_date($set->due_date); - $cleanSet->answer_date($set->answer_date); - $cleanSet->version_last_attempt_time($set->version_last_attempt_time); - $cleanSet->version_time_limit($set->version_time_limit); - $cleanSet->attempts_per_version($set->attempts_per_version); - $cleanSet->assignment_type($set->assignment_type); - $db->putSetVersion($cleanSet); - - # This is a new set version, so it's open. - $versionIsOpen = 1; - - # Set the number of attempts for this set to zero. - $currentNumAttempts = 0; + eval { assignSetVersionToUser($db, $effectiveUserID, $setTmpl) }; + if ($@) { + $c->log->error("Error creating test version of $setID for $effectiveUserID: $@"); + $c->{invalidSet} = + $c->maketext('Unable to generate a valid test version. This is usually caused by invalid ' + . 'usage of grouping sets or a database error. Please speak to your instructor to fix the ' + . 'error. A system administrator can obtain more details on this error from the logs.'); + # Attempt to delete the set version if it was created. Failure from this attempt is ignored. + eval { $db->deleteSetVersion($userID, $setID, $setVersionNumber + 1) } + if $db->existsSetVersion($userID, $setID, $setVersionNumber + 1); + } else { + # Get the right name, version number, etc., and redefine the + # $set and $problem for the remainder of this method. + + ++$setVersionNumber; + + # Get a clean version of the set and merged version to use in the rest of the routine. + my $cleanSet = $db->getSetVersion($effectiveUserID, $setID, $setVersionNumber); + $set = $db->getMergedSetVersion($effectiveUserID, $setID, $setVersionNumber); + $set->visible(1); + + $problem = $db->getMergedProblemVersion($effectiveUserID, $setID, $setVersionNumber, $setPNum[0]); + + # Convert the floating point value from Time::HiRes to an integer for use below. Truncate toward 0. + my $timeNowInt = int($c->submitTime); + + # Set up creation time, and open and due dates. + my $ansOffset = $set->answer_date - $set->due_date; + $set->version_creation_time($timeNowInt); + $set->open_date($timeNowInt); + # Figure out the due date, taking into account the time limit cap. + my $dueTime = + $timeLimit == 0 || ($set->time_limit_cap && $c->submitTime + $timeLimit > $set->due_date) + ? $set->due_date + : $timeNowInt + $timeLimit; + + $set->due_date($dueTime); + $set->answer_date($set->due_date + $ansOffset); + $set->version_last_attempt_time(0); + + # Put this new info into the database. Put back the data needed for the version, and leave blank + # any information that should be inherited from the user set or global set. Set the data which + # determines if a set is open, because a set version should not reopen after it's complete. + $cleanSet->version_creation_time($set->version_creation_time); + $cleanSet->open_date($set->open_date); + $cleanSet->due_date($set->due_date); + $cleanSet->answer_date($set->answer_date); + $cleanSet->version_last_attempt_time($set->version_last_attempt_time); + $cleanSet->version_time_limit($set->version_time_limit); + $cleanSet->attempts_per_version($set->attempts_per_version); + $cleanSet->assignment_type($set->assignment_type); + $db->putSetVersion($cleanSet); + + # This is a new set version, so it's open. + $versionIsOpen = 1; + + # Set the number of attempts for this set to zero. + $currentNumAttempts = 0; + } } elsif ($maxAttempts != -1 && $totalNumVersions > $maxAttempts) { $c->{invalidSet} = $c->maketext('No new versions of this test are available, ' . 'because you have already taken the maximum number allowed.'); diff --git a/lib/WeBWorK/Utils/Instructor.pm b/lib/WeBWorK/Utils/Instructor.pm index 60f6ac6b79..9e06ba66bd 100644 --- a/lib/WeBWorK/Utils/Instructor.pm +++ b/lib/WeBWorK/Utils/Instructor.pm @@ -243,45 +243,31 @@ sub assignProblemToUser { sub assignProblemToUserSetVersion { my ($db, $userID, $userSet, $GlobalProblem, $groupProbRef, $seed) = @_; - # conditional to allow selection of problems from a group of problems, - # defined in a set. - - # problem groups are indicated by source files "group:problemGroupName" - if ($GlobalProblem->source_file() =~ /^group:(.+)$/) { + # Conditional to allow selection of problems from a group of problems defined in a set. + # Problem groups are indicated by source files "group:problemGroupName". + if ($GlobalProblem->source_file =~ /^group:(.+)$/) { my $problemGroupName = $1; - # get list of problems in group + # Get the list of problems in the group. my @problemList = $db->listGlobalProblems($problemGroupName); - # sanity check: if the group set hasn't been defined or doesn't - # actually contain problems (oops), then we can't very well assign - # this problem to the user. we could go on and assign all other - # problems, but that results in a partial set. so we die here if - # this happens. philosophically we're requiring that the instructor - # set up the sets correctly or have to deal with the carnage after- - # wards. I'm not sure that this is the best long-term solution. - # FIXME: this means that we may have created a set version that - # doesn't have any problems. this is bad. but it's hard to see - # where else to deal with it---fixing the problem requires checking - # at the set version-creation level that all the problems in the - # set are well defined. FIXME - die("Error in set version creation: no problems are available " - . "in problem group $problemGroupName. Set " - . $userSet->set_id - . " has been created for $userID, but " - . "does not contain the right problems.\n") - if (!@problemList); + + # If the group set is not defined or doesn't actually contain problems, then this problem can not be assigned to + # the user. Continuing to assign the other problems would result in a partial set. So die here if this + # happens. This exception and any others in the set version creation process are handled in the GatewayQuiz.pm + # module, and this set is immediately deleted and a message displayed instructing the user to speak to their + # instructor. It is the instructor's responsibility to fix the issue from there. + die "No problems are available in problem group $problemGroupName.\n" if !@problemList; my $nProb = @problemList; my $whichProblem = int(rand($nProb)); - # we allow selection of multiple problems from a group, but want them to - # be different. there's probably a better way to do this + # Allow selection of multiple problems from a group, but ensure they are different. + # There's probably a better way to do this. if (defined($groupProbRef->{$problemGroupName}) && $groupProbRef->{$problemGroupName} =~ /\b$whichProblem\b/) { - my $nAvail = $nProb - ($groupProbRef->{$problemGroupName} =~ tr/,//) - 1; - - die("Too many problems selected from group.") if (!$nAvail); + die "Too many problems selected from group $problemGroupName.\n" + if !($nProb - ($groupProbRef->{$problemGroupName} =~ tr/,//) - 1); $whichProblem = int(rand($nProb)); while ($groupProbRef->{$problemGroupName} =~ /\b$whichProblem\b/) { @@ -298,7 +284,7 @@ sub assignProblemToUserSetVersion { $GlobalProblem->source_file($prob->source_file()); } - # all set; do problem assignment + # Assign the problem. my $UserProblem = $db->newProblemVersion; $UserProblem->user_id($userID); $UserProblem->set_id($userSet->set_id); @@ -321,7 +307,7 @@ sub assignProblemToUserSetVersion { } } - return (); + return; } =back