Moodle
  1. Moodle
  2. MDL-40264

Essay questions interpret the zero string '0' as incomplete

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 2.2.10, 2.3.7, 2.4.4, 2.5, 2.6
    • Fix Version/s: 2.3.8, 2.4.5, 2.5.1
    • Component/s: Questions
    • Labels:
      None
    • Testing Instructions:
      Hide

      Run the provided unit tests and ensure that all tests pass.

      Manually test as follows:
      1. Create a new quiz.
      2. Add a new essay question to said quiz; set its response format to "plain text"*. Do not include a response template.
      3. Log in as a student and attempt the quiz. In the essay response box, enter a single 0 (zero) with no leading or trailing spaces.
      4. Press 'next' to move to the summary page. Verify that the question is in the "needs grading" state: its status column should read "answer saved".
      5. Submit the quiz.
      6. Log in as an instructor, and verify that the quiz needs grading, and has not been assigned a zero. Check for the word "complete" under the question number.

      Show
      Run the provided unit tests and ensure that all tests pass. Manually test as follows: 1. Create a new quiz. 2. Add a new essay question to said quiz; set its response format to "plain text"*. Do not include a response template. 3. Log in as a student and attempt the quiz. In the essay response box, enter a single 0 (zero) with no leading or trailing spaces. 4. Press 'next' to move to the summary page. Verify that the question is in the "needs grading" state: its status column should read "answer saved". 5. Submit the quiz. 6. Log in as an instructor, and verify that the quiz needs grading, and has not been assigned a zero. Check for the word "complete" under the question number.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      MDL-40264-m24-essay-zero
    • Pull 2.5 Branch:
      MDL-40264-m25-essay-zero
    • Pull Master Branch:
      MDL-40264-essay-zero
    • Rank:
      51032

      Description

      If a respondent submits only one zero (0s) for an essay question, their submission is incorrectly considered incomplete.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Almost right. However, the $response array comes from a HTTP post request, so it will always be an array string => string. Therefore,

          1. The unit tests with array('answer' => false) and array('answer' => null) are meaningless.

          2. but you should add a test with array().

          3. The $essay->responsetemplate = 'Once upon a time'; line in the test is irrelevant, and you should probably delete it.

          4. I think the is_complete code should be
          return array_key_exists('answer', $response) && $response['answer'] !== '';
          Given what this bug was, it is much safer to use !==.

          Show
          Tim Hunt added a comment - Almost right. However, the $response array comes from a HTTP post request, so it will always be an array string => string. Therefore, 1. The unit tests with array('answer' => false) and array('answer' => null) are meaningless. 2. but you should add a test with array(). 3. The $essay->responsetemplate = 'Once upon a time'; line in the test is irrelevant, and you should probably delete it. 4. I think the is_complete code should be return array_key_exists('answer', $response) && $response ['answer'] !== ''; Given what this bug was, it is much safer to use !==.
          Hide
          Kyle Temkin added a comment - - edited

          However, the $response array comes from a HTTP post request, so it will always be an array string => string.

          1. The unit tests with array('answer' => false) and array('answer' => null) are meaningless.
          2. but you should add a test with array().

          I was under the impression that the $response array is pre-processed by get_expected_data (which in turn calls get_submitted_Var), and thus could contain any type that can be produced by clean_param. (So $response would be array string => mixed.)

          I do see why those two tests would be unnecessary now, though-- as long as get_expected_data returns array that includes 'answer' => PARAM_RAW/'answer' => PARAM_RAW_FILES, you'll always get a raw string for the 'answer' key.

          Fixed.

          The $essay->responsetemplate = 'Once upon a time'; line in the test is irrelevant, and you should probably delete it.

          Fixed.

          I think the is_complete code should be
          return array_key_exists('answer', $response) && $response['answer'] !== '';
          Given what this bug was, it is much safer to use !==.

          Given the two are guaranteed to both be strings by the above; this shouldn't afford any additional safety - though I've changed it to !==, as that seems like it would be less confusing.

          Fixed.

          Show
          Kyle Temkin added a comment - - edited However, the $response array comes from a HTTP post request, so it will always be an array string => string. 1. The unit tests with array('answer' => false) and array('answer' => null) are meaningless. 2. but you should add a test with array(). I was under the impression that the $response array is pre-processed by get_expected_data (which in turn calls get_submitted_Var ), and thus could contain any type that can be produced by clean_param . (So $response would be array string => mixed.) I do see why those two tests would be unnecessary now, though-- as long as get_expected_data returns array that includes 'answer' => PARAM_RAW / 'answer' => PARAM_RAW_FILES , you'll always get a raw string for the 'answer' key. Fixed. The $essay->responsetemplate = 'Once upon a time'; line in the test is irrelevant, and you should probably delete it. Fixed. I think the is_complete code should be return array_key_exists('answer', $response) && $response ['answer'] !== ''; Given what this bug was, it is much safer to use !==. Given the two are guaranteed to both be strings by the above; this shouldn't afford any additional safety - though I've changed it to !==, as that seems like it would be less confusing. Fixed.
          Hide
          Tim Hunt added a comment -

          Looking good:

          [Y] Syntax
          [-] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [N] Testing
          [Y] Security
          [Y] Documentation
          [N] Git
          [Y] Sanity check

          1. Testing - Please could you edit this issue and add the necessary testing instructions.

          2. Git - I think this needs to be fixed on all stable branches. Please could you backport to 2.4 and 2.3.

          If you can get those done, this is ready for integration.

          Show
          Tim Hunt added a comment - Looking good: [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [N] Testing [Y] Security [Y] Documentation [N] Git [Y] Sanity check 1. Testing - Please could you edit this issue and add the necessary testing instructions. 2. Git - I think this needs to be fixed on all stable branches. Please could you backport to 2.4 and 2.3. If you can get those done, this is ready for integration.
          Hide
          Kyle Temkin added a comment -

          Done.

          (At this point, how should I progress with the tracker? Do I just re-start peer review and wait for the component lead to submit it for integration review?)

          Show
          Kyle Temkin added a comment - Done. (At this point, how should I progress with the tracker? Do I just re-start peer review and wait for the component lead to submit it for integration review?)
          Hide
          Tim Hunt added a comment -

          Yes, normally you would submit the issue for peer review again, so the Component owner notices and can submit for integration. However, in this case I already noticed. Thanks for fixing this.

          Show
          Tim Hunt added a comment - Yes, normally you would submit the issue for peer review again, so the Component owner notices and can submit for integration. However, in this case I already noticed. Thanks for fixing this.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23, 24, 25 & master), thanks!

          Side note: The "Pull X.Y branches" should not be URLs but just branch names, FYI.

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23, 24, 25 & master), thanks! Side note: The "Pull X.Y branches" should not be URLs but just branch names, FYI.
          Hide
          Sam Hemelryk added a comment -

          Thanks guys, tested and passed.

          Show
          Sam Hemelryk added a comment - Thanks guys, tested and passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks for giving me joys and smiles
          Thanks for sharing my trouble's pile

          Thanks for wipeing the tears of my eye
          Thanks for showing me the glad view of sky

          Thanks for lending me your shoulders to lean
          Thanks for giving my words a proper mean

          Thanks for telling me the value of life
          Thanks for showing me the rules to survive

          Thanks for lending me the sympathetic ears
          Thanks for showing how much you care

          From all this what I mean in the end
          Is thanks for being my special friend.

          – Seema Chowdhury

          Sent upstream so... closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks for giving me joys and smiles Thanks for sharing my trouble's pile Thanks for wipeing the tears of my eye Thanks for showing me the glad view of sky Thanks for lending me your shoulders to lean Thanks for giving my words a proper mean Thanks for telling me the value of life Thanks for showing me the rules to survive Thanks for lending me the sympathetic ears Thanks for showing how much you care From all this what I mean in the end Is thanks for being my special friend. – Seema Chowdhury Sent upstream so... closing, thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: