I had to refactor this part, because otherwise packages:

1. productSupplier: null, uom: "XYZ", quantity: X
2. productSupplier: "ZYX", uom: "XYZ", quantity: X

did not pass the unique constraint, because when no productSupplier provided, for the validation query it was not searching for eventual productPackage with productSupplier as null, but skipped it while quering, so for the uniqness check, it was doing:

select from product_package where 
uom = ... 
and product = ... 
and quantity = ...

instead of:

select from product_package where uom = ... 
and product = ... 
and quantity = ... 
and product_supplier IS NULL

I don't like the name productPackage1, but nothing better comes to my mind, maybe other developers will have some ideas 🤔

it's been like this before, hence I assumed this would fit OK.

I had the same reaction, but I've probably done this as well. This is a case where the default it would probably make it less confusing, but that's not an official convention so meh.

In general, if there's a way to describe a variable in a more meaningful way, then do it. Here, since we're basically iterating through and trying to find a default product package, I would call it defaultProductPackage.

A better (more DRY, OOO) approach would be to add a convenience method to Product.

if (!productPackage && !orderItem?.productSupplier) {
                productPackage = orderItem.product.getDefaultProductPackage(orderItem.quantityPerUom, orderItem.quantityUom)

The actual method might look like this

// Does not handle if multiple product packages match (but this should happen due to unique constraint)
// Does not pass product since we make the call on orderItem.product
ProductPackage getDefaultProductPackage(Integer quantityPerUom, quantityUom) {
    return productPackages.find { ProductPackage productPackage ->
        return productPackage.uom == orderItem.quantityUom &&
            productPackage.quantity == orderItem.quantityPerUom &&
            !productPackage.productSupplier
    }
}

Keep this in mind. The more we can move to the domain, the easier it'll be to do unit testing.

@jmiranda would you like me to move it to a method now, or you just wanted to make a point for the future?
If the answer is, that I should move it, then I think I'd also have to pass the orderItem as well (beside quantityPerUom and uom), because if we want to have it in the Product domain, we would not have the access to the orderItem.

Yeah it was mostly a pattern to keep an eye out for in the future.

As far as I can tell we don't need orderItem there. Could you explain the need for that?

From the Product domain we don’t have an access to orderItem, hence if we wanted to make this method work we would have to pass the orderItem as an argument, which is currently passed to the updateProductPackage method where this if belongs to

Sorry, I'm missing something here. What do you need from the order item that we're not already passing into the new method?

Oops. Sorry, I just realized I had a typo in my previous version of getDefaultProductPackage. I've now removed the "orderItem" from within the find closure.

The new method on Product.groovy doesn't need the orderItem, it would take the quantity and uom as arguments.

ProductPackage getDefaultProductPackage(Integer quantityPerUom, UnitOfMeasure quantityUom) { 
    return productPackages.find { ProductPackage productPackage -> 
        return productPackage.uom == quantityUom && 
            productPackage.quantity == quantityPerUom && 
            !productPackage.productSupplier } 
}

And then the caller in updateProductPackage would just invoke the default product package method using the properties from orderItem.

    // If not found, then we look for a product package associated with the product
    if (!productPackage && !orderItem?.productSupplier) {
        productPackage = orderItem.product.getDefaultProductPackage(orderItem.quantityPerUom, orderItem.quantityUom)
        }
    }