From da3b7397bb6d14249fd63ec83c47d54a1bae0a84 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Sun, 20 Apr 2025 09:21:48 -0500 Subject: [PATCH 001/186] Improve loading time for the achievements leaderboard. Currently the user achievements are obtained from the database in the user loop. This takes a long time when there are lots of users. So instead, this gets all user achievements for all users before the loop and references them by user id and then achievement id. Testing this on a course for 5000 users shows this gives a significant speed up on load time for the page. With the develop branch it takes around 25 seconds, and with this branch it takes around 3 seconds. Note that there were also two redundant queries (one that listed all achievements and then one that fetched all achievements on lines 49 and 50 in the develop branch) for which the data from those queries was not even used. Those were removed. --- .../AchievementsLeaderboard.pm | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/lib/WeBWorK/ContentGenerator/AchievementsLeaderboard.pm b/lib/WeBWorK/ContentGenerator/AchievementsLeaderboard.pm index 2c2ba971d4..371a66f57f 100644 --- a/lib/WeBWorK/ContentGenerator/AchievementsLeaderboard.pm +++ b/lib/WeBWorK/ContentGenerator/AchievementsLeaderboard.pm @@ -46,37 +46,40 @@ sub initialize ($c) { my %globalUserAchievements = map { $_->user_id => $_ } $db->getGlobalUserAchievementsWhere({ user_id => { not_like => 'set_id:%' } }); - my @allBadgeIDs = $db->listAchievements; - my @allBadges = @allBadgeIDs ? sortAchievements($db->getAchievements(@allBadgeIDs)) : (); - $c->{showUserNames} = $c->authz->hasPermissions($c->{userName}, 'view_leaderboard_usernames'); $c->{showLevels} = 0; # Hide level column unless at least one user has a level achievement. + my %allUserAchievements; + for ($db->getUserAchievementsWhere({ + user_id => { not_like => 'set_id:%' }, + achievement_id => [ map { $_->achievement_id } grep { $_->category ne 'level' } @achievements ], + })) + { + $allUserAchievements{ $_->user_id }{ $_->achievement_id } = $_; + } + my @rows; for my $user ($db->getUsersWhere({ user_id => { not_like => 'set_id:%' } })) { # Only include users who can be shown in stats. next unless $ce->status_abbrev_has_behavior($user->status, 'include_in_stats'); + my $globalData = $globalUserAchievements{ $user->user_id }; + my $userAchievements = $allUserAchievements{ $user->user_id }; + # Skip unless user has achievement data. - my $globalData = $globalUserAchievements{ $user->user_id }; - next unless $globalData; + next unless $globalData && $userAchievements; my $level = $globalData->level_achievement_id ? $achievementsById{ $globalData->level_achievement_id } : ''; - my %userAchievements = map { $_->achievement_id => $_ } $db->getUserAchievementsWhere({ - user_id => $user->user_id, - achievement_id => [ map { $_->achievement_id } grep { $_->category ne 'level' } @achievements ], - }); - my @badges; for my $achievement (@achievements) { # Skip level achievements and only show earned achievements. last if $achievement->category eq 'level'; push(@badges, $achievement) - if $userAchievements{ $achievement->achievement_id } + if $userAchievements->{ $achievement->achievement_id } && $achievement->enabled - && $userAchievements{ $achievement->achievement_id }->earned; + && $userAchievements->{ $achievement->achievement_id }->earned; } push(@rows, [ $globalData->achievement_points || 0, $level, $user, \@badges ]); From 5c8d23f8cec85097828da4736a882a0c6188b3ef Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Sun, 20 Apr 2025 10:12:58 -0500 Subject: [PATCH 002/186] Speed up deleting of global user achievements. Instead of listing user achievements and then deleting them one by one in a loop, just delete them with one query using a `where` statement. --- lib/WeBWorK/DB.pm | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/lib/WeBWorK/DB.pm b/lib/WeBWorK/DB.pm index c44e726896..50bc67a40a 100644 --- a/lib/WeBWorK/DB.pm +++ b/lib/WeBWorK/DB.pm @@ -1467,17 +1467,8 @@ sub deleteGlobalUserAchievement { # userAchievementID can be undefined if being called from this package my $U = caller eq __PACKAGE__ ? "!" : ""; my ($self, $userID) = shift->checkArgs(\@_, "user_id$U"); - - my @achievements = $self->listUserAchievements($userID); - foreach my $achievement (@achievements) { - $self->deleteUserAchievement($userID, $achievement); - } - - if ($self->{global_user_achievement}) { - return $self->{global_user_achievement}->delete($userID); - } else { - return 0; - } + $self->{achievement_user}->delete_where({ user_id => $userID }); + return $self->{global_user_achievement}->delete($userID); } ################################################################################ From 91fc5387eec2cb3feccb51fc0c797da98cdb4458 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Thu, 3 Apr 2025 19:57:22 -0500 Subject: [PATCH 003/186] Fix an issue with feedback emails with text and html body, and an attachment. Email::Stuffer incorrectly adds an attachment together with the text and html body parts all at the same level with content type multipart/mixed. Structurally this is as follows: [ (content type multipart/mixed) text body, html body, attachment ] As a result, when an email has a text body, an html body, and an attachment, both the text and html are shown in most email clients. The text and html body should be parts of a separate part that has content type 'multipart/alternative'. So the email has two parts, and the first part has two parts in that which are the text and html body. The second part is the attachment. The following shows how it should be structurally: [ (content type multipart/mixed) [ (content type multipart/alternative) text body, html body ], attachment ] To fix this, before adding an attachment the text and html body are moved into a separate part with content type multipart/alternative. --- lib/WeBWorK/ContentGenerator/Feedback.pm | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/lib/WeBWorK/ContentGenerator/Feedback.pm b/lib/WeBWorK/ContentGenerator/Feedback.pm index 41127629d8..f4a49766f7 100644 --- a/lib/WeBWorK/ContentGenerator/Feedback.pm +++ b/lib/WeBWorK/ContentGenerator/Feedback.pm @@ -203,6 +203,19 @@ sub initialize ($c) { return; } + # Email::Stuffer incorrectly adds the attachment together with the text and html body parts at the same + # level. As a result, when an email has a text body, an html body, and an attachment, both the text and + # html are shown in most email clients. The text and html body should be parts of a separate part that has + # content type 'multipart/alternative'. So the email has two parts, and the first part has two parts which + # are the text and html body. The second part is the attachment. So before attaching the file move the text + # and html body into their own part. + $email->{parts} = [ + Email::MIME->create( + attributes => { content_type => 'multipart/alternative' }, + parts => $email->{parts} + ) + ]; + # Attach the file. $email->attach($contents, filename => $filename); } From b1885be782848f5fcd12758a4e3363562c1ba2a7 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Mon, 21 Apr 2025 14:48:15 -0500 Subject: [PATCH 004/186] Speed up loading of the "Users Assigned to Set" page. Load all user sets for the given set and all users before rendering the template instead of getting one set at a time in the for loop inside the template. This speeds up loading of this page a lot with a large number of users. With 5000 users it decreases the load time from 3.4 seconds to 0.14 seconds. --- .../ContentGenerator/Instructor/UsersAssignedToSet.pm | 10 ++++++++-- .../Instructor/UsersAssignedToSet.html.ep | 4 ++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/WeBWorK/ContentGenerator/Instructor/UsersAssignedToSet.pm b/lib/WeBWorK/ContentGenerator/Instructor/UsersAssignedToSet.pm index 687ac4961c..fa9ada3e8e 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/UsersAssignedToSet.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/UsersAssignedToSet.pm @@ -33,6 +33,10 @@ sub initialize ($c) { my $setID = $c->stash('setID'); my $user = $c->param('user'); + # Make sure these are defined for the template. + $c->stash->{user_records} = []; + $c->stash->{set_records} = {}; + # Check permissions return unless $authz->hasPermissions($user, "access_instructor_tools"); return unless $authz->hasPermissions($user, "assign_problem_sets"); @@ -62,7 +66,7 @@ sub initialize ($c) { } # Get all user records and cache them for later use. - $c->{user_records} = + $c->stash->{user_records} = [ $db->getUsersWhere({ user_id => { not_like => 'set_id:%' } }, [qw/section last_name first_name/]) ]; if ($doAssignToSelected) { @@ -71,7 +75,7 @@ sub initialize ($c) { my %setUsers = map { $_ => 1 } $db->listSetUsers($setID); my @usersToAdd; - for my $selectedUser (map { $_->user_id } @{ $c->{user_records} }) { + for my $selectedUser (map { $_->user_id } @{ $c->stash->{user_records} }) { if (exists $selectedUsers{$selectedUser}) { unless ($setUsers{$selectedUser}) { # skip users already in the set debug("saving $selectedUser to be added to set later"); @@ -89,6 +93,8 @@ sub initialize ($c) { } } + $c->stash->{set_records} = { map { $_->user_id => $_ } $db->getUserSetsWhere({ set_id => $setID }) }; + return; } diff --git a/templates/ContentGenerator/Instructor/UsersAssignedToSet.html.ep b/templates/ContentGenerator/Instructor/UsersAssignedToSet.html.ep index b520fa2036..670849becd 100644 --- a/templates/ContentGenerator/Instructor/UsersAssignedToSet.html.ep +++ b/templates/ContentGenerator/Instructor/UsersAssignedToSet.html.ep @@ -41,9 +41,9 @@ - % for my $user (@{ $c->{user_records} }) { + % for my $user (@$user_records) { % my $userID = $user->user_id; - % my $userSet = $db->getUserSet($userID, $setID); + % my $userSet = $set_records->{$userID}; % From b2cb98a2d4b3e23f76ece4d6974c459cf9eee031 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Mon, 21 Apr 2025 12:39:16 -0500 Subject: [PATCH 005/186] Speed up class list importing. Use a batch insertion and update for users, permissions, and passwords when importing a class list. This improves the speed of imporing large class lists quite a bit. For example, importing a classlist containing 5000 users improves from around 22 seconds to around 7 seconds in my testing. --- .../ContentGenerator/Instructor/UserList.pm | 56 ++++++++++--------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/lib/WeBWorK/ContentGenerator/Instructor/UserList.pm b/lib/WeBWorK/ContentGenerator/Instructor/UserList.pm index cdbced085a..3a5e2ba5b2 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/UserList.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/UserList.pm @@ -626,22 +626,18 @@ sub menuLabels ($c, $hashRef) { } sub importUsersFromCSV ($c, $fileName, $replaceExisting, $fallbackPasswordSource, @replaceList) { - my $ce = $c->ce; - my $db = $c->db; - my $dir = $ce->{courseDirs}{templates}; - my $user = $c->param('user'); - my $perm = $c->{userPermission}; + my $ce = $c->ce; + my $db = $c->db; - die $c->maketext("Illegal '/' character in input.") if $fileName =~ m|/|; - die $c->maketext("File [_1]/[_2] either does not exist or is not readable.", $dir, $fileName) - unless -r "$dir/$fileName"; + die $c->maketext(q{Illegal '/' character in input.}) if $fileName =~ m|/|; + die $c->maketext('File [_1]/[_2] either does not exist or is not readable.', + $ce->{courseDirs}{templates}, $fileName) + unless -r "$ce->{courseDirs}{templates}/$fileName"; my %allUserIDs = map { $_ => 1 } @{ $c->{allUserIDs} }; my %replaceOK; - if ($replaceExisting eq 'none') { - %replaceOK = (); - } elsif ($replaceExisting eq 'listed') { + if ($replaceExisting eq 'listed') { %replaceOK = map { $_ => 1 } @replaceList; } elsif ($replaceExisting eq 'any') { %replaceOK = %allUserIDs; @@ -649,15 +645,16 @@ sub importUsersFromCSV ($c, $fileName, $replaceExisting, $fallbackPasswordSource my $default_permission_level = $ce->{default_permission_level}; - my (@replaced, @added, @skipped); + my (@usersToInsert, @passwordsToInsert, @permissionsToInsert, + @usersToUpdate, @passwordsToUpdate, @permissionsToUpdate, @skipped); # get list of hashrefs representing lines in classlist file - my @classlist = parse_classlist("$dir/$fileName"); + my @classlist = parse_classlist("$ce->{courseDirs}{templates}/$fileName"); # Default status is enrolled -- fetch abbreviation for enrolled my $default_status_abbrev = $ce->{statuses}{Enrolled}{abbrevs}[0]; - foreach my $record (@classlist) { + for my $record (@classlist) { my %record = %$record; my $user_id = $record{user_id}; @@ -665,11 +662,11 @@ sub importUsersFromCSV ($c, $fileName, $replaceExisting, $fallbackPasswordSource push @skipped, $user_id; next; } - if ($user_id eq $user) { # don't replace yourself!! + if ($user_id eq $c->param('user')) { # don't replace yourself!! push @skipped, $user_id; next; } - if ($record{permission} && $perm < $record{permission}) { + if ($record{permission} && $c->{userPermission} < $record{permission}) { push @skipped, $user_id; next; } @@ -681,7 +678,7 @@ sub importUsersFromCSV ($c, $fileName, $replaceExisting, $fallbackPasswordSource # set default status is status field is "empty" $record{status} = $default_status_abbrev - unless defined $record{status} and $record{status} ne ""; + unless defined $record{status} && $record{status} ne ''; # Determine what to use for the password (if anything). if (!$record{password}) { @@ -697,7 +694,7 @@ sub importUsersFromCSV ($c, $fileName, $replaceExisting, $fallbackPasswordSource # set default permission level if permission level is "empty" $record{permission} = $default_permission_level - unless defined $record{permission} and $record{permission} ne ""; + unless defined $record{permission} && $record{permission} ne ''; my $User = $db->newUser(%record); my $PermissionLevel = $db->newPermissionLevel(user_id => $user_id, permission => $record{permission}); @@ -705,24 +702,29 @@ sub importUsersFromCSV ($c, $fileName, $replaceExisting, $fallbackPasswordSource # DBFIXME use REPLACE if (exists $allUserIDs{$user_id}) { - $db->putUser($User); - $db->putPermissionLevel($PermissionLevel); - $db->putPassword($Password) if $Password; + push(@usersToUpdate, $User); + push(@permissionsToUpdate, $PermissionLevel); + push(@passwordsToUpdate, $Password) if $Password; $User->{permission} = $PermissionLevel->permission; $User->{passwordExists} = 1 if $Password; - push @replaced, $User; } else { $allUserIDs{$user_id} = 1; - $db->addUser($User); - $db->addPermissionLevel($PermissionLevel); - $db->addPassword($Password) if $Password; + push(@usersToInsert, $User); + push(@permissionsToInsert, $PermissionLevel); + push(@passwordsToInsert, $Password) if $Password; $User->{permission} = $PermissionLevel->permission; $User->{passwordExists} = 1 if $Password; - push @added, $User; } } - return \@replaced, \@added, \@skipped; + $db->User->insert_records(\@usersToInsert) if @usersToInsert; + $db->User->update_records(\@usersToUpdate) if @usersToUpdate; + $db->Password->insert_records(\@passwordsToInsert) if @passwordsToInsert; + $db->Password->update_records(\@passwordsToUpdate) if @passwordsToUpdate; + $db->PermissionLevel->insert_records(\@permissionsToInsert) if @permissionsToInsert; + $db->PermissionLevel->update_records(\@permissionsToUpdate) if @permissionsToUpdate; + + return \@usersToUpdate, \@usersToInsert, \@skipped; } sub exportUsersToCSV ($c, $fileName, @userIDsToExport) { From 09f3fa2fb2575b0f93b9f064a0cffa3c234929e4 Mon Sep 17 00:00:00 2001 From: Jaimos Skriletz Date: Sun, 4 May 2025 19:40:09 -0600 Subject: [PATCH 006/186] Make WebworkWebservice honor showEvaluatedAnswers when rendering a problem. --- lib/WebworkWebservice/RenderProblem.pm | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/WebworkWebservice/RenderProblem.pm b/lib/WebworkWebservice/RenderProblem.pm index 5e848719a6..f68a353141 100644 --- a/lib/WebworkWebservice/RenderProblem.pm +++ b/lib/WebworkWebservice/RenderProblem.pm @@ -226,8 +226,9 @@ async sub renderProblem { forceScaffoldsOpen => $rh->{WWcorrectAnsOnly} ? 1 : ($rh->{forceScaffoldsOpen} // 0), QUIZ_PREFIX => $rh->{answerPrefix}, showFeedback => $rh->{previewAnswers} || $rh->{WWsubmit} || $rh->{WWcorrectAns}, - showAttemptAnswers => $rh->{WWcorrectAnsOnly} ? 0 : ($rh->{showAttemptAnswers} // 1), - showAttemptPreviews => ( + showAttemptAnswers => $rh->{WWcorrectAnsOnly} ? 0 + : ($rh->{showAttemptAnswers} // $ce->{pg}{options}{showEvaluatedAnswers}), + showAttemptPreviews => ( $rh->{WWcorrectAnsOnly} ? 0 : ($rh->{showAttemptPreviews} // ($rh->{previewAnswers} || $rh->{WWsubmit} || $rh->{WWcorrectAns})) ), From 9b2bab4e32f03ead626562eb24a6fbb529830e5b Mon Sep 17 00:00:00 2001 From: Alex Jordan Date: Tue, 15 Apr 2025 08:33:45 -0700 Subject: [PATCH 007/186] LTI section identification --- lib/WeBWorK/Authen/LTIAdvanced.pm | 2 +- lib/WeBWorK/Authen/LTIAdvantage.pm | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/WeBWorK/Authen/LTIAdvanced.pm b/lib/WeBWorK/Authen/LTIAdvanced.pm index 853040d4db..ad430a9f85 100644 --- a/lib/WeBWorK/Authen/LTIAdvanced.pm +++ b/lib/WeBWorK/Authen/LTIAdvanced.pm @@ -215,7 +215,7 @@ sub get_credentials { [ 'oauth_signature', 'oauth_signature' ], [ 'oauth_nonce', 'oauth_nonce' ], [ 'oauth_timestamp', 'oauth_timestamp' ], - [ 'section', 'custom_section' ], + [ 'section', 'context_label' ], [ 'recitation', 'custom_recitation' ], ); diff --git a/lib/WeBWorK/Authen/LTIAdvantage.pm b/lib/WeBWorK/Authen/LTIAdvantage.pm index 0062fc0499..97e15b15b1 100644 --- a/lib/WeBWorK/Authen/LTIAdvantage.pm +++ b/lib/WeBWorK/Authen/LTIAdvantage.pm @@ -177,7 +177,7 @@ sub get_credentials ($self) { [ roles => 'https://purl.imsglobal.org/spec/lti/claim/roles' ], [ last_name => 'family_name' ], [ first_name => 'given_name' ], - [ section => 'https://purl.imsglobal.org/spec/lti/claim/custom#section' ], + [ section => 'https://purl.imsglobal.org/spec/lti/claim/lis#course_section_sourcedid' ], [ recitation => 'https://purl.imsglobal.org/spec/lti/claim/custom#recitation' ], ); From 3db2a97dca73d7173bb1ee7be14af691f8774c43 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Thu, 17 Apr 2025 07:53:12 -0500 Subject: [PATCH 008/186] 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. --- htdocs/js/UserList/userlist.js | 5 ++- .../ContentGenerator/Instructor/UserList.pm | 32 +++++++++---------- .../Instructor/UserList/export_form.html.ep | 2 +- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/htdocs/js/UserList/userlist.js b/htdocs/js/UserList/userlist.js index 74f16a1596..617594371f 100644 --- a/htdocs/js/UserList/userlist.js +++ b/htdocs/js/UserList/userlist.js @@ -113,7 +113,10 @@ e.preventDefault(); e.stopPropagation(); show_errors(['select_user_err_msg'], [export_select]); - } else if (export_select_target?.value === 'new' && export_filename.value === '') { + } else if ( + export_select_target?.value === 'new' && + (export_filename.value === '' || /\//.test(export_filename.value)) + ) { e.preventDefault(); e.stopPropagation(); show_errors(['export_file_err_msg'], [export_filename, export_select_target]); diff --git a/lib/WeBWorK/ContentGenerator/Instructor/UserList.pm b/lib/WeBWorK/ContentGenerator/Instructor/UserList.pm index 3a5e2ba5b2..bee8b9f671 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/UserList.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/UserList.pm @@ -728,31 +728,29 @@ sub importUsersFromCSV ($c, $fileName, $replaceExisting, $fallbackPasswordSource } sub exportUsersToCSV ($c, $fileName, @userIDsToExport) { - my $ce = $c->ce; - my $db = $c->db; - my $dir = $ce->{courseDirs}->{templates}; + my $ce = $c->ce; + my $db = $c->db; - die $c->maketext("illegal character in input: '/'") if $fileName =~ m|/|; + die $c->maketext(q{illegal character in input: '/'}) if $fileName =~ m|/|; my @records; - my @Users = $db->getUsers(@userIDsToExport); - my @Passwords = $db->getPasswords(@userIDsToExport); - my @PermissionLevels = $db->getPermissionLevels(@userIDsToExport); - foreach my $i (0 .. $#userIDsToExport) { - my $User = $Users[$i]; - my $Password = $Passwords[$i]; - my $PermissionLevel = $PermissionLevels[$i]; - next unless defined $User; - my %record = ( - defined $PermissionLevel ? $PermissionLevel->toHash : (), - defined $Password ? $Password->toHash : (), - $User->toHash, + my @users = $db->getUsers(@userIDsToExport); + my %passwords = map { $_->user_id => $_ } $db->getPasswords(@userIDsToExport); + my %permissionLevels = map { $_->user_id => $_ } $db->getPermissionLevels(@userIDsToExport); + + for my $user (@users) { + my $password = $passwords{ $user->user_id }; + my $permissionLevel = $permissionLevels{ $user->user_id }; + my %record = ( + defined $permissionLevel ? $permissionLevel->toHash : (), + defined $password ? $password->toHash : (), + $user->toHash, ); push @records, \%record; } - write_classlist("$dir/$fileName", @records); + write_classlist("$ce->{courseDirs}{templates}/$fileName", @records); return; } diff --git a/templates/ContentGenerator/Instructor/UserList/export_form.html.ep b/templates/ContentGenerator/Instructor/UserList/export_form.html.ep index 2b70ca5f92..5dacc40238 100644 --- a/templates/ContentGenerator/Instructor/UserList/export_form.html.ep +++ b/templates/ContentGenerator/Instructor/UserList/export_form.html.ep @@ -34,6 +34,6 @@
- <%= maketext('Please input file name to export to.') %> + <%= maketext('Please input a file name to export to that does not contain forward slashes.') %>
From 69e49979820c083d6c5b03570c7055725d4389fe Mon Sep 17 00:00:00 2001 From: Alex Jordan Date: Sun, 20 Apr 2025 00:06:51 -0700 Subject: [PATCH 009/186] changes to email template for feedback --- .../Feedback/feedback_email.html.ep | 262 +++++++++--------- 1 file changed, 131 insertions(+), 131 deletions(-) diff --git a/templates/ContentGenerator/Feedback/feedback_email.html.ep b/templates/ContentGenerator/Feedback/feedback_email.html.ep index 3027c79615..6b0e4f1d63 100644 --- a/templates/ContentGenerator/Feedback/feedback_email.html.ep +++ b/templates/ContentGenerator/Feedback/feedback_email.html.ep @@ -1,48 +1,29 @@ % use WeBWorK::Utils qw(decodeAnswers); +% use WeBWorK::Utils::Sets qw(format_set_name_display); % - - <%= stylesheet begin =%> - .data-table td, .data-table th { - text-align: left; - padding: 0.25rem; - } - .data-table.bordered { - border-collapse: collapse; - } - .data-table.bordered > * > tr > td, .data-table.bordered > * > tr > th { - border: 1px solid black; - } - .data-table.bordered thead tr:first-child th { - border-bottom-width: 2px; - } - .mb-1 { - margin-bottom: 1rem - } - .user-message { - border: 1px solid black; - border-radius: 0.375rem; - padding: 1rem; - } - .user-message-line { - margin: 0; - white-space: pre-wrap; - } - <% end =%> - -

- Message from <%= $user->full_name %> (<%= $user->user_id %>) via WeBWorK at <%= url_for('root')->to_abs %> -

-

- <%= $user->first_name %> sent feedback from <%= link_to 'this page' => $emailableURL %>. -

+ % if ($problem) { +

+ Message from <%= $user->full_name %> (<%= $user->user_id %>) via WeBWorK at + <%= $ce->{institutionName} %> (sent from + <%= link_to format_set_name_display($set->set_id) . ', #' . $problem->problem_id => $emailableURL %>). +

+ % } elsif ($set) { + Message from <%= $user->full_name %> (<%= $user->user_id %>) via WeBWorK at + <%= $ce->{institutionName} %> (sent from <%= link_to format_set_name_display($set->set_id) %>) + + % } else { +

+ Message from <%= $user->full_name %> (<%= $user->user_id %>) via WeBWorK at + <%= $ce->{institutionName} %> (sent from <%= link_to 'this page' => $emailableURL %>). +

+ % } % if ($feedback) { -

<%= $user->full_name %> (<%= $user->user_id %>) wrote:

-
+
% for (split /\n\r?/, $feedback) { % if ($_) { -

<%= $_ %>

+

<%= $_ %>

% } else {
% } @@ -50,134 +31,153 @@
% } % if ($problem && $verbosity >= 1) { - +
- + + % my @rows = ( + % ['Problem ID', $problem->problem_id], + % ['Source file', $problem->source_file], + % ['Value', $problem->value], + % ['Max attempts', $problem->max_attempts == -1 ? 'unlimited' : $problem->max_attempts], + % ['Random seed', $problem->problem_seed], + % ['Status', $problem->status], + % ['Attempted', $problem->attempted ? 'yes' : 'no'], + % ['Correct attempts', $problem->num_correct], + % ['Incorrect attempts', $problem->num_incorrect] + % ); + % for (@rows) { + + + + + % } + % my %last_answer = decodeAnswers($problem->last_answer); - - - - - - - - - - - - - - - - - - + + % if (%last_answer) { + + % } else { + + % }
Data about the problem processor
Data about the problem
<%= $_->[0] %>:<%= $_->[1] %>
Display Mode:<%= param('displayMode') %>
Show Old Answers:<%= param('showOldAnswers') ? 'yes' : 'no' %>
Show Correct Answers:<%= param('showCorrectAnswers') ? 'yes' : 'no' %>
Show Hints:<%= param('showHints') ? 'yes' : 'no' %>
Show Solutions:<%= param('showSolutions') ? 'yes' : 'no' %>Last answer: + % for my $key (sort keys %last_answer) { + % if ($last_answer{$key}) { + + + + + % } + % } +
<%= $key %>:<%= $last_answer{$key} %>
none
% } - % - % if ($user && $verbosity >= 1) { - + % if ($set && $verbosity >= 1) { +
- + - - - - % unless ($ce->{blockStudentIDinFeedback}) { - + % my @rows = ( + % ['Set ID', $set->set_id], + % ['Set header file', $set->set_header], + % ['Hardcopy header file', $set->hardcopy_header], + % ['Open date', $c->formatDateTime($set->open_date)], + % ['Close date', $c->formatDateTime($set->due_date)], + % ['Answer date', $c->formatDateTime($set->answer_date)], + % ['Visible', $set->visible ? 'yes' : 'no'], + % ['Assignment type', $set->assignment_type] + % ); + % if ($set->assignment_type =~ /gateway/) { + % push @rows, ( + % ['Attempts per version', $set->attempts_per_version], + % ['Time interval', $set->time_interval], + % ['Versions per interval', $set->versions_per_interval], + % ['Version time limit', $set->version_time_limit], + % ['Version creation time', $c->formatDateTime($set->version_creation_time)], + % ['Problem randorder', $set->problem_randorder], + % ['Version last attempt time', $set->version_last_attempt_time] + % ); + % } + % for (@rows) { + + + + % } - % my $status_name = $ce->status_abbrev_to_name($user->status); - %my $status_string = - % defined $status_name - % ? "$status_name ('" . $user->status . q{')} - % : $user->status . ' (unknown status abbreviation)'; - - - - -
Data about the user
Data about the assignment
User ID:<%= $user->user_id %>
Name:<%= $user->full_name %>
Email:<%= $user->email_address %>
Student ID:<%= $user->student_id %>
<%= $_->[0] %>:<%= $_->[1] %>
Status:<%= $status_string %>
Section:<%= $user->section %>
Recitation:<%= $user->recitation %>
Comment:<%= $user->comment %>
IP Address:<%= $remote_host %>:<%= $remote_port %>
% } % if ($problem && $verbosity >= 1) { - +
- + - - - - - - - - - - - % my %last_answer = decodeAnswers($problem->last_answer); - - - % if (%last_answer) { - - % } else { - - % } - - - + % my @rows = ( + % ['Display Mode', param('displayMode')], + % ['Show Old Answers', param('showOldAnswers') ? 'yes' : 'no'], + % ['Show Correct Answers', param('showCorrectAnswers') ? 'yes' : 'no'], + % ['Show Hints', param('showHints') ? 'yes' : 'no'], + % ['Show Solutions', param('showSolutions') ? 'yes' : 'no'] + % ); + % for (@rows) { + + + + + % }
Data about the problem
Data about the problem processor
Problem ID:<%= $problem->problem_id %>
Source file:<%= $problem->source_file %>
Value:<%= $problem->value %>
Max attempts<%= $problem->max_attempts == -1 ? 'unlimited' : $problem->max_attempts %>
Random seed:<%= $problem->problem_seed %>
Status:<%= $problem->status %>
Attempted:<%= $problem->attempted ? 'yes' : 'no' %>
Last answer: - - % for my $key (sort keys %last_answer) { - % if ($last_answer{$key}) { - - % } - % } -
<%= $key %>:<%= $last_answer{$key} %>
-
none
Number of correct attempts:<%= $problem->num_correct %>
Number of incorrect attempts:<%= $problem->num_incorrect %>
<%= $_->[0] %>:<%= $_->[1] %>
% } - % if ($set && $verbosity >= 1) { - + % + % if ($user && $verbosity >= 1) { +
- + - - - - - - - - - % if ($set->assignment_type =~ /gateway/) { - - - - + % my @rows = ( + % ['User ID', $user->user_id], + % ['Name', $user->full_name], + % ['Email', $user->email_address] + % ); + % unless ($ce->{blockStudentIDinFeedback}) {push @rows, ['Student ID', $user->student_id];} + % my $status_name = $ce->status_abbrev_to_name($user->status); + % my $status_string = + % defined $status_name + % ? "$status_name ('" . $user->status . q{')} + % : $user->status . ' (unknown status abbreviation)'; + % push @rows, ( + % ['Status', $status_string], + % ['Section', $user->section], + % ['Recitation', $user->recitation], + % ['Comment', $user->comment], + % ['IP Address', $remote_host] + %); + % for (@rows) { - - + + - - % }
Data about the homework set
Data about the user
Set ID:<%= $set->set_id %>
Set header file:<%= $set->set_header %>
Hardcopy header file:<%= $set->hardcopy_header %>
Open date:<%= $c->formatDateTime($set->open_date) %>
Due date:<%= $c->formatDateTime($set->due_date) %>
Answer date:<%= $c->formatDateTime($set->answer_date) %>
Visible:<%= $set->visible ? 'yes' : 'no' %>
Assignment type:<%= $set->assignment_type %>
Attempts per version:<%= $set->assignment_type %>
Time interval:<%= $set->time_interval %>
Versions per interval:<%= $set->versions_per_interval %>
Version time limit:<%= $set->version_time_limit %>
Version creation time:<%= $c->formatDateTime($set->version_creation_time) %><%= $_->[0] %>:<%= $_->[1] %>
Problem randorder:<%= $set->problem_randorder %>
Version last attempt time:<%= $set->version_last_attempt_time %>
% } % if ($verbosity >= 2) { - +
- + - +
Data about the environment
Data about the environment
<%= dumper($ce) %>
<%= dumper($ce) %>
% } From 3f931e23d819ce6153bc57a0a3caac89ec9ad083 Mon Sep 17 00:00:00 2001 From: Alex Jordan Date: Wed, 23 Apr 2025 16:28:25 -0700 Subject: [PATCH 010/186] Update templates/ContentGenerator/Feedback/feedback_email.html.ep Co-authored-by: Glenn Rice <47527406+drgrice1@users.noreply.github.com> --- .../Feedback/feedback_email.html.ep | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/templates/ContentGenerator/Feedback/feedback_email.html.ep b/templates/ContentGenerator/Feedback/feedback_email.html.ep index 6b0e4f1d63..921b5f9f45 100644 --- a/templates/ContentGenerator/Feedback/feedback_email.html.ep +++ b/templates/ContentGenerator/Feedback/feedback_email.html.ep @@ -3,22 +3,20 @@ % - % if ($problem) { -

+

+ % if ($problem) { Message from <%= $user->full_name %> (<%= $user->user_id %>) via WeBWorK at <%= $ce->{institutionName} %> (sent from <%= link_to format_set_name_display($set->set_id) . ', #' . $problem->problem_id => $emailableURL %>). -

- % } elsif ($set) { + % } elsif ($set) { Message from <%= $user->full_name %> (<%= $user->user_id %>) via WeBWorK at - <%= $ce->{institutionName} %> (sent from <%= link_to format_set_name_display($set->set_id) %>) - - % } else { -

+ <%= $ce->{institutionName} %> + (sent from <%= link_to format_set_name_display($set->set_id) => $emailableURL %>) + % } else { Message from <%= $user->full_name %> (<%= $user->user_id %>) via WeBWorK at <%= $ce->{institutionName} %> (sent from <%= link_to 'this page' => $emailableURL %>). -

- % } + % } +

% if ($feedback) {
% for (split /\n\r?/, $feedback) { From 76e88beb6524fce4575e9773e81dc5a1f1df4bfb Mon Sep 17 00:00:00 2001 From: Alex Jordan Date: Thu, 24 Apr 2025 08:25:28 -0700 Subject: [PATCH 011/186] style "last submission" row to make it more clearly delimited in HTML email feedback template --- templates/ContentGenerator/Feedback/feedback_email.html.ep | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/templates/ContentGenerator/Feedback/feedback_email.html.ep b/templates/ContentGenerator/Feedback/feedback_email.html.ep index 921b5f9f45..7a36596045 100644 --- a/templates/ContentGenerator/Feedback/feedback_email.html.ep +++ b/templates/ContentGenerator/Feedback/feedback_email.html.ep @@ -53,10 +53,10 @@ % } % my %last_answer = decodeAnswers($problem->last_answer); - - Last answer: + + Last submission: % if (%last_answer) { - +
% for my $key (sort keys %last_answer) { % if ($last_answer{$key}) { From e49b9b02912eb2cf8af6a662b615f77bd6ba3aad Mon Sep 17 00:00:00 2001 From: Alex Jordan Date: Thu, 24 Apr 2025 08:42:11 -0700 Subject: [PATCH 012/186] update feedback text template to be consistent with html template --- .../Feedback/feedback_email.txt.ep | 79 +++++++++---------- 1 file changed, 39 insertions(+), 40 deletions(-) diff --git a/templates/ContentGenerator/Feedback/feedback_email.txt.ep b/templates/ContentGenerator/Feedback/feedback_email.txt.ep index dd3fbf48d5..4fc5654638 100644 --- a/templates/ContentGenerator/Feedback/feedback_email.txt.ep +++ b/templates/ContentGenerator/Feedback/feedback_email.txt.ep @@ -1,9 +1,7 @@ % use WeBWorK::Utils qw(decodeAnswers); % Message from <%== $user->full_name %> (<%== $user->user_id %>) via WeBWorK at -<%== url_for('root')->to_abs %> - -To visit the page from which <%== $user->first_name %> sent feedback, go to: +<%== $ce->{institutionName} %> (sent from <%== $emailableURL %> % if ($feedback) { @@ -15,37 +13,6 @@ To visit the page from which <%== $user->first_name %> sent feedback, go to: % } % if ($problem && $verbosity >= 1) { -***** Data about the problem processor: ***** - -Display Mode: <%== param('displayMode') %> -Show Old Answers: <%== param('showOldAnswers') ? 'yes' : 'no' %> -Show Correct Answers: <%== param('showCorrectAnswers') ? 'yes' : 'no' %> -Show Hints: <%== param('showHints') ? 'yes' : 'no' %> -Show Solutions: <%== param('showSolutions') ? 'yes' : 'no' %> -% } -% if ($user && $verbosity >= 1) { - -***** Data about the user: ***** - -User ID: <%== $user->user_id %> -Name: <%== $user->full_name %> -Email: <%== $user->email_address %> - % unless ($ce->{blockStudentIDinFeedback}) { -Student ID: <%== $user->student_id %> - % } -% my $status_name = $ce->status_abbrev_to_name($user->status); - %my $status_string = - % defined $status_name - % ? "$status_name ('" . $user->status . q{')} - % : $user->status . ' (unknown status abbreviation)'; -Status: <%== $status_string %> -Section: <%== $user->section %> -Recitation: <%== $user->recitation %> -Comment: <%== $user->comment %> -IP Address: <%== $remote_host %>:<%== $c->tx->remote_port || 'UNKNOWN' %> -% } -% if ($problem && $verbosity >= 1) { - ***** Data about the problem: ***** Problem ID: <%== $problem->problem_id %> @@ -55,23 +22,23 @@ Max attempts <%== $problem->max_attempts == -1 ? 'unlimited' : Random seed: <%== $problem->problem_seed %> Status: <%== $problem->status %> Attempted: <%== $problem->attempted ? 'yes' : 'no' %> +Number of correct attempts: <%== $problem->num_correct %> +Number of incorrect attempts: <%== $problem->num_incorrect %> % my %last_answer = decodeAnswers($problem->last_answer); % if (%last_answer) { -Last answer: +Last submission: % for my $key (sort keys %last_answer) { % if ($last_answer{$key}) { <%== $key %>: <%== $last_answer{$key} %> % } % } % } else { -Last answer: none +Last submission: none % } -Number of correct attempts: <%== $problem->num_correct %> -Number of incorrect attempts: <%== $problem->num_incorrect %> % } % if ($set && $verbosity >= 1) { -***** Data about the homework set: ***** +***** Data about the assignment: ***** Set ID: <%== $set->set_id %> Set header file: <%== $set->set_header %> @@ -82,7 +49,7 @@ Answer date: <%== $c->formatDateTime($set->answer_date) %> Visible: <%== $set->visible ? 'yes' : 'no' %> Assignment type: <%== $set->assignment_type %> % if ($set->assignment_type =~ /gateway/) { -Attempts per version: <%== $set->assignment_type %> +Attempts per version: <%== $set->attempts_per_version %> Time interval: <%== $set->time_interval %> Versions per interval: <%== $set->versions_per_interval %> Version time limit: <%== $set->version_time_limit %> @@ -90,6 +57,38 @@ Version creation time: <%== $c->formatDateTime($set->version_creation_time) Problem randorder: <%== $set->problem_randorder %> Version last attempt time: <%== $set->version_last_attempt_time %> % } +% } +% if ($problem && $verbosity >= 1) { + +***** Data about the problem processor: ***** + +Display Mode: <%== param('displayMode') %> +Show Old Answers: <%== param('showOldAnswers') ? 'yes' : 'no' %> +Show Correct Answers: <%== param('showCorrectAnswers') ? 'yes' : 'no' %> +Show Hints: <%== param('showHints') ? 'yes' : 'no' %> +Show Solutions: <%== param('showSolutions') ? 'yes' : 'no' %> +% } +% if ($user && $verbosity >= 1) { + +***** Data about the user: ***** + +User ID: <%== $user->user_id %> +Name: <%== $user->full_name %> +Email: <%== $user->email_address %> + % unless ($ce->{blockStudentIDinFeedback}) { +Student ID: <%== $user->student_id %> + % } +% my $status_name = $ce->status_abbrev_to_name($user->status); + %my $status_string = + % defined $status_name + % ? "$status_name ('" . $user->status . q{')} + % : $user->status . ' (unknown status abbreviation)'; +Status: <%== $status_string %> +Section: <%== $user->section %> +Recitation: <%== $user->recitation %> +Comment: <%== $user->comment %> +IP Address: <%== $remote_host %>:<%== $c->tx->remote_port || 'UNKNOWN' %> + % } % if ($verbosity >= 2) { From b63b7e8535b2be6de33a38a38ed07d3cd5dc87c9 Mon Sep 17 00:00:00 2001 From: Alex Jordan Date: Thu, 24 Apr 2025 08:45:45 -0700 Subject: [PATCH 013/186] restore border radius and change border color in html feedback form --- templates/ContentGenerator/Feedback/feedback_email.html.ep | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/ContentGenerator/Feedback/feedback_email.html.ep b/templates/ContentGenerator/Feedback/feedback_email.html.ep index 7a36596045..a4825029bf 100644 --- a/templates/ContentGenerator/Feedback/feedback_email.html.ep +++ b/templates/ContentGenerator/Feedback/feedback_email.html.ep @@ -18,7 +18,7 @@ % }

% if ($feedback) { -
+
% for (split /\n\r?/, $feedback) { % if ($_) {

<%= $_ %>

From 3eb18f07fa4c3135b716a378fa33cb6294382314 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Thu, 24 Apr 2025 13:25:34 -0700 Subject: [PATCH 014/186] improve message at start of feedback text template --- .../ContentGenerator/Feedback/feedback_email.txt.ep | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/templates/ContentGenerator/Feedback/feedback_email.txt.ep b/templates/ContentGenerator/Feedback/feedback_email.txt.ep index 4fc5654638..24a162a709 100644 --- a/templates/ContentGenerator/Feedback/feedback_email.txt.ep +++ b/templates/ContentGenerator/Feedback/feedback_email.txt.ep @@ -1,7 +1,15 @@ % use WeBWorK::Utils qw(decodeAnswers); +% use WeBWorK::Utils::Sets qw(format_set_name_display); % -Message from <%== $user->full_name %> (<%== $user->user_id %>) via WeBWorK at -<%== $ce->{institutionName} %> (sent from +Message from <%= $user->full_name %> (<%= $user->user_id %>) via WeBWorK at <%= $ce->{institutionName} %>. + +% if ($problem) { +Feedback sent from <%= format_set_name_display($set->set_id) . ', #' . $problem->problem_id %>: +% } elsif ($set) { +Feedback sent from <%= format_set_name_display($set->set_id) %>: +% } else { +Feedback sent from: +% } <%== $emailableURL %> % if ($feedback) { From 0b9548155939079a0466b7b3992d5ac09cd0931f Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Thu, 24 Apr 2025 11:05:35 -0500 Subject: [PATCH 015/186] A little code clean up. --- .../Feedback/feedback_email.html.ep | 76 +++++++++---------- .../Feedback/feedback_email.txt.ep | 2 - 2 files changed, 38 insertions(+), 40 deletions(-) diff --git a/templates/ContentGenerator/Feedback/feedback_email.html.ep b/templates/ContentGenerator/Feedback/feedback_email.html.ep index a4825029bf..901e899a28 100644 --- a/templates/ContentGenerator/Feedback/feedback_email.html.ep +++ b/templates/ContentGenerator/Feedback/feedback_email.html.ep @@ -36,15 +36,15 @@
% my @rows = ( - % ['Problem ID', $problem->problem_id], - % ['Source file', $problem->source_file], - % ['Value', $problem->value], - % ['Max attempts', $problem->max_attempts == -1 ? 'unlimited' : $problem->max_attempts], - % ['Random seed', $problem->problem_seed], - % ['Status', $problem->status], - % ['Attempted', $problem->attempted ? 'yes' : 'no'], - % ['Correct attempts', $problem->num_correct], - % ['Incorrect attempts', $problem->num_incorrect] + % [ 'Problem ID', $problem->problem_id ], + % [ 'Source file', $problem->source_file ], + % [ 'Value', $problem->value ], + % [ 'Max attempts', $problem->max_attempts == -1 ? 'unlimited' : $problem->max_attempts ], + % [ 'Random seed', $problem->problem_seed ], + % [ 'Status', $problem->status ], + % [ 'Attempted', $problem->attempted ? 'yes' : 'no' ], + % [ 'Correct attempts', $problem->num_correct ], + % [ 'Incorrect attempts', $problem->num_incorrect ] % ); % for (@rows) { @@ -81,24 +81,24 @@ % my @rows = ( - % ['Set ID', $set->set_id], - % ['Set header file', $set->set_header], - % ['Hardcopy header file', $set->hardcopy_header], - % ['Open date', $c->formatDateTime($set->open_date)], - % ['Close date', $c->formatDateTime($set->due_date)], - % ['Answer date', $c->formatDateTime($set->answer_date)], - % ['Visible', $set->visible ? 'yes' : 'no'], - % ['Assignment type', $set->assignment_type] + % [ 'Set ID', $set->set_id ], + % [ 'Set header file', $set->set_header ], + % [ 'Hardcopy header file', $set->hardcopy_header ], + % [ 'Open date', $c->formatDateTime($set->open_date) ], + % [ 'Close date', $c->formatDateTime($set->due_date) ], + % [ 'Answer date', $c->formatDateTime($set->answer_date) ], + % [ 'Visible', $set->visible ? 'yes' : 'no' ], + % [ 'Assignment type', $set->assignment_type ] % ); % if ($set->assignment_type =~ /gateway/) { % push @rows, ( - % ['Attempts per version', $set->attempts_per_version], - % ['Time interval', $set->time_interval], - % ['Versions per interval', $set->versions_per_interval], - % ['Version time limit', $set->version_time_limit], - % ['Version creation time', $c->formatDateTime($set->version_creation_time)], - % ['Problem randorder', $set->problem_randorder], - % ['Version last attempt time', $set->version_last_attempt_time] + % [ 'Attempts per version', $set->attempts_per_version ], + % [ 'Time interval', $set->time_interval ], + % [ 'Versions per interval', $set->versions_per_interval ], + % [ 'Version time limit', $set->version_time_limit ], + % [ 'Version creation time', $c->formatDateTime($set->version_creation_time) ], + % [ 'Problem randorder', $set->problem_randorder ], + % [ 'Version last attempt time', $set->version_last_attempt_time ] % ); % } % for (@rows) { @@ -118,11 +118,11 @@ % my @rows = ( - % ['Display Mode', param('displayMode')], - % ['Show Old Answers', param('showOldAnswers') ? 'yes' : 'no'], - % ['Show Correct Answers', param('showCorrectAnswers') ? 'yes' : 'no'], - % ['Show Hints', param('showHints') ? 'yes' : 'no'], - % ['Show Solutions', param('showSolutions') ? 'yes' : 'no'] + % [ 'Display Mode', param('displayMode') ], + % [ 'Show Old Answers', param('showOldAnswers') ? 'yes' : 'no' ], + % [ 'Show Correct Answers', param('showCorrectAnswers') ? 'yes' : 'no' ], + % [ 'Show Hints', param('showHints') ? 'yes' : 'no' ], + % [ 'Show Solutions', param('showSolutions') ? 'yes' : 'no' ] % ); % for (@rows) { @@ -142,22 +142,22 @@ % my @rows = ( - % ['User ID', $user->user_id], - % ['Name', $user->full_name], - % ['Email', $user->email_address] + % [ 'User ID', $user->user_id ], + % [ 'Name', $user->full_name ], + % [ 'Email', $user->email_address ] % ); - % unless ($ce->{blockStudentIDinFeedback}) {push @rows, ['Student ID', $user->student_id];} + % unless ($ce->{blockStudentIDinFeedback}) { push @rows, ['Student ID', $user->student_id]; } % my $status_name = $ce->status_abbrev_to_name($user->status); % my $status_string = % defined $status_name % ? "$status_name ('" . $user->status . q{')} % : $user->status . ' (unknown status abbreviation)'; % push @rows, ( - % ['Status', $status_string], - % ['Section', $user->section], - % ['Recitation', $user->recitation], - % ['Comment', $user->comment], - % ['IP Address', $remote_host] + % [ 'Status', $status_string ], + % [ 'Section', $user->section ], + % [ 'Recitation', $user->recitation ], + % [ 'Comment', $user->comment ], + % [ 'IP Address', $remote_host ] %); % for (@rows) { diff --git a/templates/ContentGenerator/Feedback/feedback_email.txt.ep b/templates/ContentGenerator/Feedback/feedback_email.txt.ep index 24a162a709..3771d933b1 100644 --- a/templates/ContentGenerator/Feedback/feedback_email.txt.ep +++ b/templates/ContentGenerator/Feedback/feedback_email.txt.ep @@ -15,7 +15,6 @@ Feedback sent from: % if ($feedback) { <%== $user->full_name %> (<%== $user->user_id %>) wrote: - <%== $feedback %> % } @@ -96,7 +95,6 @@ Section: <%== $user->section %> Recitation: <%== $user->recitation %> Comment: <%== $user->comment %> IP Address: <%== $remote_host %>:<%== $c->tx->remote_port || 'UNKNOWN' %> - % } % if ($verbosity >= 2) { From c18c6722513889dce0173facb1d7feac246e56a4 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Wed, 2 Apr 2025 22:13:27 -0500 Subject: [PATCH 016/186] Rewrite the WeBWorK::Upload module. This takes advantage of things available with Mojolicious. This does not use `Data::UUID` anymore. A comment stated that it was overkill. It is and it was never a good idea to use that for this purpose. This just uses the `Mojo::Asset::Memory::to_file` method which converts to a `Mojo::Asset::File` object and saves the file to a temporary file using `Mojo::File::tempfile` (which uses `File::Temp` under the hood). That is guaranteed to give a safe filename that does not conflict with any existing files. Setting the environment variable `MOJO_TMPDIR` locally to be the passed in directory (which is `$ce->{webworkDirs}{uploadCache}`) ensures that the file is saved in the usual webwork2 upload location (although there is no need for that, and we could drop that configuration variable and just use the system temporary directory for this). The info file that is written is now UTF-8 encoded and decoded. This fixes issue #2690. The `dir` option which was previously required and the only "option" is now just a required argument. It doesn't make sense to use an "option" hash things that are required. Particularly if there is only one option and it is required. Other than what is mentioned above the module behaves much the same. --- lib/WeBWorK.pm | 15 +- lib/WeBWorK/ContentGenerator/Feedback.pm | 4 +- .../Instructor/FileManager.pm | 22 +- lib/WeBWorK/Upload.pm | 323 ++++++------------ 4 files changed, 131 insertions(+), 233 deletions(-) diff --git a/lib/WeBWorK.pm b/lib/WeBWorK.pm index 95499668ca..50656f8b6a 100644 --- a/lib/WeBWorK.pm +++ b/lib/WeBWorK.pm @@ -122,17 +122,14 @@ async sub dispatch ($c) { my @uploads = @{ $c->req->uploads }; - foreach my $u (@uploads) { - # Make sure it's a "real" upload. - next unless $u->filename; - + for my $u (@uploads) { # Store the upload. - my $upload = WeBWorK::Upload->store($u, dir => $ce->{webworkDirs}{uploadCache}); + my $upload = WeBWorK::Upload->store($u, $ce->{webworkDirs}{uploadCache}); - # Store the upload ID and hash in the file upload field. - my $id = $upload->id; - my $hash = $upload->hash; - $c->param($u->name => "$id $hash"); + # Store the upload temporary file location and hash in the file upload field. + my $tmpFile = $upload->tmpFile; + my $hash = $upload->hash; + $c->param($u->name => "$tmpFile $hash"); } # Create these out here. They should fail if they don't have the right information. diff --git a/lib/WeBWorK/ContentGenerator/Feedback.pm b/lib/WeBWorK/ContentGenerator/Feedback.pm index f4a49766f7..1f21c07c69 100644 --- a/lib/WeBWorK/ContentGenerator/Feedback.pm +++ b/lib/WeBWorK/ContentGenerator/Feedback.pm @@ -174,7 +174,7 @@ sub initialize ($c) { my $fileIDhash = $c->param('attachment'); if ($fileIDhash) { my $attachment = - WeBWorK::Upload->retrieve(split(/\s+/, $fileIDhash), dir => $ce->{webworkDirs}{uploadCache}); + WeBWorK::Upload->retrieve(split(/\s+/, $fileIDhash), $ce->{webworkDirs}{uploadCache}); # Get the filename and read its contents. my $filename = $attachment->filename; @@ -184,7 +184,7 @@ sub initialize ($c) { local $/; $contents = <$fh>; }; - close $fh; + $fh->close; $attachment->dispose; # Check to see that this is an allowed filetype. diff --git a/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm b/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm index 61e48458d6..46933b79dc 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm @@ -720,8 +720,7 @@ sub Upload ($c) { return $c->Refresh; } - my ($id, $hash) = split(/\s+/, $fileIDhash); - my $upload = WeBWorK::Upload->retrieve($id, $hash, dir => $c->{ce}{webworkDirs}{uploadCache}); + my $upload = WeBWorK::Upload->retrieve(split(/\s+/, $fileIDhash), $c->{ce}{webworkDirs}{uploadCache}); my $name = $upload->filename; my $invalidUploadFilenameMsg = $c->checkName($name); @@ -794,21 +793,24 @@ sub Upload ($c) { if ($type ne 'Binary') { my $fh = $upload->fileHandle; my @lines = <$fh>; + $fh->close; $data = join('', @lines); if ($type eq 'Automatic') { $type = isText($data) ? 'Text' : 'Binary' } } if ($type eq 'Text') { $upload->dispose; $data =~ s/\r\n?/\n/g; + + my $backup_data = $data; + my $success = utf8::decode($data); # try to decode as utf8 + unless ($success) { + warn "Trying to convert file $file from latin1? to UTF-8"; + utf8::upgrade($backup_data); # try to convert data from latin1 to utf8. + $data = $backup_data; + } + if (open(my $UPLOAD, '>:encoding(UTF-8)', $file)) { - my $backup_data = $data; - my $success = utf8::decode($data); # try to decode as utf8 - unless ($success) { - warn "Trying to convert file $file from latin1? to UTF-8"; - utf8::upgrade($backup_data); # try to convert data from latin1 to utf8. - $data = $backup_data; - } - print $UPLOAD $data; # print massaged data to file. + print $UPLOAD $data; close $UPLOAD; } else { $c->addbadmessage($c->maketext(q{Can't create file "[_1]": [_2]}, $name, $!)); diff --git a/lib/WeBWorK/Upload.pm b/lib/WeBWorK/Upload.pm index eacd660a37..daeab85aa2 100644 --- a/lib/WeBWorK/Upload.pm +++ b/lib/WeBWorK/Upload.pm @@ -21,275 +21,202 @@ WeBWorK::Upload - store uploads securely across requests. =head1 SYNOPSIS -Given C<$u>, an Mojo::Upload object +Given C<$u>, a C object - my $upload = WeBWorK::Upload->store($u, - dir => $ce->{webworkDirs}->{DATA} - ); - my $id = $upload->id; - my $hash = $upload->hash; + my $upload = WeBWorK::Upload->store($u, $ce->{webworkDirs}{uploadCache}); + my $tmpFile = $upload->tmpFile; + my $hash = $upload->hash; Later... - my $upload = WeBWorK::Upload->retrieve($id, $hash, - dir => $ce->{webworkDirs}->{uploadCache} - ); - my $fh = $upload->fileHandle; - my $path = $upload->filePath; + my $upload = WeBWorK::Upload->retrieve($tmpFile, $hash, $ce->{webworkDirs}{uploadCache}); + my $fh = $upload->fileHandle; + my $path = $upload->filePath; - # get rid of the upload -- $upload is useless after this! - $upload->dispose; + # Get rid of the upload -- $upload is useless after this! + $upload->dispose; - # ...or move it somewhere before disposal - $upload->disposeTo($path); + # Or move it somewhere and dispose of it - $upload is also useless after this! + $upload->disposeTo($path); =head1 DESCRIPTION WeBWorK::Upload provides a method for securely storing uploaded files until such -time as they are needed. This is useful for situations in which an upload cannot -be handled by the system until some later request, such as the case where a user -is not yet authenticated, and a login page must be returned. Since a file upload -should not be sent back to the client and then uploaded again with the user -provides his login information, some proxy must be sent in its place. -WeBWorK::Upload generates a unique ID which can be used to retrieve the original -file. +time as they are needed. This is useful for situations in which an upload needs +to be handled in a later request. WeBWorK::Upload generates a unique temporary +file name and hash which can be used to retrieve the original file. =cut use strict; use warnings; -use Carp qw(croak); -use Data::UUID; # this is probably overkill ;) + +use Carp qw(croak); use Digest::MD5 qw(md5_hex); -use File::Copy qw(copy move); +use Mojo::File qw(path); =head1 STORING UPLOADS -Uploads can be stored in an upload cache -and later retrieved, given the proper ID and hash. The hash is used to confirm -the authenticity of the ID. - -Uploads are Mojo::Upload objects under. - -=head2 CONSTRUCTOR +Uploads can be stored in an upload cache and later retrieved, given the +temporary file name and hash. The hash is used to confirm the authenticity of +the temporary file. -=over +Uploads are constructed from Mojo::Upload objects. -=item store($u, %options) +=head2 store -Stores the Mojo::Upload C<$u> securely. The following keys must be defined in -%options: + my $upload = WeBWorK::Upload->store($u, $dir); - dir => the directory in which to store the uploaded file +Stores the Mojo::Upload C<$u> into the directory specified by C<$dir>. =cut sub store { - my ($invocant, $upload, %options) = @_; + my ($invocant, $upload, $dir) = @_; croak "no upload specified" unless $upload; - # generate UUID - my $ug = new Data::UUID; - my $uuid = $ug->create_str; - - # generate one-time secret - my $secret = sprintf("%X" x 4, map { int rand 2**32 } 1 .. 4); - - # generate hash from $uuid and $secret - my $hash = md5_hex($uuid, $secret); + local $ENV{MOJO_TMPDIR} = $dir; + my $asset = $upload->asset->to_file; + $asset->cleanup(0); + my $tmpFile = path($asset->path)->basename; - my $infoName = "$uuid.info"; - my $infoPath = "$options{dir}/$infoName"; + # Generate a one-time secret. + my $secret = sprintf('%X' x 4, map { int rand 2**32 } 1 .. 4); - my $fileName = "$uuid.file"; - my $filePath = "$options{dir}/$fileName"; - - # get information about uploaded file + # Get the original file name of the uploaded file. my $realFileName = $upload->filename; - # Copy uploaded file - $upload->move_to($filePath); - - # write info file - open my $infoFH, ">", $infoPath - or die "failed to write upload info file $infoPath: $!"; - print $infoFH "$realFileName\n$secret\n"; - close $infoFH; + # Write the info file. + my $infoPath = path($dir)->child("$tmpFile.info"); + eval { $infoPath->spew("$realFileName\n$secret\n", 'UTF-8') }; + die "failed to write upload info file $infoPath: $@" if $@; return bless { - uuid => $uuid, - dir => $options{dir}, - hash => $hash, + tmpFile => $tmpFile, + dir => $dir, + hash => md5_hex($tmpFile, $secret), realFileName => $realFileName, }, ref($invocant) || $invocant; } -=item id +=head2 tmpFile -Return the upload's unique ID, or an undefiend value if the upload is not valid. +Return the temporary file name of the upload, or an undefined value if the +upload is not valid. =cut -sub id { +sub tmpFile { my ($self) = @_; - my $uuid = $self->{uuid}; - my $dir = $self->{dir}; - - my $infoName = "$uuid.info"; - my $infoPath = "$dir/$infoName"; - # make sure info file still exists (i.e. the file hasn't been disposed of) - return unless -e $infoPath; + # Make sure file still exists (i.e. the file hasn't been disposed of). + return unless -e "$self->{dir}/$self->{tmpFile}"; - return $uuid; + return $self->{tmpFile}; } -=item hash +=head2 hash -Return the upload's hash, or an undefiend value if the upload is not valid. +Return the hash of the upload, or an undefined value if the upload is not valid. =cut sub hash { my ($self) = @_; - my $uuid = $self->{uuid}; - my $dir = $self->{dir}; - my $hash = $self->{hash}; - my $infoName = "$uuid.info"; - my $infoPath = "$dir/$infoName"; + # Make sure file still exists (i.e. the file hasn't been disposed of). + return unless -e "$self->{dir}/$self->{tmpFile}"; - # make sure info file still exists (i.e. the file hasn't been disposed of) - return unless -e $infoPath; - - return $hash; + return $self->{hash}; } -=back - =head1 RETRIEVING UPLOADS -An upload stored in the upload cache can be retrieved by supplying its ID and -hash (accessible from the above C and C methods, respectivly. The file -can then be accessed by name or file handle, moved, and disposed of. - -=head2 CONSTRUCTOR - -=over +An upload stored in the upload cache can be retrieved by supplying its temporary +file name and hash (accessible from the above C and C methods), +respectively. The file can then be accessed by name or file handle, moved, and +disposed of. -=item retrieve($id, $hash, %options) +=head2 retrieve -Retrieves the Mojo::Upload referenced by C<$id> and C<$hash>. The following -keys must be defined in %options: + my $upload = WeBWorK::Upload->retrieve($tmpFile, $hash, $dir); - dir => the directory in which to store the uploaded file +Retrieves the upload referenced by C<$tempFile> and C<$hash> and located in +C<$dir>. =cut sub retrieve { - my ($invocant, $uuid, $hash, %options) = @_; - - croak "no upload ID specified" unless $uuid; - croak "no upload hash specified" unless $hash; - - my $infoName = "$uuid.info"; - my $infoPath = "$options{dir}/$infoName"; - - my $fileName = "$uuid.file"; - my $filePath = "$options{dir}/$fileName"; - - croak "no upload matches the ID specified" unless -e $infoPath; + my ($invocant, $tmpFile, $hash, $dir) = @_; - # get real file name and secret from info file - open my $infoFH, "<", $infoPath - or die "failed to read upload info file $infoPath: $!"; - my ($realFileName, $secret) = <$infoFH>; - close $infoFH; + croak 'no upload temporary file name specified' unless $tmpFile; + croak 'no upload hash specified' unless $hash; + croak 'no upload directory specified' unless $dir; - # jesus christ - chomp $realFileName; - chomp $secret; + my $infoPath = path($dir)->child("$tmpFile.info"); - # generate correct hash from $uuid and $secret - my $correctHash = md5_hex($uuid, $secret); + croak 'no upload matches the ID specified' unless -e $infoPath; - #warn __PACKAGE__, ": secret is $secret\n"; - #warn __PACKAGE__, ": correctHash is $correctHash\n"; + # Get the original file name and secret from info file. + my ($realFileName, $secret) = eval { split(/\n/, $infoPath->slurp('UTF-8')) }; + die "failed to read upload info file $infoPath: $@" if $@; - croak "upload hash incorrect!" unless $hash eq $correctHash; + # Generate the correct hash from the $tmpFile and $secret. + my $correctHash = md5_hex($tmpFile, $secret); - # -- you passed the test... -- + croak 'upload hash incorrect!' unless $hash eq $correctHash; return bless { - uuid => $uuid, - dir => $options{dir}, + tmpFile => $tmpFile, + dir => $dir, hash => $hash, realFileName => $realFileName, }, ref($invocant) || $invocant; } -=back - =head2 METHODS -=over - -=item filename +=head3 filename -Returns the original name of the uploaded file. +Returns the original name of the uploaded file, or an undefined value if the +upload is not valid. =cut sub filename { - my ($self) = @_; - my $uuid = $self->{uuid}; - my $dir = $self->{dir}; - my $realFileName = $self->{realFileName}; - - my $infoName = "$uuid.info"; - my $infoPath = "$dir/$infoName"; - - my $fileName = "$uuid.file"; - my $filePath = "$dir/$fileName"; + my ($self) = @_; - # make sure info file still exists (i.e. the file hasn't been disposed of) - return unless -e $infoPath; + # Make sure info file still exists (i.e. the file hasn't been disposed of). + return unless -e "$self->{dir}/$self->{tmpFile}"; - return $realFileName; + return $self->{realFileName}; } -=item fileHandle +=head3 fileHandle -Return a file handle pointing to the uploaded file, or an undefiend value if the -upload is not valid. Suitable for reading. +Return a file handle pointing to the uploaded file suitable for reading, or an +undefined value if the upload is not valid. =cut sub fileHandle { my ($self) = @_; - my $uuid = $self->{uuid}; - my $dir = $self->{dir}; - my $infoName = "$uuid.info"; - my $infoPath = "$dir/$infoName"; + my $filePath = path($self->{dir})->child($self->{tmpFile}); - my $fileName = "$uuid.file"; - my $filePath = "$dir/$fileName"; + # Make sure file still exists (i.e. the file hasn't been disposed of). + return unless -e $filePath; - # make sure info file still exists (i.e. the file hasn't been disposed of) - return unless -e $infoPath; - - open my $fh, "<", $filePath - or die "failed to open upload $filePath for reading: $!"; + my $fh = $filePath->open('<') or die "failed to open upload $filePath for reading: $!"; return $fh; } -=item filePath +=head3 filePath -Return the path to the uploaded file, or an undefiend value if the upload is not +Return the path to the uploaded file, or an undefined value if the upload is not valid. If you use this, bear in mind that you must not dispose of the upload (either by @@ -300,83 +227,55 @@ wish to move the file, use the C method instead. sub filePath { my ($self) = @_; - my $uuid = $self->{uuid}; - my $dir = $self->{dir}; - - my $infoName = "$uuid.info"; - my $infoPath = "$dir/$infoName"; - my $fileName = "$uuid.file"; - my $filePath = "$dir/$fileName"; + my $filePath = "$self->{dir}/$self->{tmpFile}"; - # make sure info file still exists (i.e. the file hasn't been disposed of) - return unless -e $infoPath; + # Make sure file still exists (i.e. the file hasn't been disposed of). + return unless -e $filePath; return $filePath; } -=item dispose +=head3 dispose -Remove the file from the upload cache. Returns true if the upload was -successfully destroyed, or an undefiend value if the upload is not valid. +Remove the file from the upload cache. =cut sub dispose { my ($self) = @_; - my $uuid = $self->{uuid}; - my $dir = $self->{dir}; - - my $infoName = "$uuid.info"; - my $infoPath = "$dir/$infoName"; - - my $fileName = "$uuid.file"; - my $filePath = "$dir/$fileName"; - # make sure info file still exists (i.e. the file hasn't been disposed of) - return unless -e $infoPath; + my $dir = path($self->{dir}); + $dir->child("$self->{tmpFile}.info")->remove; + $dir->child($self->{tmpFile})->remove; - unlink $infoPath; - unlink $filePath; - - return 1; + return; } -=item disposeTo($path) +=head3 disposeTo -Remove the file from the upload cache, and move it to C<$path>. Returns true if -the upload was successfully moved, or an undefiend value if the upload is not -valid. + $upload->diposeTo($path); + +Remove the file from the upload cache, and move it to C<$path>. Returns the +destination as a C object if the upload was successfully moved, or +an undefined value if the upload is not valid. =cut sub disposeTo { my ($self, $newPath) = @_; - my $uuid = $self->{uuid}; - my $dir = $self->{dir}; - croak "no path specified" unless $newPath; + croak 'no path specified' unless $newPath; - my $infoName = "$uuid.info"; - my $infoPath = "$dir/$infoName"; + my $dir = path($self->{dir}); + $dir->child("$self->{tmpFile}.info")->remove; - my $fileName = "$uuid.file"; - my $filePath = "$dir/$fileName"; + my $filePath = $dir->child($self->{tmpFile}); - # make sure info file still exists (i.e. the file hasn't been disposed of) - return unless -e $infoPath; + # Make sure file still exists (i.e. the file hasn't been disposed of). + return unless -e $filePath; - unlink $infoPath; - move($filePath, $newPath); + return $filePath->move_to($newPath); } -=back - -=head1 AUTHOR - -Written by Sam Hathaway, sh002i at math.rochester.edu. Based on the original -WeBWorK::Upload module by Dennis Lambe, Jr., malsyned at math.rochester.edu. - -=cut - 1; From 06c04d9a11e279ee23f90ed82a8384040efec3d7 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Tue, 8 Apr 2025 20:37:58 -0500 Subject: [PATCH 017/186] Convert the conf/database.conf.dist file to a Perl module. The database layout is now defined in the Perl `WeBWorK::DB::Layout` package. An instance of the `WeBWorK::DB` package is no longer instantiated by passing the layout. Instead it is passed a course environment object. An instance of the `WeBWorK::DB::Layout` package is constructed from that. In addition, the `WeBWorK::DB::Driver`, `WeBWorK::DB::Driver::Null`, and `WeBWorK::DB::Driver::SQL.pm` modules have been deleted. Instead there is the `WeBWorK::DB::Database` module that does everything those modules used to do. It is now the only "driver" for all tables. Although, that never should have been called a driver. The driver is still optional and is `DBD::MariaDB` or `DBD::mysql`. That is what is usually referred to as the driver, not the DBI connection handle which is what the `WeBWorK::DB::Database` module deals with, and what the previous "driver" modules did as well. Furthermore, only one instance of this module is constructed when a `WeBWorK::DB` object is constructed, and that is passed to each table. That is essentially the same as before in terms of DBI connection handles except that before there were multiple `WeBWorK::DB::Driver::SQL` instances that all shared the same DBI conneciton handle via the `connect_cached` call. So the layout is simplified considerably. The "driver", "source", "engine", and "character_set" are no longer options for a table, and there are no "%sqlParams" passed to each table. The only thing that is variable in the layout is the "$courseName" used for the tableOverride. The "source", "engine", and "character_set" are parameters of the `WeBWorK::DB::Database` object and retrieved from there as needed. Since there is no choice of database layout there is no point in having that be part of the authentication module selection in the configuration files. So the `$authen{user_module}` in the configuration files should be either a string naming a single authentication module or an array of strings. For example, ```perl $authen{user_module} = [ 'WeBWorK::Authen::LTIAdvantage', 'WeBWorK::Authen::LTIAdvanced', 'WeBWorK::Authen::Basic_TheLastOption' ]; ``` The old format of `{ '*' => 'WeBWorK::Authen::LTIAdvantage' }` (where `*` meant use this authentication for all database layouts) is still allowed for now, but eventually support for that will be dropped. Several related unused files were deleted. Those are * `bin/check_database_charsets.pl` (This should have never been in the repository.) * `bin/wwdb` (This has been broken for a long time.) * `lib/WeBWorK/DB/Driver/Null.pm` (This was mentioned above, but I don't think this ever was used or even made sense to be used.) * `lib/WeBWorK/Utils/CourseManagement/sql_moodle.pm` (This is not used, I doubt this has worked for a long time, and is no longer supported in any case.) * `lib/WeBWorK/Utils/CourseManagement/sql_single.pm` (Also not used and hasn't been working for a while now.) * `lib/WeBWorK/Utils/DBImportExport.pm` (This hasn't worked for a while now and code that called this in `lib/WeBWorK/DB.pm` was removed.) * `lib/WeBWorK/DB/Schema/NewSQL/VersionedMerge.pm` (This is unused. At some point the `lib/WeBWorK/DB/Schema/NewSQL/Merge.pm` module was updated to handle its functionality directly.) --- bin/addcourse | 52 +- bin/change_user_id | 2 +- bin/check_database_charsets.pl | 10 - bin/dump-past-answers.pl | 2 +- bin/importClassList.pl | 4 +- bin/newpassword | 2 +- bin/upgrade-database-to-utf8mb4.pl | 2 +- bin/ww_purge_old_nonces | 2 +- bin/wwdb | 129 ----- bin/wwsh | 2 +- conf/README.md | 2 - conf/authen_CAS.conf.dist | 4 +- conf/authen_LTI.conf.dist | 6 +- conf/authen_ldap.conf.dist | 2 +- conf/database.conf.dist | 358 ------------ conf/defaults.config | 45 +- conf/localOverrides.conf.dist | 30 +- courses.dist/modelCourse/course.conf | 11 - lib/FormatRenderedProblem.pm | 1 - lib/Mojolicious/WeBWorK.pm | 3 +- .../WeBWorK/Tasks/AchievementNotification.pm | 2 +- .../WeBWorK/Tasks/LTIMassUpdate.pm | 2 +- .../WeBWorK/Tasks/SendInstructorEmail.pm | 2 +- lib/WeBWorK.pm | 2 +- lib/WeBWorK/Authen.pm | 34 +- lib/WeBWorK/ContentGenerator/CourseAdmin.pm | 176 +++--- lib/WeBWorK/ContentGenerator/GatewayQuiz.pm | 2 +- lib/WeBWorK/ContentGenerator/LTIAdvanced.pm | 2 +- lib/WeBWorK/ContentGenerator/LTIAdvantage.pm | 11 +- lib/WeBWorK/DB.pm | 165 +++--- lib/WeBWorK/DB/Database.pm | 143 +++++ lib/WeBWorK/DB/Driver.pm | 42 -- lib/WeBWorK/DB/Driver/Null.pm | 37 -- lib/WeBWorK/DB/Driver/SQL.pm | 116 ---- lib/WeBWorK/DB/Layout.pm | 218 ++++++++ lib/WeBWorK/DB/Schema.pm | 19 +- lib/WeBWorK/DB/Schema/NewSQL.pm | 23 +- lib/WeBWorK/DB/Schema/NewSQL/Merge.pm | 10 +- lib/WeBWorK/DB/Schema/NewSQL/NonVersioned.pm | 8 +- lib/WeBWorK/DB/Schema/NewSQL/Std.pm | 153 ++---- lib/WeBWorK/DB/Schema/NewSQL/Versioned.pm | 9 +- .../DB/Schema/NewSQL/VersionedMerge.pm | 208 ------- lib/WeBWorK/DB/Utils.pm | 28 +- lib/WeBWorK/Utils/CourseDBIntegrityCheck.pm | 2 +- lib/WeBWorK/Utils/CourseManagement.pm | 377 +++---------- .../Utils/CourseManagement/sql_moodle.pm | 33 -- .../Utils/CourseManagement/sql_single.pm | 275 ---------- lib/WeBWorK/Utils/DBImportExport.pm | 508 ------------------ lib/WebworkSOAP.pm | 2 +- lib/WebworkWebservice/CourseActions.pm | 7 +- .../ContentGenerator/Base/admin_links.html.ep | 1 - .../CourseAdmin/add_course_form.html.ep | 1 - .../CourseAdmin/upgrade_course_form.html.ep | 16 +- 53 files changed, 728 insertions(+), 2575 deletions(-) delete mode 100755 bin/check_database_charsets.pl delete mode 100755 bin/wwdb delete mode 100644 conf/database.conf.dist create mode 100644 lib/WeBWorK/DB/Database.pm delete mode 100644 lib/WeBWorK/DB/Driver.pm delete mode 100644 lib/WeBWorK/DB/Driver/Null.pm delete mode 100644 lib/WeBWorK/DB/Driver/SQL.pm create mode 100644 lib/WeBWorK/DB/Layout.pm delete mode 100644 lib/WeBWorK/DB/Schema/NewSQL/VersionedMerge.pm delete mode 100644 lib/WeBWorK/Utils/CourseManagement/sql_moodle.pm delete mode 100644 lib/WeBWorK/Utils/CourseManagement/sql_single.pm delete mode 100644 lib/WeBWorK/Utils/DBImportExport.pm diff --git a/bin/addcourse b/bin/addcourse index 81a9587279..e0cdb51581 100755 --- a/bin/addcourse +++ b/bin/addcourse @@ -32,11 +32,6 @@ be granted professor privileges. =over -=item B<--db-layout>=I - -The specified database layout will be used in place of the default specified in -F. - =item B<--users>=I The users listed in the comma-separated text file I will be added to the @@ -77,30 +72,26 @@ BEGIN { use lib "$ENV{WEBWORK_ROOT}/lib"; use WeBWorK::CourseEnvironment; - -# Grab course environment (by reading webwork2/conf/defaults.config) -my $ce = WeBWorK::CourseEnvironment->new; - -use WeBWorK::DB; use WeBWorK::File::Classlist; use WeBWorK::Utils qw(runtime_use cryptPassword); use WeBWorK::Utils::CourseManagement qw(addCourse); use WeBWorK::File::Classlist qw(parse_classlist); +use WeBWorK::DB::Record::User; +use WeBWorK::DB::Record::Password; +use WeBWorK::DB::Record::PermissionLevel; sub usage_error { warn "@_\n"; warn "usage: $0 [options] COURSEID\n"; warn "Options:\n"; - warn " [--db-layout=LAYOUT]\n"; warn " [--users=FILE [--professors=USERID[,USERID]...] ]\n"; exit; } -my ($dbLayout, $users, $templates_from) = ('', '', ''); +my ($users, $templates_from) = ('', ''); my @professors; GetOptions( - "db-layout=s" => \$dbLayout, "users=s" => \$users, "professors=s" => \@professors, "templates-from=s" => \$templates_from, @@ -110,33 +101,16 @@ my $courseID = shift; usage_error('The COURSEID must be provided.') unless $courseID; -$ce = WeBWorK::CourseEnvironment->new({ courseName => $courseID }); +my $ce = WeBWorK::CourseEnvironment->new({ courseName => $courseID }); die "Aborting addcourse: Course ID cannot exceed $ce->{maxCourseIdLength} characters." if length($courseID) > $ce->{maxCourseIdLength}; -if ($dbLayout) { - die "Database layout $dbLayout does not exist in the course environment.", - " (It must be defined in defaults.config.)\n" - unless exists $ce->{dbLayouts}{$dbLayout}; -} else { - $dbLayout = $ce->{dbLayoutName}; -} - usage_error("Can't specify --professors without also specifying --users.") if @professors && !$users; my @users; if ($users) { - # This is a hack to create records without bringing up a DB object - my $userClass = $ce->{dbLayouts}{$dbLayout}{user}{record}; - my $passwordClass = $ce->{dbLayouts}{$dbLayout}{password}{record}; - my $permissionClass = $ce->{dbLayouts}{$dbLayout}{permission}{record}; - - runtime_use($userClass); - runtime_use($passwordClass); - runtime_use($permissionClass); - my @classlist = parse_classlist($users); for my $record (@classlist) { my %record = %$record; @@ -168,9 +142,9 @@ if ($users) { push @users, [ - $userClass->new(%record), - $record{password} ? $passwordClass->new(user_id => $user_id, password => $record{password}) : undef, - $permissionClass->new( + WeBWorK::DB::Record::User->new(%record), + WeBWorK::DB::Record::Password->new(user_id => $user_id, password => $record{password}), + WeBWorK::DB::Record::PermissionLevel->new( user_id => $user_id, permission => defined $professors{$user_id} ? $ce->{userRoles}{professor} @@ -190,15 +164,7 @@ if ($templates_from) { $optional_arguments{copyTemplatesHtml} = 1; } -eval { - addCourse( - courseID => $courseID, - ce => $ce, - courseOptions => { dbLayoutName => $dbLayout }, - users => \@users, - %optional_arguments, - ); -}; +eval { addCourse(courseID => $courseID, ce => $ce, users => \@users, %optional_arguments,); }; die "$@\n" if $@; diff --git a/bin/change_user_id b/bin/change_user_id index 088ec26a81..7acdc335b1 100755 --- a/bin/change_user_id +++ b/bin/change_user_id @@ -47,7 +47,7 @@ my $ce = WeBWorK::CourseEnvironment->new({ courseName => $courseID }); -my $db = new WeBWorK::DB($ce->{dbLayout}); +my $db = WeBWorK::DB->new($ce); die "Error: $old_user_id does not exist!" unless $db->existsUser($old_user_id); unless($db->existsUser($new_user_id)) { diff --git a/bin/check_database_charsets.pl b/bin/check_database_charsets.pl deleted file mode 100755 index c46feb1475..0000000000 --- a/bin/check_database_charsets.pl +++ /dev/null @@ -1,10 +0,0 @@ -#!/usr/bin/env perl - -my $host = $ENV{WEBWORK_DB_HOST}; -my $port = $ENV{WEBWORK_DB_PORT}; -my $database_name = $ENV{WEBWORK_DB_NAME}; -my $database_user = $ENV{WEBWORK_DB_USER}; -my $database_password = $ENV{WEBWORK_DB_PASSWORD}; - -print - `mysql -u $database_user -p$database_password $database_name -h $host -e "SHOW VARIABLES WHERE Variable_name LIKE \'character\_set\_%\' OR Variable_name LIKE \'collation%\' or Variable_name LIKE \'init_connect\' "`; diff --git a/bin/dump-past-answers.pl b/bin/dump-past-answers.pl index 551bb31d3a..a476a54527 100755 --- a/bin/dump-past-answers.pl +++ b/bin/dump-past-answers.pl @@ -137,7 +137,7 @@ sub write_past_answers_csv { next if $courseID eq ($minimal_ce->{admin_course_id} // 'admin') || $courseID eq 'modelCourse'; my $ce = WeBWorK::CourseEnvironment->new({ webwork_dir => $ENV{WEBWORK_ROOT}, courseName => $courseID }); - my $db = WeBWorK::DB->new($ce->{dbLayout}); + my $db = WeBWorK::DB->new($ce); my %permissionLabels = reverse %{ $ce->{userRoles} }; diff --git a/bin/importClassList.pl b/bin/importClassList.pl index 7946ed5c85..d4c21a408b 100755 --- a/bin/importClassList.pl +++ b/bin/importClassList.pl @@ -29,7 +29,7 @@ BEGIN use WeBWorK::CourseEnvironment; -use WeBWorK::DB qw(check_user_id); +use WeBWorK::DB; use WeBWorK::File::Classlist; use WeBWorK::Utils qw(cryptPassword); use WeBWorK::File::Classlist qw(parse_classlist); @@ -49,7 +49,7 @@ BEGIN courseName => $courseID }); -my $db = WeBWorK::DB->new($ce->{dbLayout}); +my $db = WeBWorK::DB->new($ce); my $createNew = 1; # Always set to true, so add new users my $replaceExisting = "none"; # Always set to "none" so no existing accounts are changed diff --git a/bin/newpassword b/bin/newpassword index c627161c75..10944702cb 100755 --- a/bin/newpassword +++ b/bin/newpassword @@ -94,7 +94,7 @@ my $ce = WeBWorK::CourseEnvironment->new({ courseName => $courseID }); -my $db = new WeBWorK::DB($ce->{dbLayout}); +my $db = WeBWorK::DB->new($ce); dopasswd($db, $user, $newP); print "Changed password for $user in $courseID\n"; diff --git a/bin/upgrade-database-to-utf8mb4.pl b/bin/upgrade-database-to-utf8mb4.pl index 3751a2fd30..cadb854694 100755 --- a/bin/upgrade-database-to-utf8mb4.pl +++ b/bin/upgrade-database-to-utf8mb4.pl @@ -212,7 +212,7 @@ BEGIN }, ); -my $db = WeBWorK::DB->new($ce->{dbLayouts}{ $ce->{dbLayoutName} }); +my $db = WeBWorK::DB->new($ce); my @table_types = sort(grep { !$db->{$_}{params}{non_native} } keys %$db); sub checkAndUpdateTableColumnTypes { diff --git a/bin/ww_purge_old_nonces b/bin/ww_purge_old_nonces index 7b99b14dc6..db721627ba 100755 --- a/bin/ww_purge_old_nonces +++ b/bin/ww_purge_old_nonces @@ -72,7 +72,7 @@ my $ce = WeBWorK::CourseEnvironment->new({ courseName => $course, }); -my $db = WeBWorK::DB->new($ce->{dbLayout}); +my $db = WeBWorK::DB->new($ce); my @errors; diff --git a/bin/wwdb b/bin/wwdb deleted file mode 100755 index a739890798..0000000000 --- a/bin/wwdb +++ /dev/null @@ -1,129 +0,0 @@ -#!/usr/bin/env perl -################################################################################ -# WeBWorK Online Homework Delivery System -# Copyright © 2000-2024 The WeBWorK Project, https://github.com/openwebwork -# -# This program is free software; you can redistribute it and/or modify it under -# the terms of either: (a) the GNU General Public License as published by the -# Free Software Foundation; either version 2, or (at your option) any later -# version, or (b) the "Artistic License" which comes with this package. -# -# This program is distributed in the hope that it will be useful, but WITHOUT -# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS -# FOR A PARTICULAR PURPOSE. See either the GNU General Public License or the -# Artistic License for more details. -################################################################################ - -=head1 NAME - -wwdb - export and import webwork databases. - -=head1 SYNOPSIS - - wwdb [-f] course { import | export } file [table ...] - -=head1 DESCRIPTION - -Exports data from a course database to an XML file, or imports data from an XML -file to a course database. Optionally restrict which tables are imported or -exported and specify a duplicate policy. - -=head1 OPTIONS - -=over - -=item -f - -Overwite duplicate records. - -=item course - -Course to use for import or export. - -=item { import | export } - -Specify action -- export or import data. - -=item file - -XML file to write to (in the case of export) or read from (in the case of -import). - -=item [table ...] - -If specified, only the listed tables will be imported or exported. - -=back - -=cut - -use strict; -use warnings; -use Getopt::Std; - -BEGIN { - use Mojo::File qw(curfile); - use Env qw(WEBWORK_ROOT); - - $WEBWORK_ROOT = curfile->dirname->dirname; -} - -use lib "$ENV{WEBWORK_ROOT}/lib"; - -use WeBWorK::CourseEnvironment; -use WeBWorK::DB; -use WeBWorK::Utils::DBImportExport qw/listTables dbExport dbImport/; - -sub usage { - print STDERR "usage: $0 [-f] course { import | export } file [table ...]\n"; - print STDERR "tables: ", join(" ", listTables()), "\n"; - exit 1; -} - -our $opt_f; -getopts("f"); - -my ($course, $command, $file, @tables) = @ARGV; - -usage() unless $course and $command and $file; - -my $ce = WeBWorK::CourseEnvironment->new({ - webwork_dir => $ENV{WEBWORK_ROOT}, - courseName => $course, -}); - -my $db = WeBWorK::DB->new($ce->{dbLayout}); - -my @errors; - -if ($command eq "export") { - my $fh; - if ($file eq "-") { - $fh = *STDOUT; - } else { - open $fh, ">", $file or die "failed to open file '$file' for writing: $!\n"; - } - @errors = dbExport( - db => $db, - xml => $fh, - tables => \@tables, - ); - close $fh; -} elsif ($command eq "import") { - my $conflict = ($opt_f ? "replace" : "skip"); - open my $fh, "<", $file or die "failed to open file '$file' for writing: $!\n"; - @errors = dbImport( - db => $db, - xml => $fh, - tables => \@tables, - conflict => $conflict, - ); - close $fh; -} else { - die "$command: unrecognized command.\n"; -} - -if (@errors) { - warn "The following errors occurred:\n", map { "* $_\n" } @errors; - exit 1; -} diff --git a/bin/wwsh b/bin/wwsh index b5d4eb32d2..25f635a863 100755 --- a/bin/wwsh +++ b/bin/wwsh @@ -54,7 +54,7 @@ $ce = WeBWorK::CourseEnvironment->new({ }); -$db = WeBWorK::DB->new($ce->{dbLayout}); +$db = WeBWorK::DB->new($ce); print <<'EOF'; wwsh - The WeBWorK Shell diff --git a/conf/README.md b/conf/README.md index 4ef53a321d..45ebac0def 100644 --- a/conf/README.md +++ b/conf/README.md @@ -16,8 +16,6 @@ Basic webwork2 configuration files. - `localOverrides.conf.dist` should be copied to `localOverrides.conf`. `localOverrides.conf` will be read after the `defaults.config` file is processed and will overwrite configurations in `defaults.config`. Use this file to make changes to the settings in `defaults.config`. -- `database.conf.dist` contains database configuration parameters. It is included by `defaults.config`. This file - should not be copied or modified unless you really know what you are doing. Configuration extension files. diff --git a/conf/authen_CAS.conf.dist b/conf/authen_CAS.conf.dist index 9c968d0717..4095283e96 100644 --- a/conf/authen_CAS.conf.dist +++ b/conf/authen_CAS.conf.dist @@ -8,9 +8,7 @@ ######################################################################################## # Set CAS as the authentication module to use. -$authen{user_module} = { - "*" => "WeBWorK::Authen::CAS", -}; +$authen{user_module} = 'WeBWorK::Authen::CAS'; # List of authentication modules that may be used to enter the admin course. # This is used instead of $authen{user_module} when logging into the admin course. diff --git a/conf/authen_LTI.conf.dist b/conf/authen_LTI.conf.dist index 9e2ab3ee76..20196f4f49 100644 --- a/conf/authen_LTI.conf.dist +++ b/conf/authen_LTI.conf.dist @@ -40,9 +40,9 @@ $debug_lti_grade_passback = 0; # the LTIAdvantage will be used. If you know a site will not use one or the other, it can be # commented out. Failover to Basic_TheLastOption is necessary to authenticate with cookie keys. $authen{user_module} = [ - { '*' => 'WeBWorK::Authen::LTIAdvantage' }, # first try LTI 1.3 - { '*' => 'WeBWorK::Authen::LTIAdvanced' }, # next try LTI 1.1 - { '*' => 'WeBWorK::Authen::Basic_TheLastOption' } # fallback authorization method + 'WeBWorK::Authen::LTIAdvantage', # first try LTI 1.3 + 'WeBWorK::Authen::LTIAdvanced', # next try LTI 1.1 + 'WeBWorK::Authen::Basic_TheLastOption' # fallback authorization method ]; # List of authentication modules that may be used to enter the admin course. diff --git a/conf/authen_ldap.conf.dist b/conf/authen_ldap.conf.dist index 6dbb1f2895..45cc46179a 100644 --- a/conf/authen_ldap.conf.dist +++ b/conf/authen_ldap.conf.dist @@ -8,7 +8,7 @@ ######################################################################################## # Set LDAP as the authentication module to use. -$authen{user_module} = { "*" => "WeBWorK::Authen::LDAP" }; +$authen{user_module} = 'WeBWorK::Authen::LDAP'; # List of authentication modules that may be used to enter the admin course. # This is used instead of $authen{user_module} when logging into the admin course. diff --git a/conf/database.conf.dist b/conf/database.conf.dist deleted file mode 100644 index eeda93d23e..0000000000 --- a/conf/database.conf.dist +++ /dev/null @@ -1,358 +0,0 @@ -#!perl -################################################################################ -# WeBWorK Online Homework Delivery System -# Copyright © 2000-2024 The WeBWorK Project, https://github.com/openwebwork -# -# This program is free software; you can redistribute it and/or modify it under -# the terms of either: (a) the GNU General Public License as published by the -# Free Software Foundation; either version 2, or (at your option) any later -# version, or (b) the "Artistic License" which comes with this package. -# -# This program is distributed in the hope that it will be useful, but WITHOUT -# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS -# FOR A PARTICULAR PURPOSE. See either the GNU General Public License or the -# Artistic License for more details. -################################################################################ - -=head1 NAME - -database.conf - define standard database layouts - -=head1 SYNOPSIS - -In defaults.config: - - include "conf/database.conf"; - *dbLayout = $dbLayouts{layoutName}; - -=head1 DESCRIPTION - -This file contains definitions for the commonly-used database layouts. Database -layouts consist of all the information necessary to describe how to access data -used by WeBWorK. For more information on the format of a database layout, -consult the documentation for the WeBWorK::DB module. - -A database layout is selected from the list of possible layouts by adding a -line like the one below to the F or F file. - - $dbLayoutName = "layoutName"; - *dbLayout = $dbLayouts{$dbLayoutName}; - -=head2 THE SQL_SINGLE DATABASE LAYOUT - -The C layout is similar to the C layout, except that it uses a -single database for all courses. This is accomplished by prefixing each table -name with the name of the course. The names and passwords of these accounts are -given as parameters to each table in the layout. - - username the username to use when connecting to the database - password the password to use when connecting to the database - -Be default, username is "webworkRead" and password is "". It is not recommended -that you use only a non-empty password to secure database access. Most RDBMSs -allow IP-based authorization as well. As the system administrator, IT IS YOUR -RESPONSIBILITY TO SECURE DATABASE ACCESS. - -Don't confuse the account information above with the accounts of the users of a -course. This is a system-wide account which allow WeBWorK to talk to the -database server. - -Other parameters that can be given are as follows: - - tableOverride an alternate name to use when referring to the table (used - when a table name is a reserved word) - debug if true, SQL statements are printed before being executed - -=cut - -# params common to all tables - -my %sqlParams = ( - username => $database_username, - password => $database_password, - debug => $database_debug, - # kinda hacky, but needed for table dumping - mysql_path => $externalPrograms{mysql}, - mysqldump_path => $externalPrograms{mysqldump}, -); - -if ($ce->{database_driver} =~ /^mysql$/i) { - # The extra UTF8 connection setting is ONLY needed for older DBD:mysql driver - # and forbidden by the newer DBD::MariaDB driver - if ($ENABLE_UTF8MB4) { - $sqlParams{mysql_enable_utf8mb4} = 1; # Full 4-bit UTF-8 - } else { - $sqlParams{mysql_enable_utf8} = 1; # Only the partial 3-bit mySQL UTF-8 - } -} - -%dbLayouts = (); # layouts are added to this hash below - -$dbLayouts{sql_single} = { - locations => { - record => "WeBWorK::DB::Record::Locations", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, non_native => 1 }, - }, - location_addresses => { - record => "WeBWorK::DB::Record::LocationAddresses", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, non_native => 1 }, - }, - depths => { - record => "WeBWorK::DB::Record::Depths", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - params => { %sqlParams, non_native => 1 }, - }, - lti_launch_data => { - record => "WeBWorK::DB::Record::LTILaunchData", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, non_native => 1 }, - }, - lti_course_map => { - record => "WeBWorK::DB::Record::LTICourseMap", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, non_native => 1 }, - }, - password => { - record => "WeBWorK::DB::Record::Password", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, tableOverride => "${courseName}_password" }, - }, - permission => { - record => "WeBWorK::DB::Record::PermissionLevel", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, tableOverride => "${courseName}_permission" }, - }, - key => { - record => "WeBWorK::DB::Record::Key", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, tableOverride => "${courseName}_key" }, - }, - user => { - record => "WeBWorK::DB::Record::User", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, tableOverride => "${courseName}_user" }, - }, - set => { - record => "WeBWorK::DB::Record::Set", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, tableOverride => "${courseName}_set" }, - }, - set_user => { - record => "WeBWorK::DB::Record::UserSet", - schema => "WeBWorK::DB::Schema::NewSQL::NonVersioned", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, tableOverride => "${courseName}_set_user" }, - }, - set_merged => { - record => "WeBWorK::DB::Record::UserSet", - schema => "WeBWorK::DB::Schema::NewSQL::Merge", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - depend => [qw/set_user set/], - params => { - %sqlParams, - non_native => 1, - merge => [qw/set_user set/], - }, - }, - set_version => { - record => "WeBWorK::DB::Record::SetVersion", - schema => "WeBWorK::DB::Schema::NewSQL::Versioned", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - params => { - %sqlParams, - non_native => 1, - tableOverride => "${courseName}_set_user", - - }, - }, - set_version_merged => { - record => "WeBWorK::DB::Record::SetVersion", - schema => "WeBWorK::DB::Schema::NewSQL::Merge", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - depend => [qw/set_version set_user set/], - params => { - %sqlParams, - non_native => 1, - merge => [qw/set_version set_user set/], - }, - }, - set_locations => { - record => "WeBWorK::DB::Record::SetLocations", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, tableOverride => "${courseName}_set_locations" }, - }, - set_locations_user => { - record => "WeBWorK::DB::Record::UserSetLocations", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, tableOverride => "${courseName}_set_locations_user" }, - }, - problem => { - record => "WeBWorK::DB::Record::Problem", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, tableOverride => "${courseName}_problem" }, - }, - problem_user => { - record => "WeBWorK::DB::Record::UserProblem", - schema => "WeBWorK::DB::Schema::NewSQL::NonVersioned", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, tableOverride => "${courseName}_problem_user" }, - }, - problem_merged => { - record => "WeBWorK::DB::Record::UserProblem", - schema => "WeBWorK::DB::Schema::NewSQL::Merge", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - depend => [qw/problem_user problem/], - params => { - %sqlParams, - non_native => 1, - merge => [qw/problem_user problem/], - }, - }, - problem_version => { - record => "WeBWorK::DB::Record::ProblemVersion", - schema => "WeBWorK::DB::Schema::NewSQL::Versioned", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { - %sqlParams, - non_native => 1, - tableOverride => "${courseName}_problem_user", - }, - }, - problem_version_merged => { - record => "WeBWorK::DB::Record::ProblemVersion", - schema => "WeBWorK::DB::Schema::NewSQL::Merge", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - depend => [qw/problem_version problem_user problem/], - params => { - %sqlParams, - non_native => 1, - merge => [qw/problem_version problem_user problem/], - }, - }, - setting => { - record => "WeBWorK::DB::Record::Setting", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, tableOverride => "${courseName}_setting" }, - }, - achievement => { - record => "WeBWorK::DB::Record::Achievement", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, tableOverride => "${courseName}_achievement" }, - }, - past_answer => { - record => "WeBWorK::DB::Record::PastAnswer", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, tableOverride => "${courseName}_past_answer" }, - }, - - achievement_user => { - record => "WeBWorK::DB::Record::UserAchievement", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, tableOverride => "${courseName}_achievement_user" }, - }, - global_user_achievement => { - record => "WeBWorK::DB::Record::GlobalUserAchievement", - schema => "WeBWorK::DB::Schema::NewSQL::Std", - driver => "WeBWorK::DB::Driver::SQL", - source => $database_dsn, - engine => $database_storage_engine, - character_set => $database_character_set, - params => { %sqlParams, tableOverride => "${courseName}_global_user_achievement" }, - }, -}; - -# include ("conf/database.conf"); # uncomment to provide local overrides - -1; diff --git a/conf/defaults.config b/conf/defaults.config index 5d607fed0f..dfd35768ff 100644 --- a/conf/defaults.config +++ b/conf/defaults.config @@ -599,34 +599,14 @@ $default_status = "Enrolled"; # Database options ################################################################################ -# Database schemas are defined in the file conf/database.conf and stored in the -# hash %dbLayouts. The standard schema is called "sql_single"; - -include( "./conf/database.conf.dist"); # always include database.conf.dist - - # in the rare case where you want local overrides - # you can place include("conf/database.conf") in - # the database.conf.dist file -# this change is meant to help alleviate the common mistake of forgetting to update the -# database.conf file when changing WW versions. - -# Select the default database layout. This can be overridden in the course.conf -# file of a particular course. The only database layout supported in WW 2.1.4 -# and up is "sql_single". -$dbLayoutName = "sql_single"; - -# This sets the symbol "dbLayout" as an alias for the selected database layout. -*dbLayout = $dbLayouts{$dbLayoutName}; - # This sets the max course id length. It might need to be changed depending # on what database tables are present. Mysql allows a max table length of 64 # characters. With the ${course_id}_global_user_achievement table that means # the max ${course_id} is exactly 40 characters. +# Reference: https://dev.mysql.com/doc/refman/8.0/en/identifier-length.html $maxCourseIdLength = 40; -# Reference: https://dev.mysql.com/doc/refman/8.0/en/identifier-length.html - ################################################################################ # Problem library options ################################################################################ @@ -720,25 +700,14 @@ $modelCoursesForCopy = [ "modelCourse" ]; # Authentication system ################################################################################ -# FIXME This mechanism is a little awkward and probably should be merged with -# the dblayout selection system somehow. - # Select the authentication module to use for normal logins. -# -# If this value is a string, the given authentication module will be used -# regardless of the database layout. -# -# If it is a hash, the database layout name will be looked up in the hash and -# the resulting value will be used as the authentication module. The special -# hash key "*" is used if no entry for the current database layout is found. -# -# If this value is a sequence of strings or hashes, then each string or hash in -# the sequence will be successively tested to see if it provides a module that -# can handle the authentication request by calling the module's sub -# request_has_data_for_this_verification_module(). The first module that +# If this value is a string, then that authentication module will be used. If +# this value is a reference to an array of strings, then each string in the +# array will be successively tested to see if it provides a module that can +# handle the authentication request (by calling that module's +# request_has_data_for_this_verification_module method). The first module that # responds affirmatively will be used. - -$authen{user_module} = {"*" => "WeBWorK::Authen::Basic_TheLastOption"}; +$authen{user_module} = 'WeBWorK::Authen::Basic_TheLastOption'; # Select the authentication module to use for proctor logins. # A string or a hash is accepted, as above. diff --git a/conf/localOverrides.conf.dist b/conf/localOverrides.conf.dist index 5a19b9e0a4..766795ec32 100644 --- a/conf/localOverrides.conf.dist +++ b/conf/localOverrides.conf.dist @@ -26,14 +26,12 @@ # localOverrides.conf contains the local modifications commonly made # when installing WeBWorK on a new site. The configurations in defaults.config -# and in database.conf can usually remain untouched. +# should not be changed. # # localOverride.conf is the appropriate place to override permission settings, # paths to macros and other customizations that are specific to your # WeBWorK site - - ################################################################################ # Additional mail settings in defaults.config can be overridden here ################################################################################ @@ -460,24 +458,16 @@ $mail{feedbackRecipients} = [ # methods of authentication. # Select the authentication module to use for normal logins. -# -# If this value is a string, the given authentication module will be used -# regardless of the database layout. -# -# If it is a hash, the database layout name will be looked up in the hash and -# the resulting value will be used as the authentication module. The special -# hash key "*" is used if no entry for the current database layout is found. -# -# If this value is a sequence of strings or hashes, then each string or hash in -# the sequence will be successively tested to see if it provides a module that -# can handle the authentication request by calling the module's sub -# request_has_data_for_this_verification_module(). The first module that +# If this value is a string, then that authentication module will be used. If +# this value is a reference to an array of strings, then each string in the +# array will be successively tested to see if it provides a module that can +# handle the authentication request (by calling that module's +# request_has_data_for_this_verification_module method). The first module that # responds affirmatively will be used. - -#$authen{user_module} = { -# "*" => "WeBWorK::Authen::LDAP" -# "*" => "WeBWorK::Authen::Basic_TheLastOption" -#}; +#$authen{user_module} = [ +# "WeBWorK::Authen::LDAP", +# "WeBWorK::Authen::Basic_TheLastOption" +#]; # Select the authentication module to use for proctor logins. # A string or a hash is accepted, as above. diff --git a/courses.dist/modelCourse/course.conf b/courses.dist/modelCourse/course.conf index fb80c242a5..ff74987a9a 100644 --- a/courses.dist/modelCourse/course.conf +++ b/courses.dist/modelCourse/course.conf @@ -2,17 +2,6 @@ # This file is used to override the global WeBWorK course environment for this course. -# Database Layout (global value typically defined in global.conf) -# Several database are defined in the file conf/database.conf and stored in the -# hash %dbLayouts. -# The database layout is always set here, since one should be able to change the -# default value in global.conf without disrupting existing courses. -# global.conf values: -# $dbLayoutName = 'sql_single'; -# *dbLayout = $dbLayouts{$dbLayoutName}; -$dbLayoutName = 'sql_single'; -*dbLayout = $dbLayouts{$dbLayoutName}; - # Users for whom to label problems with the PG file name # For users in this list, PG will display the source file name when rendering a problem. #$pg{specialPGEnvironmentVars}{PRINT_FILE_NAMES_FOR} = ['user_id1']; diff --git a/lib/FormatRenderedProblem.pm b/lib/FormatRenderedProblem.pm index 785caaed14..dbbc2754c7 100644 --- a/lib/FormatRenderedProblem.pm +++ b/lib/FormatRenderedProblem.pm @@ -430,7 +430,6 @@ sub pretty_print { # certain internals of the CourseEnvironment in case one slips in. next if (($key =~ /database/) - || ($key =~ /dbLayout/) || ($key eq "ConfigValues") || ($key eq "ENV") || ($key eq "externalPrograms") diff --git a/lib/Mojolicious/WeBWorK.pm b/lib/Mojolicious/WeBWorK.pm index 3a881fee2a..1dd4c44eb0 100644 --- a/lib/Mojolicious/WeBWorK.pm +++ b/lib/Mojolicious/WeBWorK.pm @@ -178,8 +178,7 @@ sub startup ($app) { writeTimingLogEntry( $c->ce, '[' . $c->url_for . ']', - sprintf('runTime = %.3f sec', $c->timing->elapsed('content_generator_rendering')) . ' ' - . $c->ce->{dbLayoutName} + sprintf('runTime = %.3f sec', $c->timing->elapsed('content_generator_rendering')) ); } } diff --git a/lib/Mojolicious/WeBWorK/Tasks/AchievementNotification.pm b/lib/Mojolicious/WeBWorK/Tasks/AchievementNotification.pm index 7a42b1787b..73a97e302e 100644 --- a/lib/Mojolicious/WeBWorK/Tasks/AchievementNotification.pm +++ b/lib/Mojolicious/WeBWorK/Tasks/AchievementNotification.pm @@ -39,7 +39,7 @@ sub run ($job, $mail_data) { $job->{language_handle} = WeBWorK::Localize::getLoc($ce->{language} || 'en'); - my $db = WeBWorK::DB->new($ce->{dbLayout}); + my $db = WeBWorK::DB->new($ce); return $job->fail($job->maketext('Could not obtain database connection for [_1].', $courseID)) unless $db; diff --git a/lib/Mojolicious/WeBWorK/Tasks/LTIMassUpdate.pm b/lib/Mojolicious/WeBWorK/Tasks/LTIMassUpdate.pm index 83d3001a9e..86e638b60e 100644 --- a/lib/Mojolicious/WeBWorK/Tasks/LTIMassUpdate.pm +++ b/lib/Mojolicious/WeBWorK/Tasks/LTIMassUpdate.pm @@ -36,7 +36,7 @@ sub run ($job, $userID = '', $setID = '') { $job->{language_handle} = WeBWorK::Localize::getLoc($ce->{language} || 'en'); - my $db = WeBWorK::DB->new($ce->{dbLayout}); + my $db = WeBWorK::DB->new($ce); return $job->fail($job->maketext('Could not obtain database connection.')) unless $db; my @messages; diff --git a/lib/Mojolicious/WeBWorK/Tasks/SendInstructorEmail.pm b/lib/Mojolicious/WeBWorK/Tasks/SendInstructorEmail.pm index 3f9c177e19..db397129c0 100644 --- a/lib/Mojolicious/WeBWorK/Tasks/SendInstructorEmail.pm +++ b/lib/Mojolicious/WeBWorK/Tasks/SendInstructorEmail.pm @@ -36,7 +36,7 @@ sub run ($job, $mail_data) { $job->{language_handle} = WeBWorK::Localize::getLoc($ce->{language} || 'en'); - my $db = WeBWorK::DB->new($ce->{dbLayout}); + my $db = WeBWorK::DB->new($ce); return $job->fail($job->maketext('Could not obtain database connection.')) unless $db; my @result_messages = eval { $job->mail_message_to_recipients($ce, $db, $mail_data) }; diff --git a/lib/WeBWorK.pm b/lib/WeBWorK.pm index 50656f8b6a..a468bec896 100644 --- a/lib/WeBWorK.pm +++ b/lib/WeBWorK.pm @@ -157,7 +157,7 @@ async sub dispatch ($c) { || -e "$ce->{webwork_courses_dir}/$ce->{admin_course_id}/archives/$routeCaptures{courseID}.tar.gz"); return (0, 'This course has been archived and closed.') unless -e $ce->{courseDirs}{root}; - my $db = WeBWorK::DB->new($ce->{dbLayout}); + my $db = WeBWorK::DB->new($ce); debug("(here's the DB handle: $db)\n"); $c->db($db); diff --git a/lib/WeBWorK/Authen.pm b/lib/WeBWorK/Authen.pm index ec034439b6..6549cc2700 100644 --- a/lib/WeBWorK/Authen.pm +++ b/lib/WeBWorK/Authen.pm @@ -94,29 +94,21 @@ sub class { my ($ce, $type) = @_; if (exists $ce->{authen}{$type}) { - if (ref $ce->{authen}{$type} eq "ARRAY") { + if (ref $ce->{authen}{$type} eq 'ARRAY') { my $authen_type = shift @{ $ce->{authen}{$type} }; - if (ref($authen_type) eq "HASH") { - if (exists $authen_type->{ $ce->{dbLayoutName} }) { - return $authen_type->{ $ce->{dbLayoutName} }; - } elsif (exists $authen_type->{"*"}) { - return $authen_type->{"*"}; - } else { - die "authentication type '$type' in the course environment has no entry for db layout '", - $ce->{dbLayoutName}, "' and no default entry (*)"; - } + if (ref($authen_type) eq 'HASH') { + # Basic backwards compatibility. + return $authen_type->{'*'} if exists $authen_type->{'*'}; + return $authen_type->{sql_single} if exists $authen_type->{sql_single}; + die 'Unsupported authentication module format in the course environment.'; } else { return $authen_type; } - } elsif (ref $ce->{authen}{$type} eq "HASH") { - if (exists $ce->{authen}{$type}{ $ce->{dbLayoutName} }) { - return $ce->{authen}{$type}{ $ce->{dbLayoutName} }; - } elsif (exists $ce->{authen}{$type}{"*"}) { - return $ce->{authen}{$type}{"*"}; - } else { - die "authentication type '$type' in the course environment has no entry for db layout '", - $ce->{dbLayoutName}, "' and no default entry (*)"; - } + } elsif (ref $ce->{authen}{$type} eq 'HASH') { + # Basic backwards compatibility. + return $ce->{authen}{$type}{'*'} if exists $ce->{authen}{$type}{'*'}; + return $ce->{authen}{$type}{sql_single} if exists $ce->{authen}{$type}{sql_single}; + die 'Unsupported authentication module format in the course environment.'; } else { return $ce->{authen}{$type}; } @@ -130,9 +122,7 @@ sub call_next_authen_method { my $c = $self->{c}; my $ce = $c->{ce}; - my $user_authen_module = - WeBWorK::Authen::class($ce, $ce->{courseName} eq $ce->{admin_course_id} ? 'admin_module' : 'user_module'); - + my $user_authen_module = class($ce, $ce->{courseName} eq $ce->{admin_course_id} ? 'admin_module' : 'user_module'); if (!defined $user_authen_module || $user_authen_module eq '') { $self->{error} = $c->maketext( "No authentication method found for your request. If this recurs, please speak with your instructor."); diff --git a/lib/WeBWorK/ContentGenerator/CourseAdmin.pm b/lib/WeBWorK/ContentGenerator/CourseAdmin.pm index 479930a4dc..3ef0c02666 100644 --- a/lib/WeBWorK/ContentGenerator/CourseAdmin.pm +++ b/lib/WeBWorK/ContentGenerator/CourseAdmin.pm @@ -40,7 +40,6 @@ use WeBWorK::DB; sub pre_header_initialize ($c) { my $ce = $c->ce; - my $db = $c->db; my $authz = $c->authz; my $user = $c->param('user'); @@ -48,7 +47,7 @@ sub pre_header_initialize ($c) { # Check that the non-native tables are present in the database. # These are the tables which are not course specific. - my @table_update_messages = initNonNativeTables($ce, $ce->{dbLayoutName}); + my @table_update_messages = initNonNativeTables($ce); $c->addgoodmessage($c->c(@table_update_messages)->join($c->tag('br'))) if @table_update_messages; my @errors; @@ -251,7 +250,6 @@ sub add_course_validate ($c) { my $add_courseID = trim_spaces($c->param('new_courseID')) || ''; my $number_of_additional_users = $c->param('number_of_additional_users') || 0; - my $add_dbLayout = trim_spaces($c->param('add_dbLayout')) || ''; my @errors; @@ -281,17 +279,6 @@ sub add_course_validate ($c) { } } - if ($add_dbLayout eq '') { - push @errors, 'You must select a database layout.'; - } else { - if (exists $ce->{dbLayouts}{$add_dbLayout}) { - # we used to check for layout-specific fields here, but there aren't any layouts that require them - # anymore. (in the future, we'll probably deal with this in layout-specific modules.) - } else { - push @errors, "The database layout $add_dbLayout doesn't exist."; - } - } - return @errors; } @@ -307,11 +294,9 @@ sub do_add_course ($c) { my $copy_from_course = trim_spaces($c->param('copy_from_course')) // ''; - my $add_dbLayout = trim_spaces($c->param('add_dbLayout')) || ''; - my $ce2 = WeBWorK::CourseEnvironment->new({ courseName => $add_courseID }); - my %courseOptions = (dbLayoutName => $add_dbLayout); + my %courseOptions; my @users; @@ -487,9 +472,8 @@ sub rename_course_confirm ($c) { # Create strings confirming title and institution change. # Connect to the database to get old title and institution. - my $dbLayoutName = $ce->{dbLayoutName}; - my $db = WeBWorK::DB->new($ce->{dbLayouts}{$dbLayoutName}); - my $oldDB = WeBWorK::DB->new($ce2->{dbLayouts}{$dbLayoutName}); + my $db = WeBWorK::DB->new($ce); + my $oldDB = WeBWorK::DB->new($ce2); my $rename_oldCourseTitle = $oldDB->getSettingValue('courseTitle') // ''; my $rename_oldCourseInstitution = $oldDB->getSettingValue('courseInstitution') // ''; @@ -513,49 +497,45 @@ sub rename_course_confirm ($c) { rename_oldCourseID => $rename_oldCourseID ) unless $c->param('rename_newCourseID_checkbox'); - if ($ce2->{dbLayoutName}) { - my $CIchecker = WeBWorK::Utils::CourseDBIntegrityCheck->new($ce2); - - # Check database - my ($tables_ok, $dbStatus) = $CIchecker->checkCourseTables($rename_oldCourseID); + my $CIchecker = WeBWorK::Utils::CourseDBIntegrityCheck->new($ce2); - # Upgrade the database if requested. - my @upgrade_report; - if ($c->param('upgrade_course_tables')) { - my @schema_table_names = keys %$dbStatus; - my @tables_to_create = - grep { $dbStatus->{$_}->[0] == WeBWorK::Utils::CourseDBIntegrityCheck::ONLY_IN_A } @schema_table_names; - my @tables_to_alter = - grep { $dbStatus->{$_}->[0] == WeBWorK::Utils::CourseDBIntegrityCheck::DIFFER_IN_A_AND_B } - @schema_table_names; - push(@upgrade_report, $CIchecker->updateCourseTables($rename_oldCourseID, [@tables_to_create])); - for my $table_name (@tables_to_alter) { - push(@upgrade_report, $CIchecker->updateTableFields($rename_oldCourseID, $table_name)); - } + # Check database + my ($tables_ok, $dbStatus) = $CIchecker->checkCourseTables($rename_oldCourseID); - ($tables_ok, $dbStatus) = $CIchecker->checkCourseTables($rename_oldCourseID); + # Upgrade the database if requested. + my @upgrade_report; + if ($c->param('upgrade_course_tables')) { + my @schema_table_names = keys %$dbStatus; + my @tables_to_create = + grep { $dbStatus->{$_}->[0] == WeBWorK::Utils::CourseDBIntegrityCheck::ONLY_IN_A } @schema_table_names; + my @tables_to_alter = + grep { $dbStatus->{$_}->[0] == WeBWorK::Utils::CourseDBIntegrityCheck::DIFFER_IN_A_AND_B } + @schema_table_names; + push(@upgrade_report, $CIchecker->updateCourseTables($rename_oldCourseID, [@tables_to_create])); + for my $table_name (@tables_to_alter) { + push(@upgrade_report, $CIchecker->updateTableFields($rename_oldCourseID, $table_name)); } - # Check directories - my ($directories_ok, $directory_report) = checkCourseDirectories($ce2); - - return $c->include( - 'ContentGenerator/CourseAdmin/rename_course_confirm', - upgrade_report => \@upgrade_report, - tables_ok => $tables_ok, - dbStatus => $dbStatus, - directory_report => $directory_report, - directories_ok => $directories_ok, - rename_oldCourseTitle => $rename_oldCourseTitle, - change_course_title_str => $change_course_title_str, - rename_oldCourseInstitution => $rename_oldCourseInstitution, - change_course_institution_str => $change_course_institution_str, - rename_oldCourseID => $rename_oldCourseID, - rename_newCourseID => $rename_newCourseID - ); - } else { - return $c->tag('p', class => 'text-danger fw-bold', "Unable to find database layout for $rename_oldCourseID"); + ($tables_ok, $dbStatus) = $CIchecker->checkCourseTables($rename_oldCourseID); } + + # Check directories + my ($directories_ok, $directory_report) = checkCourseDirectories($ce2); + + return $c->include( + 'ContentGenerator/CourseAdmin/rename_course_confirm', + upgrade_report => \@upgrade_report, + tables_ok => $tables_ok, + dbStatus => $dbStatus, + directory_report => $directory_report, + directories_ok => $directories_ok, + rename_oldCourseTitle => $rename_oldCourseTitle, + change_course_title_str => $change_course_title_str, + rename_oldCourseInstitution => $rename_oldCourseInstitution, + change_course_institution_str => $change_course_institution_str, + rename_oldCourseID => $rename_oldCourseID, + rename_newCourseID => $rename_newCourseID + ); } sub rename_course_validate ($c) { @@ -980,48 +960,44 @@ sub archive_course_confirm ($c) { my $ce2 = WeBWorK::CourseEnvironment->new({ courseName => $archive_courseID }); - if ($ce2->{dbLayoutName}) { - my $CIchecker = WeBWorK::Utils::CourseDBIntegrityCheck->new($ce2); - - # Check database - my ($tables_ok, $dbStatus) = $CIchecker->checkCourseTables($archive_courseID); + my $CIchecker = WeBWorK::Utils::CourseDBIntegrityCheck->new($ce2); - # Upgrade the database if requested. - my @upgrade_report; - if ($c->param('upgrade_course_tables')) { - my @schema_table_names = keys %$dbStatus; - my @tables_to_create = - grep { $dbStatus->{$_}->[0] == WeBWorK::Utils::CourseDBIntegrityCheck::ONLY_IN_A } @schema_table_names; - my @tables_to_alter = - grep { $dbStatus->{$_}->[0] == WeBWorK::Utils::CourseDBIntegrityCheck::DIFFER_IN_A_AND_B } - @schema_table_names; - push(@upgrade_report, $CIchecker->updateCourseTables($archive_courseID, [@tables_to_create])); - for my $table_name (@tables_to_alter) { - push(@upgrade_report, $CIchecker->updateTableFields($archive_courseID, $table_name)); - } + # Check database + my ($tables_ok, $dbStatus) = $CIchecker->checkCourseTables($archive_courseID); - ($tables_ok, $dbStatus) = $CIchecker->checkCourseTables($archive_courseID); + # Upgrade the database if requested. + my @upgrade_report; + if ($c->param('upgrade_course_tables')) { + my @schema_table_names = keys %$dbStatus; + my @tables_to_create = + grep { $dbStatus->{$_}->[0] == WeBWorK::Utils::CourseDBIntegrityCheck::ONLY_IN_A } @schema_table_names; + my @tables_to_alter = + grep { $dbStatus->{$_}->[0] == WeBWorK::Utils::CourseDBIntegrityCheck::DIFFER_IN_A_AND_B } + @schema_table_names; + push(@upgrade_report, $CIchecker->updateCourseTables($archive_courseID, [@tables_to_create])); + for my $table_name (@tables_to_alter) { + push(@upgrade_report, $CIchecker->updateTableFields($archive_courseID, $table_name)); } - # Update and check directories. - my $dir_update_messages = $c->param('upgrade_course_tables') ? updateCourseDirectories($ce2) : []; - my ($directories_ok, $directory_report) = checkCourseDirectories($ce2); - - return $c->include( - 'ContentGenerator/CourseAdmin/archive_course_confirm', - ce2 => $ce2, - upgrade_report => \@upgrade_report, - tables_ok => $tables_ok, - dbStatus => $dbStatus, - dir_update_messages => $dir_update_messages, - directory_report => $directory_report, - directories_ok => $directories_ok, - archive_courseID => $archive_courseID, - archive_courseIDs => \@archive_courseIDs - ); - } else { - return $c->tag('p', class => 'text-danger fw-bold', "Unable to find database layout for $archive_courseID"); + ($tables_ok, $dbStatus) = $CIchecker->checkCourseTables($archive_courseID); } + + # Update and check directories. + my $dir_update_messages = $c->param('upgrade_course_tables') ? updateCourseDirectories($ce2) : []; + my ($directories_ok, $directory_report) = checkCourseDirectories($ce2); + + return $c->include( + 'ContentGenerator/CourseAdmin/archive_course_confirm', + ce2 => $ce2, + upgrade_report => \@upgrade_report, + tables_ok => $tables_ok, + dbStatus => $dbStatus, + dir_update_messages => $dir_update_messages, + directory_report => $directory_report, + directories_ok => $directories_ok, + archive_courseID => $archive_courseID, + archive_courseIDs => \@archive_courseIDs + ); } sub do_archive_course ($c) { @@ -1247,7 +1223,7 @@ sub do_unarchive_course ($c) { if ($c->param('clean_up_course')) { my $ce_new = WeBWorK::CourseEnvironment->new({ courseName => $new_courseID }); - my $db_new = WeBWorK::DB->new($ce_new->{dbLayout}); + my $db_new = WeBWorK::DB->new($ce_new); for my $student_id ($db_new->listPermissionLevelsWhere({ permission => $ce->{userRoles}{student} })) { $db_new->deleteUser($student_id->[0]); @@ -2450,7 +2426,7 @@ sub manage_otp_secrets_form ($c) { # Create course data first, since it is used in all cases and initializes course db references. for my $courseID (listCourses($c->ce)) { my $ce = WeBWorK::CourseEnvironment->new({ courseName => $courseID }); - $dbs->{$courseID} = WeBWorK::DB->new($ce->{dbLayouts}{ $ce->{dbLayoutName} }); + $dbs->{$courseID} = WeBWorK::DB->new($ce); # By default ignore courses larger than 200 users, as this can cause a large load building menus. my @users = $dbs->{$courseID}->listUsers; @@ -2558,7 +2534,7 @@ sub copy_otp_secrets_confirm ($c) { my @rows; my %dbs; my $source_ce = WeBWorK::CourseEnvironment->new({ courseName => $source_course }); - $dbs{$source_course} = WeBWorK::DB->new($source_ce->{dbLayouts}{ $source_ce->{dbLayoutName} }); + $dbs{$source_course} = WeBWorK::DB->new($source_ce); for my $s_user (@source_users) { my $s_user_password = $dbs{$source_course}->getPassword($s_user); @@ -2586,7 +2562,7 @@ sub copy_otp_secrets_confirm ($c) { unless ($dbs{$d_course}) { my $dest_ce = WeBWorK::CourseEnvironment->new({ courseName => $d_course }); - $dbs{$d_course} = WeBWorK::DB->new($dest_ce->{dbLayouts}{ $dest_ce->{dbLayoutName} }); + $dbs{$d_course} = WeBWorK::DB->new($dest_ce); } my $d_user_password = $dbs{$d_course}->getPassword($d_user); @@ -2635,7 +2611,7 @@ sub reset_otp_secrets_confirm ($c) { } my $ce = WeBWorK::CourseEnvironment->new({ courseName => $source_course }); - my $db = WeBWorK::DB->new($ce->{dbLayouts}{ $ce->{dbLayoutName} }); + my $db = WeBWorK::DB->new($ce); my @rows; for my $user (@dest_users) { my $password = $db->getPassword($user); diff --git a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm index 488c0a63a5..dab92494a6 100644 --- a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm +++ b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm @@ -439,7 +439,7 @@ async sub pre_header_initialize ($c) { } else { # If there is not a requested version or a latest version, then create dummy set to proceed. # FIXME RETURN TO: should this be global2version? - $set = global2user($ce->{dbLayout}{set_version}{record}, $db->getGlobalSet($setID)); + $set = global2user($db->{set_version}{record}, $db->getGlobalSet($setID)); $set->user_id($effectiveUserID); $set->psvn('000'); $set->set_id($setID); # redundant? diff --git a/lib/WeBWorK/ContentGenerator/LTIAdvanced.pm b/lib/WeBWorK/ContentGenerator/LTIAdvanced.pm index d07a5c5620..2d8e655f23 100644 --- a/lib/WeBWorK/ContentGenerator/LTIAdvanced.pm +++ b/lib/WeBWorK/ContentGenerator/LTIAdvanced.pm @@ -37,7 +37,7 @@ sub initializeRoute ($c, $routeCaptures) { if (!$courseID && $c->param('context_id')) { # The database object used here is not associated to any course, # and so the only has access to non-native tables. - my @matchingCourses = WeBWorK::DB->new(WeBWorK::CourseEnvironment->new->{dbLayout}) + my @matchingCourses = WeBWorK::DB->new(WeBWorK::CourseEnvironment->new) ->getLTICourseMapsWhere({ lms_context_id => $c->param('context_id') }); if (@matchingCourses == 1) { diff --git a/lib/WeBWorK/ContentGenerator/LTIAdvantage.pm b/lib/WeBWorK/ContentGenerator/LTIAdvantage.pm index eca31c6c6e..64e0e4abb3 100644 --- a/lib/WeBWorK/ContentGenerator/LTIAdvantage.pm +++ b/lib/WeBWorK/ContentGenerator/LTIAdvantage.pm @@ -76,11 +76,10 @@ sub initializeRoute ($c, $routeCaptures) { # The database object used here is not associated to any course, # and so the only has access to non-native tables. - my @matchingCourses = - WeBWorK::DB->new(WeBWorK::CourseEnvironment->new->{dbLayout})->getLTICourseMapsWhere({ - lms_context_id => + my @matchingCourses = WeBWorK::DB->new(WeBWorK::CourseEnvironment->new)->getLTICourseMapsWhere({ + lms_context_id => $c->stash->{lti_jwt_claims}{'https://purl.imsglobal.org/spec/lti/claim/context'}{id} - }); + }); if (@matchingCourses == 1) { $c->stash->{courseID} = $matchingCourses[0]->course_id; @@ -348,7 +347,7 @@ sub extract_jwt_claims ($c) { return unless $c->param('state'); # The following database object is not associated to any course, and so the only has access to non-native tables. - my $db = WeBWorK::DB->new(WeBWorK::CourseEnvironment->new->{dbLayout}); + my $db = WeBWorK::DB->new(WeBWorK::CourseEnvironment->new); # Retrieve the launch data saved in the login phase, and then delete it from the database. Note that this verifies # the state in the request. If there is no launch data saved in the database for the state in the request, then the @@ -371,7 +370,7 @@ sub extract_jwt_claims ($c) { 'Failed to initialize course environment for ' . $c->stash->{LTILaunchData}->data->{courseID} . ": $@\n"; return; } - $db = WeBWorK::DB->new($ce->{dbLayout}); + $db = WeBWorK::DB->new($ce); $c->purge_expired_lti_data($ce, $db); diff --git a/lib/WeBWorK/DB.pm b/lib/WeBWorK/DB.pm index 50bc67a40a..81095e1264 100644 --- a/lib/WeBWorK/DB.pm +++ b/lib/WeBWorK/DB.pm @@ -14,6 +14,7 @@ ################################################################################ package WeBWorK::DB; +use Mojo::Base -strict; =head1 NAME @@ -21,7 +22,7 @@ WeBWorK::DB - interface with the WeBWorK databases. =head1 SYNOPSIS - my $db = WeBWorK::DB->new($dbLayout); + my $db = WeBWorK::DB->new($ce); my @userIDs = $db->listUsers(); my $Sam = $db->{user}->{record}->new(); @@ -40,11 +41,10 @@ WeBWorK::DB - interface with the WeBWorK databases. =head1 DESCRIPTION -WeBWorK::DB provides a consistent interface to a number of database backends. -Access and modification functions are provided for each logical table used by -the webwork system. The particular backend ("schema" and "driver"), record -class, data source, and additional parameters are specified by the hash -referenced by C<$dbLayout>, usually taken from the course environment. +WeBWorK::DB provides a database interface. Access and modification functions +are provided for each logical table used by the webwork system. The particular +schema, record class, and additional parameters are specified by the hash return +by the C method. =head1 ARCHITECTURE @@ -69,41 +69,34 @@ They are called "schema" modules because they control the structure of the data for a table. The schema modules provide an API that matches the requirements of the DB -layer, on a per-table basis. Each schema module has a style that determines -which drivers it can interface with. For example, SQL is an "dbi" style -schema. +layer, on a per-table basis. -=head2 Bottom Layer: Drivers +=head2 Bottom Layer: Database -Driver modules implement a style for a schema. They provide physical access to -a data source containing the data for a table. The style of a driver determines -what methods it provides. All drivers provide C and -C methods. A dbi style driver provides a C method which -returns the DBI handle. +The C module implements a DBI connection handle. It provides physical +access to the database. =head2 Record Types -In C<%dblayout>, each table is assigned a record class, used for passing +In the database layout, each table is assigned a record class, used for passing complete records to and from the database. The default record classes are subclasses of the WeBWorK::DB::Record class, and are named as follows: User, Password, PermissionLevel, Key, Set, UserSet, Problem, UserProblem. In the -following documentation, a reference the record class for a table means the -record class currently defined for that table in C<%dbLayout>. +following documentation, a reference to the record class for a table means the +record class currently defined for that table in the database layout. =cut -use strict; -use warnings; - use Carp; use Data::Dumper; -use Scalar::Util qw/blessed/; -use HTML::Entities qw( encode_entities ); +use Scalar::Util qw(blessed); +use HTML::Entities qw(encode_entities); use Mojo::JSON qw(encode_json decode_json); +use WeBWorK::DB::Database; use WeBWorK::DB::Schema; -use WeBWorK::DB::Utils qw/make_vsetID grok_vsetID grok_setID_from_vsetID_sql - grok_versionID_from_vsetID_sql/; +use WeBWorK::DB::Layout qw(databaseLayout); +use WeBWorK::DB::Utils qw(make_vsetID grok_vsetID grok_setID_from_vsetID_sql grok_versionID_from_vsetID_sql); use WeBWorK::Debug; use WeBWorK::Utils qw(runtime_use); @@ -164,77 +157,51 @@ use Exception::Class ( }, ); -################################################################################ -# constructor -################################################################################ - =head1 CONSTRUCTOR -=over - -=item new($dbLayout) - -The C method creates a DB object and brings up the underlying schema/driver -structure according to the hash referenced by C<$dbLayout>. - -=back - -=head2 C<$dbLayout> Format - -C<$dbLayout> is a hash reference consisting of items keyed by table names. The -value of each item is a reference to a hash containing the following items: - -=over - -=item record - -The name of a perl module to use for representing the data in a record. - -=item schema - -The name of a perl module to use for access to the table. + my $db = WeBWorK::DB->new($ce) -=item driver +The C method creates a DB object, connects to the database via the +C module, and brings up the underlying schema structure according to +the hash referenced in the L. A course +environment object is the only required argument (as it is used to construct the +database layout). -The name of a perl module to use for access to the data source. - -=item source - -The location of the data source that should be used by the driver module. -Depending on the driver, this may be a path, a url, or a DBI spec. - -=item params - -A reference to a hash containing extra information needed by the schema. Some -schemas require parameters, some do not. Consult the documentation for the -schema in question. - -=back - -For each table defined in C<$dbLayout>, C loads the record, schema, and -driver modules. It the schema module's C method lists the current table -(or contains the string "*") and the output of the schema and driver modules' -C