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
    • Rank:
      46729

      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

        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: