Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      19514

      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

        Activity

        Hide
        Charles Fulton added a comment -

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

        Show
        Charles Fulton added a comment - Thrown together patch implementing option (a): https://github.com/mackensen/moodle/compare/master...MDL-29970 .
        Hide
        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
        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
        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
        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
        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
        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
        Tim Hunt added a comment -

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

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

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

        Show
        Charles Fulton added a comment - Looks good to me--I appreciate your taking to time to explain best practices to a newbie.
        Hide
        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
        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
        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
        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
        Aparup Banerjee added a comment -

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

        Show
        Aparup Banerjee added a comment - Thank you, this has been integrated and is up for testing.
        Hide
        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
        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
        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
        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
        Eloy Lafuente (stronk7) added a comment -

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

        Ciao

        Show
        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: