-
Notifications
You must be signed in to change notification settings - Fork 6
Feature 2345/update pulse response object #2351
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
Conversation
…ithub.com/objectcomputing/check-ins into feature-2345/update-pulse-response-object
|
@mkimberlin Please give this an eyeball/approval before I merge. |
server/src/main/java/com/objectcomputing/checkins/services/pulseresponse/PulseResponse.java
Outdated
Show resolved
Hide resolved
| @NotNull | ||
| @Schema(required = true, description = "date for updatedDate") | ||
| private LocalDate updatedDate; | ||
| @Schema(required = true, description = "integer value of external score") |
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 we should make external scores and feelings nullable. That would mean changing the field below this as well. We definitely want both feelings to be nullable. Those are comments. The only thing that I think we want to require is a score on the "internal" (about work) feelings.
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.
Adjusted + tests
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.
Almost what I meant... There are three fields that should be nullable: internalFeelings, externalScore, and externalFeelings
internalScore should not be nullable.
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.
@mkimberlin This has been done.
|
@mkimberlin please review the changes |
…e-pulse-response-object
In relation to:
#2345