diff --git a/.github/workflows/enforce-target-branch.yml b/.github/workflows/enforce-target-branch.yml index 36f127d81..2b41b2ec4 100644 --- a/.github/workflows/enforce-target-branch.yml +++ b/.github/workflows/enforce-target-branch.yml @@ -68,6 +68,38 @@ jobs: return; } + // Dependabot cannot be pointed at dev. `target-branch` in dependabot.yml + // applies to version updates only; security updates always open against + // the default branch, and security updates are all this repository gets + // (there is no dependabot.yml). Labelling them meant close-stale-wrong-branch + // shut them 24 hours later, which is how 13 of them died. Retarget instead: + // a bot cannot click "Edit" the way the comment below asks a human to. + if (context.payload.pull_request.user.login === 'dependabot[bot]') { + try { + await github.rest.pulls.update({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber, + base: 'dev', + }); + if (labels.includes('wrong-base-branch')) { + await github.rest.issues.removeLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + name: 'wrong-base-branch', + }); + } + console.log('Retargeted the Dependabot PR onto dev.'); + return; + } catch (err) { + // Retargeting can fail when the branch was cut from main and dev has + // moved on. Fall through to the normal path so a human sees it rather + // than the check passing on a PR that is still aimed at main. + console.log(`Could not retarget the Dependabot PR: ${err.message}`); + } + } + // Base is main — check if this user is a maintainer let permission = 'none'; try { diff --git a/Dockerfile b/Dockerfile index b6ce171c2..cc20b9f32 100644 --- a/Dockerfile +++ b/Dockerfile @@ -114,6 +114,7 @@ RUN mkdir -p /app/data/logs /app/uploads/files /app/uploads/covers /app/uploads/ chown -h node:node /app/server/uploads /app/server/data ENV NODE_ENV=production +ENV NODE_USE_ENV_PROXY=1 ENV PORT=3000 ARG APP_VERSION=dev ENV APP_VERSION=${APP_VERSION} diff --git a/client/e2e/urlbar-scroll-1809.spec.ts b/client/e2e/urlbar-scroll-1809.spec.ts new file mode 100644 index 000000000..917748f76 --- /dev/null +++ b/client/e2e/urlbar-scroll-1809.spec.ts @@ -0,0 +1,105 @@ +import { test, expect, devices } from '@playwright/test' +import { dismissSystemNotices } from './helpers' + +// Phone regression guard for #1809: the dashboard must scroll the DOCUMENT. +// +// iOS Safari minimises its address bar only in reaction to the root scroller +// moving. V4 blocked that twice over: html was globally overflow:hidden and the +// mobile shell added a scroll container of its own, so the gesture never +// reached the viewport and the bar kept ~1/6th of the screen forever. The bar +// itself is not observable from page context, but the invariant it hangs off is: +// document.scrollingElement has to move. +// +// Needs WebKit (`npx playwright install webkit`). playwright.config.ts has no +// webkit project, the engine comes from this file-level device (defaultBrowserType +// 'webkit'), same as e2e/ipad-scroll-1432.spec.ts. WebKit is the point here, not +// a nicety: viewport propagation and the toolbar behaviour are Safari's. +// +// The file sorts last in the e2e run, so the trips it seeds cannot show up in +// another spec's dashboard assertions. +test.use({ ...devices['iPhone 14'] }) + +function readScroll() { + const el = document.scrollingElement as HTMLElement + return { + top: el.scrollTop, + scrollHeight: el.scrollHeight, + viewport: window.innerHeight, + htmlOverflow: getComputedStyle(document.documentElement).overflowY, + } +} + +test('#1809 iPhone: flow screens scroll the document, full-screen ones do not', async ({ page }) => { + await page.goto('/dashboard') + await dismissSystemNotices(page) + + // The context has to be the one from the report: phone width, coarse pointer. + // If either is wrong, nothing below proves anything. + const env = await page.evaluate(() => ({ + width: window.innerWidth, + coarse: window.matchMedia('(pointer: coarse)').matches, + })) + expect(env.width, 'iPhone sits below the 768px phone breakpoint').toBeLessThan(768) + expect(env.coarse, 'iPhone reports a coarse primary pointer').toBe(true) + + // Enough trips for the dashboard to overflow a phone screen. + const stamp = Date.now() + for (let i = 1; i <= 20; i++) { + const res = await page.request.post('/api/trips', { + data: { title: `URL bar ${stamp} ${i}`, start_date: '2099-06-01', end_date: '2099-06-10' }, + }) + expect(res.ok(), `seed trip ${i}`).toBeTruthy() + } + await page.reload() + await dismissSystemNotices(page) + await expect(page.getByText(`URL bar ${stamp} 1`).first()).toBeVisible({ timeout: 20_000 }) + + // 1. The root scroller is unlocked and the page is longer than the viewport. + const before = await page.evaluate(readScroll) + expect(before.htmlOverflow, 'html must not lock the viewport scroller').not.toBe('hidden') + expect(before.scrollHeight, 'dashboard is longer than one viewport').toBeGreaterThan(before.viewport) + expect(before.top).toBe(0) + + // 2. Scrolling moves the document, the signal Safari retracts its bar for. + await page.evaluate(() => window.scrollBy(0, 400)) + const scrolled = await page.evaluate(readScroll) + expect(scrolled.top, 'the document itself scrolled').toBeGreaterThan(0) + + // 3. The brand tile scrolls the page back up (it used to walk up to the shell + // container, which no longer exists). + await page.getByRole('button', { name: 'TREK' }).click() + await expect.poll(() => page.evaluate(() => document.scrollingElement!.scrollTop)).toBe(0) + + // 4. The shared body scroll lock: with the document as the scroller, an open + // sheet must freeze the page behind it and give the position back. + await page.evaluate(() => window.scrollBy(0, 300)) + const locked = await page.evaluate(readScroll) + expect(locked.top).toBeGreaterThan(0) + + await page.getByRole('button', { name: 'New Trip' }).click() + await expect(page.getByRole('dialog')).toBeVisible() + await page.evaluate(() => window.scrollBy(0, 300)) + expect(await page.evaluate(() => document.scrollingElement!.scrollTop)).toBe(locked.top) + + await page.keyboard.press('Escape') + await expect(page.getByRole('dialog')).toHaveCount(0) + expect(await page.evaluate(() => document.scrollingElement!.scrollTop)).toBe(locked.top) + await page.evaluate(() => window.scrollBy(0, 200)) + expect(await page.evaluate(() => document.scrollingElement!.scrollTop)).toBeGreaterThan(locked.top) + + // 5. Counter-probe: the map screens keep their own full-viewport layout. They + // deliberately do NOT scroll the document (and so keep the address bar), and + // the map must not collapse to zero height now that the shell has none. + await page.goto('/atlas') + await expect(page.locator('.leaflet-container')).toBeVisible({ timeout: 20_000 }) + const atlas = await page.evaluate(() => { + const map = document.querySelector('.leaflet-container') as HTMLElement + window.scrollBy(0, 400) + return { + top: document.scrollingElement!.scrollTop, + mapHeight: map.getBoundingClientRect().height, + } + }) + expect(atlas.top, 'the atlas does not move the document').toBe(0) + expect(atlas.mapHeight, 'the atlas map keeps a real height').toBeGreaterThan(200) +}) diff --git a/client/src/api/collections.ts b/client/src/api/collections.ts index ab6b15dec..be4b4c085 100644 --- a/client/src/api/collections.ts +++ b/client/src/api/collections.ts @@ -17,6 +17,7 @@ import type { CollectionInviteActionRequest, CollectionInviteCancelRequest, CollectionStatus, + CollectionSetStatusManyResponse, Collection, CollectionPlace, CollectionLabel, @@ -77,6 +78,10 @@ export const collectionsApi = { postMultipart(`${base}/places/${pid}/image`, formData), setStatus: (pid: number, status: CollectionStatus): Promise => ax.post(`${base}/places/${pid}/status`, { status }).then((r: AxiosResponse) => r.data), + setStatusMany: (ids: number[], status: CollectionStatus): Promise => + ax.post(`${base}/places/status-many`, { ids, status }).then((r: AxiosResponse) => r.data), + setStatusFromTrip: (tripId: number, placeIds: number[], status: CollectionStatus): Promise => + ax.post(`${base}/places/status-from-trip`, { trip_id: tripId, place_ids: placeIds, status }).then((r: AxiosResponse) => r.data), ratePlace: (pid: number, rating: number | null): Promise => rating === null ? ax.delete(`${base}/places/${pid}/rating`).then((r: AxiosResponse) => r.data) diff --git a/client/src/components/Budget/CostsPanel.helpers.test.ts b/client/src/components/Budget/CostsPanel.helpers.test.ts index ec2d15cb6..31c97c102 100644 --- a/client/src/components/Budget/CostsPanel.helpers.test.ts +++ b/client/src/components/Budget/CostsPanel.helpers.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest' -import { hasTicketSplit, payerSum, payersBalanced, readTicketItems, readUserNote, rebalancePayers, splitCents, writeTicketItems } from './CostsPanel.helpers' +import { calculateTicketShares, hasTicketSplit, payerSum, payersBalanced, readTicketItems, readUserNote, rebalancePayers, splitCents, writeTicketItems, type TicketItem } from './CostsPanel.helpers' describe('splitCents', () => { it('splits evenly when it divides cleanly', () => { @@ -132,3 +132,47 @@ describe('readUserNote', () => { expect(readUserNote(null)).toBe('') }) }) + +// ── Receipt splits have to reconcile (#1382) ───────────────────────────────── + +describe('calculateTicketShares', () => { + const line = (price: string, parts: number[]): TicketItem => + ({ id: price + parts.join(), name: 'Line', price, participants: new Set(parts) }) + const shareSum = (shares: Record) => + Object.values(shares).reduce((a, v) => a + Math.round(v * 100), 0) + + it('splits each line among the people on it', () => { + const { shares, total } = calculateTicketShares([line('10', [1, 2]), line('6', [2])]) + expect(shares).toEqual({ 1: 5, 2: 11 }) + expect(total).toBe(16) + }) + + it('spreads a line nobody was ticked for across everyone on the receipt', () => { + // The service charge used to count toward the total and toward nobody's share, + // so the expense carried a permanent difference no settle-up could clear. + const { shares, total } = calculateTicketShares([ + line('10', [1, 2]), + line('5', []), + line('x', [2]), + ]) + expect(shares).toEqual({ 1: 7.5, 2: 7.5 }) + expect(total).toBe(15) + expect(shareSum(shares)).toBe(Math.round(total * 100)) + }) + + it('hands out the remainder cents so the shares add back up to the total', () => { + const { shares, total } = calculateTicketShares([line('10', [1, 2, 3]), line('0.01', [])]) + expect(shareSum(shares)).toBe(Math.round(total * 100)) + expect(total).toBe(10.01) + }) + + it('splits nothing when the receipt has no participants anywhere', () => { + const { shares, total } = calculateTicketShares([line('10', []), line('5', [])]) + expect(shares).toEqual({}) + expect(total).toBe(15) + }) + + it('is empty for an empty receipt', () => { + expect(calculateTicketShares([])).toEqual({ shares: {}, total: 0 }) + }) +}) diff --git a/client/src/components/Budget/CostsPanel.helpers.ts b/client/src/components/Budget/CostsPanel.helpers.ts index 74e6d631e..8bc566144 100644 --- a/client/src/components/Budget/CostsPanel.helpers.ts +++ b/client/src/components/Budget/CostsPanel.helpers.ts @@ -135,24 +135,36 @@ export function readUserNote(item: { note?: string | null } | null | undefined): return note && !note.startsWith('TICKETJSON:') ? note : '' } -/** Per-person totals for a receipt split line by line, plus the receipt total. */ +/** + * Per-person totals for a receipt split line by line, plus the receipt total. + * + * Every cent on the receipt lands on somebody: a line nobody was ticked for is + * carried by everyone who appears on the receipt, so the shares add back up to + * the total. They used to count toward the total only, which left the expense + * with member debits that could never cancel the payer's credit — a permanent + * difference the settle-up view showed as debt but had no flow to clear (#1382). + * A receipt with no participants at all splits nothing; it is not saveable + * anyway, since every line needs someone on it. + */ export function calculateTicketShares(items: TicketItem[]): { shares: Record; total: number } { const shares: Record = {} let totalCents = 0 + const everyone = [...new Set(items.flatMap(i => [...i.participants]))].sort((a, b) => a - b) + for (const item of items) { const priceNum = parseFloat(item.price) || 0 const priceCents = Math.round(priceNum * 100) totalCents += priceCents - const partIds = [...item.participants] - const n = partIds.length + const sortedPartIds = item.participants.size > 0 + ? [...item.participants].sort((a, b) => a - b) + : everyone + const n = sortedPartIds.length if (n === 0) continue const baseCents = Math.floor(priceCents / n) - const remainder = priceCents % n - - const sortedPartIds = [...partIds].sort((a, b) => a - b) + const remainder = priceCents - baseCents * n for (let i = 0; i < n; i++) { const id = sortedPartIds[i] diff --git a/client/src/components/Budget/CostsPanel.test.tsx b/client/src/components/Budget/CostsPanel.test.tsx index 6dd9ec25c..bba2289e5 100644 --- a/client/src/components/Budget/CostsPanel.test.tsx +++ b/client/src/components/Budget/CostsPanel.test.tsx @@ -1393,6 +1393,26 @@ describe('CostsPanel — expense modal', () => { expect(posted).toMatchObject({ name: 'Hotel Astoria', category: 'accommodation', total_price: 240, reservation_id: 12 }) expect(onSaved).toHaveBeenCalled() }) + + it('FE-W5COSTS-038b: a prefill from a place links the expense to that place (#1298)', async () => { + const user = userEvent.setup() + let posted: Record | null = null + server.use(http.post('/api/trips/1/budget', async ({ request }) => { + posted = await request.json() as Record + return HttpResponse.json({ item: dinner() }) + })) + render( + {}} onSaved={vi.fn()} /> + ) + + await user.click(screen.getByRole('button', { name: 'Add expense' })) + + await waitFor(() => expect(posted).toBeTruthy()); + expect(posted).toMatchObject({ name: 'Louvre', category: 'activities', total_price: 34, place_id: 7 }) + expect(posted).not.toHaveProperty('reservation_id') + }) }) describe('CostsPanel — remaining paths', () => { @@ -1742,13 +1762,15 @@ describe('CostsPanel — split maths', () => { expect(splitEqualShares(10, [{ user_id: 1 }, { user_id: 2 }, { user_id: 3 }], 1)).toEqual({ 1: 3.33, 2: 3.34, 3: 3.33 }) }) - it('FE-W5COSTS-041: ticket items nobody is assigned to still count toward the total', () => { + it('FE-W5COSTS-041: a receipt line nobody is assigned to is carried by everyone on the receipt', () => { const items: TicketItem[] = [ { id: 'a', name: 'Apples', price: '10', participants: new Set([1, 2]) }, { id: 'b', name: 'Service', price: '5', participants: new Set() }, { id: 'c', name: 'Cake', price: 'x', participants: new Set([2]) }, ] - expect(calculateTicketShares(items)).toEqual({ shares: { 1: 5, 2: 5 }, total: 15 }) + // The service line used to count toward the total without landing on anyone, + // so the shares stayed a permanent 5.00 short of it (#1382). + expect(calculateTicketShares(items)).toEqual({ shares: { 1: 7.5, 2: 7.5 }, total: 15 }) }) }) diff --git a/client/src/components/Budget/CostsPanel.tsx b/client/src/components/Budget/CostsPanel.tsx index 4c10528cd..4bca99248 100644 --- a/client/src/components/Budget/CostsPanel.tsx +++ b/client/src/components/Budget/CostsPanel.tsx @@ -1000,6 +1000,8 @@ export interface ExpensePrefill { category?: string amount?: number reservationId?: number + /** Set when the expense is being created from a place (#1298). */ + placeId?: number } export function ExpenseModal({ tripId, base, people, me, editing, prefill, onClose, onSaved }: { @@ -1232,7 +1234,13 @@ export function ExpenseModal({ tripId, base, people, me, editing, prefill, onClo .map(id => ({ user_id: id, amount: parseFloat(payerAmounts[id]) || 0 })) .filter(p => p.amount > 0) : payerId > 0 ? [{ user_id: payerId, amount: totalNum }] : [] - const memberList = [...participants].map(id => ({ + // A receipt line can name somebody who is not ticked as a participant. Sending + // only the ticked set would drop their share, leaving the member sum short of + // total_price and handing the settlement a difference it can never clear (#1382). + const memberIds = splitMode === 'ticket' + ? [...new Set([...participants, ...Object.keys(ticketInfo.shares).map(Number)])].sort((a, b) => a - b) + : [...participants] + const memberList = memberIds.map(id => ({ user_id: id, amount: splitMode === 'custom' ? (parseFloat(customAmounts[id]) || 0) @@ -1246,12 +1254,13 @@ export function ExpenseModal({ tripId, base, people, me, editing, prefill, onClo currency, payers: payerList, members: memberList, - member_ids: [...participants], + member_ids: memberIds, expense_date: day || null, total_price: totalNum, note: note.trim() || null, ticket_json: splitMode === 'ticket' ? writeTicketItems(ticketItems) : null, ...(!editing && prefill?.reservationId ? { reservation_id: prefill.reservationId } : {}), + ...(!editing && prefill?.placeId ? { place_id: prefill.placeId } : {}), } try { if (editing) await updateBudgetItem(tripId, editing.id, data) diff --git a/client/src/components/Collab/CollabChatMessages.tsx b/client/src/components/Collab/CollabChatMessages.tsx index b335c0bef..9427bee4c 100644 --- a/client/src/components/Collab/CollabChatMessages.tsx +++ b/client/src/components/Collab/CollabChatMessages.tsx @@ -13,7 +13,10 @@ export function ChatMessages(props: any) { <> {/* Messages */} {messages.length === 0 ? ( - + // flex-1, like the message list it stands in for: without it the empty + // state was only as tall as its own content and the composer rode up to + // sit right under the mascot, with the rest of the panel left blank. + ) : (
{ await waitFor(() => expect(addToast).toHaveBeenCalledWith('Places API disabled', 'error', undefined)) }) - it('FE-COMP-ADDPLACECOL-009: a search response without places yields no dropdown', async () => { + it('FE-COMP-ADDPLACECOL-009: a search that finds nothing says so instead of staying silent', async () => { server.use(http.post('/api/maps/search', () => HttpResponse.json({ source: 'nominatim' }))) setup() fireEvent.change(screen.getByPlaceholderText('Search for a place…'), { target: { value: 'kissa' } }) fireEvent.click(screen.getByRole('button', { name: /Search/ })) - await waitFor(() => expect(screen.getByRole('button', { name: /Search/ })).not.toBeDisabled()) - expect(screen.queryByRole('button', { name: 'Close' })).not.toBeInTheDocument() + + expect(await screen.findByText('No places found')).toBeInTheDocument() + // Typing the next query clears it again. + fireEvent.change(screen.getByPlaceholderText('Search for a place…'), { target: { value: 'kissaten' } }) + expect(screen.queryByText('No places found')).not.toBeInTheDocument() + }) + + it('FE-COMP-ADDPLACECOL-009b: the empty search panel can be dismissed', async () => { + server.use(http.post('/api/maps/search', () => HttpResponse.json({ places: [], source: 'nominatim' }))) + setup() + fireEvent.change(screen.getByPlaceholderText('Search for a place…'), { target: { value: 'kissa' } }) + fireEvent.click(screen.getByRole('button', { name: /Search/ })) + await screen.findByText('No places found') + + fireEvent.click(screen.getByRole('button', { name: 'Close' })) + expect(screen.queryByText('No places found')).not.toBeInTheDocument() }) it('FE-COMP-ADDPLACECOL-010: saving posts the manually typed place with nulls for what is empty', async () => { @@ -360,4 +374,49 @@ describe('AddPlaceToCollectionModal', () => { expect(props.onClose).toHaveBeenCalledTimes(1) expect(savedBodies).toHaveLength(0) }) + + it('FE-COMP-ADDPLACECOL-025: the caret goes back to the search field after an add', async () => { + setup() + const nameField = screen.getByPlaceholderText('Name') + typeName('Manual spot') + // Where the focus sits in the browser once the add button disables itself. + nameField.focus() + fireEvent.click(screen.getByRole('button', { name: /Add$/ })) + + await waitFor(() => expect(nameField).toHaveValue('')) + expect(screen.getByPlaceholderText('Search for a place…')).toHaveFocus() + }) + + it('FE-COMP-ADDPLACECOL-026: the search still runs after a place was added', async () => { + mockSearch([mapsResult]) + setup() + typeName('Manual spot') + fireEvent.click(screen.getByRole('button', { name: /Add$/ })) + await waitFor(() => expect(screen.getByPlaceholderText('Name')).toHaveValue('')) + + const input = screen.getByPlaceholderText('Search for a place…') + expect(input).toHaveValue('') + fireEvent.change(input, { target: { value: 'kissa' } }) + expect(screen.getByRole('button', { name: /Search/ })).not.toBeDisabled() + fireEvent.keyDown(input, { key: 'Enter' }) + expect(await screen.findByText('Kissa Sakaiki')).toBeInTheDocument() + }) + + it('FE-COMP-ADDPLACECOL-027: the search row is pinned to the top of the scrolling body', () => { + setup() + // The dialog stays open after an add, so the row has to survive a scrolled form. + expect(screen.getByPlaceholderText('Search for a place…').closest('.sticky')).not.toBeNull() + }) + + it('FE-COMP-ADDPLACECOL-028: a failed save leaves the caret where the user had it', async () => { + server.use(http.post('/api/addons/collections/places', () => HttpResponse.json({ error: 'List is full' }, { status: 400 }))) + setup() + const nameField = screen.getByPlaceholderText('Name') + typeName('Manual spot') + nameField.focus() + fireEvent.click(screen.getByRole('button', { name: /Add$/ })) + + await waitFor(() => expect(addToast).toHaveBeenCalledWith('List is full', 'error', undefined)) + expect(nameField).toHaveFocus() + }) }) diff --git a/client/src/components/Collections/AddPlaceToCollectionModal.tsx b/client/src/components/Collections/AddPlaceToCollectionModal.tsx index 0e53dc78c..da2117dc8 100644 --- a/client/src/components/Collections/AddPlaceToCollectionModal.tsx +++ b/client/src/components/Collections/AddPlaceToCollectionModal.tsx @@ -42,6 +42,9 @@ export default function AddPlaceToCollectionModal({ isOpen, collectionId, collec const [query, setQuery] = useState('') const [results, setResults] = useState([]) const [searching, setSearching] = useState(false) + // A search that came back empty used to render nothing at all, which reads as + // "the dialog is dead". Say so instead (#1921). + const [noResults, setNoResults] = useState(false) // The picked location (address/coords/ids) plus the editable fields. const [picked, setPicked] = useState(null) const [name, setName] = useState('') @@ -56,16 +59,20 @@ export default function AddPlaceToCollectionModal({ isOpen, collectionId, collec const [status, setStatus] = useState('idea') const [saving, setSaving] = useState(false) const descRef = useRef(null) + const searchRef = useRef(null) - const reset = () => { setQuery(''); setResults([]); setPicked(null); setName(''); setAddress(''); setLat(''); setLng(''); setCategoryId(null); setDescription(''); setLinks([]); setStatus('idea') } + const reset = () => { setQuery(''); setResults([]); setNoResults(false); setPicked(null); setName(''); setAddress(''); setLat(''); setLng(''); setCategoryId(null); setDescription(''); setLinks([]); setStatus('idea') } useEffect(() => { if (!isOpen) reset() }, [isOpen]) const search = async () => { if (!query.trim()) return setSearching(true) + setNoResults(false) try { const res = await mapsApi.search(query, language) - setResults((res.places as MapsPlace[]) || []) + const places = (res.places as MapsPlace[]) || [] + setResults(places) + setNoResults(places.length === 0) } catch (err) { toast.error(getApiErrorMessage(err, t('places.mapsSearchError'))) } finally { @@ -73,6 +80,8 @@ export default function AddPlaceToCollectionModal({ isOpen, collectionId, collec } } + const dismissResults = () => { setResults([]); setNoResults(false) } + const pick = (r: MapsPlace) => { setPicked(r) setName(str(r.name) ?? '') @@ -80,7 +89,7 @@ export default function AddPlaceToCollectionModal({ isOpen, collectionId, collec const la = num(r.lat); const lo = num(r.lng) setLat(la != null ? String(la) : '') setLng(lo != null ? String(lo) : '') - setResults([]); setQuery(str(r.name) ?? query) + setResults([]); setNoResults(false); setQuery(str(r.name) ?? query) } const setLink = (i: number, patch: Partial) => setLinks(links.map((l, idx) => (idx === i ? { ...l, ...patch } : l))) @@ -112,6 +121,11 @@ export default function AddPlaceToCollectionModal({ isOpen, collectionId, collec if (res.duplicate) toast.info(t('collections.duplicateWarning')) else { toast.success(t('collections.addedToList', { name: collectionName })); onAdded() } reset() + // The dialog stays open for the next place, so hand the caret back to the + // search field. The add button the user just clicked goes disabled with the + // cleared name, which drops the focus to and leaves the dialog dead + // to the keyboard (#1921). + searchRef.current?.focus() } catch (err) { toast.error(getApiErrorMessage(err, t('common.error'))) } finally { @@ -142,42 +156,51 @@ export default function AddPlaceToCollectionModal({ isOpen, collectionId, collec } >
- {/* Search — picking a result fills the location below */} -
-
-
- - setQuery(e.target.value)} - onKeyDown={e => { if (e.key === 'Enter') { e.preventDefault(); search() } }} - placeholder={t('collections.addPlaceSearch')} - className="w-full pl-9 pr-3 py-2 rounded-lg border border-edge bg-surface-input text-content text-[14px] outline-none focus:border-accent" - /> -
- -
- {results.length > 0 && ( -
-
- {t('common.search')} - + {/* Search — picking a result fills the location below. Pinned to the top of + the scrolling dialog body: the dialog stays open after an add, and the + search row used to sit above the viewport once the form was scrolled, + so it looked like the button had vanished (#1921). The negative margins + let its background cover the body padding while it is stuck. */} +
+
+
+
+ + { setQuery(e.target.value); setNoResults(false) }} + onKeyDown={e => { if (e.key === 'Enter') { e.preventDefault(); search() } }} + placeholder={t('collections.addPlaceSearch')} + className="w-full pl-9 pr-3 py-2 rounded-lg border border-edge bg-surface-input text-content text-[14px] outline-none focus:border-accent" + />
- {results.map((r, i) => ( - - ))} +
- )} + {(results.length > 0 || noResults) && ( +
+
+ {t('common.search')} + +
+ {noResults ? ( +
{t('planner.noPlacesFound')}
+ ) : results.map((r, i) => ( + + ))} +
+ )} +
{/* Name */} diff --git a/client/src/components/Collections/CollectionPlaceDetail.test.tsx b/client/src/components/Collections/CollectionPlaceDetail.test.tsx index c57a2f933..a4ea825f8 100644 --- a/client/src/components/Collections/CollectionPlaceDetail.test.tsx +++ b/client/src/components/Collections/CollectionPlaceDetail.test.tsx @@ -1,4 +1,4 @@ -// FE-COMP-COLDETAIL-001 to FE-COMP-COLDETAIL-035 +// FE-COMP-COLDETAIL-001 to FE-COMP-COLDETAIL-043 import React from 'react'; import { render, screen, fireEvent, waitFor, within } from '../../../tests/helpers/render'; import userEvent from '@testing-library/user-event'; @@ -335,6 +335,8 @@ describe('CollectionPlaceDetail', () => { links: [{ label: undefined, url: 'https://x.com' }], category_id: 2, label_ids: [1, 2], + // Untouched in this case, but the address rides along on every save (#1870). + address: 'Somewhere', lat: 52.5, lng: 13.4, }); @@ -547,3 +549,56 @@ describe('CollectionPlaceDetail — open in maps', () => { expect(screen.queryByRole('button', { name: /navigation|google maps/i })).toBeNull(); }); }); + +// ── Editable address (#1870) ───────────────────────────────────────────────── + +describe('CollectionPlaceDetail: editable address', () => { + it('FE-COMP-COLDETAIL-040: edit mode offers an address field seeded from the place', async () => { + const user = userEvent.setup(); + renderDetail({ canEdit: true }); + await user.click(await screen.findByRole('button', { name: 'Edit' })); + + expect(screen.getByText('Address')).toBeInTheDocument(); + expect(screen.getByPlaceholderText('Street, City, Country')).toHaveValue('Somewhere'); + }); + + it('FE-COMP-COLDETAIL-041: a corrected address is trimmed into the patch', async () => { + const user = userEvent.setup(); + const props = renderDetail({ canEdit: true }); + await user.click(await screen.findByRole('button', { name: 'Edit' })); + + const addressInput = screen.getByPlaceholderText('Street, City, Country'); + await user.clear(addressInput); + await user.type(addressInput, ' Via Nuova 1 '); + await user.click(screen.getByRole('button', { name: /Save/ })); + + await waitFor(() => expect(props.onSave).toHaveBeenCalled()); + expect(props.onSave).toHaveBeenCalledWith(expect.objectContaining({ address: 'Via Nuova 1' })); + }); + + it('FE-COMP-COLDETAIL-042: an emptied address is sent as null', async () => { + const user = userEvent.setup(); + const props = renderDetail({ canEdit: true }); + await user.click(await screen.findByRole('button', { name: 'Edit' })); + + await user.clear(screen.getByPlaceholderText('Street, City, Country')); + await user.click(screen.getByRole('button', { name: /Save/ })); + + await waitFor(() => expect(props.onSave).toHaveBeenCalled()); + expect(props.onSave).toHaveBeenCalledWith(expect.objectContaining({ address: null })); + }); + + it('FE-COMP-COLDETAIL-043: Cancel restores the stored address', async () => { + const user = userEvent.setup(); + const props = renderDetail({ canEdit: true }); + await user.click(await screen.findByRole('button', { name: 'Edit' })); + + await user.type(screen.getByPlaceholderText('Street, City, Country'), ' 2'); + await user.click(screen.getByRole('button', { name: 'Cancel' })); + expect(props.onSave).not.toHaveBeenCalled(); + expect(screen.getByText('Somewhere')).toBeInTheDocument(); + + await user.click(screen.getByRole('button', { name: 'Edit' })); + expect(screen.getByPlaceholderText('Street, City, Country')).toHaveValue('Somewhere'); + }); +}); diff --git a/client/src/components/Collections/CollectionPlaceDetail.tsx b/client/src/components/Collections/CollectionPlaceDetail.tsx index 2684b5cc0..7a3166b50 100644 --- a/client/src/components/Collections/CollectionPlaceDetail.tsx +++ b/client/src/components/Collections/CollectionPlaceDetail.tsx @@ -35,7 +35,7 @@ interface CollectionPlaceDetailProps { anchorRect?: { left: number; width: number } | null onClose: () => void onSetStatus: (status: CollectionStatus) => void - onSave: (patch: { name?: string; description?: string | null; links?: CollectionLink[]; category_id?: number | null; label_ids?: number[]; image_url?: string | null; lat?: number | null; lng?: number | null }) => Promise + onSave: (patch: { name?: string; description?: string | null; links?: CollectionLink[]; category_id?: number | null; label_ids?: number[]; image_url?: string | null; lat?: number | null; lng?: number | null; address?: string | null }) => Promise /** Upload a custom cover image (#1136); enables the cover change/remove controls. */ onUploadImage?: (file: File) => Promise onCopyToTrip: () => void @@ -83,6 +83,7 @@ export default function CollectionPlaceDetail({ const [description, setDescription] = useState(place.description ?? '') const [links, setLinks] = useState(place.links ?? []) const [labelIds, setLabelIds] = useState(place.label_ids ?? []) + const [address, setAddress] = useState(place.address ?? '') const [lat, setLat] = useState(place.lat != null ? String(place.lat) : '') const [lng, setLng] = useState(place.lng != null ? String(place.lng) : '') const [saving, setSaving] = useState(false) @@ -99,6 +100,7 @@ export default function CollectionPlaceDetail({ setDescription(place.description ?? '') setLinks(place.links ?? []) setLabelIds(place.label_ids ?? []) + setAddress(place.address ?? '') setLat(place.lat != null ? String(place.lat) : '') setLng(place.lng != null ? String(place.lng) : '') // eslint-disable-next-line react-hooks/exhaustive-deps @@ -147,7 +149,7 @@ export default function CollectionPlaceDetail({ const setLink = (i: number, patch: Partial) => setLinks(links.map((l, idx) => (idx === i ? { ...l, ...patch } : l))) const toggleLabel = (id: number) => setLabelIds(labelIds.includes(id) ? labelIds.filter(x => x !== id) : [...labelIds, id]) - const resetForm = () => { setEditing(false); setName(place.name); setCategoryId(place.category_id ?? null); setDescription(place.description ?? ''); setLinks(place.links ?? []); setLabelIds(place.label_ids ?? []); setLat(place.lat != null ? String(place.lat) : ''); setLng(place.lng != null ? String(place.lng) : '') } + const resetForm = () => { setEditing(false); setName(place.name); setCategoryId(place.category_id ?? null); setDescription(place.description ?? ''); setLinks(place.links ?? []); setLabelIds(place.label_ids ?? []); setAddress(place.address ?? ''); setLat(place.lat != null ? String(place.lat) : ''); setLng(place.lng != null ? String(place.lng) : '') } const coordPaste = (e: React.ClipboardEvent) => { const text = e.clipboardData.getData('text').trim() const match = text.match(/^(-?\d+\.?\d*)\s*[,;\s]\s*(-?\d+\.?\d*)$/) @@ -161,7 +163,7 @@ export default function CollectionPlaceDetail({ const lngNum = lng.trim() ? Number(lng) : NaN setSaving(true) try { - await onSave({ name: name.trim() || place.name, description: description.trim() || null, links: cleanLinks, category_id: categoryId, label_ids: labelIds, lat: Number.isFinite(latNum) ? latNum : null, lng: Number.isFinite(lngNum) ? lngNum : null }) + await onSave({ name: name.trim() || place.name, description: description.trim() || null, links: cleanLinks, category_id: categoryId, label_ids: labelIds, address: address.trim() || null, lat: Number.isFinite(latNum) ? latNum : null, lng: Number.isFinite(lngNum) ? lngNum : null }) setEditing(false) } catch (err) { toast.error(getApiErrorMessage(err, t('common.error'))) @@ -280,6 +282,11 @@ export default function CollectionPlaceDetail({ })}
+ {/* Address (#1870): free text, the coordinates stay separate */} +
+
{t('places.formAddress')}
+ setAddress(e.target.value)} placeholder={t('places.formAddressPlaceholder')} className="col-detail-input" /> +
{/* Coordinates */}
{t('collections.coordinates')}
diff --git a/client/src/components/Collections/MSaveToCollectionSheet.test.tsx b/client/src/components/Collections/MSaveToCollectionSheet.test.tsx index 19b2940d4..bd4279f01 100644 --- a/client/src/components/Collections/MSaveToCollectionSheet.test.tsx +++ b/client/src/components/Collections/MSaveToCollectionSheet.test.tsx @@ -115,7 +115,7 @@ describe('MSaveToCollectionSheet', () => { it('FE-COMP-MSAVESHEET-006: lists already holding the place are marked saved', async () => { vi.spyOn(collectionsApi, 'membership').mockResolvedValue({ saved: true, - lists: [{ collection_id: 1, name: 'Favorites', place_id: 900 }], + lists: [{ collection_id: 1, name: 'Favorites', place_id: 900, status: 'want', can_edit: true }], }); openFor(); render(); @@ -167,7 +167,7 @@ describe('MSaveToCollectionSheet', () => { it('FE-COMP-MSAVESHEET-010: tapping a saved list removes that place instead', async () => { vi.spyOn(collectionsApi, 'membership').mockResolvedValue({ saved: true, - lists: [{ collection_id: 1, name: 'Favorites', place_id: 900 }], + lists: [{ collection_id: 1, name: 'Favorites', place_id: 900, status: 'want', can_edit: true }], }); const del = vi.spyOn(collectionsApi, 'deletePlace').mockResolvedValue({}); openFor(); diff --git a/client/src/components/Collections/MSaveToCollectionSheet.tsx b/client/src/components/Collections/MSaveToCollectionSheet.tsx index b5ddadf76..8afd008b0 100644 --- a/client/src/components/Collections/MSaveToCollectionSheet.tsx +++ b/client/src/components/Collections/MSaveToCollectionSheet.tsx @@ -1,6 +1,6 @@ import { useEffect, useMemo, useState, useCallback } from 'react' import { useNavigate } from 'react-router' -import { Bookmark, BookmarkCheck, Check, Loader2, Plus, X } from 'lucide-react' +import { Bookmark, BookmarkCheck, Check, CheckCircle2, Loader2, Plus, X } from 'lucide-react' import MSheet from '../../mobile/components/MSheet' import MIconBtn from '../../mobile/components/MIconBtn' import { useToast } from '../shared/Toast' @@ -8,7 +8,8 @@ import { useTranslation } from '../../i18n' import { collectionsApi } from '../../api/collections' import { useSaveToCollectionStore } from '../../store/saveToCollectionStore' import { getApiErrorMessage } from '../../utils/apiError' -import type { Collection, CollectionMembership } from '@trek/shared' +import { STATUS_META, nextStatus } from '../../pages/collections/collectionsModel' +import type { Collection, CollectionMembership, CollectionStatus } from '@trek/shared' /** * Mobile counterpart of SaveToCollectionModal — the same store-driven list @@ -71,12 +72,44 @@ export default function MSaveToCollectionSheet() { // eslint-disable-next-line react-hooks/exhaustive-deps }, [target]) - const savedByCollection = new Map() - for (const l of membership?.lists ?? []) savedByCollection.set(l.collection_id, l.place_id) + const savedByCollection = new Map() + for (const l of membership?.lists ?? []) savedByCollection.set(l.collection_id, l) + + /** Lists holding this place that the viewer may edit and that are not visited yet. */ + const unvisited = (membership?.lists ?? []).filter(l => l.can_edit && l.status !== 'visited') + + const handleStatus = async (entry: CollectionMembership['lists'][number], next: CollectionStatus) => { + if (busyId != null) return + setBusyId(entry.collection_id) + try { + await collectionsApi.setStatus(entry.place_id, next) + await refreshMembership() + bumpVersion() + } catch (err) { + toast.error(getApiErrorMessage(err, t('common.error'))) + } finally { + setBusyId(null) + } + } + + const handleVisitedEverywhere = async () => { + if (busyId != null || unvisited.length === 0) return + setBusyId(-1) + try { + const { updated } = await collectionsApi.setStatusMany(unvisited.map(l => l.place_id), 'visited') + toast.success(t('collections.markedVisited', { count: updated })) + await refreshMembership() + bumpVersion() + } catch (err) { + toast.error(getApiErrorMessage(err, t('common.error'))) + } finally { + setBusyId(null) + } + } const handleToggle = async (list: Collection) => { if (busyId != null || !target) return - const savedPlaceId = savedByCollection.get(list.id) + const savedPlaceId = savedByCollection.get(list.id)?.place_id setBusyId(list.id) try { if (savedPlaceId != null) { @@ -156,8 +189,11 @@ export default function MSaveToCollectionSheet() {
) : ( lists.map(list => { - const saved = savedByCollection.has(list.id) + const entry = savedByCollection.get(list.id) + const saved = !!entry const busy = busyId === list.id + const statusMeta = entry ? STATUS_META[entry.status] : null + const StatusIcon = statusMeta?.icon return (
+ {unvisited.length > 0 && ( +
+ +
+ )} + {lists.length > 0 && (
+ )} +
{loading ? (
@@ -162,7 +211,8 @@ export default function SaveToCollectionModal(): React.ReactElement | null { ) : (
{lists.map(list => { - const saved = savedByCollection.has(list.id) + const entry = savedByCollection.get(list.id) + const saved = !!entry const busy = busyId === list.id return (