-
Notifications
You must be signed in to change notification settings - Fork 504
Issue1666 - Include specific parameters to success/error messages on all routes #1689
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
base: development
Are you sure you want to change the base?
Changes from 7 commits
837541c
bc99aaf
f46ecad
b33e5db
987b48a
279c70e
cf493b5
cf01bb4
330b745
607d0f3
0f08ada
45f4297
4015e2e
b2713f6
6695107
2c5034b
9910f13
0023a43
b5935fc
78b6656
76ccb34
fbd43d9
874d390
d1e90b8
a8602c5
0b954fb
1356747
3e94055
0392153
e33480a
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 |
|---|---|---|
|
|
@@ -393,19 +393,24 @@ router.put('/edit', adminAuthMiddleware('edit groups'), async (req, res) => { | |
|
|
||
| return t.batch(flatten([adoptGroupsQueries, disownGroupsQueries, adoptMetersQueries, disownMetersQueries])); | ||
| }); | ||
| res.sendStatus(HTTP_CODES.OK); | ||
| //res.sendStatus(HTTP_CODES.OK); | ||
|
Member
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. See other comment. The changes here also overlap the other PR. Thoughts?
Contributor
Author
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. Noting that the merge has replaced this with the consistent implementation from #1688. |
||
| success(res); | ||
| } catch (err) { | ||
| if (err.message && err.message === 'Cyclic group detected') { | ||
| res.status(HTTP_CODES.BAD_REQUEST).send({ message: err.message }); | ||
| //res.status(HTTP_CODES.BAD_REQUEST).send({ message: err.message }); | ||
| failure(res, HTTP_CODES.BAD_REQUEST, err.message); | ||
| } else { | ||
| log.error(`Error while editing existing group ${err}`, err); | ||
| res.sendStatus(HTTP_CODES.INTERNAL_SERVER_ERROR); | ||
| //res.sendStatus(HTTP_CODES.INTERNAL_SERVER_ERROR); | ||
| failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, "Got request to edit group with invalid data. Error(s): " + validatorResult.errors.toString()); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| router.post('/delete', adminAuthMiddleware('delete groups'), async (req, res) => { | ||
| // TODO DEBUG: to force the showErrorNotification to pass in EditGroupsModalComponent.tsx | ||
| //req.body.id = -1; | ||
| const validParams = { | ||
| type: 'object', | ||
| additionalProperties: false, | ||
|
|
@@ -423,10 +428,12 @@ router.post('/delete', adminAuthMiddleware('delete groups'), async (req, res) => | |
| const conn = getConnection(); | ||
| try { | ||
| await Group.delete(req.body.id, conn); | ||
| res.sendStatus(HTTP_CODES.OK); | ||
| //res.sendStatus(HTTP_CODES.OK); | ||
| success(res, 'Successfully deleted group'); | ||
| } catch (err) { | ||
| log.error(`Error while deleting group ${err}`, err); | ||
| res.sendStatus(HTTP_CODES.INTERNAL_SERVER_ERROR); | ||
| //res.sendStatus(HTTP_CODES.INTERNAL_SERVER_ERROR); | ||
| failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, `'Error while deleting group: ${err}`); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -340,7 +340,14 @@ router.post('/edit', adminAuthMiddleware('edit meters'), async (req, res) => { | |
| res.json(formatMeterForResponse(meter, true)); | ||
| } catch (err) { | ||
| log.error(`Error while editing a meter with detail "${err['detail']}"`, err); | ||
| failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, err.toString() + ' with detail ' + err['detail']); | ||
| if (err.toString().includes('duplicate key value violates unique constraint')) { | ||
|
Member
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. See other comment. The changes here also overlap the other PR but I think bring in other features. Do you think they could be merged? Thoughts?
Contributor
Author
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 has been updated to consider the changes from the other PR. |
||
| failure(res, HTTP_CODES.BAD_REQUEST, `Meter name "${req.body.name}" already exists`); | ||
| } else if (err.toString().includes('violates check constraint')) { | ||
| failure(res, HTTP_CODES.BAD_REQUEST, `Invalid meter data: ${err.toString()}`); | ||
| } else { | ||
| log.error(`Error while editing a meter with detail "${err['detail']}"`, err); | ||
| failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, err.toString() + ' with detail ' + err['detail']); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
|
|
@@ -411,7 +418,14 @@ router.post('/addMeter', adminAuthMiddleware('add meter'), async (req, res) => { | |
| res.json(formatMeterForResponse(newMeter, true)); | ||
| } catch (err) { | ||
| log.error(`Error while inserting new meter with detail "${err['detail']}"`, err); | ||
| failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, err.toString() + ' with detail ' + err['detail']); | ||
| if (err.toString().includes('duplicate key value violates unique constraint')) { | ||
| failure(res, HTTP_CODES.BAD_REQUEST, `Meter name "${req.body.name}" already exists`); | ||
| } else if (err.toString().includes('violates check constraint')) { | ||
| failure(res, HTTP_CODES.BAD_REQUEST, `Invalid meter data: ${err.toString()}`); | ||
| } else { | ||
| log.error(`Error while inserting new meter with detail "${err['detail']}"`, err); | ||
| failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, err.toString() + ' with detail ' + err['detail']); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -158,7 +158,15 @@ router.post('/edit', adminAuthMiddleware('edit units'), async (req, res) => { | |
| success(res, 'Successfully edited unit'); | ||
| } catch (err) { | ||
| log.error(`Failed to update unit: ${err}`, err); | ||
| failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, 'Unable to update unit'); | ||
| //failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, 'Unable to update unit'); | ||
|
Member
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. See other comment. The changes here also overlap the other PR but with new items. Thoughts?
Contributor
Author
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 has been updated to consider the changes from the other PR. |
||
| if (err.toString().includes('duplicate key value violates unique constraint')) { | ||
| failure(res, HTTP_CODES.BAD_REQUEST, `Unit name "${req.body.name}" already exists`); | ||
| } else if (err.toString().includes('violates check constraint')) { | ||
| failure(res, HTTP_CODES.BAD_REQUEST, `Invalid unit data: ${err.toString()}`); | ||
| } else { | ||
| log.error(`Error while editing a unit with detail "${err['detail']}"`, err); | ||
| failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, err.toString() + ' with detail ' + err['detail']); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
|
|
@@ -197,7 +205,15 @@ router.post('/addUnit', adminAuthMiddleware('add units'), async (req, res) => { | |
| success(res, 'Unit created successfully'); | ||
| } catch (err) { | ||
| log.error(`Error while inserting new unit: ${err}`, err); | ||
| failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, `Error while inserting new unit: ${err}`); | ||
| //failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, `Error while inserting new unit: ${err}`); | ||
|
Member
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. See comment above in this file.
Contributor
Author
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 has been updated to consider the changes from the other PR. |
||
| if (err.toString().includes('duplicate key value violates unique constraint')) { | ||
| failure(res, HTTP_CODES.BAD_REQUEST, `Unit name "${req.body.name}" already exists`); | ||
| } else if (err.toString().includes('violates check constraint')) { | ||
| failure(res, HTTP_CODES.BAD_REQUEST, `Invalid unit data: ${err.toString()}`); | ||
| } else { | ||
| log.error(`Error while inserting new unit with detail "${err['detail']}"`, err); | ||
| failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, err.toString() + ' with detail ' + err['detail']); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
|
|
@@ -206,6 +222,8 @@ router.post('/addUnit', adminAuthMiddleware('add units'), async (req, res) => { | |
| * Route for deleting a unit by ID. | ||
| */ | ||
| router.post('/delete', adminAuthMiddleware('delete units'), async (req, res) => { | ||
| // TODO DEBUG: to force the showErrorNotification to pass in EditUnitsModalComponent.tsx | ||
| //req.body.id = -1; | ||
| const validParams = { | ||
| type: 'object', | ||
| additionalProperties: false, | ||
|
|
||
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.
I think this change overlaps a change in PR #1688 that came in at about the same time. My current thinking is their change should stay as all routes were aligned with this new methodology. Thought?
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.
Yes, I definitely agree. The changes made in #1688 should take priority as extensive work has been done to have consistent implementations across all the routes. When merging the work from #1688, their change has overwritten this one.