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

Fix typo in address parameter helper #47

Merged
merged 3 commits into from
Mar 22, 2021
Merged

Fix typo in address parameter helper #47

merged 3 commits into from
Mar 22, 2021

Conversation

forkata
Copy link
Collaborator

@forkata forkata commented Mar 15, 2021

What is the goal of this PR?

Fixes a typo in the helper method which transforms a Spree::Address into the payload parameters we send to TaxJar.

How do you manually test these changes? (if applicable)

  1. Use an order with an address without a state association, but with a state_name instead.
    • The order address should be serialized correctly and the state_name field should be serialized and sent to Tax Jar.

Merge Checklist

  • Run the manual tests
  • Update the changelog (I am not sure if this needs to be done for this change?)

Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

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

Changelog should reflect the bug fix please!

Copy link
Member

@Noah-Silvera Noah-Silvera left a comment

Choose a reason for hiding this comment

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

LGTM pending the changelog update!

@forkata forkata requested a review from jarednorman March 18, 2021 07:49
forkata added 2 commits March 18, 2021 00:49
This change adds a failing spec which exposes a typo in our API params
helper method. In the next change we'll fix the issue and un-pend the
spec.
This change fixes a typo in the params helper for addresses without a
`state` association. This was a previously untested behaviour so we never
caught this. This change also marks the failing test as no longer
pending.
Noah-Silvera
Noah-Silvera approved these changes Mar 18, 2021
This links to the PR where we fixed the issue from the changelog file,
so we can know what is in the next release of the gem.
@forkata forkata requested a review from Noah-Silvera March 19, 2021 21:52
Copy link
Member

@Noah-Silvera Noah-Silvera left a comment

Choose a reason for hiding this comment

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

LGTM! Merge at will

@forkata forkata merged commit 5355651 into master Mar 22, 2021
@forkata forkata deleted the forkata/fix-typo branch March 22, 2021 18:02
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