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
    • Rank:
      18124

      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.

        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 Škoda 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 Škoda 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 Škoda added a comment -

          Integrated, thanks.

          Please test in all 4 supported databases, thanks.

          Show
          Petr Škoda 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: