diff --git a/mod/data/classes/external.php b/mod/data/classes/external.php index 65670ef0728ab..95588de1fe2ce 100644 --- a/mod/data/classes/external.php +++ b/mod/data/classes/external.php @@ -264,7 +264,6 @@ public static function get_data_access_information($databaseid, $groupid = 0) { 'warnings' => $warnings ); - $groupmode = groups_get_activity_groupmode($cm); if (!empty($params['groupid'])) { $groupid = $params['groupid']; // Determine is the group is visible to user. @@ -273,12 +272,9 @@ public static function get_data_access_information($databaseid, $groupid = 0) { } } else { // Check to see if groups are being used here. + $groupmode = groups_get_activity_groupmode($cm); if ($groupmode) { $groupid = groups_get_activity_group($cm); - // Determine is the group is visible to user (this is particullary for the group 0 -> all groups). - if (!groups_group_visible($groupid, $course, $cm)) { - throw new moodle_exception('notingroup'); - } } else { $groupid = 0; } @@ -399,11 +395,8 @@ public static function get_entries($databaseid, $groupid = 0, $returncontents = } else { // Check to see if groups are being used here. if ($groupmode = groups_get_activity_groupmode($cm)) { + // We don't need to validate a possible groupid = 0 since it would be handled by data_search_entries. $groupid = groups_get_activity_group($cm); - // Determine is the group is visible to user (this is particullary for the group 0 -> all groups). - if (!groups_group_visible($groupid, $course, $cm)) { - throw new moodle_exception('notingroup'); - } } else { $groupid = 0; } @@ -726,11 +719,8 @@ public static function search_entries($databaseid, $groupid = 0, $returncontents } else { // Check to see if groups are being used here. if ($groupmode = groups_get_activity_groupmode($cm)) { + // We don't need to validate a possible groupid = 0 since it would be handled by data_search_entries. $groupid = groups_get_activity_group($cm); - // Determine is the group is visible to user (this is particullary for the group 0 -> all groups). - if (!groups_group_visible($groupid, $course, $cm)) { - throw new moodle_exception('notingroup'); - } } else { $groupid = 0; } @@ -985,26 +975,18 @@ public static function add_entry($databaseid, $groupid, $data) { // Check database is open in time. data_require_time_available($database, null, $context); - $groupmode = groups_get_activity_groupmode($cm); - if (!empty($params['groupid'])) { - $groupid = $params['groupid']; - // Determine is the group is visible to user. - if (!groups_group_visible($groupid, $course, $cm)) { - throw new moodle_exception('notingroup'); - } - } else { + // Determine default group. + if (empty($params['groupid'])) { // Check to see if groups are being used here. + $groupmode = groups_get_activity_groupmode($cm); if ($groupmode) { $groupid = groups_get_activity_group($cm); - // Determine is the group is visible to user (this is particullary for the group 0 -> all groups). - if (!groups_group_visible($groupid, $course, $cm)) { - throw new moodle_exception('notingroup'); - } } else { $groupid = 0; } } + // Group is validated inside the function. if (!data_user_can_add_entry($database, $groupid, $groupmode, $context)) { throw new moodle_exception('noaccess', 'data'); } diff --git a/mod/data/tests/externallib_test.php b/mod/data/tests/externallib_test.php index 5a37334773ef2..8a3808454737a 100644 --- a/mod/data/tests/externallib_test.php +++ b/mod/data/tests/externallib_test.php @@ -416,6 +416,8 @@ public function populate_database_with_entries() { $this->setUser($this->student2); $entry12 = $generator->create_entry($this->database, $fieldcontents, $this->group1->id); $entry13 = $generator->create_entry($this->database, $fieldcontents, $this->group1->id); + // Entry not in group. + $entry14 = $generator->create_entry($this->database, $fieldcontents, 0); $this->setUser($this->student3); $entry21 = $generator->create_entry($this->database, $fieldcontents, $this->group2->id); @@ -423,9 +425,10 @@ public function populate_database_with_entries() { // 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' => $entry14]); $DB->set_field('data_records', 'approved', 1, ['id' => $entry21]); - return [$entry11, $entry12, $entry13, $entry21]; + return [$entry11, $entry12, $entry13, $entry14, $entry21]; } /** @@ -433,15 +436,16 @@ public function populate_database_with_entries() { */ public function test_get_entries() { global $DB; - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); // First of all, expect to see only my group entries (not other users in other groups ones). + // We may expect entries without group also. $this->setUser($this->student1); $result = mod_data_external::get_entries($this->database->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']); + $this->assertEquals(3, $result['totalcount']); $this->assertEquals($entry11, $result['entries'][0]['id']); $this->assertEquals($this->student1->id, $result['entries'][0]['userid']); $this->assertEquals($this->group1->id, $result['entries'][0]['groupid']); @@ -450,36 +454,44 @@ public function test_get_entries() { $this->assertEquals($this->student2->id, $result['entries'][1]['userid']); $this->assertEquals($this->group1->id, $result['entries'][1]['groupid']); $this->assertEquals($this->database->id, $result['entries'][1]['dataid']); + $this->assertEquals($entry14, $result['entries'][2]['id']); + $this->assertEquals($this->student2->id, $result['entries'][2]['userid']); + $this->assertEquals(0, $result['entries'][2]['groupid']); + $this->assertEquals($this->database->id, $result['entries'][2]['dataid']); // Other user in same group. $this->setUser($this->student2); $result = mod_data_external::get_entries($this->database->id); $result = external_api::clean_returnvalue(mod_data_external::get_entries_returns(), $result); $this->assertCount(0, $result['warnings']); - $this->assertCount(3, $result['entries']); // I can see my entry not approved yet. - $this->assertEquals(3, $result['totalcount']); + $this->assertCount(4, $result['entries']); // I can see my entry not approved yet. + $this->assertEquals(4, $result['totalcount']); - // Now try with the user in the second group that must see only one entry. + // Now try with the user in the second group that must see only two entries (his group entry and the one without group). $this->setUser($this->student3); $result = mod_data_external::get_entries($this->database->id); $result = external_api::clean_returnvalue(mod_data_external::get_entries_returns(), $result); $this->assertCount(0, $result['warnings']); - $this->assertCount(1, $result['entries']); - $this->assertEquals(1, $result['totalcount']); - $this->assertEquals($entry21, $result['entries'][0]['id']); - $this->assertEquals($this->student3->id, $result['entries'][0]['userid']); - $this->assertEquals($this->group2->id, $result['entries'][0]['groupid']); + $this->assertCount(2, $result['entries']); + $this->assertEquals(2, $result['totalcount']); + $this->assertEquals($entry14, $result['entries'][0]['id']); + $this->assertEquals($this->student2->id, $result['entries'][0]['userid']); + $this->assertEquals(0, $result['entries'][0]['groupid']); $this->assertEquals($this->database->id, $result['entries'][0]['dataid']); + $this->assertEquals($entry21, $result['entries'][1]['id']); + $this->assertEquals($this->student3->id, $result['entries'][1]['userid']); + $this->assertEquals($this->group2->id, $result['entries'][1]['groupid']); + $this->assertEquals($this->database->id, $result['entries'][1]['dataid']); // Now, as teacher we should see all (we have permissions to view all groups). $this->setUser($this->teacher); $result = mod_data_external::get_entries($this->database->id); $result = external_api::clean_returnvalue(mod_data_external::get_entries_returns(), $result); $this->assertCount(0, $result['warnings']); - $this->assertCount(4, $result['entries']); // I can see the not approved one. - $this->assertEquals(4, $result['totalcount']); + $this->assertCount(5, $result['entries']); // I can see the not approved one. + $this->assertEquals(5, $result['totalcount']); $entries = $DB->get_records('data_records', array('dataid' => $this->database->id), 'id'); - $this->assertCount(4, $entries); + $this->assertCount(5, $entries); $count = 0; foreach ($entries as $entry) { $this->assertEquals($entry->id, $result['entries'][$count]['id']); @@ -491,17 +503,17 @@ public function test_get_entries() { $result = mod_data_external::get_entries($this->database->id, $this->group1->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']); + $this->assertEquals(3, $result['totalcount']); // Test ordering (reverse). $this->setUser($this->student1); $result = mod_data_external::get_entries($this->database->id, $this->group1->id, false, null, 'DESC'); $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->assertEquals($entry12, $result['entries'][0]['id']); + $this->assertCount(3, $result['entries']); + $this->assertEquals(3, $result['totalcount']); + $this->assertEquals($entry14, $result['entries'][0]['id']); // Test pagination. $this->setUser($this->student1); @@ -509,14 +521,14 @@ public function test_get_entries() { $result = external_api::clean_returnvalue(mod_data_external::get_entries_returns(), $result); $this->assertCount(0, $result['warnings']); $this->assertCount(1, $result['entries']); - $this->assertEquals(2, $result['totalcount']); + $this->assertEquals(3, $result['totalcount']); $this->assertEquals($entry11, $result['entries'][0]['id']); $result = mod_data_external::get_entries($this->database->id, $this->group1->id, false, null, null, 1, 1); $result = external_api::clean_returnvalue(mod_data_external::get_entries_returns(), $result); $this->assertCount(0, $result['warnings']); $this->assertCount(1, $result['entries']); - $this->assertEquals(2, $result['totalcount']); + $this->assertEquals(3, $result['totalcount']); $this->assertEquals($entry12, $result['entries'][0]['id']); // Now test the return contents. @@ -525,7 +537,7 @@ public function test_get_entries() { $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->assertEquals(3, $result['totalcount']); $this->assertCount(9, $result['entries'][0]['contents']); $this->assertCount(9, $result['entries'][1]['contents']); // Search for some content. @@ -543,7 +555,7 @@ 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(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); // Check I can see my approved group entries. $this->setUser($this->student1); @@ -566,7 +578,7 @@ public function test_get_entry_visible_groups() { */ public function test_get_entry_separated_groups() { global $DB; - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); // Check I can see my approved group entries. $this->setUser($this->student1); @@ -633,7 +645,7 @@ public function test_get_entry_separated_groups() { * 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(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); // We should not be able to view other gropu entries (in separated groups). $this->setUser($this->student1); @@ -646,7 +658,7 @@ public function test_get_entry_other_group_separated_groups() { */ public function test_get_fields() { global $DB; - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); $this->setUser($this->student1); $result = mod_data_external::get_fields($this->database->id); @@ -676,14 +688,14 @@ public function test_get_fields_database_without_fields() { */ public function test_search_entries() { global $DB; - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); $this->setUser($this->student1); // Empty search, it should return all the visible entries. $result = mod_data_external::search_entries($this->database->id, 0, false); $result = external_api::clean_returnvalue(mod_data_external::search_entries_returns(), $result); - $this->assertCount(2, $result['entries']); - $this->assertEquals(2, $result['totalcount']); + $this->assertCount(3, $result['entries']); + $this->assertEquals(3, $result['totalcount']); // Search for something that does not exists. $result = mod_data_external::search_entries($this->database->id, 0, false, 'abc'); @@ -694,15 +706,15 @@ public function test_search_entries() { // Search by text matching all the entries. $result = mod_data_external::search_entries($this->database->id, 0, false, 'text'); $result = external_api::clean_returnvalue(mod_data_external::search_entries_returns(), $result); - $this->assertCount(2, $result['entries']); - $this->assertEquals(2, $result['totalcount']); + $this->assertCount(3, $result['entries']); + $this->assertEquals(3, $result['totalcount']); // Now as the other student I should receive my not approved entry. Apply ordering here. $this->setUser($this->student2); $result = mod_data_external::search_entries($this->database->id, 0, false, 'text', [], DATA_APPROVED, 'ASC'); $result = external_api::clean_returnvalue(mod_data_external::search_entries_returns(), $result); - $this->assertCount(3, $result['entries']); - $this->assertEquals(3, $result['totalcount']); + $this->assertCount(4, $result['entries']); + $this->assertEquals(4, $result['totalcount']); // The not approved one should be the first. $this->assertEquals($entry13, $result['entries'][0]['id']); @@ -710,23 +722,24 @@ public function test_search_entries() { $this->setUser($this->student3); $result = mod_data_external::search_entries($this->database->id, 0, false, 'text'); $result = external_api::clean_returnvalue(mod_data_external::search_entries_returns(), $result); - $this->assertCount(1, $result['entries']); - $this->assertEquals(1, $result['totalcount']); - $this->assertEquals($this->student3->id, $result['entries'][0]['userid']); + $this->assertCount(2, $result['entries']); + $this->assertEquals(2, $result['totalcount']); + $this->assertEquals($this->student2->id, $result['entries'][0]['userid']); + $this->assertEquals($this->student3->id, $result['entries'][1]['userid']); // Same normal text search as teacher. $this->setUser($this->teacher); $result = mod_data_external::search_entries($this->database->id, 0, false, 'text'); $result = external_api::clean_returnvalue(mod_data_external::search_entries_returns(), $result); - $this->assertCount(4, $result['entries']); // I can see all groups and non approved. - $this->assertEquals(4, $result['totalcount']); + $this->assertCount(5, $result['entries']); // I can see all groups and non approved. + $this->assertEquals(5, $result['totalcount']); // Pagination. $this->setUser($this->teacher); $result = mod_data_external::search_entries($this->database->id, 0, false, 'text', [], DATA_TIMEADDED, 'ASC', 0, 2); $result = external_api::clean_returnvalue(mod_data_external::search_entries_returns(), $result); $this->assertCount(2, $result['entries']); // Only 2 per page. - $this->assertEquals(4, $result['totalcount']); + $this->assertEquals(5, $result['totalcount']); // Now advanced search or not dinamic fields (user firstname for example). $this->setUser($this->student1); @@ -735,8 +748,8 @@ public function test_search_entries() { ]; $result = mod_data_external::search_entries($this->database->id, 0, false, '', $advsearch); $result = external_api::clean_returnvalue(mod_data_external::search_entries_returns(), $result); - $this->assertCount(1, $result['entries']); - $this->assertEquals(1, $result['totalcount']); + $this->assertCount(2, $result['entries']); + $this->assertEquals(2, $result['totalcount']); $this->assertEquals($this->student2->id, $result['entries'][0]['userid']); // I only found mine! // Advanced search for fields. @@ -746,8 +759,8 @@ public function test_search_entries() { ]; $result = mod_data_external::search_entries($this->database->id, 0, false, '', $advsearch); $result = external_api::clean_returnvalue(mod_data_external::search_entries_returns(), $result); - $this->assertCount(2, $result['entries']); // Found two entries matching this. - $this->assertEquals(2, $result['totalcount']); + $this->assertCount(3, $result['entries']); // Found two entries matching this. + $this->assertEquals(3, $result['totalcount']); // Combined search. $field2 = $DB->get_record('data_fields', array('type' => 'number')); @@ -758,8 +771,8 @@ public function test_search_entries() { ]; $result = mod_data_external::search_entries($this->database->id, 0, false, '', $advsearch); $result = external_api::clean_returnvalue(mod_data_external::search_entries_returns(), $result); - $this->assertCount(1, $result['entries']); // Only one matching everything. - $this->assertEquals(1, $result['totalcount']); + $this->assertCount(2, $result['entries']); // Only one matching everything. + $this->assertEquals(2, $result['totalcount']); // Combined search (no results). $field2 = $DB->get_record('data_fields', array('type' => 'number')); @@ -778,7 +791,7 @@ public function test_search_entries() { */ public function test_approve_entry() { global $DB; - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); $this->setUser($this->teacher); $this->assertEquals(0, $DB->get_field('data_records', 'approved', array('id' => $entry13))); @@ -792,7 +805,7 @@ public function test_approve_entry() { */ public function test_unapprove_entry() { global $DB; - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); $this->setUser($this->teacher); $this->assertEquals(1, $DB->get_field('data_records', 'approved', array('id' => $entry11))); @@ -806,7 +819,7 @@ public function test_unapprove_entry() { */ public function test_approve_entry_missing_permissions() { global $DB; - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); $this->setUser($this->student1); $this->expectException('moodle_exception'); @@ -818,7 +831,7 @@ public function test_approve_entry_missing_permissions() { */ public function test_delete_entry_as_teacher() { global $DB; - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); $this->setUser($this->teacher); $result = mod_data_external::delete_entry($entry11); @@ -836,7 +849,7 @@ public function test_delete_entry_as_teacher() { */ public function test_delete_entry_as_student() { global $DB; - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); $this->setUser($this->student1); $result = mod_data_external::delete_entry($entry11); @@ -849,7 +862,7 @@ public function test_delete_entry_as_student() { */ public function test_delete_entry_as_student_in_read_only_period() { global $DB; - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); // Set a time period. $this->database->timeviewfrom = time() - HOURSECS; $this->database->timeviewto = time() + HOURSECS; @@ -865,7 +878,7 @@ public function test_delete_entry_as_student_in_read_only_period() { */ public function test_delete_entry_missing_permissions() { global $DB; - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); $this->setUser($this->student1); $this->expectException('moodle_exception'); @@ -878,7 +891,7 @@ public function test_delete_entry_missing_permissions() { public function test_add_entry() { global $DB; // First create the record structure and add some entries. - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); $this->setUser($this->student1); $newentrydata = []; @@ -1029,7 +1042,7 @@ public function test_add_entry_empty_form() { */ public function test_add_entry_read_only_period() { global $DB; - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); // Set a time period. $this->database->timeviewfrom = time() - HOURSECS; $this->database->timeviewto = time() + HOURSECS; @@ -1046,7 +1059,7 @@ public function test_add_entry_read_only_period() { */ public function test_add_entry_max_num_entries() { global $DB; - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); // Set a time period. $this->database->maxentries = 1; $DB->update_record('data', $this->database); @@ -1063,7 +1076,7 @@ public function test_add_entry_max_num_entries() { public function test_update_entry() { global $DB; // First create the record structure and add some entries. - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); $this->setUser($this->student1); $newentrydata = []; @@ -1203,7 +1216,7 @@ public function test_update_entry() { * Test update_entry sending empty data. */ public function test_update_entry_empty_data() { - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); $this->setUser($this->student1); $result = mod_data_external::update_entry($entry11, []); @@ -1219,7 +1232,7 @@ public function test_update_entry_empty_data() { */ public function test_update_entry_read_only_period() { global $DB; - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); // Set a time period. $this->database->timeviewfrom = time() - HOURSECS; $this->database->timeviewto = time() + HOURSECS; @@ -1236,7 +1249,7 @@ public function test_update_entry_read_only_period() { */ public function test_update_entry_other_user() { // Try to update other user entry. - list($entry11, $entry12, $entry13, $entry21) = self::populate_database_with_entries(); + list($entry11, $entry12, $entry13, $entry14, $entry21) = self::populate_database_with_entries(); $this->setUser($this->student2); $this->expectExceptionMessage(get_string('noaccess', 'data')); $this->expectException('moodle_exception');