Set source payment_type in cc hosted fields #79

tvdeyen · 2017-05-03T21:53:05Z

The hosted fields does not set the payment_type value on credit card
sources. This leads to sources which type can’t be recognized anymore.

tvdeyen · 2017-05-03T22:09:01Z

Dang. The Travis push hook gets triggered because I accidentally pushed my branch to this repo instead of my fork. I deleted the branch while the build was running. This is why I failed. But the PR hook triggered the correct build of my Fork and this passes 😅

tvdeyen · 2017-05-04T07:49:42Z

Closing and reopen, to get correct Travis result

tvdeyen · 2017-05-04T07:54:19Z

It turns out that Travis seem to have mayor caching issues. Sorry about that. Please ignore the continuous-integration/travis-ci/push result.

Senjai

To my knowledge, the "card_type" and friends should be delegated to Braintree, via the delegate call there. As for determining what type of payment the source represents, the source already includes several query methods for that, are those not working in this scenario?

Senjai · 2017-05-05T05:50:42Z

spec/features/braintree_credit_card_checkout_spec.rb

      expect(page).to have_content("Your order has been processed successfully")
    end
+
+    it "sets the payment type of source to CreditCard" do
+      expect(SolidusPaypalBraintree::Source.last.payment_type).to eq('CreditCard')


This assertion does not belong in feature spec. Could we do this at the unit test level? We should probably also avoid using last unless we're sure the system is only creating the one record.

We should probably also avoid using last unless we're sure the system is only creating the one record.

We know exactly that in this scenario the last source is the one we created through the controller. I had a version where I explicitly tested that we only have on source here, but this seemed overkill for me. I can add this if you want to be extra save.

Feature tests should only ever test user interactions. Since there is no reliable way to ensure that the user can see that this is the case, it should not be included in a feature test. If there was a way for a user to use the site in a manner that would cause a failure, we could test that here. Otherwise this should be tested in a controller/request test.

Senjai · 2017-05-05T05:53:18Z

lib/solidus_paypal_braintree/engine.rb

@@ -10,7 +10,7 @@ class Engine < Rails::Engine

    initializer "register_solidus_paypal_braintree_gateway", after: "spree.register.payment_methods" do |app|
      app.config.spree.payment_methods << SolidusPaypalBraintree::Gateway
-      Spree::PermittedAttributes.source_attributes << :nonce
+      Spree::PermittedAttributes.source_attributes.concat [:nonce, :payment_type]


This value isn't whitelisted and can be modified by users. Further if we know the payment type in advance, we should probably avoid requiring it to be set in the form.

This value isn't whitelisted and can be modified by users.

I need to whitelist it, otherwise I can not pass it into the controller from the view where the value needs top be set, as we don't know the type upfront.

Further if we know the payment type in advance, we should probably avoid requiring it to be set in the form.

We don't know the type upfront. I didn't find any code that guarantees this. Maybe you can point me to?

I need to whitelist it, otherwise I can not pass it into the controller from the view where the value needs top be set, as we don't know the type upfront.

I meant that the value isn't whitelisted. I can insert "lol" into this field and it would save correctly. We should probably add an inclusion validation to the model to prevent this.

We don't know the type upfront. I didn't find any code that guarantees this. Maybe you can point me to?

Once we have the token, payment_method.braintree.payment_method.find(source.token).is_a?(Braintree::PaypalAccount)

As we only use this for display purposes, I no longer think that's worth doing though.

tvdeyen · 2017-05-05T07:03:18Z

To my knowledge, the "card_type" and friends should be delegated to Braintree, via the delegate call there. As for determining what type of payment the source represents, the source already includes several query methods for that, are those not working in this scenario?

The payment_type is a custom database column from our side, so Braintree does not know about it.

For PayPal and ApplePay sources we set these value explicitly, but not for credit card payments. My pending spec e4e4589 shows that this is currently not working.

This is why I chose to test this integrative in stead of on the unit level. The model is polymorphic (not in the AR kind of things, but I plays multiple roles). We can't know the value upfront, so testing it on the unit level did not really made sense for me, we need to set this value while creating it through the controller. Therefore passing it along the params from the view - where we exactly know what type the source is - made the most sense for me.

Any idea how to approach this differently?

tvdeyen · 2017-05-05T07:11:23Z

the source already includes several query methods for that

The query methods depend on the payment_type to be set. friendly_payment_type even blows up if no payment_type is set.

tvdeyen · 2017-05-05T10:07:55Z

The "best solution" would be to actually provide three payment sources (and even gateways). Then we wouldn't even need to have these query methods at all.

I will provide a RFC for this, as I think now that we have a common interface for payment sources in Solidus 2.2 we should start to embrace it. For older stores its an easy back port we would ship with this gem.

Senjai · 2017-05-05T18:18:52Z

lib/solidus_paypal_braintree/engine.rb

@@ -10,7 +10,7 @@ class Engine < Rails::Engine

    initializer "register_solidus_paypal_braintree_gateway", after: "spree.register.payment_methods" do |app|
      app.config.spree.payment_methods << SolidusPaypalBraintree::Gateway
-      Spree::PermittedAttributes.source_attributes << :nonce
+      Spree::PermittedAttributes.source_attributes.concat [:nonce, :payment_type]


I need to whitelist it, otherwise I can not pass it into the controller from the view where the value needs top be set, as we don't know the type upfront.

I meant that the value isn't whitelisted. I can insert "lol" into this field and it would save correctly. We should probably add an inclusion validation to the model to prevent this.

We don't know the type upfront. I didn't find any code that guarantees this. Maybe you can point me to?

Once we have the token, payment_method.braintree.payment_method.find(source.token).is_a?(Braintree::PaypalAccount)

As we only use this for display purposes, I no longer think that's worth doing though.

Senjai · 2017-05-05T18:22:15Z

spec/features/braintree_credit_card_checkout_spec.rb

      expect(page).to have_content("Your order has been processed successfully")
    end
+
+    it "sets the payment type of source to CreditCard" do
+      expect(SolidusPaypalBraintree::Source.last.payment_type).to eq('CreditCard')


Feature tests should only ever test user interactions. Since there is no reliable way to ensure that the user can see that this is the case, it should not be included in a feature test. If there was a way for a user to use the site in a manner that would cause a failure, we could test that here. Otherwise this should be tested in a controller/request test.

Senjai · 2017-05-05T18:27:14Z

RE: the query methods, yes you're correct. Missed that, I should have looked at that more closely. I'm 👍 on including on the form, would just prefer the test moved.

As for the RFC can you hold off? We've had a couple discussions on that architecture and have the beginnings of a plan in place. If you're interested we can chat about it, but I don't want to duplicate efforts here.

tvdeyen · 2017-05-08T07:51:09Z

Build fixes waits for #84

Senjai · 2017-05-08T20:25:59Z

db/migrate/20170508085402_add_not_null_constraint_to_sources_payment_type.rb

+    reversible do |dir|
+      dir.up do
+        SolidusPaypalBraintree::Source.where(payment_type: nil).
+          update_all(payment_type: 'CreditCard')


This isn't accurate. Is there a scenario that generated nil payment_types in the past?

Yes. All credit card payments. PayPal and ApplePay payments have this value set via JS tailored params.

Although this isn't accurate it's the only thing we can do about, besides not doing this at all of course.

Sure, fair point.

Senjai · 2017-05-08T20:26:25Z

app/models/solidus_paypal_braintree/source.rb

@@ -10,6 +10,8 @@ class Source < ApplicationRecord

    belongs_to :customer, class_name: "SolidusPaypalBraintree::Customer"

+    validates :payment_type, presence: true


validates :payment_type, inclusion: [PAYPAL, APPLE_PAY, CREDIT_CARD]

Very good point! 👌🏻

When submiting credit cards in checkout the payment_type is missing. We should validates it’s presence on the source model.

Our javascript takes care of setting this column in the params during checkout for PayPal and ApplePay payments, but not for credit card payments. The easiest way is to send the value is from within the hosted fields form.

The payment_type is used to differ payment soruces. I.E. lots of query methods uses this column to decide which kind of payment this source instance is, even presentor methods uss this clumn to look up the human name in the translation files by this column. Therefore it is necessary to validate its presence and add a not null constrained in the database.

Senjai · 2017-05-16T15:29:27Z

@tvdeyen Have some vcr issues no idea why though :/

tvdeyen · 2017-05-16T17:00:10Z

@Senjai yeah. Pretty strange. Only for Solidus < 2.1. Even locally. Investigated this morning, but gave up because lack of time.

Senjai · 2017-05-18T02:23:10Z

Closed in favor of #91 thanks thomas.

tvdeyen closed this May 4, 2017

tvdeyen reopened this May 4, 2017

tvdeyen closed this May 4, 2017

tvdeyen reopened this May 4, 2017

Senjai suggested changes May 5, 2017

View reviewed changes

tvdeyen force-pushed the set-payment-type branch from 8af76e2 to 3d37e78 Compare May 8, 2017 07:24

Senjai reviewed May 8, 2017

View reviewed changes

Senjai suggested changes May 8, 2017

View reviewed changes

tvdeyen added 3 commits May 15, 2017 20:33

Pending spec for setting credit card payment_type

3618d86

When submiting credit cards in checkout the payment_type is missing. We should validates it’s presence on the source model.

Set source payment_type in cc hosted fields

fcd37c8

Our javascript takes care of setting this column in the params during checkout for PayPal and ApplePay payments, but not for credit card payments. The easiest way is to send the value is from within the hosted fields form.

tvdeyen force-pushed the set-payment-type branch from 3d37e78 to c6f5ec7 Compare May 15, 2017 19:32

Senjai approved these changes May 16, 2017

View reviewed changes

Senjai mentioned this pull request May 17, 2017

Set payment type for credit cards #91

Merged

Senjai closed this May 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set source payment_type in cc hosted fields #79

Set source payment_type in cc hosted fields #79

tvdeyen commented May 3, 2017

tvdeyen commented May 3, 2017

tvdeyen commented May 4, 2017

tvdeyen commented May 4, 2017

Senjai left a comment

Senjai May 5, 2017

tvdeyen May 5, 2017

Senjai May 5, 2017

Senjai May 5, 2017

tvdeyen May 5, 2017

Senjai May 5, 2017

tvdeyen commented May 5, 2017

tvdeyen commented May 5, 2017

tvdeyen commented May 5, 2017

Senjai May 5, 2017

Senjai May 5, 2017

Senjai commented May 5, 2017

tvdeyen commented May 8, 2017

Senjai May 8, 2017

tvdeyen May 8, 2017

Senjai May 8, 2017

Senjai May 8, 2017

tvdeyen May 8, 2017

Senjai commented May 16, 2017

tvdeyen commented May 16, 2017

Senjai commented May 18, 2017

		@@ -10,6 +10,8 @@ class Source < ApplicationRecord

		belongs_to :customer, class_name: "SolidusPaypalBraintree::Customer"

		validates :payment_type, presence: true

Set source payment_type in cc hosted fields #79

Set source payment_type in cc hosted fields #79

Conversation

tvdeyen commented May 3, 2017

tvdeyen commented May 3, 2017

tvdeyen commented May 4, 2017

tvdeyen commented May 4, 2017

Senjai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen commented May 5, 2017

tvdeyen commented May 5, 2017

tvdeyen commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Senjai commented May 5, 2017

tvdeyen commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Senjai commented May 16, 2017

tvdeyen commented May 16, 2017

Senjai commented May 18, 2017