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

          Attachments

            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 Mudrák added a comment -

            Yes, the AMOS script looks ok.

            Show
            mudrd8mz David Mudrák 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