diff --git a/lib/Service/CheckinService.php b/lib/Service/CheckinService.php index ffc76b9d..82291a4a 100644 --- a/lib/Service/CheckinService.php +++ b/lib/Service/CheckinService.php @@ -130,11 +130,15 @@ public function getCheckinData(int $appointmentId): array { $appointment = $this->appointmentMapper->find($appointmentId); $responses = $this->responseMapper->findByAppointment($appointmentId); - // Get whitelisted groups for filtering + // The whitelist narrows the audience; the admin's grouping mode + // decides which groups section the list. $whitelistedGroups = $this->configService->getWhitelistedGroups(); - - // Get the target attendees efficiently - filters by whitelisted groups when configured $targetAttendees = $this->visibilityService->getTargetAttendees($appointment, $whitelistedGroups); + $sectionGroups = ConfigService::sectionGroups( + $this->configService->getResponseSummaryGroupsMode(), + $whitelistedGroups, + $this->visibilityService->getVisibilitySettings($appointment)['groups'], + ); // Create a map of user responses $userResponseMap = []; @@ -143,7 +147,7 @@ public function getCheckinData(int $appointmentId): array { } // Build group list for filtering UI - $userGroups = $this->buildGroupList($whitelistedGroups); + $userGroups = $sectionGroups ?? $this->allGroupsWithOthers(); // A place in line is not a place at the appointment. The list still // carries everyone — an organizer has to be able to check in whoever @@ -156,33 +160,30 @@ public function getCheckinData(int $appointmentId): array { } $users = array_map( - fn ($user) => $this->buildUserData($user, $userResponseMap, $whitelistedGroups, $confirmedIds), + fn ($user) => $this->buildUserData($user, $userResponseMap, $sectionGroups, $confirmedIds), array_values($targetAttendees), ); return [ 'appointment' => $this->appointmentSerializer->serialize($appointment), 'users' => $users, - 'userGroups' => array_values($userGroups), + 'userGroups' => $userGroups, ]; } /** - * Build the list of groups for filtering. + * Every group of the instance plus "Others", for the "all" mode filter. + * + * @return list */ - private function buildGroupList(array $whitelistedGroups): array { - if (empty($whitelistedGroups)) { - $groups = $this->visibilityService->getAllGroupIds(); - // Hide the Guests app system group; guests fall under "Others" - // unless an admin opted them in explicitly via the whitelist. - $groups = array_values(array_filter( - $groups, - fn (string $g) => !GuestService::isGuestsSystemGroup($g), - )); - $groups[] = 'Others'; - } else { - $groups = $whitelistedGroups; - } + private function allGroupsWithOthers(): array { + // Hide the Guests app system group; guests fall under "Others" + // unless an admin opted them in explicitly via the whitelist. + $groups = array_values(array_filter( + $this->visibilityService->getAllGroupIds(), + fn (string $g) => !GuestService::isGuestsSystemGroup($g), + )); + $groups[] = 'Others'; return $groups; } @@ -239,38 +240,29 @@ public function getCheckinSummary(Appointment $appointment, array $responses): a /** * Build data structure for a single user. + * + * @param ?list $sectionGroups null means every group */ - private function buildUserData($user, array $userResponseMap, array $whitelistedGroups, array $confirmedIds = []): array { + private function buildUserData($user, array $userResponseMap, ?array $sectionGroups, array $confirmedIds = []): array { $userId = $user->getUID(); $userGroupIds = $this->groupManager->getUserGroupIds($user); - // Hide the Guests app system group from the per-user group list - // unless the admin explicitly whitelisted it. - $guestAppWhitelisted = in_array( - GuestService::GUESTS_SYSTEM_GROUP, - array_map('strtolower', $whitelistedGroups), - true, - ); - if (!$guestAppWhitelisted) { - $userGroupIds = array_values(array_filter( + // The sections this user belongs to, spelled as the section list + // spells them. Guests only get a section of their own when an admin + // opted the Guests app system group in. + if ($sectionGroups === null) { + $groups = array_values(array_filter( $userGroupIds, fn (string $g) => !GuestService::isGuestsSystemGroup($g), )); + } else { + $userGroupsLower = array_map('strtolower', $userGroupIds); + $groups = array_values(array_filter( + $sectionGroups, + fn (string $g) => in_array(strtolower($g), $userGroupsLower, true), + )); } - - // Check if user belongs to any whitelisted group - $userInWhitelistedGroup = empty($whitelistedGroups); - if (!empty($whitelistedGroups)) { - foreach ($userGroupIds as $groupId) { - if (in_array($groupId, $whitelistedGroups)) { - $userInWhitelistedGroup = true; - break; - } - } - } - - // Determine effective groups - $effectiveGroups = $userInWhitelistedGroup && !empty($userGroupIds) ? $userGroupIds : ['Others']; + $effectiveGroups = $groups !== [] ? $groups : ['Others']; // Base user data $userData = [ diff --git a/lib/Service/ConfigService.php b/lib/Service/ConfigService.php index f41deff3..abbfd16f 100644 --- a/lib/Service/ConfigService.php +++ b/lib/Service/ConfigService.php @@ -70,6 +70,30 @@ public function setResponseSummaryGroupsMode(string $mode): void { $this->setStoredMode('response_summary_groups_mode', $mode, self::RESPONSE_SUMMARY_GROUP_MODES); } + /** + * Which groups a list sections by under $mode, or null when every group + * does: the admin's list in "specific" mode, otherwise the groups a + * restricted appointment names (issue #199), otherwise all or none. + * Static so the services sharing it stay testable with a mocked instance. + * + * @param array $whitelistedGroups + * @param array $restrictionGroups The appointment's own visibility groups + * @return ?list + */ + public static function sectionGroups(string $mode, array $whitelistedGroups, array $restrictionGroups): ?array { + if ($mode === self::RESPONSE_SUMMARY_MODE_SPECIFIC) { + return array_values($whitelistedGroups); + } + + // A numerically-named group decodes to an int out of the JSON column. + $restrictionGroups = array_values(array_map('strval', $restrictionGroups)); + if ($restrictionGroups !== []) { + return $restrictionGroups; + } + + return $mode === self::RESPONSE_SUMMARY_MODE_ALL ? null : []; + } + /** * Check if a group is allowed based on whitelist configuration. * If no whitelist is configured, all groups are allowed. diff --git a/lib/Service/ExportService.php b/lib/Service/ExportService.php index ba80bcb4..1331fbf6 100644 --- a/lib/Service/ExportService.php +++ b/lib/Service/ExportService.php @@ -104,42 +104,24 @@ static function (array $a, array $b): int { /** * Which groups the column may name, lowercased, or null when it may name - * any of them. - * - * Mirrors the rule the response summary sections by - * (ResponseSummaryService::isGroupAllowedCached), so the sheet says what - * the overview shows. In order: the admin's group list decides when the - * mode is "specific"; otherwise the groups a restricted appointment names - * decide, because those are what let people in (issue #252); otherwise - * every group counts in "all" mode and none in "none". - * - * A run covering several appointments takes the union of their - * restrictions — one column cannot answer per appointment. + * any of them — the groups the response summary sections by, so the sheet + * says what the overview shows (issue #252). A run covering several + * appointments takes the union of their restrictions — one column cannot + * answer per appointment. * * @param list $appointments * @param array $whitelistedGroups * @return ?list null means "any group" */ private function nameableGroups(array $appointments, array $whitelistedGroups): ?array { - if ($this->configService->getResponseSummaryGroupsMode() === ConfigService::RESPONSE_SUMMARY_MODE_SPECIFIC) { - return array_values(array_map('strtolower', $whitelistedGroups)); - } - $restrictionGroups = []; foreach ($appointments as $appointment) { - // A numerically-named group decodes to an int out of the JSON column. - $groups = array_map('strval', $this->visibilityService->getVisibilitySettings($appointment)['groups']); - foreach ($groups as $groupId) { - $restrictionGroups[strtolower($groupId)] = true; - } - } - if ($restrictionGroups !== []) { - return array_keys($restrictionGroups); + $restrictionGroups = array_merge($restrictionGroups, $this->visibilityService->getVisibilitySettings($appointment)['groups']); } - return $this->configService->getResponseSummaryGroupsMode() === ConfigService::RESPONSE_SUMMARY_MODE_ALL - ? null - : []; + $groups = ConfigService::sectionGroups($this->configService->getResponseSummaryGroupsMode(), $whitelistedGroups, $restrictionGroups); + + return $groups === null ? null : array_values(array_unique(array_map('strtolower', $groups))); } /** diff --git a/lib/Service/ResponseSummaryService.php b/lib/Service/ResponseSummaryService.php index 0004a00a..449a7f8f 100644 --- a/lib/Service/ResponseSummaryService.php +++ b/lib/Service/ResponseSummaryService.php @@ -178,24 +178,13 @@ private function buildCache(Appointment $appointment, array $responses): array { } } - // Pre-fetch group objects and their users for whichever groups can - // render as sections, mirroring isGroupAllowedCached()'s precedence: - // the whitelist in "specific" mode; a restricted appointment's own - // restriction groups regardless of mode (issue #199 — only those can - // ever pass, so fetching every instance group would be wasted work); - // every Nextcloud group otherwise, but only in "all" mode ("none" - // suppresses grouping entirely on a fully open appointment). + // Pre-fetch members only of the groups that can render as sections — + // scanning every instance group for a restricted appointment would be + // wasted work (issue #199). /** @var array> $groupUsers */ $groupUsers = []; - if ($groupsMode === ConfigService::RESPONSE_SUMMARY_MODE_SPECIFIC) { - $sectionGroupIds = $sectionGroups; - } elseif (!empty($appointmentVisibleGroupsLower)) { - $sectionGroupIds = $visibilitySettings['groups']; - } elseif ($groupsMode === ConfigService::RESPONSE_SUMMARY_MODE_ALL) { - $sectionGroupIds = $this->visibilityService->getAllGroupIds(); - } else { - $sectionGroupIds = []; - } + $sectionGroupIds = ConfigService::sectionGroups($groupsMode, $whitelistedGroups, $visibilitySettings['groups']) + ?? $this->visibilityService->getAllGroupIds(); foreach ($sectionGroupIds as $groupId) { $groupUsers[$groupId] = $this->visibilityService->getGroupMembers($groupId); } diff --git a/playwright.config.js b/playwright.config.js index 83515e3e..368cf66d 100644 --- a/playwright.config.js +++ b/playwright.config.js @@ -111,6 +111,8 @@ export default defineConfig({ '39-vacation-calendar.spec.js', // Wipes every appointment and answers as two users. '40-capacity-ui.spec.js', + // Switches the response summary grouping mode and the group list. + '41-checkin-group-filter.spec.js', ], fullyParallel: false, workers: 1, diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 3f519f68..64f08496 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1620,7 +1620,6 @@ - diff --git a/src/components/checkin/CheckinControls.vue b/src/components/checkin/CheckinControls.vue index 37393e0a..9308c090 100644 --- a/src/components/checkin/CheckinControls.vue +++ b/src/components/checkin/CheckinControls.vue @@ -9,7 +9,7 @@ -
+
{ // Opt the system group in: now it should render like any other tracked group. await saveAdminSettings(request, { + responseSummaryGroupsMode: 'specific', whitelistedGroups: ['guest_app'], whitelistedTeams: [], permissions: { @@ -263,6 +264,7 @@ test.describe('Attendance App - Guests integration', () => { // Whitelist guest_app so the select shows it as a selected pill, then // verify the friendly label replaces the raw id. await saveAdminSettings(request, { + responseSummaryGroupsMode: 'specific', whitelistedGroups: ['guest_app'], whitelistedTeams: [], permissions: { @@ -295,6 +297,7 @@ test.describe('Attendance App - Guests integration', () => { data: { email: GUEST_EMAIL_B, displayName: 'E2E Guest B' }, }) await saveAdminSettings(request, { + responseSummaryGroupsMode: 'specific', whitelistedGroups: ['guest_app'], whitelistedTeams: [], permissions: { diff --git a/tests/e2e/4-admin-settings.spec.js b/tests/e2e/4-admin-settings.spec.js index 73ff1242..b6eab1ff 100644 --- a/tests/e2e/4-admin-settings.spec.js +++ b/tests/e2e/4-admin-settings.spec.js @@ -290,6 +290,7 @@ test.describe('Attendance App - Admin Settings', () => { test.beforeEach(async ({ request }) => { await saveAdminSettings(request, { + responseSummaryGroupsMode: 'specific', whitelistedGroups: [firstGroup, secondGroup], whitelistedTeams: [], permissions: { ...PERMISSIVE_PERMISSIONS }, @@ -336,7 +337,7 @@ test.describe('Attendance App - Admin Settings', () => { }) test('should not offer the order list for fewer than two groups', async ({ page, loginAsUser, request }) => { - await saveAdminSettings(request, { whitelistedGroups: [firstGroup] }) + await saveAdminSettings(request, { responseSummaryGroupsMode: 'specific', whitelistedGroups: [firstGroup] }) await loginAsUser('admin', 'admin') await page.goto('/settings/admin/attendance') @@ -425,7 +426,10 @@ test.describe('Attendance App - Admin Settings', () => { await page.goto('/settings/admin/attendance') await page.waitForLoadState('networkidle') - // Find the whitelisted groups selector + // The picker only appears once "Specific groups" mode is selected + const groupingRow = page.locator('[data-test="response-summary-groups"]') + await groupingRow.getByText('Specific groups', { exact: true }).click() + const groupsSelect = page.locator('[data-test="select-whitelisted-groups"]') await expect(groupsSelect).toBeVisible() @@ -468,6 +472,9 @@ test.describe('Attendance App - Admin Settings', () => { await row.getByText('Specific groups', { exact: true }).click() } + const groupingRow = page.locator('[data-test="response-summary-groups"]') + await groupingRow.getByText('Specific groups', { exact: true }).click() + const selectFields = [ 'select-whitelisted-groups', ...permissionNames.map((permission) => `permission-${permission}-groups`), diff --git a/tests/e2e/41-checkin-group-filter.spec.js b/tests/e2e/41-checkin-group-filter.spec.js new file mode 100644 index 00000000..f34aa171 --- /dev/null +++ b/tests/e2e/41-checkin-group-filter.spec.js @@ -0,0 +1,48 @@ +import { createAppointmentViaAPI, deleteAllAppointments, expect, goToCheckin, reloadWebWorkers, resetAdminSettings, saveAdminSettings, test } from './fixtures/nextcloud.js' + +// The check-in list's group filter follows the response summary grouping +// mode: nothing to pick from under "No grouping" (issue #212). +test.describe('Check-in group filter follows the grouping mode', () => { + let appointmentId + + test.beforeAll(async ({ request }) => { + const data = await createAppointmentViaAPI(request, { + name: 'Group Filter Test Meeting', + daysFromNow: 3, + durationHours: 1, + }) + appointmentId = data.id + }) + + test.afterAll(async ({ request }) => { + await resetAdminSettings(request) + await reloadWebWorkers() + await deleteAllAppointments(request) + }) + + test.beforeEach(async ({ loginAsUser }) => { + await loginAsUser('admin', 'admin') + }) + + test('offers the group filter once the admin picked groups', async ({ page, request }) => { + await saveAdminSettings(request, { + responseSummaryGroupsMode: 'specific', + whitelistedGroups: ['admin'], + }) + await reloadWebWorkers() + + await goToCheckin(page, appointmentId) + await expect(page.locator('[data-test="select-group-filter"]')).toBeVisible() + }) + + test('hides the group filter again under "No grouping"', async ({ page, request }) => { + await saveAdminSettings(request, { + responseSummaryGroupsMode: 'none', + whitelistedGroups: [], + }) + await reloadWebWorkers() + + await goToCheckin(page, appointmentId) + await expect(page.locator('[data-test="select-group-filter"]')).toBeHidden() + }) +}) diff --git a/tests/e2e/checkin.spec.js b/tests/e2e/checkin.spec.js index 2cc57962..29125f0b 100644 --- a/tests/e2e/checkin.spec.js +++ b/tests/e2e/checkin.spec.js @@ -1,20 +1,7 @@ -import { test, expect, createAppointmentViaAPI, deleteAllAppointments, checkinUserViaAPI, getResponseSummaryViaAPI } from './fixtures/nextcloud.js' +import { test, expect, createAppointmentViaAPI, deleteAllAppointments, checkinUserViaAPI, getResponseSummaryViaAPI, goToCheckin } from './fixtures/nextcloud.js' let checkinAppointmentId -// Helper to navigate to checkin view and dismiss "future appointment" warning if shown -async function goToCheckin(page, appointmentId) { - await page.goto(`/apps/attendance/checkin/${appointmentId}`) - await page.waitForLoadState('networkidle') - - // Dismiss "appointment is way in the future" warning if present - const continueButton = page.getByRole('button', { name: 'Continue anyway' }) - if (await continueButton.isVisible({ timeout: 2000 }).catch(() => false)) { - await continueButton.click() - await page.waitForLoadState('networkidle') - } -} - test.describe('Attendance App - Check-in Workflow', () => { test.beforeAll(async ({ request }) => { const data = await createAppointmentViaAPI(request, { @@ -57,7 +44,8 @@ test.describe('Attendance App - Check-in Workflow', () => { await expect(page.locator('[data-test="checkin-view"]')).toBeVisible() await expect(page.locator('[data-test="input-search"]')).toBeVisible() - await expect(page.locator('[data-test="select-group-filter"]')).toBeVisible() + // "No grouping" is the default: nothing to filter by (issue #212). + await expect(page.locator('[data-test="select-group-filter"]')).toBeHidden() }) test('should mark user as present', async ({ page }) => { diff --git a/tests/e2e/fixtures/nextcloud.js b/tests/e2e/fixtures/nextcloud.js index 4777c9f3..037b813a 100644 --- a/tests/e2e/fixtures/nextcloud.js +++ b/tests/e2e/fixtures/nextcloud.js @@ -446,11 +446,27 @@ export async function resetAdminSettings(request) { return saveAdminSettings(request, { whitelistedGroups: [], whitelistedTeams: [], + responseSummaryGroupsMode: 'none', + responseSummaryTeamsMode: 'none', permissions: { ...PERMISSIVE_PERMISSIONS }, reminders: { enabled: false, days_before: 1, frequency_days: 1 }, }) } +/** + * Open the check-in view, past the "way in the future" gate if it shows. + */ +export async function goToCheckin(page, appointmentId) { + await page.goto(`/apps/attendance/checkin/${appointmentId}`) + await page.waitForLoadState('networkidle') + + const continueButton = page.getByRole('button', { name: 'Continue anyway' }) + if (await continueButton.isVisible({ timeout: 2000 }).catch(() => false)) { + await continueButton.click() + await page.waitForLoadState('networkidle') + } +} + /** * Force every Apache worker to reload the app config. APCu (the local * memcache) is per-worker, so a permission change saved through one worker diff --git a/tests/unit/Service/CheckinServiceTest.php b/tests/unit/Service/CheckinServiceTest.php index 65a06d39..6b004268 100644 --- a/tests/unit/Service/CheckinServiceTest.php +++ b/tests/unit/Service/CheckinServiceTest.php @@ -23,20 +23,24 @@ class CheckinServiceTest extends TestCase { private AppointmentMapper|MockObject $appointmentMapper; private AttendanceResponseMapper|MockObject $responseMapper; + private ConfigService|MockObject $configService; private VisibilityService|MockObject $visibilityService; + private IGroupManager|MockObject $groupManager; private CheckinService $service; protected function setUp(): void { $this->appointmentMapper = $this->createMock(AppointmentMapper::class); $this->responseMapper = $this->createMock(AttendanceResponseMapper::class); + $this->configService = $this->createMock(ConfigService::class); $this->visibilityService = $this->createMock(VisibilityService::class); + $this->groupManager = $this->createMock(IGroupManager::class); $this->service = new CheckinService( $this->appointmentMapper, $this->responseMapper, - $this->createMock(ConfigService::class), + $this->configService, $this->visibilityService, - $this->createMock(IGroupManager::class), + $this->groupManager, $this->createMock(GuestService::class), $this->createMock(AuditEventService::class), $this->createMock(CapacityService::class), @@ -57,6 +61,93 @@ private function checkin(string $userId, string $state): AttendanceResponse { return $response; } + /** + * @param array> $groupsByUser The audience and each member's groups + */ + private function givenAudience(array $groupsByUser): void { + $this->appointmentMapper->method('find')->willReturn(new Appointment()); + + $attendees = []; + foreach (array_keys($groupsByUser) as $uid) { + $attendees[$uid] = $this->user($uid); + } + $this->visibilityService->method('getTargetAttendees')->willReturn($attendees); + $this->groupManager->method('getUserGroupIds') + ->willReturnCallback(fn (IUser $user) => $groupsByUser[$user->getUID()]); + } + + /** + * @param list $whitelist + * @param list $restrictionGroups The appointment's own visibility groups + */ + private function givenGrouping(string $mode, array $whitelist = [], array $restrictionGroups = []): void { + $this->configService->method('getResponseSummaryGroupsMode')->willReturn($mode); + $this->configService->method('getWhitelistedGroups')->willReturn($whitelist); + $this->visibilityService->method('getVisibilitySettings') + ->willReturn(['users' => [], 'groups' => $restrictionGroups, 'teams' => []]); + } + + /** + * @return array> The groups shipped per user + */ + private function shippedGroups(array $checkinData): array { + return array_column($checkinData['users'], 'groups', 'userId'); + } + + /** + * "No grouping" ships no sections at all (issue #212): the clients render + * a flat list without a group filter. + */ + public function testNoGroupingShipsAFlatList(): void { + $this->givenGrouping(ConfigService::RESPONSE_SUMMARY_MODE_NONE); + $this->givenAudience(['alice' => ['choir', 'board'], 'bob' => []]); + $this->visibilityService->expects($this->never())->method('getAllGroupIds'); + + $data = $this->service->getCheckinData(7); + + $this->assertSame([], $data['userGroups']); + $this->assertSame(['alice' => ['Others'], 'bob' => ['Others']], $this->shippedGroups($data)); + } + + public function testAllGroupsSectionsByEveryGroup(): void { + $this->givenGrouping(ConfigService::RESPONSE_SUMMARY_MODE_ALL); + $this->givenAudience(['alice' => ['choir', 'board'], 'bob' => []]); + $this->visibilityService->method('getAllGroupIds')->willReturn(['board', 'choir', 'guest_app']); + + $data = $this->service->getCheckinData(7); + + $this->assertSame(['board', 'choir', 'Others'], $data['userGroups']); + $this->assertSame(['alice' => ['choir', 'board'], 'bob' => ['Others']], $this->shippedGroups($data)); + } + + /** + * A user carries only the sections they belong to, spelled as the admin's + * list spells them, so clients match them without normalising. + */ + public function testSpecificGroupsSectionsByTheWhitelist(): void { + $this->givenGrouping(ConfigService::RESPONSE_SUMMARY_MODE_SPECIFIC, ['Choir']); + $this->givenAudience(['alice' => ['choir', 'board'], 'bob' => ['board']]); + + $data = $this->service->getCheckinData(7); + + $this->assertSame(['Choir'], $data['userGroups']); + $this->assertSame(['alice' => ['Choir'], 'bob' => ['Others']], $this->shippedGroups($data)); + } + + /** + * A restricted appointment keeps sectioning by the groups that let people + * in, as the summary does (issue #199), even with grouping switched off. + */ + public function testRestrictedAppointmentSectionsByItsOwnGroups(): void { + $this->givenGrouping(ConfigService::RESPONSE_SUMMARY_MODE_NONE, [], ['soprano', 42]); + $this->givenAudience(['alice' => ['soprano'], 'bob' => ['board']]); + + $data = $this->service->getCheckinData(7); + + $this->assertSame(['soprano', '42'], $data['userGroups']); + $this->assertSame(['alice' => ['soprano'], 'bob' => ['Others']], $this->shippedGroups($data)); + } + /** * The list hands over the row's entity and its responses; the summary * counts the audience by check-in state without reading anything itself.