OBPIH-6193 Fix the bin locations report export#4547
OBPIH-6193 Fix the bin locations report export#4547awalkowiak merged 3 commits intofeature/upgrade-to-grails-3.3.10from
Conversation
kchelstowski
requested review from
alannadolny,
awalkowiak,
drodzewicz and
jmiranda
jmiranda
left a comment
jmiranda
left a comment
There was a problem hiding this comment.
Other than the comments below, I wanted to ask whether this export would ever be used for importing data at any time? If so, we have a potential problem if that unitCost should be updated since we'd be setting the value to NULL.
I don't think that's an issue here, but we need to be careful when performing redactions in other exports like the Export Products CSV and then use the export as input to the Import Products feature. It might be better to have a special value (an enum?) that represents a redacted data element. NULL should not be used for this purpose since it's already used to mean "data was not provided".
But this is probably a topic for discussion on the ticket since we'll need Manon and Kelsey to weigh in.
| unitCost : formatNumber(number: it.unitCost), | ||
| totalValue : formatNumber(number: it.totalValue) | ||
| unitCost : hasRoleFinance ? formatNumber(number: unitCost) : null, | ||
| totalValue : hasRoleFinance ? formatNumber(number: it.quantity * unitCost) : null |
There was a problem hiding this comment.
I would recommend keeping the logic hidden behind these convenience methods.
unitCost : hasRoleFinance ? formatNumber(number: it.unitCost) : null,
totalValue : hasRoleFinance ? formatNumber(number: it.totalValue) : null
There was a problem hiding this comment.
Or justify the switch to moving the logic here.
| def products = binLocations.collect { it.product.productCode }.unique() | ||
| binLocations = binLocations.collect { | ||
| List<BinLocationItem> binLocations = inventoryService.getQuantityByBinLocation(location) | ||
| List<String> products = binLocations.collect { it.product.productCode }.unique() |
There was a problem hiding this comment.
I was so confused by these two lines. I thought the unique products were then going to be used to manipulate the binLocations somehow and was going to suggest creating another service method to return the result.
Then I realized products is only used to return a productCount. So I would recommend just returning the count here and setting a default value.
int productCount = binLocations.collect { it.product.productCode }.unique()?.size() ?: 0
There's gotta be a more Groovier way of doing this ^^ no? I spend a few minutes and couldn't find what I was looking for, but it feels like something we should be able to do more succinctly.
There was a problem hiding this comment.
I agree with you, initially I didn't pay much attention to that, since I did only a cosmetic change (def -> type) for that line.
I think I can try something with .groupBy.
There was a problem hiding this comment.
I went with the .groupBy approach. I hope it's cleaner now.
awalkowiak
left a comment
awalkowiak
left a comment
There was a problem hiding this comment.
Please remove one unnecessary import, other than that lgtm
| import grails.plugins.csv.CSVWriter | ||
| import grails.plugins.quartz.GrailsJobClassConstants | ||
| import org.apache.commons.lang.StringEscapeUtils | ||
| import org.grails.buffer.StreamCharBuffer |
There was a problem hiding this comment.
yeah, I'm sorry, forgot to remove it
3 participants
No description provided.