From 8a3b52be1856a69afc6e8a12a6644a4379e1b43c Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Tue, 19 Mar 2024 17:09:01 -0500 Subject: [PATCH] Switch the "edit set for users" and "show answers" actions to form submissions. Any action on the instructor index page (the Instructor Tools) that possibly includes multiple users or multiple sets can not be a GET request (i.e., a redirect). They must be a POST request (i.e., a form submission). This is because GET requests limit the amount of data that they can contain 2-8 KB (depending on the browser and server). If a large number of users or sets are selected this can easily exceed this size limit. The "show answers" action makes this even worse by including the problem ids. POST requests have much larger limits depending on both the browser and the server. Mojolicious limits this to 16GB by default, and it varies with the browser. In any case it is quite unlikely that this limit will be attained for any of these requests. This issue came up with this page previously when all requests were redirects (GET requests). There was an issue reported several years ago about this when an instructor selected all users in a large class. So I fixed this by using a direct form submission via the formaction atttibutes on the respective submit buttons for which this was a problem. In some cases, since the parameters don't always match up with the page the form will be submitted to, javascript fills in the required parameters. At the time I did not fix the "edit set for users" action (i.e., "Edit one set's details for some or all users"). It was more challenging than the others, since it needs to post to a different URL depending on which set is selected. I have a little more experience now, and figured out how to do it this time by using a regular expression replacement to add the correct set to the URL (and preserve URL parameters like effectiveUser and such -- Although I think these aren't actually needed since they are in the form). The "show answers" action (i.e., "View answer log for selected users, for selected sets") is new, and I missed this when it was added. Although, that one is even more challenging, because it needs to have the problems ids of the selected sets. Those problem ids are now added to the page in hidden inputs that the javascript can access to fill in the needed information. I also added the `formtarget => 'WW_Info'` in this case. That means that the answer log will open in a new page like it usually does. I don't exaclty like this though, but if this is not done, and then the instructor clicks the "Display Past Answers" button on the answer log page then another window is opened with a second instance of the answer log. Perhaps it is time to stop opening pages in new windows like this? This is considered bad practice in many circles. --- htdocs/js/InstructorTools/instructortools.js | 68 ++++++++++++++++++- .../ContentGenerator/Instructor/Index.pm | 35 +++------- .../ContentGenerator/Instructor/Index.html.ep | 21 ++++-- 3 files changed, 90 insertions(+), 34 deletions(-) diff --git a/htdocs/js/InstructorTools/instructortools.js b/htdocs/js/InstructorTools/instructortools.js index 6c09bc852a..d8357373f4 100644 --- a/htdocs/js/InstructorTools/instructortools.js +++ b/htdocs/js/InstructorTools/instructortools.js @@ -57,6 +57,17 @@ return; } + // Any additional form parameters added below should be placed in this div. Each time the form is submitted it + // is emptied so that form parameters don't build up in the DOM. This can happen if the form is submitted, and + // then the user uses the browser back button. + const additionalParams = document.querySelector('.additional-params') || document.createElement('div'); + if (additionalParams.classList.contains('additional-params')) { + while (additionalParams.firstChild) additionalParams.firstChild.remove(); + } else { + additionalParams.classList.add('d-none', 'additional-params'); + form.append(additionalParams); + } + // The UserList.pm and Scoring.pm modules have different form parameters than the form in the instructor tools // Index.pm module. This sets the correct parameters for those modules from the parameters in the form in // Index.pm. @@ -82,7 +93,7 @@ editMode.name = 'editMode'; editMode.value = 1; - form.append(visibleUsers, editMode); + additionalParams.append(visibleUsers, editMode); } break; case 'score_sets': @@ -106,7 +117,60 @@ scoreSelected.name = 'scoreSelected'; scoreSelected.value = 1; - form.append(selectedSet, scoreSelected); + additionalParams.append(selectedSet, scoreSelected); + } + break; + case 'edit_set_for_users': + { + if ( + !new RegExp(`\\/instructor\\/sets\\/?${selectedSets[0].value}(\\?.*)?$`).test( + e.submitter.formAction + ) + ) { + e.submitter.formAction = e.submitter.formAction.replace( + /\/instructor\/sets\/?.*?(\?.*)?$/, + `/instructor/sets/${selectedSets[0].value}$1` + ); + } + + for (const user of selectedUsers) { + const editForUser = document.createElement('input'); + editForUser.name = 'editForUser'; + editForUser.type = 'hidden'; + editForUser.style.display = 'none'; + editForUser.value = user.value; + additionalParams.append(editForUser); + } + } + break; + case 'show_answers': + { + if (!selectedSets.length) return; + + const selectedProblems = document.createElement('select'); + selectedProblems.name = 'selected_problems'; + selectedProblems.type = 'select'; + selectedProblems.multiple = true; + selectedProblems.style.display = 'none'; + + const allProblemIds = {}; + + for (const set of selectedSets) { + for (const id of JSON.parse( + document.getElementsByName(`${set.value}_problem_ids`)[0]?.value || '[]' + )) { + allProblemIds[id] = 1; + } + } + + for (const id of Object.keys(allProblemIds)) { + const selectedProblemsOption = document.createElement('option'); + selectedProblemsOption.value = id; + selectedProblemsOption.selected = true; + selectedProblems.append(selectedProblemsOption); + } + + additionalParams.append(selectedProblems); } break; } diff --git a/lib/WeBWorK/ContentGenerator/Instructor/Index.pm b/lib/WeBWorK/ContentGenerator/Instructor/Index.pm index fffd452ab1..a2890c69a8 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/Index.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/Index.pm @@ -24,7 +24,7 @@ pages =cut use WeBWorK::Utils qw(x); -use WeBWorK::Utils::JITAR qw(jitar_id_to_seq prob_id_sort); +use WeBWorK::Utils::JITAR qw(jitar_id_to_seq); use WeBWorK::Utils::Sets qw(format_set_name_internal); use constant E_MAX_ONE_SET => x('Please select at most one set.'); @@ -43,6 +43,7 @@ sub pre_header_initialize ($c) { # Make sure these are defined for the template. $c->stash->{users} = []; $c->stash->{globalSets} = []; + $c->stash->{setProblemIDs} = {}; $c->stash->{E_MAX_ONE_SET} = E_MAX_ONE_SET; $c->stash->{E_ONE_USER} = E_ONE_USER; $c->stash->{E_ONE_SET} = E_ONE_SET; @@ -143,30 +144,6 @@ sub pre_header_initialize ($c) { push @error, E_ONE_USER unless $nusers == 1; push @error, E_MAX_ONE_SET unless $nsets <= 1; } - } elsif (defined $c->param('edit_set_for_users')) { - if ($nusers >= 1 and $nsets == 1) { - $route = 'instructor_set_detail'; - $args{setID} = $firstSetID; - $params{editForUser} = \@selectedUserIDs; - } elsif ($nsets == 1) { - $route = 'instructor_set_detail'; - $args{setID} = $firstSetID; - } else { - push @error, E_ONE_SET unless $nsets == 1; - } - } elsif (defined $c->param('show_answers')) { - my %all_problems; - for my $setID (@selectedSetIDs) { - my @problems = $db->listGlobalProblems($setID); - if ($db->getGlobalSet($setID)->assignment_type && $db->getGlobalSet($setID)->assignment_type eq 'jitar') { - @problems = map { join('.', jitar_id_to_seq($_)) } @problems; - } - @all_problems{@problems} = (1) x @problems; - } - $route = 'answer_log'; - $params{selected_users} = \@selectedUserIDs; - $params{selected_sets} = \@selectedSetIDs; - $params{selected_problems} = [ prob_id_sort keys %all_problems ]; } elsif (defined $c->param('create_set')) { my $setname = format_set_name_internal($c->param('new_set_name') // ''); if ($setname) { @@ -230,6 +207,14 @@ sub pre_header_initialize ($c) { $c->stash->{globalSets} = [ $db->getGlobalSetsWhere ]; + # Problem IDs for each set are needed for the "View answer log ..." action. + for my $globalSet (@{ $c->stash->{globalSets} }) { + my @problems = $db->listGlobalProblems($globalSet->set_id); + @problems = map { join('.', jitar_id_to_seq($_)) } @problems + if $globalSet->assignment_type && $globalSet->assignment_type eq 'jitar'; + $c->stash->{setProblemIDs}{ $globalSet->set_id } = \@problems; + } + return; } diff --git a/templates/ContentGenerator/Instructor/Index.html.ep b/templates/ContentGenerator/Instructor/Index.html.ep index 7e8f0f669c..c88f1e1891 100644 --- a/templates/ContentGenerator/Instructor/Index.html.ep +++ b/templates/ContentGenerator/Instructor/Index.html.ep @@ -1,3 +1,5 @@ +% use Mojo::JSON qw(encode_json); +% % use WeBWorK::Utils qw(getAssetURL); % use WeBWorK::HTML::ScrollingRecordList qw(scrollingRecordList); % @@ -21,6 +23,11 @@ <%= form_for current_route, method => 'POST', id => 'instructor-tools-form', begin =%> <%= $c->hidden_authen_fields =%> % + % # Add problem ids in hidden fields for each set (for the "View answer log ..." action. + % for (keys %$setProblemIDs) { + <%= hidden_field "${_}_problem_ids" => encode_json($setProblemIDs->{$_}) =%> + % } + %
<%= label_for selected_users => maketext('Users') %>
@@ -197,12 +204,10 @@
<%= submit_button maketext('Edit'), - name => 'edit_set_for_users', - class => 'btn btn-sm btn-secondary', - data => { - sets_needed => 'exactly one', - error_sets => maketext($E_ONE_SET) - } =%> + name => 'edit_set_for_users', + class => 'btn btn-sm btn-secondary', + formaction => $c->systemLink(url_for 'instructor_set_detail'), + data => { sets_needed => 'exactly one', error_sets => maketext($E_ONE_SET) } =%> <%== maketext(q{one set's details for some or all users}) =%> @@ -210,7 +215,9 @@
<%= submit_button maketext('View'), name => 'show_answers', - class => 'btn btn-sm btn-secondary' =%> + class => 'btn btn-sm btn-secondary', + formaction => $c->systemLink(url_for 'answer_log'), + formtarget => 'WW_Info' =%> <%== maketext('answer log for selected users, ' . 'for selected sets')