OBPIH-6499 [backend] Calculate inverse items#4779
OBPIH-6499 [backend] Calculate inverse items#4779awalkowiak merged 6 commits intofeature/OBPIH-6398-partial-invoices-for-prepaid-pofrom
Conversation
alannadolny
added
the
warn: do not merge
github-actions
bot
added
domain: backend
awalkowiak
added
the
status: work in progress
|
I will be taking over this one |
github-actions
bot
added
the
domain: frontend
awalkowiak
removed
status: work in progress
awalkowiak
requested review from
awalkowiak,
ewaterman,
jmiranda and
kchelstowski
awalkowiak
changed the base branch from
develop
to
feature/OBPIH-6398-partial-invoices-for-prepaid-po
|
|
||
| String id | ||
| Invoice invoice | ||
| InvoiceItemType invoiceItemType |
There was a problem hiding this comment.
@jmiranda let me know if you prefer this one as a separate InvoiceItemType domain that has name and code as InvoiceItemTypeCode enum instead straight away using enum here, and I'll adjust that
There was a problem hiding this comment.
Yeah, now that I'm seeing how it's being used, I don't think InvoiceItemType is correct. An invoice item type would describe what the invoice item is for i.e. product, service, adjustment, subscription, labor, tax, discount, fees, etc.
What we're talking about here is something I still don't really understand. But it feels like it might be the invoice type (prepayment, standard, final) and should go at the Invoice level.
This is why I wish we had done the "from scratch" design instead of trying to build on what we had (at least from the data model perspective) so that we could have spent more time thinking about the proper way to do this. Frankly, I don't have a good enough understanding of it so I guess I'll leave it up to you. But I would really consider moving this "type" to the invoice and derive the "inverse" logic based on, I guess whether the item is in an invoice with InvoiceTypeCode.FINAL (and whatever other)
In either case, the safer option would be to use the enum here since we could always migrate to using a type domain that maps to the enum later.
grails-app/migrations/0.9.x/changelog-2024-08-05-1300-add-invoice-item-type-to-invoice-item.xml
Outdated
Show resolved
Hide resolved
| return invoiceItem | ||
| } | ||
|
|
||
| InvoiceItem createFromOrderItem(OrderItem orderItem) { |
There was a problem hiding this comment.
If anyone wonders, I moved these to PrepaymntInvoiceService
grails-app/controllers/org/pih/warehouse/api/InvoiceApiController.groovy
Outdated
Show resolved
Hide resolved
grails-app/services/org/pih/warehouse/invoice/PrepaymentInvoiceService.groovy
Show resolved
Hide resolved
grails-app/services/org/pih/warehouse/invoice/PrepaymentInvoiceService.groovy
Outdated
Show resolved
Hide resolved
|
|
||
| enum InvoiceItemType { | ||
|
|
||
| INVERSE('Inverse'), |
There was a problem hiding this comment.
@jmiranda Let me know if other types come to your mind you'd like to have added here
There was a problem hiding this comment.
If we added InvoiceTypeCode.FINAL, could the INVERSEness of an item be derived? I assume that the other two (prepayment, regular) could already be derived from the InvoiceType. So even if there's a need to store whether an invoice item is an "inverse" item, we could do so with just a boolean field.
Side note: If/when you add REGULAR InvoiceTypeCode it should be called STANDARD.
There was a problem hiding this comment.
@jmiranda Yeah, to be honest, this part was not designed properly, because we wanted to go with calculating all the time and we switched to keeping inverse items on the final invoice right before we started the development. So something that was my suggested direction in case we want to go with storing inverse items somehow was cemented in my brain as the version we want to do. As deriving item type from invoice type does not work for me here (due to mixing both standard/regular items with inverse items), I decided that the boolean fits here more, and this is what I implemented for now (we can still revise it again in the final feature PR).
grails-app/services/org/pih/warehouse/invoice/PrepaymentInvoiceService.groovy
Outdated
Show resolved
Hide resolved
grails-app/services/org/pih/warehouse/invoice/PrepaymentInvoiceService.groovy
Outdated
Show resolved
Hide resolved
grails-app/services/org/pih/warehouse/invoice/PrepaymentInvoiceService.groovy
Outdated
Show resolved
Hide resolved
src/test/groovy/unit/org/pih/warehouse/invoice/InvoiceServiceSpec.groovy
Show resolved
Hide resolved
src/test/groovy/unit/org/pih/warehouse/invoice/PrepaymentInvoiceServiceSpec.groovy
Show resolved
Hide resolved
grails-app/services/org/pih/warehouse/invoice/PrepaymentInvoiceService.groovy
Show resolved
Hide resolved
|
@ewaterman I replied (and made change requests) to your comments, but don't be mad at me for resolving conversations, I just kept track of resolved requests with this. |
| String type | ||
|
|
||
| String toString() { | ||
| return name() |
There was a problem hiding this comment.
| return name() | |
| return type |
Either that or I'd just remove the type field entirely since it's not being used anywhere but ultimately it's not a big deal.
There was a problem hiding this comment.
@ewaterman i replaced InvoiceItemType with inverse boolean
|
LGTM, but I can't approve this PR because I created it 😄 |
awalkowiak
merged commit 6ccd001
into
feature/OBPIH-6398-partial-invoices-for-prepaid-po
Co-authored-by: Walkowiak <[email protected]>
4 participants
Because I am going ooo, I am pushing my branch with the next steps:
Inverse items are created during the creation of regular, but relations are missing, so:
Check if adding relations (to shipment items etc.) to inverse items will break logic, if so, check how to avoid that, or do it another way. We have to add those relations because some data is missing while displaying invoice items on React pages. After fixing the missing data, inverse the quantity (should be a simple change in calculation + remember about prepaid / 50, etc.)