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

Move checkout coupon code section into summary #2327

Merged
merged 7 commits into from
May 25, 2018

Conversation

kennyadsl
Copy link
Member

@kennyadsl kennyadsl commented Oct 25, 2017

This change is needed if we want to stop passing from checkout controller to apply coupon codes.

At the moment we have several issues with this behavior:

  • apply_coupon_code is defined into frontend store_controller. This potentially allows all controllers' actions to apply coupon code, just passing [:order][:coupon_code] along with other params. We don't want this anymore since those actions are designed to work for orders and checkout controllers only. See render :edit for example, it could not work in all scenarios. To fix this issue I duplicated this method in both orders and checkout controllers, I think in this case duplication is not bad, since real shared logic is handled by PromotionHandler::Coupon.new(@order).apply and we'll need a different behavior in those controllers;
  • Payment step, coupon code ambiguous behaviour #2155 : Checkout controller needs extra ivars setup before rendering, after coupon code is applied with errors. This is now fixed since we can add this setup into the checkout-specific code that applies coupon code.
  • Coupon Code application UX is "strange". We have the coupon code field and submit button into the payment form. If we add an invalid code and submit the payment form with payment data, we'll lost all payment data we just added. If we add a valid coupon code and submit with ajaxy Apply Button behavior page will reload and we'll lost all payment data we just added. I think we need to make clear that those forms are separated so user would not fill both of them uselessy. To improve UX I moved the coupon code form into the summary box. Also, I made the coupon code a real form, even if it is always submitted with JS, both for semantic and to have a real working fallback if for some reason JS is not working.

TODO: in my plans next steps will be:

  • Deprecate accepting [:order][:coupon_code] param in checkout controller. If this PR is merged we'll just have this behavior called as a fallback if for some reason JS is not working.
  • Investigate if we can/makes sense to move the coupon code into a separate form also for the cart. I feel like it's not that terrible to have it in a single form there in terms on UX.
  • If we want to deprecate, we'll need another fallback endpoint, so I was thinking about creating a separate controller that will only handle coupon code application, maybe also for the api.

@mtomov
Copy link
Contributor

mtomov commented Oct 30, 2017

That's all great! And all in for all the 3 next steps

A current problem I'm always ending up solving with the coupon code not being a separate form is to detect when the enter key has been pressed, and submit just it .. as otherwise an Enter key would submit the payment form.

  $('#coupon_code').keypress (e) ->
    if(e.which == 13)
      Spree.onCouponCodeApply()
      e.preventDefault()

Separate controller and action is definitely the way to go!

Ah, and one more - I'm also often needing to make a coupon destroy logic. That would also fit well under the new controller. And yeah .. if anyone is up for writing the code for it - all the better for me : )

Thanks again

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

This makes lot of sense. Could you please rebase with latest master to see if this still works with Rails 5.1 and Rails 5.2 (See my comment about the insecure params usage).

frontend/app/controllers/spree/checkout_controller.rb Outdated Show resolved Hide resolved
frontend/app/controllers/spree/orders_controller.rb Outdated Show resolved Hide resolved
@kennyadsl
Copy link
Member Author

@tvdeyen sure, I'll fix and rebase after we merge #2690 so we can test it on the CI with both versions.

@kennyadsl kennyadsl force-pushed the coupon-code-into-summary branch from ca6fab3 to c1e37e8 Compare May 4, 2018 08:05
kennyadsl added 7 commits May 11, 2018 11:07
It will be easier to add other context, like checking out with
logged in user.
Both in checkout and orders controller.

This feature was not tested at controller level. Adding specs
will allow more confidence for future changes on this part.
There's no need to try to apply coupon code when its params is
empty.

Right now, submitting every form that have the coupon code field
(cart and checkout payment), even with the field empty, it is uselessy
calling the Spree::PromotionHandler::Coupon `new` and `apply`.
This method is used into Checkout and Order controllers, but they
need some different behavior which now would be hard to add in a
non-hacky way.
When coupon code handler has errors checkout controller
needs an extra setup for current step views or it will
not render correctly.
This commit moves the coupon code application section of checkout into the
summary box, still visible in payment step only.

It also wraps the coupon code input + button into a real form so it still
works if, for some reason, JS is not working, falling back to submitting
a simple order form for now.
@kennyadsl kennyadsl force-pushed the coupon-code-into-summary branch from c1e37e8 to 212287a Compare May 11, 2018 09:08
@kennyadsl
Copy link
Member Author

@tvdeyen changes requested done!

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Thanks

@kennyadsl kennyadsl merged commit 4857815 into solidusio:master May 25, 2018
@kennyadsl kennyadsl deleted the coupon-code-into-summary branch May 25, 2018 12:18
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.

5 participants