diff --git a/client/src/pages/AtlasPage.wiring.test.tsx b/client/src/pages/AtlasPage.wiring.test.tsx index 899af7193..c5a3ad40c 100644 --- a/client/src/pages/AtlasPage.wiring.test.tsx +++ b/client/src/pages/AtlasPage.wiring.test.tsx @@ -44,6 +44,8 @@ beforeEach(() => { http.post('/api/addons/atlas/country/:code/mark', () => HttpResponse.json({ success: true })), http.post('/api/addons/atlas/region/:code/mark', () => HttpResponse.json({ success: true })), http.delete('/api/addons/atlas/region/:code/mark', () => HttpResponse.json({ success: true })), + // After a region goes the atlas asks whether the country is still marked (#2597). + http.get('/api/addons/atlas/country/:code', () => HttpResponse.json({ manually_marked: false })), http.post('/api/addons/atlas/bucket-list', () => HttpResponse.json({ item: buildBucketItem({ id: 99, name: 'Germany', country_code: 'DE' }) }), ), diff --git a/client/src/pages/atlas/useAtlasCountryActions.test.ts b/client/src/pages/atlas/useAtlasCountryActions.test.ts index addbb6070..45e396676 100644 --- a/client/src/pages/atlas/useAtlasCountryActions.test.ts +++ b/client/src/pages/atlas/useAtlasCountryActions.test.ts @@ -1,4 +1,4 @@ -// FE-PAGE-ATLASACTIONS-001 to FE-PAGE-ATLASACTIONS-014: the country / region popup logic +// FE-PAGE-ATLASACTIONS-001 to FE-PAGE-ATLASACTIONS-015: the country / region popup logic // behind both the desktop dialog and the phone sheet. import { act, renderHook } from '@testing-library/react'; import { http, HttpResponse } from 'msw'; @@ -73,6 +73,9 @@ beforeEach(() => { deleted.push(new URL(request.url).pathname); return HttpResponse.json({ success: true }); }), + http.get('/api/addons/atlas/country/:code', () => + HttpResponse.json({ places: [], trips: [], manually_marked: false, marked_source: null, status: 'idea' }) + ), http.post('/api/addons/atlas/bucket-list', async ({ request }) => { const body = (await request.json()) as Record; posted.push({ url: '/api/addons/atlas/bucket-list', body }); @@ -202,6 +205,32 @@ describe('useAtlasCountryActions', () => { expect(result.current.confirmAction).toBeNull(); }); + it('FE-PAGE-ATLASACTIONS-015: a country the server still holds as marked stays when its last region goes (#2597)', async () => { + const init = () => ({ + confirmAction: { type: 'unmark-region', code: 'ES', name: 'Galicia', regionCode: 'ES-GA' } as AtlasConfirmAction, + data: buildAtlasData({ + countries: [{ code: 'ES', tripCount: 0, placeCount: 0, firstVisit: null, lastVisit: null }], + }), + visitedRegions: { ES: [{ code: 'ES-GA', name: 'Galicia', placeCount: 0 }] }, + }); + server.use( + http.get('/api/addons/atlas/country/:code', () => + HttpResponse.json({ places: [], trips: [], manually_marked: true, marked_source: 'manual', status: 'visited' }) + ) + ); + const marked = setup(init()); + await act(() => marked.result.current.actions.unmarkRegion()); + expect(marked.result.current.visitedRegions).toEqual({}); + expect(marked.result.current.data?.countries.map((c) => c.code)).toEqual(['ES']); + + // No answer: the country stays until the next load rather than vanishing on a guess. + server.use(http.get('/api/addons/atlas/country/:code', () => HttpResponse.json({}, { status: 500 }))); + const unknown = setup(init()); + await act(() => unknown.result.current.actions.unmarkRegion()); + expect(unknown.result.current.data?.countries.map((c) => c.code)).toEqual(['ES']); + expect(addToast).not.toHaveBeenCalled(); + }); + it('FE-PAGE-ATLASACTIONS-010: adding to the bucket list posts the entry, prepends it and runs beforeClose', async () => { const beforeClose = vi.fn(); const { result } = setup({ diff --git a/client/src/pages/atlas/useAtlasCountryActions.ts b/client/src/pages/atlas/useAtlasCountryActions.ts index 2cc96e293..0ce92505d 100644 --- a/client/src/pages/atlas/useAtlasCountryActions.ts +++ b/client/src/pages/atlas/useAtlasCountryActions.ts @@ -67,11 +67,12 @@ export function withRegionUnmarked(prev: VisitedRegionMap, countryCode: string, } /** - * The atlas once a region is unmarked: if no visible region of the country remains - * (however a region was derived: the server hides it either way and cascades to the - * country the same way), the country goes too, but only when it has no place or trip - * of its own. A country with real places is never hidden server side (#1490), so - * removing it here would only flash and reappear on the next load. + * The atlas once a region is unmarked and the server no longer holds the country as + * marked: if no visible region of the country remains (however a region was derived: + * the server hides it either way and cascades to the country the same way), the + * country goes too, but only when it has no place or trip of its own. A country with + * real places is never hidden server side (#1490), so removing it here would only + * flash and reappear on the next load. */ export function withCountryDroppedAfterRegionUnmark( prev: AtlasData | null, @@ -93,6 +94,21 @@ export function withCountryDroppedAfterRegionUnmark( }; } +/** + * Whether the server still holds the country as marked after one of its regions was + * unmarked. A country the user marked itself survives its last region (#2597), so the + * atlas asks instead of guessing; when the answer does not come, the country stays + * until the next load settles it. + */ +async function isCountryStillMarked(code: string): Promise { + try { + const r = await apiClient.get(`/addons/atlas/country/${code}`); + return r.data?.manually_marked !== false; + } catch { + return true; + } +} + /** * What the country / region action popup does, behind both the desktop dialog and the * phone sheet (which render their own markup and their own bucket list date picker): @@ -155,7 +171,9 @@ export function useAtlasCountryActions({ try { await apiClient.delete(`/addons/atlas/region/${regionCode}/mark`); setVisitedRegions((prev) => withRegionUnmarked(prev, countryCode, regionCode)); - setData((prev) => withCountryDroppedAfterRegionUnmark(prev, countryCode, regionCode, visitedRegions)); + if (!(await isCountryStillMarked(countryCode))) { + setData((prev) => withCountryDroppedAfterRegionUnmark(prev, countryCode, regionCode, visitedRegions)); + } } catch (err) { toast.error(getApiErrorMessage(err, t('common.error'))); } diff --git a/client/tests/unit/mobile/atlas/MAtlasCountryPopup.test.tsx b/client/tests/unit/mobile/atlas/MAtlasCountryPopup.test.tsx index f2c1fc366..6654acaa6 100644 --- a/client/tests/unit/mobile/atlas/MAtlasCountryPopup.test.tsx +++ b/client/tests/unit/mobile/atlas/MAtlasCountryPopup.test.tsx @@ -30,6 +30,8 @@ beforeEach(() => { http.post('/api/addons/atlas/country/:code/mark', () => HttpResponse.json({ success: true })), http.post('/api/addons/atlas/region/:code/mark', () => HttpResponse.json({ success: true })), http.delete('/api/addons/atlas/region/:code/mark', () => HttpResponse.json({ success: true })), + // After a region goes the atlas asks whether the country is still marked (#2597). + http.get('/api/addons/atlas/country/:code', () => HttpResponse.json({ manually_marked: false })), http.post('/api/addons/atlas/bucket-list', () => HttpResponse.json({ item: { id: 42, name: 'Germany', country_code: 'DE', lat: null, lng: null, notes: null, target_date: '2027-05' } }), ), diff --git a/server/src/db/repositories/VisitedCountries.repository.ts b/server/src/db/repositories/VisitedCountries.repository.ts index 2764d5de0..579c5f1ad 100644 --- a/server/src/db/repositories/VisitedCountries.repository.ts +++ b/server/src/db/repositories/VisitedCountries.repository.ts @@ -20,6 +20,15 @@ export interface VisitedCountryRow { type VisitedCountriesReadKyselyDB = Pick; type VisitedCountriesWriteKyselyDB = Pick; +/** + * The source as callers see it. 'region' marks a country that only came onto + * the map through one of its regions and goes again with the last of them + * (#2597); to everyone else it reads as a mark made by hand, as it always did. + */ +function publicSource(source: string): string { + return source === 'region' ? 'manual' : source; +} + /** * `visited_countries` — the user's own explicit "I've been here" marks * (Plan 3f Task 1, atlas). `markVisited` (AT12) needs the `changes > 0` @@ -43,12 +52,13 @@ export class VisitedCountriesRepository extends TrekRepository /** AT10 (`listVisitedCountries`) — `SELECT country_code, created_at, source FROM visited_countries WHERE user_id = ? ORDER BY created_at DESC`. */ async listForUser(userId: number): Promise { - return await this.readDb() + const rows = await this.readDb() .selectFrom('visited_countries') .select(['country_code', 'created_at', 'source']) .where('user_id', '=', userId) .orderBy('created_at', 'desc') .execute(); + return rows.map((row) => ({ ...row, source: publicSource(row.source) })); } /** @@ -69,12 +79,29 @@ export class VisitedCountriesRepository extends TrekRepository /** AT8/AT9 (`countryPlaces`'s no-trip early return, `countryPlaces`'s mark lookup — identical text) — `SELECT source FROM visited_countries WHERE user_id = ? AND country_code = ?`. */ async findSource(userId: number, countryCode: string): Promise<{ source: string } | undefined> { - return await this.writeDb() + const row = await this.writeDb() .selectFrom('visited_countries') .select('source') .where('user_id', '=', userId) .where('country_code', '=', countryCode) .executeTakeFirst(); + return row && { source: publicSource(row.source) }; + } + + /** + * `unmarkRegion`'s cascade check: whether the user marked the country visited + * themselves (by hand or from Dawarich), rather than it only coming in + * through one of its regions (#2597). + */ + async hasOwnMark(userId: number, countryCode: string): Promise { + const row = await this.writeDb() + .selectFrom('visited_countries') + .select('country_code') + .where('user_id', '=', userId) + .where('country_code', '=', countryCode) + .where('source', '!=', 'region') + .executeTakeFirst(); + return !!row; } /** @@ -103,20 +130,34 @@ export class VisitedCountriesRepository extends TrekRepository /** * AT19 (`markRegion`, inside its transaction) — `INSERT OR IGNORE INTO - * visited_countries (user_id, country_code) VALUES (?, ?)`, a DISTINCT - * 2-column variant of {@link markVisited}'s 3-column insert. The legacy - * statement's column list omits `source`, letting the schema's own - * `DEFAULT 'manual'` fill it — reproduced here by passing that literal - * explicitly (`VisitedCountries.entity.ts`'s `source: p.text() - * .default('manual')`, the exact same value the omitted column would - * have taken). No `changes` signal here; the legacy statement never - * checked it either. + * visited_countries (user_id, country_code, source) VALUES (?, ?, 'region')`. + * A country that only came onto the map through one of its regions is + * written as 'region', not 'manual', so removing its last region can tell + * it apart from a country the user ticked off itself (#2597). The conflict + * is ignored, so a country already marked keeps its own source. Rows written + * before this change carry 'manual' and are left as they are. No `changes` + * signal here; the legacy statement never checked it either. */ async markFromRegion(userId: number, countryCode: string): Promise { await this.writeDb() .insertInto('visited_countries') - .values({ user_id: userId, country_code: countryCode, source: 'manual' }) + .values({ user_id: userId, country_code: countryCode, source: 'region' }) .onConflict((oc) => oc.doNothing()) .execute(); } + + /** + * `markCountry`, inside its transaction, right after {@link markVisited}: a + * country that only came in through a region becomes the user's own mark + * once they mark it visited themselves. Any other row is left alone. + */ + async promoteFromRegion(userId: number, countryCode: string, source: 'manual' | 'dawarich'): Promise { + await this.writeDb() + .updateTable('visited_countries') + .set({ source }) + .where('user_id', '=', userId) + .where('country_code', '=', countryCode) + .where('source', '=', 'region') + .execute(); + } } diff --git a/server/src/nest/atlas/atlas.mcp.ts b/server/src/nest/atlas/atlas.mcp.ts index 7fda92a86..ac84bbddf 100644 --- a/server/src/nest/atlas/atlas.mcp.ts +++ b/server/src/nest/atlas/atlas.mcp.ts @@ -283,7 +283,8 @@ export class AtlasMcp { @Tool({ name: 'unmark_region_visited', - description: 'Remove a region from the visited list.', + description: + 'Remove a region from the visited list. Removing the last region of a country also removes the country, unless it was marked visited itself.', inputSchema: { regionCode: z.string(), }, diff --git a/server/src/nest/atlas/atlas.service.ts b/server/src/nest/atlas/atlas.service.ts index bb6ce52ce..bd8acf07f 100644 --- a/server/src/nest/atlas/atlas.service.ts +++ b/server/src/nest/atlas/atlas.service.ts @@ -685,14 +685,13 @@ export class AtlasService { * every mark was until an integration could make one, and `INSERT OR IGNORE` * means a country already ticked by hand keeps that provenance — confirming * it again from a recording does not relabel somebody's own work as imported. - */ - /** * Returns whether this actually added the country, so a caller reporting * "3 countries added" is not counting the ones that were already there. */ async markCountry(userId: number, code: string, source: 'manual' | 'dawarich' = 'manual'): Promise { return await this.uow.transactional(async () => { const inserted = await this.visitedCountries.markVisited(userId, code, source); // AT12 + await this.visitedCountries.promoteFromRegion(userId, code, source); // #2597 // Marking it visited again lifts a previous removal. await this.hiddenCountries.unhide(userId, code); // AT13 return inserted; @@ -763,10 +762,11 @@ export class AtlasService { if (countryCode) { await this.hiddenRegions.hide(userId, code, countryCode); // AT27 - // If that was the country's last visible region, hide the country too — otherwise it - // keeps showing "visited" on the world map with nothing left to drill into. + // If that was the country's last visible region, hide the country too, or it keeps showing + // "visited" with nothing left to drill into. One the user marked itself stays (#2597). + const ownMark = await this.visitedCountries.hasOwnMark(userId, countryCode); const hidden = await this.getHiddenRegions(userId); - if (!(await this.hasVisibleRegionForCountry(userId, countryCode, hidden))) { + if (!ownMark && !(await this.hasVisibleRegionForCountry(userId, countryCode, hidden))) { await this.unmarkCountry(userId, countryCode); } } diff --git a/server/tests/integration/atlas.test.ts b/server/tests/integration/atlas.test.ts index 3e6f6392b..fe63d47a9 100644 --- a/server/tests/integration/atlas.test.ts +++ b/server/tests/integration/atlas.test.ts @@ -424,6 +424,43 @@ describe('Mark/unmark region', () => { expect(codes).toContain('DE'); }); + it('ATLAS-011: unmark last region keeps a country the user marked visited itself (#2597)', async () => { + const { user } = createUser(testDb); + const cookie = authCookie(user.id); + + await request(app).post('/api/addons/atlas/country/NO/mark').set('Cookie', cookie); + await request(app) + .post('/api/addons/atlas/region/NO-03/mark') + .set('Cookie', cookie) + .send({ name: 'Oslo', country_code: 'NO' }); + await request(app).delete('/api/addons/atlas/region/NO-03/mark').set('Cookie', cookie); + + const stats = await request(app).get('/api/addons/atlas/stats').set('Cookie', cookie); + expect((stats.body.countries as { code: string }[]).map((c) => c.code)).toContain('NO'); + const country = await request(app).get('/api/addons/atlas/country/NO').set('Cookie', cookie); + expect(country.body).toMatchObject({ manually_marked: true, marked_source: 'manual' }); + const regions = await request(app).get('/api/addons/atlas/regions').set('Cookie', cookie); + expect(regions.body.regions['NO']).toBeUndefined(); + }); + + it('ATLAS-011: a country first reached through a region stays once it is marked itself (#2597)', async () => { + const { user } = createUser(testDb); + const cookie = authCookie(user.id); + + await request(app) + .post('/api/addons/atlas/region/NO-03/mark') + .set('Cookie', cookie) + .send({ name: 'Oslo', country_code: 'NO' }); + const before = await request(app).get('/api/addons/atlas/country/NO').set('Cookie', cookie); + expect(before.body).toMatchObject({ manually_marked: true, marked_source: 'manual' }); + + await request(app).post('/api/addons/atlas/country/NO/mark').set('Cookie', cookie); + await request(app).delete('/api/addons/atlas/region/NO-03/mark').set('Cookie', cookie); + + const stats = await request(app).get('/api/addons/atlas/stats').set('Cookie', cookie); + expect((stats.body.countries as { code: string }[]).map((c) => c.code)).toContain('NO'); + }); + it('ATLAS-011 — regions are isolated between users', async () => { const { user: user1 } = createUser(testDb); const { user: user2 } = createUser(testDb); diff --git a/server/tests/unit/db/repositories/VisitedCountries.repository.test.ts b/server/tests/unit/db/repositories/VisitedCountries.repository.test.ts index 999a1ca51..67d9e33b6 100644 --- a/server/tests/unit/db/repositories/VisitedCountries.repository.test.ts +++ b/server/tests/unit/db/repositories/VisitedCountries.repository.test.ts @@ -10,7 +10,7 @@ import { VisitedCountries } from '../../../../src/db/entities/VisitedCountries.e import type { VisitedCountriesRepository } from '../../../../src/db/repositories/VisitedCountries.repository'; import { createSnapshotTestDb } from '../../../helpers/db-mock'; import { createUser } from '../../../helpers/factories'; -import { insertRows } from '../../../helpers/factories/rows'; +import { findRow, insertRows } from '../../../helpers/factories/rows'; import { resetTestDb } from '../../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; @@ -82,3 +82,46 @@ describe('VisitedCountriesRepository.listCodesForUser (AT5/AT7/AT43)', () => { expect(await repo.listCodesForUser(user.id)).toEqual([]); }); }); + +describe('VisitedCountriesRepository: countries that came in through a region (#2597)', () => { + it('VISCOUNTRYREPO-005: markFromRegion writes region, and the readers still answer manual', async () => { + const { user } = createUser(testDb); + await repo.markFromRegion(user.id, 'NO'); + + expect((await findRow(t, VisitedCountries, { user: user.id, country_code: 'NO' }))?.source).toBe('region'); + expect(await repo.findSource(user.id, 'NO')).toEqual({ source: 'manual' }); + expect((await repo.listForUser(user.id)).map((r) => r.source)).toEqual(['manual']); + expect(await repo.hasOwnMark(user.id, 'NO')).toBe(false); + }); + + it('VISCOUNTRYREPO-006: markFromRegion leaves a country the user marked itself as it was', async () => { + const { user } = createUser(testDb); + await repo.markVisited(user.id, 'NO', 'dawarich'); + await repo.markFromRegion(user.id, 'NO'); + + expect(await repo.findSource(user.id, 'NO')).toEqual({ source: 'dawarich' }); + expect(await repo.hasOwnMark(user.id, 'NO')).toBe(true); + }); + + it('VISCOUNTRYREPO-007: promoteFromRegion turns a region row into the given mark and touches nothing else', async () => { + const { user } = createUser(testDb); + await repo.markFromRegion(user.id, 'NO'); + await repo.markVisited(user.id, 'SE', 'manual'); + + await repo.promoteFromRegion(user.id, 'NO', 'dawarich'); + await repo.promoteFromRegion(user.id, 'SE', 'dawarich'); + + expect(await repo.findSource(user.id, 'NO')).toEqual({ source: 'dawarich' }); + expect(await repo.findSource(user.id, 'SE')).toEqual({ source: 'manual' }); + expect(await repo.hasOwnMark(user.id, 'NO')).toBe(true); + }); + + it('VISCOUNTRYREPO-008: hasOwnMark is false without a row and scoped to the user', async () => { + const { user } = createUser(testDb); + const { user: other } = createUser(testDb, { username: 'other' }); + await repo.markVisited(other.id, 'NO', 'manual'); + + expect(await repo.hasOwnMark(user.id, 'NO')).toBe(false); + expect(await repo.findSource(user.id, 'NO')).toBeUndefined(); + }); +}); diff --git a/server/tests/unit/mcp/tools-atlas-expanded.test.ts b/server/tests/unit/mcp/tools-atlas-expanded.test.ts index f75191125..0bc37c464 100644 --- a/server/tests/unit/mcp/tools-atlas-expanded.test.ts +++ b/server/tests/unit/mcp/tools-atlas-expanded.test.ts @@ -11,6 +11,7 @@ import { HiddenRegions } from '../../../src/db/entities/HiddenRegions.entity'; import { PlaceRegions } from '../../../src/db/entities/PlaceRegions.entity'; import { Places } from '../../../src/db/entities/Places.entity'; import { ReservationEndpoints } from '../../../src/db/entities/ReservationEndpoints.entity'; +import { VisitedCountries } from '../../../src/db/entities/VisitedCountries.entity'; import { VisitedRegions } from '../../../src/db/entities/VisitedRegions.entity'; import { createUser, @@ -424,6 +425,21 @@ describe('Tool: unmark_region_visited', () => { }); }); + it('keeps a country the user marked itself when its last region goes, like REST (#2597)', async () => { + const { user } = createUser(testDb); + await markCountryVisited(orm, user.id, 'NO'); + await markRegion(user.id, 'NO-03', 'Oslo', 'NO'); + await insertRow(orm, VisitedCountries, { user: user.id, country_code: 'SE', source: 'region' }); + await markRegion(user.id, 'SE-AB', 'Stockholm', 'SE'); + await withHarness(user.id, async (h) => { + await h.client.callTool({ name: 'unmark_region_visited', arguments: { regionCode: 'NO-03' } }); + await h.client.callTool({ name: 'unmark_region_visited', arguments: { regionCode: 'SE-AB' } }); + }); + expect(await findRow(orm, VisitedCountries, { user: user.id, country_code: 'NO' })).toBeTruthy(); + // A country that only came in through its region still goes with it. + expect(await findRow(orm, VisitedCountries, { user: user.id, country_code: 'SE' })).toBeNull(); + }); + it('succeeds even when region was not marked (no-op)', async () => { const { user } = createUser(testDb); await withHarness(user.id, async (h) => { diff --git a/wiki/MCP-Addon-Tools.md b/wiki/MCP-Addon-Tools.md index 192d945df..19c3fc2b2 100644 --- a/wiki/MCP-Addon-Tools.md +++ b/wiki/MCP-Addon-Tools.md @@ -58,7 +58,7 @@ Requires `atlas:read` or `atlas:write` scope. | `list_visited_regions` | List all manually visited sub-country regions for the current user. | | `locate_atlas_region` | Resolve a coordinate to the country and region codes the Atlas map can highlight, which is where the pair `mark_region_visited` expects comes from. A point outside every bundled polygon answers with `null` fields rather than an error. For a postal address use `reverse_geocode` instead. | | `mark_region_visited` | Mark a sub-country region as visited (e.g. `"US-CA"`). | -| `unmark_region_visited` | Remove a region from the visited list. | +| `unmark_region_visited` | Remove a region from the visited list. Removing the last region of a country also removes the country, unless it was marked visited itself. | | `get_country_atlas_places` | Get places saved in the user's atlas for a specific country. | | `create_bucket_list_item` | Add a destination to your personal bucket list with optional coordinates, country code, target date and `region_code` (a state or province, hatched on the Atlas map). The same destination for the same target date is rejected as a duplicate. | | `update_bucket_list_item` | Update a bucket list item (name, notes, coordinates, target date). |