OBPIH-6015 Endpoint to fetch product sources data with filters, pagination, sorting (post-code review fixes)#4457
Conversation
kchelstowski
requested review from
alannadolny,
awalkowiak,
drodzewicz and
jmiranda
kchelstowski
added
warn: do not merge
| List<ProductSupplierListDto> productSuppliers = productSupplierService.getProductSuppliers(filterParams) | ||
| render([data: productSuppliers, totalCount: productSuppliers.totalCount] as JSON) | ||
| List<ProductSupplier> productSuppliers = productSupplierService.getProductSuppliers(filterParams) | ||
| render([data: productSuppliers.collect { it.toJson() }, totalCount: productSuppliers.totalCount] as JSON) |
There was a problem hiding this comment.
I think you can define a Marshaler for ProductSupplier and avoid doing this collect{ it.toJson() }
I remember we were talking about it not too long ago but I am not sure if we want to move away from Marshalers and toJson() or are we still doing it?
Also made a discussion #4427 that I wanted to bring up on one of the tech-huddles but was never able to cause we had more important things to discuss.
So I would add the Marshaler for now, unless there are counterarguments to it.
There was a problem hiding this comment.
@jmiranda wanted me to revert my convention with the dto pattern and do it .toJson() way.
I remember we were talking about it not too long ago but I am not sure if we want to move away from Marshalers and toJson() or are we still doing it?
@jmiranda said he would eventually like to get rid of the marshallers.
I'm also against them, because by doing it .toJson() way, you immediately can see what will be returned, while with marshallers you can see in the method something like return productSuppliers which is List<ProductSupplier> and in the response you suddenly see some other dto.
| Integer offset | ||
|
|
||
| Integer max = 10 | ||
| Integer max |
There was a problem hiding this comment.
Probably I missed some part of the discussion about it, but what was the reason for deleting the default value of those fields?
There was a problem hiding this comment.
@alannadolny
description:
QA fixes:
I've removed the default values for pagination params, because Katarzyna suggested that if we didn't provide any pagination params, we should be getting all product sources.
|
@kchelstowski please fix the conflict |
…play name instead of id of the preference type
kchelstowski
force-pushed
the
OBPIH-6015-fix
branch
from
d9c4bfd to
6559d2e
Compare
…ation, sorting (post-code review fixes) (#4457) * OBPIH-6015 Revert DTO classes and use default Map toJson() * OBPIH-6015 Move sorting logic to a separate method * OBPIH-6015 Make the product name column a link that redirects to the stock card page * OBPIH-6015 Remove default settings for offset and max pagination params * OBPIH-6015 Include more fields for search field filter on product sources list * OBPIH-6015 Fix supplier code column and preference type column to display name instead of id of the preference type * OBPIH-6015 fix conflicts and eslint
4 participants
Implemented @jmiranda 's suggestions from the PR: #4435
delegateproperty.eachPricetounitPriceQA fixes:
Improvements that were forgotten previously:
productIdproperty to the dto).