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:

      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.

        Gliffy Diagrams

        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.
            Hide
            Mary Cooch added a comment -

            This issue is labelled docs_required; however, it seems it's a bug fix rather than a new feature or improvement. Please can anyone specify what needs documenting; otherwise the docs_required label can be removed.

            Show
            Mary Cooch added a comment - This issue is labelled docs_required; however, it seems it's a bug fix rather than a new feature or improvement. Please can anyone specify what needs documenting; otherwise the docs_required label can be removed.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: