From 72e7e57180ccd4de88636f9afde44d0d8151dbfc Mon Sep 17 00:00:00 2001 From: Florian Ludwig Date: Sat, 15 Aug 2026 21:03:54 +0200 Subject: [PATCH 01/11] fix(calendar): stop pointless writes to the organization calendar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every updateCalendarObject() raises the DAV activity "X updated event Y in calendar Z" for the calendar owner and everybody the calendar is shared with. It fires unconditionally — the dav app never compares content — so a shared organization calendar produced one such notification per answer, attributed to the person who answered, who has never seen the calendar. Two changes, both keeping the write real-time: - Skip the write when the generated document says the same thing as the stored one, ignoring DTSTAMP and LAST-MODIFIED. Those are derived from updatedAt and change whenever the row is touched, so a comment edit or a repeated identical answer produced a write that changed not one visible character. - Add an admin switch for the response summary block. Nextcloud's own counter-setting is per activity type, not per calendar, so switching it off there silences every calendar the person has; this one is narrow. Default on, and flipping it backfills, so switching off also takes the block out of the events that already carry it. Unfolding an iCal document now lives in IcalService next to the folding it inverts, rather than being written out a second time in the comparison. --- lib/Controller/AdminController.php | 3 +- lib/ResponseDefinitions.php | 1 + lib/Service/ConfigService.php | 26 ++++++ lib/Service/IcalService.php | 27 ++++++ lib/Service/OrgCalendarSyncService.php | 90 +++++++++++++++---- openapi-administration.json | 9 +- openapi-full.json | 9 +- src/views/AdminSettings.vue | 16 +++- .../Service/OrgCalendarSyncServiceTest.php | 90 ++++++++++++++++++- 9 files changed, 251 insertions(+), 20 deletions(-) diff --git a/lib/Controller/AdminController.php b/lib/Controller/AdminController.php index 22e3bb02..7b43f547 100644 --- a/lib/Controller/AdminController.php +++ b/lib/Controller/AdminController.php @@ -150,6 +150,7 @@ public function getSettings(): DataResponse { 'enabled' => $this->configService->isOrgCalendarEnabled(), 'calendarUri' => $this->configService->getOrgCalendarUri() ?: null, 'userId' => $this->configService->getOrgCalendarUserId() ?: null, + 'summary' => $this->configService->isOrgCalendarSummaryEnabled(), ], 'audit' => [ 'enabled' => $this->configService->isAuditLogEnabled(), @@ -190,7 +191,7 @@ public function getSettings(): DataResponse { * @param ?array}> $permissions Permission name to access mode (all|groups|nobody) and group IDs * @param ?array{enabled?: bool, reminderDays?: int, reminderFrequency?: int, reminderTarget?: string} $reminders Reminder settings * @param ?array{enabled?: bool} $calendarSync Calendar sync settings - * @param ?array{enabled?: bool, calendarUri?: string} $orgCalendar Organization calendar settings (target calendar for automatic event creation) + * @param ?array{enabled?: bool, calendarUri?: string, summary?: bool} $orgCalendar Organization calendar settings (target calendar for automatic event creation) * @param ?array{enabled?: bool, visibility?: string} $audit Audit log settings (master switch + read visibility) * @param ?string $displayOrder Display order for appointments: chronological, name, or group * @param ?bool $pushEnabled Whether push notifications are enabled diff --git a/lib/ResponseDefinitions.php b/lib/ResponseDefinitions.php index 63893312..1f88c635 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -231,6 +231,7 @@ * enabled: bool, * calendarUri: ?string, * userId: ?string, + * summary: bool, * } * @psalm-type AttendanceWritableCalendar = array{ * uri: string, diff --git a/lib/Service/ConfigService.php b/lib/Service/ConfigService.php index bc9265df..0ca1d3eb 100644 --- a/lib/Service/ConfigService.php +++ b/lib/Service/ConfigService.php @@ -277,6 +277,32 @@ public function setOrgCalendarUserId(string $userId): void { $this->config->setAppValue(self::APP_ID, 'org_calendar_user_id', $userId); } + /** + * Whether the response summary is carried in the organization calendar + * event's description. + * + * Every write to a calendar object raises the DAV activity "X updated event + * Y in calendar Z" for the owner and everybody the calendar is shared with — + * unconditionally, without comparing content. Carrying the summary therefore + * costs one such activity per answer. Nextcloud's own counter-setting is per + * activity type, not per calendar, so switching it off there silences every + * calendar the person has. This switch is the narrow one. + * + * Defaults to on, so an install that already carries the summary keeps it. + */ + public function isOrgCalendarSummaryEnabled(): bool { + return $this->appConfig->getValueBool(self::APP_ID, 'org_calendar_summary', true); + } + + /** + * Set whether the response summary is carried in the calendar event. + * + * @param bool $enabled Whether the summary should be carried + */ + public function setOrgCalendarSummaryEnabled(bool $enabled): void { + $this->appConfig->setValueBool(self::APP_ID, 'org_calendar_summary', $enabled); + } + /** * Check if push notifications are enabled. * diff --git a/lib/Service/IcalService.php b/lib/Service/IcalService.php index bb4f476c..52614210 100644 --- a/lib/Service/IcalService.php +++ b/lib/Service/IcalService.php @@ -434,6 +434,33 @@ public function foldIcalContent(string $content): string { return implode("\r\n", $folded) . "\r\n"; } + /** + * The inverse of foldIcalContent(): normalize line endings and rejoin the + * continuation lines, so every property is one array entry again. Lives + * beside its counterpart because both encode the same RFC 5545 Section 3.1 + * rule, and a reader that disagrees with the writer about where a line ends + * corrupts whatever it parses. + * + * Static because it is a pure transform, so callers can reach it without a + * container and tests never have to restate the rule in a stub. + * + * @return list + */ + public static function unfoldIcalContent(string $content): array { + $normalized = str_replace(["\r\n", "\r"], "\n", $content); + $normalized = preg_replace("/\n[ \t]/", '', $normalized) ?? $normalized; + + return explode("\n", trim($normalized)); + } + + /** + * The property name of an unfolded line, upper-cased and without its + * parameters — "DTSTART" for both `DTSTART:…` and `DTSTART;TZID=…:…`. + */ + public static function icalPropertyName(string $line): string { + return strtoupper(substr($line, 0, strcspn($line, ';:'))); + } + /** * Fold a single iCal line to max 75 octets, preserving UTF-8 boundaries. */ diff --git a/lib/Service/OrgCalendarSyncService.php b/lib/Service/OrgCalendarSyncService.php index fc96ca95..c2f7a6de 100644 --- a/lib/Service/OrgCalendarSyncService.php +++ b/lib/Service/OrgCalendarSyncService.php @@ -52,6 +52,16 @@ class OrgCalendarSyncService { */ public const SUMMARY_SEPARATOR = '--- Attendance ---'; + /** + * Properties managedProperties() derives from the appointment's updatedAt, + * so they change whenever the row is touched — on a comment edit or a + * repeated identical answer just as much as on a real change. Skipping them + * when comparing is what makes those writes skippable. Keep this in step + * with managedProperties(): a further per-write property left out here makes + * every document look different and quietly retires the skip. + */ + private const VOLATILE_PROPERTIES = ['DTSTAMP', 'LAST-MODIFIED']; + /** @var array{0: int, 1: string}|false|null Memoized target; false = resolution failed */ private array|false|null $resolvedTarget = null; private ?object $calDavBackend = null; @@ -88,7 +98,7 @@ public function isEnabled(): bool { * appointments when the feature was enabled or re-pointed. Owning the * transition here keeps controller and any future callers consistent. * - * @param array{enabled?: bool, calendarUri?: string} $orgCalendar Settings payload + * @param array{enabled?: bool, calendarUri?: string, summary?: bool} $orgCalendar Settings payload * @param string $actingUserId The admin performing the change */ public function applySettings(array $orgCalendar, string $actingUserId): void { @@ -99,6 +109,13 @@ public function applySettings(array $orgCalendar, string $actingUserId): void { $this->configService->setOrgCalendarEnabled((bool)$orgCalendar['enabled']); } + if (isset($orgCalendar['summary'])) { + // Backfill in both directions: switching off has to take the block + // out of the events that already carry it, not just stop adding it. + $changed = $changed || $this->configService->isOrgCalendarSummaryEnabled() !== $orgCalendar['summary']; + $this->configService->setOrgCalendarSummaryEnabled($orgCalendar['summary']); + } + if (isset($orgCalendar['calendarUri']) && $orgCalendar['calendarUri'] !== '') { $oldUri = $this->configService->getOrgCalendarUri(); $this->configService->setOrgCalendarUri($orgCalendar['calendarUri']); @@ -156,20 +173,19 @@ public function syncAppointment(Appointment $appointment): bool { $ics = $existingIcs !== null ? $this->patchIcs($existingIcs, $appointment) : $this->buildIcs($appointment, $uid); + if ($existingIcs !== null && $this->matchesIgnoringTimestamps($existingIcs, $ics)) { + // Nothing the reader would see changed. Writing anyway would + // still raise a calendar activity for everybody the calendar + // is shared with — see isOrgCalendarSummaryEnabled(). + $this->linkAppointment($appointment, $uid, $ownerCalendarUri); + return false; + } $backend->updateCalendarObject($calendarId, $objectUri, $ics); } else { $backend->createCalendarObject($calendarId, $objectUri, $this->buildIcs($appointment, $uid)); } - // Store the link with the owner's calendar URI — calendar events - // dispatched by the server carry that URI, so the existing - // CalendarObjectUpdateListener can match edits back to us. - if ($appointment->getCalendarEventUid() !== $uid - || $appointment->getCalendarUri() !== $ownerCalendarUri) { - $appointment->setCalendarUri($ownerCalendarUri); - $appointment->setCalendarEventUid($uid); - $this->appointmentMapper->update($appointment); - } + $this->linkAppointment($appointment, $uid, $ownerCalendarUri); return true; } catch (\Throwable $e) { @@ -247,6 +263,49 @@ public function syncAllUpcoming(): int { return $count; } + /** + * Store the link with the owner's calendar URI — calendar events dispatched + * by the server carry that URI, so the existing CalendarObjectUpdateListener + * can match edits back to us. Also runs when the write itself was skipped: + * the link is what makes an event ours, and a skipped write must not leave + * it unset. + */ + private function linkAppointment(Appointment $appointment, string $uid, string $ownerCalendarUri): void { + if ($appointment->getCalendarEventUid() === $uid + && $appointment->getCalendarUri() === $ownerCalendarUri) { + return; + } + $appointment->setCalendarUri($ownerCalendarUri); + $appointment->setCalendarEventUid($uid); + $this->appointmentMapper->update($appointment); + } + + /** + * Whether two VCALENDAR documents say the same thing to a reader. + * + * VOLATILE_PROPERTIES are excluded — see the constant. Folding and line + * endings are normalized first, because the stored document may have been + * folded by the Calendar app rather than by us. + */ + public function matchesIgnoringTimestamps(string $left, string $right): bool { + return $this->comparableLines($left) === $this->comparableLines($right); + } + + /** + * @return list Unfolded lines without the volatile timestamps + */ + private function comparableLines(string $ics): array { + $lines = []; + foreach (IcalService::unfoldIcalContent($ics) as $line) { + if (in_array(IcalService::icalPropertyName($line), self::VOLATILE_PROPERTIES, true)) { + continue; + } + $lines[] = $line; + } + + return $lines; + } + /** * Deterministic VEVENT UID for an appointment. */ @@ -313,10 +372,7 @@ public function buildIcs(Appointment $appointment, string $uid): string { * to a full rebuild if no VEVENT is found. */ public function patchIcs(string $existingIcs, Appointment $appointment): string { - // Normalize newlines and unfold continuation lines (RFC 5545 3.1) - $content = str_replace(["\r\n", "\r"], "\n", $existingIcs); - $content = preg_replace("/\n[ \t]/", '', $content) ?? $content; - $lines = explode("\n", trim($content)); + $lines = IcalService::unfoldIcalContent($existingIcs); $props = $this->managedProperties($appointment); $result = []; @@ -356,7 +412,7 @@ public function patchIcs(string $existingIcs, Appointment $appointment): string continue; } if ($nested === 0) { - $name = strtoupper(substr($line, 0, strcspn($line, ';:'))); + $name = IcalService::icalPropertyName($line); if (array_key_exists($name, $props)) { $newLine = $props[$name]; unset($props[$name]); @@ -434,6 +490,10 @@ private function resolveCategoryName(?int $categoryId): ?string { private function buildDescription(Appointment $appointment): string { $description = trim($appointment->getDescription() ?? ''); + if (!$this->configService->isOrgCalendarSummaryEnabled()) { + return $description; + } + $summary = $this->buildResponseSummary($appointment); if ($summary !== null) { $description = ($description !== '' ? $description . "\n\n" : '') diff --git a/openapi-administration.json b/openapi-administration.json index b98fa89a..c5692f8f 100644 --- a/openapi-administration.json +++ b/openapi-administration.json @@ -117,7 +117,8 @@ "required": [ "enabled", "calendarUri", - "userId" + "userId", + "summary" ], "properties": { "enabled": { @@ -130,6 +131,9 @@ "userId": { "type": "string", "nullable": true + }, + "summary": { + "type": "boolean" } } }, @@ -657,6 +661,9 @@ }, "calendarUri": { "type": "string" + }, + "summary": { + "type": "boolean" } } }, diff --git a/openapi-full.json b/openapi-full.json index 63397456..9b3c592a 100644 --- a/openapi-full.json +++ b/openapi-full.json @@ -117,7 +117,8 @@ "required": [ "enabled", "calendarUri", - "userId" + "userId", + "summary" ], "properties": { "enabled": { @@ -130,6 +131,9 @@ "userId": { "type": "string", "nullable": true + }, + "summary": { + "type": "boolean" } } }, @@ -7392,6 +7396,9 @@ }, "calendarUri": { "type": "string" + }, + "summary": { + "type": "boolean" } } }, diff --git a/src/views/AdminSettings.vue b/src/views/AdminSettings.vue index 2a690e5c..2c6b1a5a 100644 --- a/src/views/AdminSettings.vue +++ b/src/views/AdminSettings.vue @@ -411,6 +411,17 @@

{{ t('attendance', 'Events are written using the account of {user}.', { user: orgCalendarUserId }) }}

+ + + + {{ t('attendance', 'Show the response summary in the calendar event') }} + +

+ {{ t('attendance', 'Everyone the calendar is shared with sees how many people accepted, without opening the app. Keeping it current means writing to the event after every answer, and Nextcloud reports each of those writes as a calendar change.') }} +

({ +autoSave([orgCalendarEnabled, selectedOrgCalendar, orgCalendarSummary], 'orgCalendar', () => ({ orgCalendar: { enabled: orgCalendarEnabled.value, + summary: orgCalendarSummary.value, ...(selectedOrgCalendar.value?.uri ? { calendarUri: selectedOrgCalendar.value.uri } : {}), }, }), SELECT_DEBOUNCE) @@ -1119,6 +1132,7 @@ async function loadSettings() { if (config.orgCalendar) { orgCalendarEnabled.value = config.orgCalendar.enabled || false orgCalendarUserId.value = config.orgCalendar.userId || null + orgCalendarSummary.value = config.orgCalendar.summary !== false const storedUri = config.orgCalendar.calendarUri if (storedUri) { selectedOrgCalendar.value = writableCalendars.value.find((c) => c.uri === storedUri) diff --git a/tests/unit/Service/OrgCalendarSyncServiceTest.php b/tests/unit/Service/OrgCalendarSyncServiceTest.php index 2ace8c6d..2c0eb588 100644 --- a/tests/unit/Service/OrgCalendarSyncServiceTest.php +++ b/tests/unit/Service/OrgCalendarSyncServiceTest.php @@ -191,10 +191,11 @@ protected function setUp(): void { /** @var array */ private array $responseSummary = []; - private function configureEnabled(): void { + private function configureEnabled(bool $summary = true): void { $this->configService->method('isOrgCalendarEnabled')->willReturn(true); $this->configService->method('getOrgCalendarUri')->willReturn('org-events'); $this->configService->method('getOrgCalendarUserId')->willReturn('admin'); + $this->configService->method('isOrgCalendarSummaryEnabled')->willReturn($summary); $this->calendarService->method('findWritableCalendar') ->with('admin', 'org-events') @@ -469,6 +470,68 @@ public function testNoSummaryBlockWithoutResponses(): void { $this->assertStringNotContainsString(OrgCalendarSyncService::SUMMARY_SEPARATOR, $this->backend->created[0][2]); } + public function testSummaryOmittedWhenTheAdminSwitchedItOff(): void { + $this->configureEnabled(false); + $appointment = $this->buildAppointment(); + $this->appointmentMapper->method('update')->willReturnArgument(0); + $this->responseSummary = ['yes' => 2, 'no' => 1, 'maybe' => 1]; + + $this->assertTrue($this->service->syncAppointment($appointment)); + + $ics = $this->backend->created[0][2]; + $this->assertStringNotContainsString(OrgCalendarSyncService::SUMMARY_SEPARATOR, $ics); + $this->assertStringContainsString('DESCRIPTION:Bring instruments', $ics); + } + + /** + * Every write raises a calendar activity for everybody the calendar is + * shared with, so a write that changes nothing visible is pure noise. + */ + public function testWriteSkippedWhenOnlyTheTimestampsWouldChange(): void { + $this->configureEnabled(); + $appointment = $this->buildAppointment(); + $appointment->setCalendarUri('org-events-owner-uri'); + $appointment->setCalendarEventUid('attendance-org-5@cloud.example.com'); + + // What a previous sync left behind, with a stale DTSTAMP/LAST-MODIFIED + $stored = $this->service->buildIcs($appointment, 'attendance-org-5@cloud.example.com'); + $this->backend->existingObjects['attendance-org-5@cloud.example.com.ics'] = [ + 'id' => 1, + 'calendardata' => str_replace('20260801T100000Z', '20260101T090000Z', $stored), + ]; + + $this->assertFalse($this->service->syncAppointment($appointment)); + $this->assertSame([], $this->backend->updated); + } + + public function testWriteHappensWhenTheSummaryLineMoved(): void { + $this->configureEnabled(); + $appointment = $this->buildAppointment(); + $appointment->setCalendarUri('org-events-owner-uri'); + $appointment->setCalendarEventUid('attendance-org-5@cloud.example.com'); + + $this->responseSummary = ['yes' => 2, 'no' => 0, 'maybe' => 0]; + $stored = $this->service->buildIcs($appointment, 'attendance-org-5@cloud.example.com'); + $this->backend->existingObjects['attendance-org-5@cloud.example.com.ics'] = [ + 'id' => 1, + 'calendardata' => $stored, + ]; + + $this->responseSummary = ['yes' => 3, 'no' => 0, 'maybe' => 0]; + + $this->assertTrue($this->service->syncAppointment($appointment)); + $this->assertCount(1, $this->backend->updated); + } + + public function testComparisonIgnoresFoldingAndLineEndings(): void { + $folded = "BEGIN:VEVENT\r\nDESCRIPTION:A very long descripti\r\n on that got folded\r\nEND:VEVENT\r\n"; + $unfolded = "BEGIN:VEVENT\nDESCRIPTION:A very long description that got folded\nEND:VEVENT"; + $this->assertTrue($this->service->matchesIgnoringTimestamps($folded, $unfolded)); + + $other = "BEGIN:VEVENT\nDESCRIPTION:Something else\nEND:VEVENT"; + $this->assertFalse($this->service->matchesIgnoringTimestamps($folded, $other)); + } + public function testStripResponseSummary(): void { $description = "Bring instruments\n\n" . OrgCalendarSyncService::SUMMARY_SEPARATOR . "\n2 attending, 1 declined, 1 maybe"; $this->assertSame('Bring instruments', OrgCalendarSyncService::stripResponseSummary($description)); @@ -523,6 +586,31 @@ public function testApplySettingsStoresActingUserOnCalendarChange(): void { $this->service->applySettings(['calendarUri' => 'new-uri'], 'acting-admin'); } + /** + * Switching the summary off has to take the block out of the events that + * already carry it, so the toggle counts as a change worth backfilling. + */ + public function testApplySettingsBackfillsWhenTheSummaryToggleFlips(): void { + $this->configService->method('isOrgCalendarEnabled')->willReturn(true); + $this->configService->method('getOrgCalendarUri')->willReturn('org-events'); + $this->configService->method('getOrgCalendarUserId')->willReturn('admin'); + $this->configService->method('isOrgCalendarSummaryEnabled')->willReturn(true); + $this->configService->expects($this->once())->method('setOrgCalendarSummaryEnabled')->with(false); + $this->appointmentMapper->expects($this->once())->method('findUpcoming')->willReturn([]); + + $this->service->applySettings(['summary' => false], 'admin'); + } + + public function testApplySettingsLeavesTheSummaryToggleAloneWhenUnchanged(): void { + $this->configService->method('isOrgCalendarEnabled')->willReturn(true); + $this->configService->method('getOrgCalendarUri')->willReturn('org-events'); + $this->configService->method('getOrgCalendarUserId')->willReturn('admin'); + $this->configService->method('isOrgCalendarSummaryEnabled')->willReturn(true); + $this->appointmentMapper->expects($this->never())->method('findUpcoming'); + + $this->service->applySettings(['summary' => true], 'admin'); + } + public function testApplySettingsKeepsStoredUserWhenUriUnchanged(): void { $this->configService->method('isOrgCalendarEnabled')->willReturn(true); $this->configService->method('getOrgCalendarUri')->willReturn('org-events'); From 269b9b0cd80cc0d216a6001ce95cbadbe02ce1c7 Mon Sep 17 00:00:00 2001 From: Florian Ludwig Date: Sat, 15 Aug 2026 21:04:15 +0200 Subject: [PATCH 02/11] feat(statistics): list a person's appointments newest first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking a name opens that person's appointments in the order the timeline needs them — oldest first. But the drill-down answers "what has this person been doing lately", not "how did the season run", so it should open at the recent end. Reversed in getPersonDetail() rather than in the query: the same findForStatistics() call feeds the chronological timeline chart, which has to stay ascending. Doing it server-side keeps web and mobile in step, and changes no field, so no client breaks on it. --- lib/Service/StatisticsService.php | 7 ++++-- tests/unit/Service/StatisticsServiceTest.php | 25 ++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/lib/Service/StatisticsService.php b/lib/Service/StatisticsService.php index 0c4d16f4..fcf294d7 100644 --- a/lib/Service/StatisticsService.php +++ b/lib/Service/StatisticsService.php @@ -117,7 +117,10 @@ public function getStatistics(StatisticsFilter $filter, ?string $limitToUserId = } /** - * One person's appointments in the filtered range, for the drill-down. + * One person's appointments in the filtered range, for the drill-down, + * newest first — the drill-down answers "what has this person been doing + * lately", not "how did the season run". The chronological order the + * timeline needs stays with the timeline. * * @param bool $withComments Whether the viewer may read this person's comments * @return StatsPersonDetail @@ -153,7 +156,7 @@ public function getPersonDetail(StatisticsFilter $filter, string $userId, bool $ 'userId' => $userId, 'displayName' => $user?->getDisplayName() ?? $userId, 'isGuest' => $this->guestService->isGuestUser($userId), - 'entries' => $entries, + 'entries' => array_reverse($entries), ]; } diff --git a/tests/unit/Service/StatisticsServiceTest.php b/tests/unit/Service/StatisticsServiceTest.php index 54136aef..eb1b481f 100644 --- a/tests/unit/Service/StatisticsServiceTest.php +++ b/tests/unit/Service/StatisticsServiceTest.php @@ -276,6 +276,31 @@ public function testPersonDetailListsTheAppointmentsBehindTheRow(): void { $this->assertTrue($detail['entries'][0]['attendanceRecorded']); } + /** + * The drill-down reads newest first, while findForStatistics() — which also + * feeds the chronological timeline — hands them over oldest first. + */ + public function testPersonDetailListsTheNewestAppointmentFirst(): void { + $this->givenUsers(['alice' => 'Alice']); + $this->givenUnrestrictedVisibility(); + $this->givenGroupSections(); + + $this->appointmentMapper->method('findForStatistics')->willReturn([ + $this->appointment(1, '2026-05-01 18:00:00', '2026-05-01 20:00:00'), + $this->appointment(2, '2026-06-01 18:00:00', '2026-06-01 20:00:00'), + $this->appointment(3, '2026-07-01 18:00:00', '2026-07-01 20:00:00'), + ]); + $this->givenResponses([ + $this->response(1, 'alice', 'yes', 'yes'), + $this->response(2, 'alice', 'no', ''), + $this->response(3, 'alice', 'maybe', ''), + ]); + + $detail = $this->service->getPersonDetail($this->filter(), 'alice'); + + $this->assertSame([3, 2, 1], array_column($detail['entries'], 'appointmentId')); + } + public function testPersonDetailWithholdsCommentsUnlessAsked(): void { $this->givenUsers(['alice' => 'Alice']); $this->givenUnrestrictedVisibility(); From ebc0bfa3bacacc60cd4004a6ecb24ea4e4fd7994 Mon Sep 17 00:00:00 2001 From: Florian Ludwig Date: Sat, 15 Aug 2026 21:04:35 +0200 Subject: [PATCH 03/11] feat(statistics): count how often an acceptance got a place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Managers who use the planning mode want to know how often somebody who said yes was then actually scheduled — the counterpart to the acceptance rate, which only says how often they offered. The denominator is the careful part. A yes counts only once the inquiry is closed AND somebody was actually scheduled for it, mirroring BookingService::isScheduledOut(): before closing nothing is decided, so a person who accepted yesterday would otherwise read as "not scheduled", and an inquiry closed without scheduling anyone is one where the feature was not used — counting it would measure the manager rather than the person. This is the same shape the attendance rate already uses ("over AND check-in list worked"), now the third denominator in the evaluation. The response carries schedulingEnabled so every consumer — table, export, mobile — reads the rule from the one place that applied it, instead of asking the config again in its own layer. The columns and the ODS export follow it. The effective-status precedence (booking_status wins, booking_notified_status is the fallback) moves into a static on BookingService, so the bulk query can apply it without hydrating the entity it exists to avoid, and without restating the rule. --- lib/Db/AttendanceResponseMapper.php | 10 +- lib/ResponseDefinitions.php | 13 ++ lib/Service/BookingService.php | 12 +- lib/Service/StatisticsExportService.php | 51 ++++++-- lib/Service/StatisticsService.php | 49 +++++-- lib/Service/StatisticsTally.php | 29 ++++- openapi-full.json | 73 ++++++++++- openapi.json | 73 ++++++++++- tests/unit/Service/StatisticsServiceTest.php | 127 ++++++++++++++++++- 9 files changed, 400 insertions(+), 37 deletions(-) diff --git a/lib/Db/AttendanceResponseMapper.php b/lib/Db/AttendanceResponseMapper.php index 2f3d8a19..342a6393 100644 --- a/lib/Db/AttendanceResponseMapper.php +++ b/lib/Db/AttendanceResponseMapper.php @@ -4,6 +4,7 @@ namespace OCA\Attendance\Db; +use OCA\Attendance\Service\BookingService; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\QBMapper; use OCP\DB\QueryBuilder\IQueryBuilder; @@ -81,7 +82,7 @@ public function findByAppointment(int $appointmentId): array { * @param list $appointmentIds * @param ?string $userId Restrict to one person, for the drill-down * @param bool $withComments Read the comment column too — only the drill-down can afford to, it being one person's rows - * @return list + * @return list */ public function findStatisticsRows(array $appointmentIds, ?string $userId = null, bool $withComments = false): array { if ($appointmentIds === []) { @@ -89,7 +90,7 @@ public function findStatisticsRows(array $appointmentIds, ?string $userId = null } $qb = $this->db->getQueryBuilder(); - $qb->select('appointment_id', 'user_id', 'response', 'checkin_state') + $qb->select('appointment_id', 'user_id', 'response', 'checkin_state', 'booking_status', 'booking_notified_status') ->from($this->getTableName()) ->where( $qb->expr()->in('appointment_id', $qb->createNamedParameter($appointmentIds, IQueryBuilder::PARAM_INT_ARRAY)) @@ -113,6 +114,11 @@ public function findStatisticsRows(array $appointmentIds, ?string $userId = null 'userId' => (string)$row['user_id'], 'response' => $row['response'] !== null ? (string)$row['response'] : null, 'checkinState' => $row['checkin_state'] !== null ? (string)$row['checkin_state'] : null, + // The status as the person was actually told it + 'bookingStatus' => BookingService::effectiveStatusOf( + $row['booking_status'] !== null ? (string)$row['booking_status'] : null, + $row['booking_notified_status'] !== null ? (string)$row['booking_notified_status'] : null, + ), ]; if ($withComments) { $mapped['comment'] = isset($row['comment']) ? (string)$row['comment'] : null; diff --git a/lib/ResponseDefinitions.php b/lib/ResponseDefinitions.php index 1f88c635..75bb0ea6 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -338,9 +338,13 @@ * notRecorded: int, * attendanceBase: int, * noShow: int, + * scheduled: int, + * notScheduled: int, + * schedulingBase: int, * responseRate: ?float, * acceptRate: ?float, * attendanceRate: ?float, + * scheduledRate: ?float, * } * @psalm-type AttendanceStatisticsTotals = array{ * targetCount: int, @@ -353,9 +357,13 @@ * notRecorded: int, * attendanceBase: int, * noShow: int, + * scheduled: int, + * notScheduled: int, + * schedulingBase: int, * responseRate: ?float, * acceptRate: ?float, * attendanceRate: ?float, + * scheduledRate: ?float, * } * @psalm-type AttendanceStatisticsSection = array{ * id: string, @@ -371,9 +379,13 @@ * notRecorded: int, * attendanceBase: int, * noShow: int, + * scheduled: int, + * notScheduled: int, + * schedulingBase: int, * responseRate: ?float, * acceptRate: ?float, * attendanceRate: ?float, + * scheduledRate: ?float, * } * @psalm-type AttendanceStatisticsTimelinePoint = array{ * appointmentId: int, @@ -400,6 +412,7 @@ * pastCount: int, * attendanceRecordedCount: int, * groupBy: string, + * schedulingEnabled: bool, * people: list, * sections: list, * totals: AttendanceStatisticsTotals, diff --git a/lib/Service/BookingService.php b/lib/Service/BookingService.php index b8495be6..349c2239 100644 --- a/lib/Service/BookingService.php +++ b/lib/Service/BookingService.php @@ -169,7 +169,17 @@ public function isScheduledIn(Appointment $appointment, string $userId): bool { * appointment where planning was never used stays unmarked. */ public function effectiveBookingStatus(AttendanceResponse $response): ?string { - return $response->getBookingStatus() ?? $response->getBookingNotifiedStatus(); + return self::effectiveStatusOf($response->getBookingStatus(), $response->getBookingNotifiedStatus()); + } + + /** + * The same precedence over raw column values, for callers that read the two + * columns without hydrating the entity — the statistics evaluation reads + * hundreds of thousands of rows and exists precisely to skip that. The rule + * itself must not be restated there; it lives here. + */ + public static function effectiveStatusOf(?string $bookingStatus, ?string $notifiedStatus): ?string { + return $bookingStatus ?? $notifiedStatus; } /** diff --git a/lib/Service/StatisticsExportService.php b/lib/Service/StatisticsExportService.php index 290c9a69..1883dfc1 100644 --- a/lib/Service/StatisticsExportService.php +++ b/lib/Service/StatisticsExportService.php @@ -46,15 +46,17 @@ public function exportToOds(string $userId, StatisticsFilter $filter): array { $sectionNames[$section['id']] = $section['displayName']; } + $scheduling = $statistics['schedulingEnabled']; + $content = $this->odsWriter->write( $this->odsWriter->renderTable( // TRANSLATORS Name of the spreadsheet sheet holding one row per person. The same word labels the person-count column on the other sheet. $this->l10n->t('People'), - $this->peopleRows($statistics['people'], $statistics['totals'], $sectionNames), + $this->peopleRows($statistics['people'], $statistics['totals'], $sectionNames, $scheduling), ) . $this->odsWriter->renderTable( $this->l10n->t('Groups'), - $this->sectionRows($statistics['sections'], $statistics['totals']), + $this->sectionRows($statistics['sections'], $statistics['totals'], $scheduling), ), ); @@ -67,8 +69,8 @@ public function exportToOds(string $userId, StatisticsFilter $filter): array { * @param array $sectionNames * @return list> */ - private function peopleRows(array $people, array $totals, array $sectionNames): array { - $rows = [$this->header([$this->l10n->t('Name'), $this->l10n->t('Groups')])]; + private function peopleRows(array $people, array $totals, array $sectionNames, bool $scheduling): array { + $rows = [$this->header([$this->l10n->t('Name'), $this->l10n->t('Groups')], $scheduling)]; foreach ($people as $person) { $names = array_map( @@ -77,13 +79,13 @@ private function peopleRows(array $people, array $totals, array $sectionNames): ); $rows[] = array_merge( [$person['displayName'], implode(', ', $names)], - $this->counts($person), + $this->counts($scheduling, $person), ); } $rows[] = array_merge( [['value' => $this->l10n->t('Total'), 'style' => OdsWriter::STYLE_SECTION], ''], - $this->counts($totals), + $this->counts($scheduling, $totals), ); return $rows; @@ -94,31 +96,36 @@ private function peopleRows(array $people, array $totals, array $sectionNames): * @param StatsCounts $totals * @return list> */ - private function sectionRows(array $sections, array $totals): array { + private function sectionRows(array $sections, array $totals, bool $scheduling): array { // TRANSLATORS "People" here is the column counting how many people are in the group, and also names the other sheet. - $rows = [$this->header([$this->l10n->t('Group'), $this->l10n->t('People')])]; + $rows = [$this->header([$this->l10n->t('Group'), $this->l10n->t('People')], $scheduling)]; foreach ($sections as $section) { $rows[] = array_merge( [$section['displayName'], ['value' => $section['personCount'], 'type' => 'float']], - $this->counts($section), + $this->counts($scheduling, $section), ); } $rows[] = array_merge( [['value' => $this->l10n->t('Total'), 'style' => OdsWriter::STYLE_SECTION], ''], - $this->counts($totals), + $this->counts($scheduling, $totals), ); return $rows; } /** + * Kept in the same shape as counts() below — the two are positional lists + * that have to stay index-aligned, and matching notation is what makes a + * misalignment visible while editing. + * * @param list $leading * @return list */ - private function header(array $leading): array { - $labels = array_merge($leading, [ + private function header(array $leading, bool $scheduling): array { + $labels = [ + ...$leading, $this->l10n->t('Appointments'), $this->l10n->t('Yes'), $this->l10n->t('No'), @@ -128,10 +135,19 @@ private function header(array $leading): array { $this->l10n->t('Absent'), $this->l10n->t('Not recorded'), $this->l10n->t('No-show'), + ...($scheduling ? [ + $this->l10n->t('Scheduled'), + // TRANSLATORS: Column header — the person accepted and the inquiry was closed, but they did not get a place. + $this->l10n->t('Not scheduled'), + ] : []), $this->l10n->t('Response rate'), $this->l10n->t('Acceptance rate'), $this->l10n->t('Attendance rate'), - ]); + ...($scheduling ? [ + // TRANSLATORS: Column header — share of the person's acceptances that got a place, counted only over closed inquiries where somebody was scheduled. + $this->l10n->t('Scheduling rate'), + ] : []), + ]; return array_map( static fn (string $label): array => ['value' => $label, 'style' => OdsWriter::STYLE_HEADER], @@ -143,7 +159,7 @@ private function header(array $leading): array { * @param StatsCounts|StatsPerson|StatsSection $counts * @return list */ - private function counts(array $counts): array { + private function counts(bool $scheduling, array $counts): array { return [ ['value' => $counts['targetCount'], 'type' => 'float'], ['value' => $counts['yes'], 'type' => 'float'], @@ -154,9 +170,16 @@ private function counts(array $counts): array { ['value' => $counts['absent'], 'type' => 'float'], ['value' => $counts['notRecorded'], 'type' => 'float'], ['value' => $counts['noShow'], 'type' => 'float'], + ...($scheduling ? [ + ['value' => $counts['scheduled'], 'type' => 'float'], + ['value' => $counts['notScheduled'], 'type' => 'float'], + ] : []), ['value' => $counts['responseRate'], 'type' => 'percentage'], ['value' => $counts['acceptRate'], 'type' => 'percentage'], ['value' => $counts['attendanceRate'], 'type' => 'percentage'], + ...($scheduling ? [ + ['value' => $counts['scheduledRate'], 'type' => 'percentage'], + ] : []), ]; } diff --git a/lib/Service/StatisticsService.php b/lib/Service/StatisticsService.php index fcf294d7..ca554e88 100644 --- a/lib/Service/StatisticsService.php +++ b/lib/Service/StatisticsService.php @@ -18,15 +18,18 @@ * Cross-appointment evaluation: how each person answered, and whether they * actually turned up. * - * Two denominators, deliberately different. The response rate counts every + * Three denominators, deliberately different. The response rate counts every * appointment a person was addressed to, upcoming ones included. The * attendance rate counts only appointments that are over *and* had at least * one check-in recorded — otherwise a person's number would depend on how - * diligently somebody else worked the check-in list. + * diligently somebody else worked the check-in list. The scheduled rate counts + * only the yes-answers on inquiries that are closed *and* gave somebody a + * place: before closing nothing is decided, and an inquiry nobody was scheduled + * for is one where the feature was not used. * - * @psalm-type StatsCounts = array{targetCount: int, yes: int, no: int, maybe: int, noResponse: int, present: int, absent: int, notRecorded: int, attendanceBase: int, noShow: int, responseRate: ?float, acceptRate: ?float, attendanceRate: ?float} - * @psalm-type StatsPerson = array{userId: string, displayName: string, isGuest: bool, sections: list, targetCount: int, yes: int, no: int, maybe: int, noResponse: int, present: int, absent: int, notRecorded: int, attendanceBase: int, noShow: int, responseRate: ?float, acceptRate: ?float, attendanceRate: ?float} - * @psalm-type StatsSection = array{id: string, displayName: string, personCount: int, targetCount: int, yes: int, no: int, maybe: int, noResponse: int, present: int, absent: int, notRecorded: int, attendanceBase: int, noShow: int, responseRate: ?float, acceptRate: ?float, attendanceRate: ?float} + * @psalm-type StatsCounts = array{targetCount: int, yes: int, no: int, maybe: int, noResponse: int, present: int, absent: int, notRecorded: int, attendanceBase: int, noShow: int, scheduled: int, notScheduled: int, schedulingBase: int, responseRate: ?float, acceptRate: ?float, attendanceRate: ?float, scheduledRate: ?float} + * @psalm-type StatsPerson = array{userId: string, displayName: string, isGuest: bool, sections: list, targetCount: int, yes: int, no: int, maybe: int, noResponse: int, present: int, absent: int, notRecorded: int, attendanceBase: int, noShow: int, scheduled: int, notScheduled: int, schedulingBase: int, responseRate: ?float, acceptRate: ?float, attendanceRate: ?float, scheduledRate: ?float} + * @psalm-type StatsSection = array{id: string, displayName: string, personCount: int, targetCount: int, yes: int, no: int, maybe: int, noResponse: int, present: int, absent: int, notRecorded: int, attendanceBase: int, noShow: int, scheduled: int, notScheduled: int, schedulingBase: int, responseRate: ?float, acceptRate: ?float, attendanceRate: ?float, scheduledRate: ?float} * @psalm-type StatsTimelinePoint = array{appointmentId: int, name: string, startDatetime: ?string, targetCount: int, yes: int, present: int, attendanceRecorded: bool} * @psalm-type StatsCategory = array{categoryId: ?int, displayName: string, appointmentCount: int, targetCount: int, yes: int, present: int, attendanceBase: int, acceptRate: ?float, attendanceRate: ?float} * @psalm-type StatsPersonDetail = array{userId: string, displayName: string, isGuest: bool, entries: list} @@ -67,7 +70,7 @@ public function __construct( * @param ?string $limitToUserId When set, only this person's row is * returned and the chart series are left out — * the shape for users without the permission. - * @return array{appointmentCount: int, pastCount: int, attendanceRecordedCount: int, groupBy: string, people: list, sections: list, totals: StatsCounts, timeline: list, byCategory: list} + * @return array{appointmentCount: int, pastCount: int, attendanceRecordedCount: int, groupBy: string, schedulingEnabled: bool, people: list, sections: list, totals: StatsCounts, timeline: list, byCategory: list} * @throws StatisticsRangeException */ public function getStatistics(StatisticsFilter $filter, ?string $limitToUserId = null): array { @@ -108,6 +111,10 @@ public function getStatistics(StatisticsFilter $filter, ?string $limitToUserId = 'pastCount' => $evaluation['pastCount'], 'attendanceRecordedCount' => $evaluation['attendanceRecordedCount'], 'groupBy' => $filter->groupBy, + // Whether the scheduling counters mean anything at all. Travels with + // the numbers so every consumer — table, export, mobile — reads the + // rule from the one place that applied it. + 'schedulingEnabled' => $this->configService->isBookingEnabled(), 'people' => $people, 'sections' => $sections, 'totals' => $totals->toArray(), @@ -198,12 +205,16 @@ private function evaluate(array $appointments): array { $byCategory = []; $pastCount = 0; $attendanceRecordedCount = 0; + $bookingEnabled = $this->configService->isBookingEnabled(); foreach ($appointments as $appointment) { $appointmentId = $appointment->getId(); $targets = $this->targetUserIds($appointment); $isPast = $this->hasEnded($appointment); $countsForAttendance = $isPast && isset($checkedIn[$appointmentId]); + $countsForScheduling = $bookingEnabled + && $appointment->isClosed() + && $this->anyoneScheduled($responses[$appointmentId] ?? []); if ($isPast) { $pastCount++; @@ -217,10 +228,12 @@ private function evaluate(array $appointments): array { $answer = $responses[$appointmentId][$targetUserId] ?? null; $response = $answer !== null ? $this->responseValue($answer['response']) : null; $checkin = $answer !== null ? $this->checkinState($answer['checkinState']) : null; + $isScheduled = $countsForScheduling + && ($answer['bookingStatus'] ?? null) === BookingService::STATUS_BOOKED; $tallies[$targetUserId] ??= new StatisticsTally(); - $tallies[$targetUserId]->record($response, $checkin, $countsForAttendance); - $perAppointment->record($response, $checkin, $countsForAttendance); + $tallies[$targetUserId]->record($response, $checkin, $countsForAttendance, $countsForScheduling, $isScheduled); + $perAppointment->record($response, $checkin, $countsForAttendance, $countsForScheduling, $isScheduled); } $categoryKey = $appointment->getCategoryId() ?? 0; @@ -268,7 +281,7 @@ private function idsOf(array $appointments): array { /** * @param list $appointmentIds - * @return array> appointmentId → userId → answer + * @return array> appointmentId → userId → answer */ private function indexResponses(array $appointmentIds, ?string $userId = null, bool $withComments = false): array { $indexed = []; @@ -276,6 +289,7 @@ private function indexResponses(array $appointmentIds, ?string $userId = null, b $answer = [ 'response' => $row['response'], 'checkinState' => $row['checkinState'], + 'bookingStatus' => $row['bookingStatus'], ]; if ($withComments) { $answer['comment'] = $row['comment'] ?? null; @@ -286,6 +300,23 @@ private function indexResponses(array $appointmentIds, ?string $userId = null, b return $indexed; } + /** + * Whether anybody got a place in this appointment. An inquiry the manager + * closed without scheduling anyone is one where the feature was not used — + * counting everybody there as "not scheduled" would measure the manager, + * not the person. Mirrors the guard in BookingService::isScheduledOut(). + * + * @param array $answers + */ + private function anyoneScheduled(array $answers): bool { + foreach ($answers as $answer) { + if (($answer['bookingStatus'] ?? null) === BookingService::STATUS_BOOKED) { + return true; + } + } + return false; + } + /** * @param list $appointmentIds * @return array appointment IDs whose check-in list was worked diff --git a/lib/Service/StatisticsTally.php b/lib/Service/StatisticsTally.php index 99656519..0a9ae8f3 100644 --- a/lib/Service/StatisticsTally.php +++ b/lib/Service/StatisticsTally.php @@ -20,13 +20,24 @@ final class StatisticsTally { public int $notRecorded = 0; public int $attendanceBase = 0; public int $noShow = 0; + public int $scheduled = 0; + public int $notScheduled = 0; + public int $schedulingBase = 0; /** * @param ?string $answer yes/no/maybe, or null when unanswered * @param ?string $checkin yes/no, or null when not recorded * @param bool $countsForAttendance Whether the appointment is over and had check-ins at all + * @param bool $countsForScheduling Whether the inquiry is closed and somebody got a place + * @param bool $isScheduled Whether this person got one */ - public function record(?string $answer, ?string $checkin, bool $countsForAttendance): void { + public function record( + ?string $answer, + ?string $checkin, + bool $countsForAttendance, + bool $countsForScheduling, + bool $isScheduled, + ): void { $this->targetCount++; match ($answer) { @@ -36,6 +47,13 @@ public function record(?string $answer, ?string $checkin, bool $countsForAttenda default => $this->noResponse++, }; + // Only a yes can be scheduled, and only a closed inquiry that gave + // somebody a place has decided anything — see BookingService. + if ($countsForScheduling && $answer === 'yes') { + $this->schedulingBase++; + $isScheduled ? $this->scheduled++ : $this->notScheduled++; + } + if (!$countsForAttendance) { return; } @@ -65,10 +83,13 @@ public function add(self $other): void { $this->notRecorded += $other->notRecorded; $this->attendanceBase += $other->attendanceBase; $this->noShow += $other->noShow; + $this->scheduled += $other->scheduled; + $this->notScheduled += $other->notScheduled; + $this->schedulingBase += $other->schedulingBase; } /** - * @return array{targetCount: int, yes: int, no: int, maybe: int, noResponse: int, present: int, absent: int, notRecorded: int, attendanceBase: int, noShow: int, responseRate: ?float, acceptRate: ?float, attendanceRate: ?float} + * @return array{targetCount: int, yes: int, no: int, maybe: int, noResponse: int, present: int, absent: int, notRecorded: int, attendanceBase: int, noShow: int, scheduled: int, notScheduled: int, schedulingBase: int, responseRate: ?float, acceptRate: ?float, attendanceRate: ?float, scheduledRate: ?float} */ public function toArray(): array { return [ @@ -82,9 +103,13 @@ public function toArray(): array { 'notRecorded' => $this->notRecorded, 'attendanceBase' => $this->attendanceBase, 'noShow' => $this->noShow, + 'scheduled' => $this->scheduled, + 'notScheduled' => $this->notScheduled, + 'schedulingBase' => $this->schedulingBase, 'responseRate' => self::rate($this->yes + $this->no + $this->maybe, $this->targetCount), 'acceptRate' => self::rate($this->yes, $this->targetCount), 'attendanceRate' => self::rate($this->present, $this->attendanceBase), + 'scheduledRate' => self::rate($this->scheduled, $this->schedulingBase), ]; } diff --git a/openapi-full.json b/openapi-full.json index 9b3c592a..bc6382a1 100644 --- a/openapi-full.json +++ b/openapi-full.json @@ -1392,6 +1392,7 @@ "pastCount", "attendanceRecordedCount", "groupBy", + "schedulingEnabled", "people", "sections", "totals", @@ -1414,6 +1415,9 @@ "groupBy": { "type": "string" }, + "schedulingEnabled": { + "type": "boolean" + }, "people": { "type": "array", "items": { @@ -1514,9 +1518,13 @@ "notRecorded", "attendanceBase", "noShow", + "scheduled", + "notScheduled", + "schedulingBase", "responseRate", "acceptRate", - "attendanceRate" + "attendanceRate", + "scheduledRate" ], "properties": { "userId": { @@ -1574,6 +1582,18 @@ "type": "integer", "format": "int64" }, + "scheduled": { + "type": "integer", + "format": "int64" + }, + "notScheduled": { + "type": "integer", + "format": "int64" + }, + "schedulingBase": { + "type": "integer", + "format": "int64" + }, "responseRate": { "type": "number", "format": "double", @@ -1588,6 +1608,11 @@ "type": "number", "format": "double", "nullable": true + }, + "scheduledRate": { + "type": "number", + "format": "double", + "nullable": true } } }, @@ -1673,9 +1698,13 @@ "notRecorded", "attendanceBase", "noShow", + "scheduled", + "notScheduled", + "schedulingBase", "responseRate", "acceptRate", - "attendanceRate" + "attendanceRate", + "scheduledRate" ], "properties": { "id": { @@ -1728,6 +1757,18 @@ "type": "integer", "format": "int64" }, + "scheduled": { + "type": "integer", + "format": "int64" + }, + "notScheduled": { + "type": "integer", + "format": "int64" + }, + "schedulingBase": { + "type": "integer", + "format": "int64" + }, "responseRate": { "type": "number", "format": "double", @@ -1742,6 +1783,11 @@ "type": "number", "format": "double", "nullable": true + }, + "scheduledRate": { + "type": "number", + "format": "double", + "nullable": true } } }, @@ -1798,9 +1844,13 @@ "notRecorded", "attendanceBase", "noShow", + "scheduled", + "notScheduled", + "schedulingBase", "responseRate", "acceptRate", - "attendanceRate" + "attendanceRate", + "scheduledRate" ], "properties": { "targetCount": { @@ -1843,6 +1893,18 @@ "type": "integer", "format": "int64" }, + "scheduled": { + "type": "integer", + "format": "int64" + }, + "notScheduled": { + "type": "integer", + "format": "int64" + }, + "schedulingBase": { + "type": "integer", + "format": "int64" + }, "responseRate": { "type": "number", "format": "double", @@ -1857,6 +1919,11 @@ "type": "number", "format": "double", "nullable": true + }, + "scheduledRate": { + "type": "number", + "format": "double", + "nullable": true } } }, diff --git a/openapi.json b/openapi.json index 49b6afa0..e9b88c13 100644 --- a/openapi.json +++ b/openapi.json @@ -1158,6 +1158,7 @@ "pastCount", "attendanceRecordedCount", "groupBy", + "schedulingEnabled", "people", "sections", "totals", @@ -1180,6 +1181,9 @@ "groupBy": { "type": "string" }, + "schedulingEnabled": { + "type": "boolean" + }, "people": { "type": "array", "items": { @@ -1280,9 +1284,13 @@ "notRecorded", "attendanceBase", "noShow", + "scheduled", + "notScheduled", + "schedulingBase", "responseRate", "acceptRate", - "attendanceRate" + "attendanceRate", + "scheduledRate" ], "properties": { "userId": { @@ -1340,6 +1348,18 @@ "type": "integer", "format": "int64" }, + "scheduled": { + "type": "integer", + "format": "int64" + }, + "notScheduled": { + "type": "integer", + "format": "int64" + }, + "schedulingBase": { + "type": "integer", + "format": "int64" + }, "responseRate": { "type": "number", "format": "double", @@ -1354,6 +1374,11 @@ "type": "number", "format": "double", "nullable": true + }, + "scheduledRate": { + "type": "number", + "format": "double", + "nullable": true } } }, @@ -1439,9 +1464,13 @@ "notRecorded", "attendanceBase", "noShow", + "scheduled", + "notScheduled", + "schedulingBase", "responseRate", "acceptRate", - "attendanceRate" + "attendanceRate", + "scheduledRate" ], "properties": { "id": { @@ -1494,6 +1523,18 @@ "type": "integer", "format": "int64" }, + "scheduled": { + "type": "integer", + "format": "int64" + }, + "notScheduled": { + "type": "integer", + "format": "int64" + }, + "schedulingBase": { + "type": "integer", + "format": "int64" + }, "responseRate": { "type": "number", "format": "double", @@ -1508,6 +1549,11 @@ "type": "number", "format": "double", "nullable": true + }, + "scheduledRate": { + "type": "number", + "format": "double", + "nullable": true } } }, @@ -1564,9 +1610,13 @@ "notRecorded", "attendanceBase", "noShow", + "scheduled", + "notScheduled", + "schedulingBase", "responseRate", "acceptRate", - "attendanceRate" + "attendanceRate", + "scheduledRate" ], "properties": { "targetCount": { @@ -1609,6 +1659,18 @@ "type": "integer", "format": "int64" }, + "scheduled": { + "type": "integer", + "format": "int64" + }, + "notScheduled": { + "type": "integer", + "format": "int64" + }, + "schedulingBase": { + "type": "integer", + "format": "int64" + }, "responseRate": { "type": "number", "format": "double", @@ -1623,6 +1685,11 @@ "type": "number", "format": "double", "nullable": true + }, + "scheduledRate": { + "type": "number", + "format": "double", + "nullable": true } } }, diff --git a/tests/unit/Service/StatisticsServiceTest.php b/tests/unit/Service/StatisticsServiceTest.php index eb1b481f..509cfba5 100644 --- a/tests/unit/Service/StatisticsServiceTest.php +++ b/tests/unit/Service/StatisticsServiceTest.php @@ -245,6 +245,119 @@ public function testReducedShapeIsTheOwnRowAndNothingElse(): void { $this->assertSame([], $result['byCategory']); } + /** + * The scheduled rate counts a yes only once the inquiry is closed and + * somebody got a place. Alice is scheduled on the closed one, Bob is not; + * the open inquiry and the closed one nobody was scheduled for decide + * nothing and must stay out of the basis entirely. + */ + public function testScheduledRateOnlyCountsDecidedInquiries(): void { + $this->givenUsers(['alice' => 'Alice', 'bob' => 'Bob']); + $this->givenUnrestrictedVisibility(); + $this->givenGroupSections(); + $this->configService->method('isBookingEnabled')->willReturn(true); + + $decided = $this->appointment(1, '2026-05-01 18:00:00', '2026-05-01 20:00:00'); + $decided->setClosedAt('2026-04-25 10:00:00'); + $open = $this->appointment(2, '2026-06-01 18:00:00', '2026-06-01 20:00:00'); + $unused = $this->appointment(3, '2026-07-01 18:00:00', '2026-07-01 20:00:00'); + $unused->setClosedAt('2026-06-25 10:00:00'); + + $this->appointmentMapper->method('findForStatistics')->willReturn([$decided, $open, $unused]); + $this->givenResponses([ + $this->response(1, 'alice', 'yes', '', null, 'booked'), + $this->response(1, 'bob', 'yes', '', null, null), + $this->response(2, 'alice', 'yes', '', null, null), + $this->response(2, 'bob', 'yes', '', null, null), + $this->response(3, 'alice', 'yes', '', null, null), + $this->response(3, 'bob', 'yes', '', null, null), + ]); + + $result = $this->service->getStatistics($this->filter()); + $people = array_column($result['people'], null, 'userId'); + + $this->assertSame(1, $people['alice']['schedulingBase'], 'only the decided inquiry counts'); + $this->assertSame(1, $people['alice']['scheduled']); + $this->assertSame(0, $people['alice']['notScheduled']); + $this->assertSame(1.0, $people['alice']['scheduledRate']); + + $this->assertSame(1, $people['bob']['schedulingBase']); + $this->assertSame(0, $people['bob']['scheduled']); + $this->assertSame(1, $people['bob']['notScheduled']); + $this->assertSame(0.0, $people['bob']['scheduledRate']); + } + + /** + * Being told "you are not scheduled" is recorded in bookingNotifiedStatus, + * not in bookingStatus — the mapper has to fold the two like + * BookingService::effectiveBookingStatus() does. + */ + public function testDecliningAnswersStillCountTowardsTheBasis(): void { + $this->givenUsers(['alice' => 'Alice', 'bob' => 'Bob']); + $this->givenUnrestrictedVisibility(); + $this->givenGroupSections(); + $this->configService->method('isBookingEnabled')->willReturn(true); + + $closed = $this->appointment(1, '2026-05-01 18:00:00', '2026-05-01 20:00:00'); + $closed->setClosedAt('2026-04-25 10:00:00'); + $this->appointmentMapper->method('findForStatistics')->willReturn([$closed]); + $this->givenResponses([ + $this->response(1, 'alice', 'yes', '', null, 'booked'), + $this->response(1, 'bob', 'yes', '', null, 'declined'), + ]); + + $result = $this->service->getStatistics($this->filter()); + $people = array_column($result['people'], null, 'userId'); + + $this->assertSame(1, $people['bob']['schedulingBase']); + $this->assertSame(1, $people['bob']['notScheduled']); + } + + public function testNothingIsScheduledWhenPlanningIsOff(): void { + $this->givenUsers(['alice' => 'Alice']); + $this->givenUnrestrictedVisibility(); + $this->givenGroupSections(); + $this->configService->method('isBookingEnabled')->willReturn(false); + + $closed = $this->appointment(1, '2026-05-01 18:00:00', '2026-05-01 20:00:00'); + $closed->setClosedAt('2026-04-25 10:00:00'); + $this->appointmentMapper->method('findForStatistics')->willReturn([$closed]); + $this->givenResponses([ + $this->response(1, 'alice', 'yes', '', null, 'booked'), + ]); + + $result = $this->service->getStatistics($this->filter()); + + $this->assertSame(0, $result['people'][0]['schedulingBase']); + $this->assertNull($result['people'][0]['scheduledRate'], 'no basis, no rate'); + } + + /** + * Only a yes can be scheduled, so a no or an unanswered row must not widen + * the basis — otherwise the rate would punish people for declining. + */ + public function testOnlyAcceptancesFormTheSchedulingBasis(): void { + $this->givenUsers(['alice' => 'Alice', 'bob' => 'Bob']); + $this->givenUnrestrictedVisibility(); + $this->givenGroupSections(); + $this->configService->method('isBookingEnabled')->willReturn(true); + + $closed = $this->appointment(1, '2026-05-01 18:00:00', '2026-05-01 20:00:00'); + $closed->setClosedAt('2026-04-25 10:00:00'); + $this->appointmentMapper->method('findForStatistics')->willReturn([$closed]); + $this->givenResponses([ + $this->response(1, 'alice', 'yes', '', null, 'booked'), + $this->response(1, 'bob', 'no', '', null, null), + ]); + + $result = $this->service->getStatistics($this->filter()); + $people = array_column($result['people'], null, 'userId'); + + $this->assertSame(0, $people['bob']['schedulingBase']); + $this->assertNull($people['bob']['scheduledRate']); + $this->assertSame(1, $result['totals']['schedulingBase'], 'the totals row agrees'); + } + public function testRefusesRangesBeyondTheAppointmentLimit(): void { $appointments = []; for ($id = 1; $id <= StatisticsService::MAX_APPOINTMENTS + 1; $id++) { @@ -427,14 +540,22 @@ private function appointment(int $id, string $start, string $end): Appointment { } /** - * @return array{appointmentId: int, userId: string, response: ?string, checkinState: ?string} + * @return array{appointmentId: int, userId: string, response: ?string, checkinState: ?string, bookingStatus: ?string} */ - private function response(int $appointmentId, string $userId, string $answer, string $checkinState, ?string $comment = null): array { + private function response( + int $appointmentId, + string $userId, + string $answer, + string $checkinState, + ?string $comment = null, + ?string $bookingStatus = null, + ): array { return [ 'appointmentId' => $appointmentId, 'userId' => $userId, 'response' => $answer, 'checkinState' => $checkinState === '' ? null : $checkinState, + 'bookingStatus' => $bookingStatus, 'comment' => $comment, ]; } @@ -443,7 +564,7 @@ private function response(int $appointmentId, string $userId, string $answer, st * Stubs both response queries from one row set, so a test never has to keep * the rows and the "was anyone checked in" flags in step by hand. * - * @param list $rows + * @param list $rows */ private function givenResponses(array $rows): void { $this->responseMapper->method('findStatisticsRows')->willReturnCallback( From 45e46d3f4050f8c4d44d311a58ab41ba3a4f0c52 Mon Sep 17 00:00:00 2001 From: Florian Ludwig Date: Sat, 15 Aug 2026 21:05:05 +0200 Subject: [PATCH 04/11] feat(statistics): make the table fit the screen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fully expanded the table shows fifteen columns of nowrap numbers and needs roughly 1800px, while the page was capped at the 1200px every other view uses. The overflow was therefore guaranteed on every screen, and the scroll container's bar sits below the last row — out of sight on any list long enough to be worth reading. Reported as "no horizontal scrollbar in Firefox"; it was neither Firefox nor the reporter's install. Three changes: - The statistics page gets the width it actually needs. It is the one view in the app with a real case for it. - Columns can be ticked individually, including the groups column, which is the widest one and the one that prompted the report. Compact/Full stay as the two presets; ticking refines whichever was picked last. - The name column stays put while scrolling right, so the numbers under the cursor still belong to somebody. Column descriptors move into src/utils/statisticsColumns.js because the table and the picker have to agree on what exists and what it is called — including the scheduling columns from the previous commit, which appear only where the evaluation says the planning mode is on. Row highlighting now travels through a custom property. The sticky name cell paints its own background, which would otherwise out-specify the row states and leave the frozen cell of every highlighted row unpainted. --- src/components/statistics/StatisticsTable.vue | 96 ++++++++--------- src/utils/statisticsColumns.js | 102 ++++++++++++++++++ src/views/StatisticsOverview.vue | 81 +++++++++++++- 3 files changed, 227 insertions(+), 52 deletions(-) create mode 100644 src/utils/statisticsColumns.js diff --git a/src/components/statistics/StatisticsTable.vue b/src/components/statistics/StatisticsTable.vue index 2054be36..8a4b3e18 100644 --- a/src/components/statistics/StatisticsTable.vue +++ b/src/components/statistics/StatisticsTable.vue @@ -15,7 +15,7 @@ :size="16" /> - + {{ sectionColumnLabel }} @@ -41,7 +41,7 @@ {{ t("attendance", "Your numbers") }} - + {{ sectionsOf(ownPerson) }} @@ -88,7 +88,7 @@ {{ t("attendance", "Guest") }} - + {{ sectionsOf(person) }} @@ -103,7 +103,7 @@ {{ t("attendance", "Total") }} - + {{ cell(totals, column) }} @@ -118,6 +118,7 @@ import { translatePlural as n, translate as t } from '@nextcloud/l10n' import { computed, ref } from 'vue' import MenuDownIcon from 'vue-material-design-icons/MenuDown.vue' import MenuUpIcon from 'vue-material-design-icons/MenuUp.vue' +import { SECTIONS_COLUMN, sectionsColumnLabel, STATISTICS_COLUMNS } from '../../utils/statisticsColumns.js' const props = defineProps({ sections: { type: Array, required: true }, @@ -126,11 +127,7 @@ const props = defineProps({ reduced: { type: Boolean, default: false }, selectable: { type: Boolean, default: false }, grouped: { type: Boolean, required: true }, - detail: { - type: String, - default: 'compact', - validator: (value) => ['compact', 'full'].includes(value), - }, + visibleColumns: { type: Array, required: true }, groupBy: { type: String, default: 'groups' }, ownUserId: { type: String, default: '' }, search: { type: String, default: '' }, @@ -138,36 +135,10 @@ const props = defineProps({ const emit = defineEmits(['selectPerson']) -// `compact` marks the columns that answer the question at a glance: who was -// asked, what they said, whether they came. -const COLUMNS = [ - { key: 'targetCount', label: t('attendance', 'Appointments') }, - { key: 'yes', label: t('attendance', 'Yes'), compact: true }, - { key: 'no', label: t('attendance', 'No'), compact: true }, - { key: 'maybe', label: t('attendance', 'Maybe'), compact: true }, - { key: 'noResponse', label: t('attendance', 'No response') }, - { key: 'present', label: t('attendance', 'Present'), compact: true }, - { key: 'absent', label: t('attendance', 'Absent') }, - // TRANSLATORS: Column header — for how many appointments nobody wrote down whether this person was there. Not the same as being absent. - { key: 'notRecorded', label: t('attendance', 'Not recorded') }, - { - key: 'noShow', - // TRANSLATORS: Column header — the person said yes and then was not there. English uses the noun "no-show"; other languages usually need a short phrase. - label: t('attendance', 'No-show'), - hint: t('attendance', 'Said yes but was recorded as absent'), - }, - // TRANSLATORS: Column header — share of appointments the person answered at all, whatever the answer was. - { key: 'responseRate', label: t('attendance', 'Response rate'), rate: true, compact: true }, - // TRANSLATORS: Column header — share of appointments the person answered with yes. Sits next to "Response rate" and "Attendance rate", which count different things. - { key: 'acceptRate', label: t('attendance', 'Acceptance rate'), rate: true, compact: true }, - // TRANSLATORS: Column header — share of appointments the person was actually there for, counted only over appointments where somebody worked the check-in list. - { key: 'attendanceRate', label: t('attendance', 'Attendance rate'), rate: true, compact: true }, -] - const sortKey = ref('displayName') const sortAsc = ref(true) -const columns = computed(() => (props.detail === 'full' ? COLUMNS : COLUMNS.filter((column) => column.compact))) +const columns = computed(() => STATISTICS_COLUMNS.filter((column) => props.visibleColumns.includes(column.key))) // A column the compact view drops takes its sort with it, arrow and all. Derived // rather than reset, so no render can fall between the two. @@ -181,11 +152,13 @@ const sortIcon = computed(() => (activeSort.value.asc ? MenuUpIcon : MenuDownIco // Ungrouped, the membership a section heading would have carried becomes a // column of its own — it is the one thing a flat list would otherwise lose. -const columnCount = computed(() => columns.value.length + (props.grouped ? 1 : 2)) +// Grouped, the headings already say it, so the column goes whatever is ticked. +const showSections = computed(() => !props.grouped && props.visibleColumns.includes(SECTIONS_COLUMN)) + +// The name column is always there; the sections column joins it when shown. +const columnCount = computed(() => columns.value.length + 1 + (showSections.value ? 1 : 0)) -const sectionColumnLabel = computed(() => (props.groupBy === 'teams' - ? t('attendance', 'Teams') - : t('attendance', 'Groups'))) +const sectionColumnLabel = computed(() => sectionsColumnLabel(props.groupBy)) const sectionNames = computed(() => { return Object.fromEntries(props.sections.map((section) => [section.id, section.displayName])) @@ -288,6 +261,24 @@ function cell(row, column) { white-space: nowrap; } +/* The name has to survive scrolling right — without it the numbers under the + cursor belong to nobody. It paints its own background, so it takes whatever + colour the row is carrying rather than a hardcoded one. Paired with the + sticky header below, the top-left corner needs the higher stacking order or + the two overlap. */ +.statistics-table th[scope="row"], +.statistics-table thead th:first-child, +.statistics-table tfoot th { + background-color: var(--row-background, var(--color-main-background)); + inset-inline-start: 0; + position: sticky; + z-index: 1; +} + +.statistics-table thead th:first-child { + z-index: 3; +} + /* Compounded with the element: the generic `th`/`td` rule above is a class plus an element, so a bare class would lose to it. */ .statistics-table th[scope="row"], @@ -315,7 +306,7 @@ function cell(row, column) { position: sticky; top: 0; background-color: var(--color-main-background); - z-index: 1; + z-index: 2; } .statistics-table__sort { @@ -333,8 +324,15 @@ function cell(row, column) { padding: 0; } -.statistics-table__section > * { - background-color: var(--color-background-hover); +/* Row states set a custom property rather than a background: it inherits into + every cell including the sticky one, so no rule has to out-specify another. */ +.statistics-table tbody td, +.statistics-table tbody th { + background-color: var(--row-background, transparent); +} + +.statistics-table__section { + --row-background: var(--color-background-hover); font-weight: bold; } @@ -345,14 +343,14 @@ function cell(row, column) { cursor: pointer; } -.statistics-table__person:hover > * { - background-color: var(--color-background-hover); +.statistics-table__person:hover { + --row-background: var(--color-background-hover); } -.statistics-table__own > *, -.statistics-table__person--self > *, -.statistics-table__person--self:hover > * { - background-color: var(--color-primary-element-light); +.statistics-table__own, +.statistics-table__person--self, +.statistics-table__person--self:hover { + --row-background: var(--color-primary-element-light); font-weight: bold; } diff --git a/src/utils/statisticsColumns.js b/src/utils/statisticsColumns.js new file mode 100644 index 00000000..bece65c2 --- /dev/null +++ b/src/utils/statisticsColumns.js @@ -0,0 +1,102 @@ +import { translate as t } from '@nextcloud/l10n' + +/** + * The membership a section heading carries when the table is grouped. Ungrouped + * it has to become a column, which is the one thing a flat list would otherwise + * lose — and the widest column on the page, so it is worth being able to drop. + */ +export const SECTIONS_COLUMN = 'sections' + +/** + * Every data column the table can show, in display order. + * + * `compact` marks the ones that answer the question at a glance: who was asked, + * what they said, whether they came. `scheduling` marks the ones that only mean + * anything while the planning mode is switched on. + */ +export const STATISTICS_COLUMNS = [ + { key: 'targetCount', label: t('attendance', 'Appointments') }, + { key: 'yes', label: t('attendance', 'Yes'), compact: true }, + { key: 'no', label: t('attendance', 'No'), compact: true }, + { key: 'maybe', label: t('attendance', 'Maybe'), compact: true }, + { key: 'noResponse', label: t('attendance', 'No response') }, + { key: 'present', label: t('attendance', 'Present'), compact: true }, + { key: 'absent', label: t('attendance', 'Absent') }, + // TRANSLATORS: Column header — for how many appointments nobody wrote down whether this person was there. Not the same as being absent. + { key: 'notRecorded', label: t('attendance', 'Not recorded') }, + { + key: 'noShow', + // TRANSLATORS: Column header — the person said yes and then was not there. English uses the noun "no-show"; other languages usually need a short phrase. + label: t('attendance', 'No-show'), + hint: t('attendance', 'Said yes but was recorded as absent'), + }, + { + key: 'scheduled', + // TRANSLATORS: Column header — how often the person accepted and then got a place in the appointment. + label: t('attendance', 'Scheduled'), + compact: true, + scheduling: true, + }, + { + key: 'notScheduled', + // TRANSLATORS: Column header — the person accepted and the inquiry was closed, but they did not get a place. + label: t('attendance', 'Not scheduled'), + scheduling: true, + }, + // TRANSLATORS: Column header — share of appointments the person answered at all, whatever the answer was. + { key: 'responseRate', label: t('attendance', 'Response rate'), rate: true, compact: true }, + // TRANSLATORS: Column header — share of appointments the person answered with yes. Sits next to "Response rate" and "Attendance rate", which count different things. + { key: 'acceptRate', label: t('attendance', 'Acceptance rate'), rate: true, compact: true }, + // TRANSLATORS: Column header — share of appointments the person was actually there for, counted only over appointments where somebody worked the check-in list. + { key: 'attendanceRate', label: t('attendance', 'Attendance rate'), rate: true, compact: true }, + { + key: 'scheduledRate', + // TRANSLATORS: Column header — share of the person's acceptances that got a place. Sits next to "Acceptance rate", which counts something else. + label: t('attendance', 'Scheduling rate'), + hint: t('attendance', 'Counted only over closed inquiries where somebody was scheduled'), + rate: true, + compact: true, + scheduling: true, + }, +] + +/** + * What the sections column is called, which depends on what the evaluation was + * grouped by. Lives here because the table header and the column picker both + * have to say the same word. + * + * @param {string} groupBy - 'groups' or 'teams'. + * @return {string} The column label. + */ +export function sectionsColumnLabel(groupBy) { + return groupBy === 'teams' ? t('attendance', 'Teams') : t('attendance', 'Groups') +} + +/** + * Every column that makes sense on this instance, sections column first. + * + * @param {boolean} scheduling - Whether the planning mode is switched on. + * @param {string} groupBy - 'groups' or 'teams'. + * @return {Array} Column descriptors. + */ +export function availableColumns(scheduling, groupBy) { + return [ + { key: SECTIONS_COLUMN, label: sectionsColumnLabel(groupBy), compact: true }, + ...STATISTICS_COLUMNS.filter((column) => scheduling || !column.scheduling), + ] +} + +/** + * The two presets behind the compact/full switch. Both include the sections + * column — the table drops it on its own while grouped, where the section + * headings already carry it. + * + * @param {'compact'|'full'} detail - Which preset. + * @param {boolean} scheduling - Whether the planning mode is switched on. + * @return {Array} Column keys. + */ +export function presetColumns(detail, scheduling) { + return availableColumns(scheduling, 'groups') + .filter((column) => detail === 'full' || column.compact) + .map((column) => column.key) +} diff --git a/src/views/StatisticsOverview.vue b/src/views/StatisticsOverview.vue index 091b8810..ce8d2dcb 100644 --- a/src/views/StatisticsOverview.vue +++ b/src/views/StatisticsOverview.vue @@ -211,6 +211,37 @@ + +
+ + + + + + +
@@ -221,7 +252,7 @@ :reduced="reduced" :selectable="canDrillDown" :grouped="grouped" - :detail="detail" + :visibleColumns="visibleColumns" :groupBy="statistics.groupBy" :ownUserId="ownUserId" :search="search" @@ -251,6 +282,7 @@ import { computed, defineAsyncComponent, ref, watch } from 'vue' import ChartLineIcon from 'vue-material-design-icons/ChartLine.vue' import CheckIcon from 'vue-material-design-icons/Check.vue' import DownloadIcon from 'vue-material-design-icons/Download.vue' +import TableColumnIcon from 'vue-material-design-icons/TableColumn.vue' import TagIcon from 'vue-material-design-icons/TagOutline.vue' import LoadingState from '../components/common/LoadingState.vue' import StatisticsPersonSidebar from '../components/statistics/StatisticsPersonSidebar.vue' @@ -259,6 +291,7 @@ import { useCategories } from '../composables/useCategories.js' import { useCategoryFilterChips } from '../composables/useCategoryFilterChips.js' import { usePermissions } from '../composables/usePermissions.js' import { categoryIconComponent } from '../utils/categoryIcons.js' +import { availableColumns, presetColumns } from '../utils/statisticsColumns.js' import { defaultPeriod, periodFromKey, periodOptions, periodTypeForKey } from '../utils/statisticsPeriods.js' // chart.js is only ever needed here — keep it out of the bundle every other @@ -291,7 +324,28 @@ const detail = ref(initial.detail) // Both switches only pick what is drawn from an answer the server already sent, // so they must stay out of `query` — flipping one is not worth an evaluation. const grouped = computed(() => grouping.value === 'grouped') -watch([grouping, detail], writeUrlState) + +// Whether the scheduling counters mean anything travels with the numbers, so +// the columns settle the moment the evaluation arrives — no second source that +// resolves on its own schedule. +const schedulingEnabled = computed(() => statistics.value?.schedulingEnabled === true) + +const columnChoices = computed(() => availableColumns(schedulingEnabled.value, groupBy.value)) + +// The user's own pick, or null while they have not made one. Everything else is +// derived, so the preset never has to be re-applied by a watcher. +const chosenColumns = ref(initial.columns) + +const visibleColumns = computed(() => chosenColumns.value + ?? presetColumns(detail.value, schedulingEnabled.value)) + +// Compact/Full stay the two presets people reach for; ticking individual +// columns refines whichever they picked last. +watch(detail, () => { + chosenColumns.value = null +}) + +watch([grouping, detail, visibleColumns], writeUrlState) const reduced = computed(() => !permissions.canSeeStatistics) @@ -376,6 +430,15 @@ async function exportStatistics() { } } +/** + * @param {string} key - Column to toggle. + */ +function toggleColumn(key) { + chosenColumns.value = visibleColumns.value.includes(key) + ? visibleColumns.value.filter((column) => column !== key) + : [...visibleColumns.value, key] +} + /** * @param {number} categoryId - Category to toggle. */ @@ -419,6 +482,11 @@ function readUrlState() { groupBy: params.get('groupBy') === 'teams' ? 'teams' : 'groups', grouping: params.get('grouping') === 'flat' ? 'flat' : 'grouped', detail: params.get('detail') === 'full' ? 'full' : 'compact', + // null, not [], so an explicitly emptied selection stays empty rather + // than falling back to the preset on every reload. + columns: params.has('columns') + ? params.get('columns').split(',').filter(Boolean) + : null, } } @@ -442,6 +510,9 @@ function writeUrlState() { // Below here: linkable, but not part of `query` — see the note on `grouped`. if (!grouped.value) params.set('grouping', 'flat') if (detail.value === 'full') params.set('detail', 'full') + // Only an explicit pick — the preset is already implied by `detail`, and + // spelling it out would put a dozen redundant keys on every visit. + if (chosenColumns.value !== null) params.set('columns', chosenColumns.value.join(',')) window.history.replaceState( window.history.state, @@ -452,8 +523,12 @@ function writeUrlState() { diff --git a/src/components/statistics/StatisticsTable.vue b/src/components/statistics/StatisticsTable.vue index 8a4b3e18..b938eee5 100644 --- a/src/components/statistics/StatisticsTable.vue +++ b/src/components/statistics/StatisticsTable.vue @@ -118,7 +118,7 @@ import { translatePlural as n, translate as t } from '@nextcloud/l10n' import { computed, ref } from 'vue' import MenuDownIcon from 'vue-material-design-icons/MenuDown.vue' import MenuUpIcon from 'vue-material-design-icons/MenuUp.vue' -import { SECTIONS_COLUMN, sectionsColumnLabel, STATISTICS_COLUMNS } from '../../utils/statisticsColumns.js' +import { formatRate, SECTIONS_COLUMN, sectionsColumnLabel, STATISTICS_COLUMNS } from '../../utils/statisticsColumns.js' const props = defineProps({ sections: { type: Array, required: true }, @@ -237,9 +237,7 @@ function cell(row, column) { if (!column.rate) { return String(value ?? 0) } - return value === null || value === undefined - ? '–' - : `${Math.round(value * 1000) / 10} %` + return formatRate(value) } diff --git a/src/utils/statisticsCards.js b/src/utils/statisticsCards.js new file mode 100644 index 00000000..e24e5358 --- /dev/null +++ b/src/utils/statisticsCards.js @@ -0,0 +1,224 @@ +import { translate as t } from '@nextcloud/l10n' + +/** + * At most this many rows per card. Rows are *values*, not people — everyone + * tied at a value shares one row, so a five-row card can name any number of + * people without inventing a rank order between equals. + */ +const MAX_ROWS = 5 + +/** + * A rate over very few appointments is not comparable with one over the whole + * period: somebody invited to their first appointment and present at it holds + * 100 % and would lead every card. Only people whose denominator reaches this + * share of the widest one in the period are considered. Relative rather than a + * fixed number, because a quarter with four appointments and a year with sixty + * have nothing in common. + */ +const MIN_BASIS_SHARE = 0.5 + +/** + * Score of somebody who was never recorded present. Sorts to the head of the + * ascending date card instead of dropping out of it, and Number.isFinite keeps + * it out of the mean for free. Named so the component can ask about it without + * spelling the sentinel a second time. + */ +const NEVER_PRESENT = -Infinity + +/** + * Every card, in display order. + * + * `direction` says which end is interesting and therefore which side of the + * average survives: `desc` keeps everyone at or above it, `asc` everyone at or + * below. The award cards are `desc`, so nobody is ever shown as the tail of a + * ranking; "longest not attended" is `asc`, where the tail is the whole point. + * + * `basis` marks a rate: those cards carry their denominator per name and drop + * anyone below MIN_BASIS_SHARE. Count cards need neither — a small denominator + * cannot win a count. + */ +export const STATISTICS_CARDS = [ + { + key: 'attendanceRate', + // TRANSLATORS: Card title. Lists the people who turned up most reliably. + label: t('attendance', 'Highest attendance rate'), + metric: 'attendanceRate', + basis: 'attendanceBase', + direction: 'desc', + default: true, + }, + { + key: 'acceptRate', + // TRANSLATORS: Card title. Lists the people who accepted most often. + label: t('attendance', 'Highest acceptance rate'), + metric: 'acceptRate', + basis: 'targetCount', + direction: 'desc', + default: true, + }, + { + key: 'maybe', + // TRANSLATORS: Card title. Lists the people who answered "maybe" most often — a light-hearted one, not a reproach. + label: t('attendance', 'Most maybe answers'), + metric: 'maybe', + direction: 'desc', + default: true, + }, + { + key: 'lastPresent', + // TRANSLATORS: Card title. Lists who has not been to an appointment for the longest, so nobody quietly drops off the radar. + label: t('attendance', 'Longest not attended'), + metric: 'lastPresentAt', + direction: 'asc', + date: true, + default: true, + }, + { + key: 'responseRate', + // TRANSLATORS: Card title. Lists the people who answered most reliably, whatever the answer was. + label: t('attendance', 'Highest response rate'), + metric: 'responseRate', + basis: 'targetCount', + direction: 'desc', + }, + { + key: 'scheduledRate', + // TRANSLATORS: Card title. Lists the people whose acceptances most often got them a place. + label: t('attendance', 'Highest scheduling rate'), + metric: 'scheduledRate', + basis: 'schedulingBase', + direction: 'desc', + scheduling: true, + }, + { + key: 'scheduled', + // TRANSLATORS: Card title. Lists who got a place in the most appointments. + label: t('attendance', 'Most times scheduled'), + metric: 'scheduled', + direction: 'desc', + scheduling: true, + }, +] + +/** + * @param {boolean} scheduling - Whether the planning mode is switched on. + * @return {Array} The cards that mean anything on this instance. + */ +export function availableCards(scheduling) { + return STATISTICS_CARDS.filter((card) => scheduling || !card.scheduling) +} + +/** + * @param {boolean} scheduling - Whether the planning mode is switched on. + * @return {Array} Keys of the cards shown until somebody says otherwise. + */ +export function defaultCards(scheduling) { + return availableCards(scheduling).filter((card) => card.default).map((card) => card.key) +} + +/** + * Build one card's rows. + * + * @param {object} card - A descriptor from STATISTICS_CARDS. + * @param {Array} people - The person rows of the evaluation. + * @param {object} totals - The totals row, used as the average for rates. + * @return {Array|null} Rows, or null when the card has nothing to say. + */ +export function cardRows(card, people, totals) { + const descending = card.direction === 'desc' + + // Scored once: the rest of this function only ever compares numbers, and on + // the date card each score is a Date.parse nobody should pay four times. + const scored = eligiblePeople(card, people).map((person) => ({ person, score: value(card, person) })) + if (scored.length === 0) { + return null + } + + const threshold = averageOf(card, scored, totals) + // A zero never earns a line on a counting card: with nobody saying maybe all + // period everyone sits at zero, clears the average and the card would crown + // the whole team for something that never happened. The date card is exempt + // — there "never attended" is precisely what deserves a line. + const keep = card.date + ? (score) => threshold === null || score <= threshold + : (score) => score > 0 && (threshold === null || (descending ? score >= threshold : score <= threshold)) + + /** @type {Map>} */ + const byValue = new Map() + for (const { person, score } of scored) { + if (!keep(score)) { + continue + } + const bucket = byValue.get(score) + if (bucket) { + bucket.push(person) + } else { + byValue.set(score, [person]) + } + } + + if (byValue.size === 0) { + return null + } + + return [...byValue.entries()] + .sort(([a], [b]) => (descending ? b - a : a - b)) + .slice(0, MAX_ROWS) + .map(([score, group]) => ({ value: score, never: score === NEVER_PRESENT, people: group })) +} + +/** + * @param {object} card - Card descriptor. + * @param {object} person - A person row. + * @return {number} The sortable value, with "never present" as -Infinity so it + * leads the ascending date card rather than dropping out of it. + */ +function value(card, person) { + if (!card.date) { + return person[card.metric] + } + const raw = person[card.metric] + return raw ? Date.parse(raw) : NEVER_PRESENT +} + +/** + * @param {object} card - Card descriptor. + * @param {Array} people - The person rows. + * @return {Array} Those the card may consider at all. + */ +function eligiblePeople(card, people) { + if (card.date) { + return people + } + + const withValue = people.filter((person) => person[card.metric] !== null && person[card.metric] !== undefined) + if (!card.basis) { + return withValue + } + + const widest = Math.max(0, ...withValue.map((person) => person[card.basis])) + return withValue.filter((person) => person[card.basis] >= widest * MIN_BASIS_SHARE) +} + +/** + * The line the card cuts at. Rates take the totals row, which is the weighted + * average the table already shows; counts and dates take the plain mean over + * the people the card considers. + * + * @param {object} card - Card descriptor. + * @param {Array} scored - The people under consideration, with scores. + * @param {object} totals - The totals row. + * @return {?number} The threshold, or null when there is nothing to cut at. + */ +function averageOf(card, scored, totals) { + if (card.basis && totals[card.metric] !== null && totals[card.metric] !== undefined) { + return totals[card.metric] + } + + const values = scored.map(({ score }) => score).filter(Number.isFinite) + if (values.length === 0) { + return null + } + + return values.reduce((sum, v) => sum + v, 0) / values.length +} diff --git a/src/utils/statisticsColumns.js b/src/utils/statisticsColumns.js index bece65c2..61a5fe01 100644 --- a/src/utils/statisticsColumns.js +++ b/src/utils/statisticsColumns.js @@ -100,3 +100,31 @@ export function presetColumns(detail, scheduling) { .filter((column) => detail === 'full' || column.compact) .map((column) => column.key) } + +/** + * A rate as a percentage, one decimal. Shared so the table, the charts and the + * highlight cards cannot drift apart on rounding or on the space before the + * sign — they render the same numbers side by side. + * + * @param {?number} rate - A rate between 0 and 1, or null without a basis. + * @param {string} fallback - Rendered instead when there is no rate. + * @return {string} The formatted percentage. + */ +export function formatRate(rate, fallback = '–') { + return rate === null || rate === undefined + ? fallback + : `${Math.round(rate * 1000) / 10} %` +} + +/** + * A selection with `value` flipped in or out. Written once because this view + * alone toggles cards, columns and categories, and copies of it drift — the + * card toggle shipped with the column toggle's doc comment. + * + * @param {Array} list - The current selection. + * @param {string|number} value - What to flip. + * @return {Array} A new selection. + */ +export function toggled(list, value) { + return list.includes(value) ? list.filter((entry) => entry !== value) : [...list, value] +} diff --git a/src/views/StatisticsOverview.vue b/src/views/StatisticsOverview.vue index 77b9ab45..a1f3ddd7 100644 --- a/src/views/StatisticsOverview.vue +++ b/src/views/StatisticsOverview.vue @@ -179,6 +179,45 @@ }}

+
+ +

{{ t("attendance", "Highlights") }}

+ + + + +
+ + + chosenColumns.value ?? presetColumns(detail.value, schedulingEnabled.value)) +const cardChoices = computed(() => availableCards(schedulingEnabled.value)) + +const chosenCards = ref(initial.cards) + +const visibleCards = computed(() => chosenCards.value ?? defaultCards(schedulingEnabled.value)) + // Compact/Full stay the two presets people reach for; ticking individual // columns refines whichever they picked last. watch(detail, () => { chosenColumns.value = null }) -watch([grouping, detail, visibleColumns], writeUrlState) +watch([grouping, detail, visibleColumns, visibleCards], writeUrlState) const reduced = computed(() => !permissions.canSeeStatistics) @@ -429,22 +477,25 @@ async function exportStatistics() { } } +/** + * @param {string} key - Card to toggle. + */ +function toggleCard(key) { + chosenCards.value = toggled(visibleCards.value, key) +} + /** * @param {string} key - Column to toggle. */ function toggleColumn(key) { - chosenColumns.value = visibleColumns.value.includes(key) - ? visibleColumns.value.filter((column) => column !== key) - : [...visibleColumns.value, key] + chosenColumns.value = toggled(visibleColumns.value, key) } /** * @param {number} categoryId - Category to toggle. */ function toggleCategory(categoryId) { - selectedCategoryIds.value = selectedCategoryIds.value.includes(categoryId) - ? selectedCategoryIds.value.filter((id) => id !== categoryId) - : [...selectedCategoryIds.value, categoryId] + selectedCategoryIds.value = toggled(selectedCategoryIds.value, categoryId) } /** @@ -486,6 +537,9 @@ function readUrlState() { columns: params.has('columns') ? params.get('columns').split(',').filter(Boolean) : null, + cards: params.has('cards') + ? params.get('cards').split(',').filter(Boolean) + : null, } } @@ -512,6 +566,7 @@ function writeUrlState() { // Only an explicit pick — the preset is already implied by `detail`, and // spelling it out would put a dozen redundant keys on every visit. if (chosenColumns.value !== null) params.set('columns', chosenColumns.value.join(',')) + if (chosenCards.value !== null) params.set('cards', chosenCards.value.join(',')) window.history.replaceState( window.history.state, @@ -620,6 +675,19 @@ function writeUrlState() { gap: 4px; } +.statistics__highlights-head { + align-items: center; + display: flex; + gap: 8px; + margin-bottom: 8px; +} + +.statistics__highlights-head h3 { + font-size: 15px; + font-weight: bold; + margin: 0; +} + .statistics__summary { color: var(--color-text-maxcontrast); margin-bottom: 16px; diff --git a/tests/unit/Service/StatisticsServiceTest.php b/tests/unit/Service/StatisticsServiceTest.php index 509cfba5..7bf96507 100644 --- a/tests/unit/Service/StatisticsServiceTest.php +++ b/tests/unit/Service/StatisticsServiceTest.php @@ -358,6 +358,47 @@ public function testOnlyAcceptancesFormTheSchedulingBasis(): void { $this->assertSame(1, $result['totals']['schedulingBase'], 'the totals row agrees'); } + /** + * Feeds the "longest not attended" card, so it has to be the *latest* + * appointment the person turned up for, and null for somebody who never did. + */ + public function testRecordsWhenEachPersonWasLastPresent(): void { + $this->givenUsers(['alice' => 'Alice', 'bob' => 'Bob']); + $this->givenUnrestrictedVisibility(); + $this->givenGroupSections(); + + $this->appointmentMapper->method('findForStatistics')->willReturn([ + $this->appointment(1, '2026-05-01 18:00:00', '2026-05-01 20:00:00'), + $this->appointment(2, '2026-05-08 18:00:00', '2026-05-08 20:00:00'), + ]); + $this->givenResponses([ + $this->response(1, 'alice', 'yes', 'yes'), + $this->response(2, 'alice', 'yes', 'yes'), + $this->response(1, 'bob', 'yes', 'yes'), + $this->response(2, 'bob', 'yes', 'no'), + ]); + + $people = array_column($this->service->getStatistics($this->filter())['people'], null, 'userId'); + + $this->assertStringStartsWith('2026-05-08', (string)$people['alice']['lastPresentAt'], 'the later of the two'); + $this->assertStringStartsWith('2026-05-01', (string)$people['bob']['lastPresentAt'], 'absent on the later one'); + } + + public function testLastPresentIsNullForSomebodyWhoNeverTurnedUp(): void { + $this->givenUsers(['alice' => 'Alice']); + $this->givenUnrestrictedVisibility(); + $this->givenGroupSections(); + + $this->appointmentMapper->method('findForStatistics')->willReturn([ + $this->appointment(1, '2026-05-01 18:00:00', '2026-05-01 20:00:00'), + ]); + $this->givenResponses([ + $this->response(1, 'alice', 'no', 'no'), + ]); + + $this->assertNull($this->service->getStatistics($this->filter())['people'][0]['lastPresentAt']); + } + public function testRefusesRangesBeyondTheAppointmentLimit(): void { $appointments = []; for ($id = 1; $id <= StatisticsService::MAX_APPOINTMENTS + 1; $id++) { From 90eea752dd5efeb57799d809d486c77354542357 Mon Sep 17 00:00:00 2001 From: Florian Ludwig Date: Sat, 15 Aug 2026 23:45:38 +0200 Subject: [PATCH 09/11] fix(calendar): count appointments the sync covers, not writes it made MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The skip introduced with the no-op change also changed what syncAppointment() reports. It returned false when the event was already current, and syncAllUpcoming() counts those returns — so the admin's sync button answered "0 appointments synced" on an instance where everything was in order, which reads as a failure rather than as nothing to do. Caught by the e2e suite, which is not part of the local gate. The return now means "the appointment is in the calendar afterwards", which is what the button's own promise is: create or update the events for all upcoming appointments. The write is still skipped; only the report changed. --- lib/Service/OrgCalendarSyncService.php | 20 +++++++++------ .../Service/OrgCalendarSyncServiceTest.php | 25 ++++++++++++++++++- 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/lib/Service/OrgCalendarSyncService.php b/lib/Service/OrgCalendarSyncService.php index c2f7a6de..4097657d 100644 --- a/lib/Service/OrgCalendarSyncService.php +++ b/lib/Service/OrgCalendarSyncService.php @@ -140,7 +140,12 @@ public function applySettings(array $orgCalendar, string $actingUserId): void { * are skipped: they already live in a calendar, and overwriting the source * event with our plain-text representation would be lossy. * - * @return bool True if an event was written + * @return bool True if the appointment is in the calendar afterwards — + * including when it was already current and nothing had to be + * written. Callers count coverage, not writes: the admin's + * sync button promises to create or update the events for all + * upcoming appointments, and reporting 0 because they were all + * already correct reads as a failure. */ public function syncAppointment(Appointment $appointment): bool { try { @@ -173,14 +178,13 @@ public function syncAppointment(Appointment $appointment): bool { $ics = $existingIcs !== null ? $this->patchIcs($existingIcs, $appointment) : $this->buildIcs($appointment, $uid); - if ($existingIcs !== null && $this->matchesIgnoringTimestamps($existingIcs, $ics)) { - // Nothing the reader would see changed. Writing anyway would - // still raise a calendar activity for everybody the calendar - // is shared with — see isOrgCalendarSummaryEnabled(). - $this->linkAppointment($appointment, $uid, $ownerCalendarUri); - return false; + // Skip a write that changes nothing a reader would see: it would + // still raise a calendar activity for everybody the calendar is + // shared with — see isOrgCalendarSummaryEnabled(). The appointment + // is in the calendar either way, so the caller is told so. + if ($existingIcs === null || !$this->matchesIgnoringTimestamps($existingIcs, $ics)) { + $backend->updateCalendarObject($calendarId, $objectUri, $ics); } - $backend->updateCalendarObject($calendarId, $objectUri, $ics); } else { $backend->createCalendarObject($calendarId, $objectUri, $this->buildIcs($appointment, $uid)); } diff --git a/tests/unit/Service/OrgCalendarSyncServiceTest.php b/tests/unit/Service/OrgCalendarSyncServiceTest.php index 2c0eb588..ceba1f86 100644 --- a/tests/unit/Service/OrgCalendarSyncServiceTest.php +++ b/tests/unit/Service/OrgCalendarSyncServiceTest.php @@ -500,10 +500,33 @@ public function testWriteSkippedWhenOnlyTheTimestampsWouldChange(): void { 'calendardata' => str_replace('20260801T100000Z', '20260101T090000Z', $stored), ]; - $this->assertFalse($this->service->syncAppointment($appointment)); + // True because the appointment *is* in the calendar — what the caller + // counts is coverage, not writes. The absent write is the point. + $this->assertTrue($this->service->syncAppointment($appointment)); $this->assertSame([], $this->backend->updated); } + /** + * The admin's sync button reports how many upcoming appointments the + * calendar now covers. Appointments whose event was already current must + * count, or a healthy instance reports zero and reads as broken. + */ + public function testBackfillCountsAppointmentsThatWereAlreadyCurrent(): void { + $this->configureEnabled(); + $appointment = $this->buildAppointment(); + $appointment->setCalendarUri('org-events-owner-uri'); + $appointment->setCalendarEventUid('attendance-org-5@cloud.example.com'); + + $this->backend->existingObjects['attendance-org-5@cloud.example.com.ics'] = [ + 'id' => 1, + 'calendardata' => $this->service->buildIcs($appointment, 'attendance-org-5@cloud.example.com'), + ]; + $this->appointmentMapper->method('findUpcoming')->willReturn([$appointment]); + + $this->assertSame(1, $this->service->syncAllUpcoming()); + $this->assertSame([], $this->backend->updated, 'counted without writing'); + } + public function testWriteHappensWhenTheSummaryLineMoved(): void { $this->configureEnabled(); $appointment = $this->buildAppointment(); From 5cb1d7d13278e68ae518e238dcd6578ebeada2de Mon Sep 17 00:00:00 2001 From: Florian Ludwig Date: Sun, 16 Aug 2026 11:41:06 +0200 Subject: [PATCH 10/11] feat(statistics): measure absence as a rate, not a last-seen date MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Longest not attended" answered when somebody was last there. What a manager plans against is how often somebody drops out, so the card now reads the absence rate and is titled plainly: "Absent more often". Not 1 - attendanceRate. An appointment nobody ticked off counts in the base but says nothing about whether the person was there, so only recorded absences count — absent over attendanceBase, sitting beside the other rates in the tally where it belongs. That retires lastPresentAt, added two commits ago for the date card: the field, its tests and its API type are gone again. With it goes the whole date apparatus in the card module — the "never present" sentinel, the scoring indirection it needed, and the two-way threshold. Every card now simply wants its highest values, and the module is a good deal shorter for it. The rate cards take Andreas' own wording, "Top …". The two counting cards keep theirs: "Top" suits a rate, not a tally. --- l10n/de.js | 14 +-- l10n/de.json | 14 +-- l10n/de_DE.js | 14 +-- l10n/de_DE.json | 14 +-- lib/ResponseDefinitions.php | 4 +- lib/Service/StatisticsService.php | 23 +--- lib/Service/StatisticsTally.php | 5 +- openapi-full.json | 23 +++- openapi.json | 23 +++- src/utils/statisticsCards.js | 105 ++++++------------- tests/unit/Service/StatisticsServiceTest.php | 40 +++---- 11 files changed, 122 insertions(+), 157 deletions(-) diff --git a/l10n/de.js b/l10n/de.js index 66a382fd..a91b1f0a 100644 --- a/l10n/de.js +++ b/l10n/de.js @@ -944,14 +944,14 @@ OC.L10N.register( "Show the response summary in the calendar event" : "Antwortübersicht im Kalendereintrag anzeigen", "Everyone the calendar is shared with sees how many people accepted, without opening the app. Keeping it current means writing to the event after every answer, and Nextcloud reports each of those writes as a calendar change." : "Alle, für die der Kalender freigegeben ist, sehen ohne die App zu öffnen, wie viele zugesagt haben. Dafür wird der Kalendereintrag nach jeder Antwort neu geschrieben, und Nextcloud meldet jeden dieser Schreibvorgänge als Kalenderänderung.", "Highlights" : "Highlights", - "Choose cards" : "Karten auswählen", - "Highest attendance rate" : "Höchste Anwesenheitsquote", - "Highest acceptance rate" : "Höchste Zusagequote", - "Highest response rate" : "Höchste Antwortquote", - "Highest scheduling rate" : "Höchste Einplanungsquote", "Most maybe answers" : "Meiste Vielleicht-Antworten", "Most times scheduled" : "Am häufigsten eingeplant", - "Longest not attended" : "Am längsten nicht dabei", - "Never" : "Nie" + "Never" : "Nie", + "Top attendance rate" : "Top Anwesenheitsquote", + "Top acceptance rate" : "Top Zusagequote", + "Top response rate" : "Top Antwortquote", + "Top scheduling rate" : "Top Einplanungsquote", + "Absent more often" : "Öfter nicht dabei", + "Choose highlights" : "Highlights auswählen" }, "nplurals=2; plural=(n != 1);"); diff --git a/l10n/de.json b/l10n/de.json index 0c9c4258..1ab419c2 100644 --- a/l10n/de.json +++ b/l10n/de.json @@ -942,14 +942,14 @@ "Show the response summary in the calendar event" : "Antwortübersicht im Kalendereintrag anzeigen", "Everyone the calendar is shared with sees how many people accepted, without opening the app. Keeping it current means writing to the event after every answer, and Nextcloud reports each of those writes as a calendar change." : "Alle, für die der Kalender freigegeben ist, sehen ohne die App zu öffnen, wie viele zugesagt haben. Dafür wird der Kalendereintrag nach jeder Antwort neu geschrieben, und Nextcloud meldet jeden dieser Schreibvorgänge als Kalenderänderung.", "Highlights" : "Highlights", - "Choose cards" : "Karten auswählen", - "Highest attendance rate" : "Höchste Anwesenheitsquote", - "Highest acceptance rate" : "Höchste Zusagequote", - "Highest response rate" : "Höchste Antwortquote", - "Highest scheduling rate" : "Höchste Einplanungsquote", "Most maybe answers" : "Meiste Vielleicht-Antworten", "Most times scheduled" : "Am häufigsten eingeplant", - "Longest not attended" : "Am längsten nicht dabei", - "Never" : "Nie" + "Never" : "Nie", + "Top attendance rate" : "Top Anwesenheitsquote", + "Top acceptance rate" : "Top Zusagequote", + "Top response rate" : "Top Antwortquote", + "Top scheduling rate" : "Top Einplanungsquote", + "Absent more often" : "Öfter nicht dabei", + "Choose highlights" : "Highlights auswählen" },"pluralForm" :"nplurals=2; plural=(n != 1);" } \ No newline at end of file diff --git a/l10n/de_DE.js b/l10n/de_DE.js index a03d9c0d..79f236d0 100644 --- a/l10n/de_DE.js +++ b/l10n/de_DE.js @@ -944,14 +944,14 @@ OC.L10N.register( "Show the response summary in the calendar event" : "Antwortübersicht im Kalendereintrag anzeigen", "Everyone the calendar is shared with sees how many people accepted, without opening the app. Keeping it current means writing to the event after every answer, and Nextcloud reports each of those writes as a calendar change." : "Alle, für die der Kalender freigegeben ist, sehen ohne die App zu öffnen, wie viele zugesagt haben. Dafür wird der Kalendereintrag nach jeder Antwort neu geschrieben, und Nextcloud meldet jeden dieser Schreibvorgänge als Kalenderänderung.", "Highlights" : "Highlights", - "Choose cards" : "Karten auswählen", - "Highest attendance rate" : "Höchste Anwesenheitsquote", - "Highest acceptance rate" : "Höchste Zusagequote", - "Highest response rate" : "Höchste Antwortquote", - "Highest scheduling rate" : "Höchste Einplanungsquote", "Most maybe answers" : "Meiste Vielleicht-Antworten", "Most times scheduled" : "Am häufigsten eingeplant", - "Longest not attended" : "Am längsten nicht dabei", - "Never" : "Nie" + "Never" : "Nie", + "Top attendance rate" : "Top Anwesenheitsquote", + "Top acceptance rate" : "Top Zusagequote", + "Top response rate" : "Top Antwortquote", + "Top scheduling rate" : "Top Einplanungsquote", + "Absent more often" : "Öfter nicht dabei", + "Choose highlights" : "Highlights auswählen" }, "nplurals=2; plural=(n != 1);"); diff --git a/l10n/de_DE.json b/l10n/de_DE.json index f377c971..10a5145d 100644 --- a/l10n/de_DE.json +++ b/l10n/de_DE.json @@ -942,14 +942,14 @@ "Show the response summary in the calendar event" : "Antwortübersicht im Kalendereintrag anzeigen", "Everyone the calendar is shared with sees how many people accepted, without opening the app. Keeping it current means writing to the event after every answer, and Nextcloud reports each of those writes as a calendar change." : "Alle, für die der Kalender freigegeben ist, sehen ohne die App zu öffnen, wie viele zugesagt haben. Dafür wird der Kalendereintrag nach jeder Antwort neu geschrieben, und Nextcloud meldet jeden dieser Schreibvorgänge als Kalenderänderung.", "Highlights" : "Highlights", - "Choose cards" : "Karten auswählen", - "Highest attendance rate" : "Höchste Anwesenheitsquote", - "Highest acceptance rate" : "Höchste Zusagequote", - "Highest response rate" : "Höchste Antwortquote", - "Highest scheduling rate" : "Höchste Einplanungsquote", "Most maybe answers" : "Meiste Vielleicht-Antworten", "Most times scheduled" : "Am häufigsten eingeplant", - "Longest not attended" : "Am längsten nicht dabei", - "Never" : "Nie" + "Never" : "Nie", + "Top attendance rate" : "Top Anwesenheitsquote", + "Top acceptance rate" : "Top Zusagequote", + "Top response rate" : "Top Antwortquote", + "Top scheduling rate" : "Top Einplanungsquote", + "Absent more often" : "Öfter nicht dabei", + "Choose highlights" : "Highlights auswählen" },"pluralForm" :"nplurals=2; plural=(n != 1);" } \ No newline at end of file diff --git a/lib/ResponseDefinitions.php b/lib/ResponseDefinitions.php index 3ff3513c..7bf3564a 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -328,7 +328,6 @@ * displayName: string, * isGuest: bool, * sections: list, - * lastPresentAt: ?string, * targetCount: int, * yes: int, * no: int, @@ -345,6 +344,7 @@ * responseRate: ?float, * acceptRate: ?float, * attendanceRate: ?float, + * absenceRate: ?float, * scheduledRate: ?float, * } * @psalm-type AttendanceStatisticsTotals = array{ @@ -364,6 +364,7 @@ * responseRate: ?float, * acceptRate: ?float, * attendanceRate: ?float, + * absenceRate: ?float, * scheduledRate: ?float, * } * @psalm-type AttendanceStatisticsSection = array{ @@ -386,6 +387,7 @@ * responseRate: ?float, * acceptRate: ?float, * attendanceRate: ?float, + * absenceRate: ?float, * scheduledRate: ?float, * } * @psalm-type AttendanceStatisticsTimelinePoint = array{ diff --git a/lib/Service/StatisticsService.php b/lib/Service/StatisticsService.php index 3f9e121c..a3b4ec47 100644 --- a/lib/Service/StatisticsService.php +++ b/lib/Service/StatisticsService.php @@ -27,9 +27,9 @@ * place: before closing nothing is decided, and an inquiry nobody was scheduled * for is one where the feature was not used. * - * @psalm-type StatsCounts = array{targetCount: int, yes: int, no: int, maybe: int, noResponse: int, present: int, absent: int, notRecorded: int, attendanceBase: int, noShow: int, scheduled: int, notScheduled: int, schedulingBase: int, responseRate: ?float, acceptRate: ?float, attendanceRate: ?float, scheduledRate: ?float} - * @psalm-type StatsPerson = array{userId: string, displayName: string, isGuest: bool, sections: list, lastPresentAt: ?string, targetCount: int, yes: int, no: int, maybe: int, noResponse: int, present: int, absent: int, notRecorded: int, attendanceBase: int, noShow: int, scheduled: int, notScheduled: int, schedulingBase: int, responseRate: ?float, acceptRate: ?float, attendanceRate: ?float, scheduledRate: ?float} - * @psalm-type StatsSection = array{id: string, displayName: string, personCount: int, targetCount: int, yes: int, no: int, maybe: int, noResponse: int, present: int, absent: int, notRecorded: int, attendanceBase: int, noShow: int, scheduled: int, notScheduled: int, schedulingBase: int, responseRate: ?float, acceptRate: ?float, attendanceRate: ?float, scheduledRate: ?float} + * @psalm-type StatsCounts = array{targetCount: int, yes: int, no: int, maybe: int, noResponse: int, present: int, absent: int, notRecorded: int, attendanceBase: int, noShow: int, scheduled: int, notScheduled: int, schedulingBase: int, responseRate: ?float, acceptRate: ?float, attendanceRate: ?float, absenceRate: ?float, scheduledRate: ?float} + * @psalm-type StatsPerson = array{userId: string, displayName: string, isGuest: bool, sections: list, targetCount: int, yes: int, no: int, maybe: int, noResponse: int, present: int, absent: int, notRecorded: int, attendanceBase: int, noShow: int, scheduled: int, notScheduled: int, schedulingBase: int, responseRate: ?float, acceptRate: ?float, attendanceRate: ?float, absenceRate: ?float, scheduledRate: ?float} + * @psalm-type StatsSection = array{id: string, displayName: string, personCount: int, targetCount: int, yes: int, no: int, maybe: int, noResponse: int, present: int, absent: int, notRecorded: int, attendanceBase: int, noShow: int, scheduled: int, notScheduled: int, schedulingBase: int, responseRate: ?float, acceptRate: ?float, attendanceRate: ?float, absenceRate: ?float, scheduledRate: ?float} * @psalm-type StatsTimelinePoint = array{appointmentId: int, name: string, startDatetime: ?string, targetCount: int, yes: int, present: int, attendanceRecorded: bool} * @psalm-type StatsCategory = array{categoryId: ?int, displayName: string, appointmentCount: int, targetCount: int, yes: int, present: int, attendanceBase: int, acceptRate: ?float, attendanceRate: ?float} * @psalm-type StatsPersonDetail = array{userId: string, displayName: string, isGuest: bool, entries: list} @@ -103,7 +103,6 @@ public function getStatistics(StatisticsFilter $filter, ?string $limitToUserId = 'displayName' => $displayName, 'isGuest' => $this->guestService->isGuestUser($userId), 'sections' => $membership[$userId] ?? [], - 'lastPresentAt' => $evaluation['lastPresent'][$userId] ?? null, ] + $tallies[$userId]->toArray(); } @@ -192,7 +191,7 @@ private function loadAppointments(StatisticsFilter $filter): array { /** * @param list $appointments - * @return array{tallies: array, lastPresent: array, timeline: list, byCategory: array, pastCount: int, attendanceRecordedCount: int} + * @return array{tallies: array, timeline: list, byCategory: array, pastCount: int, attendanceRecordedCount: int} */ private function evaluate(array $appointments): array { $appointmentIds = $this->idsOf($appointments); @@ -201,12 +200,6 @@ private function evaluate(array $appointments): array { /** @var array $tallies */ $tallies = []; - // userId → start of the most recent appointment they turned up for. - // Kept beside the tallies rather than inside them: it is a fact about a - // person, and merging it into a group or the totals row would answer a - // question nobody asks. - /** @var array $lastPresent */ - $lastPresent = []; $timeline = []; /** @var array $byCategory */ $byCategory = []; @@ -218,8 +211,7 @@ private function evaluate(array $appointments): array { $appointmentId = $appointment->getId(); $targets = $this->targetUserIds($appointment); $isPast = $this->hasEnded($appointment); - // Hoisted: startOf() serializes the whole entity, and both the inner - // loop and the timeline entry below want the same string. + // Hoisted: startOf() serializes the whole entity to read one field. $start = $this->startOf($appointment); $countsForAttendance = $isPast && isset($checkedIn[$appointmentId]); $countsForScheduling = $bookingEnabled @@ -245,10 +237,6 @@ private function evaluate(array $appointments): array { $tallies[$targetUserId]->record($response, $checkin, $countsForAttendance, $countsForScheduling, $isScheduled); $perAppointment->record($response, $checkin, $countsForAttendance, $countsForScheduling, $isScheduled); - // findForStatistics() orders ascending, so the last write wins. - if ($checkin === 'yes' && $start !== null) { - $lastPresent[$targetUserId] = $start; - } } $categoryKey = $appointment->getCategoryId() ?? 0; @@ -275,7 +263,6 @@ private function evaluate(array $appointments): array { return [ 'tallies' => $tallies, - 'lastPresent' => $lastPresent, 'timeline' => $timeline, 'byCategory' => $byCategory, 'pastCount' => $pastCount, diff --git a/lib/Service/StatisticsTally.php b/lib/Service/StatisticsTally.php index 0a9ae8f3..e983f33a 100644 --- a/lib/Service/StatisticsTally.php +++ b/lib/Service/StatisticsTally.php @@ -89,7 +89,7 @@ public function add(self $other): void { } /** - * @return array{targetCount: int, yes: int, no: int, maybe: int, noResponse: int, present: int, absent: int, notRecorded: int, attendanceBase: int, noShow: int, scheduled: int, notScheduled: int, schedulingBase: int, responseRate: ?float, acceptRate: ?float, attendanceRate: ?float, scheduledRate: ?float} + * @return array{targetCount: int, yes: int, no: int, maybe: int, noResponse: int, present: int, absent: int, notRecorded: int, attendanceBase: int, noShow: int, scheduled: int, notScheduled: int, schedulingBase: int, responseRate: ?float, acceptRate: ?float, attendanceRate: ?float, absenceRate: ?float, scheduledRate: ?float} */ public function toArray(): array { return [ @@ -109,6 +109,9 @@ public function toArray(): array { 'responseRate' => self::rate($this->yes + $this->no + $this->maybe, $this->targetCount), 'acceptRate' => self::rate($this->yes, $this->targetCount), 'attendanceRate' => self::rate($this->present, $this->attendanceBase), + // Not 1 - attendanceRate: appointments nobody wrote down count in the + // base but say nothing about whether the person was there. + 'absenceRate' => self::rate($this->absent, $this->attendanceBase), 'scheduledRate' => self::rate($this->scheduled, $this->schedulingBase), ]; } diff --git a/openapi-full.json b/openapi-full.json index 01eeafba..67c9ebfb 100644 --- a/openapi-full.json +++ b/openapi-full.json @@ -1508,7 +1508,6 @@ "displayName", "isGuest", "sections", - "lastPresentAt", "targetCount", "yes", "no", @@ -1525,6 +1524,7 @@ "responseRate", "acceptRate", "attendanceRate", + "absenceRate", "scheduledRate" ], "properties": { @@ -1543,10 +1543,6 @@ "type": "string" } }, - "lastPresentAt": { - "type": "string", - "nullable": true - }, "targetCount": { "type": "integer", "format": "int64" @@ -1614,6 +1610,11 @@ "format": "double", "nullable": true }, + "absenceRate": { + "type": "number", + "format": "double", + "nullable": true + }, "scheduledRate": { "type": "number", "format": "double", @@ -1709,6 +1710,7 @@ "responseRate", "acceptRate", "attendanceRate", + "absenceRate", "scheduledRate" ], "properties": { @@ -1789,6 +1791,11 @@ "format": "double", "nullable": true }, + "absenceRate": { + "type": "number", + "format": "double", + "nullable": true + }, "scheduledRate": { "type": "number", "format": "double", @@ -1855,6 +1862,7 @@ "responseRate", "acceptRate", "attendanceRate", + "absenceRate", "scheduledRate" ], "properties": { @@ -1925,6 +1933,11 @@ "format": "double", "nullable": true }, + "absenceRate": { + "type": "number", + "format": "double", + "nullable": true + }, "scheduledRate": { "type": "number", "format": "double", diff --git a/openapi.json b/openapi.json index 7f5904ac..04fb0911 100644 --- a/openapi.json +++ b/openapi.json @@ -1274,7 +1274,6 @@ "displayName", "isGuest", "sections", - "lastPresentAt", "targetCount", "yes", "no", @@ -1291,6 +1290,7 @@ "responseRate", "acceptRate", "attendanceRate", + "absenceRate", "scheduledRate" ], "properties": { @@ -1309,10 +1309,6 @@ "type": "string" } }, - "lastPresentAt": { - "type": "string", - "nullable": true - }, "targetCount": { "type": "integer", "format": "int64" @@ -1380,6 +1376,11 @@ "format": "double", "nullable": true }, + "absenceRate": { + "type": "number", + "format": "double", + "nullable": true + }, "scheduledRate": { "type": "number", "format": "double", @@ -1475,6 +1476,7 @@ "responseRate", "acceptRate", "attendanceRate", + "absenceRate", "scheduledRate" ], "properties": { @@ -1555,6 +1557,11 @@ "format": "double", "nullable": true }, + "absenceRate": { + "type": "number", + "format": "double", + "nullable": true + }, "scheduledRate": { "type": "number", "format": "double", @@ -1621,6 +1628,7 @@ "responseRate", "acceptRate", "attendanceRate", + "absenceRate", "scheduledRate" ], "properties": { @@ -1691,6 +1699,11 @@ "format": "double", "nullable": true }, + "absenceRate": { + "type": "number", + "format": "double", + "nullable": true + }, "scheduledRate": { "type": "number", "format": "double", diff --git a/src/utils/statisticsCards.js b/src/utils/statisticsCards.js index e24e5358..41039362 100644 --- a/src/utils/statisticsCards.js +++ b/src/utils/statisticsCards.js @@ -18,42 +18,30 @@ const MAX_ROWS = 5 const MIN_BASIS_SHARE = 0.5 /** - * Score of somebody who was never recorded present. Sorts to the head of the - * ascending date card instead of dropping out of it, and Number.isFinite keeps - * it out of the mean for free. Named so the component can ask about it without - * spelling the sentinel a second time. - */ -const NEVER_PRESENT = -Infinity - -/** - * Every card, in display order. - * - * `direction` says which end is interesting and therefore which side of the - * average survives: `desc` keeps everyone at or above it, `asc` everyone at or - * below. The award cards are `desc`, so nobody is ever shown as the tail of a - * ranking; "longest not attended" is `asc`, where the tail is the whole point. + * Every card, in display order. Each one names whoever sits highest on its + * metric — including "absent more often", where the top of the list is the + * point. Award wording stays with the rates that reflect an effort; the two + * cards about falling short are titled plainly. * - * `basis` marks a rate: those cards carry their denominator per name and drop - * anyone below MIN_BASIS_SHARE. Count cards need neither — a small denominator - * cannot win a count. + * `basis` marks a rate: those carry their denominator and drop anyone below + * MIN_BASIS_SHARE. Count cards need neither — a small denominator cannot win + * a count. */ export const STATISTICS_CARDS = [ { key: 'attendanceRate', // TRANSLATORS: Card title. Lists the people who turned up most reliably. - label: t('attendance', 'Highest attendance rate'), + label: t('attendance', 'Top attendance rate'), metric: 'attendanceRate', basis: 'attendanceBase', - direction: 'desc', default: true, }, { key: 'acceptRate', // TRANSLATORS: Card title. Lists the people who accepted most often. - label: t('attendance', 'Highest acceptance rate'), + label: t('attendance', 'Top acceptance rate'), metric: 'acceptRate', basis: 'targetCount', - direction: 'desc', default: true, }, { @@ -61,33 +49,29 @@ export const STATISTICS_CARDS = [ // TRANSLATORS: Card title. Lists the people who answered "maybe" most often — a light-hearted one, not a reproach. label: t('attendance', 'Most maybe answers'), metric: 'maybe', - direction: 'desc', default: true, }, { - key: 'lastPresent', - // TRANSLATORS: Card title. Lists who has not been to an appointment for the longest, so nobody quietly drops off the radar. - label: t('attendance', 'Longest not attended'), - metric: 'lastPresentAt', - direction: 'asc', - date: true, + key: 'absenceRate', + // TRANSLATORS: Card title. Lists who was recorded absent most often, so nobody quietly drops off the radar. Deliberately not phrased as an award. + label: t('attendance', 'Absent more often'), + metric: 'absenceRate', + basis: 'attendanceBase', default: true, }, { key: 'responseRate', // TRANSLATORS: Card title. Lists the people who answered most reliably, whatever the answer was. - label: t('attendance', 'Highest response rate'), + label: t('attendance', 'Top response rate'), metric: 'responseRate', basis: 'targetCount', - direction: 'desc', }, { key: 'scheduledRate', // TRANSLATORS: Card title. Lists the people whose acceptances most often got them a place. - label: t('attendance', 'Highest scheduling rate'), + label: t('attendance', 'Top scheduling rate'), metric: 'scheduledRate', basis: 'schedulingBase', - direction: 'desc', scheduling: true, }, { @@ -95,7 +79,6 @@ export const STATISTICS_CARDS = [ // TRANSLATORS: Card title. Lists who got a place in the most appointments. label: t('attendance', 'Most times scheduled'), metric: 'scheduled', - direction: 'desc', scheduling: true, }, ] @@ -125,28 +108,21 @@ export function defaultCards(scheduling) { * @return {Array|null} Rows, or null when the card has nothing to say. */ export function cardRows(card, people, totals) { - const descending = card.direction === 'desc' - - // Scored once: the rest of this function only ever compares numbers, and on - // the date card each score is a Date.parse nobody should pay four times. - const scored = eligiblePeople(card, people).map((person) => ({ person, score: value(card, person) })) - if (scored.length === 0) { + const eligible = eligiblePeople(card, people) + if (eligible.length === 0) { return null } - const threshold = averageOf(card, scored, totals) - // A zero never earns a line on a counting card: with nobody saying maybe all - // period everyone sits at zero, clears the average and the card would crown - // the whole team for something that never happened. The date card is exempt - // — there "never attended" is precisely what deserves a line. - const keep = card.date - ? (score) => threshold === null || score <= threshold - : (score) => score > 0 && (threshold === null || (descending ? score >= threshold : score <= threshold)) + const threshold = averageOf(card, eligible, totals) /** @type {Map>} */ const byValue = new Map() - for (const { person, score } of scored) { - if (!keep(score)) { + for (const person of eligible) { + const score = person[card.metric] + // A zero never earns a line: with nobody saying maybe all period everyone + // sits at zero, clears the average, and the card would name the whole + // team for something that never happened. + if (score <= 0 || (threshold !== null && score < threshold)) { continue } const bucket = byValue.get(score) @@ -162,23 +138,9 @@ export function cardRows(card, people, totals) { } return [...byValue.entries()] - .sort(([a], [b]) => (descending ? b - a : a - b)) + .sort(([a], [b]) => b - a) .slice(0, MAX_ROWS) - .map(([score, group]) => ({ value: score, never: score === NEVER_PRESENT, people: group })) -} - -/** - * @param {object} card - Card descriptor. - * @param {object} person - A person row. - * @return {number} The sortable value, with "never present" as -Infinity so it - * leads the ascending date card rather than dropping out of it. - */ -function value(card, person) { - if (!card.date) { - return person[card.metric] - } - const raw = person[card.metric] - return raw ? Date.parse(raw) : NEVER_PRESENT + .map(([score, group]) => ({ value: score, people: group })) } /** @@ -187,10 +149,6 @@ function value(card, person) { * @return {Array} Those the card may consider at all. */ function eligiblePeople(card, people) { - if (card.date) { - return people - } - const withValue = people.filter((person) => person[card.metric] !== null && person[card.metric] !== undefined) if (!card.basis) { return withValue @@ -206,19 +164,18 @@ function eligiblePeople(card, people) { * the people the card considers. * * @param {object} card - Card descriptor. - * @param {Array} scored - The people under consideration, with scores. + * @param {Array} eligible - The people under consideration. * @param {object} totals - The totals row. * @return {?number} The threshold, or null when there is nothing to cut at. */ -function averageOf(card, scored, totals) { +function averageOf(card, eligible, totals) { if (card.basis && totals[card.metric] !== null && totals[card.metric] !== undefined) { return totals[card.metric] } - const values = scored.map(({ score }) => score).filter(Number.isFinite) - if (values.length === 0) { + if (eligible.length === 0) { return null } - return values.reduce((sum, v) => sum + v, 0) / values.length + return eligible.reduce((sum, person) => sum + person[card.metric], 0) / eligible.length } diff --git a/tests/unit/Service/StatisticsServiceTest.php b/tests/unit/Service/StatisticsServiceTest.php index 7bf96507..5b23faab 100644 --- a/tests/unit/Service/StatisticsServiceTest.php +++ b/tests/unit/Service/StatisticsServiceTest.php @@ -359,44 +359,34 @@ public function testOnlyAcceptancesFormTheSchedulingBasis(): void { } /** - * Feeds the "longest not attended" card, so it has to be the *latest* - * appointment the person turned up for, and null for somebody who never did. + * Feeds the "absent more often" card. Deliberately not 1 - attendanceRate: + * an appointment nobody wrote down counts in the base but says nothing + * about whether the person was there. */ - public function testRecordsWhenEachPersonWasLastPresent(): void { - $this->givenUsers(['alice' => 'Alice', 'bob' => 'Bob']); + public function testAbsenceRateCountsOnlyRecordedAbsences(): void { + $this->givenUsers(['alice' => 'Alice']); $this->givenUnrestrictedVisibility(); $this->givenGroupSections(); $this->appointmentMapper->method('findForStatistics')->willReturn([ $this->appointment(1, '2026-05-01 18:00:00', '2026-05-01 20:00:00'), $this->appointment(2, '2026-05-08 18:00:00', '2026-05-08 20:00:00'), + $this->appointment(3, '2026-05-15 18:00:00', '2026-05-15 20:00:00'), ]); $this->givenResponses([ $this->response(1, 'alice', 'yes', 'yes'), - $this->response(2, 'alice', 'yes', 'yes'), - $this->response(1, 'bob', 'yes', 'yes'), - $this->response(2, 'bob', 'yes', 'no'), + $this->response(2, 'alice', 'yes', 'no'), + // Somebody worked this list, but nobody ticked Alice either way + $this->response(3, 'alice', 'yes', ''), + $this->response(3, 'bob', 'yes', 'yes'), ]); - $people = array_column($this->service->getStatistics($this->filter())['people'], null, 'userId'); - - $this->assertStringStartsWith('2026-05-08', (string)$people['alice']['lastPresentAt'], 'the later of the two'); - $this->assertStringStartsWith('2026-05-01', (string)$people['bob']['lastPresentAt'], 'absent on the later one'); - } - - public function testLastPresentIsNullForSomebodyWhoNeverTurnedUp(): void { - $this->givenUsers(['alice' => 'Alice']); - $this->givenUnrestrictedVisibility(); - $this->givenGroupSections(); - - $this->appointmentMapper->method('findForStatistics')->willReturn([ - $this->appointment(1, '2026-05-01 18:00:00', '2026-05-01 20:00:00'), - ]); - $this->givenResponses([ - $this->response(1, 'alice', 'no', 'no'), - ]); + $alice = $this->person($this->service->getStatistics($this->filter()), 'alice'); - $this->assertNull($this->service->getStatistics($this->filter())['people'][0]['lastPresentAt']); + $this->assertSame(3, $alice['attendanceBase']); + $this->assertSame(1, $alice['absent']); + $this->assertSame(1, $alice['notRecorded']); + $this->assertEqualsWithDelta(1 / 3, $alice['absenceRate'], 0.0001); } public function testRefusesRangesBeyondTheAppointmentLimit(): void { From 9e02bc5b26c5cad6c1a62fd13311c681739c6166 Mon Sep 17 00:00:00 2001 From: Florian Ludwig Date: Sun, 16 Aug 2026 11:41:07 +0200 Subject: [PATCH 11/11] fix(statistics): make the highlight cards readable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things the first cut got wrong, all visible the moment real data went through it. Every name sat on its own line with a leading comma. The button reset took the colours and the border but not the box model, and Nextcloud styles every button as a full-width block with its own min-height — the same trap as the row backgrounds in the table. The value sat beside the names as a nowrap column. Carrying the basis it ran to half the card width and left the names a strip so narrow that "Christina Vogel" broke in two. It heads its group now, and the names run on beneath it. The separator leaned on the whitespace between the buttons, which rendered as a space in front of every comma and stranded one at the start of a wrapped line. It is a pseudo-element plus a column gap now, so nothing depends on invisible text nodes. Also: the basis is stated once per row instead of after every name — four repetitions of "(4 appointments)" drowned out the names it was meant to qualify — and rows are further apart than the lines within them, so it is visible which names belong to which value. --- .../statistics/StatisticsHighlights.vue | 85 ++++++++++++++----- src/views/StatisticsOverview.vue | 4 +- 2 files changed, 67 insertions(+), 22 deletions(-) diff --git a/src/components/statistics/StatisticsHighlights.vue b/src/components/statistics/StatisticsHighlights.vue index ca605fc0..bad20e30 100644 --- a/src/components/statistics/StatisticsHighlights.vue +++ b/src/components/statistics/StatisticsHighlights.vue @@ -10,7 +10,12 @@
  • - {{ formatValue(card, row) }} + + {{ formatValue(card, row) }} + + {{ sharedBasis(card, row) }} + + - {{ row.people.map((person) => nameOf(card, person)).join(", ") }} + {{ row.people.map((person) => nameOf(card, row, person)).join(", ") }}
@@ -61,7 +66,9 @@ const cards = computed(() => STATISTICS_CARDS function formatValue(card, row) { if (card.date) { // TRANSLATORS: Shown instead of a date for somebody who was not recorded present at any appointment in the period. - return row.never ? t('attendance', 'Never') : formatDate(new Date(row.value).toISOString(), 'short') + // Medium like the person sidebar: here the date is the content, not an + // axis label, and "21 July 2026" beats "21/07/2026" for reading. + return row.never ? t('attendance', 'Never') : formatDate(new Date(row.value)) } if (card.basis) { return formatRate(row.value) @@ -70,16 +77,36 @@ function formatValue(card, row) { } /** - * Rate cards carry the denominator behind each name: two people can share - * 100 % on wildly different numbers of appointments, and the reader deserves - * to see which. + * The denominator a rate row rests on, when everybody in it shares one — which + * is the normal case, the card only ever considering comparable bases. Said + * once beside the value instead of after every name, where four repetitions of + * "(4 appointments)" drown out the names themselves. * * @param {object} card - Card descriptor. - * @param {object} person - A person row. - * @return {string} The name, with its basis where one applies. + * @param {object} row - The row to label. + * @return {?string} The basis, or null when it differs within the row. */ -function nameOf(card, person) { +function sharedBasis(card, row) { if (!card.basis) { + return null + } + const first = row.people[0][card.basis] + return row.people.every((person) => person[card.basis] === first) + ? `(${n('attendance', '%n appointment', '%n appointments', first)})` + : null +} + +/** + * Two people can share a rate on different numbers of appointments. Where that + * happens the row cannot label itself, so each name carries its own basis. + * + * @param {object} card - Card descriptor. + * @param {object} row - The row the name sits in. + * @param {object} person - A person row. + * @return {string} The name, with its basis where the row could not say it. + */ +function nameOf(card, row, person) { + if (!card.basis || sharedBasis(card, row)) { return person.displayName } return `${person.displayName} (${n('attendance', '%n appointment', '%n appointments', person[card.basis])})` @@ -90,7 +117,7 @@ function nameOf(card, person) { .highlights { display: grid; gap: 12px; - grid-template-columns: repeat(auto-fit, minmax(220px, 1fr)); + grid-template-columns: repeat(auto-fit, minmax(240px, 1fr)); margin-bottom: 16px; } @@ -112,43 +139,61 @@ function nameOf(card, person) { .highlights__rows { display: flex; flex-direction: column; - gap: 4px; + /* Groups need more air between them than a value has to its own names, or + the reader has to work out which names belong to which value. */ + gap: 14px; } .highlights__row { - display: flex; font-size: 13px; - gap: 8px; + line-height: 1.35; } +/* The value heads its group rather than sitting beside it: it is nowrap and + carries the basis, so as a column it left the names a strip too narrow to + fit one. Above them they get the whole card. */ .highlights__value { - color: var(--color-main-text); + display: block; font-weight: bold; - white-space: nowrap; } +.highlights__basis { + color: var(--color-text-maxcontrast); + font-weight: normal; +} + +/* The separator is a gap plus a pseudo-element, not literal text: relying on + the whitespace between the buttons put a space in front of every comma and + left a stray one at the start of a wrapped line. */ .highlights__names { color: var(--color-text-maxcontrast); + column-gap: 4px; + display: flex; + flex-wrap: wrap; } -/* Reset rather than restyle: these sit inline inside a comma-separated run, so - any of the button chrome the server theme adds would break the line up. */ +/* Nextcloud styles every button as a full-width block with its own min-height; + these sit in a comma-separated run, so the reset has to take the box model + with it, not just the colours. */ .highlights__person { background: none; border: none; color: inherit; cursor: pointer; font: inherit; + line-height: inherit; margin: 0; + min-height: 0; padding: 0; text-align: start; + width: auto; } .highlights__person:hover { text-decoration: underline; } -.highlights__person:not(:first-child)::before { - content: ", "; +.highlights__person:not(:last-child)::after { + content: ","; } diff --git a/src/views/StatisticsOverview.vue b/src/views/StatisticsOverview.vue index a1f3ddd7..1940b239 100644 --- a/src/views/StatisticsOverview.vue +++ b/src/views/StatisticsOverview.vue @@ -184,11 +184,11 @@

{{ t("attendance", "Highlights") }}