-
Notifications
You must be signed in to change notification settings - Fork 0
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
Addressed QA feedback #119
Conversation
Added help tooltip Corrected date format Added security
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.
Please see comments
@@ -173,25 +177,34 @@ private List<PatientRegisterModel> convertPatientRegistration(List<PatientRegist | |||
|
|||
} | |||
|
|||
private String formatDate(Date date) { | |||
LocalDate localDate = Instant.ofEpochMilli(date.getTime()).atZone(ZoneId.systemDefault()).toLocalDate(); |
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.
There is a method below that does this so could be reused.
@@ -173,25 +177,34 @@ private List<PatientRegisterModel> convertPatientRegistration(List<PatientRegist | |||
|
|||
} | |||
|
|||
private String formatDate(Date date) { | |||
LocalDate localDate = Instant.ofEpochMilli(date.getTime()).atZone(ZoneId.systemDefault()).toLocalDate(); | |||
DateTimeFormatter formatters = DateTimeFormatter.ofPattern(DATE_FORMAT); |
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.
No benefit to declaring and returning on separate lines, could just use:
return localDate.format(DateTimeFormatter.ofPattern(DATE_FORMAT));
There would be some benefit to decaling the formatter statically.
private static final DateTimeFormatter DATE_TIME_FORMATTER_yyyyMMdd = DateTimeFormatter.ofPattern(DATE_FORMAT);
then reusing it
return localDate.format(DATE_TIME_FORMATTER_yyyyMMdd);
Note, always use full name to avoid confusion between objects.
personDetailsResponse.setDateOfBirth(birthDate); | ||
|
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.
Can remove blank line for better grouping of code
@@ -176,7 +204,6 @@ export default { | |||
}, | |||
resetForm() { | |||
this.phn = '' | |||
this.payee = '' |
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.
Why is this removed, payee field should be reset also?
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 will be a read only field once https://dev.azure.com/cgi-vic-hlth/HNWeb/_workitems/edit/7102 is done.
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.
Changes look good
Added help tooltip
Corrected date format
Added security