Fixed Updating customer billing address now correctly reflected to stripe #3696

nidhi-soni1104 · 2024-02-20T08:21:10Z

What this PR does / why we need it:

Changing customer billing address, in the developer portal, is updated to Stripe only when credit card details are changed. Just editing and saving billing address details creates problems, when customer details are not consistent between 3scale and Stripe. This can be forcefully fixed, when the customer updates their credit card information as well.

Expected behavior:

Editing just billing address details should update customer details on Stripe.

Which issue(s) this PR fixes
https://issues.redhat.com/browse/THREESCALE-9532

Verification steps
Mentioned in jira with snapshots

…e Stripe

mayorova · 2024-03-19T15:55:11Z

app/lib/payment_gateways/stripe_crypt.rb

+      rescue Stripe::StripeError => stripe_error
+        handle_stripe_error(stripe_error)
+      ensure
+        reset_stripe_api_key


@nidhi-soni1104 Can you explain, why do we need to set and unset the Stripe key this way?... Can't we just keep passing it to #retrieve as a second argument?

I did this because when I tried updating on stripe passing Stripe key as second argument, it didn't get updated and then I tried it n number of times with other different ways but only this method worked for me .

mayorova · 2024-03-20T12:48:02Z

app/lib/payment_gateways/stripe_crypt.rb

+        Stripe.api_key = api_key  # Set actual Stripe secret key
+        latest_payment_method_id = latest_payment_method_id_for_customer
+        return true unless latest_payment_method_id.present?
+        payment_method = Stripe::PaymentMethod.retrieve(payment_method_id)


I'm getting an error here when trying to update the address:

ummmm I have not received this error but let me try to reproduce it.

@mayorova Yes due to code refactor id name was passed wrong

I have fixed it and now it works well

mayorova · 2024-04-10T11:53:23Z

app/lib/payment_gateways/payment_gateway_crypt.rb

@@ -41,6 +41,19 @@ def notify_exception(e, query_string = nil)
      Rails.logger.error(msg)
    end

+    def update_payment_detail(card, payment_method_id, payment_method)


This method, as well as handle_stripe_error cannot be here, because they are Stripe-specific, and also payment_detail is only defined in StripeCrypt, so this code needs to be moved there.

mayorova · 2024-04-10T12:01:52Z

app/lib/payment_gateways/stripe_crypt.rb

-      payment_detail.credit_card_auth_code      = payment_method.customer
-      payment_detail.payment_method_id          = payment_method_id
-      payment_detail.save
+      update_payment_detail(card, payment_method_id, payment_method)


Seems that card belongs to payment_method, so we don't need to path both to the method. You can pass just (payment_method, payment_method_id), and move the card = payment_method.card line within the method.

mayorova · 2024-04-10T12:23:01Z

app/lib/payment_gateways/stripe_crypt.rb

+      payment_method.save
+    end
+
+    def retrieve_customer(customer_id)


What's the purpose of this refactor?... 🤔

I'd probably keep it as it is, the method is not too long, and splitting it I think makes it less readable.

mayorova · 2024-04-10T12:43:47Z

...per_portal/app/controllers/developer_portal/admin/account/payment_details_base_controller.rb

@@ -28,7 +28,7 @@ def edit
  def update
    current_account.updating_payment_detail = true
    if current_account.update account_params
-      redirect_to payment_details_path, notice: 'Your billing address was successfully stored'
+      update_billing_address_on_stripe(account_params)


This line is a bit concerning, because this controller is generic, and not Stripe-specific.
I've been looking in to this, and it seems that we currently support just Stripe and Braintree, and Braintree doesn't use this path, so it's kind of fine (in the sense that it doesn't break the Braintree flow). Also, it seems that we can't move this to the Stripe controller, because we don't want to break the existing portals.

But I think we do need to at least check here whether the current account's payment gateway is actually stripe, before trying to update anything.

Hm... probably we can actually change this (e.g. by moving the method to the Stripe Controller) without breaking the portals, because this path is not in the template, but in the Liquid drop:

porta/lib/developer_portal/lib/liquid/forms/account.rb

Line 11 in 0155b4a

admin_account_payment_details_path

But we need to think about whether that's the right thing to do.

mayorova · 2024-04-10T13:05:40Z

app/lib/payment_gateways/stripe_crypt.rb

@@ -36,21 +32,41 @@ def customer
      @customer ||= find_or_create_customer
    end

+    def update_billing_address(billing_address)


I think it's quite confusing to have update_billing_address, update_billing_details and update_and_save_billing_details methods, as you need to jump between them to understand what is done there.

At least the last two need to be merged, IMO.

mayorova · 2024-04-10T13:16:46Z

...per_portal/app/controllers/developer_portal/admin/account/payment_details_base_controller.rb

@@ -28,7 +28,7 @@ def edit
  def update
    current_account.updating_payment_detail = true
    if current_account.update account_params
-      redirect_to payment_details_path, notice: 'Your billing address was successfully stored'
+      update_billing_address_on_stripe(account_params)


Also, this is a bit hard to follow - I'd avoid having a redirect, and a duplicated error-rendering functionality in a method.
Instead, I would do something like this:

if update_address_on_gateway && current_account.update account_params redirect_to payment_details_path, notice: 'Your billing address was successfully stored' else flash[:notice] ... end

And then (in pseudocode):

def update_address_on_gateway return true if <payment gateway is not Stripe> <the code updating the address in Stripe, returning true or false> end

But probably still better to move this whole method to the Stripe controller.

mayorova · 2024-04-10T13:27:26Z

app/lib/payment_gateways/stripe_crypt.rb

+      latest_payment_method = Stripe::PaymentMethod.list(
+        customer: customer.id,
+        type: 'card',
+        limit: 1
+      ).data.first&.id
+    end


I don't think this is a reliable method to get the right payment mehtod 🤔
Are we sure about the order of the list? Maybe the first one added will be returned here, and not the one that was added last?

What we actually care about is the payment method which is identified by payment_method_id that we store in our database. So, I think we need to retrieve that one (I guess it would be something like Stripe::PaymentMethod.retrieve(current_account.payment_detail.payment_method_id, api_key)), and update it instead.

mayorova · 2024-04-10T13:33:43Z

app/lib/payment_gateways/stripe_crypt.rb

+        payment_method = Stripe::PaymentMethod.retrieve(latest_payment_method_id)
+        update_and_save_billing_details(payment_method, billing_address)
+      rescue Stripe::StripeError => stripe_error
+        handle_stripe_error(stripe_error)


Can't you just report_error("Failed to update billing address on Stripe: #{stripe_error.message}") here? I think the handle_stripe_error is not needed at all.

github-actions · 2024-05-11T03:34:04Z

This PR is stale because it has not received activity for more than 30 days. Remove stale label or comment or this will be closed in 15 days.

nidhi-soni1104 self-assigned this Feb 20, 2024

nidhi-soni1104 marked this pull request as draft February 20, 2024 08:21

nidhi-soni1104 marked this pull request as ready for review March 4, 2024 13:54

nidhi-soni1104 force-pushed the THREESCALE-9532-fix-stripe-billing-adress-update branch from 4173d32 to be59a8d Compare March 4, 2024 13:55

nidhi-soni1104 added 11 commits March 11, 2024 14:14

Fixed Updating customer billing address now correctly reflected to th…

8266e0a

…e Stripe

fixed code climate issues and failed test

1516ee8

fixed climate code

a8437d7

fixed failing cucumber

f06de18

fixed first time adding billing address

eaa1497

fixed cuccumber faliures

d55d068

fixed code climate issues

f4ae3fd

refactored code

3576fff

added missing arguments

88deb27

fixed failed uni test

52c1c5d

fixed code climate

5024303

nidhi-soni1104 force-pushed the THREESCALE-9532-fix-stripe-billing-adress-update branch from a8ee671 to 5024303 Compare March 11, 2024 08:45

nidhi-soni1104 added 4 commits March 11, 2024 14:22

keep method in its own file

3bd8de4

code refactored

84783a6

refactored code

d6b0603

updated stripe error

be03bcd

nidhi-soni1104 requested a review from josemigallas March 19, 2024 08:07

mayorova reviewed Mar 19, 2024

View reviewed changes

mayorova reviewed Mar 20, 2024

View reviewed changes

fixed id faliure error

5f32a22

mayorova reviewed Apr 10, 2024

View reviewed changes

github-actions bot added the Stale label May 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Updating customer billing address now correctly reflected to stripe #3696

Fixed Updating customer billing address now correctly reflected to stripe #3696

nidhi-soni1104 commented Feb 20, 2024 •

edited

mayorova Mar 19, 2024

nidhi-soni1104 Apr 3, 2024

mayorova Mar 20, 2024

nidhi-soni1104 Apr 3, 2024

nidhi-soni1104 Apr 8, 2024

mayorova Apr 10, 2024 •

edited

mayorova Apr 10, 2024

mayorova Apr 10, 2024

mayorova Apr 10, 2024

mayorova Apr 10, 2024

mayorova Apr 10, 2024

mayorova Apr 10, 2024 •

edited

mayorova Apr 10, 2024

mayorova Apr 10, 2024

github-actions bot commented May 11, 2024

Fixed Updating customer billing address now correctly reflected to stripe #3696

Are you sure you want to change the base?

Fixed Updating customer billing address now correctly reflected to stripe #3696

Conversation

nidhi-soni1104 commented Feb 20, 2024 • edited

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

mayorova Apr 10, 2024 • edited

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

mayorova Apr 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 11, 2024

nidhi-soni1104 commented Feb 20, 2024 •

edited

mayorova Apr 10, 2024 •

edited

mayorova Apr 10, 2024 •

edited