Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-27414

Upgrade the randomsamatch question type to the new question engine

    Details

    • Testing Instructions:
      Hide

      Installation and upgrade

      1. Test upgrade 2.5.4 -> latest MOODLE_25_STABLE -> master
      2. Test upgrade 2.6.1 -> latest MOODLE_26_STABLE -> master
      3. Test upgrade last week's master -> master.
      4. Test new install on each branch. (I believe that re-initialising PHP unit does this, and the CI server has done that already.)

      Question creation, editing and preview

      1. Create some shortanswer questions in various categories (you need a category with a subcategory to be able to test that shortanswer questions from subcategories are correctly picked).
      2. Then create some randomsamatch questions in the same categories.
      3. Re-edit the questions and verify that everything was saved and re-displayed accurately.
      4. Preview the questions using the preview icon in the question bank, and verify that they work properly.

      Things to test:

      • shortanswers questions with images in question text
      • that the checkbox to include or not shortanswers questions in subcategories works
      • shortanswers questions where there are several correct answers (the first one should be used) and also where the correct answer is not the first one
      • that you can't choose more shortanswers questions than available when creating the randomsamatch question
      • that if you remove some shortanswers questions after the randomsamatch question is created, trying to attempt it throw an error.
      • that if 2 shortanswer questions with the same correct answer are chosen in an attempt to a randomsamatch question, this choice is only displayed once in the dropdown menus and it works.

      Using the questions in a quiz

      1. Make a quiz using some of all of your test questions.
      2. Attempt the quiz as a student.
      3. Review the attempt as a student, and as a teacher. Verify that everything is displayed and graded correctly.

      Import / Export

      The only import export format available for randomsamatch questions is Moodle XML and this is the first version it is possible to export/import randomsamatch questions (it was not available in Moodle 1.9/2.0).

      1. Try to export some categories with randomsamatch questions.
      2. Import them into another course.
      3. Verify that the questions were restored accurately. (E.g. open the editing form.)

      Backup and Restore using the new code.

      After having done all the other steps you should have some randomsamatch questions in your question bank, and some quizes using them with some attempts.

      1. Backup your test course, with user data.
      2. Restore it as a new course, or adding to another course.
      3. Verify all the data was transferred accurately.

      Restore of 1.9 backups

      1. Make some quizzes with randomsamatch in a course in a Moodle 1.9 install.
      2. Backup that course.
      3. Restore it. Ensure the question definitions have come across accurately. (It is not possible to restore user data from 1.9.)

      Restore of 2.0 backups

      1. A first test is to try to import the backup file attached to MDL-44053 and verify that the randomsamatch question in the question bank is working, that the "One Quiz" activity which uses it is working and that the attempt on that activity is OK and can be reviewed.
      2. If you can be bothered create some new data in a 2.0 site, including user data, then backup and restore that into a site running the new code..
      Show
      Installation and upgrade Test upgrade 2.5.4 -> latest MOODLE_25_STABLE -> master Test upgrade 2.6.1 -> latest MOODLE_26_STABLE -> master Test upgrade last week's master -> master. Test new install on each branch. (I believe that re-initialising PHP unit does this, and the CI server has done that already.) Question creation, editing and preview Create some shortanswer questions in various categories (you need a category with a subcategory to be able to test that shortanswer questions from subcategories are correctly picked). Then create some randomsamatch questions in the same categories. Re-edit the questions and verify that everything was saved and re-displayed accurately. Preview the questions using the preview icon in the question bank, and verify that they work properly. Things to test: shortanswers questions with images in question text that the checkbox to include or not shortanswers questions in subcategories works shortanswers questions where there are several correct answers (the first one should be used) and also where the correct answer is not the first one that you can't choose more shortanswers questions than available when creating the randomsamatch question that if you remove some shortanswers questions after the randomsamatch question is created, trying to attempt it throw an error. that if 2 shortanswer questions with the same correct answer are chosen in an attempt to a randomsamatch question, this choice is only displayed once in the dropdown menus and it works. Using the questions in a quiz Make a quiz using some of all of your test questions. Attempt the quiz as a student. Review the attempt as a student, and as a teacher. Verify that everything is displayed and graded correctly. Import / Export The only import export format available for randomsamatch questions is Moodle XML and this is the first version it is possible to export/import randomsamatch questions (it was not available in Moodle 1.9/2.0). Try to export some categories with randomsamatch questions. Import them into another course. Verify that the questions were restored accurately. (E.g. open the editing form.) Backup and Restore using the new code. After having done all the other steps you should have some randomsamatch questions in your question bank, and some quizes using them with some attempts. Backup your test course, with user data. Restore it as a new course, or adding to another course. Verify all the data was transferred accurately. Restore of 1.9 backups Make some quizzes with randomsamatch in a course in a Moodle 1.9 install. Backup that course. Restore it. Ensure the question definitions have come across accurately. (It is not possible to restore user data from 1.9.) Restore of 2.0 backups A first test is to try to import the backup file attached to MDL-44053 and verify that the randomsamatch question in the question bank is working, that the "One Quiz" activity which uses it is working and that the attempt on that activity is OK and can be reviewed. If you can be bothered create some new data in a 2.0 site, including user data, then backup and restore that into a site running the new code..
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull Master Branch:

      Description

      randomsamatch question type does not work with 2.1 question engine.

      Attempting a quiz that contains a randomsamatch question results in error:
      Coding error detected, it must be fixed by a programmer: Unknown question type (no definition) randomsamatch

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            timhunt Tim Hunt added a comment -

            We have decided, almost certainly, not to convert this question type to Moodle 2.1. We need to:

            1. move the code to contrib.
            2. add suitable warnings before the upgrade.

            Show
            timhunt Tim Hunt added a comment - We have decided, almost certainly, not to convert this question type to Moodle 2.1. We need to: 1. move the code to contrib. 2. add suitable warnings before the upgrade.
            Hide
            timhunt Tim Hunt added a comment -

            An alternative is to upgrade the qtype, but in a way that does not work 100% in Moodle 2.1. It would have the known issue that two different randomsamatch qtypes in the same quiz could pick the same shortnaswer question, but in every other sense (e.g. reviewing old quiz attempts) it would work.

            I think we should probably do that, but also disable this qtype by default so new questions cannot easily be created, and also mark it deprecated.

            Show
            timhunt Tim Hunt added a comment - An alternative is to upgrade the qtype, but in a way that does not work 100% in Moodle 2.1. It would have the known issue that two different randomsamatch qtypes in the same quiz could pick the same shortnaswer question, but in every other sense (e.g. reviewing old quiz attempts) it would work. I think we should probably do that, but also disable this qtype by default so new questions cannot easily be created, and also mark it deprecated.
            Hide
            timhunt Tim Hunt added a comment -

            Check Moodle XML import and export.

            Show
            timhunt Tim Hunt added a comment - Check Moodle XML import and export.
            Hide
            timhunt Tim Hunt added a comment -

            Also check restoring a backup containging that question type.

            Show
            timhunt Tim Hunt added a comment - Also check restoring a backup containging that question type.
            Hide
            timhunt Tim Hunt added a comment -

            This will have to wait until after the 2.1 release now. I will add a note to the release notes page.

            Show
            timhunt Tim Hunt added a comment - This will have to wait until after the 2.1 release now. I will add a note to the release notes page.
            Hide
            cfollin Chris Follin added a comment -

            Hi, Tim. Is there any update on this issue now that 2.1 was released? We ran into this problem while testing 2.1 and unfortunately it causes errors when upgrading to 2.1 or restoring a 2.0 course in 2.1.

            Show
            cfollin Chris Follin added a comment - Hi, Tim. Is there any update on this issue now that 2.1 was released? We ran into this problem while testing 2.1 and unfortunately it causes errors when upgrading to 2.1 or restoring a 2.0 course in 2.1.
            Hide
            timhunt Tim Hunt added a comment -

            Sorry, no.It is still somewhere on my todo list, but so is a lot of other stuff.

            Show
            timhunt Tim Hunt added a comment - Sorry, no.It is still somewhere on my todo list, but so is a lot of other stuff.
            Hide
            rezeau Joseph Rézeau added a comment -

            Tim wrote on 07/Jun/11 1:38 AM

            An alternative is to upgrade the qtype, but in a way that does not work 100% in Moodle 2.1. It would have the known issue that two different randomsamatch qtypes in the same quiz could pick the same shortnaswer question, but in every other sense (e.g. reviewing old quiz attempts) it would work.
            I think we should probably do that, but also disable this qtype by default so new questions cannot easily be created, and also mark it deprecated.

            I think it's a pity that that question type should no longer be available in 2.1 and further version of Moodle. Used in conjunction with my Export Glossary to Quiz block it would be very useful for study/ revision / self-study of Glossaries, for example.

            If the only issue is the one mentioned above vis. two different randomsamatch qtypes in the same quiz could pick the same shortnaswer question, I do not see this as a problem at all, in the use case of self-testing.

            If it can be made to work, I do not see why it should be disabled by default and marked deprecated; that would really be a regression.

            Joseph

            PS.- I am quite willing to help on making randomsamatch work for moodle 2.1 and am awaiting for instructions.

            Show
            rezeau Joseph Rézeau added a comment - Tim wrote on 07/Jun/11 1:38 AM An alternative is to upgrade the qtype, but in a way that does not work 100% in Moodle 2.1. It would have the known issue that two different randomsamatch qtypes in the same quiz could pick the same shortnaswer question, but in every other sense (e.g. reviewing old quiz attempts) it would work. I think we should probably do that, but also disable this qtype by default so new questions cannot easily be created, and also mark it deprecated. I think it's a pity that that question type should no longer be available in 2.1 and further version of Moodle. Used in conjunction with my Export Glossary to Quiz block it would be very useful for study/ revision / self-study of Glossaries, for example. If the only issue is the one mentioned above vis. two different randomsamatch qtypes in the same quiz could pick the same shortnaswer question , I do not see this as a problem at all, in the use case of self-testing. If it can be made to work, I do not see why it should be disabled by default and marked deprecated; that would really be a regression. Joseph PS.- I am quite willing to help on making randomsamatch work for moodle 2.1 and am awaiting for instructions.
            Hide
            timhunt Tim Hunt added a comment -

            My own view is that something like your Export Glossary to Quiz block is a much better way to go. That is one of the reasons I want to make a new type of 'question bank' plugin. That would be more appropriate than a block for this. See http://docs.moodle.org/dev/Future_question_bank/sharing/versioning_requirements#Question_bank_should_be_made_up_of_separate_plugins. The plugin could automatically generate short-answer or matching questions directly from the glossary, into a category, then you could add random questions from that category to your quiz.

            However, we do need to sort out randomsamatch. The basic outline of what needs to be done is:

            1. Upgrade questiontype.php and the editing form, just like you would have to for any question type being upgraded to 2.1.

            2. Make question.php by doing class qtype_randomsamatch_definition extends qtype_match_definition. I think the only methods you need to implement are start_attempt and apply_attempt_state. This is the hard bit. You should look at calculated qtype, which has to randomly load datasets. That has simiar issues.

            3. Make renderer.php by doing qtype_randomsamatch_renderer extends qtype_match_renderer. This should require almost no code, except the kind of thing you just did in 2.0 to make files work - so you might have to refactor qtype_match_renderer slighly to make that possible.

            4. We really need unit tests to prove this works. Not how calculated uses mock objects to avoid needing to touch the DB during tests.

            5. Then there is upgrade from 2.0 to implement. This is nasty, but it should be possible to take the kind of test-driven approach used elsewhere, so it is not too bad.

            6. Then there is backup/restore to implement. The bad bits are re-coding references to other questions in the attempt data on restore.

            7. And restoring 2.0 attempt backups. That should mostly just work if you have done 5. and 6., but you may need some extra work to cope if one of the questions used by the randomsamatch question is not included in the backup for some reason.

            That is the basic overview. You don't have to do it all, but any of it that you can do would help.

            Show
            timhunt Tim Hunt added a comment - My own view is that something like your Export Glossary to Quiz block is a much better way to go. That is one of the reasons I want to make a new type of 'question bank' plugin. That would be more appropriate than a block for this. See http://docs.moodle.org/dev/Future_question_bank/sharing/versioning_requirements#Question_bank_should_be_made_up_of_separate_plugins . The plugin could automatically generate short-answer or matching questions directly from the glossary, into a category, then you could add random questions from that category to your quiz. However, we do need to sort out randomsamatch. The basic outline of what needs to be done is: 1. Upgrade questiontype.php and the editing form, just like you would have to for any question type being upgraded to 2.1. 2. Make question.php by doing class qtype_randomsamatch_definition extends qtype_match_definition. I think the only methods you need to implement are start_attempt and apply_attempt_state. This is the hard bit. You should look at calculated qtype, which has to randomly load datasets. That has simiar issues. 3. Make renderer.php by doing qtype_randomsamatch_renderer extends qtype_match_renderer. This should require almost no code, except the kind of thing you just did in 2.0 to make files work - so you might have to refactor qtype_match_renderer slighly to make that possible. 4. We really need unit tests to prove this works. Not how calculated uses mock objects to avoid needing to touch the DB during tests. 5. Then there is upgrade from 2.0 to implement. This is nasty, but it should be possible to take the kind of test-driven approach used elsewhere, so it is not too bad. 6. Then there is backup/restore to implement. The bad bits are re-coding references to other questions in the attempt data on restore. 7. And restoring 2.0 attempt backups. That should mostly just work if you have done 5. and 6., but you may need some extra work to cope if one of the questions used by the randomsamatch question is not included in the backup for some reason. That is the basic overview. You don't have to do it all, but any of it that you can do would help.
            Hide
            nebgor Aparup Banerjee added a comment -

            where are we with this issue? i'm raising priority here, its really not minor to me now. now i've to restore my backup in a 2.0 install, remove the question type, and then backup it up & restore into 2.1/2.2

            Show
            nebgor Aparup Banerjee added a comment - where are we with this issue? i'm raising priority here, its really not minor to me now. now i've to restore my backup in a 2.0 install, remove the question type, and then backup it up & restore into 2.1/2.2
            Hide
            timhunt Tim Hunt added a comment -

            We are in exactly the same place we were last time anyone asked. There have been too many other things for me to do that are more important. This is still on my todo list, but I can't predict when it will get to the top.

            Show
            timhunt Tim Hunt added a comment - We are in exactly the same place we were last time anyone asked. There have been too many other things for me to do that are more important. This is still on my todo list, but I can't predict when it will get to the top.
            Hide
            bazaar Mawuli Kuivi added a comment -

            Hello Tim,
            Any news one this. Need to know what needs done exactly if l have to attempt it myself. Please.

            If you have anything, please anything, please attach it here for me.

            mk

            Show
            bazaar Mawuli Kuivi added a comment - Hello Tim, Any news one this. Need to know what needs done exactly if l have to attempt it myself. Please. If you have anything, please anything, please attach it here for me. mk
            Hide
            emerrill Eric Merrill added a comment -

            As an FYI - I've taken up development of this. I will keep status posted here as I progress.

            Show
            emerrill Eric Merrill added a comment - As an FYI - I've taken up development of this. I will keep status posted here as I progress.
            Hide
            rezeau Joseph Rézeau added a comment -

            @Eric,
            Great! Do keep us posted.

            Show
            rezeau Joseph Rézeau added a comment - @Eric, Great! Do keep us posted.
            Hide
            churchjw Jeff Church added a comment -

            Any word on how this is going?

            Show
            churchjw Jeff Church added a comment - Any word on how this is going?
            Hide
            rezeau Joseph Rézeau added a comment -

            Jeff "Any word on how this is going?"
            +1

            Show
            rezeau Joseph Rézeau added a comment - Jeff "Any word on how this is going?" +1
            Hide
            didier.jodin@free.fr Didier Jodin added a comment -

            Same for me : +1 (at least !)
            I really miss this type of question,
            and I think many other teachers do.

            Show
            didier.jodin@free.fr Didier Jodin added a comment - Same for me : +1 (at least !) I really miss this type of question, and I think many other teachers do.
            Hide
            v.ashruf Virgil Ashruf added a comment -

            I have an upgraded environment which has been upgraded with clean codebases all through 2.0.1 to 2.4.1. I have this error as well.

            +1 vote

            Show
            v.ashruf Virgil Ashruf added a comment - I have an upgraded environment which has been upgraded with clean codebases all through 2.0.1 to 2.4.1. I have this error as well. +1 vote
            Hide
            stuart Peter Ruthven-Stuart added a comment - - edited

            Hello, is this randomsamatch question type (Random short-answer matching) dead and buried?

            We've just updated a 1.9 system to 2.4 and have been burned by the apparent demise of this question type. I was aware that the randomsamatch was on the in-danger-of-extinction list back in 2011 because of a post Tim H wrote in 2011:

            https://moodle.org/mod/forum/discuss.php?d=176097

            However, I naively assumed that given the number of people expressing support for this question type there would be no way that it would be discontinued, or at least some workaround would be devised. Also, the fact that when I upgraded another system from 2 to 2.1. to 2.2 etc. there were no warnings to alert me that this question type had been killed off. Finally, the randomsamatch question type exists in 2.4, which further strongly suggested to me that it was still viable. Why is the question type still included if it doesn't work?

            Oh well, live and learn. Fortunately, I am going to be able to re-engineer my 15,000+ SA question into MC questions. Other people may not be so lucky.

            Show
            stuart Peter Ruthven-Stuart added a comment - - edited Hello, is this randomsamatch question type (Random short-answer matching) dead and buried? We've just updated a 1.9 system to 2.4 and have been burned by the apparent demise of this question type. I was aware that the randomsamatch was on the in-danger-of-extinction list back in 2011 because of a post Tim H wrote in 2011: https://moodle.org/mod/forum/discuss.php?d=176097 However, I naively assumed that given the number of people expressing support for this question type there would be no way that it would be discontinued, or at least some workaround would be devised. Also, the fact that when I upgraded another system from 2 to 2.1. to 2.2 etc. there were no warnings to alert me that this question type had been killed off. Finally, the randomsamatch question type exists in 2.4, which further strongly suggested to me that it was still viable. Why is the question type still included if it doesn't work? Oh well, live and learn. Fortunately, I am going to be able to re-engineer my 15,000+ SA question into MC questions. Other people may not be so lucky.
            Hide
            didier.jodin didier jodin added a comment -

            Hi,

            I'm one of those not-so-lucky teachers,
            missing what was a great feature of Moodle 1.9

            I'm not a pragrammer, therefore I can't understand why it seems so difficult to get randomsamatch question type available in M 2.x
            If it's technically impossible, OK, then let's say so.
            But please don't say it was not useful, for it really was...

            Show
            didier.jodin didier jodin added a comment - Hi, I'm one of those not-so-lucky teachers, missing what was a great feature of Moodle 1.9 I'm not a pragrammer, therefore I can't understand why it seems so difficult to get randomsamatch question type available in M 2.x If it's technically impossible, OK, then let's say so. But please don't say it was not useful, for it really was...
            Hide
            timhunt Tim Hunt added a comment -

            Yes, this should be fixed.

            It is not technically impossible, but it is quite difficult and time-consuming.

            Supposing I had that much time to work on Moodle, I am not convinced that fixing this is the best use of my time. My guess is that I would achieve more of value with that time if I worked on other bugs. On the other hand, we are up to 31 votes here. Anyway, I don't have that much spare time for the foreseeable future. If someone else (like Eric) wants to volunteer to have a go, then I will try to help.

            So, it summary, it should be fixed, but who knows when anyone will have time?

            Show
            timhunt Tim Hunt added a comment - Yes, this should be fixed. It is not technically impossible, but it is quite difficult and time-consuming. Supposing I had that much time to work on Moodle, I am not convinced that fixing this is the best use of my time. My guess is that I would achieve more of value with that time if I worked on other bugs. On the other hand, we are up to 31 votes here. Anyway, I don't have that much spare time for the foreseeable future. If someone else (like Eric) wants to volunteer to have a go, then I will try to help. So, it summary, it should be fixed, but who knows when anyone will have time?
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello,
            I don't known when I will be able to have some more time to work on it, but on the branch https://github.com/jmvedrine/moodle/commits/MDL-27414 I have done some work on this.
            But please note that for the moment I have only done th super easy bits, so the real work has not yet started.
            Consider this as just "the initial push" ...
            If Eric or other people want to collaborate on this, maybe we can get it done ?
            As Tim said I think he has more important and more useful things to do so if other people like me can take care of this or only part of this I think this is how open software work (or should work).

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello, I don't known when I will be able to have some more time to work on it, but on the branch https://github.com/jmvedrine/moodle/commits/MDL-27414 I have done some work on this. But please note that for the moment I have only done th super easy bits, so the real work has not yet started. Consider this as just "the initial push" ... If Eric or other people want to collaborate on this, maybe we can get it done ? As Tim said I think he has more important and more useful things to do so if other people like me can take care of this or only part of this I think this is how open software work (or should work).
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            For people interested to help, IMHO the next thing to do now is to upgrade question/type/randomsamatch/questiontype.php (you see, we are only at Tim's #1 step )
            For that you can have a lot at other question types or also at Jamie's Pratt template https://github.com/jamiepratt/moodle-qtype_TEMPLATE/blob/master/questiontype.php

            Show
            jmvedrine Jean-Michel Vedrine added a comment - For people interested to help, IMHO the next thing to do now is to upgrade question/type/randomsamatch/questiontype.php (you see, we are only at Tim's #1 step ) For that you can have a lot at other question types or also at Jamie's Pratt template https://github.com/jamiepratt/moodle-qtype_TEMPLATE/blob/master/questiontype.php
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Except for get_possible_responses questiontype.php is mostly done, but

            • Maybe my idea of caching just the questionid of the available shortanswer questions in qtype_randomsamatch (using get_question_bank::get_finder()->get_questions_from_categories that return an array questionid => questionid) is not the right one: this works well for random qtype because later we only choose one of the available questions (so we will only need to load the chosen one) but for randomsamatch qtype we will choose several of those available questions ?
            • Also I made some refactoring of the match renderer to be able to format the stem text, but I don't know if this will work when the short answer question is from a subcategory of the randomsamatch question's category ? Will see that later ... as I am unable to test this now!
            • I really need to work on start_attempt now, but if I know roughly what to do, the problem is that I don't clearly see how to do it the right way.

            My usual method in that case is to pause and think about it until it is clear in my mind . To be continued...

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Except for get_possible_responses questiontype.php is mostly done, but Maybe my idea of caching just the questionid of the available shortanswer questions in qtype_randomsamatch (using get_question_bank::get_finder()->get_questions_from_categories that return an array questionid => questionid) is not the right one: this works well for random qtype because later we only choose one of the available questions (so we will only need to load the chosen one) but for randomsamatch qtype we will choose several of those available questions ? Also I made some refactoring of the match renderer to be able to format the stem text, but I don't know if this will work when the short answer question is from a subcategory of the randomsamatch question's category ? Will see that later ... as I am unable to test this now! I really need to work on start_attempt now, but if I know roughly what to do, the problem is that I don't clearly see how to do it the right way. My usual method in that case is to pause and think about it until it is clear in my mind . To be continued...
            Hide
            timhunt Tim Hunt added a comment -

            My thinking was that in start_attempt, we should copy the full text of each choice and answer into question data. That way, later in the attempt, we don't need to load multiple other questions. However, that is just a best guess. I am not writing the code.

            (For comparison, calculated does something similar. It stores the actual value of all the variables, it does not just store the dataset id. I think.)

            I could not think of any reliable way to make sure that the same short-answer question was not selected by two different randomsamatch questions. I think we will just have to live with that as the price for getting this qtype to work at all.

            Show
            timhunt Tim Hunt added a comment - My thinking was that in start_attempt, we should copy the full text of each choice and answer into question data. That way, later in the attempt, we don't need to load multiple other questions. However, that is just a best guess. I am not writing the code. (For comparison, calculated does something similar. It stores the actual value of all the variables, it does not just store the dataset id. I think.) I could not think of any reliable way to make sure that the same short-answer question was not selected by two different randomsamatch questions. I think we will just have to live with that as the price for getting this qtype to work at all.
            Hide
            rezeau Joseph Rézeau added a comment -

            Tim "I could not think of any reliable way to make sure that the same short-answer question was not selected by two different randomsamatch questions. I think we will just have to live with that as the price for getting this qtype to work at all."
            +1

            Show
            rezeau Joseph Rézeau added a comment - Tim "I could not think of any reliable way to make sure that the same short-answer question was not selected by two different randomsamatch questions. I think we will just have to live with that as the price for getting this qtype to work at all." +1
            Hide
            jmvedrine Jean-Michel Vedrine added a comment - - edited

            Hello Tim,
            Thanks for having a look at my work in progress code
            "My thinking was that in start_attempt, we should copy the full text of each choice and answer into question data. That way, later in the attempt, we don't need to load multiple other questions. However, that is just a best guess. I am not writing the code."
            Yes I had this idea too, but I must admit that I don't really know how question definitions are cached by MUC (never looked at that code, just know there is some caching implemented by you and Sam) so I was hoping that later in the attempt re-loading the questions would not have a big cost. do you think this is the case or not ?

            Show
            jmvedrine Jean-Michel Vedrine added a comment - - edited Hello Tim, Thanks for having a look at my work in progress code "My thinking was that in start_attempt, we should copy the full text of each choice and answer into question data. That way, later in the attempt, we don't need to load multiple other questions. However, that is just a best guess. I am not writing the code." Yes I had this idea too, but I must admit that I don't really know how question definitions are cached by MUC (never looked at that code, just know there is some caching implemented by you and Sam) so I was hoping that later in the attempt re-loading the questions would not have a big cost. do you think this is the case or not ?
            Hide
            timhunt Tim Hunt added a comment -

            Yes, MUC should help a lot.

            However, it is only a cache. Rather than just performance, we need to think about this scenario:

            1. Student start randomsamach question that refers to sa question 123.
            2. Teacher edits sa question 123.

            What should happen then?

            Show
            timhunt Tim Hunt added a comment - Yes, MUC should help a lot. However, it is only a cache. Rather than just performance, we need to think about this scenario: 1. Student start randomsamach question that refers to sa question 123. 2. Teacher edits sa question 123. What should happen then?
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Yes,
            Definitely the way it was implemented in Moodle 1.x 2.0 randomsamatch questions were very prone to break. Any editing, suppression of any shortanswer question, either in that category or in any subcategory was to be avoided and there was no warning if a teacher did that.
            Maybe we can take the opportunity of this rewrite to make it more solid ?
            the sa questions could be chosen at the start of an attempt among those present in the category (including or not subcategories depending on the $question->options->subcats value) at that instant and all needed elements (sa questiontext, correct answer) loaded and saved in the attempt data. so later we don't need to look at these sa questions again all data is in the attempt. Is that your idea ?
            If we do it that way your scenario would be

            1. Student A start randomsamach question that refers to sa question 123.
            2. Teacher edits sa question 123.
            3. Nothing is changed for student A even if he continue his attempt after teacher editing
            4. Student B start the same randomsamach question that refers to sa question 123. He get the new version. Same if student A start a new attempt he also get the new version.

            Your comment remind me another thing: when we will write the code to upgrade attempts from the qe1 to qe2, the case of missing sa questions must absolutely be managed in some way

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Yes, Definitely the way it was implemented in Moodle 1.x 2.0 randomsamatch questions were very prone to break. Any editing, suppression of any shortanswer question, either in that category or in any subcategory was to be avoided and there was no warning if a teacher did that. Maybe we can take the opportunity of this rewrite to make it more solid ? the sa questions could be chosen at the start of an attempt among those present in the category (including or not subcategories depending on the $question->options->subcats value) at that instant and all needed elements (sa questiontext, correct answer) loaded and saved in the attempt data. so later we don't need to look at these sa questions again all data is in the attempt. Is that your idea ? If we do it that way your scenario would be Student A start randomsamach question that refers to sa question 123. Teacher edits sa question 123. Nothing is changed for student A even if he continue his attempt after teacher editing Student B start the same randomsamach question that refers to sa question 123. He get the new version. Same if student A start a new attempt he also get the new version. Your comment remind me another thing: when we will write the code to upgrade attempts from the qe1 to qe2, the case of missing sa questions must absolutely be managed in some way
            Hide
            timhunt Tim Hunt added a comment -

            "Is that your idea ?" yes.

            Re upgrade: you are right. In other places in the upgrade, we had to detect missing data, and replace it with things like "question that was deleted." E.g. response_summary method in question/type/match/db/upgradelib.php

            Show
            timhunt Tim Hunt added a comment - "Is that your idea ?" yes. Re upgrade: you are right. In other places in the upgrade, we had to detect missing data, and replace it with things like "question that was deleted." E.g. response_summary method in question/type/match/db/upgradelib.php
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I pushed a commit where sa questions are only loaded at attempt start.
            IMHO this new direction completely suppress "The bad bits are re-coding references to other questions in the attempt data on restore" when implementing backup/restore but what about restoring legacy 2.0 attempts ? I will see after doing upgradelib.
            Now it's really time to write some tests but the bad thing is that tomorrow my holidays are over

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I pushed a commit where sa questions are only loaded at attempt start. IMHO this new direction completely suppress "The bad bits are re-coding references to other questions in the attempt data on restore" when implementing backup/restore but what about restoring legacy 2.0 attempts ? I will see after doing upgradelib. Now it's really time to write some tests but the bad thing is that tomorrow my holidays are over
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I should read more carefully what you write "You should look at calculated qtype, which has to randomly load datasets. That has similar issues" and "Not how calculated uses mock objects to avoid needing to touch the DB during tests" I need some kind of a "saquestionsloader" isn't it ?

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I should read more carefully what you write "You should look at calculated qtype, which has to randomly load datasets. That has similar issues" and "Not how calculated uses mock objects to avoid needing to touch the DB during tests" I need some kind of a "saquestionsloader" isn't it ?
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I have introduced the question loader and adapted some tests from the matching question. This seems to indicate the code is working to produce quite similar results.
            But we definitely need more tests including walkthrough tests.
            Unfortunately holidays are over, I don't know when I will be able to work on this issue again (June ?). But I am quite satisfied with the result so far.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I have introduced the question loader and adapted some tests from the matching question. This seems to indicate the code is working to produce quite similar results. But we definitely need more tests including walkthrough tests. Unfortunately holidays are over, I don't know when I will be able to work on this issue again (June ?). But I am quite satisfied with the result so far.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I think that it's time to assign this issue to myself

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I think that it's time to assign this issue to myself
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Note so that I don't forget.
            Verification that there are enough shortanswer questions in the category is done at question/creation, but we need another check at attempt start. I don't know if the 2.0 solution (convert the question to a description showing the problem) is the good one ?

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Note so that I don't forget. Verification that there are enough shortanswer questions in the category is done at question/creation, but we need another check at attempt start. I don't know if the 2.0 solution (convert the question to a description showing the problem) is the good one ?
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Tim,
            I have rebased and would be glad if you could have a first look at the actual state of this. Of course there are still a lot of things to do:

            So if you could find some time to look at this it would help me a lot.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Tim, I have rebased and would be glad if you could have a first look at the actual state of this. Of course there are still a lot of things to do: write qtype_randomsamatch::get_possible_responses (I really don't know what to do here) increase robustness of upgrade from 1.9/2.0 attempts when there are missing data what to do when there is not enough shortanswer questions available at attempt's start all that you can notice (everybody know Tim's reviews and I saw https://tracker.moodle.org/browse/MDL-31226?focusedCommentId=236882&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-236882 too ) So if you could find some time to look at this it would help me a lot.
            Hide
            timhunt Tim Hunt added a comment -

            Wow! this is amazing.

            (To avoid any doubt, I mean amazingly good )

            1. get_possible_responses - Well, a simple option is to make can_analyse_responses return false. Then, this can be fixed in the future if anyone cares enough, but it does not need to hold up this work.

            2. Good luck

            3. The quiz throws an exception if there are not enough random questions. I think you can throw an exception too.

            4. Details:

            a. Typo: availabe https://github.com/jmvedrine/moodle/compare/master...MDL-27414#L11R42

            b. Unnecessary extra space: https://github.com/jmvedrine/moodle/compare/master...MDL-27414#L13R105

            c. Unnecessary blank lines: https://github.com/jmvedrine/moodle/compare/master...MDL-27414#L17R38

            d. https://github.com/jmvedrine/moodle/compare/master...MDL-27414#L10R48 $answer->fraction != 1.0 - Don't do this - it is floating-point comparison. You should use question_state::graded_state_for_fraction to convert floats to states. qtype_multichoice_multi_question::get_num_parts_right is an example.

            e. https://github.com/jmvedrine/moodle/compare/master...MDL-27414#L10R48 this whole method is quite tricky. You could probably make it easier to understand by breaking it into smaller functions, for example $answer = $this->find_right_answer($wrappedquestion) to get rid of the nested loops.

            f. Whitespace: https://github.com/jmvedrine/moodle/compare/master...MDL-27414#L10R76

            5. it would be useful to have some testing instructions already. For example, when looking at https://github.com/jmvedrine/moodle/compare/master...MDL-27414#L12R36, I think "I wonder if there is anything about shortanswer questions containing images in the testing instructions?"

            Show
            timhunt Tim Hunt added a comment - Wow! this is amazing. (To avoid any doubt, I mean amazingly good ) 1. get_possible_responses - Well, a simple option is to make can_analyse_responses return false. Then, this can be fixed in the future if anyone cares enough, but it does not need to hold up this work. 2. Good luck 3. The quiz throws an exception if there are not enough random questions. I think you can throw an exception too. 4. Details: a. Typo: availabe https://github.com/jmvedrine/moodle/compare/master...MDL-27414#L11R42 b. Unnecessary extra space: https://github.com/jmvedrine/moodle/compare/master...MDL-27414#L13R105 c. Unnecessary blank lines: https://github.com/jmvedrine/moodle/compare/master...MDL-27414#L17R38 d. https://github.com/jmvedrine/moodle/compare/master...MDL-27414#L10R48 $answer->fraction != 1.0 - Don't do this - it is floating-point comparison. You should use question_state::graded_state_for_fraction to convert floats to states. qtype_multichoice_multi_question::get_num_parts_right is an example. e. https://github.com/jmvedrine/moodle/compare/master...MDL-27414#L10R48 this whole method is quite tricky. You could probably make it easier to understand by breaking it into smaller functions, for example $answer = $this->find_right_answer($wrappedquestion) to get rid of the nested loops. f. Whitespace: https://github.com/jmvedrine/moodle/compare/master...MDL-27414#L10R76 5. it would be useful to have some testing instructions already. For example, when looking at https://github.com/jmvedrine/moodle/compare/master...MDL-27414#L12R36 , I think "I wonder if there is anything about shortanswer questions containing images in the testing instructions?"
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Tim, thanks a lot for the compliment, it was quite interesting to do.

            1. done
            2. understood
            3. done
            4. done
            5. thanks for pushing me to make this clearer, this is in fact a tricky bit because we can pick the same correct answer for several shortanswer questions and we don't wan any duplicates in choices.

            Testing instructions will not be an easy task because I made no note of the (numerous) tests I did, for instances I remember tesing shortanswer questions containing images

            • freshly created
            • included in a randomsamatch q created in Moodle 1.9 and upgraded
            • included in a randomsamatch q in an 1.9 backup
            • included in a randomsamatch q in a 2.0 backup

            Then only after I realized that testing images during upgrade or 1.9/2.0 backups was not really necessary because shortanswer question text was no saved as part of randomsamatch in 1.9/2.0 so I was in fact only testing shortanswer not randomsamatch, and now I am saving it in qt_var with @@PLUGINFILE@@ so the only test to do is to test my renderer, no more.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Tim, thanks a lot for the compliment, it was quite interesting to do. done understood done done thanks for pushing me to make this clearer, this is in fact a tricky bit because we can pick the same correct answer for several shortanswer questions and we don't wan any duplicates in choices. Testing instructions will not be an easy task because I made no note of the (numerous) tests I did, for instances I remember tesing shortanswer questions containing images freshly created included in a randomsamatch q created in Moodle 1.9 and upgraded included in a randomsamatch q in an 1.9 backup included in a randomsamatch q in a 2.0 backup Then only after I realized that testing images during upgrade or 1.9/2.0 backups was not really necessary because shortanswer question text was no saved as part of randomsamatch in 1.9/2.0 so I was in fact only testing shortanswer not randomsamatch, and now I am saving it in qt_var with @@PLUGINFILE@@ so the only test to do is to test my renderer, no more.
            Hide
            didier.jodin didier jodin added a comment -

            Congratulations Jean-Michel, and many thanks for that work.
            Many of us were waiting for this feature to come back !

            Show
            didier.jodin didier jodin added a comment - Congratulations Jean-Michel, and many thanks for that work. Many of us were waiting for this feature to come back !
            Hide
            rezeau Joseph Rézeau added a comment -

            Hi Jean-Michel,
            Just started conducting some tests of randmsamatch on my local moodle master (2.6dev) site. When viewing questions in preview window, I sometimes get this error message:
            Before calling format_text(), the content must be processed with file_rewrite_pluginfile_urls()

            line 1222 of \lib\weblib.php: call to debugging()
            line 336 of \question\type\questionbase.php: call to format_text()
            line 92 of \question\type\match\question.php: call to question_definition->html_to_text()
            line 267 of \question\behaviour\behaviourbase.php: call to qtype_match_question->get_question_summary()
            line 902 of \question\engine\questionattempt.php: call to question_behaviour->get_question_summary()
            line 466 of \question\engine\questionusage.php: call to question_attempt->start()
            line 117 of \question\preview.php: call to question_usage_by_activity->start_question()

            Show
            rezeau Joseph Rézeau added a comment - Hi Jean-Michel, Just started conducting some tests of randmsamatch on my local moodle master (2.6dev) site. When viewing questions in preview window, I sometimes get this error message: Before calling format_text(), the content must be processed with file_rewrite_pluginfile_urls() line 1222 of \lib\weblib.php: call to debugging() line 336 of \question\type\questionbase.php: call to format_text() line 92 of \question\type\match\question.php: call to question_definition->html_to_text() line 267 of \question\behaviour\behaviourbase.php: call to qtype_match_question->get_question_summary() line 902 of \question\engine\questionattempt.php: call to question_behaviour->get_question_summary() line 466 of \question\engine\questionusage.php: call to question_attempt->start() line 117 of \question\preview.php: call to question_usage_by_activity->start_question()
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Joseph,
            Thanks for reporting this. It will be fixed by MDL-39507 currently waiting for integration review (precisely https://github.com/timhunt/moodle/compare/master...MDL-39507#L7R336 )

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Joseph, Thanks for reporting this. It will be fixed by MDL-39507 currently waiting for integration review (precisely https://github.com/timhunt/moodle/compare/master...MDL-39507#L7R336 )
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Yesterday I found a way to break image management as I have done it:

            • As a teacher create 3 short answer questions including one image in question text
            • create a randomsamatch question with "Number of questions to select" equal to 3
            • include this randomsamatch question in a quiz
            • as a student attempt this quiz
            • as a teacher delete one of the short answer questions
            • either as a teacher or as the student review the student attempt, the image is missing.

            Of course this is somewhat of an edge case and the randomsamatch is no utterly broken as it was in 1.9 because only the image is missing but still a bug.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Yesterday I found a way to break image management as I have done it: As a teacher create 3 short answer questions including one image in question text create a randomsamatch question with "Number of questions to select" equal to 3 include this randomsamatch question in a quiz as a student attempt this quiz as a teacher delete one of the short answer questions either as a teacher or as the student review the student attempt, the image is missing. Of course this is somewhat of an edge case and the randomsamatch is no utterly broken as it was in 1.9 because only the image is missing but still a bug.
            Hide
            timhunt Tim Hunt added a comment -

            Option 1, ignore this problem. It is up to the teacher not to screw up like this.

            Option 2, copy the files when you copy the questiontext. They would have to become response files like images in essay questions, but no reason why that would not work. (Note MDL-39980.)

            You could do option 1 for now, and file a separate issue for doing option 2 later.

            Show
            timhunt Tim Hunt added a comment - Option 1, ignore this problem. It is up to the teacher not to screw up like this. Option 2, copy the files when you copy the questiontext. They would have to become response files like images in essay questions, but no reason why that would not work. (Note MDL-39980 .) You could do option 1 for now, and file a separate issue for doing option 2 later.
            Hide
            didier.jodin didier jodin added a comment -

            Jean-Michel,

            I can see that "randomsamatch question type" is not back yet in M2.6.
            Any news about your (very appreciated) work ?

            Didier

            Show
            didier.jodin didier jodin added a comment - Jean-Michel, I can see that "randomsamatch question type" is not back yet in M2.6. Any news about your (very appreciated) work ? Didier
            Hide
            rezeau Joseph Rézeau added a comment -

            +1 with didier jodin

            Show
            rezeau Joseph Rézeau added a comment - +1 with didier jodin
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Didier and Joseph,
            Unfortunately I have not been able to find some time for Moodle coding for weeks
            But the good news are :

            • As this is a bugfix and not a new feature, once it is finished it can go into all stables branchs (currently MOOODLE_25_STABLE and MOODLE_26_STABLE).
            • in fact the actual state of the code is pretty good, I think it is feature complete. Of course I am not saying it is bug free

            The only real thing missing is testing instructions, but this is a big task because this question type has been broken for years so we must test all aspects of questions usage.
            To ease testers job , my plan was first to create some shortanswer questions (in 2 separate question banks categories A and B and a third category C as a subcategory of A) and fill these categories with some questions (my plan was to use questions on animals because many questions in Moodle already use questions on animals for testing).
            Some of these questions must include images in question's text and the first answer with a 100% grade must not always be the first one.
            Then I was planning to provide these questions both as xml files and as a course backup (in 1.9, 2.0 and 2.6 formats to be able to test upgrade) and to write the testing instructions.
            Maybe some Moodle users interested in seeing this question type's come back into Moodle could do part of this work ? For sure it would help me.
            I will try to rebase the code on the current versions.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Didier and Joseph, Unfortunately I have not been able to find some time for Moodle coding for weeks But the good news are : As this is a bugfix and not a new feature, once it is finished it can go into all stables branchs (currently MOOODLE_25_STABLE and MOODLE_26_STABLE). in fact the actual state of the code is pretty good, I think it is feature complete. Of course I am not saying it is bug free The only real thing missing is testing instructions, but this is a big task because this question type has been broken for years so we must test all aspects of questions usage. To ease testers job , my plan was first to create some shortanswer questions (in 2 separate question banks categories A and B and a third category C as a subcategory of A) and fill these categories with some questions (my plan was to use questions on animals because many questions in Moodle already use questions on animals for testing). Some of these questions must include images in question's text and the first answer with a 100% grade must not always be the first one. Then I was planning to provide these questions both as xml files and as a course backup (in 1.9, 2.0 and 2.6 formats to be able to test upgrade) and to write the testing instructions. Maybe some Moodle users interested in seeing this question type's come back into Moodle could do part of this work ? For sure it would help me. I will try to rebase the code on the current versions.
            Hide
            rezeau Joseph Rézeau added a comment -

            Jean-Michel Vedrine"I will try to rebase the code on the current versions."
            Please rebase at least for moodle 2.6 and I will conduct tests.
            Joseph

            Show
            rezeau Joseph Rézeau added a comment - Jean-Michel Vedrine "I will try to rebase the code on the current versions." Please rebase at least for moodle 2.6 and I will conduct tests. Joseph
            Hide
            didier.jodin didier jodin added a comment -

            Same here : I'd be happy to help.

            Show
            didier.jodin didier jodin added a comment - Same here : I'd be happy to help.
            Hide
            tillmad Daniel Tillman added a comment -

            I would love to help - tillmad@mcgill-toolen.org

            I am running the latest 2.5 and have a 2.6 install as well.

            I loved this type because I use them for vocab quizzes.

            Show
            tillmad Daniel Tillman added a comment - I would love to help - tillmad@mcgill-toolen.org I am running the latest 2.5 and have a 2.6 install as well. I loved this type because I use them for vocab quizzes.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            linking with MDL-44053 because this issue has a 2.0 backup attached which contains some randomsamach questions. Will need to test that once MDL-44053 is closed and fixed this file is correctly restored by my code.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - linking with MDL-44053 because this issue has a 2.0 backup attached which contains some randomsamach questions. Will need to test that once MDL-44053 is closed and fixed this file is correctly restored by my code.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment - - edited

            Adding the doc_required label (because current docs say that this question type is no more available so we should modify them when it will be fixed and also document the new features) and qa_test_required label because qa tests have been altered when this question type was broken so we need to make sure it will be tested again when fixed.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - - edited Adding the doc_required label (because current docs say that this question type is no more available so we should modify them when it will be fixed and also document the new features) and qa_test_required label because qa tests have been altered when this question type was broken so we need to make sure it will be tested again when fixed.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I realise maybe I was wrong doing the same thing as in Moodle 1.9 and 2.0. Why are randomsamatch questions not exported/imported by Moodle XML ? I see no valid reason.
            If we decide to add this feature should we do it :

            • as a core question (code in format/xml/format.php)
            • as a question type plugin (code in question/type/randomsamatch/questiontype.php)

            my personal vote would be 2 as if in the future we deprecate this question type and push it to contrib it would sill work, no need to chnage the code

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I realise maybe I was wrong doing the same thing as in Moodle 1.9 and 2.0. Why are randomsamatch questions not exported/imported by Moodle XML ? I see no valid reason. If we decide to add this feature should we do it : as a core question (code in format/xml/format.php) as a question type plugin (code in question/type/randomsamatch/questiontype.php) my personal vote would be 2 as if in the future we deprecate this question type and push it to contrib it would sill work, no need to chnage the code
            Hide
            timhunt Tim Hunt added a comment -

            I a happy for you to do it whichever way you think is best. (I have considered moving all XML import/export code for all core qtypes, but not seriously enough to actually do it.)

            Show
            timhunt Tim Hunt added a comment - I a happy for you to do it whichever way you think is best. (I have considered moving all XML import/export code for all core qtypes, but not seriously enough to actually do it.)
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Pushed the code for Moodle XML import/export to Github. I think it is best if this patch is only changing files in question/type/randomsamatch so this code is in question/type/randomsamatch/questiontype.php

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Pushed the code for Moodle XML import/export to Github. I think it is best if this patch is only changing files in question/type/randomsamatch so this code is in question/type/randomsamatch/questiontype.php
            Hide
            timhunt Tim Hunt added a comment -

            I want to clean up a lot of the old upgrade stuff (MDL-44118), and now this is a blocker.

            It looks like Jean-Michel's code is very close to done, and even if it is not perfect, adding it to Moodle will certainly make things better.

            Therefore, I am starting to think we should integrate Jean-Michel's current work ASAP, into all of 2.5, 2.6 and master, and handle anything else that needs to be done as new separate issues.

            Is that possible?

            Show
            timhunt Tim Hunt added a comment - I want to clean up a lot of the old upgrade stuff ( MDL-44118 ), and now this is a blocker. It looks like Jean-Michel's code is very close to done, and even if it is not perfect, adding it to Moodle will certainly make things better. Therefore, I am starting to think we should integrate Jean-Michel's current work ASAP, into all of 2.5, 2.6 and master, and handle anything else that needs to be done as new separate issues. Is that possible?
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello,
            Yes I think all features are there and working (and nobody could contest it's a lot better than current situation )
            One thing that could stop it to be added immediately to 2.5 and 2.6 (no problem for master) is that I don't know how to handle some table changes in db/upgrade.php so that it works in all cases and on all branches. There is a fragment where I need some help

            if ($oldversion < 2013110506) {
             
                    // Define key question (foreign) to be dropped form qtype_randomsamatch_options.
                    $table = new xmldb_table('qtype_randomsamatch_options');
                    $key = new xmldb_key('question', XMLDB_KEY_FOREIGN, array('question'), 'question', array('id'));
             
                    // Launch drop key question.
                    $dbman->drop_key($table, $key);
             
                    // Record that qtype_randomsamatch savepoint was reached.
                    upgrade_plugin_savepoint(true, 2013110506, 'qtype', 'randomsamatch');
                }
             
                if ($oldversion < 2013110507) {
             
                    // Rename field question on table qtype_randomsamatch_options to questionid.
                    $table = new xmldb_table('qtype_randomsamatch_options');
                    $field = new xmldb_field('question', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, '0', 'id');
             
                    // Launch rename field question.
                    if ($dbman->field_exists($table, $field)) {
                        $dbman->rename_field($table, $field, 'questionid');
                    }
             
                    // Record that qtype_randomsamatch savepoint was reached.
                    upgrade_plugin_savepoint(true, 2013110507, 'qtype', 'randomsamatch');
                }
             
                if ($oldversion < 2013110508) {
             
                    // Define key questionid (foreign-unique) to be added to qtype_randomsamatch_options.
                    $table = new xmldb_table('qtype_randomsamatch_options');
                    $key = new xmldb_key('questionid', XMLDB_KEY_FOREIGN_UNIQUE, array('questionid'), 'question', array('id'));
             
                    // Launch add key questionid.
                    $dbman->add_key($table, $key);
             
                    // Record that qtype_randomsamatch savepoint was reached.
                    upgrade_plugin_savepoint(true, 2013110508, 'qtype', 'randomsamatch');
                }

            You see: the drop_key and add_key should only be done if needed so that the upgrade procedure works even if we are upgrading from a version where changes have already been done.
            I don't know how to do it.
            Another thing that is missing is testing instructions and given my workload in the upcoming weeks I am unlikely to find some time to write them before 6 weeks from now.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello, Yes I think all features are there and working (and nobody could contest it's a lot better than current situation ) One thing that could stop it to be added immediately to 2.5 and 2.6 (no problem for master) is that I don't know how to handle some table changes in db/upgrade.php so that it works in all cases and on all branches. There is a fragment where I need some help if ($oldversion < 2013110506) {   // Define key question (foreign) to be dropped form qtype_randomsamatch_options. $table = new xmldb_table('qtype_randomsamatch_options'); $key = new xmldb_key('question', XMLDB_KEY_FOREIGN, array('question'), 'question', array('id'));   // Launch drop key question. $dbman->drop_key($table, $key);   // Record that qtype_randomsamatch savepoint was reached. upgrade_plugin_savepoint(true, 2013110506, 'qtype', 'randomsamatch'); }   if ($oldversion < 2013110507) {   // Rename field question on table qtype_randomsamatch_options to questionid. $table = new xmldb_table('qtype_randomsamatch_options'); $field = new xmldb_field('question', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, '0', 'id');   // Launch rename field question. if ($dbman->field_exists($table, $field)) { $dbman->rename_field($table, $field, 'questionid'); }   // Record that qtype_randomsamatch savepoint was reached. upgrade_plugin_savepoint(true, 2013110507, 'qtype', 'randomsamatch'); }   if ($oldversion < 2013110508) {   // Define key questionid (foreign-unique) to be added to qtype_randomsamatch_options. $table = new xmldb_table('qtype_randomsamatch_options'); $key = new xmldb_key('questionid', XMLDB_KEY_FOREIGN_UNIQUE, array('questionid'), 'question', array('id'));   // Launch add key questionid. $dbman->add_key($table, $key);   // Record that qtype_randomsamatch savepoint was reached. upgrade_plugin_savepoint(true, 2013110508, 'qtype', 'randomsamatch'); } You see: the drop_key and add_key should only be done if needed so that the upgrade procedure works even if we are upgrading from a version where changes have already been done. I don't know how to do it. Another thing that is missing is testing instructions and given my workload in the upcoming weeks I am unlikely to find some time to write them before 6 weeks from now.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I have rebased on latest weekly but maybe I should squash all commits into one because my progress history on this issue is rather boring and uninteresting

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I have rebased on latest weekly but maybe I should squash all commits into one because my progress history on this issue is rather boring and uninteresting
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I don't know how to have CiBoT results so that I can fix watever it find. I tried to add the cime label but I don't see it in the list of available labels ?

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I don't know how to have CiBoT results so that I can fix watever it find. I tried to add the cime label but I don't see it in the list of available labels ?
            Hide
            emerrill Eric Merrill added a comment -

            cime is correct. Yeah, it would be nice if they added it to the list of available labels. It should work, but may take a while.

            Show
            emerrill Eric Merrill added a comment - cime is correct. Yeah, it would be nice if they added it to the list of available labels. It should work, but may take a while.
            Hide
            timhunt Tim Hunt added a comment -

            The thing about cime is that CiBoT removes it once it has done its thing. So when you come to add it to your issues, it is almost always does not exist on any other issue. Just add it anyway.

            Yes please, squask to 1 comment. (Actually CiBoT will refuse to look at a branch with more than 15 comments, but this one has 14.)

            Because of the $dbman->field_exists check, it is safe to run those upgrade steps multiple times. Therefore you can put the DB chanages on all branches. On the stable branches, just increase version.php number by 1 in the last place for each step. On master use today's date (or the date when you wrote the code).

            Perhpas I should have a go at writing basic testing instructions, since you have done most of the work.

            Show
            timhunt Tim Hunt added a comment - The thing about cime is that CiBoT removes it once it has done its thing. So when you come to add it to your issues, it is almost always does not exist on any other issue. Just add it anyway. Yes please, squask to 1 comment. (Actually CiBoT will refuse to look at a branch with more than 15 comments, but this one has 14.) Because of the $dbman->field_exists check, it is safe to run those upgrade steps multiple times. Therefore you can put the DB chanages on all branches. On the stable branches, just increase version.php number by 1 in the last place for each step. On master use today's date (or the date when you wrote the code). Perhpas I should have a go at writing basic testing instructions, since you have done most of the work.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-27414

            • Remote repository: git://github.com/jmvedrine/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-27414 Remote repository: git://github.com/jmvedrine/moodle.git Remote branch MDL-27414 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1349 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1349/artifact/work/smurf.html
            Hide
            timhunt Tim Hunt added a comment -

            In case you don't know: Moodle coding style says you do not need to add PHPdocs for methods defined in the base class, if you are just overriding them. Therefore, you can, and probably should, ignore a lot of the CiBoT output. (But not all of it.)

            Show
            timhunt Tim Hunt added a comment - In case you don't know: Moodle coding style says you do not need to add PHPdocs for methods defined in the base class, if you are just overriding them. Therefore, you can, and probably should, ignore a lot of the CiBoT output. (But not all of it.)
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Thanks Tim, but fortunately I know that. I tried to fix most of the other issues reported by CiBoT.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Thanks Tim, but fortunately I know that. I tried to fix most of the other issues reported by CiBoT.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I just realised this issue never got a peer review (I think).

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I just realised this issue never got a peer review (I think).
            Hide
            cibot CiBoT added a comment -

            Results for MDL-27414

            • Remote repository: git://github.com/jmvedrine/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-27414 Remote repository: git://github.com/jmvedrine/moodle.git Remote branch MDL-27414 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1350 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1350/artifact/work/smurf.html
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I think CiBoT now report only missing phpdocs

            • for methods already defined in the base class
            • for all the tests (Do I need to add phpdocs here ? I don't really know what to write)
            Show
            jmvedrine Jean-Michel Vedrine added a comment - I think CiBoT now report only missing phpdocs for methods already defined in the base class for all the tests (Do I need to add phpdocs here ? I don't really know what to write)
            Hide
            timhunt Tim Hunt added a comment -

            I did peer review before, but that was a while ago. I will not look at the latest code.

            1. Testing instructions now mostly done, though there is one bit where you might be able to add a bit more details.

            2. CiBoT output now looks OK.

            3. First line of the commit comment is a bit too long. Also, you could write more of a comment. Surely a change this big deserves more of a write up . What might you add?

            • You could say that you have added import/export,
            • and all backup restore is supported.
            • You could add the bit about no longer avoiding repeat use of shortanswer questions.
            • And also say the bit about images in question text not working yet.

            4. We still need to backport to stable branches, which is needs a bit of work due to upgrade and version numbers. If you don't have time to do that, I can do it. Just ask.

            [Y] Syntax
            [Y] Whitespace
            [Y] Output
            [Y] Language
            [Y] Databases
            [y] Testing (instructions and automated tests) - could you add a few more details?
            [Y] Security
            [Y] Documentation
            [N] Git                   - Need to fix the commit comment & backport as above.
            [Y] Third party code
            [Y] Sanity check

            I have to repeat my comment above. This is amazing work. We really need to get it in to Moodle, so people can befit, even if there are minor issues to resolve later.

            Show
            timhunt Tim Hunt added a comment - I did peer review before, but that was a while ago. I will not look at the latest code. 1. Testing instructions now mostly done, though there is one bit where you might be able to add a bit more details. 2. CiBoT output now looks OK. 3. First line of the commit comment is a bit too long. Also, you could write more of a comment. Surely a change this big deserves more of a write up . What might you add? You could say that you have added import/export, and all backup restore is supported. You could add the bit about no longer avoiding repeat use of shortanswer questions. And also say the bit about images in question text not working yet. 4. We still need to backport to stable branches, which is needs a bit of work due to upgrade and version numbers. If you don't have time to do that, I can do it. Just ask. [Y] Syntax [Y] Whitespace [Y] Output [Y] Language [Y] Databases [y] Testing (instructions and automated tests) - could you add a few more details? [Y] Security [Y] Documentation [N] Git - Need to fix the commit comment & backport as above. [Y] Third party code [Y] Sanity check I have to repeat my comment above. This is amazing work. We really need to get it in to Moodle, so people can befit, even if there are minor issues to resolve later.
            Hide
            timhunt Tim Hunt added a comment -

            You don't need PHP doc for unit tests.

            Show
            timhunt Tim Hunt added a comment - You don't need PHP doc for unit tests.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-27414

            • Remote repository: git://github.com/jmvedrine/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-27414 Remote repository: git://github.com/jmvedrine/moodle.git Remote branch MDL-27414 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1351 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1351/artifact/work/smurf.html
            Hide
            cibot CiBoT added a comment -

            Results for MDL-27414

            • Remote repository: git://github.com/jmvedrine/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-27414 Remote repository: git://github.com/jmvedrine/moodle.git Remote branch MDL-27414 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1352 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1352/artifact/work/smurf.html Remote branch MDL-27414 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1353 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1353/artifact/work/smurf.html
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I cherry picked to MOODLE_26_STABLE but this showed my db/upgrade.php file is certainly wrong in master branch because this is in fact the same for both.
            I will try to sort this out tomorrow.
            I also need to do the MOODLE25_STABLE branch.
            But now I will go to bed.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I cherry picked to MOODLE_26_STABLE but this showed my db/upgrade.php file is certainly wrong in master branch because this is in fact the same for both. I will try to sort this out tomorrow. I also need to do the MOODLE25_STABLE branch. But now I will go to bed.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -
            • I think the upgrade scripts and version numbers are now OK (of course integrators will have to look at it on master branch)
            • I have modified the commit comment, please tell me if it is OK.
            • I have re-run phpunit tests on all branchs and that was a good idea: I had a missing 'maxfraction' => 1, in my expected qa in the test_randomsamatch_deferredfeedback_qsession1 function of qtype_randomsamatch_attempt_upgrader_test. This is corrected now and all tests are OK.
            • I tried to start from latest MOODLE_25_STABLE, upgrade to my 25 branch, upgrade to my 26 branch and finally to my master branch. It seems to work, no error, qtype_randomsamatch_options table is correct at each step.

            If Joseph, Eric, Didier, Daniel or other can do some tests it would help a lot.

            I will create a new tracker issue for the problem about images when the shortanswer is deleted after being used in a randomsamatch attempt.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I think the upgrade scripts and version numbers are now OK (of course integrators will have to look at it on master branch) I have modified the commit comment, please tell me if it is OK. I have re-run phpunit tests on all branchs and that was a good idea: I had a missing 'maxfraction' => 1, in my expected qa in the test_randomsamatch_deferredfeedback_qsession1 function of qtype_randomsamatch_attempt_upgrader_test. This is corrected now and all tests are OK. I tried to start from latest MOODLE_25_STABLE, upgrade to my 25 branch, upgrade to my 26 branch and finally to my master branch. It seems to work, no error, qtype_randomsamatch_options table is correct at each step. If Joseph, Eric, Didier, Daniel or other can do some tests it would help a lot. I will create a new tracker issue for the problem about images when the shortanswer is deleted after being used in a randomsamatch attempt.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            To potential testers: please don't test on valuable data, use a copy !

            Show
            jmvedrine Jean-Michel Vedrine added a comment - To potential testers: please don't test on valuable data, use a copy !
            Hide
            didier.jodin didier jodin added a comment -

            Thanks a lot Jean-Michel for your hard work! I'll be happy to test your code this week-end.

            Show
            didier.jodin didier jodin added a comment - Thanks a lot Jean-Michel for your hard work! I'll be happy to test your code this week-end.
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Results for MDL-27414 Remote repository: git://github.com/jmvedrine/moodle.git Remote branch MDL-27414 _25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1373 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1373/artifact/work/smurf.html Remote branch MDL-27414 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1374 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1374/artifact/work/smurf.html Remote branch MDL-27414 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1375 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1375/artifact/work/smurf.html
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Results for MDL-27414 Remote repository: git://github.com/jmvedrine/moodle.git Remote branch MDL-27414 _25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1376 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1376/artifact/work/smurf.html Remote branch MDL-27414 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1377 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1377/artifact/work/smurf.html Remote branch MDL-27414 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1378 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1378/artifact/work/smurf.html
            Hide
            timhunt Tim Hunt added a comment -

            For anyone who has not noticed yet, the new tracker issue that Jean-Michel created is MDL-44162.

            Jean-Michel, did you push the lastest master branch? upgrade.php looks different on master compared to the other branches. Is that right?

            Show
            timhunt Tim Hunt added a comment - For anyone who has not noticed yet, the new tracker issue that Jean-Michel created is MDL-44162 . Jean-Michel, did you push the lastest master branch? upgrade.php looks different on master compared to the other branches. Is that right?
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Yes you are right, after modifying upgrade.php on the stable branchs I forgot to push master branch. Done.
            But upgrade.php still don't look right to me even if it works. I think that in fact I don't fully understand the last part of your comment: "On the stable branches, just increase version.php number by 1 in the last place for each step. On master use today's date (or the date when you wrote the code)."
            I understand it is quite normal the version number of qtype randomsamatch plugin is currently the same in MOODLE_26_STABLE and in master because there was no database change specific to master branch. I also understand it will be incremented just before release, but I fail to see what version number I should use in version.php and also what number I should use for the different step in upgrade.php on master branch.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Yes you are right, after modifying upgrade.php on the stable branchs I forgot to push master branch. Done. But upgrade.php still don't look right to me even if it works. I think that in fact I don't fully understand the last part of your comment: "On the stable branches, just increase version.php number by 1 in the last place for each step. On master use today's date (or the date when you wrote the code)." I understand it is quite normal the version number of qtype randomsamatch plugin is currently the same in MOODLE_26_STABLE and in master because there was no database change specific to master branch. I also understand it will be incremented just before release, but I fail to see what version number I should use in version.php and also what number I should use for the different step in upgrade.php on master branch.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I forgot to ask how I can compare 2 commits in different branchs on Github (I suppose it's easier on Github as locally my branchs are in different git repo ?)

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I forgot to ask how I can compare 2 commits in different branchs on Github (I suppose it's easier on Github as locally my branchs are in different git repo ?)
            Hide
            rezeau Joseph Rézeau added a comment -

            Hi Jean-Michel,
            Just tested branch MDL-27414_26 on my local moodle 2.6 test site.
            I only tested a fresh install, not any update from previous Moodle versions.

            ---------------------------------------------------------------
            1.- Question creation, editing and preview
            Things to test:
            that if you remove some shortanswers questions after the randomsamatch question is created, trying to attempt it throw an error.
            WOULD IT NOT BE BETTER TO throw an error to teacher at time of removing a shortanswer question which is part of a random match ?

            ---------------------------------------------------------------
            2.- Try to export some categories with randomsamatch questions.
            Import them into another course.
            When importing into another course I get this error message:

            Parsing questions from import file.
            Importing 15 questions from file

            1. Question Text here
            At least two shortanswer questions need to be chosen!
            Import error
            More information about this error
            Debug info:
            Error code: cannotimport
            Stack trace:
            line 463 of \lib\setuplib.php: moodle_exception thrown
            line 120 of \question\import.php: call to print_error()
            ---------------------------------------------------------------
            Joseph
            PS Congratulations for your persistence!

            Show
            rezeau Joseph Rézeau added a comment - Hi Jean-Michel, Just tested branch MDL-27414 _26 on my local moodle 2.6 test site. I only tested a fresh install, not any update from previous Moodle versions. --------------------------------------------------------------- 1.- Question creation, editing and preview Things to test: that if you remove some shortanswers questions after the randomsamatch question is created, trying to attempt it throw an error. WOULD IT NOT BE BETTER TO throw an error to teacher at time of removing a shortanswer question which is part of a random match ? --------------------------------------------------------------- 2.- Try to export some categories with randomsamatch questions. Import them into another course. When importing into another course I get this error message: Parsing questions from import file. Importing 15 questions from file 1. Question Text here At least two shortanswer questions need to be chosen! Import error More information about this error Debug info: Error code: cannotimport Stack trace: line 463 of \lib\setuplib.php: moodle_exception thrown line 120 of \question\import.php: call to print_error() --------------------------------------------------------------- Joseph PS Congratulations for your persistence!
            Hide
            timhunt Tim Hunt added a comment -

            True. In this case the same version number in 2.6 and master would work, but it is still recommended to have them different, in case there are future bugs requiring DB changes.

            I don't know about github, but in your local checkout, you can do something like

            git diff MDL-27414_25 MDL-27414 question/type/randomsamatch

            I was about to submit for integration when I saw Joseph's error 2.

            Regarding Joseph's point 1, it might be nice, but in practice it is impossible. It is like MDL-6103, only more so. Anyway, that is not a change to attempt to make here.

            Show
            timhunt Tim Hunt added a comment - True. In this case the same version number in 2.6 and master would work, but it is still recommended to have them different, in case there are future bugs requiring DB changes. I don't know about github, but in your local checkout, you can do something like git diff MDL-27414 _25 MDL-27414 question/type/randomsamatch I was about to submit for integration when I saw Joseph's error 2. Regarding Joseph's point 1, it might be nice, but in practice it is impossible. It is like MDL-6103 , only more so. Anyway, that is not a change to attempt to make here.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Joseph,
            Can you attach your XML file or send it to me ?

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Joseph, Can you attach your XML file or send it to me ?
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Thanks a lot for finding that Joseph,
            In fact I don't need a sample file, I know what is happening, and it's all my fault
            The import/export was working, then I realised that as randomsamatch extra data (choose and subcats) were both integer it was useless to export them with a <text> tag so I modified the export to not use $format->writetext and to export them as <choose>2</choose><subcats>1</subcats> but I forgot to modify import accordingly, and more important I forgot to test my change.
            I will push a fix.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Thanks a lot for finding that Joseph, In fact I don't need a sample file, I know what is happening, and it's all my fault The import/export was working, then I realised that as randomsamatch extra data (choose and subcats) were both integer it was useless to export them with a <text> tag so I modified the export to not use $format->writetext and to export them as <choose>2</choose><subcats>1</subcats> but I forgot to modify import accordingly, and more important I forgot to test my change. I will push a fix.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Fix pushed to all branchs.
            This is only a one line fix:
            in the questiontype.php file in import_from_xml function

            $fromform->$extra = $format->getpath($xml, array('#', $extra, 0, '#', 'text', 0, '#'), '', true);

            should be

            $fromform->$extra = $format->getpath($xml, array('#', $extra, 0, '#'), '', true);

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Fix pushed to all branchs. This is only a one line fix: in the questiontype.php file in import_from_xml function $fromform->$extra = $format->getpath($xml, array('#', $extra, 0, '#', 'text', 0, '#'), '', true); should be $fromform->$extra = $format->getpath($xml, array('#', $extra, 0, '#'), '', true);
            Hide
            timhunt Tim Hunt added a comment -

            Great. Thanks. Submitting for integration now.

            Please keep testing, everyone. If you find any more proplems, we can re-open before it is integrated.

            (Jean-Michel, I do remember you said you were too busy to do this now! I hope we have not made you miss something else.)

            Show
            timhunt Tim Hunt added a comment - Great. Thanks. Submitting for integration now. Please keep testing, everyone. If you find any more proplems, we can re-open before it is integrated. (Jean-Michel, I do remember you said you were too busy to do this now! I hope we have not made you miss something else.)
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Well in fact I am ill so I must stay at home and had to cancel all my lessons for today !

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Well in fact I am ill so I must stay at home and had to cancel all my lessons for today !
            Hide
            timhunt Tim Hunt added a comment -

            Sorry to hear that. I hope you feel better soon. I am glad we gave you something to stop you being bored

            Show
            timhunt Tim Hunt added a comment - Sorry to hear that. I hope you feel better soon. I am glad we gave you something to stop you being bored
            Hide
            didier.jodin didier jodin added a comment - - edited

            Hello Jean-Michel,

            I'm not a programmer, therefore I'm not a pear, therefore this is not a pear review, but a quite basic user's one...

            (Everything in a production site)

            • upgrade from 2.6.1.+ stable : OK
            • Creation, editing and preview (with or without subcategories) : OK
            • Shortanswers questions with image in question text : OK (tested with linked images to an external depository)
            • preview + attempt in a quiz as a student : OK
            • import / export (with category) in XML format : OK

            Let me repeat my congratulations, and I hope you'll feel better very soon !

            Show
            didier.jodin didier jodin added a comment - - edited Hello Jean-Michel, I'm not a programmer, therefore I'm not a pear, therefore this is not a pear review, but a quite basic user's one... (Everything in a production site) upgrade from 2.6.1.+ stable : OK Creation, editing and preview (with or without subcategories) : OK Shortanswers questions with image in question text : OK (tested with linked images to an external depository) preview + attempt in a quiz as a student : OK import / export (with category) in XML format : OK Let me repeat my congratulations, and I hope you'll feel better very soon !
            Hide
            jmvedrine Jean-Michel Vedrine added a comment - - edited

            Thanks a lot Didier,
            I think only Tim can peer review the code
            But your tests and Joseph's ones makes me feel better. It's good to know that everything seems to be working.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - - edited Thanks a lot Didier, I think only Tim can peer review the code But your tests and Joseph's ones makes me feel better. It's good to know that everything seems to be working.
            Hide
            didier.jodin didier jodin added a comment - - edited

            And now that I can read the proper spelling of "peer", I feel quite ridiculous looking at my sentence : "I'm not a pear".
            Well, let's say I'm not that either !

            Show
            didier.jodin didier jodin added a comment - - edited And now that I can read the proper spelling of "peer", I feel quite ridiculous looking at my sentence : "I'm not a pear". Well, let's say I'm not that either !
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Everyone,

            Thanks a lot for your work on this - looks like its going to make a lot of people happy!

            I have to say that accepting radical changes like this in the stable branches is highly unusual, but it seems the situation can not get any worse by integrating it..

            Reflecting a bit on this issue and considering the amount of time this question type has been broken I have to question whether it really should be in core. In fact, the 2.1 release notes actually say "Backward compatibility warning: Random short-answer matching question type was moved out of the main Moodle distribution." - which seemingly was not true. We should try and avoid brokenness like this in the core distribution in future.

            Show
            poltawski Dan Poltawski added a comment - Hi Everyone, Thanks a lot for your work on this - looks like its going to make a lot of people happy! I have to say that accepting radical changes like this in the stable branches is highly unusual, but it seems the situation can not get any worse by integrating it.. Reflecting a bit on this issue and considering the amount of time this question type has been broken I have to question whether it really should be in core. In fact, the 2.1 release notes actually say "Backward compatibility warning: Random short-answer matching question type was moved out of the main Moodle distribution." - which seemingly was not true. We should try and avoid brokenness like this in the core distribution in future.
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated to master, 26 and 25 - thanks!

            Show
            poltawski Dan Poltawski added a comment - Integrated to master, 26 and 25 - thanks!
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Dan,
            If you look at Tim's first comment (written in May 2011 before Moodle 2.1 release), you will read:
            We have decided, almost certainly, not to convert this question type to Moodle 2.1. We need to:

            1. move the code to contrib.
            2. add suitable warnings before the upgrade.

            So the intention was clear, but to understand what happened after that, you will need to read all the comments above and a lot of posts in the Quiz forum

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Dan, If you look at Tim's first comment (written in May 2011 before Moodle 2.1 release), you will read: We have decided, almost certainly, not to convert this question type to Moodle 2.1. We need to: 1. move the code to contrib. 2. add suitable warnings before the upgrade. So the intention was clear, but to understand what happened after that, you will need to read all the comments above and a lot of posts in the Quiz forum
            Hide
            timhunt Tim Hunt added a comment -

            Dan, thanks for integrating this.

            That May 2011 comment was wishful thinking on my part. It came before I did this survey of qtype usage: https://docs.google.com/spreadsheet/ccc?key=0AvW3H1TnOxK-dEROU2dVQ2lkWDU3SlEzaXJqU2lSRUE&usp=drive_web#gid=0 which lead me to conclude that I could not get away with that approach. See also above comments and labels from partners. Also, Martin D did not want this qtype moved to contrib. He wanted it fixed.

            You are right, this breakage should not have been allowed (it would not be these days) and also it should have been fixed sooner. Still, the other improvements made to questions in 2.1 were worth it.

            Show
            timhunt Tim Hunt added a comment - Dan, thanks for integrating this. That May 2011 comment was wishful thinking on my part. It came before I did this survey of qtype usage: https://docs.google.com/spreadsheet/ccc?key=0AvW3H1TnOxK-dEROU2dVQ2lkWDU3SlEzaXJqU2lSRUE&usp=drive_web#gid=0 which lead me to conclude that I could not get away with that approach. See also above comments and labels from partners. Also, Martin D did not want this qtype moved to contrib. He wanted it fixed. You are right, this breakage should not have been allowed (it would not be these days) and also it should have been fixed sooner. Still, the other improvements made to questions in 2.1 were worth it.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment - - edited

            Maybe one of the reasons Martin did not want this qtype moved to contrib is that surprisingly this is one of the very first question types that were added to Moodle 1.0 on February 24th 2003 ! Wow !(see quiz history http://docs.moodle.org/dev/History_of_the_Moodle_quiz_and_question_bank compiled by Tim)
            This also means that if we manage to close this bug this week, randomsamatch will be fixed just in time for it's eleventh birthday .

            Show
            jmvedrine Jean-Michel Vedrine added a comment - - edited Maybe one of the reasons Martin did not want this qtype moved to contrib is that surprisingly this is one of the very first question types that were added to Moodle 1.0 on February 24th 2003 ! Wow !(see quiz history http://docs.moodle.org/dev/History_of_the_Moodle_quiz_and_question_bank compiled by Tim) This also means that if we manage to close this bug this week, randomsamatch will be fixed just in time for it's eleventh birthday .
            Hide
            phalacee Jason Fowler added a comment -

            Works fine, thanks

            Show
            phalacee Jason Fowler added a comment - Works fine, thanks
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Thanks Jason, thanks Dan.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Thanks Jason, thanks Dan.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I began to edit the docs for the 2.6 branch.
            Currently this is mostly a copy of what was in 1.9/2.0 docs. I will document the changes today.
            Then I will backport to 2.5 docs, and change the docs for 2.1, 2.2, 2.3, 2.4 because they currently say "Note: The Random Short-Answer Matching question type was previously a standard question type but was removed from core in Moodle 2.1 and is not available. See its tracker entry." witch was in fact not very accurate as we all know

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I began to edit the docs for the 2.6 branch. Currently this is mostly a copy of what was in 1.9/2.0 docs. I will document the changes today. Then I will backport to 2.5 docs, and change the docs for 2.1, 2.2, 2.3, 2.4 because they currently say "Note: The Random Short-Answer Matching question type was previously a standard question type but was removed from core in Moodle 2.1 and is not available. See its tracker entry." witch was in fact not very accurate as we all know
            Hide
            timhunt Tim Hunt added a comment -

            Thanks Jason for testing this monster. I gave a big sigh of relief when I saw your comment.

            Jean-Michel, You might want to message Helen Foster or Mary Cooch about copying things between different docs versions. They might have more efficient tools to do that.

            Show
            timhunt Tim Hunt added a comment - Thanks Jason for testing this monster. I gave a big sigh of relief when I saw your comment. Jean-Michel, You might want to message Helen Foster or Mary Cooch about copying things between different docs versions. They might have more efficient tools to do that.
            Hide
            tillmad Daniel Tillman added a comment -

            Seems to be working just fine for me. Used it yesterday to give an AP Euro vocab quiz to my students and there were no issues.

            Show
            tillmad Daniel Tillman added a comment - Seems to be working just fine for me. Used it yesterday to give an AP Euro vocab quiz to my students and there were no issues.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I claim to be a simple individual
            liable to err like any other fellow mortal.
            I own, however, that I have humility enough
            to confess my errors and to retrace my steps.

            Mahatma Gandhi

            Your awesome code has met upstream, closing, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I claim to be a simple individual liable to err like any other fellow mortal. I own, however, that I have humility enough to confess my errors and to retrace my steps. Mahatma Gandhi Your awesome code has met upstream, closing, thanks!
            Hide
            marycooch Mary Cooch added a comment - - edited

            Hello Jean-Michel Vedrine Thanks for offering to do the documentation. . I have also added a link to http://docs.moodle.org/26/en/Questions and the 2.5 page saying which precise version of 2.6 and 2.5 it will be in. I have edited the 2.4>2.1 docs of that page to be more accurate also.

            Show
            marycooch Mary Cooch added a comment - - edited Hello Jean-Michel Vedrine Thanks for offering to do the documentation. . I have also added a link to http://docs.moodle.org/26/en/Questions and the 2.5 page saying which precise version of 2.6 and 2.5 it will be in. I have edited the 2.4>2.1 docs of that page to be more accurate also.
            Hide
            timhunt Tim Hunt added a comment -

            Mary Cooch, did you look at recent changes? Jean-Michel has already updated a lot of the docs.

            Show
            timhunt Tim Hunt added a comment - Mary Cooch , did you look at recent changes? Jean-Michel has already updated a lot of the docs.
            Hide
            marycooch Mary Cooch added a comment -

            Yes - I have seen the 2.6 and 2.5 http://docs.moodle.org/en/Random_Short-Answer_Matching_question_type which are great; I've just re-linked them elsewhere and removed the tracker link from the older pages.

            Show
            marycooch Mary Cooch added a comment - Yes - I have seen the 2.6 and 2.5 http://docs.moodle.org/en/Random_Short-Answer_Matching_question_type which are great; I've just re-linked them elsewhere and removed the tracker link from the older pages.

              People

              • Votes:
                42 Vote for this issue
                Watchers:
                38 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Mar/14