Seems the button is not discriminating between a grader and a student in the review and always attaches the secure_window.close method to the "Finish Review" button. This may only be a problem if the quiz is set to be a secure pop-up.

It will only be a problem for 'secure' quizzes, and it will only affect teachers. Would be great if you could find a fix.

OK, I'm trying to trace things down in the midst of things and it seems that the root problem is in the accessmanager->make_rules. It seems that it doesn't bring in a superceed rule for quizaccess_securewindow.

Tim, any thoughts on where I should go from here?

quizaccess_securewindow->get_superceded_rules() is not implemented. Should we do that?

Sorry, I'm still working through figuring out git
change
/mod/quiz/accessrule/securewindow/rule.php line 57 to
if (($quizobj->get_quiz()>browsersecurity !== 'securewindow') || has_capability('mod/quiz:viewreports', $quizobj>get_context())) {

That checks the context to make sure the teacher doesn't have to open it in a secure window even if the quiz has that set.

Let me know if this is not a good approach and I'll try to work through it another way.

https://github.com/coderader/moodle_23/commit/ae7af9da29aee88c7ae40dea12a0bc21c3206f0d#mod/quiz/accessrule/securewindow/rule.php - forgot the trailing parenthesis - got excited.

https://github.com/coderader/moodle_23/commit/e51d1f73811140332017e4c81f670202f294c55d#mod/quiz/accessrule/securewindow/rule.php

You are on the right track, but I don't like the way you have fixed it.

The thing is, if $quiz->browsersecurity == 'securewindow', then browser security is in force for this quiz, and we want to display that fact to everyone, even if we do not actually force teachers to take the test in a secure window. (But, actually, I think we should. Teachers should preview the quiz the way students see it. And really, they should get to experience how bloody annoying the full-screen pop-up is.)

Actually, it is more complex than that. If the teacher clicks the Preview link in the settings block, we should not use the pop-up window. If they click the Start preview button on the quiz view.php page, it should use a pop-up.

The finish attempt JavaScript needs to be able to handle both cases.

OK, so currently, teacher does not see it in a securewindow when they go to preview unless they change to view it as a student. The patch I have maintains that same paradigm for the review.

I know it is somewhat annoying, but it provides at least a veil of trying to keep things harder for people to cheat. We use it exclusively in our distance education.

It doesn't make sense to me that we would force the teacher/grader to review it in a pop-up. This patch should only effect those with that permission.

So would you prefer that instead of patching to keep it in the main window that it would always force a new popup window regardless of the users permission or whether its a preview or a review?

Keeping the same paradigm is OK.

Note creating an instance of the rule, for a quiz where that setting is on, is unacceptable.

There is an API that is meant to handle this: the attempt_must_be_in_popup() method. Clearly, some part of the quiz code is not calling that function when it matters. Fixing that is the right way to fix this bug.

We have seen this same bug under version 2.2.4, when using "full-screen popup wi Javascript". I find that student roles don't get the error, but Teacher and Admin roles do. Setting the quiz's Browser Security to "None" avoids the issue too (obviously, since there's no popup window).

I think this fix is the right approach (https://github.com/timhunt/moodle/compare/master...MDL-34733). I have tested a bit and it seems to work for me, but it would be good if some people who rely on secure mode could give it some more thorough testing.

Despite the lack of testing, I am confident in this fix, so submitting for integration.

Hi Tim,

It looks like this has been backported 'blindly' because the upgrade.txt talks of 2.4 changes in all 3 branches.

The incompatible function signature doesn't look right to me for the stable branches, so reopening this..

Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

Bum!

We need to find a way to fix this, including on stable branches, because this is a bug that is causing problems for users. I know because there are forum posts.

However, I cannot think of a way to fix it other than changing this renderer API. In one sense I am just being over-fussy about changing the renderer API, but in another sense, documenting it is a right thing. I will think about this on my way to work.

OK, hacky, but backwards compatible fix done on stable branches.

Better fix done on master, now we admit we are changing the API.

Re-submitting for integration.

I agree that this hacky solution is best for backwards compatibility, so incregrating this.

Integrated, thanks Tim

It passes. Tested in master and 22, as a teacher and student, with both 'Finish review' links.

YEAR!*

CAF*, TOT!*

• Your effort amazingly resulted. (unbelievable :-P)
• Closing as fixed.
• Tons of thanks.