-
-
Notifications
You must be signed in to change notification settings - Fork 288
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
Add API request states features #1496
Changes from all commits
a51ee63
174c990
6384feb
0e0738d
5d1b6ee
bd25cf6
29de8c3
2c276d5
490dc79
56d0a65
fe1062c
c11315a
b49a492
4354851
7c23a03
cf62d5f
1f4b8c5
2c0455d
461cc27
d15081a
765dc8a
d6ccab4
2db8a69
82e5ff5
ad05d6b
9f56bf2
b48191d
9547b3d
b72f8ce
357d346
66fad3a
3ece7b8
1af6e66
61ff68b
1168537
fa436cb
0d72ed6
5d0bbe9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
const { Op } = require('sequelize'); | ||
const db = require('../../models'); | ||
const { NotFoundError } = require('../../utils/coreErrors'); | ||
const { Error400 } = require('../../utils/httpErrors'); | ||
|
||
const DEFAULT_OPTIONS = { | ||
skip: 0, | ||
order_dir: 'ASC', | ||
order_by: 'created_at', | ||
}; | ||
|
||
/** | ||
* @description Get all features states aggregates. | ||
* @param {string} selector - Device selector. | ||
* @param {object} options - Options of the query. | ||
* @param {string} options.from - Start date in UTC format "yyyy-mm-ddThh:mm:ss:sssZ" | ||
* or "yyyy-mm-dd hh:mm:ss:sss" (GMT time). | ||
* @param {string} [options.to] - End date in UTC format "yyyy-mm-ddThh:mm:ss:sssZ" | ||
* or "yyyy-mm-dd hh:mm:ss:sss" (GMT time). | ||
* @param {number} [options.take] - Number of elements to return. | ||
* @param {number} [options.skip] - Number of elements to skip. | ||
* @param {string} [options.attributes] - Possible values (separated by a comma ',' if several): 'id', | ||
* 'device_feature_id', 'value', 'created_at' and 'updated_at'. Leave empty to have all the columns. | ||
* @returns {Promise<Array>} - Resolve with an array of data. | ||
* @example | ||
* device.getDeviceFeaturesStates('test-device', {from: '2022-03-31T00:00:00.000Z', to: '2022-03-31T23:59:59.999Z', | ||
* take: 100, skip: 10, attributes: 'id,value,created_at'}); | ||
*/ | ||
async function getDeviceFeaturesStates(selector, options) { | ||
const deviceFeature = this.stateManager.get('deviceFeature', selector); | ||
if (deviceFeature === null) { | ||
throw new NotFoundError('DeviceFeature not found'); | ||
} | ||
if (options.from === undefined) { | ||
throw new Error400('Start date missing'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the core, we try to avoid using HTTP errors (specific to the API), and instead use core errors. In that case, I would use the "BadParameters" error in the |
||
} | ||
const fromDate = new Date(options.from); | ||
// Default end date is now | ||
const toDate = options.to ? new Date(options.to) : new Date(); | ||
|
||
const optionsWithDefault = { ...DEFAULT_OPTIONS, ...options }; | ||
|
||
const queryParams = { | ||
raw: true, | ||
where: { | ||
device_feature_id: deviceFeature.id, | ||
created_at: { | ||
[Op.gte]: fromDate, | ||
[Op.lte]: toDate, | ||
}, | ||
}, | ||
offset: optionsWithDefault.skip, | ||
order: [[optionsWithDefault.order_by, optionsWithDefault.order_dir]], | ||
}; | ||
|
||
// take is not a default | ||
if (optionsWithDefault.take !== undefined) { | ||
queryParams.limit = optionsWithDefault.take; | ||
} | ||
|
||
if (optionsWithDefault.attributes !== undefined) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the default should be to have only those attributes returned:
(And the user can modify that if needed) What do you think of that? (And don't forget to add a test to it) |
||
queryParams.attributes = optionsWithDefault.attributes.split(','); | ||
} | ||
return db.DeviceFeatureState.findAll(queryParams); | ||
} | ||
|
||
module.exports = { | ||
getDeviceFeaturesStates, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -109,6 +109,34 @@ describe('GET /api/v1/device_feature/aggregated_states', () => { | |
}); | ||
}); | ||
|
||
describe('GET /api/v1/device_feature/:device_feature_selector/states', () => { | ||
Terdious marked this conversation as resolved.
Show resolved
Hide resolved
|
||
beforeEach(async function BeforeEach() { | ||
this.timeout(1000); | ||
await insertStates(1); | ||
}); | ||
it('should get device feature states by selector', async () => { | ||
const now = new Date(); | ||
const dateState = `${now.getUTCFullYear()}-${`0${now.getUTCMonth() + 1}`.slice(-2)}-${`0${now.getUTCDate()}`.slice( | ||
-2, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use dayjs everywhere in Gladys, it might be simpler/more readable just to use a simple:
No? |
||
)}`; | ||
|
||
await authenticatedRequest | ||
.get('/api/v1/device_feature/test-device-feature/states') | ||
.query({ | ||
from: new Date(`${dateState}T00:00:00.000Z`), | ||
to: new Date(`${dateState}T23:59:59.999Z`), | ||
}) | ||
.expect('Content-Type', /json/) | ||
.expect(200) | ||
.then((res) => { | ||
expect(res.body).to.have.lengthOf(2000); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit heavy to test returning 2000 state. The goal of inserting 2000 states for aggregated tests was to verify that only 100 of the 2000 were returned in the JSON response. Here, testing that 10, 200, or 2000 are returned doesn't change anything, it'll just slow the test down on every CI run. I would suggest testing only with like 50-100 states, it should be enough ! |
||
expect(res.body).to.be.an('array'); | ||
expect(res.body[0]).to.be.an('object'); | ||
expect(Object.keys(res.body[0])).to.have.lengthOf(5); | ||
}); | ||
}); | ||
}); | ||
|
||
describe('DELETE /api/v1/device/:device_selector', () => { | ||
it('should delete device', async () => { | ||
await authenticatedRequest | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
const EventEmitter = require('events'); | ||
const { expect, assert } = require('chai'); | ||
const uuid = require('uuid'); | ||
const { fake } = require('sinon'); | ||
const db = require('../../../models'); | ||
const Device = require('../../../lib/device'); | ||
const Job = require('../../../lib/job'); | ||
|
||
const event = new EventEmitter(); | ||
const job = new Job(event); | ||
|
||
const now = new Date('2000-06-15T03:59:00.000Z'); | ||
const insertStates = async () => { | ||
const queryInterface = db.sequelize.getQueryInterface(); | ||
const deviceFeatureStateToInsert = []; | ||
const statesToInsert = 3 * 60; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is still based from the tests from the aggregated states, but it doesn't seem to be really useful to have that much states ? IMO, you're doing a simple function, you can write a simple test In your case, you could even hardcode some states here (like 4 states) with fixed dates. Then, in your test, you write a query between fixed dates and make sure that only 2 of the 4 states are returned ? (for example) If you want to test the ?take, you could add another test and do it with ?take=1 for example |
||
for (let i = 0; i < statesToInsert; i += 1) { | ||
const startAt = new Date(now.getTime() - 3 * 60 * 60 * 1000); | ||
const date = new Date(startAt.getTime() + ((3 * 60 * 60 * 1000) / statesToInsert) * i); | ||
deviceFeatureStateToInsert.push({ | ||
id: uuid.v4(), | ||
device_feature_id: 'ca91dfdf-55b2-4cf8-a58b-99c0fbf6f5e4', | ||
value: i, | ||
created_at: date, | ||
updated_at: date, | ||
}); | ||
} | ||
await queryInterface.bulkInsert('t_device_feature_state', deviceFeatureStateToInsert); | ||
}; | ||
|
||
describe('Device.getDeviceFeaturesStates', function Describe() { | ||
this.timeout(5000); | ||
|
||
afterEach(async () => { | ||
const queryInterface = db.sequelize.getQueryInterface(); | ||
await queryInterface.bulkDelete('t_device_feature_state'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not useful, database is automatically purged between each tests |
||
}); | ||
it('should return states between 01:10 and 02:09 with a target between 2000-06-15 00:10 and now using take and skip , only created_at and values', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test assertion seems to test a very specific use case, and I don't see anything in the test code that checks that it's working as advertised? |
||
await insertStates(); | ||
const variable = { | ||
getValue: fake.resolves(null), | ||
}; | ||
const stateManager = { | ||
get: fake.returns({ | ||
id: 'ca91dfdf-55b2-4cf8-a58b-99c0fbf6f5e4', | ||
name: 'my-feature', | ||
}), | ||
}; | ||
const dateState = '2000-06-15'; | ||
const device = new Device(event, {}, stateManager, {}, {}, variable, job); | ||
const states = await device.getDeviceFeaturesStates('test-device-feature', { | ||
from: new Date(`${dateState}T00:00:00.000Z`).toISOString(), | ||
attributes: 'created_at,value', | ||
take: 60, | ||
skip: 10, | ||
}); | ||
expect(states).to.have.lengthOf(60); | ||
expect(Object.keys(states[0])).to.have.lengthOf(2); | ||
expect(states[0]).to.have.property('value'); | ||
expect(states[0]).to.not.have.own.property('updated_at'); | ||
}); | ||
it('should return error, device feature doesnt exist', async () => { | ||
const variable = { | ||
getValue: fake.resolves(null), | ||
}; | ||
const stateManager = { | ||
get: fake.returns(null), | ||
}; | ||
const dateState = '2000-06-15'; | ||
const device = new Device(event, {}, stateManager, {}, {}, variable, job); | ||
const promise = device.getDeviceFeaturesStates('test-device-feature', { | ||
from: new Date(`${dateState}T00:00:00.000Z`).toISOString(), | ||
to: new Date(`${dateState}T10:00:00.000Z`).toISOString(), | ||
}); | ||
return assert.isRejected(promise, 'DeviceFeature not found'); | ||
}); | ||
it('should return error, start date missing', async () => { | ||
const variable = { | ||
getValue: fake.resolves(null), | ||
}; | ||
const stateManager = { | ||
get: fake.returns({ | ||
id: 'ca91dfdf-55b2-4cf8-a58b-99c0fbf6f5e4', | ||
name: 'my-feature', | ||
}), | ||
}; | ||
const device = new Device(event, {}, stateManager, {}, {}, variable, job); | ||
const promise = device.getDeviceFeaturesStates('this-device-does-not-exist', {}); | ||
return assert.isRejected(promise, 'Start date missing'); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*string