Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-24470

Database: upgrade doesn't seem to mark the 'notification' column as not null causing restore problem

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.9, 2.0
    • Fix Version/s: 1.9.9, 2.0
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE

      Gliffy Diagrams

      1. MDL-24470.patch
        4 kB
        Jérôme Mouneyrac

        Issue Links

          Activity

          Hide
          jerome Jérôme Mouneyrac added a comment - - edited

          The problem is:

          There were a database module upgrade fix done in HEAD two years ago (http://tracker.moodle.org/browse/MDL-14548). The fix only run for version < 2007101513
          The fix concerned only HEAD so it wasn't ported on 1.9.

          The problem being that now the 1.9 database module version = 2007101514

          So when someone upgrade the latest 1.9 to HEAD, the HEAD fix never gets run.

          I suggest to:
          1. run the fix again on HEAD if it wasn't
          2. add the upgrade fix to 1.9 if it doesn't break database module to have it set to 0 by default. So we don't run again to the same problem if ever 1.9 database module version would come again with a higher version number than 2.0. (very improbable)

          Note: example of site impacted: cool courses, qamaster. This bug is breaking restore.

          Show
          jerome Jérôme Mouneyrac added a comment - - edited The problem is: There were a database module upgrade fix done in HEAD two years ago ( http://tracker.moodle.org/browse/MDL-14548 ). The fix only run for version < 2007101513 The fix concerned only HEAD so it wasn't ported on 1.9. The problem being that now the 1.9 database module version = 2007101514 So when someone upgrade the latest 1.9 to HEAD, the HEAD fix never gets run. I suggest to: 1. run the fix again on HEAD if it wasn't 2. add the upgrade fix to 1.9 if it doesn't break database module to have it set to 0 by default. So we don't run again to the same problem if ever 1.9 database module version would come again with a higher version number than 2.0. (very improbable) Note: example of site impacted: cool courses, qamaster. This bug is breaking restore.
          Hide
          jerome Jérôme Mouneyrac added a comment -

          I added dongsheng, Martin and Eloy. Let me know what you think, I'll come up with patches soon.

          Show
          jerome Jérôme Mouneyrac added a comment - I added dongsheng, Martin and Eloy. Let me know what you think, I'll come up with patches soon.
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Note: I dond't know yet why qamaster has the bug. Jordan told me he did a fresh install from 2.0 and the install.xml was fixed 2 years ago...

          Show
          jerome Jérôme Mouneyrac added a comment - Note: I dond't know yet why qamaster has the bug. Jordan told me he did a fresh install from 2.0 and the install.xml was fixed 2 years ago...
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Great catch, Jerome!

          this case should remember as to, always, after release, roll all the plugin versions to current date, so we won't get that problem again (with old stable rolling because of any important reason and conflicting with HEAD upgrade paths).

          So yes, +1 to repeat (with current versions) the install/upgrade changes in MDL-14548, both in 1.9 and HEAD.

          About qamaster... crap, I cannot find any reason. Perhaps it was created before the fix? Strange, strange.

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Great catch, Jerome! this case should remember as to, always, after release, roll all the plugin versions to current date, so we won't get that problem again (with old stable rolling because of any important reason and conflicting with HEAD upgrade paths). So yes, +1 to repeat (with current versions) the install/upgrade changes in MDL-14548 , both in 1.9 and HEAD. About qamaster... crap, I cannot find any reason. Perhaps it was created before the fix? Strange, strange. Ciao
          Hide
          jerome Jérôme Mouneyrac added a comment -

          It was a combined effort of Dongsheng and me Here a patch for 1.9 that will resolved this problem for ever. I'll apply the fix a second time on head too.
          Thanks.

          For qamaster, my comment was because Jordan arrived not more than a year ago, so the fix should have been applied. don't get it too and could not reproduce on any HEAD version I have access.

          Show
          jerome Jérôme Mouneyrac added a comment - It was a combined effort of Dongsheng and me Here a patch for 1.9 that will resolved this problem for ever. I'll apply the fix a second time on head too. Thanks. For qamaster, my comment was because Jordan arrived not more than a year ago, so the fix should have been applied. don't get it too and could not reproduce on any HEAD version I have access.
          Hide
          jerome Jérôme Mouneyrac added a comment - - edited

          committed the fix on HEAD.
          Just to be very good, I'll run through all database module in 1.9 (backup/restore include) before to commit on 1.9.
          Anyway this is not urgent anymore as the HEAD fix is enough, it would work till 1.9 reach version number of today (I bet on never, except if there is a dev error)

          Show
          jerome Jérôme Mouneyrac added a comment - - edited committed the fix on HEAD. Just to be very good, I'll run through all database module in 1.9 (backup/restore include) before to commit on 1.9. Anyway this is not urgent anymore as the HEAD fix is enough, it would work till 1.9 reach version number of today (I bet on never, except if there is a dev error)
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          2 comments:

          1) about qamaster/qa... perhaps they were originally copied from demomaster/demo that are older, so the problem was there? Anyway, who cares.

          2) About the 1.9 patch:

          a) it is missing the version.php bump (last digit)
          b) I'd add to restore the same condition existing @ data_update_instance()
          c) what the hell is that field used for?! There aren't more occurrences along the whole module.

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - 2 comments: 1) about qamaster/qa... perhaps they were originally copied from demomaster/demo that are older, so the problem was there? Anyway, who cares. 2) About the 1.9 patch: a) it is missing the version.php bump (last digit) b) I'd add to restore the same condition existing @ data_update_instance() c) what the hell is that field used for?! There aren't more occurrences along the whole module. Ciao
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Hi Eloy, I confirm that restore need a fix, I attached a new patch with the restore fix similar to the update one.

          Show
          jerome Jérôme Mouneyrac added a comment - Hi Eloy, I confirm that restore need a fix, I attached a new patch with the restore fix similar to the update one.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          +1

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - +1
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Last comment: Perhaps 2.0 restore also needs the null => 0 fix?

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Last comment: Perhaps 2.0 restore also needs the null => 0 fix?
          Hide
          jerome Jérôme Mouneyrac added a comment -

          yes sure, 1.9 backup done before today will break on 2.0. if it's going to be supported it's need to be fixed.

          Show
          jerome Jérôme Mouneyrac added a comment - yes sure, 1.9 backup done before today will break on 2.0. if it's going to be supported it's need to be fixed.
          Hide
          jerome Jérôme Mouneyrac added a comment -

          For QA to retest here are the steps:

          1. create a database backup with a database module version.php <= to 2007101514. This database module needs a least one entry (the notification column field should be set to null by default)
          2. restore the database backup on the lastest 1.9 from CVS => it should work.

          Show
          jerome Jérôme Mouneyrac added a comment - For QA to retest here are the steps: 1. create a database backup with a database module version.php <= to 2007101514. This database module needs a least one entry (the notification column field should be set to null by default) 2. restore the database backup on the lastest 1.9 from CVS => it should work.
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Eloy: I don't know much with restore process in 2.0, do you want to fix it if it's quick?
          Otherwise I can do it, just give me docs/advices where to start

          Show
          jerome Jérôme Mouneyrac added a comment - Eloy: I don't know much with restore process in 2.0, do you want to fix it if it's quick? Otherwise I can do it, just give me docs/advices where to start
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Fix applied to 2.0 restore!

          Thanks, Jerome, ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Fix applied to 2.0 restore! Thanks, Jerome, ciao
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          So... should we close this?

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - So... should we close this?
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Hi Eloy, I didn't see you fixed it
          Thank you!
          I mark as resolved.

          Show
          jerome Jérôme Mouneyrac added a comment - Hi Eloy, I didn't see you fixed it Thank you! I mark as resolved.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                8/Jun/10