mirror_php/moodle
1
0
Fork 0
mirror of https://github.com/moodle/moodle.git synced 2025-01-17 13:38:32 +01:00
Code Issues Projects Releases Wiki Activity

MDL-78170 rating: consistently calculate average rating aggregation.

Browse Source 
Ensure it's always cast by the database as a float, to avoid loss of
precision.
This commit is contained in:
Paul Holden 2023-05-09 15:06:51 +01:00
parent 5d320dd7d1
commit dafc92b454
No known key found for this signature in database
GPG Key ID: A81A96D6045F6164
2 changed files with 24 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
rating/lib.php
View File

@ -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

26
rating/tests/rating_test.php
View File

@ -14,15 +14,6 @@
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.
/**
* 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.
}
/**