Tests, disable start button when acting as another user. #2610
Conversation
There was a problem hiding this comment.
This generally looks okay.
However, I don't quite like this. Previously the "Start New Test" button was there, and if you didn't have the permission to create a new version when acting and you clicked on the button, then you get the information "User ... is being acted as. When acting as another user, new versions of the set cannot be created." Now, the button is gone, and that information is never relayed to the user. Furthermore, things seem a bit off with the button missing. Perhaps it is just that I am accustomed to the button being there, but it seems there should be something about why the button is not there.
Instead of just hiding the "Start New Test" button and showing nothing if an instructor acts as a student, I think it would be best to instead show the message that new versions cannot be created when acting as a user in its place.
|
What about still showing the button, but disabling it and maybe a popup with the info about it is disabled because they are currently acting as another user? |
|
That would work. |
somiaj
force-pushed
the
test-start-while-acting
branch
from
f531c9a
to
c74a623
Compare
|
@drgrice1 I cannot seem to get the tooltip to work as described in the bootstrap docs. First 5.3 states that Outside of that, the start new test button is just disabled and provides a tooltip informing the user as to why. |
There was a problem hiding this comment.
Some Bootstrap components (for instance modals) are initialized automatically. If you add the appropriate data-bs-toggle attribute and other necessary markup for those, then you don't need to do anything else for them to work. However, other Bootstrap components (for instance tooltips) are opt in (for performance reasons). That means that even if you add the appropriate data-bs-toggle attribute, it won't work unless you also initialize them via JavaScript. See https://getbootstrap.com/docs/5.3/components/tooltips/#enable-tooltips.
So one option is to add code to initialize the tooltips. This would be appropriate to add in system.js since it is the only specific JavaScript for this page at this time.
Another option is to just add the set-id-tooltip class to the span. There is already code in system.js to enable tooltips on any element with this class that has a data-bs-title attribute. Note that tooltips with this class are really general purpose tooltips contrary to the class name. See my comment on lines 88-89 of system.js. Although, these tooltips are forced to be in the set position (in your case on the top) with no fallback position. That may not be appropriate for these tooltips.
Note that by switching to the title attribute, you are not making this work as a Bootstrap tooltip, and the data-bs-toggle="tooltip" attribute is still ignored and is useless. In that case the native browser title handling is in effect and you get the usual hover behavior for a title which is different.
somiaj
force-pushed
the
test-start-while-acting
branch
4 times, most recently
from
47c0b65
to
1530544
Compare
somiaj
changed the title
somiaj
force-pushed
the
test-start-while-acting
branch
from
1436c1c
to
0e36640
Compare
| . 'the "Create New Test Version" button below. Alternately, PRESS THE "BACK" BUTTON ' | ||
| . 'and continue.', | ||
| $effectiveUserID | ||
| ); |
There was a problem hiding this comment.
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 beYou 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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->{invalidSet} = "User $effectiveUserID is being acted as. " | ||
| . 'When acting as another user, new versions of the set cannot be created.'; | ||
| $c->{invalidSet} = $c->maketext( | ||
| 'User [_1] is being acted as. When acting as another user, ' |
There was a problem hiding this comment.
Also here I would change the passive voice in ''User [_1] is being acted as.'
Hide the "Start New Test" button when acting as another user unless the user has permissions to do so. Fix some logic where the warning about creating new test version wouldn't show with the record_answers_when_acting_as_student permission, and instead the version would be created without warning. Translate the error messages that are created for an invalidSet. Remove the test for invalidProblem, which isn't used in tests. Update error messages to state 'test' instead of 'set'.
When acting as another user, disable the "Start New Test" button instead of hiding it if user doesn't have permission to start a test as a different user. Include a tooltip that states why the button is disabled.
somiaj
force-pushed
the
test-start-while-acting
branch
2 times, most recently
from
db69856
to
8c36441
Compare
Instead of telling user to click "Back Button", provide a "Cancel" button which returns them to the previous page. This also includes language updates as suggested during the PR review.
somiaj
force-pushed
the
test-start-while-acting
branch
from
8c36441
to
0d78d47
Compare
|
I think this looks good, and I will merge. |
Hide the "Start New Test" button when acting as another user unless the user has permissions to do so.Edit: This now just disables the "Start New Test" button and displays a tool tip informing the user they don't have permission to start new tests when acting as another user.
Fix some logic where the warning about creating new test version wouldn't show with the record_answers_when_acting_as_student permission, and instead the version would be created without warning.
Translate the error messages that are created for an invalidSet.
Remove the test for invalidProblem, which isn't used in tests.
Update error messages to state 'test' instead of 'set'.
Note, to me it wasn't clear from the comment in defaults.conf that the
record_answers_when_acting_as_studentalso applied to creating tests and recording answers for tests, but from the code it appears that is the intent, so that is what I went with here.