@ewaterman I still remember you wanted to move it to separate method but I was not sure how to actually do it. Is something like that what you'd expect here:

BigDecimal calculateAmount(Integer quantity, BigDecimal unitPrice, BigDecimal percentage = 1, Integer inverse = 1) {
    return quantity * unitPrice * percentage * inverse
}

and then just passing what we have available or need to use in calculation, or do you see here something a bit more complex in this method?

I think the thing I'm mainly worried about is that we calculate amount all over the place and it'd be nice to have a calculateAmountForPrepaymentItem(), calculateAmountForRegularItem() and calculateAmountForInverseItem() methods just to have it unified.

I haven't thought it through fully so this might not be the best idea, but they could maybe just take in the invoice item itself and do all the lookups they need so that the caller doesn't need to know what to do.

The logic here is just a bit tough to navigate with all the transients and chained calls so it'd be great if there was a simple method of calculating it that brought it all together

Ok, I think I am going to leave it as is for now, but I'll think about this as a standalone improvement in separate ticket/pr

I'm not generally a fan of the camelcase URIs. I usually try to stick to a feature/sub-feature/entity-based system with something like: /api/invoices/prepayment/$id/items or /api/prepayment-invoices/$id/invoice-items. Up to you though

We don't have a written convention for these yet (camel vs kebab case), hence I am following for now what we have in other cases (ex. stockMovements, partialReceiving, etc)

in my opinion we should use proper types unless it's absolutely necessary to use def

I am also always trying to catch all of the unnecessary defs in review 🫶

If I'm doing an update only on the provided fields I usually use PATCH over POST (I think that's the generally accepted convention) but I'm not really bothered either way.

You do have a trailing slash in the URI which is probably best to remove for consistency's sake but up to you.

It might be best to make this method more generic (ie call it updateItems instead of updateItemsQuantity). Right now we only allow you to update the quantity field but that could change in the future.

I asked a similar question in the other PR, but what about cancelled items? You shouldn't be able to update the invoice quantity right? Is that already handled by the quantityToInvoice field? Would it just be 0 in that case?

Canceled items don't have shipment items, so these are already ruled out and we don't allow to change the quantity on them

nitpick: I'd put this case in the if block (and change the condition to if (!inverseItem)) since it's much simpler and so the big block above doesn't need to be nested inside an if block.

btw, from the frontend perspective, update/post methods should return the newly updated state, because we always have to refetch the new data. It's not a change request, because in this one case, on the frontend it's not necessary to fetch the data again, but in most cases, we should.

I agree, but here we have a bit more complex update, where we are passing one invoice item, but in reality we are updating two, so should we return both here or only the one that was sent in requests? Due to this, I decided to keep it simple and return only status, at least for now.

we don't typically bother setting a default value for localization strings do we?

I think we don't have clear convention