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

Promotion rule product limit improvements #3934

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

nirnaeth
Copy link
Contributor

Description
Ref #3902

Adding a further parameter show_all to the select2 rule when picking products in the admin interface.
The parameter should act only when the selection is initialized at page load and should not have an impact on the product drop-down.
Now the promotion rule section for products should properly show all products associated with the rule and not just up to a certain limit as described in #3902 .

Screenshots
With 8 products associated to the rule:

Before
Screenshot 2021-02-14 at 12 41 52

After
Screenshot 2021-02-14 at 12 42 14

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

Credits
@aldesantis for the lead

@nirnaeth
Copy link
Contributor Author

let me know if I have to squash or reformulate the commit messages

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.

👋 Hey Monica!

This looks great, thanks. 🙏 Please, can you squash commits into a single one?

@nirnaeth
Copy link
Contributor Author

sure, on it!

remove per_page param from product listing for admin search in backend
@nirnaeth
Copy link
Contributor Author

@kennyadsl do I need to do anything else?

@kennyadsl
Copy link
Member

Nope, this is good thanks. We will wait for the next release before merging this. Going to add the right labels/milestone so it's clear for everybody. Thanks again! 🙏

@kennyadsl kennyadsl added this to the 3.1.0 milestone Mar 23, 2021
@kennyadsl kennyadsl added the release:major Breaking change on hold until next major release label Mar 23, 2021
end
end

context 'when a per page limit is not set' do
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me that this context expectations are the same as the context above, only the params are slightly different, so it's not evident to me what difference in result the different params should produce... so I'm missing the reason for this second test 😅. Can you help me with that?

Copy link
Member

Choose a reason for hiding this comment

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

It is basically testing that, no matter which per_page you are requesting, when there's the show_all option it will return all the records, ignoring the per_page param. Makes sense @spaghetticode?

Copy link
Member

Choose a reason for hiding this comment

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

@kennyadsl sure... thanks!

@kennyadsl kennyadsl removed the release:major Breaking change on hold until next major release label Apr 30, 2021
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.

Thanks, Monica!

Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

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

@nirnaeth thank you 👍

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.

4 participants