I don't know if DashboardService is the place where this report should be placed, but this is where this report has always been, so I'm happy to discuss it

yeah I'm not a big fan of these huge general purpose controller and service classes. I'd much rather have a specific InventoryReportService, or even ReorderReportService. That way we have better encapsulation of logic and it incentivises building small, re-usable sub-components that we can hook in everywhere

ultimately I'm fine with this as it is though so I leave it up to you

yeah I'm not a big fan of these huge general purpose controller and service classes. I'd much rather have a specific InventoryReportService, or even ReorderReportService. That way we have better encapsulation of logic and it incentivises building small, re-usable sub-components that we can hook in everywhere

ultimately I'm fine with this as it is though so I leave it up to you

Shouldn't we maintain getting location via /facilities/$facilityId in the uri?

if you just do gt it includes nulls?? Weird...

what's the advantage here of building a map of closures vs just having a switch statement based on InventoryLevelStatus? I'm fine with this (though I'd be curious to know the performance difference ie how much of it is pre-compiled) but I don't see much difference compared to:

Boolean x
switch(command.inventoryLevelStatus) {
    case InventoryLevelStatus.IN_STOCK:
        x = ...
    case ...:
        x = ...
}
return x ? build...() : null

also, neat! I didn't know findResults existed

I actually haven't considered (and haven't thought about) the switch, as after finding out nested closures made me want to use them everywhere 🙈 so I can't say what is more performant, but I'd say the difference would not be even noticeable.

why the call to first()? Are we ever expecting there to be more than one level per product?

If they're one-to-one, then I'd change the above to:

Map<Product, InventoryLevel> inventoryLevelByProduct = inventoryLevels.collectEntries { [it.product, it] }

I forgot about the .collectEntries, thanks

IMO we shouldn't need to invoke taglibs unless we're trying to build html. The way I look at it is g.message is what we use for GSPs.

I recently refactored our l10n logic to create the MessageLocalizer class for this use case (where we want to localize a message within a service for use in JSON or CSV responses).

You can call it with: messageLocalizer.localize("product.label") or messageLocalizer.localize(new LocalizableMessage("product.label", "Product"))

I won't force you to use it though.

that's awesome, I have forgotten (or haven't even seen 🙈 ) you have created a util for that. I will check it out and eventually do the refactor.

@ewaterman I refactored the code to use the messageLocalizer. I have one suggestion though - it feels like with the current state of the MessageLocalizer class, we don't use the defaultMessage at all.
The same thing with the LocalizableMessage - we can provide it via constructor like you showed, but in the end we just call localize method with the message.code and message.args (defaultMessage is not used at all).

I did that intentionally actually. I saw a few places in the code where we were using a label that no longer existed and so it was always using the default but I couldn't tell because I was always working in english. I figured if we always displayed the label when we it failed to localize (instead of a default) then we'd see the problem right away and could fix it immediately.

However I do realize that there are benefits to having a default, such not totally breaking text when we accidentally remove a label that was being used.

If you don't want to do it here, I can create a separate PR to support defaults, but if you want it in this PR, you can modify MessageLocalizer.localize to add a String defaultMessage param after code (and then removing the If the code does not resolve... part of the docstring

@ewaterman if it's intentional, then I'm fine with it. Actuallly thanks to that, it made me realize a few existing labels (copied from the existing report) didn't actually exist.
I think the only reason to have the default would be if we could answer if it's even possible for the localizer not to work. If we are 100% sure it would work, I think we should even avoid using default to (as you mentioned) quickly detect not existing labels.

I'm not a huge fan of having a method in product availability service that requires a reorder report context object. It needlessly ties the two features together and forces the called service to have knowledge about the service calling it. IMO better would be to either:

1. spread the command fields out as individual method args or if that's too messy...

2. create a separate context object that this method accepts that doesn't reference the feature that uses it (and then convert ReorderReportFilterCommand to that object when calling this method).

not a required refactor though if you disagree

Good point. I assume we'd need 4 args to be passed, so if you are fine I will do it this way.
I'm also curious about the 2nd point how you would see it - what exact context object would you see in this place?

yup that's fine. I'd actually prefer that for now.

The "context object" would just be a POJO that wraps the params:

getInventoriesByProduct(ProductAvailabilityFilterContext context)

That way we don't get stuck with a method that has a really big list of params, most of them optional. In this situation though, I'd just list the params like you did.

I try not to overuse context objects because it somewhat masks the params and requires us to init a new object whenever we want to call the method, but it does help in some situations

why inList and not eq? You only have a single location

We have at least one location (current location), but you can pass additionalLocations so that you end up with the list.
I thought it would be form oversubstance if I were to do :

if (command.additionalLocations) {
  inList("location", locations)
} else {
  eq("location", AuthService.currentLocation
}

What do you think?

if you don't add this outer if check does it break if both are empty? I'm not certain but I think you could also do something like this if you prefer:

if (command.categories) {
    inList("product.category", command.categories)
}
if (command.tags) {
    ...
}

I'm wondering what I was thinking when writing this line:

if (command.categories || command.tags) {

XD

....

I was probably tired and wanted to push the code ASAP, so thanks for catching those weird cases 😆

TL:DR - it's definitely enough to do it like:

  if (command.categories) {
    inList("product.category", command.categories)
}
if (command.tags) {
    ...
}

@ewaterman Oh, I remember the reason for doing it. If I were to separate it, I would end up with:

if (categories) {
      product {
          inList("category", categories)
      }
}
if (tags) {
    product {
         tags {
            'in'("id", tags.id)
        }
    }
}

and if both categories and tags were provided, an error would be thrown that product is already joined.
I have usually handled it via usedAliases that I was storing in Set, but I believe for this simple filter, it's better to just check at the top if either one of categories/tags is provided and join product once.

yeah that's fine. Thanks for looking into it. I wonder if this would work though:

if (categories) {
    inList("product.category", categories)
}
if (tags) {
    inList("product.tags", tags)
}

It wouldn't. We either need to join the table like I did, or to use createAlias:

Can you use Instant.now() and Instant.now().plus(expirationFilter.days, ChronoUnit.DAYS) instead? Do Restrictions support it?

I haven't considered Instant as I thought if we have the expirationDate as Date we should rely on Date class. Unless Instant is an extension of Date and we can use Instant methods for Date? 🤔

This should hopefully be independent from that since all we're using it for here is to query the db and the JDBC already supports Instant

I thinkt it would be worth to have it in params (command), and if not specified then a fallback to current location. So user can technically request a report for another location without manually changing it.

Ok now I see that you have the additionalLocations, so why it is not used here? You are pulling the inventory levels only for the current location, but later it looks like you are mixing levels for products from additional locations without having the levels for the correct inventories here. Is this true and expected or am I missing something here?

As discussed on Slack, this behavior is expected, that the current location is the "central" one, and for additionalLocations we only want to sum the qatp - the inventory level values should be taken from the central locations and shouldn't include values from additionalLocations.
I will add a comment in the code explaining that as it might be confusing