Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.3
    • Fix Version/s: 2.5
    • Component/s: Questions
    • Labels:
      None
    • Testing Instructions:
      Hide

      You need to test all aspects of shortanswer questions, and multianswer questions that contain shortanswer sub-questions.

      Before upgrading, or with Moodle 2.4, please first:

      1. Export shortanswer questions as GIFT and Moodle XML
      2. Backup and restore a course with shortanswer questions

      Run upgrade to master.

      1. Preview the shortanswer question you created earlier
      2. Edit/Delete the shortanswer question you created earlier
      3. Create, edit and delete a new shortanswer question
      4. Import the GIFT and Moodle XML questions you created earlier
      5. Verify the imported questions
      6. Restore the short answer questions course you created earlier
      7. Verify the imported questions
      8. Export shortanswer questions as GIFT and Moodle XML, and re-import and check that all the options are correctly preserved.
      9. Backup and restore a course with shortanswer questions, and check that all the settings are correctly preserved.
      10. Create edit and delete multianswer questions containing short-answer subquestions.
      11. Preivew those multianswer questions.
      Show
      You need to test all aspects of shortanswer questions, and multianswer questions that contain shortanswer sub-questions. Before upgrading, or with Moodle 2.4, please first: Export shortanswer questions as GIFT and Moodle XML Backup and restore a course with shortanswer questions Run upgrade to master. Preview the shortanswer question you created earlier Edit/Delete the shortanswer question you created earlier Create, edit and delete a new shortanswer question Import the GIFT and Moodle XML questions you created earlier Verify the imported questions Restore the short answer questions course you created earlier Verify the imported questions Export shortanswer questions as GIFT and Moodle XML, and re-import and check that all the options are correctly preserved. Backup and restore a course with shortanswer questions, and check that all the settings are correctly preserved. Create edit and delete multianswer questions containing short-answer subquestions. Preivew those multianswer questions.
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      2237

      Description

      The field is obsolete, and quite bothersome when restoring, but we need to make sure we not breaking anything.

      Changes to 2.0 only probably OK, thought maybe in 1.9 we can drop filling this field without actual deleting it from db.

        Issue Links

          Activity

          Hide
          Oleg Sychev added a comment -

          To find all possible uses of "answers" field I launch search on entire moodle codebase with regex: (?<!options)>answers (options>answer filled now from question_answers table) and see all files with occurences. There was several results of interest:

          ....shortanswer\questiontype.php - obvious, only filling (save_question_options), backuping and restoring it.
          .....quiz\restorelibpre15.php - restoring it
          And there was two files, where I really can't understand, have they anything with question_shortanswer table or not: format.php for hotpot and webct formats.

          There was also some occurences in mysql.php and postgres7.php, but they are related with quite old versions, so there is probably safe to ignore them.

          Did you consider such testing enough?

          Show
          Oleg Sychev added a comment - To find all possible uses of "answers" field I launch search on entire moodle codebase with regex: (?<!options) >answers (options >answer filled now from question_answers table) and see all files with occurences. There was several results of interest: ....shortanswer\questiontype.php - obvious, only filling (save_question_options), backuping and restoring it. .....quiz\restorelibpre15.php - restoring it And there was two files, where I really can't understand, have they anything with question_shortanswer table or not: format.php for hotpot and webct formats. There was also some occurences in mysql.php and postgres7.php, but they are related with quite old versions, so there is probably safe to ignore them. Did you consider such testing enough?
          Hide
          Tim Hunt added a comment -

          That is the sort of test that needs to be done before messing around with the code. Then, after changing the code, you need to test all of the things that might be broken as a result.

          If you do it only in HEAD, you can rely on additional testing just naturally happening between you writing the code and it being used for real. However, that is not an excuse for not testing properly yourself, it just reduces the risk.

          Show
          Tim Hunt added a comment - That is the sort of test that needs to be done before messing around with the code. Then, after changing the code, you need to test all of the things that might be broken as a result. If you do it only in HEAD, you can rely on additional testing just naturally happening between you writing the code and it being used for real. However, that is not an excuse for not testing properly yourself, it just reduces the risk.
          Hide
          Oleg Sychev added a comment -

          "That is the sort of test that needs to be done before messing around with the code. Then, after changing the code, you need to test all of the things that might be broken as a result. " I do know this, but I'm unsure is whether I know Moodle well enought to identify and test all part which can use this field - there are many and I can omit something.

          Anyway, this is low priority for me too now, because getting rid of the loop in backup/restore we made much easy to support recoding this field during restore. Mayble later I still do this, but not right now.

          Show
          Oleg Sychev added a comment - "That is the sort of test that needs to be done before messing around with the code. Then, after changing the code, you need to test all of the things that might be broken as a result. " I do know this, but I'm unsure is whether I know Moodle well enought to identify and test all part which can use this field - there are many and I can omit something. Anyway, this is low priority for me too now, because getting rid of the loop in backup/restore we made much easy to support recoding this field during restore. Mayble later I still do this, but not right now.
          Hide
          Oleg Sychev added a comment -

          As an experiment I backup DB and delete answers field from it, commenting out all relevant code in shortanswer/questiontype.php.

          I try creating/editing/previewing/using shortanswer/numerical/close(with shortanswer) questions; also backup/restore and Moodle XML import/export - all this seems to work OK.

          Things I can't test were: hotpot and webct import, and pre-15 restore, 'cause I can't get relevant input files. Can continue testing if you suggests another areas of testing.

          Show
          Oleg Sychev added a comment - As an experiment I backup DB and delete answers field from it, commenting out all relevant code in shortanswer/questiontype.php. I try creating/editing/previewing/using shortanswer/numerical/close(with shortanswer) questions; also backup/restore and Moodle XML import/export - all this seems to work OK. Things I can't test were: hotpot and webct import, and pre-15 restore, 'cause I can't get relevant input files. Can continue testing if you suggests another areas of testing.
          Hide
          Tim Hunt added a comment -

          I don't think import is a problem, since it goes through the standard save_question method.

          pre-15 restore. Yuck. Again, I don't think this would break.

          So it seems likely that this coudl safely be done, HEAD only, but, as you say, not a priority.

          Show
          Tim Hunt added a comment - I don't think import is a problem, since it goes through the standard save_question method. pre-15 restore. Yuck. Again, I don't think this would break. So it seems likely that this coudl safely be done, HEAD only, but, as you say, not a priority.
          Hide
          Oleg Sychev added a comment -

          Just a note what can be dropped in the code after deleting this field (so we don't forget it later):

          type/shortanswer/questiontype.php
          1) answers field from extra_question_field function no line deletion but this should be done
          2) save_question_options(): $answers var (3 lines deleted)
          3) restore() function entirely (45 lines - with comments)

          format/xml/format.php
          1)import_shortanswer() function entirely (28 lines - with comments)
          2) shortanswer part from readquestions() (3 lines)
          3) shortanswer part from writequestion() (11 lines)

          Total: about 90 lines of code deleted.

          Show
          Oleg Sychev added a comment - Just a note what can be dropped in the code after deleting this field (so we don't forget it later): type/shortanswer/questiontype.php 1) answers field from extra_question_field function no line deletion but this should be done 2) save_question_options(): $answers var (3 lines deleted) 3) restore() function entirely (45 lines - with comments) format/xml/format.php 1)import_shortanswer() function entirely (28 lines - with comments) 2) shortanswer part from readquestions() (3 lines) 3) shortanswer part from writequestion() (11 lines) Total: about 90 lines of code deleted.
          Hide
          Shkarupa Alex added a comment -

          I tested addition of new questions and editing of exist ones. But i can't test import/export and backup/restore because it is broken

          Show
          Shkarupa Alex added a comment - I tested addition of new questions and editing of exist ones. But i can't test import/export and backup/restore because it is broken
          Hide
          Oleg Sychev added a comment -

          Tim, let me introduce you Alex Shkarupa. He is my student, who will be working on MDL-20251 later. This is his first attempt to contribute to a Moodle.

          I reviewed the patch and find it in good condition. This may be not a priority task, but then the earlier it get in Moodle the more testing will it gets before release. Also, thought it is currently impossible to test import/export and backup/restore on 2.0, my 1.9 site working without "answers" from February 2009 confirm that it should work, I tested import/export and backup/restore there. The only non-1.9 part of this patch is database upgrade, all other parts were tested on 1.9 version ...

          Show
          Oleg Sychev added a comment - Tim, let me introduce you Alex Shkarupa. He is my student, who will be working on MDL-20251 later. This is his first attempt to contribute to a Moodle. I reviewed the patch and find it in good condition. This may be not a priority task, but then the earlier it get in Moodle the more testing will it gets before release. Also, thought it is currently impossible to test import/export and backup/restore on 2.0, my 1.9 site working without "answers" from February 2009 confirm that it should work, I tested import/export and backup/restore there. The only non-1.9 part of this patch is database upgrade, all other parts were tested on 1.9 version ...
          Hide
          Tim Hunt added a comment -

          Hi Alex. This is a busy week for me (http://www.open.ac.uk/wikis/ouocmc/OU_Orchestra as well as a full week of work) so I may not find time to review this patch until next week. If I seem to have forgotten about this, please remind me. Thank you for contributing to the quiz.

          Show
          Tim Hunt added a comment - Hi Alex. This is a busy week for me ( http://www.open.ac.uk/wikis/ouocmc/OU_Orchestra as well as a full week of work) so I may not find time to review this patch until next week. If I seem to have forgotten about this, please remind me. Thank you for contributing to the quiz.
          Hide
          Tim Hunt added a comment -

          If you can do a new patch very soon that is
          1. based on the git master branch.
          2. does both affected qtypes (shortanswer and multichoice) - oh, and possibly calculatedmulti too
          3. deals with all the affected code (qtype, editing form, import/export, backup/restore, ...)

          Then we may be able to get this in before the Moodle 2.2 release, otherwise it will have to wait until 2.3.

          Show
          Tim Hunt added a comment - If you can do a new patch very soon that is 1. based on the git master branch. 2. does both affected qtypes (shortanswer and multichoice) - oh, and possibly calculatedmulti too 3. deals with all the affected code (qtype, editing form, import/export, backup/restore, ...) Then we may be able to get this in before the Moodle 2.2 release, otherwise it will have to wait until 2.3.
          Hide
          Oleg Sychev added a comment -

          How much exactly is "very soon"?

          Show
          Oleg Sychev added a comment - How much exactly is "very soon"?
          Hide
          Tim Hunt added a comment -

          Well, the feature freeze for Moodle 2.2 was 1st November. The release is going to be on 1st December. QA testing is about to start today. Will will have to plead a special case to get this in. I am happy to do the necessary pleading, but the sooner the better.

          Show
          Tim Hunt added a comment - Well, the feature freeze for Moodle 2.2 was 1st November. The release is going to be on 1st December. QA testing is about to start today. Will will have to plead a special case to get this in. I am happy to do the necessary pleading, but the sooner the better.
          Hide
          Tim Hunt added a comment -

          Finally, it is done! Submitting for integration.

          Show
          Tim Hunt added a comment - Finally, it is done! Submitting for integration.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          I think that reading your code I learned something : I already renamed some of my question types tables from question_myqtype to qtype_myqtype but in fact I should rename them to qtype_myqtype_options. True ?

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, I think that reading your code I learned something : I already renamed some of my question types tables from question_myqtype to qtype_myqtype but in fact I should rename them to qtype_myqtype_options. True ?
          Hide
          Joseph Rézeau added a comment -

          @Jean-Michel,
          I don't think it is necessary to add the "_option" suffix. Let's see what Tim says.

          Show
          Joseph Rézeau added a comment - @Jean-Michel, I don't think it is necessary to add the "_option" suffix. Let's see what Tim says.
          Hide
          Tim Hunt added a comment -

          The answer is "yes".

          All the coding style rules say is that tables belonging to plugin_whatever have to start with plugin_whatever, so either is acceptable.

          I had to make a decision. I found that in Moodle core, we already have qtype_essay_options. On the other hand, all the ou question types have tables like qtype_ddmarker. So, there was not a clear precedent.

          I based my decision on two considerations:

          1. it was more important to be consistent within Moodle core.
          2. what this table contains is not a 'short answer question'. It is 'the extra qtype-specific options for a shortanswer question'. Therefore qtype_shortanswer_options is a better description of what the table contains.

          I suggest that, if you make a new question type, you go for the _options name, but don't bother to change existing question types that already follow the qtype_whatever rules.

          Show
          Tim Hunt added a comment - The answer is "yes". All the coding style rules say is that tables belonging to plugin_whatever have to start with plugin_whatever, so either is acceptable. I had to make a decision. I found that in Moodle core, we already have qtype_essay_options. On the other hand, all the ou question types have tables like qtype_ddmarker. So, there was not a clear precedent. I based my decision on two considerations: it was more important to be consistent within Moodle core. what this table contains is not a 'short answer question'. It is 'the extra qtype-specific options for a shortanswer question'. Therefore qtype_shortanswer_options is a better description of what the table contains. I suggest that, if you make a new question type, you go for the _options name, but don't bother to change existing question types that already follow the qtype_whatever rules.
          Hide
          Joseph Rézeau added a comment -

          Thanks, Tim.

          Show
          Joseph Rézeau added a comment - Thanks, Tim.
          Hide
          Dan Poltawski added a comment -

          I've expanded the testing instructions a bit to include upgraded info and old backups/import files since they weren't considered at all in the testing instructions. You might want to add more to this.

          Show
          Dan Poltawski added a comment - I've expanded the testing instructions a bit to include upgraded info and old backups/import files since they weren't considered at all in the testing instructions. You might want to add more to this.
          Hide
          Dan Poltawski added a comment -

          Thanks. i've integrated this now.

          Hope this change is worth the potential for regressions!

          Show
          Dan Poltawski added a comment - Thanks. i've integrated this now. Hope this change is worth the potential for regressions!
          Hide
          Dan Poltawski added a comment - - edited

          The CI check is saying that we have an inconsistent state between the upgraded and newly installed dbs:

          Problems found comparing databases!

          Table question_shortanswer only available in first DB
          Table qtype_shortanswer_options only available in second DB
          http://integration.moodle.org/job/05.%20Compare%20installed-upgraded%20DB%20from%2022_STABLE%20(master)/360/

          Show
          Dan Poltawski added a comment - - edited The CI check is saying that we have an inconsistent state between the upgraded and newly installed dbs: Problems found comparing databases! Table question_shortanswer only available in first DB Table qtype_shortanswer_options only available in second DB http://integration.moodle.org/job/05.%20Compare%20installed-upgraded%20DB%20from%2022_STABLE%20(master)/360/
          Hide
          Dan Poltawski added a comment -

          Looking at the code I can't work this out easily, it does look correct.

          Show
          Dan Poltawski added a comment - Looking at the code I can't work this out easily, it does look correct.
          Hide
          Dan Poltawski added a comment -

          Seems to have resolved itself. I suspect it was some sort of concurrency problem with the ci server.

          Show
          Dan Poltawski added a comment - Seems to have resolved itself. I suspect it was some sort of concurrency problem with the ci server.
          Show
          David Monllaó added a comment - Restoring tester, sorry for the confusion Ankit ( https://tracker.moodle.org/browse/MDL-35074?focusedCommentId=199064&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-199064 )
          Hide
          Ankit Agarwal added a comment -

          Passing as I couldn't find any regressions and it worked as described.
          Thanks

          Show
          Ankit Agarwal added a comment - Passing as I couldn't find any regressions and it worked as described. Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: