diff --git a/client/scripts/feature-pairs.json b/client/scripts/feature-pairs.json index 9e74bb34c..d0e4480b9 100644 --- a/client/scripts/feature-pairs.json +++ b/client/scripts/feature-pairs.json @@ -862,6 +862,7 @@ "src/mobile/screens/atlas/MAtlasSearch.tsx": "Phone full screen country search overlay; options, suggestions and selection come through props from MAtlas.", "src/mobile/screens/atlas/MAtlasStatsCard.tsx": "Markup only; renders the AtlasStats that MAtlas passes in.", "src/mobile/screens/collections/MCollCategoryPicker.tsx": "Phone only category dropdown; value and categories come through props from the collection sheets.", + "src/mobile/screens/collections/MCollInvites.tsx": "Markup only; the invites and accept and decline come through props from useCollections via MCollections.", "src/mobile/screens/collections/MCollLabelsSheet.tsx": "Phone label sheet; create, update, delete and assign come through props from useCollections.", "src/mobile/screens/collections/MCollLinksEditor.tsx": "Markup only; link rows and the change handler come through props from the collection sheets.", "src/mobile/screens/collections/MCollListPickerSheet.tsx": "Phone list picker sheet; the move or copy action comes through onPick from useCollections.", diff --git a/client/src/components/Collections/CollectionInviteCard.tsx b/client/src/components/Collections/CollectionInviteCard.tsx new file mode 100644 index 000000000..84eeee74c --- /dev/null +++ b/client/src/components/Collections/CollectionInviteCard.tsx @@ -0,0 +1,39 @@ +import React from 'react'; +import type { IncomingCollectionInvite } from '../../store/collectionStore'; +import type { TranslationFn } from '../../types'; + +interface CollectionInviteCardProps { + invite: IncomingCollectionInvite; + onAccept: (id: number) => void; + onDecline: (id: number) => void; + t: TranslationFn; +} + +/** + * One pending list invite with Accept / Decline. Lives in the lists rail, and in + * the page body where the rail is not on screen: no lists of one's own yet, or + * the invite notification's link to a list that is still an invite (#2645). + */ +export default function CollectionInviteCard({ + invite, + onAccept, + onDecline, + t, +}: CollectionInviteCardProps): React.ReactElement { + return ( +
+
{invite.name}
+
+ {t('collections.invites.from')} {invite.from.username} +
+
+ + +
+
+ ); +} diff --git a/client/src/components/Collections/ListsRail.tsx b/client/src/components/Collections/ListsRail.tsx index 286b6017c..26f44f55a 100644 --- a/client/src/components/Collections/ListsRail.tsx +++ b/client/src/components/Collections/ListsRail.tsx @@ -4,6 +4,7 @@ import type { Collection } from '@trek/shared' import type { TranslationFn } from '../../types' import { ALL_SAVED } from '../../store/collectionStore' import type { ActiveCollectionId, IncomingCollectionInvite } from '../../store/collectionStore' +import CollectionInviteCard from './CollectionInviteCard' interface ListsRailProps { ownedLists: Collection[] @@ -90,18 +91,7 @@ export default function ListsRail(props: ListsRailProps): React.ReactElement { {incomingInvites.length} {incomingInvites.map(inv => ( -
-
{inv.name}
-
{t('collections.invites.from')} {inv.from.username}
-
- - -
-
+ ))} )} diff --git a/client/src/mobile/screens/collections/MCollInvites.tsx b/client/src/mobile/screens/collections/MCollInvites.tsx new file mode 100644 index 000000000..232791cd6 --- /dev/null +++ b/client/src/mobile/screens/collections/MCollInvites.tsx @@ -0,0 +1,42 @@ +import { Check } from 'lucide-react'; +import type { IncomingCollectionInvite } from '../../../store/collectionStore'; +import type { TranslationFn } from '../../../types'; +import { CancelPill, Eyebrow, PrimaryPill } from './MCollSheetKit'; + +interface MCollInvitesProps { + invites: IncomingCollectionInvite[]; + onAccept: (id: number) => void; + onDecline: (id: number) => void; + t: TranslationFn; +} + +/** + * Pending list invites as cards above the lists, with Accept / Decline. The + * phone has no rail, so this is the one place an invite can be answered, and + * where the invite notification lands (#2645). + */ +export default function MCollInvites({ invites, onAccept, onDecline, t }: MCollInvitesProps) { + if (invites.length === 0) return null; + return ( +
+ {t('collections.invites.title').toUpperCase()} + {invites.map((inv) => ( +
+
{inv.name}
+
+ {t('collections.invites.from')} {inv.from.username} +
+
+ onAccept(inv.collection_id)}> + {t('collections.invites.accept')} + + onDecline(inv.collection_id)}>{t('collections.invites.decline')} +
+
+ ))} +
+ ); +} diff --git a/client/src/mobile/screens/collections/MCollections.tsx b/client/src/mobile/screens/collections/MCollections.tsx index 0d1c7c3ef..bb3d81ada 100644 --- a/client/src/mobile/screens/collections/MCollections.tsx +++ b/client/src/mobile/screens/collections/MCollections.tsx @@ -21,6 +21,7 @@ import MCollEditSheet from './MCollEditSheet' import MCollLabelsSheet from './MCollLabelsSheet' import MCollTripPickerSheet from './MCollTripPickerSheet' import MCollListPickerSheet from './MCollListPickerSheet' +import MCollInvites from './MCollInvites' import { STATUS_SPEC } from './collectionsMobileModel' import { CancelPill, PrimaryPill } from './MCollSheetKit' @@ -209,6 +210,8 @@ export default function MCollections() { )} + + {c.loading && c.collections.length === 0 ? (
diff --git a/client/src/pages/CollectionsPage.test.tsx b/client/src/pages/CollectionsPage.test.tsx index 09509527b..43d566680 100644 --- a/client/src/pages/CollectionsPage.test.tsx +++ b/client/src/pages/CollectionsPage.test.tsx @@ -78,6 +78,8 @@ function makeHook(overrides: Record = {}) { hasMappable: true, members: [], incomingInvites: [], + orderedInvites: [], + routeInvite: null, counts: { all: 1, idea: 0, want: 1, visited: 0 }, view: 'list', statusFilter: 'all', @@ -167,3 +169,47 @@ describe('CollectionsPage: a marker click shows the place (#2431)', () => { expect(setSelectedPlaceId).toHaveBeenLastCalledWith(null) }) }) + +// #2645: the rail is hidden below 1080px and an invitee with no lists of their +// own saw only "Start your first collection", so the invites the person has to +// answer also sit at the top of the body. +describe('CollectionsPage: invites in the body (#2645)', () => { + const invite = { collection_id: 14, name: 'Lisbon', from: { id: 4, username: 'Lena' } } + const other = { collection_id: 3, name: 'Rome', from: { id: 5, username: 'Tom' } } + const noLists = { + collections: [], ownedLists: [], activeCollection: null, activeId: null, + places: [], visiblePlaces: [], mappable: [], hasMappable: false, canShare: false, + } + + it('with no lists of their own, every invite can be answered from the body', () => { + const handleAcceptInvite = vi.fn() + const handleDeclineInvite = vi.fn() + mockUseCollections.mockReturnValue(makeHook({ + ...noLists, incomingInvites: [invite, other], orderedInvites: [invite, other], + handleAcceptInvite, handleDeclineInvite, + })) + render() + expect(screen.getByText('Lisbon')).toBeInTheDocument() + expect(screen.getByText('Rome')).toBeInTheDocument() + expect(screen.getByText('collections.empty.firstTitle')).toBeInTheDocument() + screen.getAllByRole('button', { name: 'collections.invites.accept' })[0].click() + expect(handleAcceptInvite).toHaveBeenCalledWith(14) + screen.getAllByRole('button', { name: 'collections.invites.decline' })[1].click() + expect(handleDeclineInvite).toHaveBeenCalledWith(3) + }) + + it('with lists, only the invite the route points at is shown in the body', () => { + mockUseCollections.mockReturnValue(makeHook({ + incomingInvites: [other, invite], orderedInvites: [invite, other], routeInvite: invite, + })) + render() + expect(screen.getByText('Lisbon')).toBeInTheDocument() + expect(screen.queryByText('Rome')).not.toBeInTheDocument() + }) + + it('with lists and no invite on the route, the body leaves invites to the rail', () => { + mockUseCollections.mockReturnValue(makeHook({ incomingInvites: [invite], orderedInvites: [invite] })) + render() + expect(screen.queryByRole('button', { name: 'collections.invites.accept' })).not.toBeInTheDocument() + }) +}) diff --git a/client/src/pages/CollectionsPage.tsx b/client/src/pages/CollectionsPage.tsx index 2aab1b5fe..e1ae016f3 100644 --- a/client/src/pages/CollectionsPage.tsx +++ b/client/src/pages/CollectionsPage.tsx @@ -3,6 +3,7 @@ import { List as ListIcon, Map as MapIcon, Search, Bookmark, CheckCheck, X, Tras import Navbar from '../components/Layout/Navbar' import ConfirmDialog from '../components/shared/ConfirmDialog' import ListsRail from '../components/Collections/ListsRail' +import CollectionInviteCard from '../components/Collections/CollectionInviteCard' import ListEditorModal from '../components/Collections/ListEditorModal' import CollectionHero from '../components/Collections/CollectionHero' import CollectionList from '../components/Collections/CollectionList' @@ -50,6 +51,10 @@ function CollectionsPageDesktop(): React.ReactElement { // Labels are per-collection, so only on a real (non "All saved") list. const isRealList = !c.isAllSaved && typeof c.activeId === 'number' const canManageLabels = isRealList && c.canEdit + // Invites also sit at the top of the body: with no lists of one's own the + // body is all there is (and the rail is hidden below 1080px), and the invite + // notification links to a list that is still an invite (#2645). + const bodyInvites = noLists ? c.orderedInvites : (c.routeInvite ? [c.routeInvite] : []) // Selecting a place from the list toggles it, so clicking the row again clears // it, as does the map background. A marker click always shows the place: a second @@ -206,6 +211,17 @@ function CollectionsPageDesktop(): React.ReactElement {
+ {bodyInvites.length > 0 && ( +
+
+ {t('collections.invites.title')} + {bodyInvites.length} +
+ {bodyInvites.map(inv => ( + + ))} +
+ )} {noLists ? (
diff --git a/client/src/pages/CollectionsPage.wiring.test.tsx b/client/src/pages/CollectionsPage.wiring.test.tsx index 80a7a6579..3bd463e16 100644 --- a/client/src/pages/CollectionsPage.wiring.test.tsx +++ b/client/src/pages/CollectionsPage.wiring.test.tsx @@ -211,6 +211,8 @@ function makeHook(overrides: Hook = {}): Hook { hasMappable: true, members: [], incomingInvites: [], + orderedInvites: [], + routeInvite: null, counts: { all: 1, idea: 0, want: 1, visited: 0 }, view: 'list', statusFilter: 'all', diff --git a/client/src/pages/collections/useCollections.test.tsx b/client/src/pages/collections/useCollections.test.tsx index 65087a00e..599a64742 100644 --- a/client/src/pages/collections/useCollections.test.tsx +++ b/client/src/pages/collections/useCollections.test.tsx @@ -1207,3 +1207,53 @@ describe('useCollections: GPX (#2301)', () => { expect(answer).toEqual(read) }) }) + +// #2645: the invite notification links to /collections/:id, a list the invitee +// cannot read until they accept. The page has to offer the invite there, and a +// link to a list that is gone (declined, cancelled, deleted) must not leave the +// page pointing at nothing. +describe('useCollections — route that is an invite (#2645)', () => { + const invite = { collection_id: 14, name: 'Lisbon', from: { id: 4, username: 'Lena' } } + + it('FE-PAGE-COLL-086: a route id that is a pending invite exposes it and does not open the list', async () => { + routeParams = { id: '14' } + store.incomingInvites = [invite] + const { result } = await setup() + expect(result.current.routeInvite).toEqual(invite) + expect(store.setActive).not.toHaveBeenCalledWith(14) + expect(navigate).not.toHaveBeenCalled() + }) + + it('FE-PAGE-COLL-087: the invite on the route is listed first', async () => { + routeParams = { id: '14' } + const other = { collection_id: 3, name: 'Rome', from: { id: 5, username: 'Tom' } } + store.incomingInvites = [other, invite] + const { result } = await setup() + expect(result.current.orderedInvites).toEqual([invite, other]) + }) + + it('FE-PAGE-COLL-088: a route id that is neither a list nor an invite bounces to /collections once the lists are in', async () => { + routeParams = { id: '14' } + store.collections = [collection({ id: 2 })] + await setup() + await waitFor(() => expect(navigate).toHaveBeenCalledWith('/collections', { replace: true })) + }) + + it('FE-PAGE-COLL-089: no bounce while the lists are still loading', async () => { + routeParams = { id: '14' } + let finish: () => void = () => {} + store.loadAll.mockImplementationOnce(() => new Promise(res => { finish = res })) + await setup() + expect(navigate).not.toHaveBeenCalled() + await act(async () => { finish() }) + await waitFor(() => expect(navigate).toHaveBeenCalledWith('/collections', { replace: true })) + }) + + it('FE-PAGE-COLL-090: a route id that is one of the lists stays put', async () => { + routeParams = { id: '14' } + store.collections = [collection({ id: 14 })] + await setup() + await act(async () => {}) + expect(navigate).not.toHaveBeenCalled() + }) +}) diff --git a/client/src/pages/collections/useCollections.ts b/client/src/pages/collections/useCollections.ts index 1857e0cd5..137b1a4b6 100644 --- a/client/src/pages/collections/useCollections.ts +++ b/client/src/pages/collections/useCollections.ts @@ -79,8 +79,11 @@ export function useCollections() { // place from the detail panel, or the select-mode set for a bulk copy. const [copyIds, setCopyIds] = useState(null) - // Initial load. - useEffect(() => { loadAll() }, []) + // Initial load. listsLoaded says the rail is fresh from this visit, which + // the route check below waits for before it calls a list id unknown. A load + // that fails leaves it off, so an offline page never drops the route. + const [listsLoaded, setListsLoaded] = useState(false) + useEffect(() => { loadAll().then(() => setListsLoaded(true), () => setListsLoaded(false)) }, []) // Central admin-defined categories, for assigning a category to a saved place. const [categories, setCategories] = useState([]) @@ -104,15 +107,41 @@ export function useCollections() { return () => cancelAnimationFrame(raf) }, [view, isWide]) + // The invite notification links to /collections/:id, a list the invitee + // cannot read before they accept (#2645). When the route is such an invite + // the page offers it instead of opening the list, and lists it first. + const routeInvite = useMemo(() => { + if (!routeId) return null + const id = Number(routeId) + return incomingInvites.find(inv => inv.collection_id === id) ?? null + }, [routeId, incomingInvites]) + const orderedInvites = useMemo( + () => (routeInvite ? [routeInvite, ...incomingInvites.filter(inv => inv !== routeInvite)] : incomingInvites), + [routeInvite, incomingInvites], + ) + // Keep the active list in sync with the URL (/collections/:id, or the - // "All saved" union at /collections). + // "All saved" union at /collections). A pending invite on the route shows + // the union behind its card. useEffect(() => { - const next: ActiveCollectionId = routeId ? Number(routeId) : ALL_SAVED + const next: ActiveCollectionId = routeId && !routeInvite ? Number(routeId) : ALL_SAVED if (Number.isNaN(next as number)) return if (next !== activeId) setActive(next) - // Only re-run when the route or the loaded list set changes. + // Only re-run when the route, the loaded list set or the invite on it changes. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [routeId, collections.length]) + }, [routeId, collections.length, routeInvite]) + + // A route id that is neither one of the lists nor a pending invite is a link + // to something gone: a declined or withdrawn invite, a deleted list, one the + // person was removed from. Drop it once the lists are in, rather than leave + // the URL on a list the page cannot show. + useEffect(() => { + if (!listsLoaded || loading || !routeId) return + const id = Number(routeId) + if (Number.isNaN(id)) return + if (collections.some(l => l.id === id) || incomingInvites.some(inv => inv.collection_id === id)) return + navigate('/collections', { replace: true }) + }, [listsLoaded, loading, routeId, collections, incomingInvites, navigate]) // Live sync via WebSocket. collections:deleted / :removed must bounce a member // off a list that was removed (or that they were kicked from) under them. @@ -490,7 +519,7 @@ export function useCollections() { collections, ownedLists, sharedLists, activeCollection, isAllSaved, isOwner, myRole, canEdit, canDelete, canShare, shareMemberCount, - activeId, places, visiblePlaces, mappable, hasMappable, members, incomingInvites, counts, + activeId, places, visiblePlaces, mappable, hasMappable, members, incomingInvites, routeInvite, orderedInvites, counts, view, statusFilter, categoryFilter, categoryOptions, ratingFilter, sortMode, search, selectedPlaceId, selectMode, selectedIds, labels, labelFilter, labelOptions, loading, placesLoading, diff --git a/client/src/store/collectionStore.ts b/client/src/store/collectionStore.ts index 265c558fa..3455be9df 100644 --- a/client/src/store/collectionStore.ts +++ b/client/src/store/collectionStore.ts @@ -180,8 +180,8 @@ export const useCollectionStore = create((set, get) => ({ } catch { // The list may have been left / removed / deleted out from under us (a WS // event or the URL sync can re-request an id we just lost access to). Clear - // it instead of leaving an uncaught 403/404 rejection; the route sync then - // bounces to /collections. + // it instead of leaving an uncaught 403/404 rejection; useCollections drops + // a route id that is no longer one of the lists. if (get().activeId === id) set({ activeId: null, places: [], members: [], labels: [] }) } finally { set({ placesLoading: false }) diff --git a/client/src/styles/collections.css b/client/src/styles/collections.css index 762bfd951..22bb9dc89 100644 --- a/client/src/styles/collections.css +++ b/client/src/styles/collections.css @@ -92,6 +92,9 @@ .trek-dash .col-invite-accept:hover { opacity: .9; } .trek-dash .col-invite-decline { border: 1px solid var(--line-2); color: var(--ink-2); } .trek-dash .col-invite-decline:hover { background: var(--surface); } +/* the same cards at the top of the page body (no lists yet, or the invite's own link) */ +.trek-dash .col-invites-main { max-width: 420px; margin: 0 auto 22px; } +.trek-dash .col-invites-main .col-invite { padding: 13px; } /* ----------------- hero ----------------- */ .trek-dash .col-hero { diff --git a/client/tests/unit/mobile/collections/MCollections.test.tsx b/client/tests/unit/mobile/collections/MCollections.test.tsx index 1f90cd9ac..b44bec202 100644 --- a/client/tests/unit/mobile/collections/MCollections.test.tsx +++ b/client/tests/unit/mobile/collections/MCollections.test.tsx @@ -1,4 +1,4 @@ -// FE-MOB-COLSCR-001 to FE-MOB-COLSCR-031 +// FE-MOB-COLSCR-001 to FE-MOB-COLSCR-033 import React from 'react' import { describe, it, expect, vi, beforeEach } from 'vitest' import { render, screen, fireEvent, waitFor } from '../../../helpers/render' @@ -159,6 +159,8 @@ function makeHook(over: Record = {}): Record { mappable: [place(10, 'Elbphilharmonie', { lat: 53.5, lng: 10 })], members: [], incomingInvites: [], + orderedInvites: [], + routeInvite: null, counts: { all: 1, idea: 1, want: 0, visited: 0 }, view: 'list', statusFilter: 'all', @@ -186,6 +188,7 @@ function makeHook(over: Record = {}): Record { copyIds: null, openCopyForSelectedPlace: vi.fn(), openCopyForSelection: vi.fn(), closeCopy: vi.fn(), handleCopyToTrip: vi.fn(), handleSelectList: vi.fn(), handleDeleteList: vi.fn(), handleStatusChange: vi.fn(), handleRatePlace: vi.fn(), handleDeleteSelected: vi.fn(), + handleAcceptInvite: vi.fn(), handleDeclineInvite: vi.fn(), allVisibleSelected: false, handleSelectAll: vi.fn(), listPickerMode: null, setListPickerMode: vi.fn(), handleMoveToList: vi.fn(), handleDuplicateToList: vi.fn(), ...over, @@ -658,3 +661,34 @@ describe('MCollections', () => { expect(screen.getByText('add-target:null::1')).toBeInTheDocument() }) }) + +// #2645: the phone had no way to answer a list invite, and the notification's +// link landed on "Start your first collection". +describe('MCollections: invites (#2645)', () => { + const invite = { collection_id: 14, name: 'Lisbon', from: { id: 4, username: 'Lena' } } + + it('FE-MOB-COLSCR-032: with no lists of their own, the invite can be accepted or declined', () => { + mocks.coll = makeHook({ + collections: [], ownedLists: [], activeCollection: null, activeId: null, canShare: false, + incomingInvites: [invite], orderedInvites: [invite], routeInvite: invite, + }) + render() + + expect(screen.getByText('Lisbon')).toBeInTheDocument() + expect(screen.getByText(/Lena/)).toBeInTheDocument() + fireEvent.click(screen.getByRole('button', { name: /collections.invites.accept/ })) + expect(fn('handleAcceptInvite')).toHaveBeenCalledWith(14) + fireEvent.click(screen.getByRole('button', { name: 'collections.invites.decline' })) + expect(fn('handleDeclineInvite')).toHaveBeenCalledWith(14) + }) + + it('FE-MOB-COLSCR-033: invites sit above the lists too, in the order the hook gives', () => { + const other = { collection_id: 3, name: 'Rome', from: { id: 5, username: 'Tom' } } + mocks.coll = makeHook({ incomingInvites: [other, invite], orderedInvites: [invite, other] }) + render() + + const names = screen.getAllByText(/^(Lisbon|Rome)$/).map(el => el.textContent) + expect(names).toEqual(['Lisbon', 'Rome']) + expect(screen.getByPlaceholderText('collections.search')).toBeInTheDocument() + }) +})