From ecce181401123b70907eb8632229f0def7dffd90 Mon Sep 17 00:00:00 2001 From: Thomas Cottee Meldrum Date: Fri, 3 Oct 2025 16:29:59 +0100 Subject: [PATCH] fix: remove auto instrument removal for stfc --- .../src/auth/StfcUserAuthorization.spec.ts | 101 +----------------- .../backend/src/auth/StfcUserAuthorization.ts | 52 +-------- 2 files changed, 9 insertions(+), 144 deletions(-) diff --git a/apps/backend/src/auth/StfcUserAuthorization.spec.ts b/apps/backend/src/auth/StfcUserAuthorization.spec.ts index cf90c6d3f4..34bfe69915 100644 --- a/apps/backend/src/auth/StfcUserAuthorization.spec.ts +++ b/apps/backend/src/auth/StfcUserAuthorization.spec.ts @@ -96,11 +96,6 @@ const instrumentDataSource = container.resolve( Tokens.InstrumentDataSource ) as InstrumentDataSource; -const mockRemoveScientistFromInstruments = jest.spyOn( - instrumentDataSource, - 'removeScientistFromInstruments' -); - const mockAssignScientistToInstruments = jest.spyOn( instrumentDataSource, 'assignScientistToInstruments' @@ -132,7 +127,6 @@ beforeAll(() => { beforeEach(() => { mockAssignScientistToInstruments.mockClear(); - mockRemoveScientistFromInstruments.mockClear(); mockGetRequiredInstrumentForRole.mockClear(); }); @@ -230,108 +224,23 @@ test('When a user requests multiple instruments, only new existing ones are adde expect(result).toEqual([lsfInstrument.id]); }); -// getInstrumentsToRemove -test('When a user has an instrument they have not requested, the instrument is removed', async () => { - const requiredInstruments: string[] = []; - const currentInstruments = [isisInstrument]; - - const result = await userAuthorization.getInstrumentsToRemove( - requiredInstruments, - currentInstruments - ); - - expect(result).toEqual([isisInstrument.id]); -}); - -test('When a user has an instrument they have requested, no instrument is removed', async () => { - const requiredInstruments = [isisInstrument.name]; - const currentInstruments = [isisInstrument]; - - const result = await userAuthorization.getInstrumentsToRemove( - requiredInstruments, - currentInstruments - ); - - expect(result).toEqual([]); -}); - -test('When a user does not have an instrument they requested, no instrument is removed', async () => { - const requiredInstruments = [isisInstrument.name]; - const currentInstruments: Instrument[] = []; - - const result = await userAuthorization.getInstrumentsToRemove( - requiredInstruments, - currentInstruments - ); - - expect(result).toEqual([]); -}); - -test('When a user requests a nonexisting instrument, all other instruments are removed', async () => { - const requiredInstruments = [nonExistingInstrumentName]; - const currentInstruments = [isisInstrument, lsfInstrument]; - - const result = await userAuthorization.getInstrumentsToRemove( - requiredInstruments, - currentInstruments - ); - - expect(result).toEqual([isisInstrument.id, lsfInstrument.id]); -}); - -test('When a user requests multiple instrument, only non requested ones are removed', async () => { - const requiredInstruments = [isisInstrument.name]; - const currentInstruments: Instrument[] = [isisInstrument, lsfInstrument]; - - const result = await userAuthorization.getInstrumentsToRemove( - requiredInstruments, - currentInstruments - ); - - expect(result).toEqual([lsfInstrument.id]); -}); - //autoAssignRemoveInstruments -test('When a user requires an instrument but does not have it, the instrument is assigned and no instrument are removed', async () => { - await userAuthorization.autoAssignRemoveInstruments( - 0, - [isisInstrument.name], - [], - true - ); +test('When a user requires an instrument but does not have it, the instrument is assigned', async () => { + await userAuthorization.autoAssignInstruments(0, [isisInstrument.name], []); expect(mockAssignScientistToInstruments).toHaveBeenCalledWith(0, [ isisInstrument.id, ]); - expect(mockRemoveScientistFromInstruments).toHaveBeenCalledTimes(0); }); -test('When a user does not require an instrument but has it, no instrument is assigned and the instrument is removed', async () => { - await userAuthorization.autoAssignRemoveInstruments( - 0, - [], - [isisInstrument], - true - ); - - expect(mockAssignScientistToInstruments).toHaveBeenCalledTimes(0); - expect(mockRemoveScientistFromInstruments).toHaveBeenCalledWith(0, [ - isisInstrument.id, - ]); -}); - -test('When a user requires an instrument but has a different one, the requested instrument is assigned and the current instrument is removed', async () => { - await userAuthorization.autoAssignRemoveInstruments( +test('When a user requires an instrument but has a different one, the requested instrument is assigned and the current instrument stays the same', async () => { + await userAuthorization.autoAssignInstruments( 0, [isisInstrument.name], - [lsfInstrument], - true + [lsfInstrument] ); expect(mockAssignScientistToInstruments).toHaveBeenCalledWith(0, [ isisInstrument.id, ]); - expect(mockRemoveScientistFromInstruments).toHaveBeenCalledWith(0, [ - lsfInstrument.id, - ]); }); diff --git a/apps/backend/src/auth/StfcUserAuthorization.ts b/apps/backend/src/auth/StfcUserAuthorization.ts index 986be9c78d..2a6125ccaa 100644 --- a/apps/backend/src/auth/StfcUserAuthorization.ts +++ b/apps/backend/src/auth/StfcUserAuthorization.ts @@ -102,26 +102,10 @@ export class StfcUserAuthorization extends UserAuthorization { return instrumentsToAssign.map((instrument) => instrument.id); } - async getInstrumentsToRemove( - requiredInstrumentNames: string[], - currentInstruments: Instrument[] - ) { - // Remove instrument scientist from any instruments they are assigned to, - // but shouldn't be. - // We don't need to check if the instruments exist here, - // because we fetch the list of currently assigned instruments from the DB - return currentInstruments - .filter( - (instrument) => !requiredInstrumentNames.includes(instrument.name) - ) - .map((instrument) => instrument.id); - } - - async autoAssignRemoveInstruments( + async autoAssignInstruments( userId: number, requiredInstrumentNames: string[], - currentInstruments: Instrument[], - removeInstruments: boolean + currentInstruments: Instrument[] ) { // Assign any instruments which aren't currently assigned const instrumentsToAdd = await this.getInstrumentsToAdd( @@ -142,28 +126,6 @@ export class StfcUserAuthorization extends UserAuthorization { instrumentsToAdd ); } - - if (removeInstruments) { - // Remove any instruments which are currently assigned, but not required - const instrumentsToRemove = await this.getInstrumentsToRemove( - requiredInstrumentNames, - currentInstruments - ); - - if (instrumentsToRemove.length > 0) { - logger.logInfo( - 'Auto-removing STFC instrument scientist from instruments', - { - instruments: instrumentsToRemove, - } - ); - - this.instrumentDataSource.removeScientistFromInstruments( - userId, - instrumentsToRemove - ); - } - } } async externalTokenLogin( @@ -250,16 +212,10 @@ export class StfcUserAuthorization extends UserAuthorization { await this.instrumentDataSource.getUserInstruments(userNumber); // Don't remove instruments from ISIS Staff and User officers as these will be manually assigned - this.autoAssignRemoveInstruments( + this.autoAssignInstruments( userNumber, requiredInstruments, - currentUserInstruments, - !uniqueRoles.find( - (role) => - role.name === 'ISIS Instrument Scientist' || - role.name === 'CLF LSF Link Scientist' || - role.name === 'User Officer' - ) + currentUserInstruments ); }