OBPIH-6425 Make strings translateable via crowdin in pdf documents

[kchelstowski]

No description provided.

[jmiranda]

Looks good. Just a little cleanup for now and a future refactoring consideration re: state machines. I've added the future consideration to my tech debt backlog so don't stress. I just wanted to show what I was thinking.

[jmiranda]

A few recommendations related to i18n message keys:

1. Let's change the namespace to putaway for putaway-related labels and putawayItem for labels related to putaway items. These seem to be related to putaway items. Putaway Order is just what we call the resource internally but it's just a putaway and putaway item.
2. Make sure you keep it as putaway i.e. do not use putAway.
3. Also, make sure the i18n key spells out the thing it's describing unless the thing is abbreviated. For example, in putawayOrder.totalQty.label, totalQty should be totalQuantity unless we actually want both and need to differentiate between the full and abbreviated versions like this.
   • putawayItem.totalQty.label=Total Qty
   • putawayItem.totalQuantity.label=Total Quantity
4. Some of these labels are related to putaway items, but most of them are related to other resources associated with the putaway item (i.e. the product or inventory item). Threfore the putaway or putawayItem namespace doesn't make sense.
   1. Therefore it would preferable to always use a shared i18n key across all UIs unless there's a good reason to allow it to be overriden on a certain page or document. If there's no reason to override the label, let's change the labels accordingly
      • product.productCode.label (or default.code.label)
      • product.name.label (or default.name.label)
      • inventoryItem.lotNumber.label (default.lotNumber.label)
      • inventoryItem.expirationDate.label (or inventoryItem.expiry.label, default.expiry.label)
   2. And anything that's related to the putawayItem might look like this (keep the property names the same as the domain even if the label is different - sometimes we used Bin in places where I would have used Location).
      • putawayItem.quantity.label=Putaway Quantity
      • putawayItem.totalQuantity.label=Total Quantity
      • putawayItem.preferredLocation.label=Preferred Bin (or inventoryLevel.preferredBinLocation.label)
      • putawayItem.currentLocations.label=Current Bins
      • putawayItem.putawayLocation.label=
   3. The other option would be to create a completely separate namespace for each page, document, etc. That would allow us to correctly represent what these labels
      • document.putaway.putawayItem.productCode=Code
      • document.putaway.putawayItem.productName=Name
      • document.putaway.putawayItem.lotNumber=Lot Number

[kchelstowski]

I agree with you, but I just wanted to follow the convention used in that file. I assume this is one of the "future considerations" and not a change request?

[awalkowiak]

Yeah I think we should do an overall cleanup in all the message properties, but I would assume that we need to finalize the conventions around it (I am adding a link to Justin's comment there), plus we could create a tech debt ticket for this
https://pihemr.atlassian.net/wiki/spaces/OB/pages/2782035970/Backend+coding+conventions#i18n-conventions

[jmiranda]

Summary

Recommendations

1. Understand what state objects we're working with (command vs enum)

I would recommend the following change. Explanation below.

<g:message code="enum.ShipmentStatusCode.${shipment?.status?.code?.name}" default="${shipment?.status}" />

Future Considerations

2. Implement a displayStatus for Shipment, ShipmentStatus, and ShipmentStatusCode.

The default should just grab the display status from the shipment which just queries the status object and the enum for a display status. See details below.

<g:message code="enum.ShipmentStatusCode.${shipment?.status?.code?.name}" default="${shipment?.displayStatus}" />

Details

1. Understand what objects we're working with
   We need to be careful here (note: apologies for the confusion; this is due to the fact that we don't have a standard convention for statuses yet, so the status API across objects is not consistent).

shipment?.status returns a ShipmentStatus (command), not a ShipmentStatusCode (enum). In this case, everything probably works fine because the toString() methods are wired to the same value and all actors are cooperating. But it's a good idea to be explicit so we don't get ourselves into trouble. Debugging a toString() regression can sometimes be fun, but not always.

Here's what values are returned by each of these properties / getters in the ShipmentStatus/ShipmentStatusCode , but note that the class is different. So this only works because we wired the toString() and getters to all use the same values.

stockMovement?.shipment?.status = PENDING (class org.pih.warehouse.shipping.ShipmentStatus)
stockMovement?.shipment?.status.name = PENDING (class java.lang.String)
stockMovement?.shipment?.status.code = PENDING (class org.pih.warehouse.shipping.ShipmentStatusCode)
stockMovement?.shipment?.status.code.name = PENDING (class java.lang.String)

So it should work fine as-is, but it's brittle and not future-proof. For example, Grails or Groovy might change the API for dealing with enums.

2. Implement a displayStatus for Shipment, ShipmentStatus, and ShipmentStatusCode.

I would like for the default attribute to

<g:message code="enum.ShipmentStatusCode.${shipment?.status?.code?.name}" default="${shipment?.displayStatus}" />

where shipment?.displayStatus renders itself as a localized message string.

@drodzewicz implemented this in StockMovement using a separate getLabel() method, so you could do something similar.

But the impleentation for shipment should be much easier using either

the format metadata taglib.

${format.metadata(obj: shipment?.status?.code}
<format:metadata obj="${shipment?.status?.code}"/>

or by blessing the state machine classes with methods to help them display their own status message (see details below)

<g:message code="enum.ShipmentStatusCode.${shipment?.status?.code?.name}" default="${shipment?.displayStatus}" />

Shipment

String getDisplayStatus() { 
    return status.displayStatus
}

ShipmentStatus

ShipmentStatusCode code
String getDisplayStatus() { 
    return code.displayStatus
}

ShipmentStatusCode

String getDisplayStatus() {
    return g.message(code: "enum.ShipmentStatusCode.${name}")
}

This last method might need to be a static method

static String getDisplayStatus(ShipmentStatusCode shipmentStatusCode) {
    def g = getApplicationTagLibFromHolder()
    return g.message(code: "enum.ShipmentStatusCode.${shipmentStatusCode.name}")
}

[kchelstowski]

@jmiranda yeah, I'm aware that it works because of this:

So this only works because we wired the toString()

but I also agree that it might be explicit, so that we are protected from any toString() refactors.
I will change that to shipment.status.code.name

[jmiranda]

I looked at the ticket and saw "null" in the list of things to translate.

But please don't do this. This is the type of thing we need to either push back on or ask clarification questions about. I honestly think the inclusion of "null" is a bug that was never fixed.

We have three options:

• Leave it blank (i.e. empty string) - my preference
• default.none.label
• default.na.label

If you want to get clarification from the team please request clarification in the ticket. Otherwise, move forward with the "leave it blank" option.

[kchelstowski]

@jmiranda I usually assume that a ticket is written properly and I don't have to overthink every single bullet of the ticket, and since it wasn't challenging to implement this, I did it, but @awalkowiak pointed that out and asked Kelsey about her opinion on that, and she felt ok with leaving it empty, which I will fix now.

[kchelstowski]

Moreover - while getting to this bullet, I let Katarzyna know, that the requirement feels weird to me, but I didn't get any reaction like "wtf", so I assumed that maybe I was weird that I complained about that 😄

[awalkowiak]

Is it still used?

[awalkowiak]

I am merging this as is (I am considering doing a broader message properties refactor a separate tech debt ticket, which we can do after we finalize conventions here https://pihemr.atlassian.net/wiki/spaces/OB/pages/2782035970/Backend+coding+conventions#i18n-conventions (I linked Justin's comment there))