Moodle
  1. Moodle
  2. MDL-39230

refactor question import code to allow reuse of code for an individual question's import

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      1. Run all the unit tests in question/format.
      2. Import the four sample files in question/format/xml/tests/fixtures.
      3. Find a category in your question bank containing a lot of questions. Export it as Moodle XML, and re-import into a new course/category. Check that everything has come across correctly.

      Show
      1. Run all the unit tests in question/format. 2. Import the four sample files in question/format/xml/tests/fixtures. 3. Find a category in your question bank containing a lot of questions. Export it as Moodle XML, and re-import into a new course/category. Check that everything has come across correctly.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip_MDL-39230_2_5

      Description

      For combined question type we need a way to import and export sub questions from the question engine.

      It is easy to transform the sub question data to an xml export to include in the xml for the combined question xml.

      But to import that xml back into the db I want to reuse the existing code for interpretting the xml and then for saving the data in the db.

      I have refactored the question code so that the existing question type api is not changed but have added access to the needed functions.

      I thought it was a good idea to move the saving of the common question settings into the base question type class which is where the code for saving the common settings from the form is already.

        Gliffy Diagrams

          Activity

          Hide
          Jamie Pratt added a comment -

          I did also look into exporting the child questions as separate questions, not included inside the xml for the combined question, but as separate questions in the export and import. But I didn't see a way to do that with the current api.

          That would be an alternative approach, if we had a way for a question type to say - please export these other questions too and the parent had a way to correctly set the parent question id.

          Show
          Jamie Pratt added a comment - I did also look into exporting the child questions as separate questions, not included inside the xml for the combined question, but as separate questions in the export and import. But I didn't see a way to do that with the current api. That would be an alternative approach, if we had a way for a question type to say - please export these other questions too and the parent had a way to correctly set the parent question id.
          Hide
          Jamie Pratt added a comment -

          Here is the commit that adds the export and import of child questions to the combined question type (that depends on this improvement) :

          https://github.com/jamiepratt/moodle-qtype_combined/commit/f89163369959b4ef2613a9c0b56c099514527c6e

          Show
          Jamie Pratt added a comment - Here is the commit that adds the export and import of child questions to the combined question type (that depends on this improvement) : https://github.com/jamiepratt/moodle-qtype_combined/commit/f89163369959b4ef2613a9c0b56c099514527c6e
          Hide
          Tim Hunt added a comment -

          This sounds good in principle, Jamie, but I did not have time to review the code in detail today. I will try to look soon.

          Show
          Tim Hunt added a comment - This sounds good in principle, Jamie, but I did not have time to review the code in detail today. I will try to look soon.
          Hide
          Jamie Pratt added a comment -

          Thanks Tim. I look forward to your comments.

          Show
          Jamie Pratt added a comment - Thanks Tim. I look forward to your comments.
          Hide
          Tim Hunt added a comment -

          Most of this re-factor looks good. Certainly, the general principle looks right.

          However, something that has always sucked really badly is the duplicated code between question_type::save_question and qformat_default::importprocess. This refactor is an opportunity to deal with that, and you have not yet done so.

          It is a good thing that you have moved the duplication closer together. That makes it more glaring, and so more likely to get fixed.

          But it is a very bad thing that in order to do this you have had to add a new public question_type::save_imported_question method. Before we know what has happened, someone will have overridden that, and we won't be able to change it or remove it easily.

          I think I will have a play, to see if I can sort out the mess now.

          Show
          Tim Hunt added a comment - Most of this re-factor looks good. Certainly, the general principle looks right. However, something that has always sucked really badly is the duplicated code between question_type::save_question and qformat_default::importprocess. This refactor is an opportunity to deal with that, and you have not yet done so. It is a good thing that you have moved the duplication closer together. That makes it more glaring, and so more likely to get fixed. But it is a very bad thing that in order to do this you have had to add a new public question_type::save_imported_question method. Before we know what has happened, someone will have overridden that, and we won't be able to change it or remove it easily. I think I will have a play, to see if I can sort out the mess now.
          Hide
          Tim Hunt added a comment -

          OK, so I have fiddled around with the code, but I have not tested this yet:

          https://github.com/timhunt/moodle/compare/master...MDL-39230

          To make any sense of that, you realy have to look at the two commits separately.

          If everything still works, we would need one more commit to delete the save_imported_question function, and change format.php to call save_question.

          However, as I say, I have not tested. What needs to be tested:

          1. Create a question of each type, including files in the question text and general feedback.
          2. Edit those newly created questions and save.
          3. For GIFT and Moodle XML, export those questions, and re-import. Ensure that the copies are exact.
          4. For all import format, import all the available test files.

          Even so, there is a question, should this go into 2.5 or 2.6. Clearly, 2.5 is past the stage where we should be making changes like this.

          On the other hand, we want to be able to deploy qtype_combined on top of Moodle 2.5 at the OU in the second half this this year, so it would be really good for us if this got into 2.5.

          Show
          Tim Hunt added a comment - OK, so I have fiddled around with the code, but I have not tested this yet: https://github.com/timhunt/moodle/compare/master...MDL-39230 To make any sense of that, you realy have to look at the two commits separately. https://github.com/timhunt/moodle/commit/05bd5188b2796c1b87a4912ae9f1f374f82037f0 - just renames $fromimport to match the names used in save_question. Well, to do that, I have to make two copies. https://github.com/timhunt/moodle/commit/daf73ead640b1a600a59f72b49442a28d9d391d7 - this makes the implementation of save_question and save_imported_question itentical, in a way that should not break either. (I copied the two functions into two panes in a compare editor, and edited them until they were identical.) If everything still works, we would need one more commit to delete the save_imported_question function, and change format.php to call save_question. However, as I say, I have not tested. What needs to be tested: Create a question of each type, including files in the question text and general feedback. Edit those newly created questions and save. For GIFT and Moodle XML, export those questions, and re-import. Ensure that the copies are exact. For all import format, import all the available test files. Even so, there is a question, should this go into 2.5 or 2.6. Clearly, 2.5 is past the stage where we should be making changes like this. On the other hand, we want to be able to deploy qtype_combined on top of Moodle 2.5 at the OU in the second half this this year, so it would be really good for us if this got into 2.5.
          Hide
          Tim Hunt added a comment -

          Just to make it clear what I think should happen next: I am hoping for comments from Jamie and Jean-Michel (and anyone else who has something to contribute) about whether this seems like a good idea or not, but don't worry if you have nothing to say.

          Show
          Tim Hunt added a comment - Just to make it clear what I think should happen next: I am hoping for comments from Jamie and Jean-Michel (and anyone else who has something to contribute) about whether this seems like a good idea or not, but don't worry if you have nothing to say.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim and Jamie,
          Genaral idea of the refactor seems very good. I am somewhat worried about the idea of rushing this into 2.5, even if I quite understand the need of being able to deploy combined qtype, because I am am fearing some regressions with such a big change, but as it's you that will have to deal with those regressions, after all the decision is up to you (and integrators )
          just some minor points

          • there are 4 calls to property_exists with wrong parameter should be object and property as a string, quite easy to correct
          • I think that a code like

             if (!empty($form->questiontext['text'])) {
                        $question->questiontext = trim($form->questiontext['text']);
                    } else if (!empty($form->questiontext)) {
                        $question->questiontext = trim($form->questiontext);
                    } else {
                        $question->questiontext = '';
                    }
            

            will fails if $form->questiontext is an array and if $form->questiontext['text'] is empty string because php will not like if trim is called on an array on fourth line.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim and Jamie, Genaral idea of the refactor seems very good. I am somewhat worried about the idea of rushing this into 2.5, even if I quite understand the need of being able to deploy combined qtype, because I am am fearing some regressions with such a big change, but as it's you that will have to deal with those regressions, after all the decision is up to you (and integrators ) just some minor points there are 4 calls to property_exists with wrong parameter should be object and property as a string, quite easy to correct I think that a code like if (!empty($form->questiontext['text'])) { $question->questiontext = trim($form->questiontext['text']); } else if (!empty($form->questiontext)) { $question->questiontext = trim($form->questiontext); } else { $question->questiontext = ''; } will fails if $form->questiontext is an array and if $form->questiontext ['text'] is empty string because php will not like if trim is called on an array on fourth line.
          Hide
          Jean-Michel Vedrine added a comment -

          It also seems to me that code like

          if (!empty($form->questiontext['itemid'])) {
                      $questiontextitemid = $form->questiontext['itemid'];
                  } else if (!empty($form->questiontextitemid)) {
                      $questiontextitemid = $form->questiontextitemid;
                  } else {
                      $questiontextitemid = 0;
                  } 
          

          will not always works when $form->questiontext is a string but not quite sure as I don't clearly see how the index will be interpreted in the string considered as an array ?

          Show
          Jean-Michel Vedrine added a comment - It also seems to me that code like if (!empty($form->questiontext['itemid'])) { $questiontextitemid = $form->questiontext['itemid']; } else if (!empty($form->questiontextitemid)) { $questiontextitemid = $form->questiontextitemid; } else { $questiontextitemid = 0; } will not always works when $form->questiontext is a string but not quite sure as I don't clearly see how the index will be interpreted in the string considered as an array ?
          Hide
          Jean-Michel Vedrine added a comment -

          Bingo!
          if $form->questiontext is a string $form->questiontext['itemid'] is a character so is not empty.

          Show
          Jean-Michel Vedrine added a comment - Bingo! if $form->questiontext is a string $form->questiontext ['itemid'] is a character so is not empty.
          Hide
          Jean-Michel Vedrine added a comment -

          The more I look at these 2 functions, the more I think we should take the occasion to get rid of those questiontextformat, questiontextitemid generalfeeedbackformat generalfeedbackitemid and just use array with 'text', 'format', 'itemid' elements like for all other fields.
          Of course that may seems quite paradoxal from somebody that just said this is already quite a big change to advocate for an even bigger change that will imply to update a lot of places (including a lot of qformats plugins )

          Show
          Jean-Michel Vedrine added a comment - The more I look at these 2 functions, the more I think we should take the occasion to get rid of those questiontextformat, questiontextitemid generalfeeedbackformat generalfeedbackitemid and just use array with 'text', 'format', 'itemid' elements like for all other fields. Of course that may seems quite paradoxal from somebody that just said this is already quite a big change to advocate for an even bigger change that will imply to update a lot of places (including a lot of qformats plugins )
          Hide
          Tim Hunt added a comment -

          Right, I think this is our opportunity to really clean up a lot of historical mess. Therefore, we should do this properly in 2.6.

          That is a minor pain for me, but if the changes get implemented in 2.6, then I can back-port them to 2.5 when the OU moves there.

          Really, the data returned by the import parsers should be exactly the same as the data we get from the editing form. That is, we should make what it says on http://docs.moodle.org/dev/Question_data_structures true, rather than an over-simplication.

          Of course, doing this work, we should install https://github.com/jamiepratt/moodle-qtype_combined, so we can make sure our changes don't break Jamie's qtype.

          Also, this is probably the moment to start writing unit tests for save_question. For any question type, this should be a valid unit test:

          $qtypeobj = question_bank::get_qtype($qtype);
          $fromform = test_question_maker::get_question_form_data($qtype);
          $question = //... something ...;
          $qtypeobj->save_question($question, $fromform);
          $questiondata = question_bank::load_question_data($question->id);
          $this->assertEquals(test_question_maker::get_question_data($qtype), $questiondata);
          

          Actually, we should make a new method
          $question = question_bank::save_question($fromform);
          which does the

          $question = //... something ...;
          $qtypeobj->save_question($question, $fromform);
          

          bit.

          (Note that, with https://github.com/timhunt/moodle-qtype_pmatchreverse/blob/master/tests/questiontype_test.php#L134, I was able to do the import unit test by comparing the imported data with test_question_maker::get_question_form_data($qtype); which I thought was a step in the right direction.

          I really should be working on other things at the moment. so if anyone else fancies taking this on, be my guest. If not, I will do it for 2.6.

          Show
          Tim Hunt added a comment - Right, I think this is our opportunity to really clean up a lot of historical mess. Therefore, we should do this properly in 2.6. That is a minor pain for me, but if the changes get implemented in 2.6, then I can back-port them to 2.5 when the OU moves there. Really, the data returned by the import parsers should be exactly the same as the data we get from the editing form. That is, we should make what it says on http://docs.moodle.org/dev/Question_data_structures true, rather than an over-simplication. Of course, doing this work, we should install https://github.com/jamiepratt/moodle-qtype_combined , so we can make sure our changes don't break Jamie's qtype. Also, this is probably the moment to start writing unit tests for save_question. For any question type, this should be a valid unit test: $qtypeobj = question_bank::get_qtype($qtype); $fromform = test_question_maker::get_question_form_data($qtype); $question = //... something ...; $qtypeobj->save_question($question, $fromform); $questiondata = question_bank::load_question_data($question->id); $this->assertEquals(test_question_maker::get_question_data($qtype), $questiondata); Actually, we should make a new method $question = question_bank::save_question($fromform); which does the $question = //... something ...; $qtypeobj->save_question($question, $fromform); bit. (Note that, with https://github.com/timhunt/moodle-qtype_pmatchreverse/blob/master/tests/questiontype_test.php#L134 , I was able to do the import unit test by comparing the imported data with test_question_maker::get_question_form_data($qtype); which I thought was a step in the right direction. I really should be working on other things at the moment. so if anyone else fancies taking this on, be my guest. If not, I will do it for 2.6.
          Hide
          Jamie Pratt added a comment -

          I would agree that eliminating that duplication of code between the question import and form data saving code would be a very good thing, as well as the other clean up and testing of the save_question code.

          I would like to volunteer to tackle this but it seems like quite a big spring clean involving also the need for extensive unit testing.

          Do you think we could move the question import saving code into the question type base with a big 'warning deprecated' sign or similar saying not to override this method in a child class and that it is a temporary measure that will be removed in Moodle 2.6?? It would not be pretty but it might tide us over in the mean time.

          Show
          Jamie Pratt added a comment - I would agree that eliminating that duplication of code between the question import and form data saving code would be a very good thing, as well as the other clean up and testing of the save_question code. I would like to volunteer to tackle this but it seems like quite a big spring clean involving also the need for extensive unit testing. Do you think we could move the question import saving code into the question type base with a big 'warning deprecated' sign or similar saying not to override this method in a child class and that it is a temporary measure that will be removed in Moodle 2.6?? It would not be pretty but it might tide us over in the mean time.
          Hide
          Joseph Rézeau added a comment -

          Hi Jamie, Tim, Jean-Michel,
          Just accidentally came across this issue. What is the " combined question type"? it is a re-vamp of the Cloze/Embedded question type?

          Is the current issue linked to MDL-6371?

          Joseph

          Show
          Joseph Rézeau added a comment - Hi Jamie, Tim, Jean-Michel, Just accidentally came across this issue. What is the " combined question type"? it is a re-vamp of the Cloze/Embedded question type? Is the current issue linked to MDL-6371 ? Joseph
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Hello Joseph,
          combined is the name of a new question type developed by Jamie, commissioned by the OU.
          Tim has given the link to its specification here : https://moodle.org/mod/forum/discuss.php?d=217579 (don't you remember you pointed out it should accept to embed other qtype's questions ?)
          Jamie's repo is here https://github.com/jamiepratt/moodle-qtype_combined and it's very interesting to follow his progress
          With combined it will be possible to embed regexp subquestions. For that we need to make regep "combinable" and I have already started to write some code (adapted for what Jamie has done for pmatch) totally untested by lack of time, if you want to test it, I can send it to you if you can install combined on some Moodle site (locally or not).

          Show
          Jean-Michel Vedrine added a comment - - edited Hello Joseph, combined is the name of a new question type developed by Jamie, commissioned by the OU. Tim has given the link to its specification here : https://moodle.org/mod/forum/discuss.php?d=217579 (don't you remember you pointed out it should accept to embed other qtype's questions ?) Jamie's repo is here https://github.com/jamiepratt/moodle-qtype_combined and it's very interesting to follow his progress With combined it will be possible to embed regexp subquestions. For that we need to make regep "combinable" and I have already started to write some code (adapted for what Jamie has done for pmatch) totally untested by lack of time, if you want to test it, I can send it to you if you can install combined on some Moodle site (locally or not).
          Hide
          Tim Hunt added a comment -

          OK, so I will do this properly for 2.6.

          Jamie, for now complete qtype_combined on the assumption that your proposed core change was integrated. (Why not include the patch in the qtype folder?)

          I will deal with sorting out that mess in the OU codebase in the short term.

          I will also make any necessary changes so that qtype_combined works out-of-the-box in Moodle 2.6.

          It is a pitly that qtype_combined won't just work out-of-the-box on a Moodle 2.5 system, but I can't see a way around that.

          Show
          Tim Hunt added a comment - OK, so I will do this properly for 2.6. Jamie, for now complete qtype_combined on the assumption that your proposed core change was integrated. (Why not include the patch in the qtype folder?) I will deal with sorting out that mess in the OU codebase in the short term. I will also make any necessary changes so that qtype_combined works out-of-the-box in Moodle 2.6. It is a pitly that qtype_combined won't just work out-of-the-box on a Moodle 2.5 system, but I can't see a way around that.
          Hide
          Joseph Rézeau added a comment -

          Hi Jean-Michel,
          Sorry about my misunderstanding. Of course now I see that "combined" is the new qtype meant to replace Cloze in the future. I am certainly willing to test both Jamie's code and yours (on my moodle local site). Do email me about it.
          Joseph

          Show
          Joseph Rézeau added a comment - Hi Jean-Michel, Sorry about my misunderstanding. Of course now I see that "combined" is the new qtype meant to replace Cloze in the future. I am certainly willing to test both Jamie's code and yours (on my moodle local site). Do email me about it. Joseph
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          Just an idea,
          If we look at the part of Jamiee's patch that change question/format/xml/format.php (just that file) I think that:

          • the introduction of import_questions and import_questions is a nice refactoring of the code that make xml import process more modular and more structured
          • I don't see any risk of regression
          • I don't see any backward compatibility problem

          Could we consider the possibility of introducing just this change for Moodle 2.5 ? Or if integrators don't agree for Moodle 2.5.1 ?
          Of course it would not solve completely the problem of import/export for combined qtype but if I read correctly Jamie's commit https://github.com/jamiepratt/moodle-qtype_combined/commit/f89163369959b4ef2613a9c0b56c099514527c6e the only thing missing would be the save_imported_question function that could be as you said distributed as a patch (only a single file to patch).
          I don't know if my idea is really good as it doesn't really solve the problem so it's questionable if this change would have any real benefit ?

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, Just an idea, If we look at the part of Jamiee's patch that change question/format/xml/format.php (just that file) I think that: the introduction of import_questions and import_questions is a nice refactoring of the code that make xml import process more modular and more structured I don't see any risk of regression I don't see any backward compatibility problem Could we consider the possibility of introducing just this change for Moodle 2.5 ? Or if integrators don't agree for Moodle 2.5.1 ? Of course it would not solve completely the problem of import/export for combined qtype but if I read correctly Jamie's commit https://github.com/jamiepratt/moodle-qtype_combined/commit/f89163369959b4ef2613a9c0b56c099514527c6e the only thing missing would be the save_imported_question function that could be as you said distributed as a patch (only a single file to patch). I don't know if my idea is really good as it doesn't really solve the problem so it's questionable if this change would have any real benefit ?
          Hide
          Tim Hunt added a comment -

          Well, the alternative for making combined work in 2.5 is for combined to write a method there which converts the data that is currently sent to save_imported_question, so that instead it can call save_question.

          Show
          Tim Hunt added a comment - Well, the alternative for making combined work in 2.5 is for combined to write a method there which converts the data that is currently sent to save_imported_question, so that instead it can call save_question.
          Hide
          Jean-Michel Vedrine added a comment -

          Yes, that would work, but combined still needs the changes in question/format/xml/format.php so that import_from_xml is able to call import_questions or do you think of another way ?

          Show
          Jean-Michel Vedrine added a comment - Yes, that would work, but combined still needs the changes in question/format/xml/format.php so that import_from_xml is able to call import_questions or do you think of another way ?
          Hide
          Tim Hunt added a comment -

          Yes, which supports your idea that we should do some of this in 2.5, or at least 2.5.1. I think you have convinced me. However, now I am going to bed, and I am way this weekend.

          Show
          Tim Hunt added a comment - Yes, which supports your idea that we should do some of this in 2.5, or at least 2.5.1. I think you have convinced me. However, now I am going to bed, and I am way this weekend.
          Hide
          Joseph Rézeau added a comment -

          Hi all,

          Just decided to install Jamie's Combined question type on my local moodle 2.4 test site... and finding it quite difficult to get it to work.

          1.- Installed https://github.com/jamiepratt/moodle-qtype_combined (master branch)
          Installation goes OK, no warnings. The only requirement is Moodle 2012062504.

          2.- But... when I try to create a Combined question in my question bank, I get error messages:

          Warning: require_once(moodle/question/type/pmatch/pmatchlib.php): failed to open stream: No such file or directory in moodle\question\type\combined\combinable\pmatch\combinable.php on line 29

          Fatal error: require_once(): Failed opening required 'moodle/question/type/pmatch/pmatchlib.php' (include_path='moodle/lib/zend;moodle/lib/pear;.;M:\server\php\PEAR') in moodle\question\type\combined\combinable\pmatch\combinable.php on line 29

          This "requirement" is in contradiction with what the Combined readme states:

          It is not required to install these contrib question types but if you have the following contrib question types installed then
          you can use these as sub questions :

          It seems that pmatch IS required, and if it is, then it should be made a requirement in the Combined qtype installation, no?

          3.- OK, then, I install [pmatch](https://github.com/moodleou/moodle-qtype_pmatch/)
          Installation goes OK

          4.- I try again to create a Combined question in my question bank... and I get error messages:

          Warning: require_once(moodle/question/type/varnumericset/number_interpreter.php): failed to open stream: No such file or directory in moodle\question\type\combined\combinable\varnumeric\combinable.php on line 29

          Fatal error: require_once(): Failed opening required 'moodle/question/type/varnumericset/number_interpreter.php' (include_path='moodle/lib/zend;moodle/lib/pear;.;M:\server\php\PEAR') in moodle\question\type\combined\combinable\varnumeric\combinable.php on line 29

          OK, since I do not need numeric questions for my tests, I just delete the moodle\question\type\combined\combinable\varnumeric folder.

          5.- At last I can create a Combined question, well, almost...
          The question text if pre-filled with mysterious code, which is not explained anywhere. The online help does not help much. Where is the documentation?

          Looking forward to help and DOCUMENTATION, please!

          6.- As far as I understand, in its current state of development, the Combined question type is only meant to include the OU "special" q types. I was expecting to include most of the legacy moodle question types: short answer, multichoice, match, ...

          Joseph

          Show
          Joseph Rézeau added a comment - Hi all, Just decided to install Jamie's Combined question type on my local moodle 2.4 test site... and finding it quite difficult to get it to work. 1.- Installed https://github.com/jamiepratt/moodle-qtype_combined (master branch) Installation goes OK, no warnings. The only requirement is Moodle 2012062504. 2.- But... when I try to create a Combined question in my question bank, I get error messages: Warning: require_once(moodle/question/type/pmatch/pmatchlib.php): failed to open stream: No such file or directory in moodle\question\type\combined\combinable\pmatch\combinable.php on line 29 Fatal error: require_once(): Failed opening required 'moodle/question/type/pmatch/pmatchlib.php' (include_path='moodle/lib/zend;moodle/lib/pear;.;M:\server\php\PEAR') in moodle\question\type\combined\combinable\pmatch\combinable.php on line 29 This "requirement" is in contradiction with what the Combined readme states: It is not required to install these contrib question types but if you have the following contrib question types installed then you can use these as sub questions : [pmatch] ( https://github.com/moodleou/moodle-qtype_pmatch/ ) [gapselect] ( https://github.com/moodleou/moodle-qtype_gapselect/ ) [oumultiresponse] ( https://github.com/moodleou/moodle-qtype_oumultiresponse/ ) It seems that pmatch IS required, and if it is, then it should be made a requirement in the Combined qtype installation, no? 3.- OK, then, I install [pmatch] ( https://github.com/moodleou/moodle-qtype_pmatch/ ) Installation goes OK 4.- I try again to create a Combined question in my question bank... and I get error messages: Warning: require_once(moodle/question/type/varnumericset/number_interpreter.php): failed to open stream: No such file or directory in moodle\question\type\combined\combinable\varnumeric\combinable.php on line 29 Fatal error: require_once(): Failed opening required 'moodle/question/type/varnumericset/number_interpreter.php' (include_path='moodle/lib/zend;moodle/lib/pear;.;M:\server\php\PEAR') in moodle\question\type\combined\combinable\varnumeric\combinable.php on line 29 OK, since I do not need numeric questions for my tests, I just delete the moodle\question\type\combined\combinable\varnumeric folder. 5.- At last I can create a Combined question, well, almost... The question text if pre-filled with mysterious code, which is not explained anywhere. The online help does not help much. Where is the documentation? Looking forward to help and DOCUMENTATION, please! 6.- As far as I understand, in its current state of development, the Combined question type is only meant to include the OU "special" q types. I was expecting to include most of the legacy moodle question types: short answer, multichoice, match, ... Joseph
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Hello Joseph,
          As combined's development is paid by the OU we can expect it to be tailored first to the OU needs isn't it ?
          As it is not yet released, nor finished, maybe it's a little too early to ask for online help or documentation
          Neither pmatch nor varnumeric or other qtypes are really required but as you are using a development version (still unfinished) you are supposed to suppress yourself the question/type/combined/combinable subfolders for qtypes you don't have installed and also the default question text will need a little fiddling. I didn't had this problem as I had all the OU qtypes installed but I had to update varnumeric to an unreleased version (see Jamie's github repo) because the one in the OU repo was too old.
          The choice of the subquestions types used in this first version was explained in the combined qtype specification (see link in one of my previous messages) if you read this again you will see reasons for choosing pmatch vs shortanswer, varnumeric vs numerical and oumltiresponse vs multichoice
          Again these choices are understandable given this is a paid development for the OU.
          You seems to be looking at combined as if the first goal was to replace multianswer but to my best knowledge nobody said that !
          If I remember well, Tim said that there is no place for both multianswer and combined in core, no more.
          To be able to switch from multianswer to combined we would need quite a lot more things:

          • all the question types supported by cloze would have to be supported by combined (IMHO this is not difficult)
          • migration form existing cloze questions to combined (a lot more difficult)
          • solve the problem of restore and import when multianswer questions are present (tricky)
          • ...

          I think we are far away from that situation !
          But the important thing for me is that a lot of defaults I see in multianswer (quite understandable given it has been built many years ago) are solved from the beginning in combined

          • there is no more any non accessible tooltips for feedback
          • it is expendable to embed other qtypes
          • it is a lot easier to write combined questions
          • ...

          So this is why I am closely following combined development, but for the moment only as a Moodle developer, not as a Moodle user

          Last thing, to understand the "mysterious code" that is in the question text, just read the combined requirement document starting from page 4.

          Show
          Jean-Michel Vedrine added a comment - - edited Hello Joseph, As combined's development is paid by the OU we can expect it to be tailored first to the OU needs isn't it ? As it is not yet released, nor finished, maybe it's a little too early to ask for online help or documentation Neither pmatch nor varnumeric or other qtypes are really required but as you are using a development version (still unfinished) you are supposed to suppress yourself the question/type/combined/combinable subfolders for qtypes you don't have installed and also the default question text will need a little fiddling. I didn't had this problem as I had all the OU qtypes installed but I had to update varnumeric to an unreleased version (see Jamie's github repo) because the one in the OU repo was too old. The choice of the subquestions types used in this first version was explained in the combined qtype specification (see link in one of my previous messages) if you read this again you will see reasons for choosing pmatch vs shortanswer, varnumeric vs numerical and oumltiresponse vs multichoice Again these choices are understandable given this is a paid development for the OU. You seems to be looking at combined as if the first goal was to replace multianswer but to my best knowledge nobody said that ! If I remember well, Tim said that there is no place for both multianswer and combined in core, no more. To be able to switch from multianswer to combined we would need quite a lot more things: all the question types supported by cloze would have to be supported by combined (IMHO this is not difficult) migration form existing cloze questions to combined (a lot more difficult) solve the problem of restore and import when multianswer questions are present (tricky) ... I think we are far away from that situation ! But the important thing for me is that a lot of defaults I see in multianswer (quite understandable given it has been built many years ago) are solved from the beginning in combined there is no more any non accessible tooltips for feedback it is expendable to embed other qtypes it is a lot easier to write combined questions ... So this is why I am closely following combined development, but for the moment only as a Moodle developer, not as a Moodle user Last thing, to understand the "mysterious code" that is in the question text, just read the combined requirement document starting from page 4.
          Hide
          Joseph Rézeau added a comment -

          @Jean-Michel,
          Many thanks for your prompt answer to my questions!

          You seems to be looking at combined as if the first goal was to replace multianswer [...]
          If I remember well, Tim said that there is no place for both multianswer and combined in core, no more.

          Yes, that is exactly what I was looking for, and I am disappointed that it is not the case.

          Last thing, to understand the "mysterious code" that is in the question text, just read the combined requirement document starting from page 4.

          Er, where do I find that "document"?
          Joseph

          Show
          Joseph Rézeau added a comment - @Jean-Michel, Many thanks for your prompt answer to my questions! You seems to be looking at combined as if the first goal was to replace multianswer [...] If I remember well, Tim said that there is no place for both multianswer and combined in core, no more. Yes, that is exactly what I was looking for, and I am disappointed that it is not the case. Last thing, to understand the "mysterious code" that is in the question text, just read the combined requirement document starting from page 4. Er, where do I find that "document"? Joseph
          Hide
          Jamie Pratt added a comment -

          @Joseph here is the specification document https://docs.google.com/open?id=0B_W3H1TnOxK-RXQ0SUR3OGo4ZkE

          As originally mentioned in https://moodle.org/mod/forum/discuss.php?d=217579

          Glad to here you are following my progress with the combined qtype Jean-Michel! And thanks for your informative reply to Joseph's questions.

          Would be very happy to discuss the development of the question type further, I think it would be better to do that in the Quiz forum though. Maybe in the discussion above.

          Show
          Jamie Pratt added a comment - @Joseph here is the specification document https://docs.google.com/open?id=0B_W3H1TnOxK-RXQ0SUR3OGo4ZkE As originally mentioned in https://moodle.org/mod/forum/discuss.php?d=217579 Glad to here you are following my progress with the combined qtype Jean-Michel! And thanks for your informative reply to Joseph's questions. Would be very happy to discuss the development of the question type further, I think it would be better to do that in the Quiz forum though. Maybe in the discussion above.
          Hide
          Jamie Pratt added a comment -

          Thanks for that idea on accepting part of my proposed change to core Jean-Michel and then Tim's solution for "combined to write a method there which converts the data that is currently sent to save_imported_question, so that instead it can call save_question" would be pretty straight forward and a good way of having combined work out of the box in 2.5.

          Show
          Jamie Pratt added a comment - Thanks for that idea on accepting part of my proposed change to core Jean-Michel and then Tim's solution for "combined to write a method there which converts the data that is currently sent to save_imported_question, so that instead it can call save_question" would be pretty straight forward and a good way of having combined work out of the box in 2.5.
          Hide
          Joseph Rézeau added a comment -

          @Jamie, thanks for pointing to the Combined qtype specification document, which I am perusing right now. I will probably come back with more questions...

          I do not think that the moodle.org Quiz forum is the best place for discussing the development of this question type, at least whilst it is very much in the development stage, as it may confuse non-developer moodle users.

          Why not continue discussion in this tracker issue, or consider creating a new issue specifically for discussion?

          Joseph

          Show
          Joseph Rézeau added a comment - @Jamie, thanks for pointing to the Combined qtype specification document, which I am perusing right now. I will probably come back with more questions... I do not think that the moodle.org Quiz forum is the best place for discussing the development of this question type, at least whilst it is very much in the development stage, as it may confuse non-developer moodle users. Why not continue discussion in this tracker issue, or consider creating a new issue specifically for discussion? Joseph
          Hide
          Joseph Rézeau added a comment -

          From the Combined qtype specification document, pmatch section:

          "For open ended response we are only expecting to match correct responses and as such the ability to match and comment on specific incorrect responses is not supported."

          Too bad! Specific feedback comments on specific correct & incorrect student answers is a must have feature of any open ended question type (pmatch, regexp, preg). This is a regression from the current Cloze/multianswer core question type. I hope Jamie can come up with a solution to this crucial point before development is ended, otherwise I see no point of porting my own REGEXP question type to Combined.

          Show
          Joseph Rézeau added a comment - From the Combined qtype specification document, pmatch section: "For open ended response we are only expecting to match correct responses and as such the ability to match and comment on specific incorrect responses is not supported ." Too bad! Specific feedback comments on specific correct & incorrect student answers is a must have feature of any open ended question type (pmatch, regexp, preg). This is a regression from the current Cloze/multianswer core question type. I hope Jamie can come up with a solution to this crucial point before development is ended, otherwise I see no point of porting my own REGEXP question type to Combined.
          Hide
          Jamie Pratt added a comment - - edited

          Hi Joseph,

          I really think the proper place to discuss would be in the Quiz forum. The topic in the Quiz forum that Tim started and where he asked for feedback on the question type would be ideal : https://moodle.org/mod/forum/discuss.php?d=217579

          I feel your question is a little off topic here but is an interesting one and is the kind of feedback that would be a useful contribution to the discussion topic I mention.

          Show
          Jamie Pratt added a comment - - edited Hi Joseph, I really think the proper place to discuss would be in the Quiz forum. The topic in the Quiz forum that Tim started and where he asked for feedback on the question type would be ideal : https://moodle.org/mod/forum/discuss.php?d=217579 I feel your question is a little off topic here but is an interesting one and is the kind of feedback that would be a useful contribution to the discussion topic I mention.
          Hide
          Joseph Rézeau added a comment -

          OK, Jamie, I'll post my comments on Combined qtype to that Quiz forum discussion.

          Show
          Joseph Rézeau added a comment - OK, Jamie, I'll post my comments on Combined qtype to that Quiz forum discussion.
          Hide
          Jamie Pratt added a comment -

          Thanks Joseph. I hope you don't me saying that, it is just that if I start replying to you here, then I am also going off topic. I believe having our conversations in predictable places we ensure that people who want to participate are included and that when someone is looking through the archive they can find what they are looking for more easily.

          Show
          Jamie Pratt added a comment - Thanks Joseph. I hope you don't me saying that, it is just that if I start replying to you here, then I am also going off topic. I believe having our conversations in predictable places we ensure that people who want to participate are included and that when someone is looking through the archive they can find what they are looking for more easily.
          Hide
          Jamie Pratt added a comment -

          I have separated out just the import xml question refactoring as proposed by Jean-Michel.

          Show
          Jamie Pratt added a comment - I have separated out just the import xml question refactoring as proposed by Jean-Michel.
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks Jamie, I was just about to do that
          Tim, I will rebase MDL-39404 once this change is integrated.

          Show
          Jean-Michel Vedrine added a comment - Thanks Jamie, I was just about to do that Tim, I will rebase MDL-39404 once this change is integrated.
          Hide
          Jean-Michel Vedrine added a comment -

          you patch seems to be missing a final end of line.
          Have you tested that all sample files import is OK ? (We should also ask to do that during testing after integration)

          Show
          Jean-Michel Vedrine added a comment - you patch seems to be missing a final end of line. Have you tested that all sample files import is OK ? (We should also ask to do that during testing after integration)
          Hide
          Jamie Pratt added a comment -

          If we can get this refactor in core for 2.5 then I can simply move the save_imported_question method onto the qtype_combined class and have it call the various save_question_options of the correct qtype from there.

          It seems more sensible to do that since code transforming the data that would be used to call save_imported_question in order to call save_question would be discarded when we get to 2.6 anyway. If the method is within the qtype_combined type then there is no danger that it is going to be overridden by other question type developers.

          See the commit here : https://github.com/jamiepratt/moodle-qtype_combined/compare/wip_MDL-39230_2_5

          Show
          Jamie Pratt added a comment - If we can get this refactor in core for 2.5 then I can simply move the save_imported_question method onto the qtype_combined class and have it call the various save_question_options of the correct qtype from there. It seems more sensible to do that since code transforming the data that would be used to call save_imported_question in order to call save_question would be discarded when we get to 2.6 anyway. If the method is within the qtype_combined type then there is no danger that it is going to be overridden by other question type developers. See the commit here : https://github.com/jamiepratt/moodle-qtype_combined/compare/wip_MDL-39230_2_5
          Hide
          Jamie Pratt added a comment -

          Hi,

          The commit was missing an EOL at the end of the file. I just force pushed a branch with an amended commit.

          Jamie

          Show
          Jamie Pratt added a comment - Hi, The commit was missing an EOL at the end of the file. I just force pushed a branch with an amended commit. Jamie
          Hide
          Jamie Pratt added a comment -

          I tested all the import files in question/format/tests/fixtures/ and they all import fine, files and all.

          Show
          Jamie Pratt added a comment - I tested all the import files in question/format/tests/fixtures/ and they all import fine, files and all.
          Hide
          Jean-Michel Vedrine added a comment -

          Good

          Show
          Jean-Michel Vedrine added a comment - Good
          Hide
          Jean-Michel Vedrine added a comment -

          +1 for me.
          Now Tim, if you agree it's up to you to decide if this should go into 2.5 (and to persuade integrators) or wait for 2.5.1

          Show
          Jean-Michel Vedrine added a comment - +1 for me. Now Tim, if you agree it's up to you to decide if this should go into 2.5 (and to persuade integrators) or wait for 2.5.1
          Hide
          Tim Hunt added a comment -

          This is a good change, and I really hope we can get it in to 2.5

          Because of indent changes, you may find
          https://github.com/jamiepratt/moodle/compare/master...wip_MDL_39230_2_5?w=1
          and easier compare URL to review.

          Show
          Tim Hunt added a comment - This is a good change, and I really hope we can get it in to 2.5 Because of indent changes, you may find https://github.com/jamiepratt/moodle/compare/master...wip_MDL_39230_2_5?w=1 and easier compare URL to review.
          Hide
          Jean-Michel Vedrine added a comment -

          Yes I hope we can get this into 2.5 too.
          Fingers crossed

          Show
          Jean-Michel Vedrine added a comment - Yes I hope we can get this into 2.5 too. Fingers crossed
          Hide
          Damyon Wiese added a comment -

          I checked the code very carefully and agree that it is functionally the same as before except the public function is callable, so this change seems safe and I can see it would help upgrade paths for your custom question.

          I have integrated it to master - I had to amend the commit message from your branch - please check this in future when submitting for integration.

          Show
          Damyon Wiese added a comment - I checked the code very carefully and agree that it is functionally the same as before except the public function is callable, so this change seems safe and I can see it would help upgrade paths for your custom question. I have integrated it to master - I had to amend the commit message from your branch - please check this in future when submitting for integration.
          Hide
          Jamie Pratt added a comment -

          Thanks Damyon! Sorry about the commit message.

          Show
          Jamie Pratt added a comment - Thanks Damyon! Sorry about the commit message.
          Hide
          Tim Hunt added a comment -

          Doh! Sorry. I should have caught that bad commit message. Thanks for fixing it.

          Show
          Tim Hunt added a comment - Doh! Sorry. I should have caught that bad commit message. Thanks for fixing it.
          Hide
          Rajesh Taneja added a comment -

          Thanks Tim,

          I tried importing and exporting various questions in different formats and all seems to be working fine.

          Show
          Rajesh Taneja added a comment - Thanks Tim, I tried importing and exporting various questions in different formats and all seems to be working fine.
          Hide
          Dan Poltawski added a comment -

          Thanks! You're changes are now spread to the world through this git and our source control repositories.

          No time to rest though, we've got days to make 2.5 the best yet!

          ciao

          Show
          Dan Poltawski added a comment - Thanks! You're changes are now spread to the world through this git and our source control repositories. No time to rest though, we've got days to make 2.5 the best yet! ciao

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: