Moodle
  1. Moodle
  2. MDL-34733

Clicking either "Finish Review" button results in a 404 to /mod/quiz/0

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3, 2.3.1
    • Fix Version/s: 2.2.5, 2.3.2
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide

      Note that a different fix was done on stable branches (22 and 23) and master, so please test both versions carefully.

      1. Create a quiz with Browser security set to Full screen pop-up with some JavaScript protection. Make sure the review options allow the attempt to be reviewed.

      2. Add some questions to the quiz.

      3. Preview the quiz as teacher, work all the way through:

      3a. Start on the view.php page and click Start attempt.
      3b. Answer all the questions, clicking next, to end up on the summary page.
      3c. Click submit all and finish.
      3d. Click the finish review link, either in the navigation panel, or at the bottom of the last page of the quiz.
      3e. Make sure that you get back to the view.php page.

      4. Repeat 4, but as a student. The attempt should open in a pop-up window, and at the end of the review, the pop-up should close when you click the finish review button.

      Show
      Note that a different fix was done on stable branches (22 and 23) and master, so please test both versions carefully. 1. Create a quiz with Browser security set to Full screen pop-up with some JavaScript protection. Make sure the review options allow the attempt to be reviewed. 2. Add some questions to the quiz. 3. Preview the quiz as teacher, work all the way through: 3a. Start on the view.php page and click Start attempt. 3b. Answer all the questions, clicking next, to end up on the summary page. 3c. Click submit all and finish. 3d. Click the finish review link, either in the navigation panel, or at the bottom of the last page of the quiz. 3e. Make sure that you get back to the view.php page. 4. Repeat 4, but as a student. The attempt should open in a pop-up window, and at the end of the review, the pop-up should close when you click the finish review button.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      43201

      Description

      As teacher, review a quiz attempt and it will redirect to /mod/quiz/0 and result in a 404.

      I've tested this on my main site and on a demo site and it works the same and in Firefox and Chrome. It just fails in IE8 and does nothing and complains about a problem in module.js.

      It appears that #secureclosebutton is setup to close the secure window mod/quiz/module.js line 272. I assume this is the root of the problem. When I step through the debugger, it appears that the url gets set to "0".

      Anyone else notice this? Didn't notice this in 2.0, but haven't tested except in 2.3.

        Activity

        Hide
        Jeff Rader added a comment -

        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.

        Show
        Jeff Rader added a comment - 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.
        Hide
        Tim Hunt added a comment -

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

        Show
        Tim Hunt added a comment - It will only be a problem for 'secure' quizzes, and it will only affect teachers. Would be great if you could find a fix.
        Hide
        Jeff Rader added a comment - - edited

        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?

        Show
        Jeff Rader added a comment - - edited 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?
        Hide
        Jeff Rader added a comment -

        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.

        Show
        Jeff Rader added a comment - 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.
        Show
        Jeff Rader added a comment - - edited 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
        Hide
        Tim Hunt added a comment -

        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.

        Show
        Tim Hunt added a comment - 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.
        Hide
        Jeff Rader added a comment -

        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?

        Show
        Jeff Rader added a comment - 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?
        Hide
        Tim Hunt added a comment -

        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.

        Show
        Tim Hunt added a comment - 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.
        Hide
        E. John Love added a comment - - edited

        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).

        Show
        E. John Love added a comment - - edited 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).
        Hide
        Tim Hunt added a comment -

        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.

        Show
        Tim Hunt added a comment - 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.
        Hide
        Tim Hunt added a comment -

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

        Show
        Tim Hunt added a comment - Despite the lack of testing, I am confident in this fix, so submitting for integration.
        Hide
        Dan Poltawski added a comment -

        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..

        Show
        Dan Poltawski added a comment - 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..
        Hide
        CiBoT added a comment -

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

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        Tim Hunt added a comment -

        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.

        Show
        Tim Hunt added a comment - 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.
        Hide
        Tim Hunt added a comment -

        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.

        Show
        Tim Hunt added a comment - 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.
        Hide
        Dan Poltawski added a comment -

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

        Show
        Dan Poltawski added a comment - I agree that this hacky solution is best for backwards compatibility, so incregrating this.
        Hide
        Dan Poltawski added a comment -

        Integrated, thanks Tim

        Show
        Dan Poltawski added a comment - Integrated, thanks Tim
        Hide
        David Monllaó added a comment -

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

        Show
        David Monllaó added a comment - It passes. Tested in master and 22, as a teacher and student, with both 'Finish review' links.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        YEAR!*

        CAF*, TOT!*

        • Your effort amazingly resulted. (unbelievable :-P)
        • Closing as fixed.
        • Tons of thanks.
        Show
        Eloy Lafuente (stronk7) added a comment - YEAR!* CAF*, TOT!* Your effort amazingly resulted. (unbelievable :-P) Closing as fixed. Tons of thanks.

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: