OBPIH-6491 do not cross autopicked lines when manually import picking

[drodzewicz]

To be able to decide which lines should be crossed out and which should not we need to hold some kind of a state for picklist items that were autopicked and which were manually added or imported so I decided to implement the enum we talked about for specifying pick type OBPIH-6495

If you want more context to this issue then please go to the ticket and look at some of the scenarios described there

OLD COMMENT (NOT RELEVANT ANYMORE)

All seemed to be clear, we have three endpoints: autopick, import and update and we can just assign PickType for each of them when they are called.

But there is one case which requires a little more attention and thought, which is update.
I am still trying to figure this out which is why the PR is Work In Progress.

The case is:

• User clears the pick -> edits pick manually -> user should not see crossed lines of pick is different than suggested items
• User gets items autopicked -> edits pick manually -> user should see crossed out lines

My theory initially was to set PickType.MANUAL if we are not updating, as for existing picks that we are updating I would not change the pickType but the problem is that I can't check previous pick type because we are clearing the whole picklist before update (looking at git logs it seems it was done intentionally to solve so qty atp calculation issue)

I will come back to this issue on Monday and maybe will have some better ideas in my head.

Important

SOME QUESTIONS THAT NEED TO BE ANSWERED

• Treating old picklist (before back-data feature) as auto-picked
  Since I introduce an enum state PickType for picklist items to identify which picklists Items were picked with Import or Autopicked by the system there are also cases in the database before this feature and I was wondering how to deal with them.
  As You can see my implementation assumes that if picklistItem has PickType.AUTO or doesn't have a pickType that means the pickListItems was picked automatically.
  @kchelstowski and @alannadolny have suggested to write a migration and populate existing picklistItems with PickType.AUTO because in the future it will give us an idea that something went wrong when picklistItem doesn't have a pickType and not because it is autpicked.

• property location in which domain for PickType?
  As ticket OBPIH-6945 suggested I placed pickType inside PicklistItem Domain, but I am wondering if it is correct placement.
  PickType behavior should be similar to reasonCode, what I mean is that We Care about reason code and pick type in context of the whole picklist for the Items or requsitionItem and not individual picklistItems. I don't think there is a valid case where item XXX has 3 picklistItems that have different pickTypes so maybe pickType should be on requisitionItems instead.
  For context on reasonCodes we have this property both on requsitionItem and picklistitem which is weird unless I am missing something 🤔

[kchelstowski]

while I don't mind it, I would probably prefer to just have a migration for all existing pick list items, to have the AUTO.
Why I think it would be better is that null should be "reserved", so that if any of picklist items have null, we would know, that something must've gone wrong.
I don't which approach is preferred in this case, so while I don't mind your solution, I would like to know your and others opinion.

[alannadolny]

I agree with @kchelstowski, I also think we should go with migration.

[EWaterman]

If we do a migration we should have a plan for it. We'd want to do a select on the db to see how many production rows will be affected and also make sure that null == PickType.AUTO in 100% of cases. We don't want to break any existing data

[jmiranda]

I agree with the database migration approach. But we also might have scenarios that we do not know what is automatic vs manual. It's possible that everything can be derived using the reason code, comment and/or last updated. But it might be also be really difficult to determine.

null == PickType.AUTO in 100% of cases

This is almost certainly not true. See below.

[jmiranda]

Important

TL;DR: I think the conclusion I've come to is that we the only way to distinguish between an "original" (automatic) vs "edited" (manual) pick is to look at whether reason code is empty. This does not actually answer the question in an authoratative way because we don't store the "original" picklist items anywhere. And it seems they are re-computed when we are rendering the comparison in the UI.

I could very well be wrong about that last point, so I need someone to correct me. But if that assertion holds, then this is not ideal because the "original" picklist items can change over time due to the nature of the FEFO algorithm and stock being updated. Therefore, knowing whether something was an original (auto allocated) picklist item becomes very difficult.

My Analysis

One thing that hasn't been mentioned is that there's aleady a mix of auto vs manual picklist items in the system. As far as I recall not all picklist items were automatically allocated. Some were automatically allocated and some were edits to the automatically allocated picklist items.

Therefore, we need to find a way to distinguish between the two. That will probably require some discovery.

Note

I'm only looking at allocations based off requisitions. transfers and returns are being ignored for the moment.

mysql> select count(*) from (select picklist_id, requisition_item_id, count(*) from picklist_item group by picklist_id, requisition_item_id having count(*) > 1) as picklist_items;
+----------+
| count(*) |
+----------+
|    45174 |
+----------+
1 row in set (14.61 sec)

Warning

Holy crap, 15 seconds!!

I mentioned somewhere that the auto vs manual allocated might be derivable by a combination of properties:

• reason code
• comment
• version?

So let's take a look at an actual example

mysql> select * from picklist_item where requisition_item_id = 'ff8080818715c63401874d6fa3bb46e3' \G
*************************** 1. row ***************************
                 id: ff8080818715c634018752322a585612
           quantity: 8
        picklist_id: ff8080818715c63401874d7f3e88476c
  inventory_item_id: 09f5e1167dba69cc017dbefcc5041074
            version: 0
       date_created: 2023-04-05 16:13:52
       last_updated: 2023-04-05 16:13:52
      created_by_id: NULL
      updated_by_id: NULL
             status: NULL
        reason_code: PACKAGE_SIZE
            comment: NULL
requisition_item_id: ff8080818715c63401874d6fa3bb46e3
 picklist_items_idx: NULL
    bin_location_id: ff8080816c90f81a016cbe6e87a1774d
         sort_order: 4900
      order_item_id: NULL
    quantity_picked: NULL
        date_picked: NULL
       picked_by_id: NULL
*************************** 2. row ***************************
                 id: ff8080818715c634018752322bbe5613
           quantity: 10
        picklist_id: ff8080818715c63401874d7f3e88476c
  inventory_item_id: 8a8a9e9668ca65ad0168cb77ac040946
            version: 0
       date_created: 2023-04-05 16:13:52
       last_updated: 2023-04-05 16:13:52
      created_by_id: NULL
      updated_by_id: NULL
             status: NULL
        reason_code: PACKAGE_SIZE
            comment: NULL
requisition_item_id: ff8080818715c63401874d6fa3bb46e3
 picklist_items_idx: NULL
    bin_location_id: ff8080816c90f81a016cbe6e87a1774d
         sort_order: 4900
      order_item_id: NULL
    quantity_picked: NULL
        date_picked: NULL
       picked_by_id: NULL
*************************** 3. row ***************************
                 id: ff8080818715c634018752322d2a5614
           quantity: 30
        picklist_id: ff8080818715c63401874d7f3e88476c
  inventory_item_id: 8a8a9e9666194c890166266bbd940908
            version: 0
       date_created: 2023-04-05 16:13:53
       last_updated: 2023-04-05 16:13:53
      created_by_id: NULL
      updated_by_id: NULL
             status: NULL
        reason_code: PACKAGE_SIZE
            comment: NULL
requisition_item_id: ff8080818715c63401874d6fa3bb46e3
 picklist_items_idx: NULL
    bin_location_id: ff8080816c90f81a016cbe6e87a1774d
         sort_order: 4900
      order_item_id: NULL
    quantity_picked: NULL
        date_picked: NULL
       picked_by_id: NULL
3 rows in set (0.00 sec)

Oof. Not what I was hoping to see but I vaguely remember this decision.

Note

Edit: To be more clear about the previous statement ^^ I was talking about the fact that we don't keep track of the original picklist items (which I assumed), but we also copy the reason code to each of the picklist items. So even if there were "original" (auto allocated) items, there's no way to distinguish "original" from "edited". However, the reason code tells us that at least one of the picklist items was modified, so from a requisition item perspective we can answer the question "was the allocation for the requisition item auto or manual". But to answer the question for each picklist item, we would need to recompute the "suggested items" to know what was original vs edited.

Some observations

• There can be multiple automatically allocated picklist items for a given picklist / requisition item ID
• There can be multiple manually allocated (edited) picklist items for a given picklist / requisition item ID
• We are storing a reason code on each of the picks so it seems like we're deleting the original picklist item
• When the UI compares the "manual" picklist items vs the "original" picklist items, we actually need to re-compute the "original" picklist items.
• The "original" picklist items could change over time based on the FEFO algorithm.
• Having only one picklist item does not mean that the item has been auto-allocated (could be done through an import, at least that will be true in the future).
• Picklist items are either original (auto) or edited (manual), the only way to distinguish is to see if it has a reason code.

[kchelstowski]

this method is getting overloaded more and more, but let's say I don't look at it until we refactor this feature completely 🙈

[EWaterman]

yeah stock movement is definitely in need of a refactor but that's going to need to get done as a dedicated task because it's so massive... not looking forward to that

[jmiranda]

@EWaterman @awalkowiak Evergreen comment. This gets mentioned every few weeks, but it's such a big refactoring and we've had other big tech debt issues that have been priorities that we just keep kicking the can. But we really should schedule some time soon to do tech debt backlog grooming to understand the different refactorings involved and identify some discoveries that are worth taking on in one of the next release or two.

[EWaterman]

What happens if pickType is null? Do we return null down to the client and then the client knows what to default to in that situation?

If we do the migration that Kacper and Alan mentioned in a previous comment, null should only represent an error case so maybe it's not as big of a problem and we can just let the client handle it however it wants to

[jmiranda]

I think we need to make a determination about whether we can detect the allocation method (automatic, manual) by looking at the data. I gave my analysis above but need to hear from Dariusz on whether there's a way to do the detection (presumably without need to recompute the "suggested items").

In either case, we might not need to worry since this will likely become a boolean isAutoAllocated or isDefault based on the

You are correct that we probably don't need too many types for the method that we used to allocate picklistItems, just differentiating autopick from the manual pick should be enough
So with that in mind we might not need a whole enum to track this state unless we will have to expand these types in the future, which I doubt we will.
So looking at the problem again all we need is a boolean flag indicating that picklistItem was picked automatically something like isAutoPicked or isManualPick

Source: https://openboxes.slack.com/archives/C06JCHWR807/p1719411664343269

... in which case we'd just need to provide a sensible default value when we cannot derive the value from existing state (default value for isAutoAllocated should probably be true, but I'm not 💯 about that.

[jmiranda]

As we discussed in Slack

... I'm ok going with one of these solutions if we need to store the state on the picklist item.

• Boolean isAutoAllocated - there probably should be an allocation data model, but this flag would be fine until that happens
• Boolean isDefault - kind of generic, but seems ok

If these can be derived somehow, let's go with that. If not, then let's store state and figure out how to detect whether something was automatically allocated and have a database migration that sets all existing picklist items.

If we realize we may need more states than a boolean allows, I would be ok with an enum like this.

• enum AllocationMethod
  • AllocationMethod.AUTOMATIC
  • AllocationMethod.MANUAL

PickType has another meaning (case pick, piece pick, fulll pallet pick) so it should not be used to represent something related to allocation.

[jmiranda]

Note

Possible coding convention to document.

I always forgot the proper convention here, but I assume this allows us to get call this in two ways

picklistItem.isAutoAllocated()
picklistItem.autoAllocated

If that's the case, that's the convention I like and coincides with the javabean conventions.
https://stackoverflow.com/a/5322666/136597

Somewhere along the lines i feel like I decided we shoudl do it this way, but I don't recall why.

getIsAutoAllocated()

[jmiranda]

property location in which domain for PickType?
As ticket OBPIH-6945 suggested I placed pickType inside PicklistItem Domain, but I am wondering if it is correct placement.
PickType behavior should be similar to reasonCode, what I mean is that We Care about reason code and pick type in context of the whole picklist for the Items or requsitionItem and not individual picklistItems. I don't think there is a valid case where item XXX has 3 picklistItems that have different pickTypes so maybe pickType should be on requisitionItems instead.

This is a fantastic question. And for me, this gets at a fundamental aspect of API design (I'm referring to "API" in the most generic sense, not REST API, in particular). I mentioned this somewhere (Slack, PR, Confluence, or Jira) in the past day or two but can't seem to find the comment now. I said that we need to figure out who is responsible for answering a particular question like "was I auto or manually allocated?"

In this case, on the Edit Pick page, I assume we need to basically do two things

• Display and strikethrough the original picklist item (i.e. the suggested item) if there was an auto allocation
• Do not display the original picklist item (suggested item) if there was a manual allocation

But, from the perspective of that UI, who is responsible for answering that question? The answer to that tells us how to model this and where state probably needs to go. So you're correct, in this particular case, it probably doesn't make sense for picklist items to answer the question "was I auto vs manually allocated". It's the requisition item (or more appropriately, PickPageItem) that needs to answer that question. Right?

The next question would likely be:
Does PickPageItem (subsequently RequisitionItem) have enough information to answer that without having to add state?

I don't know the answer to that, but I suspect we could get close without a data model change. If there's not enough state and we do need a data model change, then the state probably needs to go into the picklist item. And we'll just need to manage that state ourselves when creating the picklist item. It seems like the obvious place since when we're creating a new picklist item, we'll know definitively whether the allocation comes from a manual edit from the UI, picklist import, or it was auto-generated by the exectution of the allocation algorithm.

For context on reasonCodes we have this property both on requsitionItem and picklistitem which is weird unless I am missing something 🤔

Reason code on requisition item relates to cancellations, modifications, and substitutions of the original requisition item (i.e. when we're revising quantities or doing subsitutions in the Edit step, before we get to allocation). We might also use this reason code to store the reason for pick changes as well (@awalkowiak?), but it was primarily used for these requisition item changes. The reason code on picklist item is intended to track state about why a pick was being edited.

---

Aside: We could have modeled picklist item in a way that was similar to requisition item, but I didn't want to add complexity unnecessarily. The main difference between requisition item and picklist item is the parent-child relationship that exists between the "original" requisition item and its modifications and substitutions. We keep the original requisition item around, even if the modification and substitution are superceding the "original". The same is not true for picklist items. If we make an edit to a picklist item, the original item is modified. If we choose a different pick altogether, the original item is deleted. We could go back and refactor that now, but I think I'd rather tackle those complexities within a data model that handles the allocation, rather than the picklist. That way the picklist and picklist items are just dumb copies of the final allocation (original + edits).

[drodzewicz]

So after gathering all of the comments and opinions I think that it will be best if we proceed with the following approach.

1. Add isAutoAllocated [bool] column to RequisitionItems Domain
2. Have PickPageItem return this value in the json response
3. Set default value for isAutoAllocated to true (??debatable??)

Let me explain now.

Add isAutoAllocated [bool] column to RequisitionItems Domain

@jmiranda point is correct, if possible we should use existing data to derive this value as a transient variable, the problem is there is nothing that can 100% guarantee that picklist item was autopicked or picked manually

• Good idea deriving from version but the problem is how we update our picklist items, or rather that we don't, we clear our existing picklist and create a new one so no version variable or dateCreated != dateUpdated will work.
• Using reasonCode is also not bullet proof since we don't have to provide a reason code when our edited picks satisfy the exact quantity we are expected to pick, so we don;t always have to provide a reason code when editing and of course we do not provide a reason code when importing.
• Just like you mentioned, individual picklist items doesn't care if it is allocated automatically or manually by a user, this inform should be stored a little higher. Basically, I don't care if an individual picklistIem is auto allocated I care that a collection of these picklisitItems under a requisitionItem is allocated automatically.

Have PickPageItem return this value in the json response

we will probably only need this functionality with crossed out lines on pick step so maybe including this variable in a requisitionItem response is unnecessary, which is why we probably only need to return in in a pickPageItem

Set default value for isAutoAllocated to true (??debatable??)

actually I am not sure, if it were up to me I would probably leave it as null, I don't think there is any smart way of going about it. I think we need to assume that from this point onward we are tracking our allocation method and when viewing older picklist we return null for isAutoAllocated ignore any new UI behiors like not having crossed out lines for manual picks or label to identify autpicked lines (OBPIH-6492)

[jmiranda]

This is much cleaner! Awesome job @drodzewicz

We should document the limitations of this approach in the ticket. The ones that come to mind are these:

• Requires recomputing / sync'ing autoAllocated whenever we update picklist items. Might be a good idea to move the logic to a Hibernate interceptor on the PicklistItem if this gets complicated or we see regressions. But I think it should be fine to handle this whenever we create/update/delete a picklist item.
• We still probably need a database migration and way to detect auto vs manual for all existing requisition items.
• An API client cannot make a determination of "auto" vs "manual" at the picklist item level.
• There are cases (overpicking, underpicked) where we might have a mix of automated and manually allocated picklist items. We won't be able to provide that level of detail to the API client with this approach.