Moodle
  1. Moodle
  2. MDL-27721

Discrepancy between installed and upgraded Moodle 2.1 on ratings

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1
    • Component/s: Ratings
    • Labels:
    • Rank:
      17575

      Description

      While reviewing the whole DB structure, these discrepancies have been found between one 2.1 installed from scratch and one 2.1 upgraded from 2.0:

      Column component of table rating difference found in default_value:  !== unknown
      Column ratingarea of table rating difference found in default_value:  !== unknown
      

      The patch available just makes the defaults for the component & ratingarea to be, also, "unknown", so the discrepancy will be out.

      But I'm not sure if that is the correct way or we should, instead, fill all the current "unknows" in DB and then drop the default. Perhaps we can do that after 2.1 and my (simpler) patch is enough for now. For your entire consideration.

      Ciao

        Activity

        Hide
        Andrew Davis added a comment -

        Ill get Sam to check that Im correct here but from memory the upgrade process was:

        1) add new columns with "unknown" in them.
        2) module (forum, glossary, data) upgrade code finds the relevant rows and updates those columns

        $sql = "UPDATE {rating}
                        SET component = 'mod_forum', ratingarea = 'post'
                        WHERE contextid IN (
                            SELECT ctx.id
                              FROM {context} ctx
                              JOIN {course_modules} cm ON cm.id = ctx.instanceid
                              JOIN {modules} m ON m.id = cm.module
                             WHERE ctx.contextlevel = 70 AND
                                   m.name = 'forum'
                        ) AND component = 'unknown'";
                $DB->execute($sql);
        

        So the "unknown" default is necessary unless we amend the per module update queries to just update all rows regardless of what was previously in the component and ratingarea columns.

        three options:
        1) add the "unknown" defaults to install.xml
        2) add another bit of upgrade code that removes the "unknown" defaults.
        3) amend the module upgrade code to not check for the "unknown" value.

        my vote would go for either #1 or #2.

        Show
        Andrew Davis added a comment - Ill get Sam to check that Im correct here but from memory the upgrade process was: 1) add new columns with "unknown" in them. 2) module (forum, glossary, data) upgrade code finds the relevant rows and updates those columns $sql = "UPDATE {rating} SET component = 'mod_forum', ratingarea = 'post' WHERE contextid IN ( SELECT ctx.id FROM {context} ctx JOIN {course_modules} cm ON cm.id = ctx.instanceid JOIN {modules} m ON m.id = cm.module WHERE ctx.contextlevel = 70 AND m.name = 'forum' ) AND component = 'unknown'"; $DB->execute($sql); So the "unknown" default is necessary unless we amend the per module update queries to just update all rows regardless of what was previously in the component and ratingarea columns. three options: 1) add the "unknown" defaults to install.xml 2) add another bit of upgrade code that removes the "unknown" defaults. 3) amend the module upgrade code to not check for the "unknown" value. my vote would go for either #1 or #2.
        Hide
        Andrew Davis added a comment -

        Added Sam as a watcher. Sam, as the author of the code in question do you have any wise words?

        Show
        Andrew Davis added a comment - Added Sam as a watcher. Sam, as the author of the code in question do you have any wise words?
        Hide
        Sam Hemelryk added a comment -

        Hmmm looking at this I agree that those defaults shouldn't be there.
        I am happy with dropping the defaults as a separate upgrade block either now or after 2.1 has been released.
        I don't think the bit of code to drop the default would be too big would it? in which case how about we do it now.
        However if you see potential conflicts or problems there Eloy then my vote goes for using your patch now and creating a blocker for after 2.1 release to clean up the defaults.

        I'll leave this awaiting integration review for the time being till we've had a chance to talk about it.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hmmm looking at this I agree that those defaults shouldn't be there. I am happy with dropping the defaults as a separate upgrade block either now or after 2.1 has been released. I don't think the bit of code to drop the default would be too big would it? in which case how about we do it now. However if you see potential conflicts or problems there Eloy then my vote goes for using your patch now and creating a blocker for after 2.1 release to clean up the defaults. I'll leave this awaiting integration review for the time being till we've had a chance to talk about it. Cheers Sam
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Ho, I've prepared this commit:

        https://github.com/stronk7/moodle/commit/2b64de2008404c17396267e110227393ebf298f8

        that applies clean onto current integration and follows the "drop default" alternative, I really agree it is better than the original one proposed (add default to install.xml). As far a no new ratings can arrive with empty component/area, let's enforce it.

        Ciao

        Note: I have not updated the information above to have both alternatives available, the new (drop) one is:

        repo: git://github.com/stronk7/moodle.git
        branch: MDL-27721_drop_default_integration_ready
        diff: https://github.com/stronk7/moodle/commit/2b64de2008404c17396267e110227393ebf298f8

        Show
        Eloy Lafuente (stronk7) added a comment - Ho, I've prepared this commit: https://github.com/stronk7/moodle/commit/2b64de2008404c17396267e110227393ebf298f8 that applies clean onto current integration and follows the "drop default" alternative, I really agree it is better than the original one proposed (add default to install.xml). As far a no new ratings can arrive with empty component/area, let's enforce it. Ciao Note: I have not updated the information above to have both alternatives available, the new (drop) one is: repo: git://github.com/stronk7/moodle.git branch: MDL-27721 _drop_default_integration_ready diff: https://github.com/stronk7/moodle/commit/2b64de2008404c17396267e110227393ebf298f8
        Hide
        Andrew Davis added a comment -

        Looks good to me. The only trivial problem I spotted is:

        // Define index uniqueuserrating (not unique) to be dropped form rating

        should be "from". Not "form".

        Show
        Andrew Davis added a comment - Looks good to me. The only trivial problem I spotted is: // Define index uniqueuserrating (not unique) to be dropped form rating should be "from". Not "form".
        Hide
        Sam Hemelryk added a comment -

        Thanks guys this has been integrated now.
        I did wonder whether we should remove the upgrade code in the previous block that is adding the index just to speed up the upgrade for those who haven't already upgraded yet.
        I suppose if we decide to do that we can easily create a new issue. What do you think?

        Show
        Sam Hemelryk added a comment - Thanks guys this has been integrated now. I did wonder whether we should remove the upgrade code in the previous block that is adding the index just to speed up the upgrade for those who haven't already upgraded yet. I suppose if we decide to do that we can easily create a new issue. What do you think?
        Hide
        Andrew Davis added a comment -

        +1 for letting sleeping dogs lie unless we really have to. Modifying old upgrade code makes me uneasy.

        Show
        Andrew Davis added a comment - +1 for letting sleeping dogs lie unless we really have to. Modifying old upgrade code makes me uneasy.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        I think that dropping the step: 2011052300.01 is 100% safe under all possible combinations.

        Also, it has been "dev" til now so I think we can do that easily than being stable.

        The 2011060500 step will always do its work properly (dropping conditionally in case the site already had the index) and creating later, and really it can be a benefit in sites with zillions of ratings.

        So +1 for new issue and delete 2011052300.01 and add comment in 2011060500 explaining why that drop is there or, for sure, we'll forget it in the future.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - I think that dropping the step: 2011052300.01 is 100% safe under all possible combinations. Also, it has been "dev" til now so I think we can do that easily than being stable. The 2011060500 step will always do its work properly (dropping conditionally in case the site already had the index) and creating later, and really it can be a benefit in sites with zillions of ratings. So +1 for new issue and delete 2011052300.01 and add comment in 2011060500 explaining why that drop is there or, for sure, we'll forget it in the future. Ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Tested, now install vs upgrade comparison of DB schema matches 100% thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Tested, now install vs upgrade comparison of DB schema matches 100% thanks!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        super! Thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - super! Thanks!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: