Skip to content

Prepare for deferred confirm#252

Closed
elia wants to merge 9 commits into
masterfrom
elia+rainer/prepare-for-deferred-confirm
Closed

Prepare for deferred confirm#252
elia wants to merge 9 commits into
masterfrom
elia+rainer/prepare-for-deferred-confirm

Conversation

@elia

@elia elia commented Mar 28, 2023 •
edited by rainerdema
Loading

Copy link
Copy Markdown
Member

Summary

Preparation for #249, these are all commits that are preparatory but not strictly related to the deferred confirm.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

elia and others added 5 commits March 28, 2023 17:46
@elia @rainerdema
Autocreate the sandbox before starting bin/dev
8b83f87 
Co-Authored-By: Rainer Dema <rainer.dema@gmail.com>
@elia @rainerdema
Fix migrations updater timestamp and order
d22673a 
Co-Authored-By: Rainer Dema <rainer.dema@gmail.com>
@elia @rainerdema
Don't break the obfuscated card number line
a717e97 
Co-Authored-By: Rainer Dema <rainer.dema@gmail.com>
@elia @rainerdema
Rely on the payment method's auto capture settings
1b302eb 
Co-Authored-By: Rainer Dema <rainer.dema@gmail.com>
@elia @rainerdema
Silence rubocop on multiline ternary operators
0c1e36a 
Although no one should write a 10-line ternary, for simple cases is
more than ok.

Co-Authored-By: Rainer Dema <rainer.dema@gmail.com>
@elia
elia force-pushed the elia+rainer/prepare-for-deferred-confirm branch from cde5440 to 3680a7f Compare March 28, 2023 15:46
@rainerdema @elia
Use @stripe/stripe-js as a module with importmap
68b6ffa 
Co-Authored-By: Elia Schito <elia@schito.me>
Co-Authored-By: Rainer Dema <rainer.dema@gmail.com>
@elia
elia force-pushed the elia+rainer/prepare-for-deferred-confirm branch from 3680a7f to a628c5f Compare March 28, 2023 15:52
@rainerdema rainerdema mentioned this pull request Mar 28, 2023
Merged
3 tasks
elia and others added 3 commits March 28, 2023 18:20
@elia @rainerdema
Include Stripe custom partial payment info
d73896c 
This file is needed to properly display Stripe payment information in
confirm and thank-you pages.

Co-Authored-By: Rainer Dema <rainer.dema@gmail.com>
@elia @rainerdema
Ensure it's the right form when handling the payment submit
950e678 
Co-Authored-By: Rainer Dema <rainer.dema@gmail.com>
@elia @rainerdema
Don't try to update migrations on the CI
b401f6b 
CI will start from scratch every time.
@rainerdema
rainerdema force-pushed the elia+rainer/prepare-for-deferred-confirm branch from a628c5f to b401f6b Compare March 28, 2023 16:20
@codecov

codecov Bot commented Mar 28, 2023

Copy link
Copy Markdown

Codecov Report

Merging #252 (b401f6b) into master (bad6ed0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #252   +/-   ##
=======================================
  Coverage   99.79%   99.79%           
=======================================
  Files          24       25    +1     
  Lines         478      480    +2     
=======================================
+ Hits          477      479    +2     
  Misses          1        1
Impacted Files Coverage Δ
...lates/app/views/checkouts/payment/_stripe.html.erb 100.00% <ø> (ø)
app/models/solidus_stripe/payment_intent.rb 100.00% <100.00%> (ø)
...s/checkouts/existing_payment/stripe/_card.html.erb 100.00% <100.00%> (ø)
...tes/app/views/orders/payment_info/_stripe.html.erb 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@elia
elia marked this pull request as ready for review March 28, 2023 19:50
waiting-for-dev
waiting-for-dev approved these changes Mar 29, 2023
View reviewed changes

@waiting-for-dev waiting-for-dev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! Left two questions and a non-blocking suggestion 🙏

Comment thread app/models/solidus_stripe/payment_intent.rb
# The capture method should stay manual in order to
# avoid capturing the money before the order is completed.
capture_method: 'manual',
capture_method: payment_method.auto_capture? ? 'automatic' : 'manual',

@waiting-for-dev waiting-for-dev Mar 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it fix #178? If so, do we need to implement the unhappy path (see #178 (comment)). IMHO, if so, at least this one should deserve a separate PR for better reference in the future.

@kennyadsl kennyadsl Mar 29, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agree with @waiting-for-dev here. I'm fine doing this before, and even partially (updating the issue with the remaining things to do), but I'd like this not to be part of the preparatory PR.

@elia elia Mar 29, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was added because the comment didn't seem relevant anymore with the deferred confirm and so it made sense to connect the value to the auto_capture setting instead of just deleting the comment.
But open to postponing the change, it's not strictly required for the deferred capture PR.

Comment thread ...idus_stripe/install/templates/app/javascript/controllers/solidus_stripe_intent_controller.js
kennyadsl
kennyadsl reviewed Mar 29, 2023
View reviewed changes

@kennyadsl kennyadsl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left a couple of comments/questions before approval. Overall I'm ok with this but the majority of the changes are not related to the preparation.

As usual, I'd like to have multiple PRs (eg. for the sandbox stuff), but this is not blocking.

Comment thread app/models/solidus_stripe/payment_intent.rb
# The capture method should stay manual in order to
# avoid capturing the money before the order is completed.
capture_method: 'manual',
capture_method: payment_method.auto_capture? ? 'automatic' : 'manual',

@kennyadsl kennyadsl Mar 29, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agree with @waiting-for-dev here. I'm fine doing this before, and even partially (updating the issue with the remaining things to do), but I'd like this not to be part of the preparatory PR.

Comment thread lib/generators/solidus_stripe/install/install_generator.rb
if storefront_layout_path.exist?
insert_into_file(
storefront_layout_path.to_s,
"\n <script async src='https://js.stripe.com/v3/'></script>",

@kennyadsl kennyadsl Mar 29, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why this approach?

@elia elia Mar 29, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This comes straight outta importmaps-rails https://github.com/rails/importmap-rails/blob/main/lib/install/install.rb#L1-L9 and is in line with Stripe advising to load the JS on every page to better support fraud prevention. The stripe-js imported in .js files on the other hand is just a thin layer that will load this same file if it's not already available, so we can have the global script tag marked as async and at the same time use a comfy import in our JS files.

@elia
elia requested a review from kennyadsl March 29, 2023 08:12
@elia

elia commented Mar 29, 2023

Copy link
Copy Markdown
Member Author

This was split in #253 #254 #255 #256

@elia elia closed this Mar 29, 2023
@elia
elia deleted the elia+rainer/prepare-for-deferred-confirm branch March 29, 2023 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kennyadsl kennyadsl Awaiting requested review from kennyadsl
1 more reviewer
@waiting-for-dev waiting-for-dev waiting-for-dev approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@elia elia

@rainerdema rainerdema

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@elia @waiting-for-dev @kennyadsl @rainerdema