Skip to content

Commit

Permalink
Switch the "edit set for users" and "show answers" actions to form su…
Browse files Browse the repository at this point in the history 
…bmissions.

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.
  • Loading branch information
@drgrice1
drgrice1 committed Mar 27, 2024
1 parent 3159f7f commit 8a3b52b
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 34 deletions.
68 changes: 66 additions & 2 deletions htdocs/js/InstructorTools/instructortools.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -82,7 +93,7 @@
editMode.name = 'editMode';
editMode.value = 1;

form.append(visibleUsers, editMode);
additionalParams.append(visibleUsers, editMode);
}
break;
case 'score_sets':
Expand All @@ -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;
}
Expand Down
35 changes: 10 additions & 25 deletions lib/WeBWorK/ContentGenerator/Instructor/Index.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.');
Expand All @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}

Expand Down
21 changes: 14 additions & 7 deletions templates/ContentGenerator/Instructor/Index.html.ep
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
% use Mojo::JSON qw(encode_json);
%
% use WeBWorK::Utils qw(getAssetURL);
% use WeBWorK::HTML::ScrollingRecordList qw(scrollingRecordList);
%
Expand All @@ -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->{$_}) =%>
% }
%
<div class="row">
<div class="col-xl-5 col-md-6 mb-2">
<div class="fw-bold text-center"><%= label_for selected_users => maketext('Users') %></div>
Expand Down Expand Up @@ -197,20 +204,20 @@
</div>
<div class="input-group input-group-sm mb-2">
<%= 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) } =%>
<span class="input-group-text flex-grow-1"><%==
maketext(q{<strong>one</strong> set's details for <strong>some or all</strong> users})
=%></span>
</div>
<div class="input-group input-group-sm mb-2">
<%= 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' =%>
<span class="input-group-text flex-grow-1"><%==
maketext('answer log for <strong>selected</strong> users, '
. 'for <strong>selected</strong> sets')
Expand Down

0 comments on commit 8a3b52b

Please sign in to comment.