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

use state_name in rare cases that state_id is nil, this will prevent … #13

Open
wants to merge 1 commit into
base: support-physical-card
Choose a base branch
from

Conversation

papayaah
Copy link

…the exception

@bryanmtl not exactly sure if this is worth the change - there are only 2 cases of this happening but i can't push the shipment to Quiet because it will cause an exception if state id is nil.

image

@papayaah papayaah requested review from braidn and hugobast March 31, 2017 11:15
@hugobast
Copy link

This looks harmless and I'll come back to it. My initial reaction is that we have a data problem if we have a state_name and no state_id?

@papayaah
Copy link
Author

not really, I think only US provides a dropdown option, everything else is a text field (admin screenshot):

image

@braidn
Copy link

braidn commented Mar 31, 2017

This is very true but, it's only a data thing (not really a problem). If a country requires states but has no states assigned to it, a text field will show. We really should be disciplined when we go Int to make sure places that we ship to, who are state based, have the correct states added to the country.

@papayaah
Copy link
Author

papayaah commented Mar 31, 2017

although... this is the billing address... so it's really tricky!

Copy link

@braidn braidn left a comment

Choose a reason for hiding this comment

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

Looks 💯 David. Do we know what 'state' was written in to cause us to need this fix?

@@ -42,6 +42,10 @@ def bill_to_hash
def normalize_sku(sku)
sku.chomp("-SET")
end

def state(address)
address.state ? address.state.name : address.state_name
Copy link

Choose a reason for hiding this comment

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

I believe I did this very change in another shipping implementation that utilizes XML! Eerily familiar. Anyway, looks 👍

@braidn
Copy link

braidn commented Mar 31, 2017

Ah yes, Billing 🇹🇭

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.

3 participants