Uploaded image for project: '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
    • Status: Closed
    • Priority: 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 Master Branch:

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            didier.jodin@free.fr 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@free.fr 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
            timhunt Tim Hunt added a comment -

            Is this bug still present in Moodle 2.3+?

            Show
            timhunt Tim Hunt added a comment - Is this bug still present in Moodle 2.3+?
            Hide
            didier.jodin@free.fr 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@free.fr 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
            rezeau Joseph Rézeau added a comment -

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

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

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

            Show
            ppichet Pierre Pichet added a comment - Some of the problems has been corrected see MDL-35370 which is waiting for integration.
            Hide
            ppichet 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
            ppichet 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
            ppichet Pierre Pichet added a comment -

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

            Show
            ppichet Pierre Pichet added a comment - However the shortanswer handling in 1,9 put color and icon.
            Hide
            acspike 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
            acspike 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
            jmvedrine 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
            jmvedrine 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
            timhunt 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
            timhunt 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
            rezeau 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
            rezeau 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
            jmvedrine 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
            jmvedrine 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
            timhunt 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
            timhunt 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
            jmvedrine 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
            jmvedrine 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
            jmvedrine 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
            jmvedrine 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
            jmvedrine 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
            jmvedrine 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
            timhunt 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
            timhunt 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
            jmvedrine 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
            jmvedrine 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
            timhunt 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
            timhunt 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
            jmvedrine 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
            jmvedrine 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
            jmvedrine 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
            jmvedrine 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
            timhunt 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
            timhunt 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
            jmvedrine 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
            jmvedrine 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
            jmvedrine 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
            jmvedrine 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
            timhunt 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
            timhunt 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
            jmvedrine 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
            jmvedrine 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
            jmvedrine 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
            jmvedrine 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
            timhunt 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
            timhunt 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
            jmvedrine 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
            jmvedrine 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
            timhunt Tim Hunt added a comment -

            Thanks. Submitting for integration.

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

            Integrated (23, 24 & master), thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 & master), thanks!
            Hide
            abgreeve 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
            abgreeve 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
            jmvedrine 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
            jmvedrine 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
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  13/May/13