OBPIH-6052 validation for zero items to invoice in invoices workflow by drodzewicz · Pull Request #4498 · openboxes/openboxes

drodzewicz · 2024-02-08T14:15:58Z

The most important part I would say is located in the InvoiceService.
To have the same behavior as we have in inbounde/outbound stockMovements I had to add additional logic that would eliminate invoiceItems with quantity == 0.

Additionally it was preventing user from going forward if there are no items in the add items page table.
Adding a confirmation modal with message that there are zeros in your rows and will be deleted.
And I also used this opportunity to move some hardcoded services to invoiceApi like getInvoiceItems, removeItems and saveInvoiceitems

…ice add items page

…quantity 0

alannadolny · 2024-02-08T14:33:10Z

grails-app/services/org/pih/warehouse/invoice/InvoiceService.groovy

@@ -213,16 +213,24 @@ class InvoiceService {
    def updateItems(Invoice invoice, List items) {
        items.each { item ->
            InvoiceItem invoiceItem = InvoiceItem.get(item.id)


Maybe it will be better to use getAll of this invoiceItem before starting iterating over this collection. For now, you are making calls to the database every iteration.

I see your point, but then this would require a bigger refactor of this whole updateItems method because we have two cases we need to cover
if invoiceItem exists then update it
if it doesn't;t then create a new one out of invoice item candidate.

This might be worth looking into so thanks for pointing this out, but I am going to have to think about it and find a better, more optimal approach.

alannadolny · 2024-02-08T14:38:00Z

grails-app/services/org/pih/warehouse/invoice/InvoiceService.groovy

            if (invoiceItem) {
-                invoiceItem.quantity = item.quantity
+                if (item.quantity > 0) {
+                    invoiceItem.quantity = item.quantity
+                } else {
+                    removeInvoiceItem(invoiceItem.id)
+                }
            } else {
-                InvoiceItemCandidate candidateItem = InvoiceItemCandidate.get(item.id)
-                if (!candidateItem) {
-                    throw new IllegalArgumentException("No Invoice Item Candidate found with ID ${item.id}")
+                // create new invoice item from candidate
+                if (item.quantityToInvoice > 0) {
+                    InvoiceItemCandidate candidateItem = InvoiceItemCandidate.get(item.id)
+                    if (!candidateItem) {
+                        throw new IllegalArgumentException("No Invoice Item Candidate found with ID ${item.id}")


I am trying to find a better way how to avoid nesting those conditions...
but nothing better comes to my mind, the only thing that can be done is just return from ifs, but in the case of 3 levels of nesting it will be probably unreadable. Maybe other developers will have some ideas on how to make it look cleaner.

yes, I had the same problem with this.
I had a version that would return in the first if, basically simulating the continue in the for loop, but in the end thought that maybe if/else will be better, because some developers might assume that this return breaks out of the for each loop instead of skipping the iteration.

It would be nice to deal with the exception as error handling so that we can return errors for multiple objects at once. However, this is not necessary since this code pre-exists this PR.

alannadolny · 2024-02-08T14:42:10Z

src/js/api/urls.js

 // INVOICE
 export const INVOICE_API = `${API}/invoices`;
+export const INVOICE_ITEMS = id => `${INVOICE_API}/${id}/items`;
+export const REMOVE_INVOICE_ITEM = id => `${INVOICE_API}/${id}/removeItem`;


Maybe it will be worth creating a ticket for a refactoring of part of our URLs. I mean that we should avoid RPC style endpoint and basically in this case it should be enough to remove /removeItem because we are already sending a delete request for a specific resource.
I am interested in the opinions of other developers @kchelstowski @awalkowiak

I think if we wanted to refactor it, we'd have to have a separate InvoiceItemApiController to handle that, because if we were about to remove removeItem, we'd have:

/api/invoices/:id

that in my opinion when called with DELETE, should delete an Invoice, not an InvoiceItem

@kchelstowski is correct.

We could handle this in a Restful way by adding an invoitceItems subresource to the Invoice API.

/api/invoices/:invoiceId/invoiceItems/:invoiceItemId

However, I think we can leave this refactoring for later when we actually have the API squared away.

alannadolny · 2024-02-08T14:45:16Z

src/js/components/invoice/AddItemsPage.jsx

  }

+  confirmSave(onConfirm) {
+    confirmAlert({


It's sad, because our new modal is on a different branch :(

alannadolny · 2024-02-08T14:46:59Z

src/js/components/invoice/AddItemsPage.jsx

-    this.saveInvoiceItems(values).then(() => {
-      this.props.nextPage(values);
-    });
+    const hasZeros = _.some(values.invoiceItems, (item) => _.parseInt(item.quantity) === 0);


I assume that you can't just do:

!_.parseInt(item.quantity)

right?

I would like to avoid this type of an approach like you provided in the comment.
We are not gaining anything besides writing fewer characters in the code.

I strongly believe that this _.parseInt(item.quantity) === 0 is way more explanatory than this !_.parseInt(item.quantity) because in the first case, I explicitly state that I don't want zero values.

My opinion is that this code is much easier to maintain, because when developer reads these lines there is little to think about it, you odn't have to think about casting to boolean but focus on a concrete case.

As for handling null empty strings and undefined such logic should be handled in a form validator method.

alannadolny · 2024-02-08T14:51:10Z

src/js/components/invoice/AddItemsPage.jsx

    errors.invoiceItems = [];
-
    _.forEach(values?.invoiceItems, (item, key) => {
+      if (_.isNil(_.get(item, 'quantity'))) {


Basically, I don't see any benefits of using the _.get method. Probably it was used, because we didn't have null safe operators before, and this method didn't fail when we tried to get value from a null object (but I could be wrong about that). For now, I would prefer using null safes.

if (_.isNil(item?.quantity))

kchelstowski · 2024-02-08T15:44:17Z

src/js/api/urls.js

 // INVOICE
 export const INVOICE_API = `${API}/invoices`;
+export const INVOICE_ITEMS = id => `${INVOICE_API}/${id}/items`;
+export const REMOVE_INVOICE_ITEM = id => `${INVOICE_API}/${id}/removeItem`;


I think if we wanted to refactor it, we'd have to have a separate InvoiceItemApiController to handle that, because if we were about to remove removeItem, we'd have:

/api/invoices/:id

that in my opinion when called with DELETE, should delete an Invoice, not an InvoiceItem

kchelstowski · 2024-02-08T15:46:40Z

src/js/components/invoice/AddItemsPage.jsx

+        this.saveInvoiceItems(values).then(() => this.props.nextPage(values));
+      });
+    } else {
+      this.saveInvoiceItems(values).then(() => this.props.nextPage(values));


since 347 line and 350 are the same, we could potentially move it to a separate method, not to repeat the same code, but treat it rather as a nitpicky suggestion.

…ger fetch for all invoiceItems that belong to the current invoice

jmiranda · 2024-02-12T15:55:34Z

grails-app/services/org/pih/warehouse/invoice/InvoiceService.groovy

    }

    def updateItems(Invoice invoice, List items) {
+        List<InvoiceItem> currentInvoiceItems = InvoiceItem.findAllByInvoice(invoice)


Remove this

jmiranda · 2024-02-12T15:56:15Z

grails-app/services/org/pih/warehouse/invoice/InvoiceService.groovy

+
        items.each { item ->
-            InvoiceItem invoiceItem = InvoiceItem.get(item.id)
+            InvoiceItem invoiceItem = currentInvoiceItems.find{ it.id == item?.id }


This can be simplified to

invoice.invoiceItems.find { it.id == item?.id }

jmiranda · 2024-02-12T15:59:50Z

grails-app/services/org/pih/warehouse/invoice/InvoiceService.groovy

            if (invoiceItem) {
-                invoiceItem.quantity = item.quantity
+                if (item.quantity > 0) {
+                    invoiceItem.quantity = item.quantity
+                } else {
+                    removeInvoiceItem(invoiceItem.id)
+                }
            } else {
-                InvoiceItemCandidate candidateItem = InvoiceItemCandidate.get(item.id)
-                if (!candidateItem) {
-                    throw new IllegalArgumentException("No Invoice Item Candidate found with ID ${item.id}")
+                // create new invoice item from candidate
+                if (item.quantityToInvoice > 0) {
+                    InvoiceItemCandidate candidateItem = InvoiceItemCandidate.get(item.id)
+                    if (!candidateItem) {
+                        throw new IllegalArgumentException("No Invoice Item Candidate found with ID ${item.id}")


It would be nice to deal with the exception as error handling so that we can return errors for multiple objects at once. However, this is not necessary since this code pre-exists this PR.

jmiranda · 2024-02-12T16:01:36Z

grails-app/services/org/pih/warehouse/invoice/InvoiceService.groovy

+            // update existing invoice item
            if (invoiceItem) {
-                invoiceItem.quantity = item.quantity
+                if (item.quantity > 0) {


I'm not sure where to put the validation logic, but it would be good to encapsulate all validation logic in one place and check to see if the whole object is ok. However, since we're only updating quantity here I think we're ok. So nevermind.

…ng invoiceItems separately

drodzewicz added 4 commits February 8, 2024 14:41

OBPIH-6052 Disable next button if there are not items selected

86ba486

OBPIH-6052 Added urls and services for invoices

b353f94

OBPIH-6052 Added confirmation alert for linez with 0 quantity on invo…

ac45b6c

…ice add items page

OBPIH-6052 Updated logic for invoiceItem update to remove items with …

7be1971

…quantity 0

drodzewicz requested review from alannadolny, awalkowiak and kchelstowski February 8, 2024 14:15

drodzewicz self-assigned this Feb 8, 2024

alannadolny reviewed Feb 8, 2024

View reviewed changes

kchelstowski reviewed Feb 8, 2024

View reviewed changes

OBPIH-6052 Replace individual InvoiceItem db fetches with a single ea…

ab260da

…ger fetch for all invoiceItems that belong to the current invoice

drodzewicz requested a review from alannadolny February 9, 2024 13:09

kchelstowski approved these changes Feb 12, 2024

View reviewed changes

alannadolny approved these changes Feb 12, 2024

View reviewed changes

jmiranda requested changes Feb 12, 2024

View reviewed changes

OBPIH-6052 use cached invoiceItems value on invoice instead of fetchi…

ea0cef9

…ng invoiceItems separately

awalkowiak approved these changes Feb 13, 2024

View reviewed changes

awalkowiak merged commit ec64a13 into feature/upgrade-to-grails-3.3.10 Feb 13, 2024

awalkowiak deleted the OBPIH-6052-validation-for-zero-items-to-invoice-in-invoices-workflow branch February 13, 2024 11:38

Uh oh!

Conversation

drodzewicz commented Feb 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drodzewicz Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

drodzewicz Feb 8, 2024 •

edited

Loading