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

        Attachments

          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