Moodle
  1. Moodle
  2. MDL-28465

Split the get_ratings() query into two to avoid potential postgres aggregation problems

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.3
    • Fix Version/s: 2.0.5
    • Component/s: Ratings
    • Labels:
      None
    • Database:
      PostgreSQL
    • Testing Instructions:
      Hide

      Go to a 2.0 stable site that is using a postgres database. Go to a forum and submit a rating.

      Show
      Go to a 2.0 stable site that is using a postgres database. Go to a forum and submit a rating.
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      In MDL-27845 the query in get_ratings() was split into two in 2.1 and master (will be 2.2) however it wasn't done in 2.0 as the code is substantially different and I really wanted to get the code integrated and tested.

      This issue is to remind me to go and implement the same idea in 2.0 stable.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Sam Hemelryk added a comment -

            Hi Andrew,

            Took me a sec to work out what was going on with MDL-27845.
            The changes look OK too me, would be great to tidy up the commented get_record lines, and perhaps the whitespace on the changed lines.

            As a note for the integrator: check out the changes on MDL-27845 for these changes on 21 and master. Also as part of the recent changes to the ratings system all the nasty whitespace and formatting has been tidied up in 21 and master, it's still there in 20 but I don't think we need to worry about it.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Andrew, Took me a sec to work out what was going on with MDL-27845 . The changes look OK too me, would be great to tidy up the commented get_record lines, and perhaps the whitespace on the changed lines. As a note for the integrator: check out the changes on MDL-27845 for these changes on 21 and master. Also as part of the recent changes to the ratings system all the nasty whitespace and formatting has been tidied up in 21 and master, it's still there in 20 but I don't think we need to worry about it. Cheers Sam
            Hide
            Sam Hemelryk added a comment -

            Whoops commented instead of finishing peer-review.

            Show
            Sam Hemelryk added a comment - Whoops commented instead of finishing peer-review.
            Hide
            Andrew Davis added a comment -

            I've removed the commented out get_record() lines and fixed up some white space issues.

            Show
            Andrew Davis added a comment - I've removed the commented out get_record() lines and fixed up some white space issues.
            Hide
            Petr Skoda added a comment -

            SELECT r.itemid, r.id, r.userid, r.scaleid, r.rating AS usersrating
            ...
            $userratings = $DB->get_records_sql($sql, $params);
            

            is problematic because r.itemid is not guaranteed to be unique, I understand the conditions may make it kind of unique given the current ratings logic, there might be some race condition or double click trouble somewhere or somebody decides to allow more ratings per user later.

            Show
            Petr Skoda added a comment - SELECT r.itemid, r.id, r.userid, r.scaleid, r.rating AS usersrating ... $userratings = $DB->get_records_sql($sql, $params); is problematic because r.itemid is not guaranteed to be unique, I understand the conditions may make it kind of unique given the current ratings logic, there might be some race condition or double click trouble somewhere or somebody decides to allow more ratings per user later.
            Hide
            Andrew Davis added a comment -

            r.itemid is guaranteed to be unique given the current rating logic. However I've reshuffled the query a little to ensure that it will still work even if we eventually allow a user to rate an item multiple times.

            Show
            Andrew Davis added a comment - r.itemid is guaranteed to be unique given the current rating logic. However I've reshuffled the query a little to ensure that it will still work even if we eventually allow a user to rate an item multiple times.
            Hide
            Aparup Banerjee added a comment -

            Andrew, it looks sane to me code wise

            (personally i would rename $itemidtest to something like $itemidsql, 'test' always gives me then boolean test impression)

            cheers,
            Aparup

            Show
            Aparup Banerjee added a comment - Andrew, it looks sane to me code wise (personally i would rename $itemidtest to something like $itemidsql, 'test' always gives me then boolean test impression) cheers, Aparup
            Hide
            Andrew Davis added a comment -

            I wont bother altering that variable name for this issue but will keep that in mind in the future

            Show
            Andrew Davis added a comment - I wont bother altering that variable name for this issue but will keep that in mind in the future
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Note to integrator: This is the solution for 20_STABLE. MDL-27845 is the corresponding solution for 21_STABLE and master so should be integrated at the same time.

            Show
            Eloy Lafuente (stronk7) added a comment - Note to integrator: This is the solution for 20_STABLE. MDL-27845 is the corresponding solution for 21_STABLE and master so should be integrated at the same time.
            Hide
            Petr Skoda added a comment -

            Integrated, thanks.

            Please test in all 4 supported databases, thanks.

            Show
            Petr Skoda added a comment - Integrated, thanks. Please test in all 4 supported databases, thanks.
            Hide
            Rajesh Taneja added a comment -

            Works as expected
            Thanks for fixing this Andrew.
            Note:
            Coding style for few if and for statements has missing space. As rating/lib.php has not followed any coding style, hence passing it. Probably nice to create an improvement/bug to keep a note of it.

            Show
            Rajesh Taneja added a comment - Works as expected Thanks for fixing this Andrew. Note: Coding style for few if and for statements has missing space. As rating/lib.php has not followed any coding style, hence passing it. Probably nice to create an improvement/bug to keep a note of it.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Closing, this is now part of Moodle. Big thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Closing, this is now part of Moodle. Big thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: