OBPIH-6501 Add endpoint to remove invoice item by awalkowiak · Pull Request #4788 · openboxes/openboxes

awalkowiak · 2024-08-14T16:14:06Z

✨ Description of Change

A concise summary of what is being changed. Please provide enough context for reviewers to be able to understand the change and why it is necessary. If the issue/ticket already provides enough information, you can put "See ticket" as the description.

Link to GitHub issue or Jira ticket:
https://pihemr.atlassian.net/browse/OBPIH-6501

Description:
Endpoint to remove invoice item

📈 Test Plan

We require that all code changes come paired with a method of testing them. Please select which of the following testing approaches you've included with this change:

Frontend automation tests (unit)
Backend automation tests (unit, API, smoke)
End-to-end tests (Playwright)
Manual tests (please describe below)
Not Applicable

Description of test plan (if applicable):
Will be tested with frontend changes

✅ Quality Checks

Please confirm and check each of the following to ensure that your change conforms to the coding standards of the project:

The pull request title is prefixed with the issue/ticket number (Ex: [OBS-123] for Jira, [#0000] for GitHub, or [OBS-123, OBPIH-123] if there are multiple), or with [N/A] if not applicable
The pull request description has enough information for someone without context to be able to understand the change and why it is needed
The change has tests that prove the issue is fixed / the feature works as intended

awalkowiak · 2024-08-14T16:16:50Z

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

+     * */
+    void updateInvoiceItemQuantity(String itemId, Integer quantity) {
+        InvoiceItem invoiceItem = InvoiceItem.get(itemId)
+        if (quantity > invoiceItem.quantity) {


Actually, this should be checking the quantity available to invoice from the shipment item instead of the current invoice item's quantity. Will fix that on Monday.

Also, if you're checking shipment items quantity be weary of the case where an item is split across multiple shipments

ewaterman · 2024-08-20T13:55:03Z

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

+        InvoiceItem prepaymentItem = orderItem.invoiceItems.find { it.isPrepaymentInvoice }
+        InvoiceItem inverseItem = findInverseItem(invoiceItem)
+        inverseItem.quantity = quantity
+        inverseItem.amount = quantity * prepaymentItem.unitPrice * prepaymentPercent * (-1)


can you move this amount calculation to a method so this logic doesn't need to be duplicated everywhere?

Sure will do it. btw I am moving out the edit endpoint to separate PR.

ewaterman · 2024-08-20T14:00:08Z

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

+            throw new IllegalArgumentException("Cannot update quantity invoiced to higher value")
        }
+        if (quantity <= 0) {
+            throw new IllegalArgumentException("Quantity invoiced needs to be higher than 0")


we should also have a case here for not allowing the user to edit the quantity if quantity is 0 / the shipment item is cancelled. For that case quantity must always be 0.

But we don't want to allow cancelling items through this (editing to 0), but I double check that.

ewaterman · 2024-08-20T14:04:02Z

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

+    private void deleteInvoiceItem(InvoiceItem invoiceItem) {
+        invoiceItem.orderAdjustments?.each { OrderAdjustment oa -> oa.removeFromInvoiceItems(invoiceItem) }
+        invoiceItem.orderItems?.each { OrderItem oi -> oi.removeFromInvoiceItems(invoiceItem) }
+        invoiceItem.shipmentItems?.each { ShipmentItem si -> si.removeFromInvoiceItems(invoiceItem) }


is there no way in GORM to set this to be automatic? If an item is deleted, remove it from all relationships? It's weird to me that this needs to be manual

@ewaterman These are many to many relations, and there is the case that cascade is for one to many only and when we still have invoice item instance on the related object (order item, order adjustment, or shipment item), then there would be thrown

org.springframework.dao.InvalidDataAccessApiUsageException: deleted object would be re-saved by cascade (remove deleted object from associations)

See:
https://spring.io/blog/2010/07/02/gorm-gotchas-part-2#deletes-do-not-cascade-period
https://grails.github.io/legacy-grails-doc/3.2.0/ref/Database%20Mapping/cascade.html

jmiranda · 2024-08-20T15:29:52Z

grails-app/domain/org/pih/warehouse/invoice/InvoiceItemType.groovy

@@ -0,0 +1,18 @@
+package org.pih.warehouse.invoice
+
+enum InvoiceItemType {


If we're keeping this (I assumed we wouldn't) we should call this InvoiceItemTypeCode to leave room for adding a proper domain in the future.

@jmiranda no, were removing this, the PR was not updated to the most recent changes yet

… of prepaid POs)

alannadolny · 2024-08-21T08:24:29Z

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

+        Invoice invoice = invoiceItem.invoice
+
+        // Since we can have only one of the three options I am doing this in this way (at least for now)
+        def relatedObject = invoiceItem.orderAdjustment ?: invoiceItem.orderItem ?: invoiceItem.shipmentItem ?: null


Is this fallback for null needed? I think it should be null when the shipmentItem is null.

alannadolny · 2024-08-21T08:25:08Z

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

+        Invoice invoice = invoiceItem.invoice
+        invoice.removeFromInvoiceItems(invoiceItem)


Is there a benefit of assigning invoice to the variable? You can just do:

invoiceItem.invoice.removeFromInvoiceItems(invoiceItem)

awalkowiak added status: work in progress For issues or pull requests that are in progress but not yet completed status: ready for review Flags that a pull request is ready to be reviewed labels Aug 14, 2024

awalkowiak requested review from alannadolny, drodzewicz, ewaterman, jmiranda and kchelstowski August 14, 2024 16:14

github-actions bot added the domain: backend Changes or discussions relating to the backend server label Aug 14, 2024

awalkowiak commented Aug 14, 2024

View reviewed changes

alannadolny mentioned this pull request Aug 18, 2024

OBPIH-6531 [frontend] Ability to remove invoice item #4790

Merged

Base automatically changed from OBPIH-6499 to feature/OBPIH-6398-partial-invoices-for-prepaid-po August 19, 2024 13:32

ewaterman reviewed Aug 20, 2024

View reviewed changes

jmiranda reviewed Aug 20, 2024

View reviewed changes

awalkowiak force-pushed the OBPIH-6501 branch 2 times, most recently from 25c3c13 to 5a7fb3b Compare August 21, 2024 07:42

awalkowiak changed the title ~~OBPIH-6501 and OBPIH-6502 Add endpoints to edit invoice item quantity and to remove …~~ OBPIH-6501 Add endpoint to remove invoice item Aug 21, 2024

awalkowiak force-pushed the OBPIH-6501 branch from 5a7fb3b to 48bedd5 Compare August 21, 2024 07:49

OBPIH-6501 Add API endpoint to delete invoice item (partial invoicing…

4d57dad

… of prepaid POs)

awalkowiak force-pushed the OBPIH-6501 branch from 48bedd5 to 4d57dad Compare August 21, 2024 07:50

alannadolny reviewed Aug 21, 2024

View reviewed changes

OBPIH-6501 Minor improvements after review

477a289

alannadolny approved these changes Aug 21, 2024

View reviewed changes

awalkowiak merged commit 98daa1d into feature/OBPIH-6398-partial-invoices-for-prepaid-po Aug 21, 2024

awalkowiak deleted the OBPIH-6501 branch August 21, 2024 10:38

awalkowiak added a commit that referenced this pull request Sep 6, 2024

OBPIH-6501 Add endpoint to remove invoice item (#4788)

58ac999

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

OBPIH-6501 Add endpoint to remove invoice item#4788

OBPIH-6501 Add endpoint to remove invoice item#4788
awalkowiak merged 2 commits intofeature/OBPIH-6398-partial-invoices-for-prepaid-pofrom
OBPIH-6501

awalkowiak commented Aug 14, 2024 •

edited

Loading

Uh oh!

awalkowiak Aug 14, 2024

Uh oh!

ewaterman Aug 20, 2024

Uh oh!

ewaterman Aug 20, 2024

Uh oh!

awalkowiak Aug 21, 2024

Uh oh!

ewaterman Aug 20, 2024

Uh oh!

awalkowiak Aug 21, 2024

Uh oh!

ewaterman Aug 20, 2024

Uh oh!

awalkowiak Aug 20, 2024

Uh oh!

jmiranda Aug 20, 2024

Uh oh!

awalkowiak Aug 20, 2024

Uh oh!

alannadolny Aug 21, 2024

Uh oh!

alannadolny Aug 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		@@ -0,0 +1,18 @@
		package org.pih.warehouse.invoice

		enum InvoiceItemType {

		Invoice invoice = invoiceItem.invoice
		invoice.removeFromInvoiceItems(invoiceItem)

Uh oh!

Conversation

awalkowiak commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✨ Description of Change

📈 Test Plan

✅ Quality Checks

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!

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!

4 participants

awalkowiak commented Aug 14, 2024 •

edited

Loading