Moodle
  1. Moodle
  2. MDL-28564

implement support for extra answer fields in question export and import

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1
    • Fix Version/s: 2.1.2
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      1. First run all the question unit tests. That gives this code a thorough work-out.

      2. Then, as a double-check, make sure you can export and import some questions in Moodle XML format using the question bank user-interface.

      Please check tricky things, like question texts containing < and &.

      Show
      1. First run all the question unit tests. That gives this code a thorough work-out. 2. Then, as a double-check, make sure you can export and import some questions in Moodle XML format using the question bank user-interface. Please check tricky things, like question texts containing < and &.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      18334

      Issue Links

        Activity

        Hide
        Tim Hunt added a comment -

        Jamie, is this a duplicate of MDL-24408?

        Show
        Tim Hunt added a comment - Jamie, is this a duplicate of MDL-24408 ?
        Hide
        Jamie Pratt added a comment -

        Yes, it is. Sorry didn't realize there was an outstanding issue.

        Show
        Jamie Pratt added a comment - Yes, it is. Sorry didn't realize there was an outstanding issue.
        Hide
        Jamie Pratt added a comment -

        Actually this issue just addresses the import and export of extra answer fields. MDL-24408 seems to address a lot of other aspects of extra answer fields.

        Show
        Jamie Pratt added a comment - Actually this issue just addresses the import and export of extra answer fields. MDL-24408 seems to address a lot of other aspects of extra answer fields.
        Hide
        Jamie Pratt added a comment -

        I have a patch for this :

        https://github.com/jamiepratt/moodle/compare/master...MDL-28564_wip

        Currently import and export in the varnumeric question type depends on this patch. I could move all the functionality into the question type and not call the parent class if it is undecided what the best way is to implement extra answer fields.

        Show
        Jamie Pratt added a comment - I have a patch for this : https://github.com/jamiepratt/moodle/compare/master...MDL-28564_wip Currently import and export in the varnumeric question type depends on this patch. I could move all the functionality into the question type and not call the parent class if it is undecided what the best way is to implement extra answer fields.
        Hide
        Jamie Pratt added a comment -

        Here is the related bug in the OU tracker : http://ltsredmine.open.ac.uk/issues/1171

        And my patch on github for the question type plug in code : https://github.com/jamiepratt/moodle-qtype_varnumeric/commit/b196bc1e1dc404e408e748eb875699e742ee4a07

        Show
        Jamie Pratt added a comment - Here is the related bug in the OU tracker : http://ltsredmine.open.ac.uk/issues/1171 And my patch on github for the question type plug in code : https://github.com/jamiepratt/moodle-qtype_varnumeric/commit/b196bc1e1dc404e408e748eb875699e742ee4a07
        Hide
        Michael de Raadt added a comment -

        Thanks for reporting this and providing a solution.

        Show
        Michael de Raadt added a comment - Thanks for reporting this and providing a solution.
        Hide
        Tim Hunt added a comment -

        Some code-review comments:

        1. don't use static methods like wrap_html_special_chars without good reason. This should be a normal method.

        2. do write PHP doc comments for any new methods you create.

        3. However, this method should not exist at all. It is close to duplicating the writetext method in the format class. So, the method you want belongs in the format class, and you should refactor to avoid duplication.

        I am about to make a revised patch.

        Show
        Tim Hunt added a comment - Some code-review comments: 1. don't use static methods like wrap_html_special_chars without good reason. This should be a normal method. 2. do write PHP doc comments for any new methods you create. 3. However, this method should not exist at all. It is close to duplicating the writetext method in the format class. So, the method you want belongs in the format class, and you should refactor to avoid duplication. I am about to make a revised patch.
        Hide
        Tim Hunt added a comment -

        In a sense this is a new feature, so it should not go on the 2.1 branch.

        On the other hand, it would be very helpful for the OU to have this in 2.1, and it will not break or change any existing functionality. It will only make it easier for people writing third-parth question types. And the unit tests verify that nothing is broken. Therefore, please merge this onto the 2.1 branch as well as master. Thanks.

        Show
        Tim Hunt added a comment - In a sense this is a new feature, so it should not go on the 2.1 branch. On the other hand, it would be very helpful for the OU to have this in 2.1, and it will not break or change any existing functionality. It will only make it easier for people writing third-parth question types. And the unit tests verify that nothing is broken. Therefore, please merge this onto the 2.1 branch as well as master. Thanks.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Sam Hemelryk added a comment -

        Thanks guys, this has been integrated to 21 and master.
        Just noting that the first commit message could be made clearer, certainly the quoting is interesting.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Thanks guys, this has been integrated to 21 and master. Just noting that the first commit message could be made clearer, certainly the quoting is interesting. Cheers Sam
        Hide
        Rossiani Wijaya added a comment -

        Exporting and importing question works great. Question texts are also tested with the following chars:

        • single and double quotes
        • & (and sign)
        • <> (greater and less than signs)

        Test passed.

        Show
        Rossiani Wijaya added a comment - Exporting and importing question works great. Question texts are also tested with the following chars: single and double quotes & (and sign) <> (greater and less than signs) Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        git & cvs repositories have been populated with this solution. Many thanks for your collaboration, yay!

        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - git & cvs repositories have been populated with this solution. Many thanks for your collaboration, yay! Closing, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: