OBPIH-6332 Dashboard indicator for number of backdated shipments#4628
OBPIH-6332 Dashboard indicator for number of backdated shipments#4628awalkowiak merged 13 commits intodevelopfrom
Conversation
| } | ||
|
|
||
| def getBackdatedOutboundShipments() { | ||
| Integer monthsLimit = (params.querySize as Integer) ?: 6 |
There was a problem hiding this comment.
Please put this 6 month default in a constant. Or even better as a config variable.
There was a problem hiding this comment.
Also, do we need to define a server-side max? Can the server properly handle the request if we get sent something like querySize=9999?
There was a problem hiding this comment.
I belive grails allows for reading parameter values in the following way params.int("querySize", 6) where it casts the value to integer and second argument of this method is the default value
| '5 days': 0, | ||
| '6 days': 0, | ||
| '7 days': 0, | ||
| '7+ days': 0, |
There was a problem hiding this comment.
I would suggest moving those "2 days" etc. to static constants in Constants.groovy, as this might be reused in the future.
| @Cacheable(value = "dashboardCache", key = { "getBackdatedOutboundShipments-${locationId}${monthsLimit}" }) | ||
| Map getBackdatedOutboundShipmentsData(String locationId, Integer monthsLimit) { | ||
| String query = """ | ||
| SELECT ( |
There was a problem hiding this comment.
I guess the indentations are missing here
| AND t.date_created > :timeLimit | ||
| GROUP BY days_backdated | ||
| HAVING days_backdated > :daysOffset | ||
| """ |
There was a problem hiding this comment.
the only difference between those two queries if origin vs destination. Can we somehow pass it as a variable here so that we can reuse the query in both methods?
There was a problem hiding this comment.
I am not sure if this is a good idea, I would rather say string concatenation in SQL is a thing that we should avoid.
As an example:
https://stackoverflow.com/questions/23179329/why-is-concatenating-sql-strings-a-bad-idea
If anyone else would like me to use the string concatenation here I can do it, but I am not a big fan, and that's the reason I didn't do it before.
There was a problem hiding this comment.
I know cases when concatenation is a wrong idea, and this is why they introduced an ability to use params via :param and pass it as the second argument of a query method, but in this case:
- you wouldn't use any variable that the user provides and that might break the query (e.g.
drop database openboxes;😆 ), but a variable that you would pass, - I was not really thinking about the concatenation, but rather passing it as param (if possible, probably not), or via
${variable}.
I'm not sure if any of those options are possible, hence I just asked if "we can".
grails-app/conf/runtime.groovy
Outdated
| openboxes.client.autosave.enabled = false | ||
|
|
||
| // Backdata configuration (OBPIH-6332) | ||
| openboxes.dashboard.backdate.daysOffset = 1 |
There was a problem hiding this comment.
Let's use backdatedShipments in these two names to match the indicators these are used for.
| static final String UOM_EACH_ID = "EA" | ||
|
|
||
| // Dashboard indicator axes | ||
| static final Map<String, Integer> backdataAxes = [ |
There was a problem hiding this comment.
Do you mean axis or axes?
There was a problem hiding this comment.
Axes is the plural of axis.
There was a problem hiding this comment.
nvm, you're right, plural is axes
I asked Manon about adding the second indicator within this PR (the backdated inbound shipments indicator), so I am pushing it here