Uploaded image for project: '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
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              timhunt Tim Hunt added a comment -

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

              Show
              timhunt Tim Hunt added a comment - Yes. This would be very useful to have. Thanks.
              Hide
              oa_sychev 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
              oa_sychev 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
              oa_sychev 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
              oa_sychev 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
              timhunt 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
              timhunt 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
              oa_sychev 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
              oa_sychev 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
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              oa_sychev 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
              oa_sychev 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
              oa_sychev 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
              oa_sychev 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
              timhunt Tim Hunt added a comment -

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

              Thanks for implementing this Oleg.

              Show
              timhunt 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:
                    Fix Release Date:
                    21/Oct/09