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 Sub-task
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      2240

      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.

        Activity

        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        Adrian Greeve added a comment -

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

        Show
        Adrian Greeve added a comment - Test passed. Answers are still case sensitive if usecase is on.
        Hide
        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
        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: