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

      Description

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

        Gliffy Diagrams

          Activity

          Hide
          Petr Skoda added a comment -

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

          Show
          Petr Skoda 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 Skoda 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 Skoda 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 Skoda added a comment -

          sending for pear review, thanks

          Show
          Petr Skoda 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: