diff --git a/lib/WeBWorK/ContentGenerator/Instructor/AddUsers.pm b/lib/WeBWorK/ContentGenerator/Instructor/AddUsers.pm index 90a87a922f..8c27355616 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/AddUsers.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/AddUsers.pm @@ -95,7 +95,7 @@ sub initialize ($c) { } if (defined $c->param('assignSets')) { my @setIDs = $c->param('assignSets'); - assignSetsToUsers($db, \@setIDs, \@userIDs); + assignSetsToUsers($db, $ce, \@setIDs, \@userIDs); } } diff --git a/lib/WeBWorK/ContentGenerator/Instructor/Assigner.pm b/lib/WeBWorK/ContentGenerator/Instructor/Assigner.pm index 1eec98a78a..5b19b94f74 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/Assigner.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/Assigner.pm @@ -45,7 +45,7 @@ sub pre_header_initialize ($c) { if (@selected_users && @selected_sets) { my @results; # This is not used? if (defined $c->param('assign')) { - assignSetsToUsers($db, \@selected_sets, \@selected_users); + assignSetsToUsers($db, $ce, \@selected_sets, \@selected_users); $c->addgoodmessage($c->maketext('All assignments were made successfully.')); } if (defined $c->param('unassign')) { diff --git a/lib/WeBWorK/ContentGenerator/Instructor/UsersAssignedToSet.pm b/lib/WeBWorK/ContentGenerator/Instructor/UsersAssignedToSet.pm index 156885eb43..7049316e98 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/UsersAssignedToSet.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/UsersAssignedToSet.pm @@ -25,7 +25,7 @@ users to which sets are assigned. use WeBWorK::Debug; use WeBWorK::Utils qw(format_set_name_display); -use WeBWorK::Utils::Instructor qw(assignSetToUser assignSetToAllUsers); +use WeBWorK::Utils::Instructor qw(assignSetToAllUsers assignSetToGivenUsers); sub initialize ($c) { my $authz = $c->authz; @@ -70,18 +70,23 @@ sub initialize ($c) { die "Unable to get global set record for $setID " unless $setRecord; my %setUsers = map { $_ => 1 } $db->listSetUsers($setID); + my @usersToAdd; for my $selectedUser (map { $_->user_id } @{ $c->{user_records} }) { if (exists $selectedUsers{$selectedUser}) { unless ($setUsers{$selectedUser}) { # skip users already in the set - debug("assignSetToUser($selectedUser, ...)"); - assignSetToUser($db, $selectedUser, $setRecord); - debug("done assignSetToUser($selectedUser, ...)"); + debug("saving $selectedUser to be added to set later"); + push(@usersToAdd, $selectedUser); } } else { next unless $setUsers{$selectedUser}; # skip users not in the set $db->deleteUserSet($selectedUser, $setID); } } + if (@usersToAdd) { + debug("assignSetToGivenUsers(...)"); + assignSetToGivenUsers($db, $c->ce, $setID, 1, $db->getUsers(@usersToAdd)); + debug("done assignSetToGivenUsers(...)"); + } } return; diff --git a/lib/WeBWorK/DB.pm b/lib/WeBWorK/DB.pm index f6eef63211..c8cdbcfc49 100644 --- a/lib/WeBWorK/DB.pm +++ b/lib/WeBWorK/DB.pm @@ -464,6 +464,48 @@ sub restore_tables { return 1; } +################################################################################ +# transaction support +################################################################################ + +# Any course will have the user table, so that allows getting the database +# handle. + +sub start_transaction { + my $self = shift; + eval { $self->{user}->dbh->begin_work; }; + if ($@) { + my $msg = "Error in start_transaction: $@"; + if ($msg =~ /Already in a transaction/) { + warn "Aborting active transaction."; + $self->{user}->dbh->rollback; + } + croak $msg; + } +} + +sub end_transaction { + my $self = shift; + eval { $self->{user}->dbh->commit; }; + if ($@) { + my $msg = "Error in end_transaction: $@"; + $self->abort_transaction; + croak $msg; + } +} + +sub abort_transaction { + my $self = shift; + eval { + $self->{user}->dbh->{AutoCommit} = 0; + $self->{user}->dbh->rollback; + }; + if ($@) { + my $msg = "Error in abort_transaction: $@"; + croak $msg; + } +} + ################################################################################ # user functions ################################################################################ @@ -1537,6 +1579,41 @@ sub addUserSet { } } +# Used to add a list of UserSet records to the DB in less DB calls. +sub addMultipleUserSets { + my ($self, @userSetList) = @_; + + # Do a single checkArgs call, all records in the list were made in the same manner + $self->checkArgs([ $userSetList[0] ], qw/REC:set_user/); + + my @badUserIDs; + my %badSetIDs; + my @toInsert; + for my $userSet (@userSetList) { + if ($self->{user}->exists($userSet->user_id)) { + if ($self->{set}->exists($userSet->set_id)) { + # This record is OK + push(@toInsert, $userSet); + } else { + $badSetIDs{ $userSet->set_id } = 1; + } + } else { + push(@badUserIDs, join("", 'user ', $userSet->user_id, " not found, cannot add set ", $userSet->set_id)); + } + } + my $badSetIDerrorMsg = + keys(%badSetIDs) ? join(" ", 'bad set IDs seen:', keys(%badSetIDs)) : ""; + + eval { return $self->{set_user}->insert_records([@toInsert]); }; + if ($@) { + croak join("\n", "addMultipleUserSets errors:\nDB call error: $@", $badSetIDerrorMsg, @badUserIDs); + } + + if (@badUserIDs || keys(@badUserIDs)) { + croak join("\n", "addMultipleUserSets errors:\n", $badSetIDerrorMsg, @badUserIDs); + } +} + # the code from putUserSet() is duplicated in large part in the following # putVersionedUserSet; c.f. that routine sub putUserSet { @@ -2053,6 +2130,44 @@ sub addUserProblem { } } +# Used to add multiple problems to a SINGLE set with less DB calls. +# Not for use by versioned sets +# This expects @problemLists to be a 2 dimensional array, each +# entry being the problemList array for one user. +sub addUserMultipleProblems { + my ($self, @problemLists) = @_; + + croak "Error in request to addUserMultipleProblems" if ref($problemLists[0]) ne 'ARRAY'; + + # Do a single checkArgs call, all records in the list were made in the same manner + $self->checkArgs([ $problemLists[0][0] ], qw/REC:problem_user/); + + my $set_id = $problemLists[0][0]->set_id; + + # Array in which all records to add are collected + my @collectedProblemList; + + for my $problemListRef (@problemLists) { + my $user_id = $problemListRef->[0]->user_id; + croak "addMultipleUserProblems: user set $set_id for user $user_id not found" + unless $self->{set_user}->exists($user_id, $set_id); + + # Test whether there are already problem records for this user in this set, as in that case + # inserting multiple records at once would trigger an error. + my $where = [ user_id_eq_set_id_eq => $user_id, $set_id ]; + + croak "addMultipleUserProblems cannot be run as set $set_id already contains some problems for user $user_id" + if $self->{problem_user}->count_where($where); + # If we got here, we can add this list of problems to those to insert into the database + push(@collectedProblemList, @{$problemListRef}); + } + + eval { $self->{problem_user}->insert_records([@collectedProblemList]); }; + if ($@) { + croak "addUserMultipleProblems: $@"; + } +} + # versioned_ok is an optional argument which lets us slip versioned setIDs through checkArgs. sub putUserProblem { my $V = $_[2] ? "V" : ""; diff --git a/lib/WeBWorK/Utils/Instructor.pm b/lib/WeBWorK/Utils/Instructor.pm index 67869dd9a8..27b95340bf 100644 --- a/lib/WeBWorK/Utils/Instructor.pm +++ b/lib/WeBWorK/Utils/Instructor.pm @@ -41,8 +41,10 @@ our @EXPORT_OK = qw( assignAllSetsToUser unassignAllSetsFromUser assignSetsToUsers + assignSetToGivenUsers unassignSetsFromUsers assignProblemToAllSetUsers + assignMultipleProblemsToGivenUsers addProblemToSet read_dir getCSVList @@ -90,12 +92,23 @@ sub assignSetToUser { } } - my @GlobalProblems = grep { defined $_ } $db->getAllGlobalProblems($setID); - foreach my $GlobalProblem (@GlobalProblems) { - my @result = assignProblemToUser($db, $userID, $GlobalProblem); - push @results, @result if @result and not $set_assigned; + my @globalProblemIDs = $db->listGlobalProblems($setID); + + my $result; + # Make the next operation as close to a transaction as possible + eval { + $db->start_transaction; + $result = assignMultipleProblemsToGivenUsers($db, [$userID], $setID, @globalProblemIDs); + $db->end_transaction; + }; + if ($@) { + my $msg = "assignSetToUser: error during asignMultipleProblemsToGivenUsers: $@"; + $db->abort_transaction; + die $msg; } + push @results, $result if $result and not $set_assigned; + return @results; } @@ -153,6 +166,47 @@ sub assignSetVersionToUser { return @results; } +=item assignMultipleProblemsToGivenUsers($db, $userIDsRef, $set_id, @globalProblemIDs) + +Assigns all the problems of the given $set_id to the given users. +The list of users are sent as an array reference +If any assignments fail, an error message is returned. + +=cut + +sub assignMultipleProblemsToGivenUsers { + my ($db, $userIDsRef, $set_id, @globalProblemIDs) = @_; + + if (!@globalProblemIDs) { # When the set is empty there is nothing to do + return; + } + + my @allRecords; + for my $userID (@{$userIDsRef}) { + my @records; + for my $problem_id (@globalProblemIDs) { + my $userProblem = $db->newUserProblem; + $userProblem->user_id($userID); + $userProblem->set_id($set_id); + $userProblem->problem_id($problem_id); + initializeUserProblem($userProblem, undef); # No $seed + push(@records, $userProblem); + } + push(@allRecords, [@records]); + } + + eval { $db->addUserMultipleProblems(@allRecords) }; + if ($@) { + if ($@ =~ m/user problems existed/) { + return "some problem in the set $set_id were already assigned to one of the users being processed.\n $@"; + } else { + die $@; + } + } + + return; +} + =item assignProblemToUser($db, $userID, $GlobalProblem, $seed) Assigns the given problem to the given user. If the problem is already assigned @@ -286,8 +340,10 @@ sub assignProblemToUserSetVersion { =item assignSetToAllUsers($db, $ce, $setID) -Assigns the set specified and all problems contained therein to all users in -the course. This is more efficient than repeatedly calling assignSetToUser(). +Assigns the set specified and all problems contained therein to all users +in the course. This skips users whose status does not have the behavior +include_in_assignment. +This is more efficient than repeatedly calling assignSetToUser(). If any assignments fail, a list of failure messages is returned. =cut @@ -299,35 +355,67 @@ sub assignSetToAllUsers { my @userRecords = $db->getUsersWhere({ user_id => { not_like => 'set_id:%' } }); debug("$setID: (done with that)"); + return assignSetToGivenUsers($db, $ce, $setID, 0, @userRecords); +} + +=item assignSetToGivenUsers($db, $ce, $setID, $alwaysInclude, @userRecords) + +Assigns the set specified and all problems contained therein to all users +in the list provided. +When $alwaysInclude is false, it will skip users whose status does not +have the behavior include_in_assignment. +This is more efficient than repeatedly calling assignSetToUser(). +If any assignments fail, an error message is returned. + +=cut + +sub assignSetToGivenUsers { + my ($db, $ce, $setID, $alwaysInclude, @userRecords) = @_; + debug("$setID: getting problem list"); - my @GlobalProblems = $db->getAllGlobalProblems($setID); + my @globalProblemIDs = $db->listGlobalProblems($setID); debug("$setID: (done with that)"); my @results; + my @userSetsToAdd; + my @usersToProcess; foreach my $User (@userRecords) { - next unless $ce->status_abbrev_has_behavior($User->status, "include_in_assignment"); - my $UserSet = $db->newUserSet; - my $userID = $User->user_id; - $UserSet->user_id($userID); - $UserSet->set_id($setID); - debug("$setID: adding UserSet for $userID"); - eval { $db->addUserSet($UserSet) }; - if ($@) { - next if $@ =~ m/user set exists/; - die $@; - } - debug("$setID: (done with that)"); - - debug("$setID: adding UserProblems for $userID"); - foreach my $GlobalProblem (@GlobalProblems) { - my @result = assignProblemToUser($db, $userID, $GlobalProblem); - push @results, @result if @result; - } - debug("$setID: (done with that)"); + next unless ($alwaysInclude || $ce->status_abbrev_has_behavior($User->status, "include_in_assignment")); + my $userID = $User->user_id; + next if $db->existsUserSet($userID, $setID); + + my $userSet = $db->newUserSet; + $userSet->user_id($userID); + $userSet->set_id($setID); + debug("Scheduled $setID: adding UserSet for $userID"); + push(@userSetsToAdd, $userSet); + push(@usersToProcess, $userID); } + return unless @usersToProcess; # nothing to do - return @results; + # Insert them all at once + eval { + $db->start_transaction; + $db->addMultipleUserSets(@userSetsToAdd); + }; + if ($@) { + my $msg = "assignSetToGivenUsers: error during addMultipleUserSets: $@"; + $db->abort_transaction; + die $msg; + } + # Now add the problem records - as a batch + my $result; + eval { + $result = assignMultipleProblemsToGivenUsers($db, [@usersToProcess], $setID, @globalProblemIDs); + $db->end_transaction; + }; + if ($@) { + my $msg = "assignSetToGivenUsers: error during assignMultipleProblemsToGivenUsers: $@"; + $db->abort_transaction; + die $msg; + } + return $result; } =item unassignSetFromAllUsers($db, $setID) @@ -401,7 +489,7 @@ sub unassignAllSetsFromUser { =over -=item assignSetsToUsers($db, $setIDsRef, $userIDsRef) +=item assignSetsToUsers($db, $ce, $setIDsRef, $userIDsRef) Assign each of the given sets to each of the given users. If any assignments fail, a list of failure messages is returned. @@ -409,19 +497,14 @@ fail, a list of failure messages is returned. =cut sub assignSetsToUsers { - my ($db, $setIDsRef, $userIDsRef) = @_; - - my @setIDs = @$setIDsRef; - my @userIDs = @$userIDsRef; - my @GlobalSets = $db->getGlobalSets(@setIDs); + my ($db, $ce, $setIDsRef, $userIDsRef) = @_; + my @userRecords = $db->getUsers(@$userIDsRef); my @results; - foreach my $GlobalSet (@GlobalSets) { - foreach my $userID (@userIDs) { - my @result = assignSetToUser($db, $userID, $GlobalSet); - push @results, @result if @result; - } + foreach my $setID (@$setIDsRef) { + my $result = assignSetToGivenUsers($db, $ce, $setID, 1, @userRecords); + push @results, $result if $result; } return @results; diff --git a/lib/WebworkWebservice/SetActions.pm b/lib/WebworkWebservice/SetActions.pm index d8116357ed..d63cbc2d2c 100644 --- a/lib/WebworkWebservice/SetActions.pm +++ b/lib/WebworkWebservice/SetActions.pm @@ -24,6 +24,7 @@ use JSON; use Data::Structure::Util qw(unbless); use WeBWorK::Utils qw(max formatDateTime jitar_id_to_seq seq_to_jitar_id); +use WeBWorK::Utils::Instructor qw(assignSetToGivenUsers assignMultipleProblemsToGivenUsers); use WeBWorK::Debug; use WeBWorK::DB::Utils qw(initializeUserProblem); @@ -276,62 +277,23 @@ sub assignSetToUsers { my $setID = $params->{set_id}; my $GlobalSet = $db->getGlobalSet($params->{set_id}); + my %setUsers = map { $_ => 1 } $db->listSetUsers($setID); + debug("users: " . $params->{users}); my @users = split(',', $params->{users}); - + my @usersToAdd; my @results; - for my $userID (@users) { - my $UserSet = $db->newUserSet; - $UserSet->user_id($userID); - $UserSet->set_id($setID); - - my $set_assigned = 0; - - eval { $db->addUserSet($UserSet) }; - if ($@) { - if ($@ =~ m/user set exists/) { - push @results, "set $setID is already assigned to user $userID."; - $set_assigned = 1; - } else { - die $@; - } - } - - my @GlobalProblems = grep { defined $_ } $db->getAllGlobalProblems($setID); - for my $GlobalProblem (@GlobalProblems) { - my @result = assignProblemToUser($self, $userID, $GlobalProblem); - push @results, @result if @result and not $set_assigned; - } - } + for my $user (@users) { + if ($setUsers{$user}) { + push @results, "set $setID is already assigned to user $user."; - return { ra_out => \@results, text => "Successfully assigned users to set $params->{set_id}" }; -} - -#problem utils from Instructor.pm -sub assignProblemToUser { - my ($self, $userID, $GlobalProblem, $seed) = @_; - my $db = $self->db; - - my $UserProblem = $db->newUserProblem; - $UserProblem->user_id($userID); - $UserProblem->set_id($GlobalProblem->set_id); - $UserProblem->problem_id($GlobalProblem->problem_id); - initializeUserProblem($UserProblem, $seed); - - eval { $db->addUserProblem($UserProblem) }; - if ($@) { - if ($@ =~ m/user problem exists/) { - return { text => "problem " - . $GlobalProblem->problem_id - . " in set " - . $GlobalProblem->set_id - . " is already assigned to user $userID." }; } else { - die $@; + push @usersToAdd, $user; } } + push @results, assignSetToGivenUsers($db, $self->ce, $setID, 1, $db->getUsers(@usersToAdd)); - return { text => "Assigned Problem to $userID" }; + return { ra_out => \@results, text => "Successfully assigned users to set $params->{set_id}" }; } sub deleteProblemSet { @@ -554,12 +516,9 @@ sub addProblem { my @results; my @userIDs = $db->listSetUsers($setName); - for my $userID (@userIDs) { - my @result = assignProblemToUser($self, $userID, $problemRecord); - push @results, @result if @result; - } + my $result = assignMultipleProblemsToGivenUsers($db, \@userIDs, $setName, ($problemID)); + push @results, $result if $result; - #assignProblemToAllSetUsers($self, $problemRecord); return { text => "Problem added to $setName" }; }