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
    • Rank:
      49838

      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.

        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: