Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.3.2
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide
      1. Create a course in 1.9 with a quiz with some questions, back up the course and save the file somewhere safe
      2. Run the Moodle upgrade from 2.2 to 2.3, 2.2 to master
      3. Verify that the questions.oldquestiontextformat column is no more.
      4. Go to the question bank and create / edit / preview a question just to check there are no regressions.
      5. Restore the 1.9 quiz backup
      Show
      Create a course in 1.9 with a quiz with some questions, back up the course and save the file somewhere safe Run the Moodle upgrade from 2.2 to 2.3, 2.2 to master Verify that the questions.oldquestiontextformat column is no more. Go to the question bank and create / edit / preview a question just to check there are no regressions. Restore the 1.9 quiz backup
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w32_MDL-34600_m24_questionschema
    • Rank:
      43035

      Description

      it looks like somebody forgot to drop question->oldquestiontextformat, alternatively would need to add it to install.xml

        Activity

        Hide
        Petr Škoda added a comment -

        Hi Tim, should I do it? Is it really not used any more?

        Show
        Petr Škoda added a comment - Hi Tim, should I do it? Is it really not used any more?
        Hide
        Tim Hunt added a comment -

        The situation is this:

        1. The questions table belongs to Moodle core.

        2. tables like question_multichoice belong to qtype_mutichoice (really, one day, we should rename those tables to be frankenstyle).

        3. In the upgrade from 1.9 -> 2.0, when we added the missing ...format columns, often to upgrade some question_multichoice columns, you need to know what question.questiontypeformat was.

        4. But the core upgrade 1.9 -> 2.0 sometimes changed question.questiontypeformat.

        5. Core upgrade runs before qtype upgrade.

        So, anyway, that is why we had to make a copy of question.questiontypeformat as oldquestiontypeformat in lib/db/upgrade.php, before we started changing it. But, it was only needed during that upgrade.

        Now that we are past 2.3, we can drop the column. I am happy for you to do it. If no, I will.

        Show
        Tim Hunt added a comment - The situation is this: 1. The questions table belongs to Moodle core. 2. tables like question_multichoice belong to qtype_mutichoice (really, one day, we should rename those tables to be frankenstyle). 3. In the upgrade from 1.9 -> 2.0, when we added the missing ...format columns, often to upgrade some question_multichoice columns, you need to know what question.questiontypeformat was. 4. But the core upgrade 1.9 -> 2.0 sometimes changed question.questiontypeformat. 5. Core upgrade runs before qtype upgrade. So, anyway, that is why we had to make a copy of question.questiontypeformat as oldquestiontypeformat in lib/db/upgrade.php, before we started changing it. But, it was only needed during that upgrade. Now that we are past 2.3, we can drop the column. I am happy for you to do it. If no, I will.
        Hide
        Petr Škoda added a comment -

        Hehe, I do not understand your explanation completely, but it is great we can safely drop the column in 2.3x and master upgrade code.

        Next time please always make sure that the install.xml files match the current database structures, otherwise we have huge problems in scripts like dbtransfer or any scheme validation tool if we ever decide to write it.

        I am going to write the upgrade code myself because I will need to coordinate this with other install.xml cleanup, thanks!

        Show
        Petr Škoda added a comment - Hehe, I do not understand your explanation completely, but it is great we can safely drop the column in 2.3x and master upgrade code. Next time please always make sure that the install.xml files match the current database structures, otherwise we have huge problems in scripts like dbtransfer or any scheme validation tool if we ever decide to write it. I am going to write the upgrade code myself because I will need to coordinate this with other install.xml cleanup, thanks!
        Hide
        Petr Škoda added a comment -

        sending for pear review, thanks

        Show
        Petr Škoda added a comment - sending for pear review, thanks
        Hide
        Tim Hunt added a comment -

        +1 from me.

        Show
        Tim Hunt added a comment - +1 from me.
        Hide
        Dan Poltawski added a comment -

        Ut oh, this has only had pear review, should I be concerned? :-P

        Show
        Dan Poltawski added a comment - Ut oh, this has only had pear review, should I be concerned? :-P
        Hide
        Dan Poltawski added a comment -

        But seriously, i'm not sure about dropping a field like this in the middle of stable branch. I know it shouldn't have any implications, just would like some integrator +1's for that.

        Show
        Dan Poltawski added a comment - But seriously, i'm not sure about dropping a field like this in the middle of stable branch. I know it shouldn't have any implications, just would like some integrator +1's for that.
        Hide
        Sam Hemelryk added a comment -

        Hi.

        This gets a +0.5 from me.
        Given that this field is not used in core, was created for the requirement of upgrade code, and that the associated upgrade code has now been removed I think its removal from the 23 upgrade branch is tolerable.
        There is always that slim chance someone is relying on that field being in the database, or present in records returned from the database. However I think in this situation the chance that someone was relying on it is remote enough to void this concern.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi. This gets a +0.5 from me. Given that this field is not used in core, was created for the requirement of upgrade code, and that the associated upgrade code has now been removed I think its removal from the 23 upgrade branch is tolerable. There is always that slim chance someone is relying on that field being in the database, or present in records returned from the database. However I think in this situation the chance that someone was relying on it is remote enough to void this concern. Cheers Sam
        Hide
        Dan Poltawski added a comment -

        The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

        This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P
        Apologies for the inconvenience, this will be integrated next week. Thanks for your collaboration & ciao

        Show
        Dan Poltawski added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologies for the inconvenience, this will be integrated next week. Thanks for your collaboration & ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        +0.5 here too.

        I just would add one more testing step about restoring one 1.9.x course with questions. I'm 99% sure it's completely unrelated but, as far as it mimics some of the old upgrade steps, better check it works without any warn/error depending of that DB field.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - +0.5 here too. I just would add one more testing step about restoring one 1.9.x course with questions. I'm 99% sure it's completely unrelated but, as far as it mimics some of the old upgrade steps, better check it works without any warn/error depending of that DB field. Ciao
        Hide
        Dan Poltawski added a comment -

        Thanks Eloy, I really must get into my reviewing mind backup/restore implications, i'm not very good at remembering it.

        We have over 1.0 integrator vote now, so can integrate.

        Show
        Dan Poltawski added a comment - Thanks Eloy, I really must get into my reviewing mind backup/restore implications, i'm not very good at remembering it. We have over 1.0 integrator vote now, so can integrate.
        Hide
        Dan Poltawski added a comment -

        Integrated, thanks

        Show
        Dan Poltawski added a comment - Integrated, thanks
        Hide
        Rossiani Wijaya added a comment -

        This looks good.

        Test passed.

        Show
        Rossiani Wijaya added a comment - This looks good. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Fixed STOP Closed STOP Thanks STOP

        Yay, imagination! Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Fixed STOP Closed STOP Thanks STOP Yay, imagination! Ciao

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: