Skip to content
This repository has been archived by the owner on Jun 13, 2018. It is now read-only.

Quick fix for Stamps.com rate amount bug #457

Closed
wants to merge 1 commit into from
Closed

Quick fix for Stamps.com rate amount bug #457

wants to merge 1 commit into from

Conversation

dustMason
Copy link
Contributor

In lieu of rewriting ActiveShipping::Package#cents_from as discussed in #455, this is an effective fix for the bug described on that issue.

Stamps.com returns rate estimate amounts as decimal dollar values
Copy link
Member

@kmcphillips kmcphillips 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 in to this fix.

Can you include a test to cover please?

@dustMason
Copy link
Contributor Author

I returned to this just now to add a test and realized that there are money amounts in several places in the Stamps.com API responses that should be treated the same (as decimal values).

This, plus the 😟 feeling I get whenever a money value gets converted into a Float, is making me want to re-think this fix. I'm going to close this PR and consider a better fix for the issue.

I think this means that each Carrier subclass should be able to define its own money parsing methods. I'm not sure how else to determine that a given integer value is an amount in dollars or cents.

@dustMason dustMason closed this Jan 9, 2017
@kmcphillips
Copy link
Member

Sound good. Thanks for thinking about quality.

I actually really like the idea of making each Carrier subclass override or implement its own money. cc @jonathankwok @thegedge

@kmcphillips kmcphillips mentioned this pull request Jan 9, 2017
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