From 1da59abb2dc57cfa5dead45333106741d698f075 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Mon, 5 Feb 2024 15:31:04 +0100 Subject: [PATCH] fix: inactive users query was too wide (#6133) Knex wasn't formatting the query like I expected. This changes the query to use more AND, less ORs --- src/lib/users/inactive/inactive-users-store.ts | 15 +++++---------- .../users/inactive/inactive-users-service.test.ts | 14 +++++++++++++- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/lib/users/inactive/inactive-users-store.ts b/src/lib/users/inactive/inactive-users-store.ts index 2f6a108562..f55124e7dc 100644 --- a/src/lib/users/inactive/inactive-users-store.ts +++ b/src/lib/users/inactive/inactive-users-store.ts @@ -42,20 +42,15 @@ export class InactiveUsersStore implements IInactiveUsersStore { ) .leftJoin( 'personal_access_tokens AS pat', - 'users.id', 'pat.user_id', + 'users.id', ) .where('deleted_at', null) - .andWhereRaw(`users.seen_at < now() - INTERVAL '?? DAYS'`, [ - daysInactive, - ]) - .orWhereRaw( - `users.seen_at IS NULL AND users.created_at < now() - INTERVAL '?? DAYS'`, - [daysInactive], - ) .andWhereRaw( - `pat.seen_at IS NULL OR pat.seen_at < now() - INTERVAL '?? DAYS'`, - [daysInactive], + `(users.seen_at IS NULL OR users.seen_at < now() - INTERVAL '?? days') + AND (users.created_at IS NULL OR users.created_at < now() - INTERVAL '?? days') + AND (pat.seen_at IS NULL OR pat.seen_at < now() - INTERVAL '?? days')`, + [daysInactive, daysInactive, daysInactive], ); stopTimer(); return inactiveUsers; diff --git a/src/test/e2e/users/inactive/inactive-users-service.test.ts b/src/test/e2e/users/inactive/inactive-users-service.test.ts index 49a7b452dd..7269b9ba8f 100644 --- a/src/test/e2e/users/inactive/inactive-users-service.test.ts +++ b/src/test/e2e/users/inactive/inactive-users-service.test.ts @@ -110,7 +110,8 @@ describe('Inactive users service', () => { VALUES (9595, 'test user with active PAT', 'nedryerson', 'ned@ryerson.com', now() - INTERVAL '200 DAYS')`); await db.rawDatabase.raw( - `INSERT INTO personal_access_tokens(secret, user_id, expires_at, seen_at, created_at) VALUES ('user:somefancysecret', 9595, now() + INTERVAL '6 MONTHS', now() - INTERVAL '1 WEEK', now() - INTERVAL '8 MONTHS')`, + `INSERT INTO personal_access_tokens(secret, user_id, expires_at, seen_at, created_at) + VALUES ('user:somefancysecret', 9595, now() + INTERVAL '6 MONTHS', now() - INTERVAL '1 WEEK', now() - INTERVAL '8 MONTHS')`, ); const users = await inactiveUserService.getInactiveUsers(); expect(users).toBeTruthy(); @@ -127,6 +128,17 @@ describe('Inactive users service', () => { expect(users).toBeTruthy(); expect(users).toHaveLength(1); }); + test('A user with a pat that was seen 7 months ago, but logged in yesterday should not be inactive', async () => { + await db.rawDatabase.raw( + `INSERT INTO users(id, name, username, email, created_at, seen_at) VALUES (9595, 'test user with active login and old PAT', 'nedryerson', 'ned@ryerson.com', now() - INTERVAL '1 YEAR', now() - INTERVAL '1 DAY')`, + ); + await db.rawDatabase.raw( + `INSERT INTO personal_access_tokens(secret, user_id, expires_at, seen_at, created_at) VALUES ('user:somefancysecret', 9595, now() + INTERVAL '6 MONTHS', now() - INTERVAL '7 MONTHS', now() - INTERVAL '8 MONTHS')`, + ); + const users = await inactiveUserService.getInactiveUsers(); + expect(users).toBeTruthy(); + expect(users).toHaveLength(0); + }); }); describe('Deleting inactive users', () => { test('Deletes users that have never logged in but was created before our deadline', async () => {