Add contexts api #500
Add contexts api #500
Conversation
There was a problem hiding this comment.
Hi @tomcorey26
Thanks for this great PR!
Left some small comments to make it compatible with all other APIs behavior
| } | ||
|
|
||
| if (!deleteResult) { | ||
| res.status(204).json({ message: 'Context not found' }); |
There was a problem hiding this comment.
Hi!, we usually return 404 when context not found, and 204 if it's found and deleted
| id: dataValues.id, | ||
| name: dataValues.name | ||
| })); | ||
| } else { |
There was a problem hiding this comment.
It's multiple get, so it's ok to return empty array instead 404
| try { | ||
| await database.saveContext(id, contextName); | ||
| } catch (err) { | ||
| const error = new Error(err.message); |
There was a problem hiding this comment.
we don't expect any 400 here, the open API validator needs to verify the correctness of the input data, so no need for try-catch
| async function deleteContext(contextId) { | ||
| let deleteResult; | ||
| try { | ||
| deleteResult = await database.deleteContext(contextId); |
There was a problem hiding this comment.
Same as above (no need try-catch)
There was a problem hiding this comment.
if context not found, better to throw here a 404 exception (then no need to catch it specifically in the controller)
|
|
||
| const duplicate = await contextClient.findOne({ where: { name: contextName } }); | ||
| if (duplicate) { | ||
| throw Error('Duplicate context'); |
There was a problem hiding this comment.
need to throw here an error with 409 status code.
also need to verify that context_id doesn't exist.
There was a problem hiding this comment.
you defined name as unique: true in the sequelize data spec, so this if might be unnecessary and sequelize would throw an error in the contextClient.create() function if it were a duplicated name.
The error thrown should be there with status code 409
| async function getContexts() { | ||
| const contextClient = client.model('context'); | ||
| const options = { | ||
| attributes: { exclude: ['updated_at', 'created_at'] } |
There was a problem hiding this comment.
created_at and updated_at not exists in this table
maybe it's a good idea to add create_at to this table
| @@ -68,6 +69,7 @@ module.exports = async () => { | |||
| app.use('/v1/processors', processorsRouter); | |||
| app.use('/v1/files', filesRouter); | |||
| app.use('/v1/webhooks', webhooksRouter); | |||
| app.use('/v1/contexts', contextsRouter); | |||
There was a problem hiding this comment.
Please add this api spec to
- predator/docs/devguide/docs/swagger-docs.yaml
- predator/docs/openapi3.yaml
| @@ -0,0 +1,57 @@ | |||
|
|
|||
There was a problem hiding this comment.
I think there is a need to add unit tests as well to pass coverage threshold
|
hi @tomcorey26 |
Closes #497