From dafc92b45480755d33891025db9693faeeaac9e4 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Tue, 9 May 2023 15:06:51 +0100 Subject: [PATCH] MDL-78170 rating: consistently calculate average rating aggregation. Ensure it's always cast by the database as a float, to avoid loss of precision. --- rating/lib.php | 18 +++++++++++++++--- rating/tests/rating_test.php | 26 +++++++++----------------- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/rating/lib.php b/rating/lib.php index d20191d7ff8..a8cc8cca4c1 100644 --- a/rating/lib.php +++ b/rating/lib.php @@ -516,7 +516,13 @@ class rating_manager { // Ratings are not enabled. return $options->items; } + + // Ensure average aggregation returns float. $aggregatestr = $this->get_aggregation_method($options->aggregate); + $aggregatefield = 'r.rating'; + if ($aggregatestr === 'AVG') { + $aggregatefield = "1.0 * {$aggregatefield}"; + } // Default the userid to the current user if it is not set. if (empty($options->userid)) { @@ -559,7 +565,7 @@ class rating_manager { ORDER BY r.itemid"; $userratings = $DB->get_records_sql($sql, $params); - $sql = "SELECT r.itemid, $aggregatestr(r.rating) AS aggrrating, COUNT(r.rating) AS numratings + $sql = "SELECT r.itemid, {$aggregatestr}({$aggregatefield}) AS aggrrating, COUNT(r.rating) AS numratings FROM {rating} r WHERE r.contextid = :contextid AND r.itemid {$itemidtest} AND @@ -820,7 +826,13 @@ class rating_manager { $itemtable = $options->itemtable; $itemtableusercolumn = $options->itemtableusercolumn; $scaleid = $options->scaleid; - $aggregationstring = $this->get_aggregation_method($options->aggregationmethod); + + // Ensure average aggregation returns float. + $aggregationstring = $this->get_aggregation_method($options->aggregationmethod); + $aggregationfield = 'r.rating'; + if ($aggregationstring === 'AVG') { + $aggregationfield = "1.0 * {$aggregationfield}"; + } // If userid is not 0 we only want the grade for a single user. $singleuserwhere = ''; @@ -832,7 +844,7 @@ class rating_manager { // MDL-24648 The where line used to be "WHERE (r.contextid is null or r.contextid=:contextid)". // r.contextid will be null for users who haven't been rated yet. // No longer including users who haven't been rated to reduce memory requirements. - $sql = "SELECT u.id as id, u.id AS userid, $aggregationstring(r.rating) AS rawgrade + $sql = "SELECT u.id as id, u.id AS userid, {$aggregationstring}({$aggregationfield}) AS rawgrade FROM {user} u LEFT JOIN {{$itemtable}} i ON u.id=i.{$itemtableusercolumn} LEFT JOIN {rating} r ON r.itemid=i.id diff --git a/rating/tests/rating_test.php b/rating/tests/rating_test.php index f60107dc865..b2fa6a55d15 100644 --- a/rating/tests/rating_test.php +++ b/rating/tests/rating_test.php @@ -14,15 +14,6 @@ // You should have received a copy of the GNU General Public License // along with Moodle. If not, see . -/** - * Unit tests for rating/lib.php - * - * @package core_rating - * @category test - * @copyright 2011 onwards Eloy Lafuente (stronk7) {@link http://stronk7.com} - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ - namespace core_rating; use rating_manager; @@ -41,6 +32,7 @@ require_once($CFG->dirroot . '/rating/lib.php'); * @category test * @copyright 2011 onwards Eloy Lafuente (stronk7) {@link http://stronk7.com} * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \rating */ class rating_test extends \advanced_testcase { @@ -103,7 +95,7 @@ class rating_test extends \advanced_testcase { 'ratingarea' => 'post', 'itemid' => 2, 'scaleid' => 10, - 'rating' => 5, + 'rating' => 4, 'userid' => 3, 'timecreated' => 1, 'timemodified' => 1), @@ -167,7 +159,7 @@ class rating_test extends \advanced_testcase { // Note that $result[0]->rating->rating is somewhat random. // We didn't supply a user ID so $USER was used which will vary depending on who runs the tests. - // Get results for items of user 2 (expected average 1 + 5 / 2 = 3). + // Get results for items of user 2 (expected average 1 + 4 / 2 = 2.5). $toptions = (object)array_merge($defaultoptions, array('items' => $user2posts)); $result = $rm->get_ratings($toptions); $this->assertEquals(count($result), count($user2posts)); @@ -175,7 +167,7 @@ class rating_test extends \advanced_testcase { $this->assertEquals($result[0]->userid, $user2posts[0]->userid); $this->assertEquals($result[0]->message, $user2posts[0]->message); $this->assertEquals($result[0]->rating->count, 2); - $this->assertEquals($result[0]->rating->aggregate, 3); + $this->assertEquals($result[0]->rating->aggregate, 2.5); // Note that $result[0]->rating->rating is somewhat random. // We didn't supply a user ID so $USER was used which will vary depending on who runs the tests. @@ -191,7 +183,7 @@ class rating_test extends \advanced_testcase { // Note that $result[0]->rating->rating is somewhat random. // We didn't supply a user ID so $USER was used which will vary depending on who runs the tests. - // Get results for items of user 1 & 2 together (expected averages are 2 and 3, as tested above). + // Get results for items of user 1 & 2 together (expected averages are 2 and 2.5, as tested above). $posts = array_merge($user1posts, $user2posts); $toptions = (object)array_merge($defaultoptions, array('items' => $posts)); $result = $rm->get_ratings($toptions); @@ -208,7 +200,7 @@ class rating_test extends \advanced_testcase { $this->assertEquals($result[1]->userid, $posts[1]->userid); $this->assertEquals($result[1]->message, $posts[1]->message); $this->assertEquals($result[1]->rating->count, 2); - $this->assertEquals($result[1]->rating->aggregate, 3); + $this->assertEquals($result[1]->rating->aggregate, 2.5); // Note that $result[0]->rating->rating is somewhat random. // We didn't supply a user ID so $USER was used which will vary depending on who runs the tests. @@ -256,7 +248,7 @@ class rating_test extends \advanced_testcase { $this->assertEquals($result[1]->userid, $posts[1]->userid); $this->assertEquals($result[1]->message, $posts[1]->message); $this->assertEquals($result[1]->rating->count, 2); - $this->assertEquals($result[1]->rating->aggregate, 3); + $this->assertEquals($result[1]->rating->aggregate, 2.5); $this->assertEquals($result[0]->rating->rating, 3); // User 3 rated user 2 "5". $this->assertEquals($result[1]->rating->userid, $toptions->userid); // Must be the passed userid. @@ -271,7 +263,7 @@ class rating_test extends \advanced_testcase { $this->assertNull($result[0]->rating->rating); $this->assertEquals($result[0]->rating->aggregate, 2); // Should still get the aggregate. - // Get results for items of user 2 (expected average 1 + 5 / 2 = 3). + // Get results for items of user 2 (expected average 1 + 4 / 2 = 2.5). // Supplying the user id of the user who owns the items so no rating should be found. $toptions = (object)array_merge($defaultoptions, array('items' => $user2posts)); $toptions->userid = 2; // User 2 viewing the ratings of their own item. @@ -279,7 +271,7 @@ class rating_test extends \advanced_testcase { // These should be null as the user is viewing their own item and thus cannot rate. $this->assertNull($result[0]->rating->userid); $this->assertNull($result[0]->rating->rating); - $this->assertEquals($result[0]->rating->aggregate, 3); // Should still get the aggregate. + $this->assertEquals($result[0]->rating->aggregate, 2.5); // Should still get the aggregate. } /**