Skip to content

fix: remove auto instrument removal for stfc #1212

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
TCMeldrum merged 3 commits into from
Oct 28, 2025
Merged

fix: remove auto instrument removal for stfc #1212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ecce181
fix: remove auto instrument removal for stfc
TCMeldrum Oct 3, 2025
677fed4
Merge branch 'develop' into 1453-remove-auto-stfc-instr-removel
TCMeldrum Oct 6, 2025
43f8ef4
Merge branch 'develop' into 1453-remove-auto-stfc-instr-removel
bolmsten Oct 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 5 additions & 96 deletions apps/backend/src/auth/StfcUserAuthorization.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,6 @@ const instrumentDataSource = container.resolve(
Tokens.InstrumentDataSource
) as InstrumentDataSource;

const mockRemoveScientistFromInstruments = jest.spyOn(
instrumentDataSource,
'removeScientistFromInstruments'
);

const mockAssignScientistToInstruments = jest.spyOn(
instrumentDataSource,
'assignScientistToInstruments'
Expand Down Expand Up @@ -132,7 +127,6 @@ beforeAll(() => {

beforeEach(() => {
mockAssignScientistToInstruments.mockClear();
mockRemoveScientistFromInstruments.mockClear();
mockGetRequiredInstrumentForRole.mockClear();
});

Expand Down Expand Up @@ -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,
]);
});
52 changes: 4 additions & 48 deletions apps/backend/src/auth/StfcUserAuthorization.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand Down Expand Up @@ -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
);
}

Expand Down