Define Incorrect Behavior of haveInvoiceItems #871
Conversation
- Define incorrect behavior of `InventoryBatch.haveInvoiceItems()` in model unit test `haveInvoiceItems` returns `true` is `invoiceItems` is empty. This should likely be revered or method name should change.
|
I did not want to simply "fix" this as I was unsure if the behavior needed to be preserved. This also causes issues in the validations of the model as well. Let me know after merging this and I can fix the method and update the tests to match. |
|
@mkly it looks like you came across a bug. There is no reason that haveInvoiceItems should be operating the way that it is. Can you correct haveInvoiceItems as part of this PR instead of adding an incorrect test? |
- Fix InventoryBatch.haveInventoryItems() to return true when not empty
|
Barring any issues in CI this should be ready for review. I did some manual testing for the application but I was not fully able to understand where these validations are being used. You may want to have a quick look around to make sure this does not break something that was dependent on this returning the incorrect value or point me in the direction of the routes to test and I would be more than happy to test it further. |
There was a problem hiding this comment.
@mkly I tested this PR and realized that the way it was originally functioning was as intentioned, but the name of the function/how it was used was misleading.
These validations are used on the Inventory Received screen (eg http://localhost:4200/#/inventory/batch/new) and the idea on that screen is that you can enter in multiple items from an invoice (in which case haveInvoice Items should return true and validation shouldn't take place because the invoiceItems have already been validated and the input fields can be ignored), but you can use that screen to just enter in one invoice item in which case the validation needs to fire to make sure you have filled out all the fields/have valid data.
To completely clear up the confusion, I think this PR should have the following additional change: in the validations the code should be changed from:
return object.haveInvoiceItems();to:
return !object.haveInvoiceItems();Does that make sense?
|
Absolutely makes sense. This was part of my concern with changing the behavior. I'll make those adjustments. Thanks for testing it out. |
- Fix conditional validation in inventory-batch to fix of `haveInvoiceItems`. - Add tests for validations As 'haveInvoiceItems` method has been flipped to now return true when `invoiceItems` are empty the validations for the model were made incorrect. This commit adjusts those validations to align with the updated method.
| @@ -16,7 +16,7 @@ export default AbstractModel.extend({ | |||
| }, | |||
| inventoryItemTypeAhead: { | |||
| presence: { | |||
| if(object) { | |||
| unless(object) { | |||
There was a problem hiding this comment.
This doesn't resolve the issue. unless(object) isn't the same as unless(object.haveInvoiceItems)
The validation needs to fire when object.haveInvoiceItems is false.
There was a problem hiding this comment.
It is very possible that I am missing something, but my understanding is that unless is the inverse of if. Meaning that unless will skip validation if the method returns true as opposed to if which will skip validation if the method return false. The pseudo code interpretation would be "Unless the object has invoice items, trigger validation".
An example of this is
https://github.com/mkly/hospitalrun-frontend/blob/af38a215463682ef4f31b6774078cb3145ae4497/tests/unit/models/inventory-batch-test.js#L34-L36
The validation is skipped because invoiceItems is non empty and therefore haveInvoiceItems returns true
The opposite is
https://github.com/mkly/hospitalrun-frontend/blob/af38a215463682ef4f31b6774078cb3145ae4497/tests/unit/models/inventory-batch-test.js#L32
The validation here is run because invoiceItems is not set(empty) and therefore haveInvoiceItems returns false. This is why the assertion of testInvalidPropertyValues passes here but not in the above linked test. Because in this test the validation did run.
I probably could have made this pull request easier to follow by just appending ! in order to invert the call to haveInvoiceItems. I just thought using unless was a little clearer to someone coming into this fresh. That said, unless is not a conditional keyword in JavaScript like it is in ruby and perl. Let me know if I'm way off here and my apologies in advance if I am missing the obvious. And if you would prefer I can just invert with ! and use if instead of unless.
Thanks again for taking the time to look this over.
There was a problem hiding this comment.
Sorry updated comment. One of my true should have been false in my description of the issue.
There was a problem hiding this comment.
@mkly my bad -- I misread the code.... I translated it as a regular javascript expression, eg if object is defined, return the value from object.hasInvoiceItems() to decide if validation fires.
You were correct in using unless and I think it is fine to use it in the project.
There was a problem hiding this comment.
Glad to hear I didn't mess something up. I worry with these pull requests I'll pull you away from more pressing matters if I start introducing bugs and other issues. Thanks as always.
| @@ -16,7 +16,7 @@ export default AbstractModel.extend({ | |||
| }, | |||
| inventoryItemTypeAhead: { | |||
| presence: { | |||
| if(object) { | |||
| unless(object) { | |||
There was a problem hiding this comment.
@mkly my bad -- I misread the code.... I translated it as a regular javascript expression, eg if object is defined, return the value from object.hasInvoiceItems() to decide if validation fires.
You were correct in using unless and I think it is fine to use it in the project.
Changes proposed in this pull request:
InventoryBatch.haveInvoiceItems()inmodel unit test
haveInvoiceItemsreturnstrueifinvoiceItemsis empty. Thisshould likely be reversed or method name should change.
cc @HospitalRun/core-maintainers