feat(collections): edit a saved place's provider ids, website, phone and price (#2646)

The update contract takes google_place_id, google_ftid, osm_id, website,
phone, price and currency like saving does, website with the same
normalisation and refusal. REST and update_collection_place share it, so an
agent can correct a place instead of re-saving it and losing its labels,
ratings, position and id. The assistant's change card names the new fields.

server/src/db/repositories/CollectionPlaces.repository.ts

@@ -171,6 +171,13 @@ export class CollectionPlacesRepository extends TrekRepository<CollectionPlaces>
       category_id: number | null;
       image_url: string | null;
       links: string | null;
+      google_place_id: string | null;
+      google_ftid: string | null;
+      osm_id: string | null;
+      website: string | null;
+      phone: string | null;
+      price: number | null;
+      currency: string | null;
       collection_id: number;
       owner_id: number;
     }>,

server/src/nest/assistant/cards/collection/collection-changes.ts

@@ -77,6 +77,13 @@ const FIELD_OF: Readonly<Record<string, (typeof COLLECTION_CARD_FIELDS)[number]>
   category_id: 'category',
   links: 'link',
   image_url: 'photo',
+  website: 'website',
+  phone: 'phone',
+  price: 'price',
+  currency: 'price',
+  google_place_id: 'mapEntry',
+  google_ftid: 'mapEntry',
+  osm_id: 'mapEntry',
 };
 
 function fieldsOf(args: ToolArgs): CollectionCardDelta['fields'] {

server/src/nest/collections/collections.helpers.ts

@@ -1,4 +1,4 @@
-import type { CollectionLink } from '@trek/shared';
+import type { CollectionLink, CollectionPlaceUpdateRequest } from '@trek/shared';
 
 /**
  * Pure helpers of the collections domain, moved verbatim out of
@@ -20,3 +20,31 @@ export function parseLinks(raw: unknown): CollectionLink[] | undefined {
 export function serializeLinks(links: CollectionLink[] | undefined): string | null {
   return links && links.length ? JSON.stringify(links) : null;
 }
+
+/** The provider ids, contact and price columns of a saved place, as an update writes them. */
+type ListingWrite = Partial<{
+  google_place_id: string | null;
+  google_ftid: string | null;
+  osm_id: string | null;
+  website: string | null;
+  phone: string | null;
+  price: number | null;
+  currency: string | null;
+}>;
+
+/**
+ * The fields saving a place accepts and editing it did not until #2646: each one
+ * the update names, null clearing it, the rest left out so the stored value stays.
+ * The website arrives normalised and checked by the contract, as on save.
+ */
+export function listingWriteOf(body: CollectionPlaceUpdateRequest): ListingWrite {
+  const write: ListingWrite = {};
+  if (body.google_place_id !== undefined) write.google_place_id = body.google_place_id ?? null;
+  if (body.google_ftid !== undefined) write.google_ftid = body.google_ftid ?? null;
+  if (body.osm_id !== undefined) write.osm_id = body.osm_id ?? null;
+  if (body.website !== undefined) write.website = body.website ?? null;
+  if (body.phone !== undefined) write.phone = body.phone ?? null;
+  if (body.price !== undefined) write.price = body.price ?? null;
+  if (body.currency !== undefined) write.currency = body.currency ?? null;
+  return write;
+}

server/src/nest/collections/collections.mcp.ts

@@ -327,7 +327,7 @@ export class CollectionsMcp {
   @Tool({
     name: 'update_collection_place',
     description:
-      "Update a saved place's name, address, coordinates (lat/lng), description, notes, status, category, links, tags, labels, image, or move it to another collection (set collection_id). Moving a place out of a list needs the same role there as deleting it (owner or admin).",
+      "Update a saved place's name, address, coordinates (lat/lng), description, notes, status, category, links, tags, labels, image, provider ids (google_place_id, google_ftid, osm_id), website, phone, price and currency, or move it to another collection (set collection_id). Only the fields you pass change; null clears one. A bare domain as website gets https://, any other scheme is refused. Moving a place out of a list needs the same role there as deleting it (owner or admin).",
     inputSchema: { placeId: idSchema, ...collectionPlaceUpdateRequestSchema.shape, links: linksInput },
     annotations: TOOL_ANNOTATIONS_WRITE,
     when: collectionsAddonOn,

server/src/nest/collections/collections.service.ts

@@ -75,7 +75,7 @@ import type {
 import { NotificationsService } from '../notifications/notifications.service';
 import { PlaceImportService, type ExportedCollectionFile } from '../place-import/place-import.service';
 import { UnitOfWork } from '../database/unit-of-work';
-import { parseLinks, serializeLinks } from './collections.helpers';
+import { listingWriteOf, parseLinks, serializeLinks } from './collections.helpers';
 
 // Errors: a DomainError carrying a status; TrekExceptionFilter maps it to that code with an `{ error }` body.
 
@@ -872,7 +872,7 @@ export class CollectionsService {
     await this.assertCanEdit(userId, currentCollection);
     const prevImage = body.image_url !== undefined ? (await this.collectionPlaces.imageUrl(placeId)) ?? null : null;
 
-    const write: Parameters<CollectionPlacesRepository['updateFields']>[1] = {};
+    const write: Parameters<CollectionPlacesRepository['updateFields']>[1] = listingWriteOf(body);
     if (body.name !== undefined) write.name = body.name;
     if (body.description !== undefined) write.description = body.description ?? null;
     if (body.notes !== undefined) write.notes = body.notes ?? null;

server/tests/e2e/collections.e2e.test.ts

@@ -208,6 +208,66 @@ describe('Collections e2e (real auth guard + real service + temp SQLite)', () =>
     expect(await countRows(orm, CollectionPlaces, { collection: col.id })).toBe(0);
   });
 
+  // #2646: saving took the provider ids, website, phone and price while editing
+  // did not, so fixing one meant deleting and re-saving the place, which lost its
+  // labels, ratings, position and id.
+  it('COLLECTIONS-E2E-091: PATCH sets and clears the provider ids, website, phone and price', async () => {
+    const col = (
+      await request(server)
+        .post('/api/addons/collections')
+        .set('Cookie', sessionCookie(ownerId))
+        .send({ name: 'Listings' })
+    ).body;
+    const saved = await request(server)
+      .post('/api/addons/collections/places')
+      .set('Cookie', sessionCookie(ownerId))
+      .send({ collection_id: col.id, name: 'Septime', phone: '+33 1 43 67 38 29', price: 40, currency: 'EUR' });
+    expect(saved.status).toBe(200);
+    const placeId = saved.body.place.id;
+    const patch = (body: Record<string, unknown>) =>
+      request(server)
+        .patch(`/api/addons/collections/places/${placeId}`)
+        .set('Cookie', sessionCookie(ownerId))
+        .send(body);
+
+    const patched = await patch({
+      google_place_id: 'ChIJseptime',
+      google_ftid: '0x47e6:0x1b2',
+      osm_id: 'node/42',
+      website: 'septime-charonne.fr',
+      price: 55,
+    });
+    expect(patched.status).toBe(200);
+    // A bare domain gets https:// as on save; what the update leaves out stays.
+    expect(patched.body).toMatchObject({
+      id: placeId,
+      google_place_id: 'ChIJseptime',
+      google_ftid: '0x47e6:0x1b2',
+      osm_id: 'node/42',
+      website: 'https://septime-charonne.fr',
+      phone: '+33 1 43 67 38 29',
+      price: 55,
+      currency: 'EUR',
+    });
+
+    const refused = await patch({ website: 'javascript:alert(1)' });
+    expect(refused.status).toBe(400);
+    expect(refused.body.error).toMatch(/^website: /);
+    expect((await findRow(orm, CollectionPlaces, { id: placeId }))?.website).toBe('https://septime-charonne.fr');
+
+    const cleared = await patch({ website: null, phone: null, price: null, currency: null, google_place_id: null });
+    expect(cleared.status).toBe(200);
+    expect(await findRow(orm, CollectionPlaces, { id: placeId })).toMatchObject({
+      website: null,
+      phone: null,
+      price: null,
+      currency: null,
+      google_place_id: null,
+      google_ftid: '0x47e6:0x1b2',
+      osm_id: 'node/42',
+    });
+  });
+
   // Regression for #1437: editing a place (PATCH without a status field) must NOT
   // reset a 'want'/'visited' place back to 'idea'.
   it('COLLECTIONS-E2E-012: PATCH without status leaves the saved status unchanged', async () => {

server/tests/unit/mcp/tools-collections.test.ts

@@ -565,6 +565,51 @@ describe('Tool: update_collection_place / set_collection_place_status / rate_col
     });
     expect((await findRow(orm, CollectionPlaces, { id: pid }))?.collection_id).toBe(shared);
   });
+
+  // #2646: the fields saving takes, through the same contract and checks as REST.
+  it('update_collection_place sets and clears provider ids, website, phone and price, and refuses a script link', async () => {
+    const { user } = createUser(testDb);
+    const col = await seedCollection(user.id, 'Listings');
+    const pid = await seedPlace(col, user.id, 'Septime');
+    const update = (h: McpHarness, args: Record<string, unknown>) =>
+      h.client.callTool({ name: 'update_collection_place', arguments: { placeId: pid, ...args } });
+    await withHarness(user.id, async (h) => {
+      const set = parseToolResult(
+        await update(h, {
+          google_place_id: 'ChIJseptime',
+          osm_id: 'node/42',
+          website: 'septime-charonne.fr',
+          phone: '+33 1 43 67 38 29',
+          price: 55,
+          currency: 'EUR',
+        }),
+      ) as { place: Record<string, unknown> };
+      expect(set.place).toMatchObject({
+        name: 'Septime',
+        google_place_id: 'ChIJseptime',
+        osm_id: 'node/42',
+        website: 'https://septime-charonne.fr',
+        phone: '+33 1 43 67 38 29',
+        price: 55,
+        currency: 'EUR',
+      });
+
+      const refused = await update(h, { website: 'javascript:alert(1)' });
+      expect(refused.isError).toBe(true);
+      expect((refused.content as { text: string }[])[0].text).toContain('Invalid arguments');
+
+      const cleared = parseToolResult(await update(h, { website: null, phone: null, price: null })) as {
+        place: Record<string, unknown>;
+      };
+      expect(cleared.place).toMatchObject({
+        website: null,
+        phone: null,
+        price: null,
+        currency: 'EUR',
+        google_place_id: 'ChIJseptime',
+      });
+    });
+  });
 });
 
 // ---------------------------------------------------------------------------

server/tests/unit/nest/assistant/cards/collection/collection-changes.test.ts

@@ -1,5 +1,5 @@
 /**
- * ASTCOL-020 to ASTCOL-035: the change cards of the collections' writes, as
+ * ASTCOL-020 to ASTCOL-035 and ASTCOL-037: the change cards of the collections' writes, as
  * describeToolCall writes them (the card through the guarded registry, so it
  * passed the union), and the lookups they read first.
  */
@@ -104,6 +104,21 @@ describe('collection change cards', () => {
     expect(placesOf(removed.card).places[0]?.delta?.removed).toBe(true);
   });
 
+  it('ASTCOL-037: the provider ids, contact and price fields an update now takes are named once each (#2646)', () => {
+    const items = itemsValue([savedPlace()]);
+    const args = {
+      placeId: 34,
+      website: 'septime-charonne.fr',
+      phone: null,
+      price: 45,
+      currency: 'EUR',
+      google_place_id: 'ChIJseptime',
+      osm_id: 'node/1',
+    };
+    const card = placesOf(preview('update_collection_place', args, { items }).card);
+    expect(card.places[0]?.delta?.fields).toEqual(['website', 'phone', 'price', 'mapEntry']);
+  });
+
   it('ASTCOL-023: a change of a saved place names its fields, its labels as its own list allows them and the list it moves to', () => {
     const items = itemsValue([savedPlace()]);
     // In its own list: the labels asked for replace the ones it carries.

server/tests/unit/nest/collections.helpers.test.ts

@@ -1,9 +1,9 @@
 /**
- * The link helpers of the collections domain, which read and write the JSON
- * TEXT column a list or a saved place keeps its links in.
- * COLLECTIONS-HELPERS-001 to 003.
+ * The pure helpers of the collections domain: the link helpers, which read and
+ * write the JSON TEXT column a list or a saved place keeps its links in, and the
+ * listing fields an update writes. COLLECTIONS-HELPERS-001 to 005.
  */
-import { parseLinks, serializeLinks } from '../../../src/nest/collections/collections.helpers';
+import { listingWriteOf, parseLinks, serializeLinks } from '../../../src/nest/collections/collections.helpers';
 
 import { describe, expect, it } from 'vitest';
 
@@ -29,3 +29,32 @@ describe('serializeLinks', () => {
     expect(serializeLinks(undefined)).toBeNull();
   });
 });
+
+describe('listingWriteOf', () => {
+  it('COLLECTIONS-HELPERS-004: writes each provider id, contact and price field the update names, null clearing it', () => {
+    expect(
+      listingWriteOf({
+        google_place_id: 'ChIJseptime',
+        google_ftid: null,
+        osm_id: 'node/42',
+        website: 'https://septime-charonne.fr',
+        phone: null,
+        price: 0,
+        currency: 'EUR',
+      }),
+    ).toEqual({
+      google_place_id: 'ChIJseptime',
+      google_ftid: null,
+      osm_id: 'node/42',
+      website: 'https://septime-charonne.fr',
+      phone: null,
+      price: 0,
+      currency: 'EUR',
+    });
+  });
+
+  it('COLLECTIONS-HELPERS-005: leaves out what the update does not name, so the stored value stays', () => {
+    expect(listingWriteOf({ name: 'Septime', notes: 'Book ahead' })).toEqual({});
+    expect(listingWriteOf({ website: undefined, phone: '+33 1' })).toEqual({ phone: '+33 1' });
+  });
+});