-
Notifications
You must be signed in to change notification settings - Fork 55
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
Fix filtering by line item in admin #275
Fix filtering by line item in admin #275
Conversation
This allows us to scope subscriptions tied to a particular variant, instead of having to hunt down the specific line item when filtering. This also restructures the query in the view to avoid N+1s. On a store with 100K subscriptions the query and relative map take ~30ms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
Would you mind adding an entry to the changelog about this? I would love to start having official releases on ruby gem more often with a curated changelog.
|
||
<%= | ||
f.select( | ||
:with_line_item, | ||
options_for_select(::SolidusSubscriptions::LineItem.joins(:subscribable).map { |item| [ item.subscribable.name, item.id ]}, params.dig(:q, :with_line_item)), | ||
:with_subscribable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More than a select field, this calls for a search field that does an API call to avoid the query altogether when loading the page. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I've seen this too late. I think this would be an even better UX, especially in stores with lots of subscribable variants. I'll open a ticket for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great idea. I merged this as is because this fixes a bug, but this improvement would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Thank you @nirebu and @stefano-sarioli (and @kennyadsl )! |
Summary
Fixes #274
This PR fixes the above issue by changing how the admin scopes subscriptions tied to a particular variant, instead of having to hunt down the specific line item when filtering them.
This also restructures the query in the view to avoid N+1s. On a store with 100K subscriptions the query and relative map take ~30ms.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed (
cross them outif they are not):