Updates to grading tests for other users.

[somiaj]

When able to grade a test of another user, change the "Grade Test" button to "Grade Test for UserID" to make it clear the test is being graded for another user.

Show the "Check Test" button along side the "Grade Test" button for instructor who can use the problem grader but also have the permission to submit the past due test.

Allow instructors who can use the problem grader to grade unsubmitted tests for that user without needing the permission to submit test versions or answers for another user. This way if for some reason a student's test doesn't get submitted correctly (such as a user closing their browser without grading the test or after reaching a grade proctor screen), it can still be graded with their saved answers.

Update the timer when acting as another user if it is being shown, just only suppress warnings about running out of time in this case.

Update description of record_answers_when_acting_as_student to state that it can also start and grade test versions. Also include a warning about that permission should be disabled (set back to "nobody") after using it as it can interfere with tests, and should probably only be used temporarily.

Note this is just hopefully a good start to improving being able to grade tests that didn't get submitted and grading tests for other users, there are probably tweaks that need to be made.

I also initially wanted to only allow submitting an unsubmitted testing using a students saved answers, but due to how the GatewayQuiz module works, found this to be difficult to implement (or at least I didn't see an easy way to do that), so for now an instructor could change answers before submitting the unsubmitted test even if they don't have permission to record student answers. I suspect most instructors will just use the students saved answers in this case anyways.

[somiaj]

@drgrice1 I decided to test this with a proctored test and it seems that it "mostly" works in the sense that you end up having to enter in proctor information twice to start or view the test, but otherwise things work as expected. I'm unsure of the best way to make it so you don't need to authenticate twice here, so I am asking for what you think would be best.

What I am seeing, is when starting or viewing a proctored test as a student with permissions to submit for them, the first page you are sent to is the proctor login, then after that is done you get the warning about starting or viewing open test versions, then after agreeing to the risks you get sent back to the proctor page, and finally after the second proctor authentication you get access to the test and can move around as expected. I don't think this behavior is new to this PR, but now it affects viewing test versions as well as starting them.

[drgrice1]

Yeah, that certainly isn't good. In no case should the proctor authentication be needed twice. Although the behavior is not new with this pull request, it is new with #2610. This does not happen with WeBWorK 2.18. So this is something that was missed with #2610, and needs to be fixed.

[somiaj]

Though maybe the issue is the fact that we are asking for a proctor authentication from a user who can authenticate themselves. Maybe something we can discuss at a meeting, should we really be asking an instructor (or other user) who has permissions to start a proctored test for a proctor login. They have already authenticated, so we don't need to ask them to authenticate again.

[somiaj]

I don't think it is completely new with that PR, as I didn't mess with proctor behavior directly. Users with the permission create_new_set_version_when_acting_as_student probably had a similar issue since they had to agree to the warning, but now that we are warning users with the record_answers_when_acting_as_student permission it is affecting both.

My thoughts is the warning should appear before the proctor authentication page, but unsure how to best do it. The only other way I can think of to deal with it is turn the links on the warning page to a form submit, so we can keep all the hidden fields with the proctor authentication.

[drgrice1]

Yes, that is something to discuss.

When I am acting as another user and click to continue an in progress proctored test, I am not getting the warning page at all. Also, if I do the same thing for an unproctored test, I am not getting the warning page. So something isn't working with that to begin with. In addition, in both cases there is only a "Grade Test" button. There isn't a "Grade Test for ..." button, and not a "Check Test" button.

[somiaj]

I can't reproduce your last comment, for both proctored and unproctored tests I get a warning when I click "Continue Open Test". Also the fact you don't see the "Grade Test for ....", are you still testing this branch?

[drgrice1]

Unfortunately, the proctor authentication is going to have to come first. Reworking things so that the warning page can come first would be messy, and could open potential security vulnerabilities. Making the continue link on the warning page a submit button with hidden inputs may work, but better would be to use the session. Hidden inputs and a form submission may open a loop hole for entering a proctored exam.

[drgrice1]

I am on this branch, and not getting the warning page at all. That is the warning for continuing an in progress test.

[drgrice1]

I also never get the "Grade Test for ..." button in any situation.

[somiaj]

I still can't reproduce this. Did you disable the permission to submit answers for students? You won't get the warning or the grade test button in that case.

[drgrice1]

Wait, I was on the other branch for #2610 accidentally.

[somiaj]

In testing this I did run across another issue, though hopefully not something users are actually doing. If you have a proctored test, a student starts a proctored test version with authentication, you then change the template set from proctored to normal test, it is no longer possible to access that proctored test version with the error 'Requested set "Quiz07" is a proctored test, but no valid proctor authorization has been obtained.'. Not something I think is introduced here, and hopefully not something instructors are actually doing. But maybe something we should look into?

[drgrice1]

That case is not something to concern ourselves with at this point. That is something to defer for the future, and probably something that has existed for a long time.

[drgrice1]

The problem is that these warning pages use the invalidSet key. As a result the proctor authorization is deleted on line 705. This should use something else instead of that key. In fact, looking at this I see that there are too many keys being added to the $c object for this. There is a confirmSubmitForStudent and an invalidVersionCreation and their names do not match what they are doing. The confirmSubmitForStudent is being set when a version is being created which doesn't make sense. There only needs to be one such key with a more general name that indicates some sort of "acting as" permission, and it could do everything that is needed here with the right set of values, and the invalidSet key not used at all. Furthermore, it should not be a direct key on the $c controller object. That is something that we need to stop doing. Use the stash. This is precisely what it is for.

Not using the invalidSet key would fix the proctor authorization issue, and the fact is this is not an invalid set. So that was in incorrect usage of this in any case.

[drgrice1]

Another thing that I see. The message that is shown when a user with permission to create a new set for another user attempts to do so is

The selected test (Testing_Proctor_Gateway) is not a valid test for tguy (acted as by grice1).
You are acting as user tguy. 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".

That should not include the message that the set is not a valid test. I is a valid test, it is just that the version doesn't exist. That message needs to be changed to say that the test version does not exist instead.

[somiaj]

I was looking into that too, and found the following change to not delete the key if waiting for a confirmation works.

diff --git a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm
index 6a007a388..427fba093 100644
--- a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm
+++ b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm
@@ -695,8 +695,10 @@ async sub pre_header_initialize ($c) {
        }
 
        # If the set or problem is invalid, then delete any proctor session keys and return.
-       if ($c->{invalidSet}) {
-               if (defined $c->{assignment_type} && $c->{assignment_type} eq 'proctored_gateway') {
+       # Don't delete key if asking for submit for student confirmation.
+       if (!$c->{confirmSubmitForStudent} && $c->{invalidSet}) {
+               if (defined $c->{assignment_type} && $c->{assignment_type} eq 'proctored_gateway')
+               {
                        delete $c->authen->session->{proctor_authorization_granted};
                }
                return;

I used confirmSubmitForStudent as the variable being set if confirmation is needed before either creating or viewing a test if the permission record_answers_when_acting_as_student (or one of the lesser permission) is set. I just shorted the statement to be SubmitForStudent vs RecordAnswersWhenActingAsStudent. I mostly tried to leave the invalidVersionCreation alone which was being set if trying to access a version that doesn't exist yet (i.e. when creating a new version) but it couldn't be created due to lack of permission or waiting for confirmation.

I agree, I thought that message about the set being invalid was also odd to appear when asking for confirmation, but left it alone as that was existing behavior. I can go clean this up to try to better describe what the cases are.

[drgrice1]

The invalidVersionCreation is only ever set for a user acting as another user. It is not set more generally for when an invalid version is attempted to be created. So that flag needs to be reworked.

Instead drop both of those, and use two flags that make sense. I suggest something like actingConfirmation and actingConfirmationMessage. Then rework the code for this in the GatewayQuiz.html.ep file to use completely separate code for an invalidSet (it doesn't need much) and for these new flags.

[drgrice1]

You can just use the invalidSet and go with your quick fix approach for now. The method I am proposing will take more work.

[somiaj]

@drgrice1 My simple fix (and even my more complicated fix I just pushed) only works for viewing open versions. When creating a new test version, even if I don't delete the proctor_authorization_granted key, it still requires two proctor authentication attempts. I have tried to track this down, but unsure why when trying to create a new version something in the proctor authorization fails, while viewing an open version works. I think it has something to do with having a version for the test to view, but I was not able to track down the difference.

[drgrice1]

The problem is that proctor authorization only works if the session key proctor_authorization_granted is set to a versioned set (like testId,v1). The only way this will work is to add another session key that is checked for that covers this case. I am using confirm_version_creation in my testing. So the Proctor.pm module needs to check for this key as well as the proctor_authorization_granted key, and not insist on a versioned set in the case that confirm_version_creation is set.

[somiaj]

Ahh. Can you share your commit/diff? I was thinking that when proctor_authorization_granted was just set to $setID before appending the version would have covered this case, and was confused why it wasn't working, since all my testing showed the value didn't append the version before the version creation.

[drgrice1]

What I have done is in the grade-test-for-others-fixes branch on my fork. It doesn't have a valid commit message, since I wasn't finished with it, but that is what I have at this point. I had a slightly different setup than you for some of the other things.

[somiaj]

Thanks, I adapted it to my previous changes, and everything seems to work in regard to not requiring two authentication attempts in my testing.

[somiaj]

I had move the return when waiting for acting proctor confirmation to before deleting proctor_authorization_granted from the session, otherwise after I created a version I would view the first page, but moving to another page required authentication. I may have put this in the wrong place initially.

[drgrice1]

Yeah, I think it is right now.

[drgrice1]

I think this is looking good now.