Disable codes on apply automatically promo #3502
Conversation
There was a problem hiding this comment.
Thanks! I left some notes about things that should be improved and also, can we add a spec to test that auto applying a promotion works when there's a promotion with both codes and auto_apply_promotion set? I suspect it should work but I don't think it's currently covered.
backend/app/views/spree/admin/promotions/_activations_new.html.erb
Outdated
Show resolved
Hide resolved
| return unless apply_automatically | ||
|
|
||
| errors.add(:apply_automatically, :disallowed_with_code) if codes.any? |
There was a problem hiding this comment.
Can you please expand a little bit more in its commit description? I'm not sure to get why this is needed.
There was a problem hiding this comment.
I've added a short message to explain why this commit is needed, let me know if is clear enough
There was a problem hiding this comment.
@kennyadsl did you read the new commit message? Let me know what do you think
There was a problem hiding this comment.
Thanks for updating it, I think the second part can be rewritten as:
Also, this improves backward compatibility since if we keep validating
against promotions with codes as well, it will be impossible to update
existing apply-auto promotions that already have a code associated.
While in the first part I still don't get this sentence:
Since promotions creation is a two-step process and promo codes
are added in the second step, the validation have to be there.
Why do you say that the validation has to be there if we are removing it in this commit?
There was a problem hiding this comment.
I modified the commit description to explain why we need to remove that validation and move it in the promo code creation step, let me know if now is ok
| @@ -37,6 +38,10 @@ def usage_limit | |||
| promotion.per_code_usage_limit | |||
| end | |||
|
|
|||
| def promotion_not_apply_automatically | |||
| errors.add(:base, I18n.t('spree.errors.messages.could_not_create_promotion_code_on_apply_automatically_promotion')) if promotion.apply_automatically | |||
There was a problem hiding this comment.
There's no need to use a generic key here, you can have the same result by using:
errors.add(:base, :disallaowed_with_apply_automatically) if promotion.apply_automaticallyand moving the translation into:
en.activerecord.errors.models.spree/promotion_code.attributes.base.disallaowed_with_apply_automatically
(this probably needs a double check on documentation 🙂)
There was a problem hiding this comment.
Do you mean the solidus developers guide? In that guide there isn't any section about the promotion code, maybe I can create a new page for the promotion codes.
There was a problem hiding this comment.
this probably needs a double check on documentation
No, I meant in the Rails documentation.
There was a problem hiding this comment.
Sorry, my fault, I misunderstood 🙂
backend/spec/controllers/spree/admin/promotion_codes_controller_spec.rb
Outdated
Show resolved
Hide resolved
|
@MassimilianoLattanzio can you please fixup some commits together where it makes sense? |
Done 👍 |
Otherwise it will be possible to create useless promotions codes on promotions that apply automatically to all orders
Deleted useless button/route in admin UI to create new promotion code if a promotion applies automatically.
To explain that if promotion is created with apply automatically option, promo codes creation will be disabled for that promotion.
This is important for backward compatibility since if we keep validating against promotions with codes as well, it will be impossible to update existing apply-auto promotions that already have a code associated. Since promotion codes are created in a second step, the creation validation have to be done in that step.
That requirement generate an error adding any product to cart when exist a promotion that apply automatically without any code.
kennyadsl
added
Needs Core Team Review
changelog:solidus_backend
Description
Disabled creation of promotion code if a promotion applies automatically to all orders. Managed also old "apply-automatically" promotions with and without associated promotion codes.
Solve #3472
Checklist: