5secs for safety, should be moved to config instead of hardcoding

Why can't we reuse the refresh solution we're using in another PR so we don't need to add the delay?

hmm, what do you mean here?

ok, it's probably about this - https://github.com/openboxes/openboxes/pull/5918/changes#diff-bd0b05d4be7c4ca82d41265b88d24030cd920fd750075e545885a5191c2a75c0R19

I pushed a new commit removing job delay

@druchniewicz Yes, that's what i had in mind.

corrected

Move this method to the InventoryLevel domain and register a marshaller through Bootstrap.groovy.

done

Good catch

allErrors might contain FieldErrors and ObjectErrors, so we need to be careful here.

You have a few options

1. Assume allErrors is a collection of ObjectError since FieldError extends ObjectError
2. Use it instanceof FieldError ? it.field : null to guard against a MissingPropertyException
3. Use inventoryLevel.errors.fieldErrors.collect { FieldError error -> ... but that might drop errors we want to display, so maybe not a good idea.

done

Why aren't we doing a similar thing for product that we do with inventory level.

I added bindProductData(product, json) and bindInventoryLevelData(inventoryLevel, json) methods because we moved logic from controllers to the services. Apart from that we have custom logic for binding e.g. want to pass productId or productCode in the payload or e.g. for preferredBinLocation we want to pass id or location number so we have to find correct values first. Default grails binding works in controllers and for objects it binds always by "id".

Can you elaborate on this?

This needs to be avoid sometimes two open sessions. We have one transaction on ProductService and in upsert logic we have Product.withNewTransaction { ... } what opens a new session. During saving two sessions sometimes touch the same entity sub-resources and exception is thrown: org.pih.warehouse.product.ProductService: upsert: illegally attempted to associate a proxy with two open Sessions; nested exception is org.hibernate.HibernateException: illegally attempted to associate a proxy with two open Sessions

Ok, this probably needs a bit more discovery to find the best solution. I understand the two session issue, but assume that we're also doing this because a method in a service calling another method in the same service will bypass the Spring transaction proxy.

But we can deal with that later. Perhaps a separate BulkUpsertService dedicated to the wrapper bulkUpsert but leave the upsert in the domain-specific service (e.g. ProductService) where it belongs. However, that's side-stepping the problem as well.

The identifier is currently not unique (yet) so this could hypothentically return multiple records. I'm not exactly sure how we want to handle that but just wanted to call it out.

We could actually add a unique constraint. I don't think there's a problem with that especially given the fact that you can have multiple records with a null identifier without breaking the unique constraint.

done

It feels like this should just return the list of objects saved or validated with errors. Or a DTO that wraps these and can be rendered to json.

done

Nitpick: let's call this variable json or jsonObject. body is too generic.

done

I don't love the return being a list of maps with an arbitrary shape and then the magic strings all over ("ok", "error")

Can we either return the saved and/or validated (with errors) instances and then render those. Or better yet, create an UpsertResult DTO

enum UpsertStatus { OK, ERROR }
enum UpsertAction { CREATED, UPDATED }

class UpsertResult {
    int index
    UpsertStatus status
    UpsertAction action    // note: this will be null on error
    String entityId              // productId / inventoryLevelId, etc
    String errorMessage
    List<Map> errors
}

and then the methods and return types will look like this

UpsertResult upsert(Map json, boolean deferRefresh)        // upsert one row

List<UpsertResult> bulkUpsert(List<Map> jsonArray, boolean deferRefresh) {
    items.withIndex().collect { Map json, int i -> upsert(json, deferRefresh).withIndex(i) }
}

and we can add a marshaller for the UpsertResult like we do with other objects (add a toJson method and register it in BootStrap.groovy)

done

done

done

We need to keep an eye on the performance of this when the batch size is in the 100s or 1000s. I might also recommend making this asynchronous.

I think it should not be a problem when split it to batches. We'll see

As I mentioned above, I'm realizing this is just doing an update on these tables if the product code is changing. Therefore it's not actually trigger a recompute of product availability. I think the same two methods could be used above when upserting the inventory levels.

We need to make sure this gets tested on existing databases as it could fail.

right, I removed inventory levels with duplicated identifiers

@druchniewicz, cleaning it in one of the dbs does not handle the issue, since others might still have duplicates in their dbs. For example, I have, so this still would fail for me after merging. Here is an example how we handle it in core for a similar issue: https://github.com/openboxes/openboxes/pull/5867/changes#diff-c2f5fe6aed319d3887faa5de363feb7794f117d2056575338907cb3b31dce95aR6-R37

Can we add some logging to make sure this gets fired when and how we're expecting.

done

Note: This is where I think the value converter would come in handy. Makes it so we don't need the findByIdOrProductCode above. Nothing we need to do now, but we might want to come back to it later.

This block should eventually be converted to just publishing an event. And then we can move the refresh code to an event service.

I'm just realizing that this isn't what I thought it was. We actually have no reason to recompute the product availability here. The only thing that might be happeing is an update to the product code that needs to be reflected in the product availability and inventory snapshot table. So this is not a big deal at all, could be event'd and async'd but does not need to be.

cc @awalkowiak We probably need to make a distinction between updating and refreshing / recomputing product availability. And we actually probably could try to narrow the "event" to only care about the products that have changed their product code. Not sure how we'd do that (some type of hibernate interceptor, i assume) so not worth the effort at the moment.

Lastly, this does not even need to trigger the refresh, it could just do the updates inline but it's not necessary to make that change as long as this works fine. We should try to test this out by changing a few product codes via bulk upsert (assumes we include the product IDs in the upsert JSON).

As I mentioned above, I'm realizing this is just doing an update on these tables if the product code is changing. Therefore it's not actually trigger a recompute of product availability. I think the same two methods could be used above when upserting the inventory levels.

I might be wrong about letting the PUT /api/products be used for resource and resource collections. The same endpoint responds with different response shapes depending on the input and that seems like it might eventually be confusing. We're going to leave it for now, but @druchniewicz let me know if you encounter any developer experience (DX) issues related to that.

@jmiranda yeah indeed it may be a bit confusing, but we need to somehow connect it with existing mappings to not break anything.
In my ideal world if there was no API yet and I was writing it from scratch, I would do it like this:

API for Products:

1. Get list of products -> GET /api/products (no body)
2. Get single product -> GET /api/products/{idOrCode} (no body)
3. Create single product -> POST /api/products (single product object)
4. Upsert/update single product -> PUT /api/products/{id} (single product object)
5. Bulk upsert -> POST /api/products/bulk (array of objects)
6. Delete product -> DELETE /api/products/{id}

API for Inventory Levels:

1. Get list of inventory levels -> GET /api/facilities/{facilityId}/inventory-levels (no body)
2. Get single inventory level -> GET /api/facilities/{facilityId}/inventory-levels/{identifier} (no body)
3. Create single inventory level -> POST /api/facilities/{facilityId}/inventory-levels (single inv level object)
4. Upsert/update single inventory level - PUT /api/facilities/{facilityId}/inventory-levels/{identifier} (single inv level object)
5. Bulk upsert -> POST /api/facilities/{facilityId}/inventory-levels/bulk (array of objects)
6. Delete inventory level - DELETE /api/facilities/{facilityId}/inventory-levels/{identifier}

@druchniewicz, cleaning it in one of the dbs does not handle the issue, since others might still have duplicates in their dbs. For example, I have, so this still would fail for me after merging. Here is an example how we handle it in core for a similar issue: https://github.com/openboxes/openboxes/pull/5867/changes#diff-c2f5fe6aed319d3887faa5de363feb7794f117d2056575338907cb3b31dce95aR6-R37