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

Stripe PI: add the request_extended_authorization field #5417

Merged
merged 1 commit into from
Mar 7, 2025

Conversation

yunnydang
Copy link
Contributor

adds the optional request_extended_authorization field for authorization requests

local:
6206 tests, 81265 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Unit:
70 tests, 367 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Remote:
101 tests, 451 assertions, 3 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
97.0297% passed

A note about tests: I had tried to write a remote test for this and it fails due to account related issues. Here is the error message: "This account is not eligible for the requested card features. See https://stripe.com/docs/payments/flexible-payments for more details."

Given the error message, I do think stripe is acknowledging that this field is valid, but its just that our test account is not set up for it. I tried poking around in the dashboard but havent made progress.

@yunnydang yunnydang requested a review from a team February 26, 2025 23:55
@yunnydang yunnydang self-assigned this Feb 26, 2025
Copy link
Contributor

@jcreiff jcreiff left a comment

Choose a reason for hiding this comment

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

@yunnydang I think it might be worth reaching out to Stripe Support to see if the feature can be enabled for our test account. The response you're getting suggests that the way you've updated the request is correct, but I also notice that the docs suggest you have to expand['latest_charge'] in order to get back the details about whether the extension is enabled, and the capture_before timestamp. If we plan to return those details in the response I think we'll want to see if expanding the latest charge also needs to be added to this new submethod, and if that affects anything about the way we are interpreting the response

@yunnydang
Copy link
Contributor Author

yunnydang commented Feb 27, 2025

docs suggest you have to expand['latest_charge']

Stripe support had confirmed its a paid IC+ account feature so we cant really turn it on for testing. As for the expanding the latest charge fiasco, they did confirm that we do need to expand it to see the response for extend_authorization. Now looking at the code for latest_charge is on of the candidates in the GSRF method:

def extract_charge_object(params)
return unless params

# there's 6 potential places where we could find it.
candidate1 = params.dig('charges', 'data').try(:[], 0)
candidate2 = params.dig('data', 'object', 'charges', 'data').try(:[], 0)
candidate3 = params.dig('refunds', 'data').try(:[], 0)
candidate4 = params.dig('error', 'payment_intent', 'charges', 'data').try(:[], 0)
candidate5 = params.dig('error', 'payment_intent', 'latest_charge')
candidate6 = params.dig('latest_charge')

candidate1 || candidate2 || candidate3 || candidate4 || candidate5 || candidate6

end

So i'd be wary of running an expand on it from the get go. Im going to do some more digging on the latest_charge object

@yunnydang
Copy link
Contributor Author

@yunnydang I think it might be worth reaching out to Stripe Support to see if the feature can be enabled for our test account. The response you're getting suggests that the way you've updated the request is correct, but I also notice that the docs suggest you have to expand['latest_charge'] in order to get back the details about whether the extension is enabled, and the capture_before timestamp. If we plan to return those details in the response I think we'll want to see if expanding the latest charge also needs to be added to this new submethod, and if that affects anything about the way we are interpreting the response

Also i think given that we probably would want to expand the latest_charge object eventually, i think creating a separate commit/ticket for so that it can be de-coupled would make some more sense given the risky nature of the change. What do you think?

@jcreiff
Copy link
Contributor

jcreiff commented Mar 3, 2025

If the expand is confined to the add_request_extended_authorization method you added, then I think it should be safe to include it with this change; if remote/unit testing look fine here with that addition, we can determine with some core end-to-end testing whether there are any issues with the response parsing on our side before calling it done

@yunnydang
Copy link
Contributor Author

If the expand is confined to the add_request_extended_authorization method you added, then I think it should be safe to include it with this change; if remote/unit testing look fine here with that addition, we can determine with some core end-to-end testing whether there are any issues with the response parsing on our side before calling it done

Hmm that would make sense! Ill see what this would affect!

@yunnydang yunnydang force-pushed the stripe_pi_add_extended_authorization_field branch from 9d03eac to 1fe314f Compare March 6, 2025 21:31
@yunnydang yunnydang requested a review from jcreiff March 6, 2025 21:33
@yunnydang yunnydang force-pushed the stripe_pi_add_extended_authorization_field branch from 1fe314f to f503c58 Compare March 6, 2025 21:35
@yunnydang
Copy link
Contributor Author

Heya @jcreiff so now that we're unblocked on the remote testing and seeing a response, i can confirm that the response does return the "capture_before" and "extended_authorization" status. I dont believe we will need to send and expand request for this

Copy link
Contributor

@jcreiff jcreiff left a comment

Choose a reason for hiding this comment

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

Looks good!

@yunnydang yunnydang force-pushed the stripe_pi_add_extended_authorization_field branch from f503c58 to 60c1543 Compare March 7, 2025 16:57
@yunnydang yunnydang merged commit 60c1543 into master Mar 7, 2025
5 checks passed
@yunnydang yunnydang deleted the stripe_pi_add_extended_authorization_field branch March 7, 2025 16:58
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.

2 participants