Moodle
  1. Moodle
  2. MDL-17229 Shortanswer questiontype refactoring
  3. MDL-20157

Add extra_question_fields() support to Moodle XML import/export format

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.5
    • Fix Version/s: 1.9.6
    • Component/s: Questions
    • Labels:
      None
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      35832

      Description

      I think Moodle XML question importing/exporting can also support extra_question_fields() function. As always I am willing to use shortanswer question as an example and a testing case. Basic questiontype class could implement export_to_xml and import_from_xml functions, using extra_question_fields. I hope you will be interested in it.

      1. quiz-tpr-shortanswer-new-export.xml
        2 kB
        Oleg Sychev
      2. quiz-tpr-shortanswer-old-export.xml
        2 kB
        Oleg Sychev
      3. xmlexportimport.patch
        6 kB
        Oleg Sychev
      4. xmlimport-tim.patch.txt
        6 kB
        Tim Hunt

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Yes. This would be very useful to have. Thanks.

          Show
          Tim Hunt added a comment - Yes. This would be very useful to have. Thanks.
          Hide
          Oleg Sychev added a comment -

          It seems that an obsolete "asnwers" field is interfering with this, as it isn't exported to moodle xml format.
          We could:
          1) drop "asnwers" field supporting (i.e. start with MDL-17812)
          2) create a hack working around it (ignoring this field)
          3) create a hack in 1.9 and drop "asnwers" support in 2.0

          Show
          Oleg Sychev added a comment - It seems that an obsolete "asnwers" field is interfering with this, as it isn't exported to moodle xml format. We could: 1) drop "asnwers" field supporting (i.e. start with MDL-17812 ) 2) create a hack working around it (ignoring this field) 3) create a hack in 1.9 and drop "asnwers" support in 2.0
          Hide
          Oleg Sychev added a comment -

          This is a patch to do a job. Fairly simple one so I hope review won't be that long.

          Things to consider:
          1) Due to problem with "answers" field I didn't delete shortanswer code from xml format, so shortanswer question is still importing/exporting directly using format code right now. Hovewer my testing site run without "answers" field for almost a half-year. So I was able to test implementation on shortanswer question type - it is fully interoperable with an existing one in abscence of "answers" field. I did both exporting using new code while importing using old and vice versa. I also diffed xml files generated by existing and new export. All works OK.

          Right now extended functionality will be available for any 3d party qtypes using question_extra_fields(). We can drop special handling of shortanswer from format.php after dropping "answers" field.

          2) Performance of try_importing_using_qtypes() function was not great from the beginning as it loops throught all qtypes for each question and it will be even worse after implementing import_from_xml function in base questiontype class. I improved this by letting format to make a hint on qtype of the question. You may choose to use this part (changes in two format.php files) or not, it's up to you.

          Show
          Oleg Sychev added a comment - This is a patch to do a job. Fairly simple one so I hope review won't be that long. Things to consider: 1) Due to problem with "answers" field I didn't delete shortanswer code from xml format, so shortanswer question is still importing/exporting directly using format code right now. Hovewer my testing site run without "answers" field for almost a half-year. So I was able to test implementation on shortanswer question type - it is fully interoperable with an existing one in abscence of "answers" field. I did both exporting using new code while importing using old and vice versa. I also diffed xml files generated by existing and new export. All works OK. Right now extended functionality will be available for any 3d party qtypes using question_extra_fields(). We can drop special handling of shortanswer from format.php after dropping "answers" field. 2) Performance of try_importing_using_qtypes() function was not great from the beginning as it loops throught all qtypes for each question and it will be even worse after implementing import_from_xml function in base questiontype class. I improved this by letting format to make a hint on qtype of the question. You may choose to use this part (changes in two format.php files) or not, it's up to you.
          Hide
          Tim Hunt added a comment -

          The code looks OK, but I do not have time to merge and test today. WIll attend to it when I have a moment.

          Show
          Tim Hunt added a comment - The code looks OK, but I do not have time to merge and test today. WIll attend to it when I have a moment.
          Hide
          Oleg Sychev added a comment -

          I diffed xml/format.php for 1.9 and 2.0 and see no troubles in merging changes. The testing will be more difficult for you thought, as right now new code doesn't affect any core question types at all I have to use tricks to test all modifications there.

          I do hope you'll find time to apply this before you get engrossed in another large project...

          Show
          Oleg Sychev added a comment - I diffed xml/format.php for 1.9 and 2.0 and see no troubles in merging changes. The testing will be more difficult for you thought, as right now new code doesn't affect any core question types at all I have to use tricks to test all modifications there. I do hope you'll find time to apply this before you get engrossed in another large project...
          Hide
          Tim Hunt added a comment -

          Some comments:

          1. Coding style, it should be if (is_object($qtype) ... - that is with a space after the if. http://docs.moodle.org/en/Development:Coding_style#Control_statements

          2. $qtype = $QTYPES[$qtypehint]; will generate a PHP warning if $qtypehint == '', which is the default.

          3. It would be a lot easier if you could merge things to 2.0 yourself.

          Show
          Tim Hunt added a comment - Some comments: 1. Coding style, it should be if (is_object($qtype) ... - that is with a space after the if. http://docs.moodle.org/en/Development:Coding_style#Control_statements 2. $qtype = $QTYPES [$qtypehint] ; will generate a PHP warning if $qtypehint == '', which is the default. 3. It would be a lot easier if you could merge things to 2.0 yourself.
          Hide
          Tim Hunt added a comment -

          4. And not spaces around funtion arguments. That is
          get_class( $this ) IS WRONG
          get_class($this) IS RIGHT

          Show
          Tim Hunt added a comment - 4. And not spaces around funtion arguments. That is get_class( $this ) IS WRONG get_class($this) IS RIGHT
          Hide
          Tim Hunt added a comment -

          The code in default_questiontype::import_from_xml would be better with guard clauses rather than nested ifs. (http://c2.com/cgi/wiki?GuardClause)

          Show
          Tim Hunt added a comment - The code in default_questiontype::import_from_xml would be better with guard clauses rather than nested ifs. ( http://c2.com/cgi/wiki?GuardClause )
          Hide
          Tim Hunt added a comment -

          Here is a revised patch. Can you test it before I merge it to 2.0 and commit it?

          Show
          Tim Hunt added a comment - Here is a revised patch. Can you test it before I merge it to 2.0 and commit it?
          Hide
          Oleg Sychev added a comment -

          You patch seems to have tabs before "return false" on guard clauses in default_questiontype::import_from_xml and also in qformat_default::try_importing_using_qtypes

          Show
          Oleg Sychev added a comment - You patch seems to have tabs before "return false" on guard clauses in default_questiontype::import_from_xml and also in qformat_default::try_importing_using_qtypes
          Hide
          Oleg Sychev added a comment -

          Apart from using tabs for indenting patch is working and OK. I make cross import/export tests and also diffed xml files. The only difference is an additional space that old format inserting before <answer fraction=''> You may choose to keep it or not.

          I attached files exported using an old and a new method in case you are interested.

          Show
          Oleg Sychev added a comment - Apart from using tabs for indenting patch is working and OK. I make cross import/export tests and also diffed xml files. The only difference is an additional space that old format inserting before <answer fraction=''> You may choose to keep it or not. I attached files exported using an old and a new method in case you are interested.
          Hide
          Tim Hunt added a comment -

          All whitespace problems fixed (thanks for noticing Oleg) and merged ot 2.0.

          Thanks for implementing this Oleg.

          Show
          Tim Hunt added a comment - All whitespace problems fixed (thanks for noticing Oleg) and merged ot 2.0. Thanks for implementing this Oleg.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: