Add API with queue history in CSV #166
Conversation
| @@ -86,4 +89,13 @@ | |||
| @PathVariable("queueId") String queueId) { | |||
| return ResponseEntity.ok(queueService.getQueueEvents(queueId)); | |||
| } | |||
|
|
|||
| @GetMapping(path = "/queue/{queueId}/history", produces = "text/csv") | |||
| @ResponseBody | |||
There was a problem hiding this comment.
why do we need @ResponseBody?
|
|
||
| @GetMapping(path = "/queue/{queueId}/history", produces = "text/csv") | ||
| @ResponseBody | ||
| public QueueEventsCsvResponse getQueueHistoryCSV( |
There was a problem hiding this comment.
rename function to getQueueEventsCsv and make the path "/queue/{queueId}/eventsCsv"
| import java.nio.charset.StandardCharsets; | ||
|
|
||
| @Component | ||
| public class QueueEventsCsvResponseConverter extends AbstractHttpMessageConverter<QueueEventsCsvResponse> { |
There was a problem hiding this comment.
Let's write a converter for QueueEventsResponse and remove QueueEventsCsvResponse and getQueueEventsInCsv service function.
As per this, it will use the media type to determine the converter, this should work. Can you try?
There was a problem hiding this comment.
If we are going to use Accept header to decide on the response content, then maybe just one endpoint would be enough?
GET /queue/{queueId}/eventswithAccept: application/jsonwould return the events in JSON,GET /queue/{queueId}/eventswithAccept: text/csvwould return a CSV
What do you think?
There was a problem hiding this comment.
QueueEventsResponse does not include queue name. I wanted to use it to make the filename more accessible. Do you think a generic name like history.csv is ok?
There was a problem hiding this comment.
Using the Accept header if that works would be the best way, in my opinion.
I wanted to use it to make the filename more accessible.
Yeah, that would be desirable. Can you try adding the queue Id and name to QueueEventsResponse? In case that's difficult, we can fallback to a generic name.
There was a problem hiding this comment.
@piotrwasko Can you fix the tests? Looks like you are missing some imports
|
Is this pr right ? |
PR resolves #165