I think this also should/could include TransactionCode.INVENTORY. I think it was used in the past for marking the record stock done through import inventory. At least we are still looking into this type while calculating transactions for product availability and the latest inventory dates indicator.

I am not sure about that, this subquery for getting the last stock count date is the same as in the product view (I copied this part of the query from there).
So, this method is called in the view:

This is the method that contains the query:

But, if you are sure about it, I can change that

Sorry just seeing this now. I would prefer NOT to include Inventory. We have removed all of the features that created this type of transaction so if there's data in the database it's very old and somewhat irrelevant. In other words, if a transaction exists that has an "INVENTORY" transaction code I'd be fine for the last stock count feature to return Never.

@jmiranda do we have all these types of decisions documented somewhere? I based my comment (suggestion) on the past behavior, and state of the current code where we are still looking into this transaction type, and there is some dead code in the InventoryService that is using it and it was a bit misleading. Perhaps we could at least add some comments about it in the TransactionCode enum?

and state of the current code where we are still looking into this transaction type

I think you're misread what I said.

Justin: "We have removed all of the features that created this type of transaction"

If that's not true and there's still code that creates INVENTORY transactions then please flag them and let me know.

do we have all these types of decisions documented somewhere?

Yes, this particular issue was documented before you guys were involved.
#165

The underlying issue is the QoH Calculation issue that is near and dear to my heart. It was recently documented here but there are probably countless discussions on this issue throughout tickets, documents, and community posts.
https://pihemr.atlassian.net/browse/OBPIH-6353

The pull request for #165 is this epic roller coaster.
#710

I was only able to get to a place where we could convert the INVENTORY transactions to ADJUSTMENT transactions on a product by product basis. The idea was to eventually move this to an automated feature that would convert everything, but I wasn't comfortable with that since there were some edge cases that I wasn't absolutely sure about.

So I pushed that code and converted all of the transactions in the PIH and SBHF databases. But I have been holding out hope that I'd someday get become independently wealth so I could fund my own development priorities or have time to work on it as tech debt using PIH's budget. However, it's pretty far down on the tech debt priority list, so I think it'll probably be refactored out completely once we have time to work on the Cycle Count feature and the data model refactorings around that.

Perhaps we could at least add some comments about it in the TransactionCode enum?

Yes, that's probably a good idea.

Please be a bit more descriptive here with bt and sc since it does not match any table name and it's hard to decode

I think "Never" would fit here more, unless "None" was requested, then you can disregard this comment

it was requested

@awalkowiak I guess this is the sort of detail we need to catch during "design".

One additional point (add to #3849)

1. We should use an enum if there are multiple coded classification values (i.e. Never, Long Ago, Somewhat Recently, Recently)
2. We should use a Constant if we need to support just a string literal in logic (i.e. Default, Never, None).
3. We should allow constants and enums to be i18n'd, so there's a way to translate the actual string literal.

Add it right under the default constant and call it never.

static final String DEFAULT = "DEFAULT"
static final String NEVER = "NEVER"

@jmiranda

I guess this is the sort of detail we need to catch during "design".

This particular label did not appear in the ticket at all, and it was just suggested by Manon outside the ticket. So it is hard to speak about design here if it was missed since the beginning. I am also not sure if I would spot it even with a proper dev design because I would be focusing on the SQL query itself, it is hard to tell.

(added a topic around label and messages designing for a tech huddle)

FWIW, in case it wasn't clear ... this was not criticism of what we should have done here but rather what patterns we should be looking for when we start to do proper design in the near future.

I think we want to do something like this to allow for i18n.

default.lastInventoryCount.NEVER.label
default.lastInventoryCount.NONE.label

Then the value could be a constant, enum, or null.

it[3] as String ?: Constants.DEFAULT // or TimeEnum.NEVER or null

And then somewhere else we might try to resolve this value to an i18n string

if (whatever.lastInventoryCount) { 
    formatDate(whatever.lastInventoryCount)
} else {
    return g.message(code: "default.lastInventoryCount.never.label")
}

This formatting is premature. We should return the date (or NULL) and then let the caller / client handle the rendering logic (i.e. formatting in a specific locale). And what I didn't realize was that we could just return NULL and allow the renderer to conditionally render the value properly. So probably don't even need the constant, definitely want to return a date and have the renderer handle the date format.

These need to be translatable strings. We do not need to handle that here, but we need to provide this as feedback in the ticket to make sure that Manon (at least) knows that these labels are hard-coded (along with the None label) and cannot be translated.

One way to tackle the translatable label problem is to require the ticket creator to specify multiple translations for each string at the beginning to make it clear that there's a i18n requirement.