Skip to content

OBPIH-6332 Dashboard indicator for number of backdated shipments#4628

Merged
awalkowiak merged 13 commits intodevelopfrom
OBPIH-6332
May 20, 2024
Merged

OBPIH-6332 Dashboard indicator for number of backdated shipments#4628
awalkowiak merged 13 commits intodevelopfrom
OBPIH-6332

Conversation

@alannadolny
Copy link
Collaborator

I asked Manon about adding the second indicator within this PR (the backdated inbound shipments indicator), so I am pushing it here

}

def getBackdatedOutboundShipments() {
Integer monthsLimit = (params.querySize as Integer) ?: 6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put this 6 month default in a constant. Or even better as a config variable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the indentations are missing here

AND t.date_created > :timeLimit
GROUP BY days_backdated
HAVING days_backdated > :daysOffset
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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,
  2. 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".

openboxes.client.autosave.enabled = false

// Backdata configuration (OBPIH-6332)
openboxes.dashboard.backdate.daysOffset = 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean axis or axes?

Copy link
Collaborator Author

@alannadolny alannadolny May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Axes is the plural of axis.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, you're right, plural is axes

@awalkowiak awalkowiak merged commit 7030e04 into develop May 20, 2024
@awalkowiak awalkowiak deleted the OBPIH-6332 branch May 20, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants