I think you can do it in one sum:

return invoiceItems.?.sum { it -> 
   if ( it.invoice.isRegularInvoice && !it.inverse) {
      return it.amount
   }

   return 0
} ?: 0

In the other functions, you can do the same, to avoid iterating twice over the collections.

you have "and" at the end of the method name 😄

you can also move the to the separate method, like it is already done in, for example:

• I think using class name instead of strings should be better

Actually considering @ewaterman comment in the other pr, I am wondering if I should not just return the message code, and the front-end should just handle the translation of it, but I don't remember if this is handled properly right there or not.

moving it to a method would at least make it easier to mock in a unit test and is likely good enough for this PR.

I do think we should have a larger group discussion around how localization should work in the backend. My thoughts are that the frontend should handle fetching the localized string and the backend can just return the code, args, and default (minus the "message" tag lib). Since we're planning on eventually splitting the frontend and backend, it seems strange to be working with tag libs at all from the backend. I'd rather have the backend be simple and just return the errors to the client, and then the client can display those errors in whatever way it wants (ie localize them, render them nicely...) but that can be a tech huddle discussion.

Actually, I checked the validate exception handling (ErrorsController) and we are relying on ApplicaitonTagLib there as well, so I guess it can stay as is for now. I just moved it out to the method as suggested by Alan.

Yeah the reason it's happening there is because it's a backend-driven redirect built into Grails. I haven't thought through it enough to have a super strong opinion but we might eventually need to think through a different approach if the goal is to have the backend totally split from the frontend since we won't want backend APIs rendering views.

But yeah, that's fine for now.

maybe it will look cleaner if you create a method like "updateInvoiceItem"
and within this method decide whether you want to update item quantity or unit price, instead of making this decision in the controller. In case we want to make more updates, we will have to nest thousands of if-else closures here.

will check that

I'd either make two methods: updateInvoiceItemForOrderItem and updateInvoiceItemForAdjustment that each have their separate flows (but both stay general purpose so that our controllers can stay clean like Alan said), or just make a single updateInvoiceItem like Alan said.

I know you're doing a lot of validation in the service for both update scenarios and so merging the two could be messy, but maybe that's a sign that we're better off with a command object that has its own validation step that we can define. Something like:

InvoiceItemCommand command = new InvoiceItemCommand(params)
command.validate()
if (command.hasErrors())
    // error early, before we even try to go to the db

InvoiceItem invoiceItemToUpdate = InvoiceItem.findById(command.id)
invoiceItemToUpdate.unitPrice = command.unitPrice
invoiceItemToUpdate.save()
if (invoiceItemToUpdate.hasErrors())
    // error
else
    // return normally

Or maybe we need a custom InvoiceItemValidator class that encapsulates the more complex validation.

Sorry if this is overkill. I've been reading up about request validation the past couple days. Seems like a topic we could think about how we want to handle in general https://docs.grails.org/latest/guide/validation.html#:~:text=A%20common%20pattern%20in%20Grails,share%20properties%20and%20their%20constraints.

Yeah actually a thought came to my mind about the command with validate today. That could be useful, but I wonder if we could do it in a cleanup/improvement ticket or something like that? So we get this feature to the QA asap, and then we can focus on all those improvements we wanted to have here like:

1. command with validation
2. improvements on the transients with sums
3. moving to transients everything that can be moved
4. pulling out some common logic to some service methods
5. more unit tests
6. etc (anything more that comes to your mind right away?)

@ewaterman @alannadolny what do you guys think?

yeah that seems reasonable to me. I'll think more about it today.

Did you create a ticket for that? If not, when it should be checked?

As this is still in WIP, these are TODOs that I sill have to check within this ticket. These are reminders to myself.

btw, I am concerned about not allowing updating quantity and unit price at the same time. Are we sure that we never ever would like to do that?

And I am concerned that you'd like to do that 😅

1. On invoice items from order items or shipment items, the unit price should always be based on the unit price that is on the order item (or prepayment invoice item for inverses), we should not allow changing that.
2. On invoice items from order adjustments the quantity is 1 or 0, we don't want to change from one or another.

Taking these two assumptions into account - we don't want to change both fields at the same time for this type of invoice.

I imagine these are public so that you can unit test them but I think it'd be best in general to make methods private whenever possible and then unit test starting at the entrypoint to the service (like the other tests are doing). It's more complicated to set up, but all you should need to do is set up an invoice in the given block.

Yeah, I specifically wanted to have easy access to these two methods for unit testing, due to positive and negative cases being a brain teaser.

I know this isn't new here, but I generally prefer to strongly type Lists and then do for (InvoiceItem item : items) loops instead of closures to avoid the extra overhead involved with closures. Stack traces also render more properly in regular loops and we can also do break/continue more easily in loops.

I agree with typing of variables whenever possible, I've raised that topic many times not to throw def when it's not a dynamic variable, but I don't have much knowledge what the items are. I can see that type List is passed, so whether those are some kind of items that come from the frontend and are difficult to parse or ....

what scenario is this possible for? Won't every invoice item always have a matching inverse? Yes the inverse might change but will it ever not be there already?

Well it's a pretty wild case but possible, you have to:

1. Prepay adjustment
2. Edit the amount on the adjustment to a higher value
3. Invoice it partially on two final invoices in a way where one will have inverse and the other remaining part won't
4. Remove or edit to lower the unit price on the invoice with inverse
5. then edit to higher on the invoice that initially had no inverse

this is fine, but if we ever forget in other places in the code that amount is derived from quantity and unit price, I can imagine a scenario where it falls out of sync. Maybe it would be better to have a custom setter method that auto-recomputes amount when a new unit price or quantity is set.

wasn't it possible to do it in one loop as @alannadolny suggested here? #4841 (comment)

I guess it was, but we have more of these, so I decided to leave it as is, and potentially to add it to the list of "cleanups/improvements"

I agree with typing of variables whenever possible, I've raised that topic many times not to throw def when it's not a dynamic variable, but I don't have much knowledge what the items are. I can see that type List is passed, so whether those are some kind of items that come from the frontend and are difficult to parse or ....

is there any reason why those blocks can't also be placed in the validateItem method?

moreover I think those should not throw IllegalArgumentException as this would lead to 500 being thrown, instead of 400 that are expected for validation errors.
This leads me to a statement that this would be probably better to place this type of validation in an appropriate command object and a solid validator. Would be easier to maintain this logic for any property, and also to maintain the error messages.

is there any reason why those blocks can't also be placed in the validateItem method?

I just moved out the common part from updateInvoiceItemQuantity and updateInvoiceItemUnitPrice and remaining ones are different for quantity and unit price update cases.

moreover I think those should not throw IllegalArgumentException

Yup, I agree with the ValidationException being more suitable here, but yesterday I thought that change could be done together with adding a / moving to command in a separate ticket, that's why I left these. I'll check if simple exception type change will work here (if it is a quick change I'll swap it).

@awalkowiak unfortunately it won't be a simple change. Since you don't bind the errors to the object, and we were to throw the validation exception, the object's errors would be empty.
If we wanted to do it via your methods, you would have to do something like:

invoiceItem.errors.rejectValue(....)

but I agree and I am fine doing it in the cleanup ticket, we can build a command there, and construct a proper validator.

We just need to be aware not to complicate the frontend error handling too much, as the response status would eventually change from 500 to 400 when we do the cleanup, and then, the axios error handler might build the popup differently.
cc @alannadolny @drodzewicz

@kchelstowski Yup on second thought I think if we go to the command + validation route, then it will be handled there. So I am not going to try to swap it now.

relevant to this discussion, I have a ticket for revisiting how we handle validation errors on the backend https://pihemr.atlassian.net/browse/OBPIH-6727

I hope that after that standard is built, a lot of this type of validation can be simplified.