Skip to content
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.

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

SIMSBIOHUB-646: Add Device Keys Button to Manage Deployments & Misc UI Updates #1440

Merged
merged 5 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { expect } from 'chai';
import sinon from 'sinon';
import { getDevicesInSurvey } from '.';
import { createDevice, getDevicesInSurvey } from '.';
import * as db from '../../../../../../database/db';
import { ApiError, ApiGeneralError } from '../../../../../../errors/api-error';
import { TelemetryDeviceService } from '../../../../../../services/telemetry-services/telemetry-device-service';
import { getMockDBConnection, getRequestHandlerMocks } from '../../../../../../__mocks__/db';

Expand Down Expand Up @@ -82,4 +83,142 @@ describe('getDevicesInSurvey', () => {
expect(mockDBConnection.release).to.have.been.calledOnce;
}
});

describe('createDevice', () => {
afterEach(() => {
sinon.restore();
});

it('successfully creates a device', async () => {
const mockDBConnection = getMockDBConnection({
release: sinon.stub(),
rollback: sinon.stub(),
commit: sinon.stub()
});
sinon.stub(db, 'getDBConnection').returns(mockDBConnection);

const mockSurveyId = '2';
const mockSerial = '123456';
const mockDeviceMakeId = 1;
const mockModel = 'ModelX';
const mockComment = 'Device Comment';

const mockDevice = null; // Device doesn't exist, so no error should be thrown

sinon.stub(TelemetryDeviceService.prototype, 'findDeviceBySerial').resolves(mockDevice);
sinon.stub(TelemetryDeviceService.prototype, 'createDevice').resolves();

const { mockReq, mockRes, mockNext } = getRequestHandlerMocks();

mockReq.params = {
surveyId: mockSurveyId
};

mockReq.body = {
serial: mockSerial,
device_make_id: mockDeviceMakeId,
model: mockModel,
comment: mockComment
};

const requestHandler = createDevice();

await requestHandler(mockReq, mockRes, mockNext);

expect(mockRes.status).to.have.been.calledOnceWith(200);
expect(mockRes.send).to.have.been.calledOnce;
expect(mockDBConnection.release).to.have.been.calledOnce;
expect(mockDBConnection.release).to.have.been.calledOnce;
});

it('throws error when device already exists', async () => {
const mockDBConnection = getMockDBConnection({
release: sinon.stub(),
rollback: sinon.stub()
});
sinon.stub(db, 'getDBConnection').returns(mockDBConnection);

const mockSurveyId = '2';
const mockSerial = '123456';
const mockDeviceMakeId = 1;
const mockModel = 'ModelX';
const mockComment = 'Device Comment';

const existingDevice = {
serial: mockSerial,
device_make_id: mockDeviceMakeId,
device_id: 1,
survey_id: 1,
device_key: '1:lotek',
model: null,
comment: 'comment'
};

sinon.stub(TelemetryDeviceService.prototype, 'findDeviceBySerial').resolves(existingDevice);
sinon.stub(TelemetryDeviceService.prototype, 'createDevice').resolves();

const { mockReq, mockRes, mockNext } = getRequestHandlerMocks();

mockReq.params = {
surveyId: mockSurveyId
};

mockReq.body = {
serial: mockSerial,
device_make_id: mockDeviceMakeId,
model: mockModel,
comment: mockComment
};

const requestHandler = createDevice();

try {
await requestHandler(mockReq, mockRes, mockNext);
expect.fail('Error should have been thrown');
} catch (error) {
// Assertions
expect(error).to.be.an.instanceOf(ApiGeneralError);
expect((error as ApiError).message).to.equal(
`Device ${mockSerial} of the given make already exists in the Survey.`
);
expect(mockDBConnection.release).to.have.been.calledOnce;
}
});

it('handles errors and rolls back transaction', async () => {
// Mock DB connection
const mockDBConnection = getMockDBConnection({
release: sinon.stub(),
rollback: sinon.stub()
});
sinon.stub(db, 'getDBConnection').returns(mockDBConnection);

const mockError = new Error('Database error');
sinon.stub(TelemetryDeviceService.prototype, 'findDeviceBySerial').rejects(mockError);

const { mockReq, mockRes, mockNext } = getRequestHandlerMocks();

mockReq.params = {
surveyId: '2'
};

mockReq.body = {
serial: '123456',
device_make_id: 1,
model: 'ModelX',
comment: 'Device Comment'
};

const requestHandler = createDevice();

try {
await requestHandler(mockReq, mockRes, mockNext);
expect.fail('Error should have been thrown');
} catch (actualError) {
expect(actualError).to.equal(mockError);
expect(mockDBConnection.rollback).to.have.been.calledOnce;
expect(mockDBConnection.release).to.have.been.calledOnce;
}
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { RequestHandler } from 'express';
import { Operation } from 'express-openapi';
import { PROJECT_PERMISSION, SYSTEM_ROLE } from '../../../../../../constants/roles';
import { getDBConnection } from '../../../../../../database/db';
import { ApiGeneralError } from '../../../../../../errors/api-error';
import {
paginationRequestQueryParamSchema,
paginationResponseSchema
Expand Down Expand Up @@ -137,6 +138,14 @@ export function createDevice(): RequestHandler {

const telemetryDeviceService = new TelemetryDeviceService(connection);

// Check whether device already exists in Survey
const device = await telemetryDeviceService.findDeviceBySerial(surveyId, serial, device_make_id);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: There is already a unique constraint on the device table for (survey_id, serial, device_make_id)

Copy link
Collaborator Author

@mauberti-bc mauberti-bc Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this to catch the would-be error to give a useful error message. I figured this made the goal more obvious rather than waiting for the database error, but let me know if you think we should do it differently.


// Throw error if device already exists
if (device) {
throw new ApiGeneralError(`Device ${serial} of the given make already exists in the Survey.`);
}

await telemetryDeviceService.createDevice({
survey_id: surveyId,
serial: serial,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,22 @@ describe('TelemetryDeviceRepository', () => {
});
});

describe('findDeviceBySerial', () => {
it('should find a device with a given serial and make in the given survey', async () => {
const mockRow = { device_id: 1 };
const mockDBConnection = getMockDBConnection({ knex: sinon.stub().resolves({ rows: [mockRow] }) });

const mockSurvey = 1;
const mockSerial = 12345;
const mockDeviceMakeId = 5;

const telemetryDeviceRepository = new TelemetryDeviceRepository(mockDBConnection);

const response = await telemetryDeviceRepository.findDeviceBySerial(mockSurvey, mockSerial, mockDeviceMakeId);
expect(response).to.eql(mockRow);
});
});

describe('deleteDevicesByIds', () => {
it('should delete devices by IDs', async () => {
const mockRows = [{ device_id: 1 }];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,30 @@ export class TelemetryDeviceRepository extends BaseRepository {
return response.rows;
}

/**
* Finds a device by a given serial number and make in the given survey
*
* @param {surveyId} surveyId
* @param {number} serial
* @param {number} deviceMakeId
* @returns {*} {Promise<DeviceRecord | null>}
*/
async findDeviceBySerial(surveyId: number, serial: number, deviceMakeId: number): Promise<DeviceRecord | null> {
const knex = getKnex();

const queryBuilder = knex
.select(['device_id', 'survey_id', 'device_key', 'serial', 'device.device_make_id', 'model', 'comment'])
.from('device')
.join('device_make', 'device_make.device_make_id', 'device.device_make_id')
.where('serial', serial)
.andWhere('device.device_make_id', deviceMakeId)
.andWhere('survey_id', surveyId);

const response = await this.connection.knex(queryBuilder, DeviceRecord);

return response.rows[0];
}

/**
* Retrieve the list of devices for a survey, based on pagination options.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,42 @@ describe('TelemetryDeviceService', () => {
});
});

describe('findDeviceBySerial', () => {
it('should return a device with the given serial and make if it exists in the survey', async () => {
const mockConnection = getMockDBConnection();
const service = new TelemetryDeviceService(mockConnection);

const mockDevice = {
device_make_id: 1,
model: null,
survey_id: 1,
device_id: 1,
device_key: '1:lotek',
serial: 'serial',
comment: 'comment'
};

const repoStub = sinon.stub(TelemetryDeviceRepository.prototype, 'findDeviceBySerial').resolves(mockDevice);

const device = await service.findDeviceBySerial(1, 2, 1);

expect(repoStub).to.have.been.calledOnceWithExactly(1, 2, 1);
expect(device).to.eql(mockDevice);
});

it('should return null if a device with the given serial and make does not exist in the survey', async () => {
const mockConnection = getMockDBConnection();
const service = new TelemetryDeviceService(mockConnection);

const repoStub = sinon.stub(TelemetryDeviceRepository.prototype, 'findDeviceBySerial').resolves(null);

const device = await service.findDeviceBySerial(1, 2, 1);

expect(repoStub).to.have.been.calledOnceWithExactly(1, 2, 1);
expect(device).to.eql(null);
});
});

describe('deleteDevice', () => {
it('should delete a device by its ID', async () => {
const mockConnection = getMockDBConnection();
Expand Down
12 changes: 12 additions & 0 deletions api/src/services/telemetry-services/telemetry-device-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,18 @@ export class TelemetryDeviceService extends DBService {
return devices[0];
}

/**
* Finds a device by a given serial number and make in the given survey
*
* @param {number} surveyId
* @param {number} serial
* @param {number} deviceMakeId
* @return {*} {Promise<DeviceRecord | null>}
*/
async findDeviceBySerial(surveyId: number, serial: number, deviceMakeId: number): Promise<DeviceRecord | null> {
return this.telemetryDeviceRepository.findDeviceBySerial(surveyId, serial, deviceMakeId);
}

/**
* Get a list of devices by their IDs.
*
Expand Down
4 changes: 2 additions & 2 deletions app/src/components/fields/AnimalAutocompleteField.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import Autocomplete from '@mui/material/Autocomplete';
import Autocomplete, { createFilterOptions } from '@mui/material/Autocomplete';
import Box from '@mui/material/Box';
import CircularProgress from '@mui/material/CircularProgress';
import grey from '@mui/material/colors/grey';
Expand Down Expand Up @@ -116,7 +116,7 @@ export const AnimalAutocompleteField = <T extends string | number>(props: IAnima
isOptionEqualToValue={(option, value) => {
return option.critter_id === value.critter_id;
}}
filterOptions={(item) => item}
filterOptions={createFilterOptions()}
inputValue={inputValue}
onInputChange={(_, _value, reason) => {
if (clearOnSelect && reason === 'clear') {
Expand Down
7 changes: 4 additions & 3 deletions app/src/components/fields/DateField.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import { mdiCalendar } from '@mdi/js';
import { Icon } from '@mdi/react';
import { DatePicker, LocalizationProvider } from '@mui/x-date-pickers';
import { DatePicker, DatePickerProps, LocalizationProvider } from '@mui/x-date-pickers';
import { AdapterDayjs } from '@mui/x-date-pickers/AdapterDayjs';
import { DATE_FORMAT, DATE_LIMIT } from 'constants/dateTimeFormats';
import dayjs from 'dayjs';
import dayjs, { Dayjs } from 'dayjs';
import { useFormikContext } from 'formik';
import { get } from 'lodash-es';

interface IDateFieldProps {
interface IDateFieldProps extends DatePickerProps<Dayjs> {
label: string;
name: string;
id: string;
Expand All @@ -28,6 +28,7 @@ export const DateField = <FormikPropsType extends IDateFieldProps>(props: IDateF
return (
<LocalizationProvider dateAdapter={AdapterDayjs}>
<DatePicker
{...props}
slots={{
openPickerIcon: () => <Icon path={mdiCalendar} size={1} />
}}
Expand Down
4 changes: 2 additions & 2 deletions app/src/components/fields/DeviceAutocompleteField.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import Autocomplete from '@mui/material/Autocomplete';
import Autocomplete, { createFilterOptions } from '@mui/material/Autocomplete';
import Box from '@mui/material/Box';
import grey from '@mui/material/colors/grey';
import TextField from '@mui/material/TextField';
Expand Down Expand Up @@ -119,10 +119,10 @@ export const DeviceAutocompleteField = <T extends string | number>(props: IDevic
noOptionsText="No matching options"
options={options ?? []}
getOptionLabel={(option) => option.serial}
filterOptions={createFilterOptions()}
isOptionEqualToValue={(option, value) => {
return option.device_id === value.device_id;
}}
filterOptions={(item) => item}
inputValue={inputValue}
onInputChange={(_, _value, reason) => {
if (clearOnSelect && reason === 'clear') {
Expand Down
2 changes: 1 addition & 1 deletion app/src/components/fields/HorizontalSplitFormComponent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export interface IHorizontalSplitFormComponentProps {
* @type {string}
* @memberof IHorizontalSplitFormComponentProps
*/
summary?: string;
summary?: string | ReactElement;
/**
* The form component to render
*
Expand Down
25 changes: 25 additions & 0 deletions app/src/constants/errors.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/**
* This is a substring of the database's foreign key constraint error message, used to catch
* foreign key constraint errors when trying to delete a record and displaying a more useful error message
*
* ie. While trying to delete a device:
*
* if (error.includes(FOREIGN_KEY_CONSTRAINT_ERROR)) {
* return "Delete the associated deployment before deleting the device"
* }
*
*/
export const FOREIGN_KEY_CONSTRAINT_ERROR = 'foreign key constraint';

/**
* This is a substring of the database's unique constraint error message, used to catch
* unique constraint errors when trying to insert a record
*
* ie. While trying to create a device:
*
* if (error.includes(UNIQUE_CONSTRAINT_ERROR)) {
* return "That device already eixsts in the Survey"
* }
*
*/
export const UNIQUE_CONSTRAINT_ERROR = 'already exists';
Loading
Loading