Skip to content

Fix classlist exporting with users that do not have passwords. #2705

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

drgrice1
Copy link
Member

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.

@drgrice1 drgrice1 force-pushed the classlist-export-w-missing-passwords branch 5 times, most recently from 17f093f to 50a72c4 Compare April 20, 2025 21:10
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 openwebwork#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.
@drgrice1 drgrice1 force-pushed the classlist-export-w-missing-passwords branch from 50a72c4 to e8300a7 Compare April 22, 2025 10:27
Copy link
Member

@pstaabp pstaabp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested that the code in develop does not export passwords correctly and that this does fix the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants