Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Tests, disable start button when acting as another user. #2610

Merged
merged 4 commits into from
Nov 24, 2024
Merged

Tests, disable start button when acting as another user. #2610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7dc5de9
Tests, don't show start button when acting as another user.
somiaj Oct 23, 2024
0fd85ba
Disable start new test button instead of hiding it.
somiaj Nov 13, 2024
a694743
Stash disable_start_new_test variable.
somiaj Nov 13, 2024
0d78d47
Add cancel button to create new test version warning.
somiaj Nov 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 37 additions & 28 deletions lib/WeBWorK/ContentGenerator/GatewayQuiz.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,8 @@ async sub pre_header_initialize ($c) {
$c->{assignment_type} = 'gateway';

if (!$authz->hasPermissions($userID, 'modify_problem_sets')) {
$c->{invalidSet} = 'You do not have the authorization level required to view/edit undefined sets.';
$c->{invalidSet} =
$c->maketext('You do not have the authorization level required to view/edit undefined sets.');

# Define these so that we can drop through to report the error in body.
$tmplSet = fake_set($db);
Expand All @@ -340,8 +341,8 @@ async sub pre_header_initialize ($c) {
} else {
# In this case we're creating a fake set from the input, so the input must include a source file.
if (!$c->param('sourceFilePath')) {
$c->{invalidSet} =
'An Undefined_Set was requested, but no source file for the contained problem was provided.';
$c->{invalidSet} = $c->maketext(
'An Undefined_Set was requested, but no source file for the contained problem was provided.');

# Define these so that we can drop through to report the error in body.
$tmplSet = fake_set($db);
Expand Down Expand Up @@ -529,9 +530,11 @@ async sub pre_header_initialize ($c) {
&& (
$effectiveUserID eq $userID
|| (
$authz->hasPermissions($userID, 'record_answers_when_acting_as_student')
|| ($authz->hasPermissions($userID, 'create_new_set_version_when_acting_as_student')
&& $c->param('createnew_ok'))
(
$authz->hasPermissions($userID, 'record_answers_when_acting_as_student')
|| $authz->hasPermissions($userID, 'create_new_set_version_when_acting_as_student')
)
&& $c->param('createnew_ok')
drgrice1 marked this conversation as resolved.
Show resolved Hide resolved
)
)
)
Expand Down Expand Up @@ -586,26 +589,32 @@ async sub pre_header_initialize ($c) {
$currentNumAttempts = 0;

} elsif ($maxAttempts != -1 && $totalNumVersions > $maxAttempts) {
$c->{invalidSet} = 'No new versions of this assignment are available, '
. 'because you have already taken the maximum number allowed.';
$c->{invalidSet} = $c->maketext('No new versions of this test are available, '
. 'because you have already taken the maximum number allowed.');

} elsif ($effectiveUserID ne $userID
&& $authz->hasPermissions($userID, 'create_new_set_version_when_acting_as_student'))
} elsif (
$effectiveUserID ne $userID
&& ($authz->hasPermissions($userID, 'record_answers_when_acting_as_student')
|| $authz->hasPermissions($userID, 'create_new_set_version_when_acting_as_student'))
)
{

$c->{invalidSet} =
"User $effectiveUserID is being acted "
. 'as. If you continue, you will create a new version of this set '
. 'for that user, which will count against their allowed maximum '
. 'number of versions for the current time interval. IN GENERAL, THIS '
. 'IS NOT WHAT YOU WANT TO DO. Please be sure that you want to '
. 'do this before clicking the "Create new set version" link '
. 'below. Alternately, PRESS THE "BACK" BUTTON and continue.';
$c->{invalidSet} = $c->maketext(
'You are acting as user [_1]. If you continue, you will create a new version of '
. 'this test for that user, which will count against their allowed maximum '
. 'number of versions for the current time interval. In general, this is not '
. 'what you want to do. Please be sure that you want to do this before clicking '
. 'the "Create New Test Version" button below. Alternatively, click "Cancel".',
$effectiveUserID
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm nitpicking about old wording here, but since it is now going to be translated, it seems best to change it now.

  • User [_1] is being acted as. could be You are acting as user [_1].. Less passive.
  • Using capital letters to emphasize something is not a good idea. It's not clear that will be understood as emphasis by a screen reader. I think it is fine to just use regular case.
  • I think 'Alternately' should be 'Alternatively'.
  • In 'PRESS THE "BACK" BUTTON', is that referring to the web browser's back button? If so, is that apparent and always meaningful, like on a small screen? Maybe this whole 'Alternately...' sentence should just be removed.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps in addition to the "Create New Test" button, there should be a "Cancel" button which takes the user back to the test version list page.

Copy link
Member

Choose a reason for hiding this comment

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

Note that this isn't the only place that webwork uses the "use the browser back button" wording. The email preview page also does this (and there may be other places as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a "Cancel" button, removed the caps, and updated the language via the above suggestions and a few more edits I added myself.

$c->{invalidVersionCreation} = 1;

} elsif ($effectiveUserID ne $userID) {
$c->{invalidSet} = "User $effectiveUserID is being acted as. "
. 'When acting as another user, new versions of the set cannot be created.';
$c->{invalidSet} = $c->maketext(
'You are acting as user [_1], and do not have the permission to create a new test version '
. 'when acting as another user.',
$effectiveUserID
);
$c->{invalidVersionCreation} = 2;

} elsif (($maxAttemptsPerVersion == 0 || $currentNumAttempts < $maxAttemptsPerVersion)
Expand All @@ -615,16 +624,16 @@ async sub pre_header_initialize ($c) {
$versionIsOpen = 1;
} else {
$c->{invalidSet} =
'No new versions of this assignment are available, because the set is not open or its time'
. ' limit has expired.';
$c->maketext('No new versions of this test are available, because the test is '
. 'not open or its time limit has expired.');
}

} elsif ($versionsPerInterval
&& ($currentNumVersions >= $versionsPerInterval))
{
$c->{invalidSet} =
'You have already taken all available versions of this test in the current time interval. '
. 'You may take the test again after the time interval has expired.';
$c->maketext('You have already taken all available versions of this test in the current '
. 'time interval. You may take the test again after the time interval has expired.');

}

Expand All @@ -643,7 +652,7 @@ async sub pre_header_initialize ($c) {
}

} elsif (!$c->{invalidSet} && !$requestedVersion) {
$c->{invalidSet} = 'This set is closed. No new set versions may be taken.';
$c->{invalidSet} = $c->maketext('This test is closed. No new test versions may be taken.');
}

# If the proctor session key does not have a set version id, then add it. This occurs when a student
Expand All @@ -656,7 +665,7 @@ async sub pre_header_initialize ($c) {
}

# If the set or problem is invalid, then delete any proctor session keys and return.
if ($c->{invalidSet} || $c->{invalidProblem}) {
if ($c->{invalidSet}) {
if (defined $c->{assignment_type} && $c->{assignment_type} eq 'proctored_gateway') {
delete $c->authen->session->{proctor_authorization_granted};
}
Expand Down Expand Up @@ -700,7 +709,7 @@ async sub pre_header_initialize ($c) {

# Bail without doing anything if the set isn't yet open for this user.
if (!($c->{isOpen} || $authz->hasPermissions($userID, 'view_unopened_sets'))) {
$c->{invalidSet} = 'This set is not yet open.';
$c->{invalidSet} = $c->maketext('This test is not yet open.');
return;
}

Expand Down Expand Up @@ -821,7 +830,7 @@ async sub pre_header_initialize ($c) {
my $problemN = $mergedProblems[$pIndex];

if (!defined $problemN) {
$c->{invalidSet} = 'One or more of the problems in this set have not been assigned to you.';
$c->{invalidSet} = $c->maketext('One or more of the problems in this test have not been assigned to you.');
return;
}

Expand Down
5 changes: 5 additions & 0 deletions lib/WeBWorK/ContentGenerator/ProblemSet.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ async sub initialize ($c) {
my $userID = $c->param('user');
my $eUserID = $c->param('effectiveUser');

# Don't show "Start New Test" button when acting as another user, unless user has permissions to do so.
$c->stash->{disable_start_new_test} = $userID ne $eUserID
&& !($authz->hasPermissions($userID, 'record_answers_when_acting_as_student')
|| $authz->hasPermissions($userID, 'create_new_set_version_when_acting_as_student'));

my $user = $db->getUser($userID);
my $effectiveUser = $db->getUser($eUserID);
$c->{set} = $authz->{merged_set};
Expand Down
37 changes: 26 additions & 11 deletions templates/ContentGenerator/GatewayQuiz.html.ep
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,22 +66,37 @@
% my $effectiveUserID = param('effectiveUser');
%
% # If the set or problem is invalid, then show that information and exit.
% if ($c->{invalidSet} || $c->{invalidProblem}) {
% if ($c->{invalidSet}) {
<div class="alert alert-danger mb-2">
<div class="mb-2">
<%= maketext(
'The selected problem set ([_1]) is not a valid set for [_2]: [_3]',
$setID, $effectiveUserID, $c->{invalidVersionCreation} ? " (acted as by $userID)" : ''
) =%>
% if ($c->{invalidVersionCreation}) {
<%= maketext(
'The selected test ([_1]) is not a valid test for [_2] (acted as by [_3]).',
$setID, $effectiveUserID, $userID
) =%>
% } else {
<%= maketext(
'The selected test ([_1]) is not a valid test for [_2].',
$setID, $effectiveUserID
) =%>
% }
</div>
<div><%= $c->{invalidSet} %></div>
% if ($c->{invalidVersionCreation} && $c->{invalidVersionCreation} == 1) {
<p>
<%= link_to 'Create new set version.' => $c->systemLink(
url_for,
params => { effectiveUser => $effectiveUserID, user => $userID, createnew_ok => 1 }
) =%>
</p>
<div class="mt-3">
<%= link_to maketext('Create New Test Version') => $c->systemLink(
url_for,
params => { effectiveUser => $effectiveUserID, user => $userID, createnew_ok => 1 }
),
class => 'btn btn-primary'
=%>
<%= link_to maketext('Cancel') => $c->systemLink(
url_for('problem_list', setID => $setID),
params => { effectiveUser => $effectiveUserID, user => $userID }
),
class => 'btn btn-primary'
=%>
</div>
% }
</div>
%
Expand Down
14 changes: 12 additions & 2 deletions templates/ContentGenerator/ProblemSet/version_list.html.ep
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,18 @@
% }
%
<div class="mb-3">
<%= link_to maketext('Start New Test') => $c->systemLink(url_for($routeName, setID => $set->set_id)),
class => 'btn btn-primary' =%>
% # Disable "Start New Test" button when acting as another user who doesn't have permissions to start tests.
% if ($disable_start_new_test) {
<span class="d-inline-block set-id-tooltip" tabindex="0" data-bs-toggle="tooltip" data-bs-placement="top"
data-bs-title="<%=maketext(
'You are acting as another user and do not have permission to start a new test for other users.'
) %>">
<button class="btn btn-primary" type="button" disabled><%= maketext('Start New Test') %></button>
</span>
% } else {
<%= link_to maketext('Start New Test') => $c->systemLink(url_for($routeName, setID => $set->set_id)),
class => 'btn btn-primary' =%>
% }
</div>
% } else {
% # Message about if/when next version will be available.
Expand Down