Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-34025

Call to close_attempt_popup passes parameters in wrong order.

    Details

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

      1. Create a quiz that allows multiple attempts, and set to Browser security: 'Popup ...', and no review allowed immediately after the attempt.
      2. Add a question to the quiz
      3. Create a student account and access the quiz.
      4. Finish an attempt
      5. Ensure a valid message is displayed and the pop-up window then closes after a few seconds.

      Show
      1. Create a quiz that allows multiple attempts, and set to Browser security: 'Popup ...', and no review allowed immediately after the attempt. 2. Add a question to the quiz 3. Create a student account and access the quiz. 4. Finish an attempt 5. Ensure a valid message is displayed and the pop-up window then closes after a few seconds.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-34025_master

      Description

      In mod/quiz/accessmanager.php the function back_to_view_page makes a call to close_attempt_popup and passes the message as the first parameter, and the url as the second parameter, when in fact it should be reversed. Function close_attempt_popup is declared in mod/quiz/renderer.php.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              timhunt Tim Hunt added a comment -

              Oops! That is presumably my mistake. (blush).

              Thanks for the fix. The code change is good.

              Please could you amend it to make the commit comment match the Moodle style (http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages). I would suggest

              MDL-34025 quiz secure window: fix popup closing

              Also, are you going to cherry-pick to the other stable branches and master, and write some testing instructions?

              Thanks.

              Show
              timhunt Tim Hunt added a comment - Oops! That is presumably my mistake. (blush). Thanks for the fix. The code change is good. Please could you amend it to make the commit comment match the Moodle style ( http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages ). I would suggest MDL-34025 quiz secure window: fix popup closing Also, are you going to cherry-pick to the other stable branches and master, and write some testing instructions? Thanks.
              Hide
              markn Mark Nelson added a comment -

              Hi Tim,

              It appears this issue is only introduced in Moodle 2.2 onwards, as the earlier versions do not have the same function calls.

              Regards,

              Mark

              Show
              markn Mark Nelson added a comment - Hi Tim, It appears this issue is only introduced in Moodle 2.2 onwards, as the earlier versions do not have the same function calls. Regards, Mark
              Hide
              timhunt Tim Hunt added a comment -

              Thanks Mark. I just edited the testing instructions to add two important settings that I think are required for this to be testable.

              Show
              timhunt Tim Hunt added a comment - Thanks Mark. I just edited the testing instructions to add two important settings that I think are required for this to be testable.
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks Mark!

              Integrated to master, 22 and 23

              Show
              poltawski Dan Poltawski added a comment - Thanks Mark! Integrated to master, 22 and 23
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              I get the msg "Your request has been processed. You can now close this window"
              But popup is not closed.
              Tested on chromium/integration master
              Failing this Sorry

              Show
              ankit_frenz Ankit Agarwal added a comment - I get the msg "Your request has been processed. You can now close this window" But popup is not closed. Tested on chromium/integration master Failing this Sorry
              Hide
              poltawski Dan Poltawski added a comment -

              Ankit, do you get any JS errors?

              Show
              poltawski Dan Poltawski added a comment - Ankit, do you get any JS errors?
              Hide
              ankit_frenz Ankit Agarwal added a comment - - edited

              I didn't check the console, will retry and see.
              Edit:- console is clean
              Edit2:- Just notcied this. The popup seems to keep loading forever after the attempt is finished and the msg is displayed.

              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - - edited I didn't check the console, will retry and see. Edit:- console is clean Edit2:- Just notcied this. The popup seems to keep loading forever after the attempt is finished and the msg is displayed. Thanks
              Hide
              timhunt Tim Hunt added a comment -

              A quick Google finds http://stackoverflow.com/questions/2032640/problem-with-window-close-and-chrome

              Note that this bug fix was fixing the call to the close_attempt_popup API. If the implementation of that method is broken, then that should not block integration of this fix (although it would be good to get it fixed too) (and get the fix in this week).

              Show
              timhunt Tim Hunt added a comment - A quick Google finds http://stackoverflow.com/questions/2032640/problem-with-window-close-and-chrome Note that this bug fix was fixing the call to the close_attempt_popup API. If the implementation of that method is broken, then that should not block integration of this fix (although it would be good to get it fixed too) (and get the fix in this week).
              Hide
              poltawski Dan Poltawski added a comment -

              Sounds sensible - passing.

              Ankit, could you please raise an issue for it?

              Show
              poltawski Dan Poltawski added a comment - Sounds sensible - passing. Ankit, could you please raise an issue for it?
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              Created MDL-34187 for the same.
              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - Created MDL-34187 for the same. Thanks
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Congratulations your code is upstream - gold star for you!

              This issue + 79 others made it in in time for the minor releases.
              Thank you everyone involved for your exuberant efforts.

              Show
              samhemelryk Sam Hemelryk added a comment - Congratulations your code is upstream - gold star for you! This issue + 79 others made it in in time for the minor releases. Thank you everyone involved for your exuberant efforts.

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    9/Jul/12