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

Handle checkout process for orders fully covered by store credits #348

Conversation

rainerdema
Copy link
Contributor

@rainerdema rainerdema commented Jun 29, 2023

Summary

Fixes #345

This PR adds logic to handle the checkout process for orders that are fully covered by store credits.
In the checkout process, we skip the payment selection stage if the order can be entirely covered by store credits.
A message is displayed to the user indicating that no additional payment method is required.

These changes ensure a smoother checkout experience for users who have sufficient store credits to cover their entire order.

solidus_starter_frontend.348.mov

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.

@rainerdema rainerdema self-assigned this Jun 29, 2023
@@ -3,60 +3,63 @@
<legend>
<%= t('spree.payment_information') %>
</legend>
<% if @order.covered_by_store_credit? %>
<p>Your order is fully covered by store credits, no additional payment method is required.</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This message needs to be associated with a translation key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

as pointed out by @kennyadsl in solidusio/solidus#5208 we can start storing frontend related keys in the starter itself, with time we should end up with a tighter, more coherent list of translations for the core instead of the giant ball of unused ones we currently have.

Copy link
Member

Choose a reason for hiding this comment

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

True. Still, I'd like to keep that change separate from this PR. This change will end up in the starter fronted 4.2 so I think the current approach of adding the translation key to the core is safe because Solidus Starter Frontend 4.2 will only be installed on Solidus 4.2.

I think we can go as Rainer suggested and open an issue for moving the translation keys from the core over here (I expect the usual backward compatibility pain 😅 ).

Copy link
Member

Choose a reason for hiding this comment

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

@kennyadsl I think there are ways in which it can be less painful, e.g.:

  • move all the deprecated translations in core to a separate locale files (e.g. solidus/core/config/locales/en.deprecated.yml)
  • copy the file to the host application as part of the solidus installation/upgrade process
  • remove the deprecated file in the next major

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the dedicated issue for the mentioned changes: Migrate frontend translations from solidus_core to solidus_starter_frontend #5212

A huge thanks to everyone ❤️

@rainerdema rainerdema force-pushed the nebulab/rainerd/add-message-if-order-is-fully-covered-by-store-credits branch 2 times, most recently from b488b34 to f191fbe Compare June 30, 2023 09:56
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #348 (ecdc45f) into main (dc028e7) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #348      +/-   ##
==========================================
+ Coverage   97.12%   97.26%   +0.13%     
==========================================
  Files         190      190              
  Lines        4491     4498       +7     
==========================================
+ Hits         4362     4375      +13     
+ Misses        129      123       -6     
Impacted Files Coverage Δ
templates/app/controllers/checkouts_controller.rb 96.93% <100.00%> (+0.06%) ⬆️
...s/app/views/checkouts/steps/_payment_step.html.erb 100.00% <100.00%> (ø)
templates/spec/system/checkout_spec.rb 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

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

@rainerdema rainerdema force-pushed the nebulab/rainerd/add-message-if-order-is-fully-covered-by-store-credits branch from f191fbe to 8e5cfaa Compare July 3, 2023 07:40
rainerdema added a commit to nebulab/solidus that referenced this pull request Jul 3, 2023
This commit adds a new translation key 'covered_by_store_credit' in en.yml.
The new key provides a specific message to inform users when their order is fully
covered by store credits, and no additional payment method is required.
This change is connected to an update made in Solidus Starter Frontend:
solidusio/solidus_starter_frontend#348
@rainerdema rainerdema marked this pull request as ready for review July 3, 2023 07:49
This commit adds logic to handle the checkout process for orders
that are fully covered by store credits. 
In the checkout process, we skip the payment selection stage if
the order can be entirely covered by store credits.
A message is displayed to the user indicating that no additional
payment method is required.
@rainerdema rainerdema force-pushed the nebulab/rainerd/add-message-if-order-is-fully-covered-by-store-credits branch from 8e5cfaa to ecdc45f Compare July 3, 2023 07:50
@@ -3,60 +3,63 @@
<legend>
<%= t('spree.payment_information') %>
</legend>
<% if @order.covered_by_store_credit? %>
<p>Your order is fully covered by store credits, no additional payment method is required.</p>
Copy link
Member

Choose a reason for hiding this comment

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

as pointed out by @kennyadsl in solidusio/solidus#5208 we can start storing frontend related keys in the starter itself, with time we should end up with a tighter, more coherent list of translations for the core instead of the giant ball of unused ones we currently have.

@elia
Copy link
Member

elia commented Jul 3, 2023

@rainerdema I asked for moving the translation to here, but also wanted to point out that it's great to see this fixed 🙌

Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

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

After an internal discussion the hardcoded string is fine, and the starter specific locale file will be handled in a different issue/PR

👍👍👍

@@ -3,60 +3,63 @@
<legend>
<%= t('spree.payment_information') %>
</legend>
<% if @order.covered_by_store_credit? %>
<p>Your order is fully covered by store credits, no additional payment method is required.</p>
Copy link
Member

Choose a reason for hiding this comment

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

@kennyadsl I think there are ways in which it can be less painful, e.g.:

  • move all the deprecated translations in core to a separate locale files (e.g. solidus/core/config/locales/en.deprecated.yml)
  • copy the file to the host application as part of the solidus installation/upgrade process
  • remove the deprecated file in the next major

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

👍

@kennyadsl kennyadsl merged commit df9a4a8 into main Jul 3, 2023
@kennyadsl kennyadsl deleted the nebulab/rainerd/add-message-if-order-is-fully-covered-by-store-credits branch July 3, 2023 10:44
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.

Preventing available payment method rendering for orders fully covered by Store Credits
3 participants