Moodle
  1. Moodle
  2. MDL-37157

Multianswer cloze fails to compare unicode strings correctly

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.3
    • Fix Version/s: 2.3.4, 2.4.1
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      To test this, you need to understand about the different ways that you can write accented characters like 'départ'. The options are to either use the single character LATIN SMALL LETTER E WITH ACUTE , or the combination LATIN SMALL LETTER E then COMBINING ACCENT ACUTE. When grading Moodle short-answer questions, we want to consider those the same thing.

      So, you need to create a two short answer questions, one where the right answer is represneted one way, and the other where it is represented the other way.

      Then you need to answer both questions using both representations, and make sure they are all graded correct.

      Show
      To test this, you need to understand about the different ways that you can write accented characters like 'départ'. The options are to either use the single character LATIN SMALL LETTER E WITH ACUTE , or the combination LATIN SMALL LETTER E then COMBINING ACCENT ACUTE. When grading Moodle short-answer questions, we want to consider those the same thing. So, you need to create a two short answer questions, one where the right answer is represneted one way, and the other where it is represented the other way. Then you need to answer both questions using both representations, and make sure they are all graded correct.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDLmaster-MDL-37157-2

      Description

      We received a report that two users filing the (apparently) exact same string in a cloze question were "judged" differently.

      The students were supposed to answer "répondre" (in french).

      • In one case, the equivalent unicode string was "72 65 CC 81 70 6F 6E 64 72 65" (the "é" is represented as "65
        CC 81", which is "e" + "ACUTE"
      • In the other case, the equivalent unicode string was "72 C3 A9 70 6F 6E 64 72 65" (the "é" is represented as "C3 A9", which is "EACUTE"

      This looks like a use case for using Normalizer:

      Normalizer::normalize($string, Normalizer::FORM_D)

      on all strings-to-be-compared (or FORM_C, I'm not quite sure).

      So far I failed to find where this could be tested, a pointer would be helpful.

      Cheers, OdyX

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Tim Hunt added a comment -

            I expect the same problem affects the short-answer question type too.

            It looks like relying on Normalizer is OK, because it is provided by the intl extension. See https://moodle.org/mod/forum/discuss.php?d=202478. Mind you, I think that is only recommended, not strictly required.

            Anyway, the place to look to implement this is probably in compare_string_with_wildcard in question/type/shortanswer/question.php. It would also be good to add some examples to question/type/shortanswer/tests/question_test.php.

            Show
            Tim Hunt added a comment - I expect the same problem affects the short-answer question type too. It looks like relying on Normalizer is OK, because it is provided by the intl extension. See https://moodle.org/mod/forum/discuss.php?d=202478 . Mind you, I think that is only recommended, not strictly required. Anyway, the place to look to implement this is probably in compare_string_with_wildcard in question/type/shortanswer/question.php. It would also be good to add some examples to question/type/shortanswer/tests/question_test.php.
            Hide
            Didier Raboud added a comment -

            Hi Tim,

            There you go. I have tested it on the site where we were having problems and it works. I have also added some tests that show correct behaviour.

            I'll also backport that to Moodle 2.3 as that's the release I need this in, but it should be trivial I guess.

            Comments welcome!

            Cheers, OdyX

            Show
            Didier Raboud added a comment - Hi Tim, There you go. I have tested it on the site where we were having problems and it works. I have also added some tests that show correct behaviour. I'll also backport that to Moodle 2.3 as that's the release I need this in, but it should be trivial I guess. Comments welcome! Cheers, OdyX
            Hide
            Tim Hunt added a comment -

            Looks good, apart from some minor coding style issues.

            1. You need a space after a comma at https://github.com/OdyX/moodle/compare/MDLmaster-MDL-37157#L0R106 and the following line.

            2. The body of this if-statement is indented 8-spaces. It should be 4. https://github.com/OdyX/moodle/compare/MDLmaster-MDL-37157#L1R111

            You may find this plugin helpful: https://github.com/moodlehq/moodle-local_codechecker

            Show
            Tim Hunt added a comment - Looks good, apart from some minor coding style issues. 1. You need a space after a comma at https://github.com/OdyX/moodle/compare/MDLmaster-MDL-37157#L0R106 and the following line. 2. The body of this if-statement is indented 8-spaces. It should be 4. https://github.com/OdyX/moodle/compare/MDLmaster-MDL-37157#L1R111 You may find this plugin helpful: https://github.com/moodlehq/moodle-local_codechecker
            Hide
            Didier Raboud added a comment -

            Damn. I had managed to avoided tabs, but didn't see those. Thanks! (And thanks for the local_codechecker hint, looks useful!)

            Pushed a new branch against master (dropped the one against MOODLE_24_STABLE as it cherry-picks without a glitch).

            Show
            Didier Raboud added a comment - Damn. I had managed to avoided tabs, but didn't see those. Thanks! (And thanks for the local_codechecker hint, looks useful!) Pushed a new branch against master (dropped the one against MOODLE_24_STABLE as it cherry-picks without a glitch).
            Hide
            Tim Hunt added a comment -

            To INTEGRATORS: Please cherry-pick to stable branches >=2.3.

            Show
            Tim Hunt added a comment - To INTEGRATORS: Please cherry-pick to stable branches >=2.3.
            Hide
            Dan Poltawski added a comment -

            I can create LATIN SMALL LETTER E WITH ACUTE on OSX by typing option-e, e, but trying to find the other way of doing it

            Show
            Dan Poltawski added a comment - I can create LATIN SMALL LETTER E WITH ACUTE on OSX by typing option-e, e, but trying to find the other way of doing it
            Hide
            Dan Poltawski added a comment -

            LATIN SMALL LETTER E WITH ACUTE: é
            LATIN SMALL LETTER E + COMBINING ACCENT ACUTE: é

            Show
            Dan Poltawski added a comment - LATIN SMALL LETTER E WITH ACUTE: é LATIN SMALL LETTER E + COMBINING ACCENT ACUTE: é
            Hide
            Dan Poltawski added a comment -

            Integrated to master, 24 and 23.

            Thanks Didier

            Show
            Dan Poltawski added a comment - Integrated to master, 24 and 23. Thanks Didier
            Hide
            Dan Poltawski added a comment -

            Tim/Didier, if its important to test this by typign these characters (rather than copy and paste - (which I suppose the OS might convert?)) could you help us by telling us how to type the characters on one operating system?

            (When I was trying to get this tested on Christmas eve I couldn't discover it from googling)

            Show
            Dan Poltawski added a comment - Tim/Didier, if its important to test this by typign these characters (rather than copy and paste - (which I suppose the OS might convert?)) could you help us by telling us how to type the characters on one operating system? (When I was trying to get this tested on Christmas eve I couldn't discover it from googling)
            Hide
            Tim Hunt added a comment -

            I don't know. You will need to ask Didier.

            I guess that really the unit tests do most of the testing we need.

            Show
            Tim Hunt added a comment - I don't know. You will need to ask Didier. I guess that really the unit tests do most of the testing we need.
            Hide
            Didier Raboud added a comment -

            I don't know. I could not enter the characters myself, only acknowledge that some users managed to enter those: I suspect that the differences come from users on different OSes, and/or from different locales. I agree with Tim that the unit tests are probably sufficient.

            Show
            Didier Raboud added a comment - I don't know. I could not enter the characters myself, only acknowledge that some users managed to enter those: I suspect that the differences come from users on different OSes, and/or from different locales. I agree with Tim that the unit tests are probably sufficient.
            Hide
            David Monllaó added a comment -

            It passes, all questions graded as expected. Tested with Linux and

            I've been able to reproduce the problem reverting the commit

            Show
            David Monllaó added a comment - It passes, all questions graded as expected. Tested with Linux and http://www.fileformat.info/info/unicode/char/e9/index.htm http://www.fileformat.info/info/unicode/char/0065/index.htm + http://www.fileformat.info/info/unicode/char/0301/index.htm I've been able to reproduce the problem reverting the commit
            Hide
            Eloy Lafuente (stronk7) added a comment -

            And your fantastic code has met core, hope they become good friends for a long period.

            Closing, thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!
            Hide
            Tim Hunt added a comment -

            Didier, you might be interested in this regression that this change caused: MDL-37746. I have no idea what is really going on there.

            Show
            Tim Hunt added a comment - Didier, you might be interested in this regression that this change caused: MDL-37746 . I have no idea what is really going on there.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: