-
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
add DTO for CWMS Property #666
Conversation
throw new FieldException("The 'office' field of a Property cannot be null or empty."); | ||
} | ||
if (this.value == null || this.value.trim().isEmpty()) { | ||
throw new FieldException("The 'value' field of a Property cannot be null or empty."); |
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.
This seems... odd?
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.
Which part? That the validate does anything at all or that value can't be null/empty?
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 just checked, prop_value and prop_comment are both Nullable in the database.
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.
We should also probably have the comment here, maybe hide it behind an option for the endpoint, general get and use value doesn't need it, but a GUI that lists them all for configuration would.
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.
Which part? That the validate does anything at all or that value can't be null/empty?
That value can't be null or empty. Both are a valid state for a property.
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.
good reminder that I forgot the comment. My understanding is that this method is for validating client submissions, what is the case where we'd accept a null property value? If we want to delete, we'd use the delete function.
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.
Hmm, fair point, but "Property EXISTS and is set to NULL" does equal "Property does not exist".
One is far more extreme than the other.
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.
ok, updated to remove validation on value
{ | ||
"category": "TestCategory", | ||
"name": "TestName", | ||
"office": "TestOffice", |
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.
lots of cda stuff is office-id and not office
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.
confusing that the query parameter is office and the DTO is office-id.
What about "name"? I see a similar mismatch and inconsistency among DTO's there
the cwms database allows the column to be null
4682e6a
to
f4a7dd4
Compare
includes unit tests for in-memory serialization/deserialization in addition to deserialization from disk