From 4944b9b3b6f57282857d1a5ffe1aa6374bc76a25 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sat, 5 Sep 2026 21:42:21 +0200 Subject: [PATCH] fix(usage): scope the participation notice, highlight it, and call ignoring what it is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It appears on the dashboard and settings only. It is an invitation, not an alert, so it belongs on pages an admin opens deliberately rather than on top of whatever task they are in the middle of. The activity ticker deliberately did NOT move with it. That ticker is what triggers the daily rollup — the backend has no scheduler — so tying it to the banner would have stopped reporting for an admin who works on Events and never opens the dashboard, and stopped it entirely for a participating install, where the banner never renders at all. The effect stays mounted on every admin page and only the visible aside is scoped. Two tests pin exactly that, because it is the kind of thing a later refactor would helpfully "clean up". Highlighted like the migration banner it sits under: tinted surface, border, icon, a title line above the body. It was previously the same neutral surface as the page behind it and read as filler. "Not now" is now "Ignore". The button calls dismiss(), which persists notice_dismissed on the server — the invitation never comes back. "Not now" promised otherwise. The label says what happens and a hint says where to join later. Only shown while participation is off. activation_pending, deletion_pending and identity_conflict are in-flight states the settings page explains properly; inviting someone to join in the middle of their own withdrawal would be worse than saying nothing. The first version of these tests was worthless: the negative cases asserted absence after waiting only for the status call, so the component was still rendering null for want of data and every one passed with the gates removed. They now wait for the query cache to fill. Removing the route gate fails 4; removing the status gate fails 6. Refs #1110 --- .../components/admin/ProductUsageNotice.tsx | 85 ++++++++---- .../__tests__/ProductUsageNotice.test.tsx | 124 ++++++++++++++++++ frontend/src/i18n/locales/de.json | 6 +- frontend/src/i18n/locales/en.json | 6 +- 4 files changed, 194 insertions(+), 27 deletions(-) create mode 100644 frontend/src/components/admin/__tests__/ProductUsageNotice.test.tsx diff --git a/frontend/src/components/admin/ProductUsageNotice.tsx b/frontend/src/components/admin/ProductUsageNotice.tsx index 922c8feb..1d22695d 100644 --- a/frontend/src/components/admin/ProductUsageNotice.tsx +++ b/frontend/src/components/admin/ProductUsageNotice.tsx @@ -1,21 +1,34 @@ import { useEffect } from 'react'; import { useQuery, useQueryClient } from '@tanstack/react-query'; -import { Link } from 'react-router-dom'; +import { Link, useLocation } from 'react-router-dom'; import { useTranslation } from 'react-i18next'; +import { Sparkles } from 'lucide-react'; import { usePermissions } from '../../contexts/PermissionsContext'; import { productUsageService } from '../../services/productUsage.service'; +// Where the invitation is allowed to appear. It is an invitation, not an +// alert, so it belongs on the pages an admin visits deliberately rather than +// on top of whatever task they are in the middle of. +const NOTICE_PATHS = ['/admin/dashboard', '/admin/settings']; + // Loaded only inside the authenticated admin tree. Gallery routes never import // this chunk, make usage requests, or record product usage markers. export default function ProductUsageNotice() { const { t } = useTranslation(); const { hasPermission } = usePermissions(); const queryClient = useQueryClient(); + const { pathname } = useLocation(); const { data } = useQuery({ queryKey: ['productUsage'], queryFn: productUsageService.status, enabled: hasPermission('settings.edit') }); + // Deliberately above every visibility test, and deliberately NOT limited to + // the pages the banner is shown on. This ticker is what triggers the daily + // rollup — the backend has no scheduler — so tying it to the banner would + // mean an admin who works on Events and never opens the dashboard stops + // reporting altogether, and a participating install (where the banner never + // renders at all) would never report again. useEffect(() => { let running = false; const tick = async () => { @@ -37,32 +50,58 @@ export default function ProductUsageNotice() { document.removeEventListener('visibilitychange', tick); }; }, []); - if (!hasPermission('settings.edit') || !data || data.status !== 'disabled' || data.notice_dismissed) return null; + + if (!hasPermission('settings.edit') || !data) return null; + // Only while participation is off. `activation_pending`, `deletion_pending` + // and `identity_conflict` are all in-flight states the settings page + // explains properly; inviting someone to join in the middle of their own + // withdrawal would be worse than saying nothing. + if (data.status !== 'disabled' || data.notice_dismissed) return null; + if (!NOTICE_PATHS.some((path) => pathname.startsWith(path))) return null; + return ( ); diff --git a/frontend/src/components/admin/__tests__/ProductUsageNotice.test.tsx b/frontend/src/components/admin/__tests__/ProductUsageNotice.test.tsx new file mode 100644 index 00000000..bf960277 --- /dev/null +++ b/frontend/src/components/admin/__tests__/ProductUsageNotice.test.tsx @@ -0,0 +1,124 @@ +/** + * The participation invitation (#1110). + * + * Two properties matter here and are easy to break by accident: + * + * - it is an INVITATION, so it appears only where an admin goes + * deliberately, and only while participation is actually off; + * - the activity ticker inside it is what triggers the daily rollup — the + * backend has no scheduler — so it must keep running on every admin page, + * including the ones where the banner is not rendered and the case where + * the install is already participating and the banner never renders at all. + */ +import { render, screen, waitFor, cleanup } from '@testing-library/react'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { MemoryRouter } from 'react-router-dom'; +import { beforeEach, afterEach, describe, it, expect, vi } from 'vitest'; +import ProductUsageNotice from '../ProductUsageNotice'; +import { productUsageService as service } from '../../../services/productUsage.service'; + +vi.mock('react-i18next', () => ({ + useTranslation: () => ({ t: (key: string) => key }), + initReactI18next: { type: '3rdParty', init: () => {} } +})); +vi.mock('../../../contexts/PermissionsContext', () => ({ + usePermissions: () => ({ hasPermission: () => true }) +})); +vi.mock('../../../services/productUsage.service', () => ({ + productUsageService: { status: vi.fn(), activity: vi.fn(), dismiss: vi.fn() } +})); + +const status = (over = {}) => ({ + status: 'disabled', + notice_dismissed: false, + installation_id: null, + collector_url: 'https://collector.example', + schema_version: 'usage.v1', + last_report_date: null, + last_error: null, + pending_action: null, + last_packet: null, + feedback_preferences: { name: '' }, + ...over +}); + +function renderAt(path: string) { + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + render( + + + + + + ); + // Absence only means something once the status has actually landed. + // Waiting on the service being *called* proved nothing: the component + // returns null while `data` is undefined, so every negative assertion + // passed even with the gate removed. + return { + settled: () => + waitFor(() => expect(client.getQueryData(['productUsage'])).toBeDefined()) + }; +} + +beforeEach(() => { + vi.mocked(service.status).mockResolvedValue(status() as never); + vi.mocked(service.activity).mockResolvedValue(undefined as never); +}); +afterEach(() => { cleanup(); vi.clearAllMocks(); }); + +describe('product usage notice', () => { + it.each(['/admin/dashboard', '/admin/settings', '/admin/settings?tab=usage'])( + 'invites participation on %s', + async (path) => { + renderAt(path); + expect(await screen.findByText('productUsage.noticeTitle')).toBeInTheDocument(); + // The dismissal is permanent, so the label must not promise a return. + expect(screen.getByText('productUsage.ignore')).toBeInTheDocument(); + expect(screen.getByText('productUsage.ignoreHint')).toBeInTheDocument(); + } + ); + + it.each(['/admin/events', '/admin/archives', '/admin/users'])( + 'stays out of the way on %s', + async (path) => { + const { settled } = renderAt(path); + await settled(); + expect(screen.queryByText('productUsage.noticeTitle')).not.toBeInTheDocument(); + } + ); + + it.each(['active', 'activation_pending', 'deletion_pending', 'identity_conflict'])( + 'does not invite while participation is %s', + async (state) => { + vi.mocked(service.status).mockResolvedValue(status({ status: state }) as never); + const { settled } = renderAt('/admin/dashboard'); + await settled(); + expect(screen.queryByText('productUsage.noticeTitle')).not.toBeInTheDocument(); + } + ); + + it('does not invite again once ignored', async () => { + vi.mocked(service.status).mockResolvedValue(status({ notice_dismissed: true }) as never); + const { settled } = renderAt('/admin/dashboard'); + await settled(); + expect(screen.queryByText('productUsage.noticeTitle')).not.toBeInTheDocument(); + }); + + it('still reports activity on a page where the banner is hidden', async () => { + // The rollup must not depend on which page the admin happens to be on. + const { settled } = renderAt('/admin/events'); + await waitFor(() => expect(service.activity).toHaveBeenCalled()); + await settled(); + expect(screen.queryByText('productUsage.noticeTitle')).not.toBeInTheDocument(); + }); + + it('still reports activity for an install that is already participating', async () => { + // The banner never renders in this state; reporting must continue anyway. + vi.mocked(service.status).mockResolvedValue(status({ status: 'active' }) as never); + const { settled } = renderAt('/admin/dashboard'); + await waitFor(() => expect(service.activity).toHaveBeenCalled()); + await settled(); + expect(screen.queryByText('productUsage.noticeTitle')).not.toBeInTheDocument(); + }); +}); diff --git a/frontend/src/i18n/locales/de.json b/frontend/src/i18n/locales/de.json index bd2b1364..5e282121 100644 --- a/frontend/src/i18n/locales/de.json +++ b/frontend/src/i18n/locales/de.json @@ -1,9 +1,11 @@ { "productUsage": { "title": "Produktnutzung & Feedback", - "notice": "Hilf mit, PicPeak weiterzuentwickeln. Freiwillige Nutzungsberichte zeigen, welche Funktionen der Community wichtig sind. Berichte bleiben aus, bis du ausdrücklich teilnimmst.", + "noticeTitle": "Gestalten Sie PicPeak mit", + "notice": "Optionale Nutzungsberichte zeigen, welche Funktionen für die Community wichtig sind. Die Übermittlung ist aus, bis Sie sich aktiv dafür entscheiden.", + "ignore": "Ignorieren", + "ignoreHint": "Dieser Hinweis erscheint nicht erneut — Sie können weiterhin unter Einstellungen → Produktnutzung teilnehmen.", "review": "Teilnahme prüfen", - "later": "Nicht jetzt", "cancel": "Abbrechen", "loading": "Teilnahmeeinstellungen werden geladen…", "failed": "Der Vorgang konnte nicht abgeschlossen werden. Prüfe den Status und versuche es erneut.", diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index b9f7c1de..46ee590f 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -1,9 +1,11 @@ { "productUsage": { "title": "Product usage & feedback", - "notice": "Help shape PicPeak. Optional product usage reports show which features matter to the community. Reporting is off until you choose to participate.", + "noticeTitle": "Help shape PicPeak", + "notice": "Optional product usage reports show which features matter to the community. Reporting is off until you choose to participate.", + "ignore": "Ignore", + "ignoreHint": "This notice won't appear again — you can still join from Settings → Product usage.", "review": "Review participation", - "later": "Not now", "cancel": "Cancel", "loading": "Loading participation settings…", "failed": "The operation could not be completed. Check the status and try again.",