-
-
Notifications
You must be signed in to change notification settings - Fork 403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OBPIH-6052 validation for zero items to invoice in invoices workflow #4498
OBPIH-6052 validation for zero items to invoice in invoices workflow #4498
Conversation
…ice add items page
@@ -213,16 +213,24 @@ class InvoiceService { | |||
def updateItems(Invoice invoice, List items) { | |||
items.each { item -> | |||
InvoiceItem invoiceItem = InvoiceItem.get(item.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -34,6 +34,8 @@ export const STOCK_TRANSFER_REMOVE_ALL_ITEMS = id => `${STOCK_TRANSFER_BY_ID(id) | |||
|
|||
// 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`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
.catch(() => Promise.reject(new Error('react.invoice.error.saveInvoiceItems.label'))); | ||
} | ||
return Promise.resolve(); | ||
} | ||
|
||
confirmSave(onConfirm) { | ||
confirmAlert({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's sad, because our new modal is on a different branch :(
this.saveInvoiceItems(values).then(() => { | ||
this.props.nextPage(values); | ||
}); | ||
const hasZeros = _.some(values.invoiceItems, (item) => _.parseInt(item.quantity) === 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that you can't just do:
!_.parseInt(item.quantity)
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
_.forEach(values?.invoiceItems, (item, key) => { | ||
if (_.isNil(_.get(item, 'quantity'))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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))
@@ -34,6 +34,8 @@ export const STOCK_TRANSFER_REMOVE_ALL_ITEMS = id => `${STOCK_TRANSFER_BY_ID(id) | |||
|
|||
// 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`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
this.saveInvoiceItems(values).then(() => this.props.nextPage(values)); | ||
}); | ||
} else { | ||
this.saveInvoiceItems(values).then(() => this.props.nextPage(values)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -211,18 +211,28 @@ class InvoiceService { | |||
} | |||
|
|||
def updateItems(Invoice invoice, List items) { | |||
List<InvoiceItem> currentInvoiceItems = InvoiceItem.findAllByInvoice(invoice) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this
items.each { item -> | ||
InvoiceItem invoiceItem = InvoiceItem.get(item.id) | ||
InvoiceItem invoiceItem = currentInvoiceItems.find{ it.id == item?.id } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified to
invoice.invoiceItems.find { it.id == item?.id }
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}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if (invoiceItem) { | ||
invoiceItem.quantity = item.quantity | ||
if (item.quantity > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
invoiceApi
likegetInvoiceItems
,removeItems
andsaveInvoiceitems