Do not consider promotions without actions as active #3749
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for hopping on this @DanielePalombo ! Apologies, I've been underwater with work and haven't been able to make time back to this. |
DanielePalombo
force-pushed
the
2777-inactive-actionless-promotions
branch
2 times, most recently
from
96bd049 to
473870c
Compare
kennyadsl
approved these changes
Sep 7, 2020
There was a problem hiding this comment.
Left a non-blocking comment. 👍
Thanks to both @wildbillcat and @DanielePalombo.
core/app/models/spree/promotion.rb
Outdated
| @@ -84,7 +93,7 @@ def not_expired? | |||
| end | |||
|
|
|||
| def active? | |||
| started? && not_expired? | |||
| started? && not_expired? && (Spree::Config.actionless_promotion_inactive ? actions.present? : true) | |||
There was a problem hiding this comment.
What about a different method (has_actions?) for this?
There was a problem hiding this comment.
I think that has_actions? would be confused because it will return true is the preference actionless_promotion_inactive is false.
I have considered this suggestion and following the @aldesantis comments I have named it consider_active_by_actions_presence
kennyadsl
added
Needs Core Team Review
changelog:solidus_core
This was referenced Sep 8, 2020
kennyadsl
changed the title
aldesantis
requested changes
Sep 9, 2020
core/lib/generators/solidus/install/templates/config/initializers/spree.rb.tt
Outdated
Show resolved
Hide resolved
…no actions are assigned
DanielePalombo
force-pushed
the
2777-inactive-actionless-promotions
branch
2 times, most recently
from
e69ab26 to
a9b49c4
Compare
DanielePalombo
force-pushed
the
2777-inactive-actionless-promotions
branch
from
a9b49c4 to
08e1ba7
Compare
DanielePalombo
force-pushed
the
2777-inactive-actionless-promotions
branch
from
08e1ba7 to
84192cf
Compare
kennyadsl
approved these changes
Oct 2, 2020
core/lib/generators/solidus/install/templates/config/initializers/spree.rb.tt
Outdated
Show resolved
Hide resolved
Remove the join table otherwise started_and_unexpired scope will return only the promotion with actions, and the code doesn't reproduce the deprecated behavior
DanielePalombo
force-pushed
the
2777-inactive-actionless-promotions
branch
from
84192cf to
109c1bb
Compare
|
@aldesantis can you please review again when you have some time? 🙏 |
aldesantis
approved these changes
Oct 7, 2020
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is based on #3596.
From original PR:
This is a proposed solution to #2777 where action-less promotions were considered active. As opposed to my original thought of implementing validation, given that there is no state toggle on the promotion that it would make sense to embrace the inferred state. So this implementation just causes a promotion to be considered inactive until it has actions, in addition to the other required material. Otherwise it is considered inactive.
In the process I found the coupon promotion handler already treated action-less coupons as inactive, so I updated it to continue this behavior.
I have only added the spec and refactored it.
Thank you @wildbillcat
Checklist: