From 8077107565ff52ccd6b00d7807f844e064296446 Mon Sep 17 00:00:00 2001 From: Juan Leyva Date: Fri, 10 Feb 2017 17:38:28 +0100 Subject: [PATCH 1/2] MDL-57918 mod_data: Minor refactors for the new WS - Move entry permission code to function - Retrieve user record when missing --- mod/data/lib.php | 4 ++++ mod/data/locallib.php | 34 +++++++++++++++++++++++++++------- mod/data/view.php | 8 +++----- 3 files changed, 34 insertions(+), 12 deletions(-) diff --git a/mod/data/lib.php b/mod/data/lib.php index 81cfb9102cb..f74f6a2c633 100644 --- a/mod/data/lib.php +++ b/mod/data/lib.php @@ -1335,6 +1335,10 @@ function data_print_template($template, $records, $data, $search='', $page=0, $r $patterns[] = '##userpicture##'; $ruser = user_picture::unalias($record, null, 'userid'); + // If the record didn't come with user data, retrieve the user from database. + if (!isset($ruser->picture)) { + $ruser = core_user::get_user($record->userid); + } $replacement[] = $OUTPUT->user_picture($ruser, array('courseid' => $data->course)); $patterns[]='##export##'; diff --git a/mod/data/locallib.php b/mod/data/locallib.php index 368b66815bb..ebf587ba970 100644 --- a/mod/data/locallib.php +++ b/mod/data/locallib.php @@ -1024,13 +1024,10 @@ function data_search_entries($data, $cm, $context, $mode, $currentgroup, $search if (!$records = $DB->get_records_sql($sqlselect, $allparams, $page * $nowperpage, $nowperpage)) { // Nothing to show! if ($record) { // Something was requested so try to show that at least (bug 5132) - if ($canmanageentries || empty($data->approval) || - $record->approved || (isloggedin() && $record->userid == $USER->id)) { - if (!$currentgroup || $record->groupid == $currentgroup || $record->groupid == 0) { - // OK, we can show this one - $records = array($record->id => $record); - $totalcount = 1; - } + if (data_can_view_record($data, $record, $currentgroup, $canmanageentries)) { + // OK, we can show this one + $records = array($record->id => $record); + $totalcount = 1; } } @@ -1038,3 +1035,26 @@ function data_search_entries($data, $cm, $context, $mode, $currentgroup, $search return [$records, $maxcount, $totalcount, $page, $nowperpage, $sort, $mode]; } + +/** + * Check if the current user can view the given record. + * + * @param stdClass $data database record + * @param stdClass $record the record (entry) to check + * @param int $currentgroup current group + * @param bool $canmanageentries if the user can manage entries + * @return bool true if the user can view the entry + * @since Moodle 3.3 + */ +function data_can_view_record($data, $record, $currentgroup, $canmanageentries) { + global $USER; + + if ($canmanageentries || empty($data->approval) || + $record->approved || (isloggedin() && $record->userid == $USER->id)) { + + if (!$currentgroup || $record->groupid == $currentgroup || $record->groupid == 0) { + return true; + } + } + return false; +} diff --git a/mod/data/view.php b/mod/data/view.php index 6e404f38091..2f5e648656c 100644 --- a/mod/data/view.php +++ b/mod/data/view.php @@ -327,11 +327,9 @@ $canmanageentries = has_capability('mod/data:manageentries', $context); - // detect entries not approved yet and show hint instead of not found error - if ($record and $data->approval and !$record->approved and $record->userid != $USER->id and !$canmanageentries) { - if (!$currentgroup or $record->groupid == $currentgroup or $record->groupid == 0) { - print_error('notapproved', 'data'); - } + // Detect entries not approved yet and show hint instead of not found error. + if ($record and !data_can_view_record($data, $record, $currentgroup, $canmanageentries)) { + print_error('notapproved', 'data'); } echo $OUTPUT->heading(format_string($data->name), 2); From 771effef8d83d157e611386fdfe0ae16ecdaee6e Mon Sep 17 00:00:00 2001 From: Juan Leyva Date: Fri, 10 Feb 2017 17:39:01 +0100 Subject: [PATCH 2/2] MDL-57918 mod_data: New WS mod_data_get_entry --- mod/data/classes/external.php | 87 ++++++++++++ mod/data/classes/external/record_exporter.php | 13 +- mod/data/db/services.php | 8 ++ mod/data/tests/externallib_test.php | 128 +++++++++++++++++- mod/data/version.php | 2 +- 5 files changed, 228 insertions(+), 10 deletions(-) diff --git a/mod/data/classes/external.php b/mod/data/classes/external.php index 3d07a429665..6d27f17ade7 100644 --- a/mod/data/classes/external.php +++ b/mod/data/classes/external.php @@ -480,4 +480,91 @@ class mod_data_external extends external_api { ) ); } + + /** + * Returns description of method parameters + * + * @return external_function_parameters + * @since Moodle 3.3 + */ + public static function get_entry_parameters() { + return new external_function_parameters( + array( + 'entryid' => new external_value(PARAM_INT, 'record entry id'), + 'returncontents' => new external_value(PARAM_BOOL, 'Whether to return contents or not.', VALUE_DEFAULT, false), + ) + ); + } + + /** + * Return one entry record from the database, including contents optionally. + * + * @param int $entryid the record entry id id + * @param bool $returncontents whether to return the entries contents or not + * @return array of warnings and the entries + * @since Moodle 3.3 + * @throws moodle_exception + */ + public static function get_entry($entryid, $returncontents = false) { + global $PAGE, $DB; + + $params = array('entryid' => $entryid, 'returncontents' => $returncontents); + $params = self::validate_parameters(self::get_entry_parameters(), $params); + $warnings = array(); + + $record = $DB->get_record('data_records', array('id' => $params['entryid']), '*', MUST_EXIST); + list($database, $course, $cm, $context) = self::validate_database($record->dataid); + + // Check database is open in time. + $canmanageentries = has_capability('mod/data:manageentries', $context); + data_require_time_available($database, $canmanageentries); + + if ($record->groupid !== 0) { + if (!groups_group_visible($record->groupid, $course, $cm)) { + throw new moodle_exception('notingroup'); + } + } + + // Check correct record entry. Group check was done before. + if (!data_can_view_record($database, $record, $record->groupid, $canmanageentries)) { + throw new moodle_exception('notapproved', 'data'); + } + + $related = array('context' => $context, 'database' => $database, 'user' => null); + if ($params['returncontents']) { + $related['contents'] = $DB->get_records('data_content', array('recordid' => $record->id)); + } else { + $related['contents'] = null; + } + $exporter = new record_exporter($record, $related); + $entry = $exporter->export($PAGE->get_renderer('core')); + + $result = array( + 'entry' => $entry, + 'warnings' => $warnings + ); + // Check if we should return the entry rendered. + if ($params['returncontents']) { + $records = [$record]; + $result['entryviewcontents'] = data_print_template('singletemplate', $records, $database, '', 0, true); + } + + return $result; + } + + /** + * Returns description of method result value + * + * @return external_description + * @since Moodle 3.3 + */ + public static function get_entry_returns() { + return new external_single_structure( + array( + 'entry' => record_exporter::get_read_structure(), + 'entryviewcontents' => new external_value(PARAM_RAW, 'The entry as is rendered in the site.', VALUE_OPTIONAL), + 'warnings' => new external_warnings() + ) + ); + } } diff --git a/mod/data/classes/external/record_exporter.php b/mod/data/classes/external/record_exporter.php index e3ba863b123..7fecd44474a 100644 --- a/mod/data/classes/external/record_exporter.php +++ b/mod/data/classes/external/record_exporter.php @@ -26,6 +26,7 @@ defined('MOODLE_INTERNAL') || die(); use core\external\exporter; use renderer_base; +use core_user; /** * Class for exporting record data. @@ -78,7 +79,7 @@ class record_exporter extends exporter { protected static function define_related() { return array( 'database' => 'stdClass', - 'user' => 'stdClass', + 'user' => 'stdClass?', 'context' => 'context', 'contents' => 'stdClass[]?', ); @@ -93,6 +94,7 @@ class record_exporter extends exporter { 'fullname' => array( 'type' => PARAM_TEXT, 'description' => 'The user who created the entry fullname.', + 'optional' => true, ), 'contents' => array( 'type' => content_exporter::read_properties_definition(), @@ -108,8 +110,15 @@ class record_exporter extends exporter { $values = array( 'canmanageentry' => data_user_can_manage_entry($this->data, $this->related['database'], $this->related['context']), - 'fullname' => fullname($this->related['user']), ); + + if (!empty($this->related['user']) and !empty($this->related['user']->id)) { + $values['fullname'] = fullname($this->related['user']); + } else if ($this->data->userid) { + $user = core_user::get_user($this->data->userid); + $values['fullname'] = fullname($user); + } + if (!empty($this->related['contents'])) { $contents = []; foreach ($this->related['contents'] as $content) { diff --git a/mod/data/db/services.php b/mod/data/db/services.php index bb3c592db9c..c43e594de50 100644 --- a/mod/data/db/services.php +++ b/mod/data/db/services.php @@ -59,4 +59,12 @@ $functions = array( 'capabilities' => 'mod/data:viewentry', 'services' => array(MOODLE_OFFICIAL_MOBILE_SERVICE) ), + 'mod_data_get_entry' => array( + 'classname' => 'mod_data_external', + 'methodname' => 'get_entry', + 'description' => 'Return one entry record from the database, including contents optionally.', + 'type' => 'read', + 'capabilities' => 'mod/data:viewentry', + 'services' => array(MOODLE_OFFICIAL_MOBILE_SERVICE) + ), ); diff --git a/mod/data/tests/externallib_test.php b/mod/data/tests/externallib_test.php index f919a9c08fb..4f40b81eb08 100644 --- a/mod/data/tests/externallib_test.php +++ b/mod/data/tests/externallib_test.php @@ -335,6 +335,8 @@ class mod_data_external_testcase extends externallib_advanced_testcase { public function populate_database_with_entries() { global $DB; + // Force approval. + $DB->set_field('data', 'approval', 1, array('id' => $this->data->id)); $generator = $this->getDataGenerator()->get_plugin_generator('mod_data'); $fieldtypes = array('checkbox', 'date', 'menu', 'multimenu', 'number', 'radiobutton', 'text', 'textarea', 'url'); @@ -374,10 +376,17 @@ class mod_data_external_testcase extends externallib_advanced_testcase { $entry11 = $generator->create_entry($this->data, $fieldcontents, $this->group1->id); $this->setUser($this->student2); $entry12 = $generator->create_entry($this->data, $fieldcontents, $this->group1->id); + $entry13 = $generator->create_entry($this->data, $fieldcontents, $this->group1->id); $this->setUser($this->student3); $entry21 = $generator->create_entry($this->data, $fieldcontents, $this->group2->id); - return [$entry11, $entry12, $entry21]; + + // Approve all except $entry13. + $DB->set_field('data_records', 'approved', 1, ['id' => $entry11]); + $DB->set_field('data_records', 'approved', 1, ['id' => $entry12]); + $DB->set_field('data_records', 'approved', 1, ['id' => $entry21]); + + return [$entry11, $entry12, $entry13, $entry21]; } /** @@ -385,7 +394,7 @@ class mod_data_external_testcase extends externallib_advanced_testcase { */ public function test_get_entries() { global $DB; - list($entry11, $entry12, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); // First of all, expect to see only my group entries (not other users in other groups ones). $this->setUser($this->student1); @@ -407,8 +416,8 @@ class mod_data_external_testcase extends externallib_advanced_testcase { $result = mod_data_external::get_entries($this->data->id); $result = external_api::clean_returnvalue(mod_data_external::get_entries_returns(), $result); $this->assertCount(0, $result['warnings']); - $this->assertCount(2, $result['entries']); - $this->assertEquals(2, $result['totalcount']); + $this->assertCount(3, $result['entries']); // I can see my entry not approved yet. + $this->assertEquals(3, $result['totalcount']); // Now try with the user in the second group that must see only one entry. $this->setUser($this->student3); @@ -427,11 +436,11 @@ class mod_data_external_testcase extends externallib_advanced_testcase { $result = mod_data_external::get_entries($this->data->id); $result = external_api::clean_returnvalue(mod_data_external::get_entries_returns(), $result); $this->assertCount(0, $result['warnings']); - $this->assertCount(3, $result['entries']); - $this->assertEquals(3, $result['totalcount']); + $this->assertCount(4, $result['entries']); // I can see the not approved one. + $this->assertEquals(4, $result['totalcount']); $entries = $DB->get_records('data_records', array('dataid' => $this->data->id), 'id'); - $this->assertCount(3, $entries); + $this->assertCount(4, $entries); $count = 0; foreach ($entries as $entry) { $this->assertEquals($entry->id, $result['entries'][$count]['id']); @@ -487,4 +496,109 @@ class mod_data_external_testcase extends externallib_advanced_testcase { $this->assertTrue(strpos($result['listviewcontents'], 'text for testing') !== false); $this->assertTrue(strpos($result['listviewcontents'], 'sampleurl') !== false); } + + /** + * Test get_entry_visible_groups. + */ + public function test_get_entry_visible_groups() { + global $DB; + + $DB->set_field('course', 'groupmode', VISIBLEGROUPS, ['id' => $this->course->id]); + list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + + // Check I can see my approved group entries. + $this->setUser($this->student1); + $result = mod_data_external::get_entry($entry11); + $result = external_api::clean_returnvalue(mod_data_external::get_entry_returns(), $result); + $this->assertCount(0, $result['warnings']); + $this->assertEquals($entry11, $result['entry']['id']); + $this->assertTrue($result['entry']['approved']); + $this->assertTrue($result['entry']['canmanageentry']); // Is mine, I can manage it. + + // Entry from other group. + $result = mod_data_external::get_entry($entry21); + $result = external_api::clean_returnvalue(mod_data_external::get_entry_returns(), $result); + $this->assertCount(0, $result['warnings']); + $this->assertEquals($entry21, $result['entry']['id']); + } + + /** + * Test get_entry_separated_groups. + */ + public function test_get_entry_separated_groups() { + global $DB; + list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + + // Check I can see my approved group entries. + $this->setUser($this->student1); + $result = mod_data_external::get_entry($entry11); + $result = external_api::clean_returnvalue(mod_data_external::get_entry_returns(), $result); + $this->assertCount(0, $result['warnings']); + $this->assertEquals($entry11, $result['entry']['id']); + $this->assertTrue($result['entry']['approved']); + $this->assertTrue($result['entry']['canmanageentry']); // Is mine, I can manage it. + + // Retrieve contents. + data_generate_default_template($this->data, 'singletemplate', 0, false, true); + $result = mod_data_external::get_entry($entry11, true); + $result = external_api::clean_returnvalue(mod_data_external::get_entry_returns(), $result); + $this->assertCount(0, $result['warnings']); + $this->assertCount(9, $result['entry']['contents']); + $this->assertTrue(strpos($result['entryviewcontents'], 'opt1') !== false); + $this->assertTrue(strpos($result['entryviewcontents'], 'January') !== false); + $this->assertTrue(strpos($result['entryviewcontents'], 'menu1') !== false); + $this->assertTrue(strpos($result['entryviewcontents'], 'text for testing') !== false); + $this->assertTrue(strpos($result['entryviewcontents'], 'sampleurl') !== false); + + // This is in my group but I'm not the author. + $result = mod_data_external::get_entry($entry12); + $result = external_api::clean_returnvalue(mod_data_external::get_entry_returns(), $result); + $this->assertCount(0, $result['warnings']); + $this->assertEquals($entry12, $result['entry']['id']); + $this->assertTrue($result['entry']['approved']); + $this->assertFalse($result['entry']['canmanageentry']); // Not mine. + + $this->setUser($this->student3); + $result = mod_data_external::get_entry($entry21); + $result = external_api::clean_returnvalue(mod_data_external::get_entry_returns(), $result); + $this->assertCount(0, $result['warnings']); + $this->assertEquals($entry21, $result['entry']['id']); + $this->assertTrue($result['entry']['approved']); + $this->assertTrue($result['entry']['canmanageentry']); // Is mine, I can manage it. + + // As teacher I should be able to see all the entries. + $this->setUser($this->teacher); + $result = mod_data_external::get_entry($entry11); + $result = external_api::clean_returnvalue(mod_data_external::get_entry_returns(), $result); + $this->assertEquals($entry11, $result['entry']['id']); + + $result = mod_data_external::get_entry($entry12); + $result = external_api::clean_returnvalue(mod_data_external::get_entry_returns(), $result); + $this->assertEquals($entry12, $result['entry']['id']); + // This is the not approved one. + $result = mod_data_external::get_entry($entry13); + $result = external_api::clean_returnvalue(mod_data_external::get_entry_returns(), $result); + $this->assertEquals($entry13, $result['entry']['id']); + + $result = mod_data_external::get_entry($entry21); + $result = external_api::clean_returnvalue(mod_data_external::get_entry_returns(), $result); + $this->assertEquals($entry21, $result['entry']['id']); + + // Now, try to get an entry not approved yet. + $this->setUser($this->student1); + $this->expectException('moodle_exception'); + $result = mod_data_external::get_entry($entry13); + } + + /** + * Test get_entry from other group in separated groups. + */ + public function test_get_entry_other_group_separated_groups() { + list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + + // We should not be able to view other gropu entries (in separated groups). + $this->setUser($this->student1); + $this->expectException('moodle_exception'); + $result = mod_data_external::get_entry($entry21); + } } diff --git a/mod/data/version.php b/mod/data/version.php index 43d1f0e0586..54fe1cad169 100644 --- a/mod/data/version.php +++ b/mod/data/version.php @@ -24,7 +24,7 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2016120503; // The current module version (Date: YYYYMMDDXX) +$plugin->version = 2016120504; // The current module version (Date: YYYYMMDDXX) $plugin->requires = 2016112900; // Requires this Moodle version $plugin->component = 'mod_data'; // Full name of the plugin (used for diagnostics) $plugin->cron = 0;