Moodle
  1. Moodle
  2. MDL-37172

Hardcoded strings in some question imports formats

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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 2.4 Branch:
    • Pull Master Branch:
    • Rank:
      46745

      Description

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

        Activity

        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        Tim Hunt added a comment -

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

        Show
        Tim Hunt added a comment - Looking good, although there will be a few things to fix when you run codechecker.
        Hide
        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
        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
        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
        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
        Jean-Michel Vedrine added a comment -

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

        Show
        Jean-Michel Vedrine added a comment - Yes I just noticed the MDL-37068 dependence! How to deal with that ?
        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        Tim Hunt added a comment -

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

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

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

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

        Ping! Where have we got to with this?

        Show
        Tim Hunt added a comment - Ping! Where have we got to with this?
        Hide
        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
        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
        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
        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
        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
        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
        Tim Hunt added a comment -

        That looks right to me.

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

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

        Thanks. Submitting for integration.

        Show
        Tim Hunt added a comment - Thanks. Submitting for integration.
        Hide
        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
        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
        David Mudrak added a comment -

        Yes, the AMOS script looks ok.

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

        Thanks David.

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

        Thanks guys, this has been integrated now.

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

        Tested in master. Looks good.

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

        Also tested in 2.3 and 2.4. Passing.

        Show
        Andrew Davis added a comment - Also tested in 2.3 and 2.4. Passing.
        Hide
        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
        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: