Maybe it would be better to call it startDate and endDate to keep it consistent with the rest of our app? For example in InventoryAuditCommand and CycleCountReportCommand we use those names, and Alan’s frontend seems to be set up for them as well 🤔

Actually we don't have it standardized, as we use createdFrom/createdTo also, along with startDate/endDate - we probably have to decide on the convention, so thanks for pointing that out

Can’t we do something it like this?
eq("transactionType", TransactionType.read(Constants.EXPIRATION_TRANSACTION_TYPE_ID))

good catch, the query was quickly written hence I forgot we had consts for that.

offset and max are nullable in PaginationParams so best to check for non-null before using them.

no need to do it - if they are not provided, no pagination will be applied

I'm not a fan of extending PaginationParams here. I see class inherritance as "childClass is a parentClass" so not really applicable here. I also try to avoid it when possible because you can only ever extend one class, so it could get us stuck in the future if we want to extend something else.

You're essentially using PaginationParams as a way to group commonly used params so why not make it a field in ExpirationHistoryReportFilterCommand instead?

Or if you really want to use inherritance you could do it via traits (which I see as "childClass has attributes of trait":

trait PaginatedQuery {
    PaginationParams paginationParams
}

class ExpirationHistoryReportFilterCommand implements PaginatedQuery { ... }

This saves us from being stuck extending from something and so in the future if we want to implement other common traits, we can.

I don't know how that exactly works for triggering validation though...

Hm, good point @ewaterman. I have always struggled with a proper way to use inheritance. Would you then say that wherever we extend the PaginationParams or PaginationCommand we do a mistake? Now that you've just said we could just use it as a field, I started to wonder when we actually would prefer extension over having a field. I'd like to know more about that so if you have any quick examples I'd be grateful.

My general rule is:

• can it be represented as "X HAS Y" -> use traits or fields
• if it only makes sense to be represented as "X IS (and only is) Y" -> use inherritance

So in this case, ExpirationHistoryReportFilterCommand HAS PaginationParams.

Confusingly, you could also say ExpirationHistoryReportFilterCommand IS a PaginatedCommand, but is it ONLY a PaginatedCommand? I don't think so.

I prefer to use traits/fields over abstract inherritance whenever possible because inherritance is restrictive. If we use extends and then later want to introduce other common behaviour to the command, then we're stuck.

For example, lets say we try to get smart and create a common way to filter stuff:

abstract class FilteredCommand implements Validateable {
    List<Filter> filters

    static constraints = { ... }
}

class Filter implements Validateable {
    FilterRule type   // an enum with GREATER_THAN, LESS_THAN, BEFORE, AFTER...
    String fieldName
    String filterValue

    static constraints = { ... }
}

We want to do ExpirationHistoryReportFilterCommand extends PaginatedCommand, FilteredCommand but we can't because you can only extend a single class!

To solve this I'd probably convert both into traits (I haven't tested this code so no idea how it works with constraints):

ExpirationHistoryReportFilterCommand implements Paginatable, Filterable { ... }

trait Paginatable implements Validateable { ... }
trait Filterable implements Validateable { ... }

So we could say ExpirationHistoryReportFilterCommand has the "Paginatable" and "Filterable" traits.

But when is extension preferred? I don't have the perfect answer.

As I said above, I use it when the "X is (and only is) Y" rule applies and "X has Y" doesn't make sense, but that isn't always clear. Generally I find when I'm trying to build something where there is a lot of common logic between children implementations, and that logic can't easily be broken out into helper classes/traits, then I might use extension because it's just very convenient. But that also might be a code smell that signals the logic is overly intertwined...

If you're including facilityId in the URI you should probably use that instead

It's actually a good point if we should. I mean, if we should use the facility in the URI... I did it as it was pointed out during the last PR, but there is a requirement in the ticket that the location is always supposed to be the current location, so I'm wondering if we should force to have the facility id in the URI (and then have to validate that if the one provided in the URI is your current one), or just get rid of that in the URI and keep using currentLocation in the code like it is now.

that's a very good point. I think we need to make a proper decision about how and when to use the URI approach and when to rely on the session. If we always only want to use the user's current location, then yes it makes sense to not include it in the URI. But if in the future we're going to want to be able to request reports for different locations, then it makes sense to include in the URI

same here. If you're using facilityId in the URI we should use that.

class ExpirationHistoryReportFilterCommand implements Validateable {

    Location facility

    def beforeValidate() {
        String facilityId = RequestContextHolder.getRequestAttributes().params?.facilityId
        facility = Location.findById(facilityId)
    }
}

Explanation above - let's decide whether we do it this way or get rid of the facility in the URI.

Given this is static, ExpirationHistoryReportRow.buildExpirationReportRow(it) seems like a needlessly repetitive name. How about ExpirationHistoryReportRow.from(it) or ExpirationHistoryReportRow.of(it)?

how about fromTransactionEntry to have it specific?

yeah that's fine. I feel like the X in "fromX" is implied by the type of the param. Meaning IMO:

ExpirationHistoryReportRow from(TransactionEntry transactionEntry) { ... }

already implies "fromTransactionEntry", but I'm fine either way.

Since we loop through the rows anyways, I'm confused why CSVWriter requires us to do this when building the header:

"${messageLocalizer.localize("transaction.transactionNumber.label")}" { it?.transactionNumber }

and not just:

${messageLocalizer.localize("transaction.transactionNumber.label")}

Anyways, in cycle count we do this:

Mainly:

CSVPrinter csv = CSVUtils.getCSVPrinter()
csv.printRecord(...)

which seems easier. Any reason to do differently here?

We don't seem to have the CSV stuff standardized so I was just following the code from one of the latest CSV. If I had seen the CSVPrinter when searching for an example, I would have probably used it as definitely it seems easier.

Answering that though:

I'm confused why CSVWriter requires us to do this when building the header:

It's because then when I add a row, I could potentially switch the order in the built map, and if I kept the property names, it would still work, so I could do:

 "${messageLocalizer.localize("transaction.transactionNumber.label")}" { it?.transactionNumber }
 "${messageLocalizer.localize("transaction.date.label")}" { it?.transactionDate }
 "${messageLocalizer.localize("product.productCode.label")}" { it?.productCode }

and then:

csv << [
   productCode: row.productCode,
   transactionDate: row.transactionDate,
   transactionNumber: row.transactionNumber,
]

thanks for the explanation. That makes sense

@ewaterman I decided to remove the facility id in the URL since this is always supposed to be current location, so we'd have more work with binding and validating the id.

this drove me nuts. it turned out you have to pre-initialize nested objects that are not domain objects (for those, the DomainClassBindingHelper comes in), otherwise no params would be bound when using query string params.
I know we can just send it in JSON body, but I think we want to support also the query string params.
The reason why it has to be preinitialized is that when binding, the paginationParams is null, so Groovy doesn't know to which object it should bind the offset/max params

this can't be abstract anymore, since we have to pre-initialize it in the command below

Do we also need to modify PaginationCommand then?

@SebastianLib I went with your suggestion to have startDate/endDate