k1nq/TREK
1
0
Fork 0
mirror of https://github.com/mauriceboe/TREK.git synced 2026-10-18 21:16:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(packing): correct legacy quirks kept during the DI migration

Browse Source
This commit is contained in:
jubnl
2026-07-26 16:56:18 +02:00
parent 5da2abf4f9
commit 934848e114
5 changed files with 121 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/src/nest/packing/packing.mcp.ts
+7 -4
View File
@@ -20,11 +20,12 @@ function parseId(value: string | string[]): number | null {
}
/**
* Packing MCP surface — ported 1:1 from the legacy registrars: the seventeen
* Packing MCP surface — ported from the legacy registrars: the seventeen
* tools from src/mcp/tools/packing.ts and the trek://trips/{tripId}/packing +
* trek://trips/{tripId}/packing/bags resources from src/mcp/resources.ts
* (identical names, descriptions, schemas, annotations, error/payload shapes
* and broadcasts). The registration-time gates map to `when` (the
* (identical names, descriptions, schemas, annotations and error/payload
* shapes; the one deviation is the packing:bag-deleted payload, aligned with
* the REST route). The registration-time gates map to `when` (the
* whole-registrar packing-addon early return) plus the declarative packing
* read/write access markers (the legacy `if (R)` / `if (W)` checks, resolved
* by trekMcpAccessPolicy). The two template-management tools keep their inline
@@ -232,7 +233,9 @@ export class PackingMcp {
if (!this.packing.verifyTripAccess(tripId, ctx.userId)) return noAccess();
if (!hasTripPermission('packing_edit', tripId, ctx.userId)) return permissionDenied();
this.packing.deleteBag(tripId, bagId);
safeBroadcast(tripId, 'packing:bag-deleted', { id: bagId });
// { bagId } matches the REST route and the plugin host (the legacy
// registrar's { id } was the odd one out).
safeBroadcast(tripId, 'packing:bag-deleted', { bagId });
return ok({ success: true });
}
server/src/nest/packing/packing.service.ts
+59 -43
View File
@@ -27,13 +27,15 @@ interface ImportItem {
const BAG_COLORS = ['#6366f1', '#ec4899', '#f97316', '#10b981', '#06b6d4', '#8b5cf6', '#ef4444', '#f59e0b', '#3b82f6', '#84cc16', '#d946ef', '#14b8a6', '#f43f5e', '#a855f7', '#eab308', '#64748b'];
/**
* Packing domain service — owns the packing SQL (moved 1:1 from the legacy
* services/packingService.ts: identical statements, the `||` falsy-coercion
* defaults, the bodyKeys sentinel protocol on the updates, the #858 three-tier
* sharing model and the post-write re-selects). Trip access, the
* 'packing_edit' permission and the WebSocket broadcast keep their legacy call
* paths. The remaining non-Nest consumer (the legacy MCP prompts registrar)
* goes through packing.bridge.ts instead of importing this class directly.
* Packing domain service — owns the packing SQL (moved from the legacy
* services/packingService.ts: the bodyKeys sentinel protocol on the updates,
* the #858 three-tier sharing model and the post-write re-selects). Trip
* access, the 'packing_edit' permission and the WebSocket broadcast keep their
* legacy call paths. Post-migration fixes over the legacy code: a single
* 'Other' category default, bodyKeys-gated weight_limit_grams (explicit null
* clears it), and transactions around every multi-statement write. The
* remaining non-Nest consumer (the legacy MCP prompts registrar) goes through
* packing.bridge.ts instead of importing this class directly.
*/
@Injectable()
export class PackingService {
@@ -126,8 +128,10 @@ export class PackingService {
listItems(tripId: string | number, userId?: number) {
// Three-tier visibility (#858): Common (is_private=0) is visible to everyone;
// Personal/Shared (is_private=1) only to the owner (bringer) and the recipients
// it was explicitly shared with. Without a userId (internal callers such as
// trip export) the unfiltered list is returned for back-compat.
// it was explicitly shared with. Without a userId the unfiltered list is
// returned — every current caller (trip summary, offline bundle, prompts,
// resources, plugin host) passes the viewer; omit it only for genuinely
// viewer-less internal reads.
let rows: any[];
if (userId == null) {
rows = this.db.all(
@@ -172,7 +176,7 @@ export class PackingService {
const itemId = this.db.transaction(() => {
const result = this.db.run(
'INSERT INTO packing_items (trip_id, name, checked, category, sort_order, quantity, is_private, owner_id, updated_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, CURRENT_TIMESTAMP)',
tripId, data.name, data.checked ? 1 : 0, data.category || 'Allgemein', sortOrder, qty, isPrivate, ownerId ?? null
tripId, data.name, data.checked ? 1 : 0, data.category || 'Other', sortOrder, qty, isPrivate, ownerId ?? null
);
const id = Number(result.lastInsertRowid);
// "Shared with specific people" — record the recipients it covers.
@@ -229,7 +233,7 @@ export class PackingService {
bodyKeys.includes('bag_id') ? 1 : 0,
data.bag_id ?? null,
bodyKeys.includes('quantity') ? 1 : 0,
data.quantity ? Math.max(1, Math.min(999, Number(data.quantity))) : 1,
Math.max(1, Math.min(999, Number(data.quantity) || 1)),
bodyKeys.includes('is_private') ? 1 : 0,
data.is_private ? 1 : 0,
claimOwner ? 1 : 0,
@@ -382,11 +386,13 @@ export class PackingService {
setBagMembers(tripId: string | number, bagId: string | number, userIds: number[]) {
const bag = this.db.get('SELECT * FROM packing_bags WHERE id = ? AND trip_id = ?', bagId, tripId);
if (!bag) return null;
this.db.run('DELETE FROM packing_bag_members WHERE bag_id = ?', bagId);
const ins = this.db.prepare('INSERT OR IGNORE INTO packing_bag_members (bag_id, user_id) VALUES (?, ?)');
// Only real trip members may be bag members — never write an arbitrary account id.
const roster = this.tripRosterIds(tripId);
for (const uid of userIds) if (roster.has(uid)) ins.run(bagId, uid);
this.db.transaction(() => {
this.db.run('DELETE FROM packing_bag_members WHERE bag_id = ?', bagId);
const ins = this.db.prepare('INSERT OR IGNORE INTO packing_bag_members (bag_id, user_id) VALUES (?, ?)');
// Only real trip members may be bag members — never write an arbitrary account id.
const roster = this.tripRosterIds(tripId);
for (const uid of userIds) if (roster.has(uid)) ins.run(bagId, uid);
});
const rows = this.db.all<{ user_id: number; username: string; avatar: string | null }>(`
SELECT bm.user_id, COALESCE(u.display_name, u.username) AS username, u.avatar
FROM packing_bag_members bm JOIN users u ON bm.user_id = u.id
@@ -414,15 +420,18 @@ export class PackingService {
// A bag may only be assigned to a real trip member; an off-roster id becomes unassigned.
const assignUser = data.user_id != null && this.tripRosterIds(tripId).has(data.user_id) ? data.user_id : null;
// weight_limit_grams follows the bodyKeys presence protocol like user_id:
// an omitted key leaves the limit unchanged, an explicit null clears it.
this.db.run(`UPDATE packing_bags SET
name = COALESCE(?, name),
color = COALESCE(?, color),
weight_limit_grams = ?,
weight_limit_grams = CASE WHEN ? THEN ? ELSE weight_limit_grams END,
user_id = CASE WHEN ? THEN ? ELSE user_id END
WHERE id = ?`,
data.name?.trim() || null,
data.color || null,
data.weight_limit_grams ?? (bag as any).weight_limit_grams ?? null,
bodyKeys?.includes('weight_limit_grams') ? 1 : 0,
data.weight_limit_grams ?? null,
bodyKeys?.includes('user_id') ? 1 : 0,
assignUser,
bagId
@@ -479,11 +488,13 @@ export class PackingService {
const insert = this.db.prepare('INSERT INTO packing_items (trip_id, name, checked, category, sort_order, is_private, owner_id, updated_at) VALUES (?, ?, 0, ?, ?, ?, ?, CURRENT_TIMESTAMP)');
const added: any[] = [];
for (const ti of templateItems) {
const result = insert.run(tripId, ti.name, ti.category, sortOrder++, isPrivate, owner);
const item = this.db.get('SELECT * FROM packing_items WHERE id = ?', result.lastInsertRowid);
added.push(item);
}
this.db.transaction(() => {
for (const ti of templateItems) {
const result = insert.run(tripId, ti.name, ti.category, sortOrder++, isPrivate, owner);
const item = this.db.get('SELECT * FROM packing_items WHERE id = ?', result.lastInsertRowid);
added.push(item);
}
});
return added;
}
@@ -497,24 +508,27 @@ export class PackingService {
if (items.length === 0) return null;
const result = this.db.run('INSERT INTO packing_templates (name, created_by) VALUES (?, ?)', templateName, userId);
const templateId = result.lastInsertRowid;
const categories = [...new Set(items.map(i => i.category || 'Other'))];
const catIdMap = new Map<string, number | bigint>();
for (let i = 0; i < categories.length; i++) {
const catResult = this.db.run('INSERT INTO packing_template_categories (template_id, name, sort_order) VALUES (?, ?, ?)', templateId, categories[i], i);
catIdMap.set(categories[i], catResult.lastInsertRowid);
}
const templateId = this.db.transaction(() => {
const result = this.db.run('INSERT INTO packing_templates (name, created_by) VALUES (?, ?)', templateName, userId);
const id = result.lastInsertRowid;
const itemsByCategory = new Map<string, number>();
for (const item of items) {
const catId = catIdMap.get(item.category || 'Other')!;
const order = itemsByCategory.get(item.category || 'Other') || 0;
this.db.run('INSERT INTO packing_template_items (category_id, name, sort_order) VALUES (?, ?, ?)', catId, item.name, order);
itemsByCategory.set(item.category || 'Other', order + 1);
}
const catIdMap = new Map<string, number | bigint>();
for (let i = 0; i < categories.length; i++) {
const catResult = this.db.run('INSERT INTO packing_template_categories (template_id, name, sort_order) VALUES (?, ?, ?)', id, categories[i], i);
catIdMap.set(categories[i], catResult.lastInsertRowid);
}
const itemsByCategory = new Map<string, number>();
for (const item of items) {
const catId = catIdMap.get(item.category || 'Other')!;
const order = itemsByCategory.get(item.category || 'Other') || 0;
this.db.run('INSERT INTO packing_template_items (category_id, name, sort_order) VALUES (?, ?, ?)', catId, item.name, order);
itemsByCategory.set(item.category || 'Other', order + 1);
}
return id;
});
return { id: Number(templateId), name: templateName, categoryCount: categories.length, itemCount: items.length };
}
@@ -540,12 +554,14 @@ export class PackingService {
}
updateCategoryAssignees(tripId: string | number, categoryName: string, userIds: number[] | undefined) {
this.db.run('DELETE FROM packing_category_assignees WHERE trip_id = ? AND category_name = ?', tripId, categoryName);
this.db.transaction(() => {
this.db.run('DELETE FROM packing_category_assignees WHERE trip_id = ? AND category_name = ?', tripId, categoryName);
if (Array.isArray(userIds) && userIds.length > 0) {
const insert = this.db.prepare('INSERT OR IGNORE INTO packing_category_assignees (trip_id, category_name, user_id) VALUES (?, ?, ?)');
for (const uid of userIds) insert.run(tripId, categoryName, uid);
}
if (Array.isArray(userIds) && userIds.length > 0) {
const insert = this.db.prepare('INSERT OR IGNORE INTO packing_category_assignees (trip_id, category_name, user_id) VALUES (?, ?, ?)');
for (const uid of userIds) insert.run(tripId, categoryName, uid);
}
});
const updated = this.db.all<{ user_id: number; username: string; avatar: string | null }>(`
SELECT pca.user_id, COALESCE(u.display_name, u.username) AS username, u.avatar
server/tests/e2e/packing.e2e.test.ts
+10 -2
View File
@@ -195,8 +195,8 @@ describe('Packing e2e (real auth guard + real SQL over temp SQLite)', () => {
insertItem(tripId, 'Existing', { sort_order: 4 });
const res = await request(server).post(`/api/trips/${tripId}/packing`).set('Cookie', sessionCookie(1)).send({ name: 'Socks' });
expect(res.status).toBe(201);
// 'Allgemein' is the legacy createItem category default; quantity clamps to 1.
expect(res.body.item).toMatchObject({ name: 'Socks', checked: 0, category: 'Allgemein', quantity: 1, sort_order: 5, owner_id: 1 });
// 'Other' is the unified category default (shared with bulkImport/saveAsTemplate).
expect(res.body.item).toMatchObject({ name: 'Socks', checked: 0, category: 'Other', quantity: 1, sort_order: 5, owner_id: 1 });
expect(db.prepare('SELECT name FROM packing_items WHERE id = ?').get(res.body.item.id)).toEqual({ name: 'Socks' });
});
@@ -321,6 +321,14 @@ describe('Packing e2e (real auth guard + real SQL over temp SQLite)', () => {
expect(updated.status).toBe(200);
expect(updated.body.bag).toMatchObject({ name: 'Duffel', color: '#ff0000' });
// weight_limit_grams follows the bodyKeys protocol: set, keep when omitted, clear with null.
const limited = await request(server).put(`/api/trips/${tripId}/packing/bags/${bagId}`).set('Cookie', sessionCookie(1)).send({ weight_limit_grams: 8000 });
expect(limited.body.bag.weight_limit_grams).toBe(8000);
const kept = await request(server).put(`/api/trips/${tripId}/packing/bags/${bagId}`).set('Cookie', sessionCookie(1)).send({ name: 'Duffel XL' });
expect(kept.body.bag.weight_limit_grams).toBe(8000);
const cleared = await request(server).put(`/api/trips/${tripId}/packing/bags/${bagId}`).set('Cookie', sessionCookie(1)).send({ weight_limit_grams: null });
expect(cleared.body.bag.weight_limit_grams).toBeNull();
const deleted = await request(server).delete(`/api/trips/${tripId}/packing/bags/${bagId}`).set('Cookie', sessionCookie(1));
expect(deleted.status).toBe(200);
expect(deleted.body).toEqual({ success: true });
server/tests/unit/mcp/tools-packing-advanced.test.ts
+2 -1
View File
@@ -235,7 +235,8 @@ describe('Tool: delete_packing_bag', () => {
});
const data = parseToolResult(result) as any;
expect(data.success).toBe(true);
expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'packing:bag-deleted', expect.any(Object));
// { bagId } — aligned with the REST route and the plugin host.
expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'packing:bag-deleted', expect.objectContaining({ bagId }));
expect(testDb.prepare('SELECT id FROM packing_bags WHERE id = ?').get(bagId)).toBeUndefined();
});
});
server/tests/unit/nest/packing.service.test.ts
+43 -2
View File
@@ -1,10 +1,12 @@
/**
* Unit tests for the DI-native PackingService — PACK-SVC-001 through
* PACK-SVC-049 moved 1:1 from the legacy tests/unit/services/packingService.test.ts
* PACK-SVC-049 moved from the legacy tests/unit/services/packingService.test.ts
* (real in-memory SQLite so the SQL logic is exercised faithfully), plus the
* wrapper-helper cases (canEdit, the #858 broadcast scoping, getItemPrivacy,
* notifyTagged) carried over from the old delegation suite — now running
* against real SQL — and PACK-SVC-050 pinning the packing.bridge delegation.
* against real SQL — PACK-SVC-050 pinning the packing.bridge delegation, and
* PACK-SVC-051..053 pinning the post-migration fixes over the legacy quirks
* ('Other' category default, bodyKeys-gated weight_limit_grams, quantity clamp).
*/
import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest';
@@ -576,6 +578,45 @@ describe('three-tier packing sharing (#858)', () => {
});
});
// ── Post-migration fixes over the legacy quirks ───────────────────────────────
describe('legacy-quirk fixes', () => {
it('PACK-SVC-051: createItem defaults the category to "Other" (unified with bulkImport)', () => {
const { user } = createUser(testDb);
const trip = createTrip(testDb, user.id);
const item = svc.createItem(trip.id, { name: 'Socks' }, user.id) as any;
expect(item.category).toBe('Other');
});
it('PACK-SVC-052: updateBag gates weight_limit_grams on bodyKeys — omitted keeps, explicit null clears', () => {
const { user } = createUser(testDb);
const trip = createTrip(testDb, user.id);
const bag = svc.createBag(trip.id, { name: 'Duffel' }) as any;
const limited = svc.updateBag(trip.id, bag.id, { weight_limit_grams: 8000 }, ['weight_limit_grams']) as any;
expect(limited.weight_limit_grams).toBe(8000);
// A rename without the key must not touch the limit.
const kept = svc.updateBag(trip.id, bag.id, { name: 'Duffel XL' }, ['name']) as any;
expect(kept.weight_limit_grams).toBe(8000);
// An explicit null clears it.
const cleared = svc.updateBag(trip.id, bag.id, { weight_limit_grams: null }, ['weight_limit_grams']) as any;
expect(cleared.weight_limit_grams).toBeNull();
});
it('PACK-SVC-053: updateItem clamps a provided quantity into 1..999', () => {
const { user } = createUser(testDb);
const trip = createTrip(testDb, user.id);
const item = svc.createItem(trip.id, { name: 'Socks', quantity: 5 }, user.id) as any;
expect((svc.updateItem(trip.id, item.id, { quantity: 0 }, ['quantity']) as any).quantity).toBe(1);
expect((svc.updateItem(trip.id, item.id, { quantity: 9999 }, ['quantity']) as any).quantity).toBe(999);
// Omitted key leaves the quantity unchanged.
expect((svc.updateItem(trip.id, item.id, { name: 'Wool socks' }, ['name']) as any).quantity).toBe(999);
});
});
// ── Wrapper helpers (carried over from the old delegation suite) ──────────────
describe('canEdit', () => {