From 4b11af960789ba40393e25c1aedaa65e71859247 Mon Sep 17 00:00:00 2001 From: Frederic Massart Date: Tue, 6 Oct 2015 12:56:13 +0800 Subject: [PATCH 1/2] MDL-51664 external: Validate courses does not refetch prefetched courses --- lib/externallib.php | 13 ++++++-- lib/tests/externallib_test.php | 61 ++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/lib/externallib.php b/lib/externallib.php index 8f20b35456b..5de67eaee61 100644 --- a/lib/externallib.php +++ b/lib/externallib.php @@ -940,21 +940,28 @@ class external_util { * Validate a list of courses, returning the complete course objects for valid courses. * * @param array $courseids A list of course ids + * @param array $courses An array of courses already pre-fetched. * @return array An array of courses and the validation warnings */ - public static function validate_courses($courseids) { + public static function validate_courses($courseids, $courses = array()) { // Delete duplicates. $courseids = array_unique($courseids); - $courses = array(); $warnings = array(); + // Remove courses which are not even requested. + $courses = array_intersect_key($courses, array_flip($courseids)); + foreach ($courseids as $cid) { // Check the user can function in this context. try { $context = context_course::instance($cid); external_api::validate_context($context); - $courses[$cid] = get_course($cid); + + if (!isset($courses[$cid])) { + $courses[$cid] = get_course($cid); + } } catch (Exception $e) { + unset($courses[$cid]); $warnings[] = array( 'item' => 'course', 'itemid' => $cid, diff --git a/lib/tests/externallib_test.php b/lib/tests/externallib_test.php index 0caa69eb8bf..6399eaea6e8 100644 --- a/lib/tests/externallib_test.php +++ b/lib/tests/externallib_test.php @@ -264,6 +264,67 @@ class core_externallib_testcase extends advanced_testcase { $this->setExpectedException('invalid_parameter_exception'); test_exernal_api::get_context_wrapper(array('roleid' => 3, 'userid' => $USER->id, 'instanceid' => $course->id)); } + + public function test_validate_courses() { + $this->resetAfterTest(true); + + $c1 = $this->getDataGenerator()->create_course(); + $c2 = $this->getDataGenerator()->create_course(); + $c3 = $this->getDataGenerator()->create_course(); + $u1 = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($u1->id, $c1->id); + $courseids = array($c1->id, $c2->id, $c3->id); + + $this->setAdminUser(); + list($courses, $warnings) = external_util::validate_courses($courseids); + $this->assertEmpty($warnings); + $this->assertCount(3, $courses); + $this->assertArrayHasKey($c1->id, $courses); + $this->assertArrayHasKey($c2->id, $courses); + $this->assertArrayHasKey($c3->id, $courses); + $this->assertEquals($c1->id, $courses[$c1->id]->id); + $this->assertEquals($c2->id, $courses[$c2->id]->id); + $this->assertEquals($c3->id, $courses[$c3->id]->id); + + $this->setUser($u1); + list($courses, $warnings) = external_util::validate_courses($courseids); + $this->assertCount(2, $warnings); + $this->assertEquals($c2->id, $warnings[0]['itemid']); + $this->assertEquals($c3->id, $warnings[1]['itemid']); + $this->assertCount(1, $courses); + $this->assertArrayHasKey($c1->id, $courses); + $this->assertArrayNotHasKey($c2->id, $courses); + $this->assertArrayNotHasKey($c3->id, $courses); + $this->assertEquals($c1->id, $courses[$c1->id]->id); + } + + /** + * Validate courses can re-use an array of prefetched courses. + */ + public function test_validate_courses_prefetch() { + $this->resetAfterTest(true); + + $c1 = $this->getDataGenerator()->create_course(); + $c2 = $this->getDataGenerator()->create_course(); + $c3 = $this->getDataGenerator()->create_course(); + $c4 = $this->getDataGenerator()->create_course(); + $u1 = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($u1->id, $c1->id); + $this->getDataGenerator()->enrol_user($u1->id, $c2->id); + + $courseids = array($c1->id, $c2->id, $c3->id); + $courses = array($c2->id => $c2, $c3->id => $c3, $c4->id => $c4); + + $this->setUser($u1); + list($courses, $warnings) = external_util::validate_courses($courseids, $courses); + $this->assertCount(2, $courses); + $this->assertCount(1, $warnings); + $this->assertArrayHasKey($c1->id, $courses); + $this->assertSame($c2, $courses[$c2->id]); + $this->assertArrayNotHasKey($c3->id, $courses); + // The extra course passed is not returned. + $this->assertArrayNotHasKey($c4->id, $courses); + } } /* From 583b02e4059b67af95db947cd6eaf4fb3088909c Mon Sep 17 00:00:00 2001 From: Frederic Massart Date: Tue, 6 Oct 2015 13:25:27 +0800 Subject: [PATCH 2/2] MDL-51664 external: Update usage of external_util::validate_courses --- mod/book/classes/external.php | 6 ++++-- mod/chat/classes/external.php | 6 ++++-- mod/choice/classes/external.php | 6 ++++-- mod/forum/externallib.php | 6 ++++-- mod/imscp/classes/external.php | 6 ++++-- mod/scorm/classes/external.php | 6 ++++-- 6 files changed, 24 insertions(+), 12 deletions(-) diff --git a/mod/book/classes/external.php b/mod/book/classes/external.php index 6ae937dc7f2..34d1493d515 100644 --- a/mod/book/classes/external.php +++ b/mod/book/classes/external.php @@ -181,14 +181,16 @@ class mod_book_external extends external_api { $params = self::validate_parameters(self::get_books_by_courses_parameters(), array('courseids' => $courseids)); + $courses = array(); if (empty($params['courseids'])) { - $params['courseids'] = array_keys(enrol_get_my_courses()); + $courses = enrol_get_my_courses(); + $params['courseids'] = array_keys($courses); } // Ensure there are courseids to loop through. if (!empty($params['courseids'])) { - list($courses, $warnings) = external_util::validate_courses($params['courseids']); + list($courses, $warnings) = external_util::validate_courses($params['courseids'], $courses); // Get the books in this course, this function checks users visibility permissions. // We can avoid then additional validate_context calls. diff --git a/mod/chat/classes/external.php b/mod/chat/classes/external.php index ba542295996..f78dd1820dd 100644 --- a/mod/chat/classes/external.php +++ b/mod/chat/classes/external.php @@ -523,14 +523,16 @@ class mod_chat_external extends external_api { $params = self::validate_parameters(self::get_chats_by_courses_parameters(), array('courseids' => $courseids)); + $courses = array(); if (empty($params['courseids'])) { - $params['courseids'] = array_keys(enrol_get_my_courses()); + $courses = enrol_get_my_courses(); + $params['courseids'] = array_keys($courses); } // Ensure there are courseids to loop through. if (!empty($params['courseids'])) { - list($courses, $warnings) = external_util::validate_courses($params['courseids']); + list($courses, $warnings) = external_util::validate_courses($params['courseids'], $courses); // Get the chats in this course, this function checks users visibility permissions. // We can avoid then additional validate_context calls. diff --git a/mod/choice/classes/external.php b/mod/choice/classes/external.php index 306dda3af27..a3171ff1015 100644 --- a/mod/choice/classes/external.php +++ b/mod/choice/classes/external.php @@ -480,14 +480,16 @@ class mod_choice_external extends external_api { $params = self::validate_parameters(self::get_choices_by_courses_parameters(), array('courseids' => $courseids)); + $courses = array(); if (empty($params['courseids'])) { - $params['courseids'] = array_keys(enrol_get_my_courses()); + $courses = enrol_get_my_courses(); + $params['courseids'] = array_keys($courses); } // Ensure there are courseids to loop through. if (!empty($params['courseids'])) { - list($courses, $warnings) = external_util::validate_courses($params['courseids']); + list($courses, $warnings) = external_util::validate_courses($params['courseids'], $courses); // Get the choices in this course, this function checks users visibility permissions. // We can avoid then additional validate_context calls. diff --git a/mod/forum/externallib.php b/mod/forum/externallib.php index d41444d71a6..564060b6518 100644 --- a/mod/forum/externallib.php +++ b/mod/forum/externallib.php @@ -60,8 +60,10 @@ class mod_forum_external extends external_api { $params = self::validate_parameters(self::get_forums_by_courses_parameters(), array('courseids' => $courseids)); + $courses = array(); if (empty($params['courseids'])) { - $params['courseids'] = array_keys(enrol_get_my_courses()); + $courses = enrol_get_my_courses(); + $params['courseids'] = array_keys($courses); } // Array to store the forums to return. @@ -71,7 +73,7 @@ class mod_forum_external extends external_api { // Ensure there are courseids to loop through. if (!empty($params['courseids'])) { - list($courses, $warnings) = external_util::validate_courses($params['courseids']); + list($courses, $warnings) = external_util::validate_courses($params['courseids'], $courses); // Get the forums in this course. This function checks users visibility permissions. $forums = get_all_instances_in_courses("forum", $courses); diff --git a/mod/imscp/classes/external.php b/mod/imscp/classes/external.php index 5a092dea773..4ae2fdda8c3 100644 --- a/mod/imscp/classes/external.php +++ b/mod/imscp/classes/external.php @@ -136,14 +136,16 @@ class mod_imscp_external extends external_api { $params = self::validate_parameters(self::get_imscps_by_courses_parameters(), array('courseids' => $courseids)); + $courses = array(); if (empty($params['courseids'])) { - $params['courseids'] = array_keys(enrol_get_my_courses()); + $courses = enrol_get_my_courses(); + $params['courseids'] = array_keys($courses); } // Ensure there are courseids to loop through. if (!empty($params['courseids'])) { - list($courses, $warnings) = external_util::validate_courses($params['courseids']); + list($courses, $warnings) = external_util::validate_courses($params['courseids'], $courses); // Get the imscps in this course, this function checks users visibility permissions. // We can avoid then additional validate_context calls. diff --git a/mod/scorm/classes/external.php b/mod/scorm/classes/external.php index 38502828a25..98e6fafeb73 100644 --- a/mod/scorm/classes/external.php +++ b/mod/scorm/classes/external.php @@ -640,14 +640,16 @@ class mod_scorm_external extends external_api { $params = self::validate_parameters(self::get_scorms_by_courses_parameters(), array('courseids' => $courseids)); + $courses = array(); if (empty($params['courseids'])) { - $params['courseids'] = array_keys(enrol_get_my_courses()); + $courses = enrol_get_my_courses(); + $params['courseids'] = array_keys($courses); } // Ensure there are courseids to loop through. if (!empty($params['courseids'])) { - list($courses, $warnings) = external_util::validate_courses($params['courseids']); + list($courses, $warnings) = external_util::validate_courses($params['courseids'], $courses); // Get the scorms in this course, this function checks users visibility permissions. // We can avoid then additional validate_context calls.