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 Bug
    • Status: Closed
    • Priority: Major 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
    • Rank:
      27376
    1. MDL-24470.patch
      4 kB
      Jérôme Mouneyrac

      Issue Links

        Activity

        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        Eloy Lafuente (stronk7) added a comment -

        +1

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

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

        Show
        Eloy Lafuente (stronk7) added a comment - Last comment: Perhaps 2.0 restore also needs the null => 0 fix?
        Hide
        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
        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
        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
        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
        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
        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
        Eloy Lafuente (stronk7) added a comment -

        Fix applied to 2.0 restore!

        Thanks, Jerome, ciao

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

        So... should we close this?

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

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

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