Skip to content

Commit

Permalink
MDL-64652 rating: Add optional inner join option to sql
Browse files Browse the repository at this point in the history
  • Loading branch information
@andrewnicols
andrewnicols committed Mar 5, 2019
1 parent ab45aa7 commit 9f3bf96
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 9 deletions.
4 changes: 4 additions & 0 deletions lib/upgrade.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ attribute on forms to avoid collisions in forms loaded in AJAX requests.
to be updated or queued as required. This new functionality can be found in \core\task\manager::reschedule_or_queue_adhoc_task.
* Icons are displayed for screen readers unless they have empty alt text (aria-hidden). Do not provide an icon with alt text immediately beside an element with exactly the same text.
* admin_settingpage has a new function hide_if(), modeled after the same functionality in the forms library. This allows admin settings to be dynamically hidden based on the values of other settings.
* The \core_rating provider's get_sql_join function now accepts an optional $innerjoin parameter.
It is recommended that privacy providers using this function call rewrite any long query into a number of separate
calls to add_from_sql for improved performance, and that the new argument is used.
This will allow queries to remain backwards-compatible with older versions of Moodle but will have significantly better performance in version supporting the innerjoin parameter.

=== 3.6 ===

Expand Down
33 changes: 24 additions & 9 deletions rating/classes/privacy/provider.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,26 +129,41 @@ public static function export_area_ratings(
/**
* Get the SQL required to find all submission items where this user has had any involvements.
*
* If possible an inner join should be used.
*
* @param string $alias The name of the table alias to use.
* @param string $component The na eof the component to fetch ratings for.
* @param string $ratingarea The rating area to fetch results for.
* @param string $itemidjoin The right-hand-side of the JOIN ON clause.
* @param int $userid The ID of the user being stored.
* @param bool $innerjoin Whether to use an inner join (preferred)
* @return \stdClass
*/
public static function get_sql_join($alias, $component, $ratingarea, $itemidjoin, $userid) {
public static function get_sql_join($alias, $component, $ratingarea, $itemidjoin, $userid, $innerjoin = false) {
static $count = 0;
$count++;

// Join the rating table with the specified alias and the relevant join params.
$join = "LEFT JOIN {rating} {$alias} ON ";
$join .= "{$alias}.userid = :ratinguserid{$count} AND ";
$join .= "{$alias}.component = :ratingcomponent{$count} AND ";
$join .= "{$alias}.ratingarea = :ratingarea{$count} AND ";
$join .= "{$alias}.itemid = {$itemidjoin}";
$userwhere = '';

if ($innerjoin) {
// Join the rating table with the specified alias and the relevant join params.
$join = "JOIN {rating} {$alias} ON ";
$join .= "{$alias}.itemid = {$itemidjoin}";

// Match against the specified user.
$userwhere = "{$alias}.id IS NOT NULL";
$userwhere .= "{$alias}.userid = :ratinguserid{$count} AND ";
$userwhere .= "{$alias}.component = :ratingcomponent{$count} AND ";
$userwhere .= "{$alias}.ratingarea = :ratingarea{$count}";
} else {
// Join the rating table with the specified alias and the relevant join params.
$join = "LEFT JOIN {rating} {$alias} ON ";
$join .= "{$alias}.userid = :ratinguserid{$count} AND ";
$join .= "{$alias}.component = :ratingcomponent{$count} AND ";
$join .= "{$alias}.ratingarea = :ratingarea{$count} AND ";
$join .= "{$alias}.itemid = {$itemidjoin}";

// Match against the specified user.
$userwhere = "{$alias}.id IS NOT NULL";
}

$params = [
'ratingcomponent' . $count => $component,
Expand Down
88 changes: 88 additions & 0 deletions rating/tests/privacy_provider_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,94 @@ public function test_get_sql_join() {
$this->assertFalse(isset($courses[$course3->id]));
}

/**
* Ensure that the get_sql_join function returns valid SQL which returns the correct list of rated itemids.
* This makes use of the optional inner join argument.
*/
public function test_get_sql_join_inner() {
global $DB;
$this->resetAfterTest();

$course1 = $this->getDataGenerator()->create_course();
$course2 = $this->getDataGenerator()->create_course();
$course3 = $this->getDataGenerator()->create_course();

$u1 = $this->getDataGenerator()->create_user();
$u2 = $this->getDataGenerator()->create_user();
$u3 = $this->getDataGenerator()->create_user();

// Rate the courses.
$rm = new rating_manager();
$ratingoptions = (object) [
'component' => 'core_course',
'ratingarea' => 'course',
'scaleid' => 100,
];

// Rate all courses as u1, and something else in the same context.
$this->rate_as_user($u1->id, 'core_course', 'course', $course1->id, \context_course::instance($course1->id), 25);
$this->rate_as_user($u1->id, 'core_course', 'course', $course2->id, \context_course::instance($course2->id), 50);
$this->rate_as_user($u1->id, 'core_course', 'course', $course3->id, \context_course::instance($course3->id), 75);
$this->rate_as_user($u1->id, 'core_course', 'files', $course3->id, \context_course::instance($course3->id), 99);

// Rate course2 as u2, and something else in a different context/component..
$this->rate_as_user($u2->id, 'core_course', 'course', $course2->id, \context_course::instance($course2->id), 90);
$this->rate_as_user($u2->id, 'user', 'user', $u3->id, \context_user::instance($u3->id), 10);

// Return any course which the u1 has rated.
// u1 rated all three courses.
$ratingquery = provider::get_sql_join('r', 'core_course', 'course', 'c.id', $u1->id, true);
$sql = "SELECT c.id FROM {course} c {$ratingquery->join} WHERE {$ratingquery->userwhere}";
$courses = $DB->get_records_sql($sql, $ratingquery->params);

$this->assertCount(3, $courses);
$this->assertTrue(isset($courses[$course1->id]));
$this->assertTrue(isset($courses[$course2->id]));
$this->assertTrue(isset($courses[$course3->id]));

// User u1 rated files in course 3 only.
$ratingquery = provider::get_sql_join('r', 'core_course', 'files', 'c.id', $u1->id, true);
$sql = "SELECT c.id FROM {course} c {$ratingquery->join} WHERE {$ratingquery->userwhere}";
$courses = $DB->get_records_sql($sql, $ratingquery->params);

$this->assertCount(1, $courses);
$this->assertFalse(isset($courses[$course1->id]));
$this->assertFalse(isset($courses[$course2->id]));
$this->assertTrue(isset($courses[$course3->id]));

// Return any course which the u2 has rated.
// User u2 rated only course 2.
$ratingquery = provider::get_sql_join('r', 'core_course', 'course', 'c.id', $u2->id, true);
$sql = "SELECT c.id FROM {course} c {$ratingquery->join} WHERE {$ratingquery->userwhere}";
$courses = $DB->get_records_sql($sql, $ratingquery->params);

$this->assertCount(1, $courses);
$this->assertFalse(isset($courses[$course1->id]));
$this->assertTrue(isset($courses[$course2->id]));
$this->assertFalse(isset($courses[$course3->id]));

// User u2 rated u3.
$ratingquery = provider::get_sql_join('r', 'user', 'user', 'u.id', $u2->id, true);
$sql = "SELECT u.id FROM {user} u {$ratingquery->join} WHERE {$ratingquery->userwhere}";
$users = $DB->get_records_sql($sql, $ratingquery->params);

$this->assertCount(1, $users);
$this->assertFalse(isset($users[$u1->id]));
$this->assertFalse(isset($users[$u2->id]));
$this->assertTrue(isset($users[$u3->id]));

// Return any course which the u3 has rated.
// User u3 did not rate anything.
$ratingquery = provider::get_sql_join('r', 'core_course', 'course', 'c.id', $u3->id, true);
$sql = "SELECT c.id FROM {course} c {$ratingquery->join} WHERE {$ratingquery->userwhere}";
$courses = $DB->get_records_sql($sql, $ratingquery->params);

$this->assertCount(0, $courses);
$this->assertFalse(isset($courses[$course1->id]));
$this->assertFalse(isset($courses[$course2->id]));
$this->assertFalse(isset($courses[$course3->id]));
}

/**
* Ensure that export_area_ratings exports all ratings that a user has made, and all ratings for a users own content.
*/
Expand Down

0 comments on commit 9f3bf96

Please sign in to comment.