Skip to content

Commit c4fa2d0

Browse files
drgrice1drgrice1
committed
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.
1 parent f18833f commit c4fa2d0

File tree

2 files changed

+73
-71
lines changed

2 files changed

+73
-71
lines changed

‎lib/WeBWorK/ContentGenerator/Instructor/AchievementUserEditor.pm

+64-57
Original file line numberDiff line numberDiff line change
@@ -29,105 +29,112 @@ sub initialize ($c) {
2929
my $achievementID = $c->stash('achievementID');
3030
my $user = $c->param('user');
3131

32-
# Make sure this is defined for the template.
33-
$c->stash->{userRecords} = [];
32+
# Make sure these are defined for the template.
33+
$c->stash->{userRecords} = [];
34+
$c->stash->{userAchievementRecords} = [];
3435

3536
# Check permissions
3637
return unless $authz->hasPermissions($user, 'edit_achievements');
3738

38-
my @all_users = $db->listUsers;
39+
$c->stash->{userRecords} =
40+
[ $db->getUsersWhere({ user_id => { not_like => 'set_id:%' } }, [qw/section last_name first_name/]) ];
41+
$c->stash->{userAchievementRecords} =
42+
{ map { $_->user_id => $_ } $db->getUserAchievementsWhere({ achievement_id => $achievementID }) };
43+
3944
my %selectedUsers = map { $_ => 1 } $c->param('selected');
4045

4146
my $doAssignToSelected = 0;
4247

43-
#Check and see if we need to assign or unassign things
48+
# Check and see if we need to assign or unassign achievements.
4449
if (defined $c->param('assignToAll')) {
4550
$c->addgoodmessage($c->maketext('Achievement has been assigned to all users.'));
46-
%selectedUsers = map { $_ => 1 } @all_users;
51+
%selectedUsers = map { $_->user_id => 1 } @{ $c->stash->{userRecords} };
4752
$doAssignToSelected = 1;
4853
} elsif (defined $c->param('unassignFromAll')
4954
&& defined($c->param('unassignFromAllSafety'))
5055
&& $c->param('unassignFromAllSafety') == 1)
5156
{
5257
%selectedUsers = ();
53-
$c->addbadmessage($c->maketext('Achievement has been unassigned to all students.'));
58+
$c->addgoodmessage($c->maketext('Achievement has been unassigned from all users.'));
5459
$doAssignToSelected = 1;
5560
} elsif (defined $c->param('assignToSelected')) {
5661
$c->addgoodmessage($c->maketext('Achievement has been assigned to selected users.'));
5762
$doAssignToSelected = 1;
5863
} elsif (defined $c->param('unassignFromAll')) {
59-
# no action taken
6064
$c->addbadmessage($c->maketext('No action taken'));
6165
}
6266

63-
#do actual assignment and unassignment
67+
# Do the actual assignment and unassignment.
6468
if ($doAssignToSelected) {
69+
my $achievement = $db->getAchievement($achievementID);
70+
71+
my %globalUserAchievements = map { $_->user_id => $_ } $db->getGlobalUserAchievementsWhere;
6572

66-
my %achievementUsers = map { $_ => 1 } $db->listAchievementUsers($achievementID);
67-
foreach my $selectedUser (@all_users) {
68-
if (exists $selectedUsers{$selectedUser} && $achievementUsers{$selectedUser}) {
69-
# update existing user data (in case fields were changed)
70-
my $userAchievement = $db->getUserAchievement($selectedUser, $achievementID);
73+
my (
74+
@userAchievementsToInsert, @userAchievementsToUpdate, @userAchievementsToDelete,
75+
@globalUserAchievementsToInsert, @globalUserAchievementsToUpdate,
76+
);
77+
78+
for my $user (@{ $c->stash->{userRecords} }) {
79+
my $userID = $user->user_id;
80+
if ($selectedUsers{$userID} && $c->stash->{userAchievementRecords}{$userID}) {
81+
# Update existing user data (in case fields were changed).
82+
my $updatedEarned = $c->param("$userID.earned") ? 1 : 0;
83+
my $earned = $c->stash->{userAchievementRecords}{$userID}->earned ? 1 : 0;
7184

72-
my $updatedEarned = $c->param("$selectedUser.earned") ? 1 : 0;
73-
my $earned = $userAchievement->earned ? 1 : 0;
7485
if ($updatedEarned != $earned) {
86+
$c->stash->{userAchievementRecords}{$userID}->earned($updatedEarned);
7587

76-
$userAchievement->earned($updatedEarned);
77-
my $globalUserAchievement = $db->getGlobalUserAchievement($selectedUser);
78-
my $achievement = $db->getAchievement($achievementID);
88+
my $points = $achievement->points || 0;
89+
my $initialpoints = $globalUserAchievements{$userID}->achievement_points || 0;
7990

80-
my $points = $achievement->points || 0;
81-
my $initialpoints = $globalUserAchievement->achievement_points || 0;
82-
#add the correct number of points if we
83-
# are saying that the user now earned the
84-
# achievement, or remove them otherwise
91+
# Add the correct number of points if we are saying that the
92+
# user now earned the achievement, or remove them otherwise.
8593
if ($updatedEarned) {
86-
87-
$globalUserAchievement->achievement_points($initialpoints + $points);
94+
$globalUserAchievements{$userID}->achievement_points($initialpoints + $points);
8895
} else {
89-
$globalUserAchievement->achievement_points($initialpoints - $points);
96+
$globalUserAchievements{$userID}->achievement_points($initialpoints - $points);
9097
}
9198

92-
$db->putGlobalUserAchievement($globalUserAchievement);
99+
push(@globalUserAchievementsToUpdate, $globalUserAchievements{$userID});
93100
}
94101

95-
$userAchievement->counter($c->param("$selectedUser.counter"));
96-
$db->putUserAchievement($userAchievement);
97-
98-
} elsif (exists $selectedUsers{$selectedUser}) {
99-
# add users that dont exist
100-
my $userAchievement = $db->newUserAchievement();
101-
$userAchievement->user_id($selectedUser);
102-
$userAchievement->achievement_id($achievementID);
103-
$db->addUserAchievement($userAchievement);
104-
105-
#If they dont have global achievement data, then add that too
106-
if (not $db->existsGlobalUserAchievement($selectedUser)) {
107-
my $globalUserAchievement = $db->newGlobalUserAchievement();
108-
$globalUserAchievement->user_id($selectedUser);
109-
$db->addGlobalUserAchievement($globalUserAchievement);
102+
my $updatedCounter = $c->param("$userID.counter") // '';
103+
my $counter = $c->stash->{userAchievementRecords}{$userID}->counter // '';
104+
$c->stash->{userAchievementRecords}{$userID}->counter($updatedCounter)
105+
if $updatedCounter ne $counter;
106+
107+
push(@userAchievementsToUpdate, $c->stash->{userAchievementRecords}{$userID})
108+
if $updatedEarned != $earned || $updatedCounter ne $counter;
109+
} elsif ($selectedUsers{$userID}) {
110+
# Add user achievements that don't exist.
111+
$c->stash->{userAchievementRecords}{$userID} = $db->newUserAchievement;
112+
$c->stash->{userAchievementRecords}{$userID}->user_id($userID);
113+
$c->stash->{userAchievementRecords}{$userID}->achievement_id($achievementID);
114+
push(@userAchievementsToInsert, $c->stash->{userAchievementRecords}{$userID});
115+
116+
# If the user does not have global achievement data, then add that too.
117+
if (!$globalUserAchievements{$userID}) {
118+
$globalUserAchievements{$userID} = $db->newGlobalUserAchievement(user_id => $userID);
119+
push(@globalUserAchievementsToInsert, $globalUserAchievements{$userID});
110120
}
111-
112121
} else {
113-
# delete users who are not selected
114-
# but dont delete users who dont exist
115-
next unless $achievementUsers{$selectedUser};
116-
$db->deleteUserAchievement($selectedUser, $achievementID);
122+
# Delete achievements for users that are not selected, but don't delete achievements that don't exist.
123+
next unless $c->stash->{userAchievementRecords}{$userID};
124+
push(@userAchievementsToDelete, $c->stash->{userAchievementRecords}{$userID});
125+
delete $c->stash->{userAchievementRecords}{$userID};
117126
}
118127
}
119-
}
120128

121-
my @userRecords;
122-
for my $currentUser (@all_users) {
123-
my $userObj = $c->db->getUser($currentUser);
124-
die "Unable to find user object for $currentUser. " unless $userObj;
125-
push(@userRecords, $userObj);
126-
}
127-
@userRecords =
128-
sort { (lc($a->section) cmp lc($b->section)) || (lc($a->last_name) cmp lc($b->last_name)) } @userRecords;
129+
$db->GlobalUserAchievement->insert_records(\@globalUserAchievementsToInsert) if @globalUserAchievementsToInsert;
130+
$db->GlobalUserAchievement->update_records(\@globalUserAchievementsToUpdate) if @globalUserAchievementsToUpdate;
131+
$db->UserAchievement->insert_records(\@userAchievementsToInsert) if @userAchievementsToInsert;
132+
$db->UserAchievement->update_records(\@userAchievementsToUpdate) if @userAchievementsToUpdate;
129133

130-
$c->stash->{userRecords} = \@userRecords;
134+
# This is one of the rare places this can be done since user achievements don't
135+
# have any dependent rows in other tables that also need to be deleted.
136+
$db->UserAchievement->delete_records(\@userAchievementsToDelete) if @userAchievementsToDelete;
137+
}
131138

132139
return;
133140
}

‎templates/ContentGenerator/Instructor/AchievementUserEditor.html.ep

+9-14
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,6 @@
33
% last;
44
% }
55
%
6-
% my $achievementID = stash('achievementID');
7-
%
86
<%= form_for current_route, method => 'post', name => 'user-achievement-form', id => 'user-achievement-form', begin =%>
97
% # Assign to everyone message
108
<div class="my-2">
@@ -39,31 +37,28 @@
3937
<tbody class="table-group-divider">
4038
% # Output row for user
4139
% for my $userRecord (@$userRecords) {
42-
% my $statusClass = $ce->status_abbrev_to_name($userRecord->status) || '';
4340
% my $user = $userRecord->user_id;
44-
% my $userAchievement = $db->getUserAchievement($user, $achievementID);
41+
% my $userAchievement = $userAchievementRecords->{$user};
4542
% my $prettyName = $userRecord->last_name . ', ' . $userRecord->first_name;
4643
%
4744
<tr>
4845
<td class="text-center">
4946
<input type="checkbox" name="selected" id="<%= $user %>.assigned" value="<%= $user %>"
50-
class="form-check-input" <%= defined $userAchievement ? 'checked' : '' %>>
47+
class="form-check-input" <%= $userAchievement ? 'checked' : '' %>>
5148
</td>
5249
<td><%= label_for "$user.assigned" => $user %></td>
5350
<td><%= $prettyName %></td>
5451
<td class="text-center"><%= $userRecord->section %></td>
55-
% if (defined $userAchievement) {
52+
% if ($userAchievement) {
5653
<td class="text-center">
57-
<%= check_box "$user.earned" => '1',
58-
'aria-labelledby' => 'earned_header',
59-
class => 'form-check-input',
60-
(ref $userAchievement ? $userAchievement->earned : 0) ? (checked => undef) : () =%>
54+
<input type="checkbox" name="<%= $user %>.earned" value="1"
55+
aria-labelledby="earned_header" class="form-check-input"
56+
<%= $userAchievement->earned ? 'checked' : '' %>>
6157
</td>
6258
<td class="text-center">
63-
<%= text_field "$user.counter" => ref $userAchievement ? $userAchievement->counter : 0,
64-
'aria-labelledby' => 'counter_header',
65-
size => 6,
66-
class => 'form-control form-control-sm' =%>
59+
<input name="<%= $user %>.counter" value="<%= $userAchievement->counter %>"
60+
type="number" min="0" aria-labelledby="counter_header" size="6"
61+
class="form-control form-control-sm">
6762
</td>
6863
% } else {
6964
<td></td><td></td>

0 commit comments

Comments
 (0)