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