Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-37172

Hardcoded strings in some question imports formats

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.6, 2.3.3, 2.4
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      You need to test that question import formats modified by this issue are still working :

      • Blackboard V6+
      • examview
      • Missing word
      • Learnwise
      • XHTML
        To do this try to import example files that are in the question/format/xxx/tests/fixtures/ subfolder of eachone. Some warning
      • The example file for Learnwise is at question/format/learnwise/learnwise-example.xml
      • Missing word need a special attention because it was broken since Moodle 2.0 and is now fixed. To test it import the 3 new example files in question/format/missingword/tests/fixtures (that said I don't really know if this old format is still used by somebody !)
      • XHTML is an export format only so to test it once you have some question in the question bank try to export them and look at the resulting file.
      Show
      You need to test that question import formats modified by this issue are still working : Blackboard V6+ examview Missing word Learnwise XHTML To do this try to import example files that are in the question/format/xxx/tests/fixtures/ subfolder of eachone. Some warning The example file for Learnwise is at question/format/learnwise/learnwise-example.xml Missing word need a special attention because it was broken since Moodle 2.0 and is now fixed. To test it import the 3 new example files in question/format/missingword/tests/fixtures (that said I don't really know if this old format is still used by somebody !) XHTML is an export format only so to test it once you have some question in the question bank try to export them and look at the resulting file.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:

      Description

      There are hard-coded strings in some of the question import formats.
      This prevent the translation of these strings in non English languages.

        Gliffy Diagrams

          Activity

          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          examview line 227
          missing word lines 74 82 110
          learnwise line 124
          xhtml line 147
          Also it would be good to use the same string in all formats when a question type is not handled.

          Show
          jmvedrine Jean-Michel Vedrine added a comment - examview line 227 missing word lines 74 82 110 learnwise line 124 xhtml line 147 Also it would be good to use the same string in all formats when a question type is not handled.
          Hide
          timhunt Tim Hunt added a comment -

          Are you able to fix this Jean-Michel. I agree with everything you say about what needs to be changed.

          Show
          timhunt Tim Hunt added a comment - Are you able to fix this Jean-Michel. I agree with everything you say about what needs to be changed.
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Hello Tim,
          Yes, this was my intention to fix this, but better let the "Big Boss" of quiz and question decide So you can happily assign this issue to me.

          Show
          jmvedrine Jean-Michel Vedrine added a comment - Hello Tim, Yes, this was my intention to fix this, but better let the "Big Boss" of quiz and question decide So you can happily assign this issue to me.
          Hide
          timhunt Tim Hunt added a comment -

          You are welcome to assign issues to yourself, if you are planning to work on them in the near future. Just add me as a watcher first, so I know what is going on.

          Show
          timhunt Tim Hunt added a comment - You are welcome to assign issues to yourself, if you are planning to work on them in the near future. Just add me as a watcher first, so I know what is going on.
          Hide
          timhunt Tim Hunt added a comment -

          Looking good, although there will be a few things to fix when you run codechecker.

          Show
          timhunt Tim Hunt added a comment - Looking good, although there will be a few things to fix when you run codechecker.
          Hide
          timhunt Tim Hunt added a comment -

          Oops! Note that MDL-37068 is waiting for integration. Some of your changes may conflict with some of those.

          Show
          timhunt Tim Hunt added a comment - Oops! Note that MDL-37068 is waiting for integration. Some of your changes may conflict with some of those.
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Hello Tim,
          it wasn't finished
          I had to fix the missing word format in the process because it was broken (always the same thing: the answer_format and feedback_format fields). It now seems to work both for shortanswer and multichoice. I have added the files I used to test it in question/format/missingword/tests/fixtures
          I am not really sure it is used by anybody because I found no report that it was broken !!
          I am unsure if I must delete the $string['unknownorunhandledtype'] from blackboard_six or not now that I have created exactly the same lang string in question component. I seems to remember it is forbidden to delete strings on stable branchs ?

          Show
          jmvedrine Jean-Michel Vedrine added a comment - Hello Tim, it wasn't finished I had to fix the missing word format in the process because it was broken (always the same thing: the answer_format and feedback_format fields). It now seems to work both for shortanswer and multichoice. I have added the files I used to test it in question/format/missingword/tests/fixtures I am not really sure it is used by anybody because I found no report that it was broken !! I am unsure if I must delete the $string ['unknownorunhandledtype'] from blackboard_six or not now that I have created exactly the same lang string in question component. I seems to remember it is forbidden to delete strings on stable branchs ?
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Yes I just noticed the MDL-37068 dependence! How to deal with that ?

          Show
          jmvedrine Jean-Michel Vedrine added a comment - Yes I just noticed the MDL-37068 dependence! How to deal with that ?
          Hide
          timhunt Tim Hunt added a comment -

          Correct, you cannot delete strings on stable branches, so leave the unused string there, but delete it from master. (Really, you should write an AMOS script http://docs.moodle.org/dev/Languages/AMOS#AMOS_script so that the string automatically gets moved in all language packs.)

          About MDL-37068, I think we may have to wait until that is integrated, then you can re-base your patch on top of it.

          Show
          timhunt Tim Hunt added a comment - Correct, you cannot delete strings on stable branches, so leave the unused string there, but delete it from master. (Really, you should write an AMOS script http://docs.moodle.org/dev/Languages/AMOS#AMOS_script so that the string automatically gets moved in all language packs.) About MDL-37068 , I think we may have to wait until that is integrated, then you can re-base your patch on top of it.
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Hello Tim,
          Now that MDL-37068 is closed, I need to finish this one.

          • Squash all commits.
          • Do branchs for Moodle 2.3 and 2.4 without the string removal
          • For the AMOS script I think I understand what I should put in it but it's unclear where I should put this script after that.
          • Write testing instructions, I suppose we need to test Missing word format as it was fixed in the process but for the hardcoded strings, how to test ?
          Show
          jmvedrine Jean-Michel Vedrine added a comment - Hello Tim, Now that MDL-37068 is closed, I need to finish this one. Squash all commits. Do branchs for Moodle 2.3 and 2.4 without the string removal For the AMOS script I think I understand what I should put in it but it's unclear where I should put this script after that. Write testing instructions, I suppose we need to test Missing word format as it was fixed in the process but for the hardcoded strings, how to test ?
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          I put it in peer review so that you can have a look at the code before I do the final changes in the above list

          Show
          jmvedrine Jean-Michel Vedrine added a comment - I put it in peer review so that you can have a look at the code before I do the final changes in the above list
          Hide
          timhunt Tim Hunt added a comment -

          Looking good.

          The AMOS script should go in the commit comment.

          The main thing we need to test is that nothing is broken, so import some example files of each type.

          For missingword, there are two examples in the comment at the top of question/format/missingword/format.php. It would be good to save those into tests/fixtures as an example file.

          Show
          timhunt Tim Hunt added a comment - Looking good. The AMOS script should go in the commit comment. The main thing we need to test is that nothing is broken, so import some example files of each type. For missingword, there are two examples in the comment at the top of question/format/missingword/format.php. It would be good to save those into tests/fixtures as an example file.
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Those 2 examples (and a third one) are already part of the branch . I have put them when I discovered them in the code and used them for testing.

          Show
          jmvedrine Jean-Michel Vedrine added a comment - Those 2 examples (and a third one) are already part of the branch . I have put them when I discovered them in the code and used them for testing.
          Hide
          timhunt Tim Hunt added a comment -

          Doh! sorry. There were such short files I missed them.

          Show
          timhunt Tim Hunt added a comment - Doh! sorry. There were such short files I missed them.
          Hide
          timhunt Tim Hunt added a comment -

          Just updating workflow state. I think I am waiting for you to do the things listed above.

          Show
          timhunt Tim Hunt added a comment - Just updating workflow state. I think I am waiting for you to do the things listed above.
          Hide
          timhunt Tim Hunt added a comment -

          Ping! Where have we got to with this?

          Show
          timhunt Tim Hunt added a comment - Ping! Where have we got to with this?
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Hello Tim,
          Sorry but new semester has started so I can only work on Moodle during the week-ends ... I hope to be able to finish this one tomorrow and Sunday.

          Show
          jmvedrine Jean-Michel Vedrine added a comment - Hello Tim, Sorry but new semester has started so I can only work on Moodle during the week-ends ... I hope to be able to finish this one tomorrow and Sunday.
          Hide
          timhunt Tim Hunt added a comment -

          That is OK. There is no need to apologise. I just wanted to make sure that you were not waiting for me to do anything. I am away for most of this weekend, but will probably check to see if there is anything for me to peer review once I get home on Sunday evening.

          Show
          timhunt Tim Hunt added a comment - That is OK. There is no need to apologise. I just wanted to make sure that you were not waiting for me to do anything. I am away for most of this weekend, but will probably check to see if there is anything for me to peer review once I get home on Sunday evening.
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Hello Tim,
          rebasing and squashing done.
          If I am right, master branch should have an AMOS script:
          AMOS BEGIN
          MOV [unknownorunhandledtype,qformat_blackboard_six],[unknownorunhandledtype,question]
          AMOS END
          in the commit comment.
          But what about the stable branchs ?
          As I don't delete the string and create a new one should I include an AMOS script to copy the string ?
          AMOS BEGIN
          CPY [unknownorunhandledtype,qformat_blackboard_six],[unknownorunhandledtype,question]
          AMOS END
          That seems logical to me, but not really sure as it's my first time with AMOS scripts.

          Show
          jmvedrine Jean-Michel Vedrine added a comment - Hello Tim, rebasing and squashing done. If I am right, master branch should have an AMOS script: AMOS BEGIN MOV [unknownorunhandledtype,qformat_blackboard_six] , [unknownorunhandledtype,question] AMOS END in the commit comment. But what about the stable branchs ? As I don't delete the string and create a new one should I include an AMOS script to copy the string ? AMOS BEGIN CPY [unknownorunhandledtype,qformat_blackboard_six] , [unknownorunhandledtype,question] AMOS END That seems logical to me, but not really sure as it's my first time with AMOS scripts.
          Hide
          timhunt Tim Hunt added a comment -

          That looks right to me.

          David Mudrak, if you see this, please could you confirm.

          Show
          timhunt Tim Hunt added a comment - That looks right to me. David Mudrak, if you see this, please could you confirm.
          Hide
          timhunt Tim Hunt added a comment -

          Thanks. Submitting for integration.

          Show
          timhunt Tim Hunt added a comment - Thanks. Submitting for integration.
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Thanks I have added some testing instructions. I hope my AMOS scripts are OK. But I think integrators will verify them.
          Have a nice week-end Tim.

          Show
          jmvedrine Jean-Michel Vedrine added a comment - Thanks I have added some testing instructions. I hope my AMOS scripts are OK. But I think integrators will verify them. Have a nice week-end Tim.
          Hide
          mudrd8mz David Mudrak added a comment -

          Yes, the AMOS script looks ok.

          Show
          mudrd8mz David Mudrak added a comment - Yes, the AMOS script looks ok.
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Thanks David.

          Show
          jmvedrine Jean-Michel Vedrine added a comment - Thanks David.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks guys, this has been integrated now.

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks guys, this has been integrated now.
          Hide
          andyjdavis Andrew Davis added a comment -

          Tested in master. Looks good.

          Show
          andyjdavis Andrew Davis added a comment - Tested in master. Looks good.
          Hide
          andyjdavis Andrew Davis added a comment -

          Also tested in 2.3 and 2.4. Passing.

          Show
          andyjdavis Andrew Davis added a comment - Also tested in 2.3 and 2.4. Passing.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

          (and won't be revisiting it unless some regression is found)

          Thanks and ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                11/Mar/13