Moodle
  1. Moodle
  2. MDL-35370

Cloze question type: numeric questions where answer is zero, NULL field value is not calculated in score but displays as being correct.

    Details

    • Testing Instructions:
      Hide

      Create a Cloze question containing either a numerical or shortanswer subquestion (or possibly both)

      A simple example question text would be

      Enter zero: {1:NUMERICAL:=0:0}

      Test these three possible responses (using question preview, or by adding to a quiz)

      1. Leave the input blank - should be reported as not answered.

      (The feedback for the subquestion is in a tooltip, so move your mouse over the input box.)

      2. Get the question right (type in 0) - should be reported as correct.

      3. Get the question wrong (type in 123) - should be reported as incorrect.

      Show
      Create a Cloze question containing either a numerical or shortanswer subquestion (or possibly both) A simple example question text would be Enter zero: {1:NUMERICAL:=0:0} Test these three possible responses (using question preview, or by adding to a quiz) 1. Leave the input blank - should be reported as not answered. (The feedback for the subquestion is in a tooltip, so move your mouse over the input box.) 2. Get the question right (type in 0) - should be reported as correct. 3. Get the question wrong (type in 123) - should be reported as incorrect.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      44045

      Description

      When creating a cloze type questions where the correct answer value is zero, students will often leave the answer field blank.

      When reviewing answers in the gradebook or question review interface, this results in the student and instructor visually seeing the question as being answered correctly with a green checkmark displayed, but in the gradebook it only records partial points for the question and deducts the null responses from the total score.

      I've tested this by entering the zeros and not entering the zeros. The question that I entered zeros recieved full points while the same answers without the zeros only recieved partial points.

      1. french.jpg
        27 kB
      2. french2.jpg
        26 kB
      3. MDL35370.jpg
        46 kB
      4. nmerical.jpg
        63 kB

        Issue Links

          Activity

          Hide
          Pierre Pichet added a comment -

          The following figure illustrates the problem and pinpoints to the multianswer renderer code.

          Show
          Pierre Pichet added a comment - The following figure illustrates the problem and pinpoints to the multianswer renderer code.
          Hide
          Pierre Pichet added a comment -

          The display for similar numerical questions are OK.

          Show
          Pierre Pichet added a comment - The display for similar numerical questions are OK.
          Hide
          Pierre Pichet added a comment -

          Tim,
          When the response is empty i.e. null for numerical
          the call to numerical is done by
          $matchinganswer = $subq->get_matching_answer($response, 1);
          in numerical/question.php

           
              public function get_matching_answer($value, $multiplier) {
                  if (!is_null($multiplier)) {
                      $scaledvalue = $value * $multiplier;
                  } else {
                      $scaledvalue = $value;
                  }
          

          as $value is empty or null the $scaledvalue is calculated as 0 so it is seen as a good response in the response == 0 case.

          I did not have time to test why numerical question does not have the same problem.

          Show
          Pierre Pichet added a comment - Tim, When the response is empty i.e. null for numerical the call to numerical is done by $matchinganswer = $subq->get_matching_answer($response, 1); in numerical/question.php public function get_matching_answer($value, $multiplier) { if (!is_null($multiplier)) { $scaledvalue = $value * $multiplier; } else { $scaledvalue = $value; } as $value is empty or null the $scaledvalue is calculated as 0 so it is seen as a good response in the response == 0 case. I did not have time to test why numerical question does not have the same problem.
          Hide
          Tim Hunt added a comment -

          Thanks Pierre! Great detective work.

          I seem to remember that elsewhere we had to do something like

          if ($value !== '') before calling get_matching_answer

          Anyway, now that you have tracked the problem down this far, it should be easy for me to complete a fix later today.

          Show
          Tim Hunt added a comment - Thanks Pierre! Great detective work. I seem to remember that elsewhere we had to do something like if ($value !== '') before calling get_matching_answer Anyway, now that you have tracked the problem down this far, it should be easy for me to complete a fix later today.
          Hide
          Pierre Pichet added a comment -

          Tim,
          Working with the french language I discover that we should not call $subq->get_matching_answer()
          with the $response defined as
          $response = $qa->get_last_qt_var($fieldname);

          as the decimal is not converted.
          So 1,05 is converted to 1
          I added the $response and the $response*1 result to the normal display (the XX..) to illustrate the problem.
          In this case the tolerance was set to 0.1.

          Show
          Pierre Pichet added a comment - Tim, Working with the french language I discover that we should not call $subq->get_matching_answer() with the $response defined as $response = $qa->get_last_qt_var($fieldname); as the decimal is not converted. So 1,05 is converted to 1 I added the $response and the $response*1 result to the normal display (the XX..) to illustrate the problem. In this case the tolerance was set to 0.1.
          Hide
          Pierre Pichet added a comment - - edited

          A better illustration of the problem.
          Notice that the grading which is done on the converted $response, is 0 for each subquestion as they exceed the 0.1 tolerance.
          However I don't understand why the 0.15 response to 0 with a 0.1 tolerance is shown is green although the 1.15 value is correctly displayed (red) as its exceeds the 1 with 0.1 tolerance...

          Show
          Pierre Pichet added a comment - - edited A better illustration of the problem. Notice that the grading which is done on the converted $response, is 0 for each subquestion as they exceed the 0.1 tolerance. However I don't understand why the 0.15 response to 0 with a 0.1 tolerance is shown is green although the 1.15 value is correctly displayed (red) as its exceeds the 1 with 0.1 tolerance...
          Hide
          Tim Hunt added a comment -
          Show
          Tim Hunt added a comment - Related forum thread: http://moodle.org/mod/forum/discuss.php?d=211327
          Hide
          Tim Hunt added a comment -

          Here is how it might work.

          Surprisingly, these changes do not break any unit tests.

          Arguably, I should add some unit tests that only pass once this change has applied.

          Anyway, will wait to see what people think in the above forum thread.

          Show
          Tim Hunt added a comment - Here is how it might work. Surprisingly, these changes do not break any unit tests. Arguably, I should add some unit tests that only pass once this change has applied. Anyway, will wait to see what people think in the above forum thread.
          Hide
          Pierre Pichet added a comment -

          The handling of numerical can be corrected by the following

           
          @@ -175,11 +175,12 @@ class qtype_multianswer_textfield_renderer extends qtype_multianswer_subq_render
           
                   $response = $qa->get_last_qt_var($fieldname);
                   if ($subq->qtype->name() == 'shortanswer') {
                       $matchinganswer = $subq->get_matching_answer(array('answer' => $response));
                   } else if ($subq->qtype->name() == 'numerical') {
          -            $matchinganswer = $subq->get_matching_answer($response, 1);
          +            list($value, $unit, $multiplier) = $subq->ap->apply_units($response, '');
          +            $matchinganswer = $subq->get_matching_answer($value, 1);
                   } else {
                       $matchinganswer = $subq->get_matching_answer($response);
                   }
           
                   if (!$matchinganswer) {
          

          On return from your holiday, you insert this in this bug or create a separate one.

          Show
          Pierre Pichet added a comment - The handling of numerical can be corrected by the following @@ -175,11 +175,12 @@ class qtype_multianswer_textfield_renderer extends qtype_multianswer_subq_render $response = $qa->get_last_qt_var($fieldname); if ($subq->qtype->name() == 'shortanswer') { $matchinganswer = $subq->get_matching_answer(array('answer' => $response)); } else if ($subq->qtype->name() == 'numerical') { - $matchinganswer = $subq->get_matching_answer($response, 1); + list($value, $unit, $multiplier) = $subq->ap->apply_units($response, ''); + $matchinganswer = $subq->get_matching_answer($value, 1); } else { $matchinganswer = $subq->get_matching_answer($response); } if (!$matchinganswer) { On return from your holiday, you insert this in this bug or create a separate one.
          Hide
          Tim Hunt added a comment -

          Right, I have now added some more changes to https://github.com/timhunt/moodle/compare/master...MDL-35370.

          • I reverted the bit where I change the display of not-answered parts from red to white.
          • I added Pierre's fix for , as decimal point.
          • I added a unit test to verify the blank input shown as correct problem, which is the but that was originally reported here.

          Pierre, if you could take a quick look at the latest code, that would be great.

          Hopefully, it is OK, and I can squash the three commits into one, apply to all stable branches, and then submit for integration.

          Show
          Tim Hunt added a comment - Right, I have now added some more changes to https://github.com/timhunt/moodle/compare/master...MDL-35370 . I reverted the bit where I change the display of not-answered parts from red to white. I added Pierre's fix for , as decimal point. I added a unit test to verify the blank input shown as correct problem, which is the but that was originally reported here. Pierre, if you could take a quick look at the latest code, that would be great. Hopefully, it is OK, and I can squash the three commits into one, apply to all stable branches, and then submit for integration.
          Hide
          Pierre Pichet added a comment -

          Hi Tim,
          I fetch your MDL-35370 to my site and first manual tests are Ok but there is at least a problem that remains.
          If the response exceeds the tolerance, the grade is 0 and shown as red. However in the cloze pop-up the diagnostic is UNANSWERED.

          There is also the problem with the grading that allows , but the is_complete_response that does not tolerate a , if it is the thousand separator. This test should be eliminated in the is_complete_response.

          Next time to continue the testing is tomorrow night ...

          (Sorry i did not have the time to set my window installation for PHP tests.
          This is not simple because I cannot put my moodle site and the apache on the C:drive
          )

          Show
          Pierre Pichet added a comment - Hi Tim, I fetch your MDL-35370 to my site and first manual tests are Ok but there is at least a problem that remains. If the response exceeds the tolerance, the grade is 0 and shown as red. However in the cloze pop-up the diagnostic is UNANSWERED. There is also the problem with the grading that allows , but the is_complete_response that does not tolerate a , if it is the thousand separator. This test should be eliminated in the is_complete_response. Next time to continue the testing is tomorrow night ... (Sorry i did not have the time to set my window installation for PHP tests. This is not simple because I cannot put my moodle site and the apache on the C:drive )
          Hide
          Pierre Pichet added a comment -

          The "NOT ANSWERED" popup display for a 0 grade subquestion is somehow another problem not related to numerical as I have tested with a shortanwer subquestion.

          Show
          Pierre Pichet added a comment - The "NOT ANSWERED" popup display for a 0 grade subquestion is somehow another problem not related to numerical as I have tested with a shortanwer subquestion.
          Hide
          Pierre Pichet added a comment -

          The problem is related to the fact that the $matchinganswer do not identify differently an empty respone from an incorrect one.
          The following corrects the problem

           
          
          @@ -182,11 +182,16 @@ class qtype_multianswer_textfield_renderer extends qtype_multianswer_subq_render
                   } else {
                       $matchinganswer = $subq->get_matching_answer($response);
                   }
           
                   if (!$matchinganswer) {
          +            if($response == ''){
                           $matchinganswer = new question_answer(0, '', null, '', FORMAT_HTML);
          +            } else {
          +                $matchinganswer = new question_answer(0, '', 0.0, '', FORMAT_HTML);
          +            }
          +                
                   }
          
          Show
          Pierre Pichet added a comment - The problem is related to the fact that the $matchinganswer do not identify differently an empty respone from an incorrect one. The following corrects the problem @@ -182,11 +182,16 @@ class qtype_multianswer_textfield_renderer extends qtype_multianswer_subq_render } else { $matchinganswer = $subq->get_matching_answer($response); } if (!$matchinganswer) { + if($response == ''){ $matchinganswer = new question_answer(0, '', null, '', FORMAT_HTML); + } else { + $matchinganswer = new question_answer(0, '', 0.0, '', FORMAT_HTML); + } + }
          Hide
          Tim Hunt added a comment -

          Thank you for your careful testing Pierre. I will do some more work on this today.

          Show
          Tim Hunt added a comment - Thank you for your careful testing Pierre. I will do some more work on this today.
          Hide
          Tim Hunt added a comment -

          I have done one more commit adding Pierre's code from above (https://tracker.moodle.org/browse/MDL-35370?focusedCommentId=180510&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-180510). Interestingly, that is almost identical to some code Joseph suggested in MDL-29741, so great minds think alike.

          I also added some more unit tests to verify the Not answered / Incorrect distinction.

          I have decided not to deal with the numerical , issue as part of this bug. I think it is correct that

          • for languages where , is the thousands separator,
          • if the student types a , in their response,
          • the we ask them to change it to the proper character (.) before they submit.
          • (Even though we will try to do the right thing if they submit a response containing a ','.)
          Show
          Tim Hunt added a comment - I have done one more commit adding Pierre's code from above ( https://tracker.moodle.org/browse/MDL-35370?focusedCommentId=180510&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-180510 ). Interestingly, that is almost identical to some code Joseph suggested in MDL-29741 , so great minds think alike. I also added some more unit tests to verify the Not answered / Incorrect distinction. I have decided not to deal with the numerical , issue as part of this bug. I think it is correct that for languages where , is the thousands separator, if the student types a , in their response, the we ask them to change it to the proper character (.) before they submit. (Even though we will try to do the right thing if they submit a response containing a ','.)
          Hide
          Pierre Pichet added a comment -

          I agree that we should deal with the thousands separator handling in another issue as MDL-33105

          Show
          Pierre Pichet added a comment - I agree that we should deal with the thousands separator handling in another issue as MDL-33105
          Hide
          Pierre Pichet added a comment - - edited

          Another way to see the warning to the student is "Please don't use thousands separators"
          and effectively they are not handle correctly as in Deutch where the thousands separator is . and the decimal ,

          Show
          Pierre Pichet added a comment - - edited Another way to see the warning to the student is "Please don't use thousands separators" and effectively they are not handle correctly as in Deutch where the thousands separator is . and the decimal ,
          Hide
          Pierre Pichet added a comment -

          I have loaded the new version and my "manual" tests are OK

          Show
          Pierre Pichet added a comment - I have loaded the new version and my "manual" tests are OK
          Hide
          Tim Hunt added a comment -

          Thanks for all your help with the Pierre. Submitting for integration now.

          Show
          Tim Hunt added a comment - Thanks for all your help with the Pierre. Submitting for integration now.
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Dan Poltawski added a comment -

          Thanks everyone, i've integrated this now.

          Show
          Dan Poltawski added a comment - Thanks everyone, i've integrated this now.
          Hide
          Rossiani Wijaya added a comment -

          Tested this for 2.2, 2.3 and 2.4.

          It works as expected.

          Test passed.

          Show
          Rossiani Wijaya added a comment - Tested this for 2.2, 2.3 and 2.4. It works as expected. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Closing as fixed, many thanks for your awesome collaboration.

          Show
          Eloy Lafuente (stronk7) added a comment - Closing as fixed, many thanks for your awesome collaboration.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: