From 74ba4deaccac7adff02c0383844ed8e1e83ee7fd Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Mon, 21 Apr 2025 15:21:42 -0500 Subject: [PATCH] Speed up the achievement users page. The usual thing (by now with this series of pull request) is done. That is removing database access from loops and reducing to single queries as much as possible. In addition instead of using the Mojolicious tag helpers to render the "earned" checkbox and "counter" text field, direct html is used. I discovered that this renders much faster. With the usual 5000 user test the rendering of the template alone takes 5 seconds after changing a checkbox or counter value and saving, while with the direct html it takes something like 0.2 seconds. I believe that it has something to do with their code to set the values of the inputs that is causing the slow down. I am not sure on that, but I know that the direct html is much faster. That database changes are still the biggest part of the speed improvement here in any case. This page was rendering quite slowly even on initial load before, and that part was fast with the tag helpers. After making the database changes things sped up considerably, but then timing parts of the code when saving revealed the tag helper issue. --- .../Instructor/AchievementUserEditor.pm | 121 +++++++++--------- .../Instructor/AchievementUserEditor.html.ep | 23 ++-- 2 files changed, 73 insertions(+), 71 deletions(-) diff --git a/lib/WeBWorK/ContentGenerator/Instructor/AchievementUserEditor.pm b/lib/WeBWorK/ContentGenerator/Instructor/AchievementUserEditor.pm index a0bb119857..733eb7c286 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/AchievementUserEditor.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/AchievementUserEditor.pm @@ -14,105 +14,112 @@ sub initialize ($c) { my $achievementID = $c->stash('achievementID'); my $user = $c->param('user'); - # Make sure this is defined for the template. - $c->stash->{userRecords} = []; + # Make sure these are defined for the template. + $c->stash->{userRecords} = []; + $c->stash->{userAchievementRecords} = []; # Check permissions return unless $authz->hasPermissions($user, 'edit_achievements'); - my @all_users = $db->listUsers; + $c->stash->{userRecords} = + [ $db->getUsersWhere({ user_id => { not_like => 'set_id:%' } }, [qw/section last_name first_name/]) ]; + $c->stash->{userAchievementRecords} = + { map { $_->user_id => $_ } $db->getUserAchievementsWhere({ achievement_id => $achievementID }) }; + my %selectedUsers = map { $_ => 1 } $c->param('selected'); my $doAssignToSelected = 0; - #Check and see if we need to assign or unassign things + # Check and see if we need to assign or unassign achievements. if (defined $c->param('assignToAll')) { $c->addgoodmessage($c->maketext('Achievement has been assigned to all users.')); - %selectedUsers = map { $_ => 1 } @all_users; + %selectedUsers = map { $_->user_id => 1 } @{ $c->stash->{userRecords} }; $doAssignToSelected = 1; } elsif (defined $c->param('unassignFromAll') && defined($c->param('unassignFromAllSafety')) && $c->param('unassignFromAllSafety') == 1) { %selectedUsers = (); - $c->addbadmessage($c->maketext('Achievement has been unassigned to all students.')); + $c->addgoodmessage($c->maketext('Achievement has been unassigned from all users.')); $doAssignToSelected = 1; } elsif (defined $c->param('assignToSelected')) { $c->addgoodmessage($c->maketext('Achievement has been assigned to selected users.')); $doAssignToSelected = 1; } elsif (defined $c->param('unassignFromAll')) { - # no action taken $c->addbadmessage($c->maketext('No action taken')); } - #do actual assignment and unassignment + # Do the actual assignment and unassignment. if ($doAssignToSelected) { + my $achievement = $db->getAchievement($achievementID); + + my %globalUserAchievements = map { $_->user_id => $_ } $db->getGlobalUserAchievementsWhere; - my %achievementUsers = map { $_ => 1 } $db->listAchievementUsers($achievementID); - foreach my $selectedUser (@all_users) { - if (exists $selectedUsers{$selectedUser} && $achievementUsers{$selectedUser}) { - # update existing user data (in case fields were changed) - my $userAchievement = $db->getUserAchievement($selectedUser, $achievementID); + my ( + @userAchievementsToInsert, @userAchievementsToUpdate, @userAchievementsToDelete, + @globalUserAchievementsToInsert, @globalUserAchievementsToUpdate, + ); + + for my $user (@{ $c->stash->{userRecords} }) { + my $userID = $user->user_id; + if ($selectedUsers{$userID} && $c->stash->{userAchievementRecords}{$userID}) { + # Update existing user data (in case fields were changed). + my $updatedEarned = $c->param("$userID.earned") ? 1 : 0; + my $earned = $c->stash->{userAchievementRecords}{$userID}->earned ? 1 : 0; - my $updatedEarned = $c->param("$selectedUser.earned") ? 1 : 0; - my $earned = $userAchievement->earned ? 1 : 0; if ($updatedEarned != $earned) { + $c->stash->{userAchievementRecords}{$userID}->earned($updatedEarned); - $userAchievement->earned($updatedEarned); - my $globalUserAchievement = $db->getGlobalUserAchievement($selectedUser); - my $achievement = $db->getAchievement($achievementID); + my $points = $achievement->points || 0; + my $initialpoints = $globalUserAchievements{$userID}->achievement_points || 0; - my $points = $achievement->points || 0; - my $initialpoints = $globalUserAchievement->achievement_points || 0; - #add the correct number of points if we - # are saying that the user now earned the - # achievement, or remove them otherwise + # Add the correct number of points if we are saying that the + # user now earned the achievement, or remove them otherwise. if ($updatedEarned) { - - $globalUserAchievement->achievement_points($initialpoints + $points); + $globalUserAchievements{$userID}->achievement_points($initialpoints + $points); } else { - $globalUserAchievement->achievement_points($initialpoints - $points); + $globalUserAchievements{$userID}->achievement_points($initialpoints - $points); } - $db->putGlobalUserAchievement($globalUserAchievement); + push(@globalUserAchievementsToUpdate, $globalUserAchievements{$userID}); } - $userAchievement->counter($c->param("$selectedUser.counter")); - $db->putUserAchievement($userAchievement); - - } elsif (exists $selectedUsers{$selectedUser}) { - # add users that dont exist - my $userAchievement = $db->newUserAchievement(); - $userAchievement->user_id($selectedUser); - $userAchievement->achievement_id($achievementID); - $db->addUserAchievement($userAchievement); - - #If they dont have global achievement data, then add that too - if (not $db->existsGlobalUserAchievement($selectedUser)) { - my $globalUserAchievement = $db->newGlobalUserAchievement(); - $globalUserAchievement->user_id($selectedUser); - $db->addGlobalUserAchievement($globalUserAchievement); + my $updatedCounter = $c->param("$userID.counter") // ''; + my $counter = $c->stash->{userAchievementRecords}{$userID}->counter // ''; + $c->stash->{userAchievementRecords}{$userID}->counter($updatedCounter) + if $updatedCounter ne $counter; + + push(@userAchievementsToUpdate, $c->stash->{userAchievementRecords}{$userID}) + if $updatedEarned != $earned || $updatedCounter ne $counter; + } elsif ($selectedUsers{$userID}) { + # Add user achievements that don't exist. + $c->stash->{userAchievementRecords}{$userID} = $db->newUserAchievement; + $c->stash->{userAchievementRecords}{$userID}->user_id($userID); + $c->stash->{userAchievementRecords}{$userID}->achievement_id($achievementID); + push(@userAchievementsToInsert, $c->stash->{userAchievementRecords}{$userID}); + + # If the user does not have global achievement data, then add that too. + if (!$globalUserAchievements{$userID}) { + $globalUserAchievements{$userID} = $db->newGlobalUserAchievement(user_id => $userID); + push(@globalUserAchievementsToInsert, $globalUserAchievements{$userID}); } - } else { - # delete users who are not selected - # but dont delete users who dont exist - next unless $achievementUsers{$selectedUser}; - $db->deleteUserAchievement($selectedUser, $achievementID); + # Delete achievements for users that are not selected, but don't delete achievements that don't exist. + next unless $c->stash->{userAchievementRecords}{$userID}; + push(@userAchievementsToDelete, $c->stash->{userAchievementRecords}{$userID}); + delete $c->stash->{userAchievementRecords}{$userID}; } } - } - my @userRecords; - for my $currentUser (@all_users) { - my $userObj = $c->db->getUser($currentUser); - die "Unable to find user object for $currentUser. " unless $userObj; - push(@userRecords, $userObj); - } - @userRecords = - sort { (lc($a->section) cmp lc($b->section)) || (lc($a->last_name) cmp lc($b->last_name)) } @userRecords; + $db->GlobalUserAchievement->insert_records(\@globalUserAchievementsToInsert) if @globalUserAchievementsToInsert; + $db->GlobalUserAchievement->update_records(\@globalUserAchievementsToUpdate) if @globalUserAchievementsToUpdate; + $db->UserAchievement->insert_records(\@userAchievementsToInsert) if @userAchievementsToInsert; + $db->UserAchievement->update_records(\@userAchievementsToUpdate) if @userAchievementsToUpdate; - $c->stash->{userRecords} = \@userRecords; + # This is one of the rare places this can be done since user achievements don't + # have any dependent rows in other tables that also need to be deleted. + $db->UserAchievement->delete_records(\@userAchievementsToDelete) if @userAchievementsToDelete; + } return; } diff --git a/templates/ContentGenerator/Instructor/AchievementUserEditor.html.ep b/templates/ContentGenerator/Instructor/AchievementUserEditor.html.ep index de609f50c9..48a0776d88 100644 --- a/templates/ContentGenerator/Instructor/AchievementUserEditor.html.ep +++ b/templates/ContentGenerator/Instructor/AchievementUserEditor.html.ep @@ -3,8 +3,6 @@ % last; % } % -% my $achievementID = stash('achievementID'); -% <%= form_for current_route, method => 'post', name => 'user-achievement-form', id => 'user-achievement-form', begin =%> % # Assign to everyone message
@@ -39,31 +37,28 @@ % # Output row for user % for my $userRecord (@$userRecords) { - % my $statusClass = $ce->status_abbrev_to_name($userRecord->status) || ''; % my $user = $userRecord->user_id; - % my $userAchievement = $db->getUserAchievement($user, $achievementID); + % my $userAchievement = $userAchievementRecords->{$user}; % my $prettyName = $userRecord->last_name . ', ' . $userRecord->first_name; % > + class="form-check-input" <%= $userAchievement ? 'checked' : '' %>> <%= label_for "$user.assigned" => $user %> <%= $prettyName %> <%= $userRecord->section %> - % if (defined $userAchievement) { + % if ($userAchievement) { - <%= check_box "$user.earned" => '1', - 'aria-labelledby' => 'earned_header', - class => 'form-check-input', - (ref $userAchievement ? $userAchievement->earned : 0) ? (checked => undef) : () =%> + earned ? 'checked' : '' %>> - <%= text_field "$user.counter" => ref $userAchievement ? $userAchievement->counter : 0, - 'aria-labelledby' => 'counter_header', - size => 6, - class => 'form-control form-control-sm' =%> + % } else {