Skip to content

/ solidus

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

Jump to bottom

Do not consider promotions without actions as active #3749

Merged
merged 6 commits into from Oct 7, 2020
Merged

Do not consider promotions without actions as active #3749

merged 6 commits into from Oct 7, 2020

Conversation

@DanielePalombo
Copy link
Contributor

@DanielePalombo DanielePalombo commented Aug 28, 2020

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:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)
@wildbillcat
Copy link
Contributor

@wildbillcat wildbillcat commented Aug 28, 2020

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 DanielePalombo force-pushed the DanielePalombo:2777-inactive-actionless-promotions branch 2 times, most recently from 96bd049 to 473870c Aug 31, 2020
@kennyadsl
kennyadsl approved these changes Sep 7, 2020
View changes
Copy link
Member

@kennyadsl kennyadsl left a 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)

This comment has been minimized.

Sign in to view
@kennyadsl

kennyadsl Sep 7, 2020
Member

What about a different method (has_actions?) for this?

This comment has been minimized.

Sign in to view
@DanielePalombo

DanielePalombo Oct 2, 2020
Author Contributor

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
@DanielePalombo DanielePalombo marked this pull request as ready for review Sep 7, 2020
This was referenced Sep 8, 2020
Open
Closed
@kennyadsl kennyadsl changed the title 2777 inactive actionless promotions Do not consider promotions without actions as active Sep 8, 2020
@kennyadsl kennyadsl added this to the 2.11 milestone Sep 9, 2020
@aldesantis
aldesantis requested changes Sep 9, 2020
View changes
...lib/generators/solidus/install/templates/config/initializers/spree.rb.tt Outdated Show resolved Hide resolved
core/lib/spree/app_configuration.rb Outdated Show resolved Hide resolved
@wildbillcat @DanielePalombo
added logic to indicate a the status of a promotion as inactive when …
4b99d28 
…no actions are assigned
@DanielePalombo DanielePalombo force-pushed the DanielePalombo:2777-inactive-actionless-promotions branch 2 times, most recently from e69ab26 to a9b49c4 Oct 2, 2020
@DanielePalombo DanielePalombo mentioned this pull request Oct 2, 2020
Closed
@DanielePalombo DanielePalombo force-pushed the DanielePalombo:2777-inactive-actionless-promotions branch from a9b49c4 to 08e1ba7 Oct 2, 2020
@DanielePalombo DanielePalombo requested review from aldesantis and kennyadsl Oct 2, 2020
@DanielePalombo DanielePalombo force-pushed the DanielePalombo:2777-inactive-actionless-promotions branch from 08e1ba7 to 84192cf Oct 2, 2020
@kennyadsl
kennyadsl approved these changes Oct 2, 2020
View changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Left a couple of comments about some terms. The rest looks good to me, thanks!
...lib/generators/solidus/install/templates/config/initializers/spree.rb.tt Outdated Show resolved Hide resolved
core/lib/spree/core/engine.rb Outdated Show resolved Hide resolved
core/lib/spree/app_configuration.rb Outdated Show resolved Hide resolved
@DanielePalombo
Make pref to consider actionless active explicit
6d634fd
@DanielePalombo
Show warning for deprecated value
5a969ac
@DanielePalombo
Remove joins(:promotion_actions)
b42f552 
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
Add with_action trait
306d402
@DanielePalombo
Refactor Spree::Promotion#active method
Loading status checks…
109c1bb
@DanielePalombo DanielePalombo force-pushed the DanielePalombo:2777-inactive-actionless-promotions branch from 84192cf to 109c1bb Oct 2, 2020
@kennyadsl
Copy link
Member

@kennyadsl kennyadsl commented Oct 7, 2020

@aldesantis can you please review again when you have some time? 🙏
@kennyadsl kennyadsl requested a review from solidusio/core-team Oct 7, 2020
@aldesantis
aldesantis approved these changes Oct 7, 2020
View changes
@kennyadsl kennyadsl merged commit 176e3a7 into solidusio:master Oct 7, 2020
15 of 16 checks passed
15 of 16 checks passed
ci/circleci: postgres_rails_master_activestorage Your tests failed on CircleCI
Details
Header rules - solidus-docs No header rules processed
Details
Header rules - solidus-guides No header rules processed
Details
Pages changed - solidus-docs 325 new files uploaded
Details
Pages changed - solidus-guides All files already uploaded
Details
Redirect rules - solidus-docs No redirect rules processed
Details
Redirect rules - solidus-guides No redirect rules processed
Details
Hound Smells good to me. Woof!
Mixed content - solidus-docs No mixed content detected
Details
Mixed content - solidus-guides No mixed content detected
Details
ci/circleci: mysql Your tests passed on CircleCI!
Details
ci/circleci: mysql_rails52 Your tests passed on CircleCI!
Details
ci/circleci: postgres Your tests passed on CircleCI!
Details
ci/circleci: postgres_rails52 Your tests passed on CircleCI!
Details
netlify/solidus-docs/deploy-preview Deploy preview ready!
Details
netlify/solidus-guides/deploy-preview Deploy preview ready!
Details
@DanielePalombo DanielePalombo deleted the DanielePalombo:2777-inactive-actionless-promotions branch Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kennyadsl kennyadsl

@aldesantis aldesantis

Assignees
No one assigned
Labels
Needs Core Team Review Solidus Core
Projects
None yet
Milestone
2.11
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@DanielePalombo @wildbillcat @kennyadsl @aldesantis
You can’t perform that action at this time.