diff --git a/mod/bigbluebuttonbn/classes/instance.php b/mod/bigbluebuttonbn/classes/instance.php index bba435978f3..51efd4d0e1c 100644 --- a/mod/bigbluebuttonbn/classes/instance.php +++ b/mod/bigbluebuttonbn/classes/instance.php @@ -282,9 +282,9 @@ EOF; /** * Get the current groupid if set. * - * @return null|int + * @return int */ - public function get_group_id(): ?int { + public function get_group_id(): int { return empty($this->groupid) ? 0 : $this->groupid; } @@ -294,7 +294,8 @@ EOF; * @return bool */ public function uses_groups(): bool { - return $this->groupid !== null; + $groupmode = groups_get_activity_groupmode($this->get_cm()); + return $groupmode != NOGROUPS; } /** @@ -305,7 +306,7 @@ EOF; public function get_group_name(): ?string { $groupid = $this->get_group_id(); - if ($groupid === null) { + if (!$this->uses_groups()) { return null; } diff --git a/mod/bigbluebuttonbn/classes/local/bigbluebutton/recordings/recording_data.php b/mod/bigbluebuttonbn/classes/local/bigbluebutton/recordings/recording_data.php index ee11a134fc7..453e0bdfed0 100644 --- a/mod/bigbluebuttonbn/classes/local/bigbluebutton/recordings/recording_data.php +++ b/mod/bigbluebuttonbn/classes/local/bigbluebutton/recordings/recording_data.php @@ -301,8 +301,8 @@ class recording_data { return true; } // When groups are enabled, exclude those to which the user doesn't have access to. - if ($instance->uses_groups()) { - return intval($rec->get('groupid')) === intval($instance->get_group_id()); + if ($instance->uses_groups() && !$instance->can_manage_recordings()) { + return intval($rec->get('groupid')) === $instance->get_group_id(); } return true; } diff --git a/mod/bigbluebuttonbn/classes/test/testcase_helper_trait.php b/mod/bigbluebuttonbn/classes/test/testcase_helper_trait.php index f0a210d600a..56b7433f51c 100644 --- a/mod/bigbluebuttonbn/classes/test/testcase_helper_trait.php +++ b/mod/bigbluebuttonbn/classes/test/testcase_helper_trait.php @@ -221,7 +221,7 @@ trait testcase_helper_trait { 'users' => $users) = $dataset; $plugingenerator = $this->getDataGenerator()->get_plugin_generator('mod_bigbluebuttonbn'); - $coursedata = empty($groups) ? [] : ['groupmodeforce' => true, 'groupmode' => VISIBLEGROUPS]; + $coursedata = empty($groups) ? [] : ['groupmodeforce' => true, 'groupmode' => $dataset['coursemode'] ?? VISIBLEGROUPS]; $this->course = $this->getDataGenerator()->create_course($coursedata); foreach ($users as $userdata) { diff --git a/mod/bigbluebuttonbn/tests/external/get_recordings_test.php b/mod/bigbluebuttonbn/tests/external/get_recordings_test.php index d6cd2b5fd85..31f9c11680a 100644 --- a/mod/bigbluebuttonbn/tests/external/get_recordings_test.php +++ b/mod/bigbluebuttonbn/tests/external/get_recordings_test.php @@ -210,6 +210,7 @@ class get_recordings_test extends \externallib_advanced_testcase { /** * Check we can see all recording from a course in a room only instance + * @covers \mod_bigbluebuttonbn\external\get_recordings::execute */ public function test_get_recordings_room_only() { $this->resetAfterTest(); @@ -252,6 +253,7 @@ class get_recordings_test extends \externallib_advanced_testcase { /** * Check if we can see the imported recording in a new instance + * @covers \mod_bigbluebuttonbn\external\get_recordings::execute */ public function test_get_recordings_imported() { $this->resetAfterTest(); @@ -314,19 +316,13 @@ class get_recordings_test extends \externallib_advanced_testcase { /** * Check if recording are visible/invisible depending on the group. + * + * @covers \mod_bigbluebuttonbn\external\get_recordings::execute + * @dataProvider recording_group_test_data */ - public function test_get_recordings_groups_visible() { + public function test_get_recordings_groups($type, $groups, $users, $recordingsdata, $test, $coursemode) { $this->resetAfterTest(); - $dataset = [ - 'type' => instance::TYPE_ALL, - 'groups' => ['G1' => ['s1'], 'G2' => ['s2']], - 'users' => [['username' => 't1', 'role' => 'editingteacher'], ['username' => 's1', 'role' => 'student'], - ['username' => 's2', 'role' => 'student']], - 'recordingsdata' => [ - 'G1' => [['name' => 'Recording1']], - 'G2' => [['name' => 'Recording2']] - ], - ]; + $dataset = compact('type', 'groups', 'users', 'recordingsdata', 'test', 'coursemode'); $activityid = $this->create_from_dataset($dataset); $instance = instance::get_from_instanceid($activityid); @@ -334,25 +330,83 @@ class get_recordings_test extends \externallib_advanced_testcase { $user = \core_user::get_user_by_username($userdef['username']); $this->setUser($user); $groups = array_values(groups_get_my_groups()); - $usergroupnames = array_map(function($g) { - return $g->name; - }, $groups); $mygroup = !empty($groups) ? end($groups) : null; $getrecordings = $this->get_recordings( $instance->get_instance_id(), null, !empty($mygroup) ? $mygroup->id : null); + $allrecordingsnames = []; + foreach ($recordingsdata as $groups => $rsinfo) { + $rnames = array_map(function($rdata) { + return $rdata['name']; + }, $rsinfo); + $allrecordingsnames = array_merge($allrecordingsnames, $rnames); + } // Check users see or do not see recording dependings on their groups. - foreach ($dataset['recordingsdata'] as $groupname => $recordingdata) { - foreach ($recordingdata as $recording) { - if (in_array($groupname, $usergroupnames) || $instance->can_manage_recordings()) { - $this->assertStringContainsString($recording['name'], $getrecordings['tabledata']['data'], - "User $user->username, should see recording {$recording['name']}"); - } else { - $this->assertStringNotContainsString($recording['name'], $getrecordings['tabledata']['data'], - "User $user->username, should not see recording {$recording['name']}"); - } + foreach ($dataset['test'][$user->username] as $viewablerecordings) { + $viewablerecordings = $dataset['test'][$user->username]; + $invisiblerecordings = array_diff($allrecordingsnames, $viewablerecordings); + foreach ($viewablerecordings as $viewablerecordingname) { + $this->assertStringContainsString($viewablerecordingname, $getrecordings['tabledata']['data'], + "User $user->username, should see recording {$viewablerecordingname}"); + } + foreach ($invisiblerecordings as $invisiblerecordingname) { + $this->assertStringNotContainsString($invisiblerecordingname, $getrecordings['tabledata']['data'], + "User $user->username, should not see recording {$viewablerecordingname}"); } } } } + + /** + * Recording group test + * + * @return array[] + */ + public function recording_group_test_data() { + return [ + 'visiblegroups' => [ + 'type' => instance::TYPE_ALL, + 'groups' => ['G1' => ['s1'], 'G2' => ['s2']], + 'users' => [ + ['username' => 't1', 'role' => 'editingteacher'], + ['username' => 's1', 'role' => 'student'], + ['username' => 's2', 'role' => 'student'], + ['username' => 's3', 'role' => 'student'] + ], + 'recordingsdata' => [ + 'G1' => [['name' => 'Recording1']], + 'G2' => [['name' => 'Recording2']], + '' => [['name' => 'Recording3']] + ], + 'test' => [ + 't1' => ['Recording1', 'Recording2', 'Recording3'], // A moderator should see all recordings. + 's1' => ['Recording1'], // S1 can only see the recordings from his group. + 's2' => ['Recording2'], // S2 can only see the recordings from his group. + 's3' => ['Recording3'] // S3 should see recordings which have no groups. + ], + 'coursemode' => VISIBLEGROUPS + ], + 'separategroups' => [ + 'type' => instance::TYPE_ALL, + 'groups' => ['G1' => ['s1'], 'G2' => ['s2']], + 'users' => [ + ['username' => 't1', 'role' => 'editingteacher'], + ['username' => 's1', 'role' => 'student'], + ['username' => 's2', 'role' => 'student'] + ], + 'recordingsdata' => [ + 'G1' => [['name' => 'Recording1']], + 'G2' => [['name' => 'Recording2']], + '' => [['name' => 'Recording3']] + ], + 'test' => [ + 't1' => ['Recording1', 'Recording2', 'Recording3'], // A moderator should see all recordings. + 's1' => ['Recording1'], // S1 can only see the recordings from his group. + 's2' => ['Recording2'], // S2 can only see the recordings from his group. + 's3' => ['Recording3'] // S3 should see recordings which have no groups. + ], + 'coursemode' => SEPARATEGROUPS + ] + ]; + } }