Skip to content
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

Raise exception if dividing by 0 #3305

Merged
merged 1 commit into from
Aug 23, 2019
Merged

Conversation

ericsaupe
Copy link
Contributor

While this should never happen it is possible to call this method
and subsequently try to save a NaN value if the line_item has
already been fully cancelled.

Description

This came up in our store today. I was unable to recreate it but somehow one of our customer service representatives short shipped an item and the adjustment, and subsequent adjustment total on the order, went to NaN. Looking into the calculation it looks like it may be possible for the short ship compute amount to return that result. This PR raises an exception if, somehow, that method is called and tries to divide by 0.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)

Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

👍 Probably not a bad idea. Thanks!

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

I'm fine with this change but I'd move the raise in the caller method. Let me know your thoughts and thanks!

core/app/models/spree/unit_cancel.rb Outdated Show resolved Hide resolved
@ericsaupe ericsaupe force-pushed the divide-by-0 branch 2 times, most recently from d8c10ac to 104897d Compare August 21, 2019 15:39
@kennyadsl
Copy link
Member

@ericsaupe just thinking: wouldn't this lead to another exception ZeroDivisionError without the need to explicitly raise it?

@ericsaupe
Copy link
Contributor Author

@kennyadsl because quantity_of_line_item is a BigDecimal it actually returns BigDecimal('-Infinity') when quantity_of_line_item is BigDecimal(0)

@ericsaupe
Copy link
Contributor Author

It probably makes more sense to really return that error. I'll change that.

While this should never happen it is possible to call this method
and subsequently try to save a NaN value if the line_item has
already been fully cancelled.
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks @ericsaupe!

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Looks fine

@kennyadsl kennyadsl merged commit 0ba0787 into solidusio:master Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants