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

Show only active promotions filter #3595

Merged
merged 1 commit into from
Apr 29, 2020

Conversation

wildbillcat
Copy link
Contributor

Description
This adds active as a ransackable scope to be used in the admin search.

Motivation is that for stores that have a large number of promotions build up, and it is helpful to just see what is active. This is helpful to make sure old/test promotions aren't mistakenly enabled, as they can be costly if not monitored closely.

Standard scope:
image

Active filter enabled:
image

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)

Copy link
Member

@aldesantis aldesantis left a comment

Choose a reason for hiding this comment

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

Thanks, left a few comments!


<div class="col-2">
<div class="field">
<%= label_tag :active, 'Show Only Active' %><br>
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a translation for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, though a opposed to creating a translation I leveraged spree.active. Here is an image:
image
I can change to a new translation if preferred, but looking at it I think inference is pretty strong.

<div class="col-2">
<div class="field">
<%= label_tag :active, 'Show Only Active' %><br>
<%= f.check_box :active, label: false, as: :boolean, inline_label: "#{ t 'spree.active'}", checked_value: true %>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the inline_label option here?

Copy link
Member

Choose a reason for hiding this comment

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

And even if we do, we probably just want to pass in t('spree.active') rather than "#{t 'spree.active'}" since they should produce the same value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not needed. I stripped it out.

@@ -62,6 +63,10 @@ def promotion_code_batch_params
def promotion_includes
[:promotion_actions]
end

def clear_boolean(query_params, condition)
query_params.delete(condition) if query_params[condition] == "0"
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need this? The Ransack documentation states that scopes are only applied for true values (unless the scope accepts arguments), so a value of 0 should cause Ransack to ignore the scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! Will make the change

Copy link
Member

@aldesantis aldesantis left a comment

Choose a reason for hiding this comment

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

Thanks @wildbillcat, looking great! 🙂

@aldesantis
Copy link
Member

@wildbillcat one last thing before we merge: could you squash the commits together?

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.

Thank you!

@kennyadsl kennyadsl added the changelog:solidus_backend Changes to the solidus_backend gem label Apr 29, 2020
@kennyadsl
Copy link
Member

Adding a holding label while waiting for the commits squash, thanks again!

@wildbillcat
Copy link
Contributor Author

Sorry for the delay, things got a bit crazy at work yesterday. Rewrote history to be 1 commit.

@kennyadsl kennyadsl merged commit 7e05824 into solidusio:master Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_backend Changes to the solidus_backend gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants