fix: don't count flags multiple times (bonus: don't count non-project events) (#7931)

This PR fixes an issue where the number of flags belonging to a project
was wrong in the new getProjectsForAdminUi.

The cause was that we now join with the events table to get the most
"lastUpdatedAt" data. This meant that we got multiple rows for each
flag, so we counted the same flag multiple times. The fix was to use a
"distinct".

Additionally, I used this as an excuse to write some more tests that I'd
been thinking about. And in doing so also uncovered another bug that
would only ever surface in verrry rare conditions: if a flag had been
created in project A, but moved to project B AND the
feature-project-change event hadn't fired correctly, project B's last
updated could show data from that feature in project A.

I've also taken the liberty of doing a little bit of cleanup.

src/lib/features/project/project-owners-read-model.test.ts

@@ -20,7 +20,7 @@ const mockProjectData = (name: string): ProjectForUi => ({
     createdAt: new Date(),
     favorite: false,
     lastReportedFlagUsage: null,
-    lastFlagUpdate: null,
+    lastUpdatedAt: null,
 });
 
 describe('unit tests', () => {

src/lib/features/project/project-read-model-type.ts

@@ -13,7 +13,7 @@ export type ProjectForUi = {
     archivedAt?: Date;
     featureCount: number;
     lastReportedFlagUsage: Date | null;
-    lastFlagUpdate: Date | null;
+    lastUpdatedAt: Date | null;
 };
 
 // @todo remove with flag useProjectReadModel

src/lib/features/project/project-read-model.test.ts

@@ -0,0 +1,159 @@
+import dbInit, { type ITestDb } from '../../../test/e2e/helpers/database-init';
+import getLogger from '../../../test/fixtures/no-logger';
+import type { IFeatureToggleStore } from '../feature-toggle/types/feature-toggle-store-type';
+import type {
+    IEventStore,
+    IFlagResolver,
+    IProjectReadModel,
+    IProjectStore,
+} from '../../types';
+import { ProjectReadModel } from './project-read-model';
+import type EventEmitter from 'events';
+
+let db: ITestDb;
+let flagStore: IFeatureToggleStore;
+let projectStore: IProjectStore;
+let eventStore: IEventStore;
+let projectReadModel: IProjectReadModel;
+
+const alwaysOnFlagResolver = {
+    isEnabled() {
+        return true;
+    },
+} as unknown as IFlagResolver;
+
+beforeAll(async () => {
+    db = await dbInit('feature_lifecycle_read_model', getLogger);
+    projectReadModel = new ProjectReadModel(
+        db.rawDatabase,
+        { emit: () => {} } as unknown as EventEmitter,
+        alwaysOnFlagResolver,
+    );
+    projectStore = db.stores.projectStore;
+    eventStore = db.stores.eventStore;
+    flagStore = db.stores.featureToggleStore;
+});
+
+afterAll(async () => {
+    if (db) {
+        await db.destroy();
+    }
+});
+
+beforeEach(async () => {
+    await projectStore.deleteAll();
+    await flagStore.deleteAll();
+});
+
+test("it doesn't count flags multiple times when they have multiple events associated with them", async () => {
+    const projectId = 'project';
+    const flagName = 'flag';
+    await projectStore.create({ id: projectId, name: projectId });
+
+    await flagStore.create(projectId, { name: flagName, createdByUserId: 1 });
+
+    await eventStore.store({
+        type: 'feature-created',
+        createdBy: 'admin',
+        ip: '',
+        createdByUserId: 1,
+        featureName: flagName,
+        project: projectId,
+    });
+    await eventStore.store({
+        type: 'feature-updated',
+        createdBy: 'admin',
+        ip: '',
+        createdByUserId: 1,
+        featureName: flagName,
+        project: projectId,
+    });
+
+    const withFlags = await projectReadModel.getProjectsForAdminUi();
+    expect(withFlags).toMatchObject([{ id: projectId, featureCount: 1 }]);
+
+    const insightsQuery = await projectReadModel.getProjectsForInsights();
+    expect(insightsQuery).toMatchObject([{ id: projectId, featureCount: 1 }]);
+});
+
+test('it uses the last flag change for an flag in the project as lastUpdated', async () => {
+    const projectId = 'project';
+    const flagName = 'flag';
+    await projectStore.create({ id: projectId, name: projectId });
+    await eventStore.store({
+        type: 'project-created',
+        createdBy: 'admin',
+        createdByUserId: 1,
+        project: projectId,
+        ip: '',
+    });
+
+    const noEvents = await projectReadModel.getProjectsForAdminUi();
+
+    expect(noEvents[0].lastUpdatedAt).toBeNull();
+
+    await flagStore.create(projectId, { name: flagName, createdByUserId: 1 });
+    await eventStore.store({
+        type: 'feature-created',
+        createdBy: 'admin',
+        ip: '',
+        createdByUserId: 1,
+        featureName: flagName,
+        project: projectId,
+    });
+
+    const withEvents = await projectReadModel.getProjectsForAdminUi();
+    expect(withEvents[0].lastUpdatedAt).not.toBeNull();
+});
+
+test('it does not consider flag events in a different project for lastUpdatedAt, and does not count flags belonging to a different project', async () => {
+    const projectId1 = 'project1';
+    await projectStore.create({ id: projectId1, name: 'Project1' });
+
+    const projectId2 = 'project2';
+    await projectStore.create({ id: projectId2, name: 'Project2' });
+
+    const flagName = 'flag';
+    await flagStore.create(projectId1, { name: flagName, createdByUserId: 1 });
+    await eventStore.store({
+        type: 'feature-created',
+        createdBy: 'admin',
+        ip: '',
+        createdByUserId: 1,
+        featureName: flagName,
+        project: projectId2,
+    });
+
+    const withEvents = await projectReadModel.getProjectsForAdminUi();
+
+    expect(withEvents).toMatchObject([
+        // no events for the flag in this project (i.e. if a flag
+        // has been moved from one project to the next (before the
+        // moving event has been counted)). In practice, you'll
+        // always get a "feature-project-change" event to count
+        { id: projectId1, lastUpdatedAt: null },
+        // this flag no longer exists in this project because it
+        // has been moved, so it should not be counted
+        { id: projectId2, lastUpdatedAt: null },
+    ]);
+});
+
+test('it uses the last flag metrics received for lastReportedFlagUsage', async () => {
+    const projectId = 'project';
+    const flagName = 'flag';
+    await projectStore.create({ id: projectId, name: projectId });
+
+    const noUsage = await projectReadModel.getProjectsForAdminUi();
+    expect(noUsage[0].lastReportedFlagUsage).toBeNull();
+
+    await flagStore.create(projectId, { name: flagName, createdByUserId: 1 });
+
+    await flagStore.setLastSeen([
+        { featureName: flagName, environment: 'development' },
+    ]);
+
+    const flag = await flagStore.get(flagName);
+
+    const result = await projectReadModel.getProjectsForAdminUi();
+    expect(result[0].lastReportedFlagUsage).toEqual(flag.lastSeenAt);
+});

src/lib/features/project/project-read-model.ts

@@ -28,7 +28,7 @@ const mapProjectForUi = (row): ProjectForUi => {
         archivedAt: row.archived_at,
         mode: row.project_mode || 'open',
         lastReportedFlagUsage: row.last_usage,
-        lastFlagUpdate: row.last_updated,
+        lastUpdatedAt: row.last_updated,
     };
 };
 
@@ -74,7 +74,13 @@ export class ProjectReadModel implements IProjectReadModel {
                 'project_settings.project',
                 'projects.id',
             )
-            .leftJoin('events', 'events.feature_name', 'features.name')
+            .leftJoin('events', (join) => {
+                join.on('events.feature_name', '=', 'features.name').andOn(
+                    'events.project',
+                    '=',
+                    'projects.id',
+                );
+            })
             .orderBy('projects.name', 'asc');
 
         if (this.flagResolver.isEnabled('archiveProjects')) {
@@ -92,7 +98,7 @@ export class ProjectReadModel implements IProjectReadModel {
         let selectColumns = [
             this.db.raw(
                 'projects.id, projects.name, projects.description, projects.health, projects.created_at, ' +
-                    'count(features.name) FILTER (WHERE features.archived_at is null) AS number_of_features, ' +
+                    'count(DISTINCT features.name) FILTER (WHERE features.archived_at is null) AS number_of_features, ' +
                     'MAX(features.last_seen_at) AS last_usage,' +
                     'MAX(events.created_at) AS last_updated',
             ),
@@ -169,7 +175,7 @@ export class ProjectReadModel implements IProjectReadModel {
                 'projects.id, projects.health, ' +
                     'count(features.name) FILTER (WHERE features.archived_at is null) AS number_of_features, ' +
                     'count(features.name) FILTER (WHERE features.archived_at is null and features.stale IS TRUE) AS stale_feature_count, ' +
-                    'count(features.name) FILTER (WHERE features.archived_at is null and features.potentially_stale IS TRUE) AS potentially_stale_feature_count,',
+                    'count(features.name) FILTER (WHERE features.archived_at is null and features.potentially_stale IS TRUE) AS potentially_stale_feature_count',
             ),
             'project_stats.avg_time_to_prod_current_window',
         ] as (string | Raw<any>)[];

src/lib/types/events.ts

@@ -354,7 +354,7 @@ export const IEventTypes = [
 ] as const;
 export type IEventType = (typeof IEventTypes)[number];
 
-// this rerpresents the write model for events
+// this represents the write model for events
 export interface IBaseEvent {
     type: IEventType;
     createdBy: string;