Enabling User to have multiple questions #80
Conversation
|
@daltonfury42 Can you review this PR |
|
|
||
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| public static class Question { | ||
| @JsonProperty("questionId") |
There was a problem hiding this comment.
I'm wondering if we're using our own custom representation of the questions object. Then we'll need to write our own parsing library to generate the forms on UI.
Maybe we should try an existing library like
https://github.com/rjsf-team/react-jsonschema-form ?
| } catch (JsonProcessingException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| return queueRepository.save(queue1); |
There was a problem hiding this comment.
This line should be moved in try block, and return null instead.
| .orElseThrow(SQInvalidRequestException::queueNotFoundException); | ||
| } | ||
|
|
||
| @Transactional | ||
| public CustomQuestions createCustomQuestions(String queueId, String customQuestions) throws JsonProcessingException { |
There was a problem hiding this comment.
You can remove the throws part
| try { | ||
| queue1.setCustomQuestions(mapper.writeValueAsString(customQuestionsObject)); | ||
| } catch (JsonProcessingException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
I think this is better to be logged using a logger, for future debugging.
There was a problem hiding this comment.
Also I think you should throw an 500 internal error here
| queue.getQueueCreationTimestamp())) | ||
| { | ||
| try { | ||
| CustomQuestions customQuestions = queue.getCustomQuestions() != null |
There was a problem hiding this comment.
You have duplicate code starting with line +122
There was a problem hiding this comment.
Also you can do flatMap and create a method with the following body.
Option.ofNullable(queue.getCustomQuestions()).map(this::deserializeQuestions).map(this::createQueueStatusResponse)
And the code will be ....flatMap(this::createResponse).orElseThrow... and you can use same code on the other method
There was a problem hiding this comment.
Also why do we store json? We can handle this using table relationship and we can avoid this complicated logic. Plus it adds a lot of computational overhead.
There was a problem hiding this comment.
Agreed. If you want to redo this as a table, with just textual questions and answers, we can do that. But we should think first about how we would display the user input along with each token in the admin page. Maybe some collapsable expansion panel would do the job.
That's the reason why I have been delaying this feature for a long time, till all the easy features are done and I get some real customers.
| @@ -40,6 +33,9 @@ | |||
| @Temporal(TemporalType.TIMESTAMP) | |||
| Date queueCreationTimestamp; | |||
|
|
|||
| @Column(length = 100000) | |||
There was a problem hiding this comment.
I think we can use Text type here maybe. It is just a proposal
| @PostMapping(path = "/queue/{queueId}/custom-questions") | ||
| public ResponseEntity<CustomQuestions> createCustomQuestions( | ||
| @PathVariable("queueId") String queueId, | ||
| @RequestBody String customQuestions) throws JsonProcessingException { |
| try { | ||
| queue1.setCustomQuestions(mapper.writeValueAsString(customQuestionsObject)); | ||
| } catch (JsonProcessingException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Also I think you should throw an 500 internal error here
|
@chalx Thanks for the review. All makes sense. I want to merge this change a little after, will address them then. |
|
@daltonfury42 Also we need to write tests.
|
|
@chalx Yes, I know there is no excuses that would justify not having unit tests, but I have been living with some end to end integration tests till now. |
|
@daltonfury42 I saw that tests. They are ok, but we need to take real unit tests into consideration. This can be a quality gate for each merge request |
|
Understood. I'll make it a point to write/insist on unit tests from now on. |
This PR is to address #69 .