Moodle

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

Details

  • Type: Sub-task Sub-task
  • Status: Closed 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

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.

Attachments

  1. XML File
    quiz-tpr-shortanswer-new-export.xml
    30/Sep/09 8:20 PM
    2 kB
    Oleg Sychev
  2. XML File
    quiz-tpr-shortanswer-old-export.xml
    30/Sep/09 8:20 PM
    2 kB
    Oleg Sychev
  3. Text File
    xmlexportimport.patch
    01/Sep/09 5:47 AM
    6 kB
    Oleg Sychev
  4. Text File
    xmlimport-tim.patch.txt
    30/Sep/09 7:38 PM
    6 kB
    Tim Hunt

Issue Links

This issue has been marked as being related by:
MDL-25617 Backup/restore of extra_question_fields broken in 2.0 Major Peer review in progress

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.