Moodle
  1. Moodle
  2. MDL-29758

make Cloze question feedback display more informative in 2.1 (as it was in 1.9)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.3.6, 2.4, 2.5
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      Create an "Embedded answers (Cloze)" question using Example 2 from http://docs.moodle.org/25/en/Cloze#Example_2.
      Warning: using copy/paste can introduce unwanted tags like <pre> so be sure you paste the text only in the cloze question.
      Preview the question choosing "Adaptive mode" for the "How questions behave" setting.
      Enter responses in all the sub-parts of the question and click the "Check" button.
      Verify that all responses except horizontal and vertical multichoice's ones are colored (when I say colored, I mean not only the cross/check mark but the enter field or select menu itself) accordingly to their status (red for incorrect, yellow for partially correct and green for correct). Change some of your responses and click "Ckeck" verify that colors are updated. Click on "Submit all and finish" and verify that there is no change in responses coloring.

      Show
      Create an "Embedded answers (Cloze)" question using Example 2 from http://docs.moodle.org/25/en/Cloze#Example_2 . Warning: using copy/paste can introduce unwanted tags like <pre> so be sure you paste the text only in the cloze question. Preview the question choosing "Adaptive mode" for the "How questions behave" setting. Enter responses in all the sub-parts of the question and click the "Check" button. Verify that all responses except horizontal and vertical multichoice's ones are colored (when I say colored, I mean not only the cross/check mark but the enter field or select menu itself) accordingly to their status (red for incorrect, yellow for partially correct and green for correct). Change some of your responses and click "Ckeck" verify that colors are updated. Click on "Submit all and finish" and verify that there is no change in responses coloring.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
    • Pull Master Branch:
    • Rank:
      19274

      Description

      Mode = adaptive mode
      Question text=

      Q01.- {1:SHORTANSWER:=correct answer}
      
      Q02.- {1:SHORTANSWER:=correct answer}
      
      Q03.- {1:SHORTANSWER:=correct answer}
      
      Q04.- {1:SHORTANSWER:=correct answer~%50%half correct answer}
      

      Screenshot #1 compares the 1.9 and 2.1 displays of a Cloze question, in adaptive mode, when Submit page (1.9) or Check (2.1) button is clicked.

      • the 1.9 version clearly shows by using colors and icons which sub-questions were correctly, incorrectly or partially correctly answered and - by not using any coloring or icons - the sub-question which was left un-answered. Hovering over the questions does not provide any info.
      • 2.1, on the other hand, does not display such information ; the only info available is obtained by hovering over the sub-questions, which is not so immediately visible to the student.

      Screenshot #2 compares the 1.9 and 2.1 displays of a Cloze question, in adaptive mode, when Submit all and finish button is clicked.

      • the 1.9 version uses colors and ticks to display information about the correctness of the answers. Note that Q03 is left white, since it was un-answered, which is OK. HOvering over the sub-question answer boxes provides info re the "correct answer".
      • the 2.1 version also displays colors and ticks. But it wrongly displays Q03 in red; that un-answered sub-question should be displayed in white. [BUG]
        Hovering over the sub-questions answer boxes provides interesting information. Unfortunately, Q02 tooltip shows "Not answered" where it should be "Incorrect". [BUG]

      Bugs to be fixed are marked [BUG] in above Description text.

      Improvements requested:

      • When checking his answers, in 2.1 the student should see the correctness/incorrectness of his answers clearly marked (color and icon) as it was in 1.9. (Screenshot #1) and as it is when Submitting all in 2.1 (Screenshot #2). This is quite indispensable in Adaptive mode, especially for Cloze questions containing a large number of SA sub-questions.
      • When hovering over the textbox of a sub-question that has not yet been answered, no "Mark 0 out of 1" should be displayed.

        Issue Links

          Activity

          Hide
          Didier Jodin added a comment -

          "When checking his answers, in 2.1 the student should see the correctness/incorrectness of his answers clearly marked (color and icon) as it was in 1.9"

          I do agree... In adaptative mode, it's quite useful for students to be able to see quickly and easily what is rigth and what is wrong.
          No news about this improvement ?

          Show
          Didier Jodin added a comment - "When checking his answers, in 2.1 the student should see the correctness/incorrectness of his answers clearly marked (color and icon) as it was in 1.9" I do agree... In adaptative mode, it's quite useful for students to be able to see quickly and easily what is rigth and what is wrong. No news about this improvement ?
          Hide
          Tim Hunt added a comment -

          Is this bug still present in Moodle 2.3+?

          Show
          Tim Hunt added a comment - Is this bug still present in Moodle 2.3+?
          Hide
          Didier Jodin added a comment -

          Yes it is.
          I wouldn't call it a bug, but the improvement would be very welcome...
          At the moment, finding the wrong answer(s) is less obvious than it was in Moodle 1.9

          Show
          Didier Jodin added a comment - Yes it is. I wouldn't call it a bug, but the improvement would be very welcome... At the moment, finding the wrong answer(s) is less obvious than it was in Moodle 1.9
          Hide
          Joseph Rézeau added a comment -

          Nothing has changed (in Moodle 2.3+) since I reported these bugs almost a year ago.

          Show
          Joseph Rézeau added a comment - Nothing has changed (in Moodle 2.3+) since I reported these bugs almost a year ago.
          Hide
          Pierre Pichet added a comment -

          Some of the problems has been corrected see MDL-35370 which is waiting for integration.

          Show
          Pierre Pichet added a comment - Some of the problems has been corrected see MDL-35370 which is waiting for integration.
          Hide
          Pierre Pichet added a comment -

          We should note however that the adaptative in 2,3 for shortanswer does not display color on the response input display on check button response.
          The close follow the shortanswer display.
          On close pop up the feedback is shown and the grade seems OK.
          More tests are need to compare all the possibilities...

          Show
          Pierre Pichet added a comment - We should note however that the adaptative in 2,3 for shortanswer does not display color on the response input display on check button response. The close follow the shortanswer display. On close pop up the feedback is shown and the grade seems OK. More tests are need to compare all the possibilities...
          Hide
          Pierre Pichet added a comment -

          However the shortanswer handling in 1,9 put color and icon.

          Show
          Pierre Pichet added a comment - However the shortanswer handling in 1,9 put color and icon.
          Hide
          Aaron C Spike added a comment -

          I recently added a shortsighted hack to remedy this problem with the multianswer qtype in our Moodle.

          https://github.com/martinluther/moodle/commits/multianswerfeedback

          Show
          Aaron C Spike added a comment - I recently added a shortsighted hack to remedy this problem with the multianswer qtype in our Moodle. https://github.com/martinluther/moodle/commits/multianswerfeedback
          Hide
          Jean-Michel Vedrine added a comment -

          I don't agree that once you click on the "Submit all and finish" button the not answered subquestion Q03 should be left white, IMHO as the attempt is finished it should be considered false and be red as it is now.
          And also note that in latest master Q02 tooltip correctly show "Incorrect".
          So I think the 2 things marked as BUG by Joseph are no more a problem.
          This left us with the fact that subquestions are not coloured when you click on "Check".
          It seems that all opinions expressed in the forum are in favour of bringing this feature back.

          Show
          Jean-Michel Vedrine added a comment - I don't agree that once you click on the "Submit all and finish" button the not answered subquestion Q03 should be left white, IMHO as the attempt is finished it should be considered false and be red as it is now. And also note that in latest master Q02 tooltip correctly show "Incorrect". So I think the 2 things marked as BUG by Joseph are no more a problem. This left us with the fact that subquestions are not coloured when you click on "Check". It seems that all opinions expressed in the forum are in favour of bringing this feature back.
          Hide
          Tim Hunt added a comment -

          If someone can turn my suggested change https://moodle.org/mod/forum/discuss.php?d=226739#p984687 into a git commit, and write some unit tests, then I am happy to review and submit this for integration. I am afraid I don't have time to work on the code myself.

          Show
          Tim Hunt added a comment - If someone can turn my suggested change https://moodle.org/mod/forum/discuss.php?d=226739#p984687 into a git commit, and write some unit tests, then I am happy to review and submit this for integration. I am afraid I don't have time to work on the code myself.
          Hide
          Joseph Rézeau added a comment -

          I agree with Jean-Michel's comments dated 22/Apr/13 2:20 PM.
          I would be grateful if Jean-Michel would turn Tim's suggested changes in the Quiz forum into a git commit and write the relevant test units (at which J.-M. is very good ).

          Show
          Joseph Rézeau added a comment - I agree with Jean-Michel's comments dated 22/Apr/13 2:20 PM. I would be grateful if Jean-Michel would turn Tim's suggested changes in the Quiz forum into a git commit and write the relevant test units (at which J.-M. is very good ).
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Thanks Joseph but don't forget we are both french so I learned "Le corbeau et le renard" when I was a schoolboy
          Turning Tim's change in a github branch is quite easy.
          Question for Tim:
          For the tests, I assume we only want to test that the classes are present in the output, so something like

          question_pattern_expectation('~<input[^>]* class="correct" [^>]*/>~')
          and
          question_pattern_expectation('~<input[^>]* class="incorrect" [^>]*/>~')
          

          would be enough ? or do we want to test that each subquestion has the right class associated ?

          Show
          Jean-Michel Vedrine added a comment - - edited Thanks Joseph but don't forget we are both french so I learned "Le corbeau et le renard" when I was a schoolboy Turning Tim's change in a github branch is quite easy. Question for Tim: For the tests, I assume we only want to test that the classes are present in the output, so something like question_pattern_expectation('~<input[^>]* class= "correct" [^>]*/>~') and question_pattern_expectation('~<input[^>]* class= "incorrect" [^>]*/>~') would be enough ? or do we want to test that each subquestion has the right class associated ?
          Hide
          Tim Hunt added a comment -

          If you look at some of the other unit tests, for example https://github.com/maths/moodle-qtype_stack/blob/master/tests/test_base.php#L157, you will see that PHPunit has helpers to assert whether some HTML contains a given tag with given attributes.

          I think that if you use that, rather than regular expressions, then you can assert that particular inputs have a particular class. But, if that seems like too much work, the regular expressions you propose would be OK.

          Show
          Tim Hunt added a comment - If you look at some of the other unit tests, for example https://github.com/maths/moodle-qtype_stack/blob/master/tests/test_base.php#L157 , you will see that PHPunit has helpers to assert whether some HTML contains a given tag with given attributes. I think that if you use that, rather than regular expressions, then you can assert that particular inputs have a particular class. But, if that seems like too much work, the regular expressions you propose would be OK.
          Hide
          Jean-Michel Vedrine added a comment -

          Oh yes, I remember now, I already copied your check_output_contains_text_input function when testing formulas !! Should be easy to adapt this to test that inputs have the right class.
          I will do that tomorrow, I am too tired now.

          Show
          Jean-Michel Vedrine added a comment - Oh yes, I remember now, I already copied your check_output_contains_text_input function when testing formulas !! Should be easy to adapt this to test that inputs have the right class. I will do that tomorrow, I am too tired now.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          Sorry to bother you, but before I go further, can you have a look at the way I do my tests because I am not really sure I am heading in the right direction.
          Maybe I should try to write a get_contains_xxx and a get_does_not_contain_xxx function but I must admit I don't clearly see the benefit over check_output_contains_ functions ?
          stack walkthrough tests seems tu use a lot of check_output_contains_ and check_output_does_not_contain_ but other walkthrough tests don't.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, Sorry to bother you, but before I go further, can you have a look at the way I do my tests because I am not really sure I am heading in the right direction. Maybe I should try to write a get_contains_xxx and a get_does_not_contain_xxx function but I must admit I don't clearly see the benefit over check_output_contains_ functions ? stack walkthrough tests seems tu use a lot of check_output_contains_ and check_output_does_not_contain_ but other walkthrough tests don't.
          Hide
          Jean-Michel Vedrine added a comment -

          Oops, I realize my wait to use assertTag with a regexp is flawed in some case because incorrect and partiallycorrect both contains correct as a substring

          Show
          Jean-Michel Vedrine added a comment - Oops, I realize my wait to use assertTag with a regexp is flawed in some case because incorrect and partiallycorrect both contains correct as a substring
          Hide
          Tim Hunt added a comment -

          Right, this is not recorded anywhere except in my brain, but the old get_contains_xxx way is deprecated, and the check_output_contains_ way is recommended.

          I really must find time to fix all the existing tests in Moodle core, so this is clear.

          Show
          Tim Hunt added a comment - Right, this is not recorded anywhere except in my brain, but the old get_contains_xxx way is deprecated, and the check_output_contains_ way is recommended. I really must find time to fix all the existing tests in Moodle core, so this is clear.
          Hide
          Jean-Michel Vedrine added a comment -

          Splendid so I used the new way of doing things and not the deprecated one
          Additionally working on this gave you the occasion to say to somebody else what is the recommended way
          If you can tell me if my test are OK, I will continue working on this in the evening. I have some gardening to do for now.

          Show
          Jean-Michel Vedrine added a comment - Splendid so I used the new way of doing things and not the deprecated one Additionally working on this gave you the occasion to say to somebody else what is the recommended way If you can tell me if my test are OK, I will continue working on this in the evening. I have some gardening to do for now.
          Hide
          Tim Hunt added a comment -

          Looking good. Just a few minor points.

          I think you should rename check_output_contains_text_input_class -> check_output_contains_text_input_with_class

          I would suggest moving that method to question_testcase, since it would also be useful for other qtypes.

          I thinkyou should change

          $attributes['class'] = 'regexp:/' . $class . '/';
          

          to

          $attributes['class'] = 'regexp:/\b' . $class . '\b/';
          

          (\b means start or end of a word.)

          Show
          Tim Hunt added a comment - Looking good. Just a few minor points. I think you should rename check_output_contains_text_input_class -> check_output_contains_text_input_with_class I would suggest moving that method to question_testcase, since it would also be useful for other qtypes. I thinkyou should change $attributes['class'] = 'regexp:/' . $class . '/'; to $attributes['class'] = 'regexp:/\b' . $class . '\b/'; (\b means start or end of a word.)
          Hide
          Jean-Michel Vedrine added a comment - - edited

          I want to add a walkthrough test for a multianswer question in adaptive mode, but as I must both extend qbehaviour_adaptive_walkthrough and include multianswer helper to have some cloze questions defined, am I right in thinking the best solution is to create a new file named adaptive_walkthrough_test.php in question/type/multianswer/tests/ ?

          Show
          Jean-Michel Vedrine added a comment - - edited I want to add a walkthrough test for a multianswer question in adaptive mode, but as I must both extend qbehaviour_adaptive_walkthrough and include multianswer helper to have some cloze questions defined, am I right in thinking the best solution is to create a new file named adaptive_walkthrough_test.php in question/type/multianswer/tests/ ?
          Hide
          Jean-Michel Vedrine added a comment -

          Doh, my question was stupid, I just need to create a new function in question/behaviour/adaptive/tests/walkthrough_test.php I don't need a new file at all, or to extend qbehaviour_adaptive_walkthrough, or to include question/type/multianswer/tests/helper.php !

          Show
          Jean-Michel Vedrine added a comment - Doh, my question was stupid, I just need to create a new function in question/behaviour/adaptive/tests/walkthrough_test.php I don't need a new file at all, or to extend qbehaviour_adaptive_walkthrough, or to include question/type/multianswer/tests/helper.php !
          Hide
          Tim Hunt added a comment -

          I don't mind if you put it in behaviour_adaptive or in qtype_multianswer. I guess in the qtype is slightly better.

          Show
          Tim Hunt added a comment - I don't mind if you put it in behaviour_adaptive or in qtype_multianswer. I guess in the qtype is slightly better.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          asking for a first peer review. This seems to be working as expected and has unit tests for shortanswer and multianswer.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, asking for a first peer review. This seems to be working as expected and has unit tests for shortanswer and multianswer.
          Hide
          Jean-Michel Vedrine added a comment -

          Oops, I only saw your message "I guess in the qtype is slightly better" after doing it in the behaviour, but as you don't mind ...

          Show
          Jean-Michel Vedrine added a comment - Oops, I only saw your message "I guess in the qtype is slightly better" after doing it in the behaviour, but as you don't mind ...
          Hide
          Tim Hunt added a comment -

          I can no longer remember why this issue is sitting here in the 'peer review in progress' state.

          Is there anything still to do?

          If not, can you rebase and squash down to one commit, and cherry pick to stable branches if appropriate, which I guess it is.

          Show
          Tim Hunt added a comment - I can no longer remember why this issue is sitting here in the 'peer review in progress' state. Is there anything still to do? If not, can you rebase and squash down to one commit, and cherry pick to stable branches if appropriate, which I guess it is.
          Hide
          Jean-Michel Vedrine added a comment -

          No I don't think there is anything more to do.
          I will rebase, squash and create other branchs.

          Show
          Jean-Michel Vedrine added a comment - No I don't think there is anything more to do. I will rebase, squash and create other branchs.
          Hide
          Jean-Michel Vedrine added a comment -

          About creating branchs for MOODLE_24_STABLE and MOODLE_23_STABLE
          I have 2 solutions:

          • backport only the change without the tests
          • backport also the tests but that would require to add other functions to question/engine/tests helpers (for instance I need to backport also get_tag_matcher because I use it and it is only currently in master branch)
            What is your preference ?
          Show
          Jean-Michel Vedrine added a comment - About creating branchs for MOODLE_24_STABLE and MOODLE_23_STABLE I have 2 solutions: backport only the change without the tests backport also the tests but that would require to add other functions to question/engine/tests helpers (for instance I need to backport also get_tag_matcher because I use it and it is only currently in master branch) What is your preference ?
          Hide
          Tim Hunt added a comment -

          My general principles are:

          1. If the tests back-port easily, then you should back-port them.
          2. If not, then back-port without the unit tests.

          This seems to be case 2.

          Show
          Tim Hunt added a comment - My general principles are: If the tests back-port easily, then you should back-port them. If not, then back-port without the unit tests. This seems to be case 2.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          I think this is ready, I tested all branchs. I also verified once more that all tests are OK for adaptive behaviour and all question types on master.

          Show
          Jean-Michel Vedrine added a comment - - edited I think this is ready, I tested all branchs. I also verified once more that all tests are OK for adaptive behaviour and all question types on master.
          Hide
          Tim Hunt added a comment -

          Thanks. Submitting for integration.

          Show
          Tim Hunt added a comment - Thanks. Submitting for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23, 24 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 & master), thanks!
          Hide
          Adrian Greeve added a comment -

          Tested on the 2.3, 2.4 and master integration branches.
          The adaptive mode now shows colour highlighting.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on the 2.3, 2.4 and master integration branches. The adaptive mode now shows colour highlighting. Test passed.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim, Eloy and Adrian,
          Thanks a lot for this quick integration and testing. I am sure many users will appreciate this can be part of the future releases.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, Eloy and Adrian, Thanks a lot for this quick integration and testing. I am sure many users will appreciate this can be part of the future releases.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Did you think this day was not going to arrive ever?

          Your patience has been rewarded, yay, sent upstream, thanks!

          Closing...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao

            People

            • Votes:
              15 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: