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

Fix filtering by line item in admin #275

Merged

Conversation

nirebu
Copy link
Contributor

@nirebu nirebu commented Jan 3, 2023

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:

  • I have written a thorough PR description.
  • I have kept my commits small and atomic.
  • I have used clear, explanatory commit messages.

The following are not always needed (cross them out if they are not):

  • I have added automated tests to cover my changes.
  • I have attached screenshots to demo visual changes.
  • I have opened a PR to update the guides.
  • I have updated the README to account for my changes.

nirebu added 2 commits January 3, 2023 16:59
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.
@nirebu nirebu marked this pull request as ready for review January 3, 2023 16:06
Copy link
Contributor

@stefano-sarioli stefano-sarioli left a 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,
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

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!

@kennyadsl kennyadsl merged commit 4e7bddd into solidusio:master Jan 3, 2023
@chrean
Copy link
Contributor

chrean commented Jan 3, 2023

Thank you @nirebu and @stefano-sarioli (and @kennyadsl )!

@nirebu nirebu deleted the nirebu/274-n+1-in-admin-line-item-filtering branch January 4, 2023 09:24
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.

N+1 in admin when loading subscription line items
4 participants