Skip to content

Commit e8300a7

Browse files
committed
Fix classlist exporting with users that do not have passwords.
The `exportUsersToCSV` method of `WeBWorK::ContentGenerator::Instructor::UserList` assumes that all users have password and permission records in the database. Since that is no longer the case, that code is not working right. In fact if some users do not have password records and others do, then the resulting classlist that is exported will have many passwords in associated to the wrong users. Some of the users that don't have passwords will have passwords, and vice versa. Note that the code had `defined` statements checking if the permission and password records exist, which mean that the code was technically incorrect to begin with. It only worked because usually all users had password and permission records. The general issue with this is noted in #2704 with regards to the deletion of the code that auto creates password or permission records when `getPasswords` or `getPermissionLevels` is called. The general issue is that the `getPasswords` and `getPermissionLevels` methods call the `WeBWorK::DB::Schema::NewSQL::Std::gets` method which does not even include non-existent records. So for example, if `getPasswords` is called with the list of user IDs `'user1', 'user2', 'user3'` and `user2` does not have a password, but the others do, then `getPasswords` will return an array with two elements which are the password records for `user1` and `user3`. So when iterating by index on the original list the password record for `user1` will correctly go to `user1`, but the password record for `user3` will be paired with `user2`, and `user3` will not have a password record. Also, the `exportUsersToCSV` dies if the provided filename contains a forward slash. So now the JavaScript form validation checks for this, and prevents those filenames from being submitted to the server. Thus preventing the `die` statement from occurring.
1 parent 688fd34 commit e8300a7

File tree

3 files changed

+20
-19
lines changed

3 files changed

+20
-19
lines changed

htdocs/js/UserList/userlist.js

+4-1
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,10 @@
113113
e.preventDefault();
114114
e.stopPropagation();
115115
show_errors(['select_user_err_msg'], [export_select]);
116-
} else if (export_select_target?.value === 'new' && export_filename.value === '') {
116+
} else if (
117+
export_select_target?.value === 'new' &&
118+
(export_filename.value === '' || /\//.test(export_filename.value))
119+
) {
117120
e.preventDefault();
118121
e.stopPropagation();
119122
show_errors(['export_file_err_msg'], [export_filename, export_select_target]);

lib/WeBWorK/ContentGenerator/Instructor/UserList.pm

+15-17
Original file line numberDiff line numberDiff line change
@@ -726,31 +726,29 @@ sub importUsersFromCSV ($c, $fileName, $replaceExisting, $fallbackPasswordSource
726726
}
727727

728728
sub exportUsersToCSV ($c, $fileName, @userIDsToExport) {
729-
my $ce = $c->ce;
730-
my $db = $c->db;
731-
my $dir = $ce->{courseDirs}->{templates};
729+
my $ce = $c->ce;
730+
my $db = $c->db;
732731

733-
die $c->maketext("illegal character in input: '/'") if $fileName =~ m|/|;
732+
die $c->maketext(q{illegal character in input: '/'}) if $fileName =~ m|/|;
734733

735734
my @records;
736735

737-
my @Users = $db->getUsers(@userIDsToExport);
738-
my @Passwords = $db->getPasswords(@userIDsToExport);
739-
my @PermissionLevels = $db->getPermissionLevels(@userIDsToExport);
740-
foreach my $i (0 .. $#userIDsToExport) {
741-
my $User = $Users[$i];
742-
my $Password = $Passwords[$i];
743-
my $PermissionLevel = $PermissionLevels[$i];
744-
next unless defined $User;
745-
my %record = (
746-
defined $PermissionLevel ? $PermissionLevel->toHash : (),
747-
defined $Password ? $Password->toHash : (),
748-
$User->toHash,
736+
my @users = $db->getUsers(@userIDsToExport);
737+
my %passwords = map { $_->user_id => $_ } $db->getPasswords(@userIDsToExport);
738+
my %permissionLevels = map { $_->user_id => $_ } $db->getPermissionLevels(@userIDsToExport);
739+
740+
for my $user (@users) {
741+
my $password = $passwords{ $user->user_id };
742+
my $permissionLevel = $permissionLevels{ $user->user_id };
743+
my %record = (
744+
defined $permissionLevel ? $permissionLevel->toHash : (),
745+
defined $password ? $password->toHash : (),
746+
$user->toHash,
749747
);
750748
push @records, \%record;
751749
}
752750

753-
write_classlist("$dir/$fileName", @records);
751+
write_classlist("$ce->{courseDirs}{templates}/$fileName", @records);
754752

755753
return;
756754
}

templates/ContentGenerator/Instructor/UserList/export_form.html.ep

+1-1
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,6 @@
3434
</div>
3535
</div>
3636
<div id="export_file_err_msg" class="alert alert-danger p-1 d-inline-flex d-none">
37-
<%= maketext('Please input file name to export to.') %>
37+
<%= maketext('Please input a file name to export to that does not contain forward slashes.') %>
3838
</div>
3939
</div>

0 commit comments

Comments
 (0)