Move the enrichment switch to the domain that owns the places flags

This commit is contained in:
Maurice
2026-08-10 11:07:21 +02:00
committed by Maurice
parent e3b220603c
commit 92e4eaa476
5 changed files with 72 additions and 19 deletions
+22
View File
@@ -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); }
}
+2 -2
View File
@@ -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;
}
@@ -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');
});
});
@@ -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<AdminService>));
@@ -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' });