Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Fix math in InventoryPurchaseItem.costPerUnit and add math util for rounding #864

Merged
merged 7 commits into from
Dec 14, 2016

Conversation

mkly
Copy link
Contributor

@mkly mkly commented Dec 13, 2016

Fixes math in InventoryPurchaseItem.costPerUnit and add math util for rounding

Changes proposed in this pull request:

  • Fix incorrect calculation InventoryPurchaseItem.costPerUnit to round
    property to the nearest 100th
  • Add test for demostrating issue in inv-purchase-test.js
    "costPerUnit properly round"
  • Add unit model test inv-purchase InventoryPurchaseItem
  • Add math util with function round100
  • Add tests for round100 function

cc @HospitalRun/core-maintainers

mkly added 2 commits December 13, 2016 12:56
- Add unit model test inv-purchase InventoryPurchaseItem
- Fix incorrect calculation `InventoryPurchaseItem.costPerUnit` to round
  property to the nearest 100th
- Add test for demostrating issue in `inv-purchase-test.js`
  "costPerUnit properly round"
- Add math util with function `round100`
- Add tests for `round100` function
@mkly
Copy link
Contributor Author

mkly commented Dec 13, 2016

Using fixedTo(2) fails on numbers like 35.555(rounds to 35.55). Normally this isn't a big deal with displays but since these are financials and I've seen various discussions about reporting I figure it would be wise to get a bit closer.

It is possible that this is not the only place this issue exists. I haven't looked for other spots as I wanted to make sure this was an acceptable solution going forward.

It may also be a better choice to instead bring in a library specifically designed to handle JavaScript's various math quirks. The advantage is that it would likely handle further edge cases than our internal implementations. The disadvantage is that is adds one more library to maintain and increases the overall download size of the application.

Edit: Typo in rounds to result

Copy link
Member

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkly thanks for the PR. I would prefer to update the mixin https://github.com/HospitalRun/hospitalrun-frontend/blob/master/app/mixins/number-format.js to handle this rounding error and then use that mixin vs the toFixed(2)

@mkly
Copy link
Contributor Author

mkly commented Dec 13, 2016

Sounds good. Will do. Thanks

mkly added 4 commits December 13, 2016 18:06
- Fix rounding in `_numberFormat` for number-format mixin
- Add mixin unit tests for number-format mixin
- Use `NumberFormat` mixin for `InventoryPurchaseItem` and use in
  `costPerUnit` method
- Remove math util as no longer needed
@mkly
Copy link
Contributor Author

mkly commented Dec 13, 2016

Barring any CI issues this should be ready for review. Hopefully I understood you correctly in the use of the mixin here. I went ahead and added a little test coverage for NumberFormat while I was in there. Thanks again for taking the time for the review. Still learning my way around the code base and getting up to speed with the preferred conventions. 🍹

@mkly
Copy link
Contributor Author

mkly commented Dec 13, 2016

Also, let me know if you want me to squash these commits down and open a new PR for merging

@jkleinsc
Copy link
Member

Looks good to me @mkly! I can squash the commits here on GitHub.

@jkleinsc jkleinsc merged commit 262860c into HospitalRun:master Dec 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants