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

Don't try and add billing address if there is no billing address attached to credit card #120

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

joeljackson
Copy link
Contributor

@joeljackson joeljackson commented Aug 30, 2017

No description provided.

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.

Please add a test and fix the typo. Otherwise I'm fine with that change

@@ -266,7 +266,7 @@ def transaction_options(source, options, submit_for_settlement = false)
params[:shipping] = braintree_shipping_address(options)
end

if source.credit_card?
if source.credit_card? && options[:bliing_address].present?
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to have a test for this.

Then you actually would have found your typo as well ;)

@tvdeyen
Copy link
Member

tvdeyen commented Oct 20, 2017

@joeljackson I still like this change. Are you able to provide a spec and rebase with latest master to get rid of the merge conflicts and please remove the commits from #123

Then we are fine. Thanks for your contribution.

@joeljackson
Copy link
Contributor Author

Hi @tvdeyen, still interested in this change?

If so would be interested in adding the spec and rebasing.

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.

Yes, thank you. Could you please add some tests?

@@ -306,11 +306,11 @@ def transaction_options(source, options, submit_for_settlement = false)
params[:payment_method_nonce] = source.nonce
end

if source.paypal?
if source.paypal? && options[:billing_address].present?
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be

options[:shipping_address].present?

<div><%= wallet_payment_source.payment_source.card_type %> - <%= wallet_payment_source.payment_source.display_number %></div>
<div><%= wallet_payment_source.payment_source.expiration_month %>/<%= wallet_payment_source.payment_source.expiration_year %></div>
<%- elsif wallet_payment_source.payment_source.paypal? %>
<div>Paypal</div>
Copy link
Member

Choose a reason for hiding this comment

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

Should be PayPal (Pal is also capitalized)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, Shoot. Didn't mean to include this here.

Will pull it out. See other pull request to see if we want to pull that in.

Thanks!

@stale
Copy link

stale bot commented Nov 11, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 11, 2022
@elia elia changed the title Don't try and add billing address if there is no billing address atta… Don't try and add billing address if there is no billing address attached to credit card Dec 6, 2022
@elia elia added Stale and removed wontfix labels Dec 6, 2022
@stale stale bot removed the Stale label Dec 6, 2022
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.

3 participants