Email transcript use case #33
Conversation
|
|
||
| @Document("message") | ||
| public class Message { | ||
|
|
||
| @Id | ||
| private String id; | ||
| private String content; | ||
| private LocalDate timeStamp; | ||
| private LocalTime timeStamp; |
There was a problem hiding this comment.
The java LocalTime class can only store times in the 24 hour period. This means we lose information about the day/month/year of the message. I'm assuming this change is because LocalDate does not support the hour/minute information. A good solution in this case is to use the LocalDateTime class to support both,
There was a problem hiding this comment.
I didn’t want to have the day/month/year in the transcript, because it was too repetitive, but I can see if I can use LocalDateTime, but still have the transcript only have the time.
There was a problem hiding this comment.
I would suggest putting the date at the top of the email, that way it's consistent
| @@ -54,6 +51,18 @@ public ResponseEntity<String> joinSession(@PathVariable String code) { | |||
| return new ResponseEntity<>(gson.toJson(responseBody), headers, HttpStatus.OK); | |||
| } | |||
|
|
|||
| @GetMapping("/transcript/{code}") | |||
There was a problem hiding this comment.
Since this is a seperate use case maybe we should move this into its own class to better adhere to Clean Architecture and the single responsibility principle.
| @@ -54,6 +51,18 @@ public ResponseEntity<String> joinSession(@PathVariable String code) { | |||
| return new ResponseEntity<>(gson.toJson(responseBody), headers, HttpStatus.OK); | |||
| } | |||
|
|
|||
| @GetMapping("/transcript/{code}") | |||
| public ResponseEntity<String> getTranscript(@PathVariable String code) { | |||
There was a problem hiding this comment.
Can we add documentation here since getTranscript is a public function
There was a problem hiding this comment.
Good work, very productive and quick. I liked your use of CA knowledge and separating everything into respective files.
Suggestions:
- Please add error handling/try catch clauses for all endpoints. This makes debugging easier
- Please write at least 1 test before you merge
|
|
||
| @Document("message") | ||
| public class Message { | ||
|
|
||
| @Id | ||
| private String id; | ||
| private String content; | ||
| private LocalDate timeStamp; | ||
| private LocalTime timeStamp; |
There was a problem hiding this comment.
I would suggest putting the date at the top of the email, that way it's consistent
| @GetMapping("/transcript/{code}") | ||
| public ResponseEntity<String> getTranscript(@PathVariable String code) { | ||
| HttpHeaders headers = new HttpHeaders(); | ||
| Session session = sessionRepository.getSessionsByActiveAndCode(true, code); |
There was a problem hiding this comment.
Here you check if the session exists, please add an exception in case an invalid session code was added or an error occurs. See https://github.com/Callhub-Connect/callhub-backend/blob/pdf-session-connection/src/main/java/callhub/connect/controllers/FileController.java if you want an example (at the end of the file)
|
Will refactor to clean architecture and write tests later :) |
Description
Fixes # (issue)
Type of change
How Has This Been Tested?
Manually tested. Make sure to get the associated front-end branch as well for code review.
Checklist: