Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-17229 Shortanswer questiontype refactoring
  3. MDL-29095

question_type::initialise_question_instance should use extra_question_fields to get data from $questiondata->options

    Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1, 2.2
    • Fix Version/s: 2.1.3
    • Component/s: Questions
    • Testing Instructions:
      Hide

      1) Create a shortanswer question with usecase on
      2) Re-edit it to ensure usecase value is showed correctly on the edit form
      3) Try question in the preview, changing case of the correct answer, so you are sure case is taken into account
      4) Repeat these steps with usecase off.

      Show
      1) Create a shortanswer question with usecase on 2) Re-edit it to ensure usecase value is showed correctly on the edit form 3) Try question in the preview, changing case of the correct answer, so you are sure case is taken into account 4) Repeat these steps with usecase off.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      Now question creation become more complicated, but there are no reason to use extra_question_fields() in get_question_options and not use it in initialise_question_instance. Otherwise it prevents extra_question_fields() from functioning properly.

      Shortanswer question should rely on extra_question_fields() instead of hardcoding $usecase there.

      That's actually quite easy, so please do. Preg question type class still inherited from shortanswer, and it has quite more extra question fields.

        Gliffy Diagrams

          Activity

          Hide
          oa_sychev Oleg Sychev added a comment -

          Tim, I'm writing this patch and stumble once more in the cursed redundant "answers" field in the shortanswer question table. You don't fill it in question object, yet it remains to be filled by questiontype class. That prevents writing solution to this issue.

          Could I remove handling of "answers" fields from the question type (i.e. save_question_options/get_question_options) too in this git commit (it may be leaved to be in DB)? Please? My testing site still works without this field at all for more than two years now (with only problems on upgrade to 2.1 where it's hardcoded), I have written you a patch to remove it long ago, and this nuisance still persists...

          If you intend to keep it in question type class, then it should be added to the question class too to allow extra_question_fields to work universally.

          P.S. I understand that you deleted it from the new class while preserving in the old class. But given the scale of the issue it's hardly necessary, and it's really gives problems... Could we solve it at last?

          Show
          oa_sychev Oleg Sychev added a comment - Tim, I'm writing this patch and stumble once more in the cursed redundant "answers" field in the shortanswer question table. You don't fill it in question object, yet it remains to be filled by questiontype class. That prevents writing solution to this issue. Could I remove handling of "answers" fields from the question type (i.e. save_question_options/get_question_options) too in this git commit (it may be leaved to be in DB)? Please? My testing site still works without this field at all for more than two years now (with only problems on upgrade to 2.1 where it's hardcoded), I have written you a patch to remove it long ago, and this nuisance still persists... If you intend to keep it in question type class, then it should be added to the question class too to allow extra_question_fields to work universally. P.S. I understand that you deleted it from the new class while preserving in the old class. But given the scale of the issue it's hardly necessary, and it's really gives problems... Could we solve it at last?
          Hide
          oa_sychev Oleg Sychev added a comment -

          Tim, please apply it fast, since conversion of preg for 2.1 depend on this. As always, shortanswer is converted and tested.

          I written a solution for this and even commit it to my git repository, but have trouble pushing it to github due to infamous "git under non-english version of windows" problem.

          So, because it is urgent, I generated a patch with git. The patch is done against Moodle 2.1 stable, but I don't think you'll have problem applying it to the master branch too.

          Show
          oa_sychev Oleg Sychev added a comment - Tim, please apply it fast, since conversion of preg for 2.1 depend on this. As always, shortanswer is converted and tested. I written a solution for this and even commit it to my git repository, but have trouble pushing it to github due to infamous "git under non-english version of windows" problem. So, because it is urgent, I generated a patch with git. The patch is done against Moodle 2.1 stable, but I don't think you'll have problem applying it to the master branch too.
          Hide
          timhunt Tim Hunt added a comment -

          If you want your patch applied fast, it has to be 100% right.

          1. If you want to get rid of the answers field, you need to do it properly, and remove it everywhere. (All affected qtypes, DB fields, backup/restore code, etc.) We already have a separate issue for that, don't we? Yes MDL-17812 - So I don't like you change in public function extra_question_fields() {, and I don't see why it is necessary alongside the rest of your changes.

          2. The first line of the commit comment is way too long.

          3. Is extra_question_fields used everywhere else that is necessary? e.g. backup and restore?

          4. Please can you add some testing instructions to this bug.

          Show
          timhunt Tim Hunt added a comment - If you want your patch applied fast, it has to be 100% right. 1. If you want to get rid of the answers field, you need to do it properly, and remove it everywhere. (All affected qtypes, DB fields, backup/restore code, etc.) We already have a separate issue for that, don't we? Yes MDL-17812 - So I don't like you change in public function extra_question_fields() {, and I don't see why it is necessary alongside the rest of your changes. 2. The first line of the commit comment is way too long. 3. Is extra_question_fields used everywhere else that is necessary? e.g. backup and restore? 4. Please can you add some testing instructions to this bug.
          Hide
          oa_sychev Oleg Sychev added a comment -

          1 & 2 Ok, I remove line wich deletes 'answers' out of extra_question_fields and shorten first line (any rules on it's length?) and reupload patch shortly

          3. The issue with backup and restore & extra_question_fields is MDL-25617. It is stalled because first you waited to review it so long I lost access to the people I used to get it done, and when you reviewed it at last I was already missing both time and helpers to fix problems you found (mostly trivia). I remember about it but far too busy to do something (you know my timezone is Moscow, so just look at the time of my attachment...) right now.

          Meanwhile, it is broken since Moodle 2.0 release and I don't get any single complaint about it, while I already got several ones about not releasing preg for Moodle 2.1. (and I still have to deal with behavours and penalty calculation). So I consider this issue, which stop all question from working, far more urgent than backup/restore thing.

          4. Testing with core qtype ensures that handling of usecase field in shortanswer working correctly:
          1) create shortanswer question with usecase on
          2) re-edit it to ensure usecase value is showed correctly on the edit form
          3) try question in the preview, changing case of the correct answer, so you are sure case is taken into account
          4) you may repeat these steps with usecase off (I did), thought it less necessary since this is default mode

          Show
          oa_sychev Oleg Sychev added a comment - 1 & 2 Ok, I remove line wich deletes 'answers' out of extra_question_fields and shorten first line (any rules on it's length?) and reupload patch shortly 3. The issue with backup and restore & extra_question_fields is MDL-25617 . It is stalled because first you waited to review it so long I lost access to the people I used to get it done, and when you reviewed it at last I was already missing both time and helpers to fix problems you found (mostly trivia). I remember about it but far too busy to do something (you know my timezone is Moscow, so just look at the time of my attachment...) right now. Meanwhile, it is broken since Moodle 2.0 release and I don't get any single complaint about it, while I already got several ones about not releasing preg for Moodle 2.1. (and I still have to deal with behavours and penalty calculation). So I consider this issue, which stop all question from working, far more urgent than backup/restore thing. 4. Testing with core qtype ensures that handling of usecase field in shortanswer working correctly: 1) create shortanswer question with usecase on 2) re-edit it to ensure usecase value is showed correctly on the edit form 3) try question in the preview, changing case of the correct answer, so you are sure case is taken into account 4) you may repeat these steps with usecase off (I did), thought it less necessary since this is default mode
          Hide
          oa_sychev Oleg Sychev added a comment -

          'answers' field remained and first line of commit shortened.

          Thought that was much easy to do without using git, reverting commits etc...

          Show
          oa_sychev Oleg Sychev added a comment - 'answers' field remained and first line of commit shortened. Thought that was much easy to do without using git, reverting commits etc...
          Hide
          timhunt Tim Hunt added a comment -

          Thanks Oleg. Submitting for integration now.

          Note that this change will not break any third-party question types (although it does make them slightly easier to write.)

          (Just one more thing. We tend not to used Signed-off by in Moodle, for some reason. Better if you don't use it with future patches, but it does not matter here.)

          Show
          timhunt Tim Hunt added a comment - Thanks Oleg. Submitting for integration now. Note that this change will not break any third-party question types (although it does make them slightly easier to write.) (Just one more thing. We tend not to used Signed-off by in Moodle, for some reason. Better if you don't use it with future patches, but it does not matter here.)
          Hide
          oa_sychev Oleg Sychev added a comment -

          Tim, will these set of fix branches mean preg isn't going to work for Moodle 2.1 at all, only for 2.2?

          Show
          oa_sychev Oleg Sychev added a comment - Tim, will these set of fix branches mean preg isn't going to work for Moodle 2.1 at all, only for 2.2?
          Hide
          timhunt Tim Hunt added a comment -

          Well, I was being risk-averse. Looking again, it is safe to do this in 2.1 too. New branch made.

          Show
          timhunt Tim Hunt added a comment - Well, I was being risk-averse. Looking again, it is safe to do this in 2.1 too. New branch made.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          PS: Note this is the last message of this type that you will receive along the whole November month, because we are running continuous integration this weeks while QA tests for release of Moodle 2.2 (Dec 1st) are being performed.

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao PS: Note this is the last message of this type that you will receive along the whole November month, because we are running continuous integration this weeks while QA tests for release of Moodle 2.2 (Dec 1st) are being performed.
          Hide
          aolley Adam Olley added a comment -

          Hi All,

          We (NetSpot) have a couple of clients that use this qtype in 2.0.x looking to upgrade to 2.1/2.2 and not being able to have this qtype is seen as a bit of a blocker to them.

          +1 to getting this in

          Show
          aolley Adam Olley added a comment - Hi All, We (NetSpot) have a couple of clients that use this qtype in 2.0.x looking to upgrade to 2.1/2.2 and not being able to have this qtype is seen as a bit of a blocker to them. +1 to getting this in
          Hide
          oa_sychev Oleg Sychev added a comment -

          Integration of this patch (which unbreaks once more "extra_question_fields" feature) into Moodle code hinders release of Preg question type for Moodle 2.1. I have quite a number of requests for getting Preg working for 2.1, so the faster we get this in - the better.

          P.S. Eloy, it's useless to ask me for rebase patch since (as you could see in the path) Tim is maintaining these branches on the server. Still I have no trouble rebasing these on local repository.

          Show
          oa_sychev Oleg Sychev added a comment - Integration of this patch (which unbreaks once more "extra_question_fields" feature) into Moodle code hinders release of Preg question type for Moodle 2.1. I have quite a number of requests for getting Preg working for 2.1, so the faster we get this in - the better. P.S. Eloy, it's useless to ask me for rebase patch since (as you could see in the path) Tim is maintaining these branches on the server. Still I have no trouble rebasing these on local repository.
          Hide
          lvazdela Luis Vázquez de Lara added a comment - - edited

          Hi,
          I'm a heavy user of regular expressions for my quizes, PREG is ideal for me because allows the use of open regexps. I'm prof atthe School of Medicine of the University of Puebla in Mexico, these questions allow to write a diagnosis with a reasonable freedom of redaction and without suggesting answers as in multiple choice questions. I would be great to have it in the core course, not as a plug in.
          Thanks
          Luis

          Show
          lvazdela Luis Vázquez de Lara added a comment - - edited Hi, I'm a heavy user of regular expressions for my quizes, PREG is ideal for me because allows the use of open regexps. I'm prof atthe School of Medicine of the University of Puebla in Mexico, these questions allow to write a diagnosis with a reasonable freedom of redaction and without suggesting answers as in multiple choice questions. I would be great to have it in the core course, not as a plug in. Thanks Luis
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          I was a bit annoyed because, in the patch I only saw how the old "usecase" code was being deleted but the realized that qtype_shortanswer->extra_question_fields() already has it.

          PS1: Plz, try to put one space between the '//' and the comments. Not blocker but a coding recommendation.

          PS2: The message above about suggesting to rebase is sent automatically to all issues pending to integrate after git repos have been updated, aiming to reduce conflict chances (only happening rarely, mostly when there is some upgrade script involved, or certain area of code is being modified by various developers independently).

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks! I was a bit annoyed because, in the patch I only saw how the old "usecase" code was being deleted but the realized that qtype_shortanswer->extra_question_fields() already has it. PS1: Plz, try to put one space between the '//' and the comments. Not blocker but a coding recommendation. PS2: The message above about suggesting to rebase is sent automatically to all issues pending to integrate after git repos have been updated, aiming to reduce conflict chances (only happening rarely, mostly when there is some upgrade script involved, or certain area of code is being modified by various developers independently).
          Hide
          abgreeve Adrian Greeve added a comment -

          Test passed. Answers are still case sensitive if usecase is on.

          Show
          abgreeve Adrian Greeve added a comment - Test passed. Answers are still case sensitive if usecase is on.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          And this has landed upstream, just on time for the upcoming new releases week. Thanks for it!

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - And this has landed upstream, just on time for the upcoming new releases week. Thanks for it! Ciao

            People

            • Votes:
              7 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                28/Nov/11