-
Notifications
You must be signed in to change notification settings - Fork 14
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
CTO-118 Updated Gate Change Controller and DAO #904
Conversation
…hanges store/delete functionality
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.
Generally looks good. Some changes for clarity required.
CWMS_OUTLET_PACKAGE.call_DELETE_OUTLET(DSL.using(conn).configuration(), locationId, | ||
deleteRule.getRule(), officeId); | ||
} catch (IntegrityConstraintViolationException e) { | ||
throw new NotFoundException(e); |
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.
Not found doesn't make sense given the parent exception. 409 Conflict
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/409 makes more sense.
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.
Swapped for a DeleteConflictException. The error code received by HCDA isn't the most useful, ("Data already exists for this request"). Maybe there's another exception that will provide more detail?
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.
It should send basically what the constraint is "something depends on it." We'll have to look into that later.
@@ -199,6 +199,7 @@ static GateChange buildTestGateChange(CwmsId projectId, Instant changeDate, Gate | |||
.withNotes(notes) | |||
.withChangeDate(changeDate) | |||
.withSettings(settings) | |||
// .withType("gate-change") |
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.
Either delete if not needed, or provide explanation of why it's sitting here commented out (in the comment itself.)
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.
Deleted unnecessary comment
"type" : "gate-change", | ||
"project-id" : { | ||
"office-id" : "SPK", | ||
"name" : "PROJECT1" |
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.
use a "real" project name.
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.
Swapped for a project name used in other Outlet testing
"type" : "gate-setting", | ||
"location-id" : { | ||
"office-id" : "SPK", | ||
"name" : "PROJECT1-CG100" |
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.
same as above comment, use a "real" name.
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.
Swapped for a project name used in other Outlet testing
There are failing tests, but they look unrelated to this PR so I'm merging in anyways. |
Updated Gate Change Controller and DAO to support operation changes store/delete functionality