I know it was done like that before, so I am not requesting change, but maybe we should create a ticket for improving this search? For me, it's completely broken, what if:

• someone enter more than 2 words?
• someone enter capitalized words?

I agree it could be much better, and I was also surprised how it is currently handled, but I've just copied the logic from current imports.

Yeah the person finder logic is naive at best and doesn't handle most of the complex use cases we encounter.

There have been tickets that discuss the limitations of the current solution and start to discuss the complexity of a proper solution (most recently this https://pihemr.atlassian.net/browse/OBPIH-5972).

But similar to other search use cases, in the long-term we shouldn't be handle search ourselves if we can help it. This would be better handled by a search index, like Elasticsearch.

For the import case, I would love it if we could require an email or username to avoid the issues with the name search. We should discuss that with Manon and Kelsey. Otherwise this method needs to fail-fast if unique values aren't found (we should throw an error if the given input returns 0 or more than 1 results are found).

In general, we should also avoid duplicating logic if it exists elsewhere. Moving this to the Person domain makes sense (see code in PersonService as well). But if we're moving logic to Person we should have a plan for migrating the other code as well.

do we have a final decision to create migrations in XML instead of Groovy? I forgot how this discussion went.

I assumed we would eventually come back to xml again after the discussion we had had, so I went with the xml.

Yes, XML.

Does this return work? 😄

yeah, since we don't have any additional statements below (except isNull which we would not want to be executed if bin location is present), this works fine.

I think I understand what you're doing, but I don't know why. It's confusing, not documented, and could get us in trouble if someone decides to remove it because it looks superfluous.

Change it to be more readable:

if (binLocation) {
    eq("binLocation", binLocation)
} else {
    isNull("binLocation")
}

We can also move the g.message to the new line.

Is the indentation correct?

I followed the pattern from already existing importer classes

wouldn't it be better to move it to something like EmptyStringsToNullBinder.groovy? I would like to avoid adding a lot of code between properties, seeing this I am expecting to see methods under this, not the rest of the properties. But it can be just my opinion.

yeah, this could be moved to a method, for now I've just focused on the functionality, but when I will do the final cleanup, I will take this suggestion into account.

is there a benefit from assigning it to the variable? The name of the property already suggests that it is a current location.

probably there isn't, it just felt better for me to pass a variable to the method below instead of passing the AuthService.currentLocation, but I could agree that for this particular case it probably doesn't make sense to create the variable

You should look for a bin location name under a specific parent. I think this might be the main reason why you got the performance issues. Bin location name is unique under a specific parent but can be duplicated globally, in this version you can't be sure that you found the bin location that you wanted to find.

Well maybe it is not the main reason of performance issues (it is), but definitely looking by name is not enough in terms what you are trying to find here.

@awalkowiak yeah, something smelled to me as well. I mean - the performance issues are surely due to the fact that we have not had any index for the name column (because we search for only one location, and searching via id, that has index works fast), but if I understand you correctly, I should look for a location under origin's locations, so:
origin.getInternalLocation(source['binLocation'])?

Yes this one would be good

have just realized after @awalkowiak 's comment about binLocation, that I should probably check for the quantity atp for the origin, not the current location?

in 99% scenarios, the current location would be origin as it is filled in by default, but I assume we want to allow changing the origin by a user?

yeah, I've just checked how it works for the regular outbound (we allow to change the origin), so I'll leave that note for me, not to forget to change this line to the origin.

Yes if possible please rely on the origin and destination directly

If you look for bin by parent and name, then this should not be required

Why do you say that @awalkowiak?

If we're doing any queries with the name column and comparison or logical operators (=, like, ilike) and not using wrapped regex matching (name ilike "%term%"), then an index is probably a good idea.

Now that I've read through the entire review I'm understanding this a bit better. And while Artur is right that it probably isn't necessary for this PR, we should have an index on location name. However, we need to look into how we're using the name column when querying locations.

For example, if we always query name and another column together then we'd want to create a composite index. Even if you do some queries with both columns and some queries with just one column, MySQL should use the composite index. So it's better to have the composite index then two separate indexes.

So while there's definitely a need to create an index on location name, we might want to investigate this as a separate issue as long as the performance issue (alluded to below) is no longer an issue.

Yes, XML.

Why do you say that @awalkowiak?

If we're doing any queries with the name column and comparison or logical operators (=, like, ilike) and not using wrapped regex matching (name ilike "%term%"), then an index is probably a good idea.

TL;DR: I've seen this a bunch of times and want to correct it. So in case it's not clear, this must be changed before merging.

While it's probably not documented, there is a convention established (previous database migrations) that I would like to follow.

If you include the indexName in the changeset, then the name of the file should be:

<include file="0.9.x/changelog-2024-08-02-1200-add-index-location-name-idx.xml" />

If you don't include the indexName (assuming it can be optional) then it would be:

<include file="0.9.x/changelog-2024-08-02-1200-add-index-location-name.xml" />

Other examples

add-column-<tableName>-<columnName>.xml
add-index-<indexName>.xml
add-index-<tableName>-<columnName>.xml
create-table-<tableName>.xml

For example, two changesets above this one should have been

<include file="0.9.x/changelog-2024-05-08-1110-rename-column-picker-id.groovy" />
<include file="0.9.x/changelog-2024-06-27-1200-add-column-requisition-item-auto-allocated.groovy" />

1. Change the order of the method arguments. The method above this one shows the right order for the method arguments.
2. Shorten these methods names to be a little more concise.

getQuantityAvailableToPromise(Location location, Location binLocation, InventoryItem inventoryItem)

TL;DR: This needs to be dealt with in this PR.

While it might not seem possible for there to be multiple rows per location/binLocation/inventoryItem, we can't make that assumption unless it's actually true and unless the database prevents multiple rows from being added to the table.

The following query was executed on production. I haven't looked into whether this is valid or invalid. But the fact that the query returns two separate instance with two records indicates that the table allows multiple rows per location, bin location, inventory item.

MariaDB [openboxes]> select location_id, bin_location_id, inventory_item_id, count(*) from product_availability group by location_id, bin_location_id, inventory_item_id having count(*) > 1\G
*************************** 1. row ***************************
      location_id: 8a8a9e9665c4f59d0165c5516779002f
  bin_location_id: NULL
inventory_item_id: ff8080818662c5070187049481ef7a3d
         count(*): 2
*************************** 2. row ***************************
      location_id: 8a8a9e9665c4f59d0165c558178f0041
  bin_location_id: 8a8a9e9669680d660169725db72521b2
inventory_item_id: 8a8a9e9669680d660169725d888c2163
         count(*): 2
2 rows in set (0.990 sec)

The code needs to handle this case, so please investigate why there could be two records and determine whether it's:

• a valid case
  • we probably just need to sum the QATP across the records
• an invalid case
  • we need to workaround the issue to reduce the duplicate records to one record
  • add a ticket to discover why it's happening and prevent it

@jmiranda this is the issue I've once fixed: https://pihemr.atlassian.net/browse/OBPIH-6008
I've run your query on my local db, and then checked all the columns how they look like.
Here is an example:

id                                  |version|bin_location_id                 |inventory_item_id               |location_id                     |product_id|product_code|lot_number  |bin_location_name|quantity_allocated|quantity_on_hand|date_created       |last_updated       |quantity_on_hold|quantity_available_to_promise|
------------------------------------+-------+--------------------------------+--------------------------------+--------------------------------+----------+------------+------------+-----------------+------------------+----------------+-------------------+-------------------+----------------+-----------------------------+
0ac391ad-4d6d-4a27-bc01-77139d1fd332|      0|09f5e116831aefb00183377b1e5d58c4|09f5e1167c3c9801017c4ba7755715fe|09f5e1167e6ee8ec017e9c3c480f6f43|10001     |21064       |MWM062620N47|R-307CFC         |                 0|               0|2023-08-30 18:09:01|2023-08-30 18:09:01|               0|                            0|
f47db6bf-6ee9-4d73-8a3a-f7c58e39f5dc|     22|09f5e116831aefb00183377b1e5d58c4|09f5e1167c3c9801017c4ba7755715fe|09f5e1167e6ee8ec017e9c3c480f6f43|10001     |10001       |MWM062620N47|R-307CFC         |                 0|               0|2023-09-18 21:01:21|2023-10-25 21:18:03|               0|                            0|

so what needs to be done on production is to refresh PA on the locations where those rows exist

@kchelstowski Although you've fixed the data issue, the problem is that this scenario can still happen. So we need to understand how it happens and be defensive about how to handle it.

@awalkowiak Let me know if you have any thoughts. It might not be fixable but we shouldn't ignore it if it's a possibility.

Let's call the property "productCode" in order to ensure there's no confusion.

The other fields (like binLocation) are fine, because there's ambiguity to what data will be in the Bin Location column (i.e. id, code, name of bin location).

But there's no ambiquity in the Product Code column.

I called it product, so that I would not have to do any mapping on the frontend side in order to bind the product in the ImportPackingListItem.
I will change that, and will just adjust the frontend, so that for the validation endpoint we would send this property as:

product: record.productCode

name this overpick since it's an actual word and not two words "over pick"). This is similar to "putaway". It's putaway, not "put away".

Sources:

• https://www.merriam-webster.com/dictionary/overpick
• https://docs.oracle.com/en/cloud/saas/supply-chain-and-manufacturing/24b/famml/overpick-for-sales-orders-transfer-orders-and-work-orders.html#s20077043

I understand the desire to have this here in teh constraints. But this seems dangerous. I think I'd prefer if the validateData was doing this validation. One benefit would be that we'd inject the productAvailabilityService into the Importer once vs once each time domain.validate() is called (likely more than once per domain).

With that said, there an argument for allowing the domain to call the service.
https://stackoverflow.com/a/3654488/136597
https://stackoverflow.com/a/2460029/136597

But perhaps we could inject the service into the domain once
https://stackoverflow.com/a/9699609/136597

We decided not to perform the validation at import file level, hence we can't do it in the validateData method, as it would require us to refactor the whole thing now.
The productAvailabilityService is not injected per each item, since for each item we have the same memory reference to the service, so for each item it uses "the same" instance of product availability service.

let's call this one
stockout or unavailable.

ANd make sure that !quantity returns true for both 0 and < 0.

good point about the quantity < 0. I thought it's not possible to have negative quantity in the product availability table.

Now that I've read through the entire review I'm understanding this a bit better. And while Artur is right that it probably isn't necessary for this PR, we should have an index on location name. However, we need to look into how we're using the name column when querying locations.

For example, if we always query name and another column together then we'd want to create a composite index. Even if you do some queries with both columns and some queries with just one column, MySQL should use the composite index. So it's better to have the composite index then two separate indexes.

So while there's definitely a need to create an index on location name, we might want to investigate this as a separate issue as long as the performance issue (alluded to below) is no longer an issue.

@awalkowiak don't look at this file at all. It's just a 🍝 on my testing purposes. I will make it cleaner etc in the frontend ticket.
I decided not to remove it though, to make the feature testable for Kasia and Manon

I like the thought process here. Maybe someday we can find a way to apply a more generalized version across the app so that we can have a common format for our error responses.

@ewaterman we actually have a generalized version - we have errors handler (ErrorsController). I'd say this is just a new case, where instead of throwing a popup error, we want to associate an error with a particular row in the table.

Can we move the build errors into CommandUtils as well?

Is the +1 because value.after doesn't handle them being equal? What happens if delivery date and shipment date are the same?

@ewaterman yeah, a similar addition is implemented in the Shipment domain.

why is productCode being accessed differently?

good question - previously I was probably accessing some deeper associations (or copied it from somewhere) and didn't realize I've mixed the type of access. I will unify it.

Because "invalid" is very general but the error is a very specific error scenario. This way leaves room for us to add further error messages on the origin/destination in the future

I feel like a non-developer would understand "empty" more than "null". Same comment for all these.

I'm not a big fan of having to pull a service in like this to do validation. IMO request objects should be simple and not depend on any other large classes.

My preferred approach to handling situations like this is to add an ImportPackingListValidator that can autowire in whatever classes it needs.

• https://docs.spring.io/spring-framework/reference/core/validation/validator.html
• https://stackoverflow.com/questions/41996143/spring-service-method-and-a-complex-validation-logic-rules

That way request objects can act more like simple POJOs. But maybe there's a better way to achieve this via Grails that I don't know about.

I'm okay with doing it this way for now, but I would like to brainstorm some potential longterm solutions

@ewaterman I didn't want to introduce a validator class or proceed with the validation in the service for this one property, not to mix two approaches - validator, and a custom validation in a validator class.
I know injecting a service into a domain is debatable, but since Grails' recommended validation happens in the domain (validator), I've gone this way.
As I mentioned somewhere above - this doesn't "recreate" the product availability service, or anything like this, because the PA service is singleton, and we always get the same reference no matter which item we are iterating through.

IMO you probably don't need a lot of these comments since they're just reiterating what the method names are (which means you have good method names). My general approach is to have self-documenting code (ie writing code in a way that makes the functionality obvious by just reading the code) and then add comments if it's still unclear.

Not a big deal though and better to be overly verbose than have very unclear code.