Skip to content

Commit 27957b1

Browse files
drgrice1drgrice1
committed
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 openwebwork#2669 and probably the best way without massively revising the GatewayQuiz module.
1 parent 28b21ed commit 27957b1

File tree

2 files changed

+77
-78
lines changed

2 files changed

+77
-78
lines changed

‎lib/WeBWorK/ContentGenerator/GatewayQuiz.pm

+60-47
Original file line numberDiff line numberDiff line change
@@ -550,55 +550,68 @@ async sub pre_header_initialize ($c) {
550550
)
551551
)
552552
{
553-
# Assign the set, get the right name, version number, etc., and redefine the $set and $problem for the
554-
# remainder of this method.
553+
# Attempt to assign the set.
555554
my $setTmpl = $db->getUserSet($effectiveUserID, $setID);
556-
assignSetVersionToUser($db, $effectiveUserID, $setTmpl);
557-
$setVersionNumber++;
558-
559-
# Get a clean version of the set and merged version to use in the rest of the routine.
560-
my $cleanSet = $db->getSetVersion($effectiveUserID, $setID, $setVersionNumber);
561-
$set = $db->getMergedSetVersion($effectiveUserID, $setID, $setVersionNumber);
562-
$set->visible(1);
563-
564-
$problem = $db->getMergedProblemVersion($effectiveUserID, $setID, $setVersionNumber, $setPNum[0]);
565-
566-
# Convert the floating point value from Time::HiRes to an integer for use below. Truncate toward 0.
567-
my $timeNowInt = int($c->submitTime);
568-
569-
# Set up creation time, and open and due dates.
570-
my $ansOffset = $set->answer_date - $set->due_date;
571-
$set->version_creation_time($timeNowInt);
572-
$set->open_date($timeNowInt);
573-
# Figure out the due date, taking into account the time limit cap.
574-
my $dueTime =
575-
$timeLimit == 0 || ($set->time_limit_cap && $c->submitTime + $timeLimit > $set->due_date)
576-
? $set->due_date
577-
: $timeNowInt + $timeLimit;
578-
579-
$set->due_date($dueTime);
580-
$set->answer_date($set->due_date + $ansOffset);
581-
$set->version_last_attempt_time(0);
582-
583-
# Put this new info into the database. Put back the data needed for the version, and leave blank any
584-
# information that should be inherited from the user set or global set. Set the data which determines
585-
# if a set is open, because a set version should not reopen after it's complete.
586-
$cleanSet->version_creation_time($set->version_creation_time);
587-
$cleanSet->open_date($set->open_date);
588-
$cleanSet->due_date($set->due_date);
589-
$cleanSet->answer_date($set->answer_date);
590-
$cleanSet->version_last_attempt_time($set->version_last_attempt_time);
591-
$cleanSet->version_time_limit($set->version_time_limit);
592-
$cleanSet->attempts_per_version($set->attempts_per_version);
593-
$cleanSet->assignment_type($set->assignment_type);
594-
$db->putSetVersion($cleanSet);
595-
596-
# This is a new set version, so it's open.
597-
$versionIsOpen = 1;
598-
599-
# Set the number of attempts for this set to zero.
600-
$currentNumAttempts = 0;
555+
eval { assignSetVersionToUser($db, $effectiveUserID, $setTmpl) };
601556

557+
if ($@) {
558+
$c->log->error("Error creating test version of $setID for $effectiveUserID: $@");
559+
$c->{invalidSet} =
560+
$c->maketext('Unable to generate a valid test version. This is usually caused by invalid '
561+
. 'usage of grouping sets or a database error. Please speak to your instructor to fix the '
562+
. 'error. A system administrator can obtain more details on this error from the logs.');
563+
# Attempt to delete the set version if it was created. Failure from this attempt is ignored.
564+
eval { $db->deleteSetVersion($userID, $setID, $setVersionNumber + 1) }
565+
if $db->existsSetVersion($userID, $setID, $setVersionNumber + 1);
566+
} else {
567+
# Get the right name, version number, etc., and redefine the
568+
# $set and $problem for the remainder of this method.
569+
570+
++$setVersionNumber;
571+
572+
# Get a clean version of the set and merged version to use in the rest of the routine.
573+
my $cleanSet = $db->getSetVersion($effectiveUserID, $setID, $setVersionNumber);
574+
$set = $db->getMergedSetVersion($effectiveUserID, $setID, $setVersionNumber);
575+
$set->visible(1);
576+
577+
$problem = $db->getMergedProblemVersion($effectiveUserID, $setID, $setVersionNumber, $setPNum[0]);
578+
579+
# Convert the floating point value from Time::HiRes to an integer for use below. Truncate toward 0.
580+
my $timeNowInt = int($c->submitTime);
581+
582+
# Set up creation time, and open and due dates.
583+
my $ansOffset = $set->answer_date - $set->due_date;
584+
$set->version_creation_time($timeNowInt);
585+
$set->open_date($timeNowInt);
586+
# Figure out the due date, taking into account the time limit cap.
587+
my $dueTime =
588+
$timeLimit == 0 || ($set->time_limit_cap && $c->submitTime + $timeLimit > $set->due_date)
589+
? $set->due_date
590+
: $timeNowInt + $timeLimit;
591+
592+
$set->due_date($dueTime);
593+
$set->answer_date($set->due_date + $ansOffset);
594+
$set->version_last_attempt_time(0);
595+
596+
# Put this new info into the database. Put back the data needed for the version, and leave blank
597+
# any information that should be inherited from the user set or global set. Set the data which
598+
# determines if a set is open, because a set version should not reopen after it's complete.
599+
$cleanSet->version_creation_time($set->version_creation_time);
600+
$cleanSet->open_date($set->open_date);
601+
$cleanSet->due_date($set->due_date);
602+
$cleanSet->answer_date($set->answer_date);
603+
$cleanSet->version_last_attempt_time($set->version_last_attempt_time);
604+
$cleanSet->version_time_limit($set->version_time_limit);
605+
$cleanSet->attempts_per_version($set->attempts_per_version);
606+
$cleanSet->assignment_type($set->assignment_type);
607+
$db->putSetVersion($cleanSet);
608+
609+
# This is a new set version, so it's open.
610+
$versionIsOpen = 1;
611+
612+
# Set the number of attempts for this set to zero.
613+
$currentNumAttempts = 0;
614+
}
602615
} elsif ($maxAttempts != -1 && $totalNumVersions > $maxAttempts) {
603616
$c->{invalidSet} = $c->maketext('No new versions of this test are available, '
604617
. 'because you have already taken the maximum number allowed.');

‎lib/WeBWorK/Utils/Instructor.pm

+17-31
Original file line numberDiff line numberDiff line change
@@ -243,45 +243,31 @@ sub assignProblemToUser {
243243
sub assignProblemToUserSetVersion {
244244
my ($db, $userID, $userSet, $GlobalProblem, $groupProbRef, $seed) = @_;
245245

246-
# conditional to allow selection of problems from a group of problems,
247-
# defined in a set.
248-
249-
# problem groups are indicated by source files "group:problemGroupName"
250-
if ($GlobalProblem->source_file() =~ /^group:(.+)$/) {
246+
# Conditional to allow selection of problems from a group of problems defined in a set.
247+
# Problem groups are indicated by source files "group:problemGroupName".
248+
if ($GlobalProblem->source_file =~ /^group:(.+)$/) {
251249
my $problemGroupName = $1;
252250

253-
# get list of problems in group
251+
# Get the list of problems in the group.
254252
my @problemList = $db->listGlobalProblems($problemGroupName);
255-
# sanity check: if the group set hasn't been defined or doesn't
256-
# actually contain problems (oops), then we can't very well assign
257-
# this problem to the user. we could go on and assign all other
258-
# problems, but that results in a partial set. so we die here if
259-
# this happens. philosophically we're requiring that the instructor
260-
# set up the sets correctly or have to deal with the carnage after-
261-
# wards. I'm not sure that this is the best long-term solution.
262-
# FIXME: this means that we may have created a set version that
263-
# doesn't have any problems. this is bad. but it's hard to see
264-
# where else to deal with it---fixing the problem requires checking
265-
# at the set version-creation level that all the problems in the
266-
# set are well defined. FIXME
267-
die("Error in set version creation: no problems are available "
268-
. "in problem group $problemGroupName. Set "
269-
. $userSet->set_id
270-
. " has been created for $userID, but "
271-
. "does not contain the right problems.\n")
272-
if (!@problemList);
253+
254+
# If the group set is not defined or doesn't actually contain problems, then this problem can not be assigned to
255+
# the user. Continuing to assign the other problems would result in a partial set. So die here if this
256+
# happens. This exception and any others in the set version creation process are handled in the GatewayQuiz.pm
257+
# module, and this set is immediately deleted and a message displayed instructing the user to speak to their
258+
# instructor. It is the instructor's responsibility to fix the issue from there.
259+
die "No problems are available in problem group $problemGroupName.\n" if !@problemList;
273260

274261
my $nProb = @problemList;
275262
my $whichProblem = int(rand($nProb));
276263

277-
# we allow selection of multiple problems from a group, but want them to
278-
# be different. there's probably a better way to do this
264+
# Allow selection of multiple problems from a group, but ensure they are different.
265+
# There's probably a better way to do this.
279266
if (defined($groupProbRef->{$problemGroupName})
280267
&& $groupProbRef->{$problemGroupName} =~ /\b$whichProblem\b/)
281268
{
282-
my $nAvail = $nProb - ($groupProbRef->{$problemGroupName} =~ tr/,//) - 1;
283-
284-
die("Too many problems selected from group.") if (!$nAvail);
269+
die "Too many problems selected from group $problemGroupName.\n"
270+
if !($nProb - ($groupProbRef->{$problemGroupName} =~ tr/,//) - 1);
285271

286272
$whichProblem = int(rand($nProb));
287273
while ($groupProbRef->{$problemGroupName} =~ /\b$whichProblem\b/) {
@@ -298,7 +284,7 @@ sub assignProblemToUserSetVersion {
298284
$GlobalProblem->source_file($prob->source_file());
299285
}
300286

301-
# all set; do problem assignment
287+
# Assign the problem.
302288
my $UserProblem = $db->newProblemVersion;
303289
$UserProblem->user_id($userID);
304290
$UserProblem->set_id($userSet->set_id);
@@ -321,7 +307,7 @@ sub assignProblemToUserSetVersion {
321307
}
322308
}
323309

324-
return ();
310+
return;
325311
}
326312

327313
=back

0 commit comments

Comments
 (0)