Migrate deleting a boxes measurements #658
Conversation
* feat(api): add api routes for /users/me * fix(tests): api me PUT * feat(api): add delete me endpoint
| ): Promise<z.infer<typeof DeleteQueryParams>> => { | ||
| const url = new URL(request.url) | ||
| let params: Record<string, any> | ||
| if (request.method !== 'GET') { |
There was a problem hiding this comment.
Ì think this endpoint should only support the delete method?
There was a problem hiding this comment.
I don't remember why its there and you are perfectly correct.. removing it!
| try { | ||
| params = await request.json() | ||
| } catch { | ||
| params = Object.fromEntries(url.searchParams) |
There was a problem hiding this comment.
I think in principle it is a good idea to fall back to params, but i am a bit unsure if we should do this here with regards to backwards-compatibility. In the old docs the endpoint is described as using the request body, so i guess this would introduce a new behaviour? Also if the content type is invalid (so != application/json) the old app would have thrown a 415 if i see it correctly
There was a problem hiding this comment.
Trying it with the existing API, even though the docs specify it to work with body parameters, I actually does not seem to work with those at all.
Thus I better remove the request.json() part, right?
(funnily enough, you have to set content-type: application/json for it to work even though you never send json data. Definitely an inconsistency. The question is if we should loosen that or keep it for now?)
| const userDevices = await getUserDevices(jwtResponse.id) | ||
| if (!userDevices.some((d) => d.id === deviceId)) | ||
| return StandardResponse.forbidden( | ||
| 'You are not allowed to delete data of the given device', |
There was a problem hiding this comment.
Unlikely to break anything, but the message in the old api was User does not own this senseBox (mentioning this because in the old app there was also a test for this: ```expect(response).to.have.json({ code: 'Forbidden', message: 'User does not own this senseBox' });
There was a problem hiding this comment.
Yes good point, I'm thinking we should still changes this, because we should get rid of the strict "senseBox" branding and use the more generic "device" term. As you said its unlikely to break anything, so I guess we can keep the message?
| ) | ||
|
|
||
| const device = userDevices.find((d) => d.id === deviceId) | ||
| if (!device?.sensors.some((s) => s.id === sensorId)) |
There was a problem hiding this comment.
I think the old app would have thrown a 404 here (see Box.js l. 387)
There was a problem hiding this comment.
I tried with the existing api and it indeed returns 404. Thanks for pointing out!
| }, | ||
| ] | ||
|
|
||
| describe('openSenseMap API Routes: /boxes/:deviceId/:sensorId/measurement', () => { |
There was a problem hiding this comment.
I think it would be good to add a test for the case when the user does not own the device (see the test "should deny to delete sensordata of boxes not owned by the user" in the old app)
Type of Change
Implementation
Implementation of existing endpoint
Checklist
devbranchAdditional Information