Database code clean up. #2704
Open
Database code clean up. #2704
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Note that this has no conflicts with #2702. |
drgrice1
force-pushed
the
db-exception-rework
branch
from
37967e0 to
21434ab
Compare
drgrice1
added a commit
to drgrice1/webwork2
that referenced
this pull request
Apr 17, 2025
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
added a commit
to drgrice1/webwork2
that referenced
this pull request
Apr 17, 2025
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
added a commit
to drgrice1/webwork2
that referenced
this pull request
Apr 17, 2025
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
added a commit
to drgrice1/webwork2
that referenced
this pull request
Apr 17, 2025
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
added a commit
to drgrice1/webwork2
that referenced
this pull request
Apr 17, 2025
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
force-pushed
the
db-exception-rework
branch
2 times, most recently
from
b277158 to
7a9d897
Compare
drgrice1
added a commit
to drgrice1/webwork2
that referenced
this pull request
Apr 20, 2025
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
added a commit
to drgrice1/webwork2
that referenced
this pull request
Apr 20, 2025
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
force-pushed
the
db-exception-rework
branch
2 times, most recently
from
df233ff to
a61a043
Compare
drgrice1
added a commit
to drgrice1/webwork2
that referenced
this pull request
Apr 22, 2025
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
force-pushed
the
db-exception-rework
branch
2 times, most recently
from
feaf58e to
7790732
Compare
pstaabp
approved these changes
Apr 26, 2025
drgrice1
added a commit
to drgrice1/webwork2
that referenced
this pull request
Apr 27, 2025
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
force-pushed
the
db-exception-rework
branch
from
7790732 to
2545c81
Compare
drgrice1
added a commit
to drgrice1/webwork2
that referenced
this pull request
Apr 29, 2025
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
force-pushed
the
db-exception-rework
branch
from
2545c81 to
7b7ae4b
Compare
somiaj
approved these changes
May 6, 2025
drgrice1
added a commit
that referenced
this pull request
May 6, 2025
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.
The main thing that this does is implement the `WeBWorK::DB::Ex` exceptions as the comments in `lib/WeBWorK/DB.pm` (that have been there since 2008) say they were intended to be used. For `WeBWorK::DB::Ex::DependencyNotFound` and `WeBWorK::DB::Ex::RecordNotFound` the error message for the exception is the same as before. So for those the only difference is that they are now Exception::Class objects (which contain more information than just the message). For `WeBWorK::DB::Ex::RecordExists` and `WeBWorK::DB::Ex::TableMissing` the error message for the exception is the error message that DBI gives. The previous messages for `WeBWorK::DB::Ex::RecordExists` like "addUserSet: user set exists (perhaps you meant to use putUserSet?)" no longer replace the DBI exception messages. So as the comments from 2008 stated don't check for the string "user set exists" in the exception message anymore. Instead check if `WeBWorK::DB::Ex::RecordExists->caught` is true. In addition the code for auto creation of password records in `getPasswords` and the auto creation of permission records in `getPermissionLevels` has been deleted simply because that code doesn't even work. It hasn't worked for a long time if it ever did. In both methods the `gets` method is called to retrieve the requested records. The `gets` method does not even include non-existent records in the list it returns. So the auto creation loops were only looping over records that were guaranteed to exist. So that code was purely an inneficient loop that did nothing. Next, the `addMultipleUserSets` and `addUserMultipleProblems` methods of `lib/WeBWorK/DB.pm` were removed. Instead what is needed from those methods is used directly in the `lib/WeBWorK/Utils/Instructor.pm` module where those methods were called. Those methods added a high level of inneficiency to the process of assigning sets and problems to multiple users. For example the methods in `lib/WeBWorK/Utils/Instructor.pm` check that the sets don't already exist and if so it skips them. Then the `addMultipleUserSets` method again checks if the sets exist. Note that the previous `lib/WeBWorK/DB.pm` methods did check for the existence of the users, and this no longer does. That check slows things down considerably, and in all cases those users come from the database to begin with. So it is not really a necessary check. This rewrite speeds up the assignment of multiple sets and problems considerably. Finally, the methods in `lib/WeBWorK/Utils/Instructor.pm` no longer return unused lists of failures. In all of the uses of those methods in the webwork code, there was only one place where those return values were ever used, and even in that case it was not in an essential way. Furthermore, that one usage was in `lib/WeBWorK/SetActions.pm` (for the WebworkWebservice) which is not actually used by webwork2 anyway (or probably even by anyone). Instead of catching and rethrowing exceptions, the exceptions are just let through. The exceptions that were ignored before (but put into a return value that was later ignored) are still ignored. The POD in this file is cleaned up as well.
drgrice1
force-pushed
the
db-exception-rework
branch
from
7b7ae4b to
94aafae
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The main thing that this does is implement the
WeBWorK::DB::Exexceptions as the comments inlib/WeBWorK/DB.pm(that have been there since 2008) say they were intended to be used.For
WeBWorK::DB::Ex::DependencyNotFoundandWeBWorK::DB::Ex::RecordNotFoundthe error message for the exception is the same as before. So for those the only difference is that they are now Exception::Class objects (which contain more information than just the message).For
WeBWorK::DB::Ex::RecordExistsandWeBWorK::DB::Ex::TableMissingthe error message for the exception is the error message that DBI gives. The previous messages forWeBWorK::DB::Ex::RecordExistslike "addUserSet: user set exists (perhaps you meant to use putUserSet?)" no longer replace the DBI exception messages. So as the comments from 2008 stated don't check for the string "user set exists" in the exception message anymore. Instead check ifWeBWorK::DB::Ex::RecordExists->caughtis true.In addition the code for auto creation of password records in
getPasswordsand the auto creation of permission records ingetPermissionLevelshas been deleted simply because that code doesn't even work. It hasn't worked for a long time if it ever did. In both methods thegetsmethod is called to retrieve the requested records. Thegetsmethod does not even include non-existent records in the list it returns. So the auto creation loops were only looping over records that were guaranteed to exist. So that code was purely an inefficient loop that did nothing.Next, the
addMultipleUserSetsandaddUserMultipleProblemsmethods oflib/WeBWorK/DB.pmwere removed. Instead what is needed from those methods is used directly in thelib/WeBWorK/Utils/Instructor.pmmodule where those methods were called. Those methods added a high level of inneficiency to the process of assigning sets and problems to multiple users. For example the methods inlib/WeBWorK/Utils/Instructor.pmcheck that the sets don't already exist and if so it skips them. Then theaddMultipleUserSetsmethod again checks if the sets exist. Note that the previouslib/WeBWorK/DB.pmmethods did check for the existence of the users, and this no longer does. That check slows things down considerably, and in all cases those users come from the database to begin with. So it is not really a necessary check. This rewrite speeds up the assignment of multiple sets and problems considerably.Finally, the methods in
lib/WeBWorK/Utils/Instructor.pmno longer return unused lists of failures. In all of the uses of those methods in the webwork code, there was only one place where those return values were ever used, and even in that case it was not in an essential way. Furthermore, that one usage was inlib/WeBWorK/SetActions.pm(for the WebworkWebservice) which is not actually used by webwork2 anyway (or probably even by anyone). Instead of catching and rethrowing exceptions, the exceptions are just let through. The exceptions that were ignored before (but put into a return value that was later ignored) are still ignored. The POD in this file is cleaned up as well.