diff --git a/server/src/nest/addons/addons.service.ts b/server/src/nest/addons/addons.service.ts index 4d2793438..827725b09 100644 --- a/server/src/nest/addons/addons.service.ts +++ b/server/src/nest/addons/addons.service.ts @@ -174,4 +174,26 @@ export class AddonsService { updatePlacesAutocomplete(enabled: boolean) { return this.writeFlag('places_autocomplete_enabled', enabled); } getPlacesDetails() { return this.readFlag('places_details_enabled'); } updatePlacesDetails(enabled: boolean) { return this.writeFlag('places_details_enabled', enabled); } + + /** + * Enrichment reads fail-OPEN, unlike the three switches above. + * + * Those needed migration 185 to backfill 'true' precisely because they read + * `=== 'true'`: without a row, an install that had been happily using the + * feature would have lost it on upgrade. This switch is new, so there is no + * row to backfill anywhere and no migration worth writing for one boolean — + * reading it the other way round gets the same outcome for free. + * + * It has to agree with PlaceEnrichmentService.enrichDisabled(), which reads + * the same key the same way. If these two ever disagree the admin panel shows + * "off" while the feature runs, which is worse than either default. + */ + getPlacesEnrich() { + const row = this.db.prepare("SELECT value FROM app_settings WHERE key = 'places_enrich_enabled'").get() as + | { value: string } + | undefined; + return { enabled: row?.value !== 'false' }; + } + + updatePlacesEnrich(enabled: boolean) { return this.writeFlag('places_enrich_enabled', enabled); } } diff --git a/server/src/nest/admin/admin.controller.ts b/server/src/nest/admin/admin.controller.ts index c4a069377..d8d24af50 100644 --- a/server/src/nest/admin/admin.controller.ts +++ b/server/src/nest/admin/admin.controller.ts @@ -214,11 +214,11 @@ export class AdminController { } @Get('places-enrich') - getPlacesEnrich() { return this.admin.getPlacesEnrich(); } + getPlacesEnrich() { return this.addons.getPlacesEnrich(); } @Put('places-enrich') updatePlacesEnrich(@CurrentUser() user: User, @Body() body: AdminFeatureToggleDto, @Req() req: Request) { - const result = this.admin.updatePlacesEnrich(body.enabled); + const result = this.addons.updatePlacesEnrich(body.enabled); this.audit.writeAudit({ userId: user.id, action: 'admin.places_enrich', ip: getClientIp(req), details: { enabled: result.enabled } }); return result; } diff --git a/server/tests/unit/nest/addons.service.test.ts b/server/tests/unit/nest/addons.service.test.ts index b3a4f58ed..81ac0a2b3 100644 --- a/server/tests/unit/nest/addons.service.test.ts +++ b/server/tests/unit/nest/addons.service.test.ts @@ -343,3 +343,41 @@ describe('AddonsService places flags', () => { } }); }); + +/** + * Enrichment sits beside the three above and reads the opposite way round. + * + * They are fail-closed because a migration backfilled a row for every install + * that predates the change. This one is new: there is nothing to backfill, so + * fail-open reaches the same place without a migration for one boolean. What + * matters is that it agrees with PlaceEnrichmentService.enrichDisabled(), which + * reads the same key — if the two ever diverge the panel shows "off" while the + * feature runs. + */ +describe('AddonsService places enrichment flag', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('ADDONS-SVC-083 an unset flag reads as ON', () => { + dbMock._stmt.get.mockReturnValueOnce(undefined); + expect(svc().getPlacesEnrich()).toEqual({ enabled: true }); + }); + + it('ADDONS-SVC-084 only the literal "false" switches it off', () => { + dbMock._stmt.get.mockReturnValueOnce({ value: 'false' }); + expect(svc().getPlacesEnrich()).toEqual({ enabled: false }); + + for (const value of ['true', 'garbage', '']) { + dbMock._stmt.get.mockReturnValueOnce({ value }); + expect(svc().getPlacesEnrich()).toEqual({ enabled: true }); + } + }); + + it('ADDONS-SVC-085 the setter persists the literal string and echoes the boolean back', () => { + expect(svc().updatePlacesEnrich(false)).toEqual({ enabled: false }); + expect(dbMock._stmt.run).toHaveBeenLastCalledWith('places_enrich_enabled', 'false'); + expect(svc().updatePlacesEnrich(true)).toEqual({ enabled: true }); + expect(dbMock._stmt.run).toHaveBeenLastCalledWith('places_enrich_enabled', 'true'); + }); +}); diff --git a/server/tests/unit/nest/admin.controller.test.ts b/server/tests/unit/nest/admin.controller.test.ts index e217162ce..847b13318 100644 --- a/server/tests/unit/nest/admin.controller.test.ts +++ b/server/tests/unit/nest/admin.controller.test.ts @@ -41,6 +41,9 @@ const addonsStub = () => ({ updatePlacesAutocomplete: vi.fn((enabled: boolean) => ({ enabled })), getPlacesDetails: vi.fn(() => ({ enabled: false })), updatePlacesDetails: vi.fn((enabled: boolean) => ({ enabled })), + // Fail-open, unlike its three neighbours — see AddonsService.getPlacesEnrich. + getPlacesEnrich: vi.fn(() => ({ enabled: true })), + updatePlacesEnrich: vi.fn((enabled: boolean) => ({ enabled })), getCollabFeatures: vi.fn(() => ({ chat: false })), updateCollabFeatures: vi.fn(() => ({ features: { chat: true }, changed: true })), }) as unknown as AddonsService; @@ -242,9 +245,16 @@ describe('AdminController feature toggles', () => { expect(c.getPlacesPhotos()).toEqual({ enabled: false }); expect(c.getPlacesAutocomplete()).toEqual({ enabled: false }); expect(c.getPlacesDetails()).toEqual({ enabled: false }); + expect(c.getPlacesEnrich()).toEqual({ enabled: true }); expect(c.getCollabFeatures()).toEqual({ chat: false }); }); + it('ADMIN-TOGGLE-002b places-enrich updates through the addons domain and is audited', () => { + const c = adminCtl(svc()); + expect(c.updatePlacesEnrich(user, { enabled: false }, req)).toEqual({ enabled: false }); + expect(writeAudit).toHaveBeenCalledWith(expect.objectContaining({ action: 'admin.places_enrich' })); + }); + it('ADMIN-TOGGLE-003 collab-features invalidates MCP sessions only when a flag flipped (#1414)', () => { const invalidateMcpSessions = vi.fn(); const c = adminCtl(svc({ invalidateMcpSessions } as Partial)); diff --git a/server/tests/unit/nest/admin.service.test.ts b/server/tests/unit/nest/admin.service.test.ts index a4640418c..3a2edcb75 100644 --- a/server/tests/unit/nest/admin.service.test.ts +++ b/server/tests/unit/nest/admin.service.test.ts @@ -505,23 +505,6 @@ describe('admin.bridge', () => { describe('admin quirk fixes (post-fold)', () => { - it('ADMIN-SVC-071b — places enrichment is fail-OPEN, so no migration is needed for it', () => { - // Deliberately the other way round from the three above: unset means on, and - // PlaceEnrichmentService reads the same setting the same way. Reading it - // fail-closed here would show "off" in the panel while the feature ran. - expect(svc.getPlacesEnrich()).toEqual({ enabled: true }); - - svc.updatePlacesEnrich(false); - expect(svc.getPlacesEnrich()).toEqual({ enabled: false }); - - svc.updatePlacesEnrich(true); - expect(svc.getPlacesEnrich()).toEqual({ enabled: true }); - - // Only the literal 'false' switches it off; anything else stays on. - testDb.prepare("INSERT OR REPLACE INTO app_settings (key, value) VALUES ('places_enrich_enabled', 'garbage')").run(); - expect(svc.getPlacesEnrich()).toEqual({ enabled: true }); - }); - it('ADMIN-SVC-072 — updateUser rejects an empty username/email instead of silently no-opping', () => { const { user } = createUser(testDb); expect(updateUser(String(user.id), { username: '' }) as any).toMatchObject({ status: 400, error: 'Username cannot be empty' });