Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.2, 2.2
    • Fix Version/s: 2.1.3
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      1. Create an Essay question
      2. Click the preview icon
      3. In the preview pop-up window, verify that the button "Fill in correct responses" is disabled.
      4. Use Firebug (or similar) to hack the DOM to enable the button.
      5. Click the button, and verify that there is no error message. (The page should reload with no changes.)

      Show
      1. Create an Essay question 2. Click the preview icon 3. In the preview pop-up window, verify that the button "Fill in correct responses" is disabled. 4. Use Firebug (or similar) to hack the DOM to enable the button. 5. Click the button, and verify that there is no error message. (The page should reload with no changes.)
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      Steps to reproduce

      1. Create an Essay question
      2. Click the preview icon
      3. In the preview pop-up windows, click the button "Fill in correct responses"

      Expected behaviour

      a) Either the button is not displayed at all as the essay question does not have correct responses predefined
      b) or pressing the button does nothing

      What actually happens

      Debugging error is displayed

      • line 1041 of /question/engine/questionattempt.php: call to question_attempt_step->__construct()
      • line 539 of /question/engine/questionusage.php: call to question_attempt->process_action()
      • line 148 of /question/preview.php: call to question_usage_by_activity->process_action()

      Warning: array_key_exists() expects parameter 2 to be array, null given in .../question/engine/questionattemptstep.php on line 283

      Warning: array_key_exists() expects parameter 2 to be array, null given in .../question/engine/questionattemptstep.php on line 283

      Warning: Invalid argument supplied for foreach() in .../question/engine/questionattemptstep.php on line 270

        Gliffy Diagrams

          Activity

          Hide
          cfulton Charles Fulton added a comment -

          Thrown together patch implementing option (a): https://github.com/mackensen/moodle/compare/master...MDL-29970.

          Show
          cfulton Charles Fulton added a comment - Thrown together patch implementing option (a): https://github.com/mackensen/moodle/compare/master...MDL-29970 .
          Hide
          timhunt Tim Hunt added a comment -

          I assume you don't need me to tell you that patch is crap. (In case you do, hard-coding a plugin name like that is wrong.)

          get_correct_response returns null if this question cannot automatically compute a correct answer. We should

          1. decide whether to disable the button by calling get_correct_response and testing the value against null.

          2. also repeat the check for null where we process the button action, to avoid the fatal error if someone does something like use firebug to re-enable the button.

          3. update the PHP docs to explain the returning null think for get_correct_response.

          Show
          timhunt Tim Hunt added a comment - I assume you don't need me to tell you that patch is crap. (In case you do, hard-coding a plugin name like that is wrong.) get_correct_response returns null if this question cannot automatically compute a correct answer. We should 1. decide whether to disable the button by calling get_correct_response and testing the value against null. 2. also repeat the check for null where we process the button action, to avoid the fatal error if someone does something like use firebug to re-enable the button. 3. update the PHP docs to explain the returning null think for get_correct_response.
          Hide
          cfulton Charles Fulton added a comment -

          I think I knew it was crap when I posted it, although it has the unique bonus of being working crap. Hopefully less crappy patch submitted.

          Show
          cfulton Charles Fulton added a comment - I think I knew it was crap when I posted it, although it has the unique bonus of being working crap. Hopefully less crappy patch submitted.
          Hide
          timhunt Tim Hunt added a comment -

          The change looks functionally correct, but there are a few minor problems.

          1. $correctresponse != null does not do what you think in PHP. Try echo "Oops: ", (array() != null); Correct PHP is either !== null or !is_null(). Both are used in Moodle, but I have consistently used is_null in the question engine.

          2. I think redirect($actionurl); should be outside the if.

          3. ? : operator is not recommended by the Moodle coding guidelines, and it seems particularly bad to use it when the other nearby code is already using proper if statements.

          4. Your commit comment does not quite follow the Moodle convention. See http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages

          Attached is the fix-ups I am proposing to make to your code.

          Show
          timhunt Tim Hunt added a comment - The change looks functionally correct, but there are a few minor problems. 1. $correctresponse != null does not do what you think in PHP. Try echo "Oops: ", (array() != null); Correct PHP is either !== null or !is_null(). Both are used in Moodle, but I have consistently used is_null in the question engine. 2. I think redirect($actionurl); should be outside the if. 3. ? : operator is not recommended by the Moodle coding guidelines, and it seems particularly bad to use it when the other nearby code is already using proper if statements. 4. Your commit comment does not quite follow the Moodle convention. See http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages Attached is the fix-ups I am proposing to make to your code.
          Hide
          timhunt Tim Hunt added a comment -

          Here is the amended commit. Are you happy for me to push that into integration?

          Show
          timhunt Tim Hunt added a comment - Here is the amended commit. Are you happy for me to push that into integration?
          Hide
          cfulton Charles Fulton added a comment -

          Looks good to me--I appreciate your taking to time to explain best practices to a newbie.

          Show
          cfulton Charles Fulton added a comment - Looks good to me--I appreciate your taking to time to explain best practices to a newbie.
          Hide
          timhunt Tim Hunt added a comment -

          It is entirely self-interest. I am hoping for more bug-fixes in future

          Thanks. Submitting for integration now.

          Show
          timhunt Tim Hunt added a comment - It is entirely self-interest. I am hoping for more bug-fixes in future Thanks. Submitting for integration now.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          nebgor Aparup Banerjee added a comment -

          Thank you, this has been integrated and is up for testing.

          Show
          nebgor Aparup Banerjee added a comment - Thank you, this has been integrated and is up for testing.
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks for fixing this Tim and Charles
          No error message encountered, although "Start again" button is enabled after "Fill in correct response" button is pressed.
          Passing this, as enabling "Start again" button is a minor/no issue and clicking "start again" do not throw any error.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Tim and Charles No error message encountered, although "Start again" button is enabled after "Fill in correct response" button is pressed. Passing this, as enabling "Start again" button is a minor/no issue and clicking "start again" do not throw any error.
          Hide
          timhunt Tim Hunt added a comment -

          Of course the start again button is enabled after you have done Fill in correct response! That button always becomes disabled when you do something to the question - once you have changed from the initial state, you need a way to restart.

          Show
          timhunt Tim Hunt added a comment - Of course the start again button is enabled after you have done Fill in correct response! That button always becomes disabled when you do something to the question - once you have changed from the initial state, you need a way to restart.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks!

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                28/Nov/11