OBPIH-6213 Create a StockMovementStatus helper which manages displaying statuses for all cases #4589

drodzewicz · 2024-04-22T13:15:23Z

Where do I begin...

This was quite an overwhelming investigation and there is at the moment at least one case that I am not sure how to approach in a generic way. The case is for displaying statuses for requests on stock request dashboard and inbound list (will discuss ion the tech-huddle)

I tried to align my approach with the one provided by Kacper in #4587 , which os basically creating a Helper/util class for a centralized manager for displaying statuses.

One thing I noticed from the requirements is that there are cases where a list page should show a one set of statuses but once we go to a show page we see other statuses: (Inbound, Outbound list page vs show page where we have a combination of both shipment statuses and requisition statuses) which is why I created two separate methods for displaying the status, one for list pages and one for show pages

follow discussion on OBPIH-6213

kchelstowski

I will come back to review this, when it's finished, but since I saw this one small thing already, I decided to point it out now.
Anyway - so far, good job with the refactor after Monday's tech huddle 👏

kchelstowski · 2024-04-24T13:53:37Z

src/main/groovy/util/StockMovementContext.groovy

+ Requisition requisition
+
+ StockMovementDirection getStockMovementDirection(Location currentLocation) {
+ if (currentLocation == shipment?.origin) {


I'm not sure if we could get rid of the origin and destination properties and just use the shipment ones. Logically, it would be ok, but looking at the view (stock-movement-list-item or stock-movement) the origin and destination is taken from the requisition OR order (if it's return), not from the shipment.
Probably there is no chance the shipment origin/destination pair would be different from the requisition/order's origin/destination, but maybe we should be safer?

EWaterman · 2024-04-25T19:14:55Z

grails-app/domain/org/pih/warehouse/inventory/OutboundStockMovement.groovy

@@ -229,6 +230,19 @@ class OutboundStockMovement implements Serializable, Validateable {
 return shipment?.currentStatus == ShipmentStatusCode.RECEIVED
 }

+ @Deprecated


can you add a comment here discribing why we're deprecating this and what to use instead? It's useful for future us to remember.

Do we actually want to deprecate this method?

If so, Evan is correct. As stated in the Java conventions, we should use @deprecated and add a @deprecated javadoc comment that provides more context / reason for the deprecatation as well as an alternative, if one exists.
https://docs.oracle.com/javase/1.5.0/docs/guide/javadoc/deprecation/deprecation.html#how

/** * @deprecated as of 0.9.x because it was not good; use {@link #theNotDeprecatedMethod()} instead. */ @Deprecated getDisplayStatus() { ... }

Here's a link to a stackoverflow with some examples.
https://stackoverflow.com/questions/9031832/how-to-declare-or-mark-a-java-method-as-deprecated

I'm also wondering why we actually want to deprecate the method we've just created? Did I miss something?

Unless I misunderstood our conversation on the tech-huddle I remember you @jmiranda asking for it to be deprecated out of the gate since this is kind of a "workaround" before the actual refactor for statuses and to prevent developers from relying on this method.

I agree, my bad for not including any comment for it.

EWaterman · 2024-04-25T19:16:31Z

grails-app/domain/org/pih/warehouse/inventory/OutboundStockMovementListItem.groovy

@@ -102,17 +103,17 @@ class OutboundStockMovementListItem implements Serializable, Validateable {
 statusSortOrder(nullable: true)
 }

- def getDisplayStatus() {
- StockMovementStatusHelper statusHelper = new StockMovementStatusHelper(
+ @Deprecated


same here. Please document why we're deprecating and what we should use instead.

EWaterman · 2024-04-25T19:16:40Z

src/main/groovy/org/pih/warehouse/api/StockMovement.groovy

- default:
- return getTranslatedDisplayStatus(shipment?.status?.code)
- }
+ @Deprecated


same here. Please document why we're deprecating and what we should use instead.

EWaterman · 2024-04-25T19:40:10Z

src/main/groovy/org/pih/warehouse/api/StockMovement.groovy

- return getTranslatedDisplayStatus(shipment?.status?.code)
- }
+ @Deprecated
+ Map<String, String> getDisplayStatus() {


this is really stretching the limit of what a transient field should be (which is a very simple computation such as taking a timestamp field and converting it to a date). Every time we need to compute this value we're doing 5 additional lookups, which is really heavy.

That said, I don't know what GORM is doing behind the scenes. If it's pre-fetching all of these foreign keys when we fetch the stock movement itself and then re-using them here, then it's probably fine. I'm just a teeny bit scared that it's not that smart and is doing an extra lookup for each of these on top of the regular lookup when fetching them for the stock movement fields. I don't know if there's any easy way to verify that though

This is really stretching the limit of what a transient field should be (which is a very simple computation such as taking a timestamp field and converting it to a date).

Honestly, a transient property is just a way to indicate that "the return value of this method is derived and we don't need to store this property in the database".

Since StockMovement is a command class, we might not even need to use transient. The only reason to use it within a command class would be to avoid having to write a setter method, I guess.

Transient properties can have getter methods that are as lightweight or heavyweight as we need them to be but with the knowledge that a method that does a lot of database access should probably not be referenced in a haphazard manner that causes N+1 or M(N+1) problems. For example, invoking the method in the StockMovement.toJson() method and then calling an API that fetches and renders a list of all stock movements in the system.

Every time we need to compute this value we're doing 5 additional lookups, which is really heavy.

In this case, I don't see 5 additional lookups. I assume you mean the objects that are passed to the context. These objects have already been loaded into memory and there's not much of a chance of any access to lazy properties within those objects. So I think we should be good. But it would definitely be smart to make sure we're not doing anything dumb here.

That said, I don't know what GORM is doing behind the scenes. If it's pre-fetching all of these foreign keys when we fetch the stock movement itself and then re-using them here, then it's probably fine.

Yes, that's exactly what we're doing. Stock Movement is not a domain. It's a command class with references that have been fully hydrated by the time we're doing anything related to status rendering. But you're correct. If there is a lazy access on a property somewhere in the code we're using to render the status, then we should pre-fetch that data.

I don't know if there's any easy way to verify that though

At the moment, you could use a debugger or enable SQL logging (either in the app or the MySQL general log) to see what SQL is being generated.

It would be amazing if there was a developer plugin like the Django debug toolbar that provided this information in the page as we're developing.

There's also possibly a way to allow our NewRelic integration to provide some of this information while we are in dev mode. Right now, NewRelic is mainly used in production (and maybe some staging / dev servers). Another option might be to look to see if Sentry's APM provides a more developer-focused experience.

@EWaterman we are safe, but it's good that you care and point out such things. Basicallly @jmiranda is right - those should be lazily loaded while getting the stockMovement (e.g. in the def show method). Invoking some of the properties (like using them as properties of the helper class, which you pointed out) don't change anything (none SQL should be run).
It will make N+1 (do additional call to any of the properties - order/shipment etc.) whenever you try to access them "deeper", e.g. in this switch/case:

private static RequisitionStatus getOutboundReturnDisplayStatus(Order order) { switch (order?.status) { case OrderStatus.PENDING: case OrderStatus.PLACED: return RequisitionStatus.CREATED case OrderStatus.APPROVED: return RequisitionStatus.PICKING case OrderStatus.CANCELED: return RequisitionStatus.CANCELED case OrderStatus.PARTIALLY_RECEIVED: case OrderStatus.RECEIVED: case OrderStatus.COMPLETED: return RequisitionStatus.ISSUED case OrderStatus.REJECTED: return RequisitionStatus.CANCELED default: return RequisitionStatus.PENDING } }

here, we would make the fetch for the order, as we access a property of an order.

It should not be expensive anyway, since we should touch only 10 SMs at one time (default page size of a list page) and reducing the N+1 would not make sense at the moment, since we would probably have to join fetch all the associations, as we don't know, which of them would be used for a particular context (e.g. for outbound return, we would use order, whereas for an inbound return we would use the shipment, so there is no sense to join fetch shipment for outbound return and vice versa, and it's impossible to determine what would be needed at the level of initialization of the helper instance.)

If you feel like you still didn't get it in 100%, let me know so I could record a video for you with sql logging to show, what I mean here.

thanks for the in depth explanation guys!

EWaterman · 2024-04-25T19:51:01Z

src/test/groovy/unit/org/pih/warehouse/core/StockMovementStatusSpec.groovy

+
+ void "should return requisition status #expected for outbound stock movement returns"() {
+ given:
+ StockMovementContext context = new StockMovementContext() {


I recommend doing mocks/stubs this way instead of hard overriding the methods. It feels less hacky too me and it gives you the benefit of being able to do asserts on the stub such as asserting a specific method was called X times with Y args. Plus this way you can re-stub it later in the test if you want to change the behaviour. You're not doing either here, but still I think it's a good practice to stub it out

StockMovementContext context = Stub(StockMovementContext) { isReturn() >> true isInbound() >> false isOutbound() >> true }

EWaterman · 2024-04-25T19:56:00Z

I thought you said you were bad a writing tests 😎 These are awesome!

jmiranda · 2024-04-26T04:53:10Z

grails-app/domain/org/pih/warehouse/inventory/OutboundStockMovement.groovy

@@ -229,6 +230,19 @@ class OutboundStockMovement implements Serializable, Validateable {
 return shipment?.currentStatus == ShipmentStatusCode.RECEIVED
 }

+ @Deprecated


Do we actually want to deprecate this method?

If so, Evan is correct. As stated in the Java conventions, we should use @deprecated and add a @deprecated javadoc comment that provides more context / reason for the deprecatation as well as an alternative, if one exists.
https://docs.oracle.com/javase/1.5.0/docs/guide/javadoc/deprecation/deprecation.html#how

/** * @deprecated as of 0.9.x because it was not good; use {@link #theNotDeprecatedMethod()} instead. */ @Deprecated getDisplayStatus() { ... }

Here's a link to a stackoverflow with some examples.
https://stackoverflow.com/questions/9031832/how-to-declare-or-mark-a-java-method-as-deprecated

jmiranda · 2024-04-26T04:57:36Z

grails-app/views/returns/show.gsp

@@ -130,7 +130,7 @@
 <g:message code="stockMovement.status.label"/>
 </td>
 <td class="value">
- <format:metadata obj="${stockMovement?.displayStatus ?: (stockMovement?.shipment?.status?.code ?: stockMovement?.statusCode)}"/>
+ ${stockMovement?.displayStatus?.label}


You can leave the

<format:metadata obj="${stockMovement?.displayStatus}"/>

We just need to make sure the object returned by the method has an appropriate handler in the metadata tag. In this case, I assume we should be returning an Enum of some kind, which means we'll probably be trying to convert that from an Enum (let's call it status) to a string that can be resolved by the message provider.

enum.<status.class>.<status.value>

which is what I think the metadata tag will do by default.

This is basically the same thing becasue in the statusResolver class I created a method that would return a Map of variables that we require for display status

static Map getStatusMetaData(Enum status) { return [ name: status?.name(), label: applicationTagLib.message(code: 'enum.' + status?.getClass()?.getSimpleName() + '.' + status), variant: status.hasProperty('variant') ? status?.variant?.name : null, ] }

which essentially does the exact thing as format:metadata with translating Enums.
I would leave this as is, as the current version because otherwise I would need to return Enum in cases for OutboundStockMovement and this "metadata Map" for StockMovement and OutboundStockMovementListItem which will be a little inconsistent for displayStatus() methods on all of the mentioned classes

jmiranda · 2024-04-26T05:04:02Z

src/main/groovy/util/StockMovementContext.groovy

@@ -0,0 +1,58 @@
+package util


We should move this to the same package as StockMovement.groovy.

jmiranda · 2024-04-26T05:09:52Z

src/main/groovy/util/StockMovementContext.groovy

+import org.pih.warehouse.requisition.RequisitionSourceType
+import org.pih.warehouse.shipping.Shipment
+
+class StockMovementContext {


Let's call this class StockMovementStatusContext since it's really the context we are using in order to resolve a status, not a stock movement.

jmiranda · 2024-04-26T05:13:05Z

src/main/groovy/util/StockMovementStatusHelper.groovy

@@ -12,44 +11,7 @@ import org.pih.warehouse.shipping.ShipmentStatusCode

 class StockMovementStatusHelper {


Let's rename this to be StockMovementStatusResolver.

jmiranda

This is a good start. However, I need another day to review and think about this.

kchelstowski · 2024-04-26T06:57:12Z

src/main/groovy/util/StockMovementStatusHelper.groovy

+ return context?.requisition?.status
+ }
+
+ return context?.shipment?.status?.code ?: context?.requisition?.status


I assume we do not need the defaultStatus anymore, as for any context we would want to first look for shipment status and have a fallback to requisition status?

kchelstowski · 2024-04-26T07:02:41Z

grails-app/domain/org/pih/warehouse/inventory/OutboundStockMovement.groovy

+ requisition: requisition,
+ shipment: shipment,
+ origin: origin,
+ destination: destination


I'm wondering if it was safe to remove the defaultStatus fallback. Wouldn't it be safer to have the fallback to current status property of the OutboundStockMovement, as it would already have the context determined and an expected status, instead of "hard-coding" the default status as shipment.status ?: requisition.status
Unless we intentionally want to remove it and catch all the regression easier instead of having "a safe fallback"?

Correct me if I am wrong but for all 3 cases

outboundStockMovement

StockMovement

OutboundStockMovementListItem

defaultStatus is a requisition status that is returned as a fallback in the StockMovementStatusResolver getStatus methods

yeah, you currently implemented it like this, but with the defaultStatus version, I did a fallback to status which was something else depending on the DTO you've mentioned.
So for the OutboundStockMovement status is not really the requisition status (at least not always), but it is determined in the stock-movement.sql view - as for regular SMs it is apparently the requisition status, but for returns it is mapped to something else.
We should be safe with your version anyway, I just wanted to explain the purpose of the defaultStatus - so its purpose was to (if something goes wrong) fallback to the status, that was returned before out refactor.

kchelstowski · 2024-04-26T07:03:18Z

grails-app/domain/org/pih/warehouse/inventory/OutboundStockMovement.groovy

@@ -229,6 +230,19 @@ class OutboundStockMovement implements Serializable, Validateable {
 return shipment?.currentStatus == ShipmentStatusCode.RECEIVED
 }

+ @Deprecated


I'm also wondering why we actually want to deprecate the method we've just created? Did I miss something?

kchelstowski · 2024-04-26T07:04:58Z

src/main/groovy/util/StockMovementContext.groovy

+ }
+
+ Boolean isElectronicType() {
+ return requisition?.sourceType == RequisitionSourceType.ELECTRONIC


I think it would be better to add this method to requisition in the same way it is implemented for OutboundStockMovement/OutboundStockMovementListItem (initially I wanted to say, that you have this method implemented in the Requisition domain, but apparently I can't find it there.

kchelstowski · 2024-04-26T07:08:46Z

src/main/groovy/util/StockMovementStatusHelper.groovy

+ static Map getStatusMetaData(Enum status) {
+ return [
+ name: status?.name(),
+ label: applicationTagLib.message(code: 'enum.' + status?.getClass()?.getSimpleName() + '.' + status),


shouldn't we add a default message in case something does not have the translated label?

kchelstowski · 2024-04-26T07:10:24Z

src/main/groovy/org/pih/warehouse/api/StockMovement.groovy

@@ -88,7 +90,8 @@ class StockMovement implements Validateable{

 static transients = [
 "electronicType",
- "pendingApproval"
+ "pendingApproval",
+ "displayStatus"


there is nothing wrong in that, but just a note: we don't need to add transients in Grails 3 anymore. Not having a setter doesn't break the build anymore.
source: https://docs.grails.org/latest/ref/Domain%20Classes/transients.html

As of Grails 2.0 if there is only a getter or only a setter method, you don’t need to declare the property name of the method in the transients list.

kchelstowski · 2024-04-26T07:12:06Z

src/js/components/stock-movement/outbound/StockMovementOutboundTable.jsx

@@ -337,7 +337,7 @@ const StockMovementOutboundTable = ({
 width: 150,
 Cell: row => (<DateCell {...row} />),
 },
- ], [requisitionStatuses]);
+ ], [requisitionStatuses, translate]);


does it really rely on translate, when statuses are translated on the backend?

I noticed that there was a problem with displaying status tooltips which are not translated on the backend and use the translate method on the frontend

drodzewicz · 2024-04-26T09:38:06Z

src/test/groovy/unit/org/pih/warehouse/core/StockMovementStatusSpec.groovy

@EWaterman A few questions that came up for tests
In this file I am testing StockMovementStatusHelper class, is there a pattern I must follow in naming the Spec files, like instead of StockMovementStatusSpec it needs to be the name of the class that is being tested StockMovementStatusHelperSpec ?

what is the convention on test names/method names in spock?
I followed the one I used for jest which follows the pattern of should return (expected value) when (condition)

Since I am testing a utils function that is located in a utils package, should the test mirror the structure of the given class and be located in test/groovy/unit/org/warehouse/utils

Is there a way to group tests and is there a need for that? In jest you can create a describe block that kind of visually groups these test together, for example my current tests I am testing two methods, getStatus and getListStatus and I wanted to group the tests by the method that I am testing, but I am not sure if it is necessary or is it just a convention that I am used to from jest 😅

First off, I want to say your tests are super clean, so real nice job!

I usually break up my unit tests so that I'm only testing one class per test spec/class and so I usually name it classname + Spec to be super clear about what I'm testing (so StockMovementStatusResolverSpec).

For the same reason I try to mirror the package structure of the code (so yeah the structure you proposed). I will say I'm not a huge fan of having a generic "util" package for all of our utils because that can result in packages that have tons of files and becomes really hard to find anything. I'd rather structure our code to be feature based so would be something like:

feature X -> controller -> service -> util -> ...

that way everything is grouped by logical area/feature and we can drive a more clena division of responsibliity between our classes, but that's not a discussion we need to have here. For now yeah lets just do it as you proposed.

I don't think there's a universal standard for test names but I agree with the one you proposed.

I don't know if there's a way to group tests. I've seen it done on other projects with JUnit I think but not with Spock. It might be possible but I'm not sure.

I think all this hilights our need to have a standards document for our project. Something like this: https://wiki.onap.org/pages/viewpage.action?pageId=92998161

I tried to start one here https://pihemr.atlassian.net/wiki/spaces/OB/pages/3013869579/Backend+Unit+Testing+Standards but ultimately I'd like for it to be a decision that we make together as a team

…ng statuses for all cases

…vementDisplayStatusHelper

…for sm statuses

awalkowiak

I have one minor question in the StockMovementStatusContext and I think you forgot about Evan's request around deprecated annotations. Other than that I think it looks good to me.

awalkowiak · 2024-05-06T16:01:26Z

src/main/groovy/org/pih/warehouse/api/StockMovementStatusContext.groovy

+
+ Location destination
+
+ Shipment getShipment() {


Are these three getters required (getShipment, getOrder, getRequisition)?

I created these 3 getters specifically for tests because otherwise I had some trouble stubbing it.
Do you @EWaterman maybe know if there is any other way for creating a nested stub?

You might be able to do something like this:

StockMovementStatusContext context = Stub(StockMovementStatusContext) { isReturn() >> false isInbound() >> true ... } context.shipment = Stub(Shipment) { getStatus() >> new ShipmentStatus(code: ShipmentStatusCode.SHIPPED) } context.requisition = new Requisition(status: status)

tried it and for some reason it doesn't work, context.shipment returns null when inspecting it in a debugger 😞

Ok, we can leave it as is then

drodzewicz · 2024-05-06T17:13:02Z

I have one minor question in the StockMovementStatusContext and I think you forgot about Evan's request around deprecated annotations. Other than that I think it looks good to me.

@awalkowiak I am not exactly sure what information I should include in the deprecated comments because we don't have an alternative method or property to use instead of displayStatus
I continued the thread about it over here #4589 (comment)

drodzewicz requested review from jmiranda, EWaterman, awalkowiak, alannadolny and kchelstowski April 22, 2024 13:15

drodzewicz self-assigned this Apr 22, 2024

drodzewicz force-pushed the OBPIH-6213-spike-align-statuses-for-stock-movements-between-list-page-view-page branch from 17b5283 to 70b64ae Compare April 24, 2024 13:08

kchelstowski reviewed Apr 24, 2024

View reviewed changes

drodzewicz marked this pull request as ready for review April 25, 2024 16:39

EWaterman reviewed Apr 25, 2024

View reviewed changes

jmiranda reviewed Apr 26, 2024

View reviewed changes

kchelstowski reviewed Apr 26, 2024

View reviewed changes

drodzewicz commented Apr 26, 2024

View reviewed changes

drodzewicz force-pushed the OBPIH-6213-spike-align-statuses-for-stock-movements-between-list-page-view-page branch from 86d9ace to ec81dcb Compare April 26, 2024 16:14

drodzewicz requested review from kchelstowski, EWaterman and jmiranda April 29, 2024 13:30

drodzewicz added 7 commits May 6, 2024 14:37

OBPIH-6213 Create a StockMovementStatus helper which manages displayi…

7909caa

…ng statuses for all cases

OBPIH-6213 Refactor StockMovementStatusHelper

e4875ad

OBPIH-6213 Moved logic from StockMovementDisplayStatusUtil to StockMo…

0225f65

…vementDisplayStatusHelper

OBPIH-6213 Added unit test for StockMovementStatusHelper

cdcfb62

OBPIH-6213 rename spec

36a0130

OBPIH-6213 Refactor test with spcok stubs and rename Utility classes …

2fd363d

…for sm statuses

OBPIH-6213 remove transient displayStatus from StockMovement

f2979c8

OBPIH-6213 Fix tests after returns status fix

c37ee95

drodzewicz force-pushed the OBPIH-6213-spike-align-statuses-for-stock-movements-between-list-page-view-page branch from a432f70 to c37ee95 Compare May 6, 2024 12:37

awalkowiak reviewed May 6, 2024

View reviewed changes

OBPIH-6213 Rename StockMovementStatusResolver spec file

6f5e97c

awalkowiak merged commit 4231592 into feature/upgrade-to-grails-3.3.10 May 7, 2024
3 checks passed

awalkowiak deleted the OBPIH-6213-spike-align-statuses-for-stock-movements-between-list-page-view-page branch May 7, 2024 10:35

		@@ -12,44 +11,7 @@ import org.pih.warehouse.shipping.ShipmentStatusCode

		class StockMovementStatusHelper {

OBPIH-6213 Create a StockMovementStatus helper which manages displaying statuses for all cases #4589

OBPIH-6213 Create a StockMovementStatus helper which manages displaying statuses for all cases #4589

Conversation

drodzewicz commented Apr 22, 2024 • edited by jira bot Loading

kchelstowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EWaterman commented Apr 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmiranda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drodzewicz May 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EWaterman May 6, 2024 • edited Loading

Choose a reason for hiding this comment

awalkowiak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drodzewicz commented May 6, 2024 • edited Loading

drodzewicz commented Apr 22, 2024 •

edited by jira bot

Loading

drodzewicz May 6, 2024 •

edited

Loading

EWaterman May 6, 2024 •

edited

Loading

drodzewicz commented May 6, 2024 •

edited

Loading