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

[WIP] Always use the default store #2022

Closed
wants to merge 2 commits into from

Conversation

kennyadsl
Copy link
Member

@kennyadsl kennyadsl commented Jun 15, 2017

This commit avoids doing 2 extra queries for every request (#1971)
even when solidus store is not using solidus_multi_domain extension.

This logic has now been moved to the solidus_multi_domain extension
since it is needed only when multiple stores are expected
(solidusio-contrib/solidus_multi_domain#67).

NOTE: there's something I'm not too happy about this PR. I'm moving out to solidus_multi_domain extension a spec about filtering api orders by store. But the code that filters api orders at the moment is still in core.
Since the new version of the "store selector class" always returns the default store, I'm not sure it makes sense to test that logic in core, even if the code is here. What about moving the controller code into the extension as it is done for shipments?

@kennyadsl kennyadsl self-assigned this Jun 15, 2017
@kennyadsl kennyadsl changed the title Always use the default store [WIP] Always use the default store Jun 15, 2017
This commit avoids doing 2 extra queries for every request (solidusio#1971)
even when solidus store is not using solidus_multi_domain extension.

This logic has now been moved to the solidus_multi_domain extension
since it is needed only when multiple stores are expected
(solidusio-contrib/solidus_multi_domain#67).
They'll be moved in solidus_multi_domain. This commit also
reorganizes a bit api orders specs for the displaying orders
part.
@kennyadsl kennyadsl force-pushed the use-default-store branch from a18dd4c to 01ae9b6 Compare June 16, 2017 07:46
@kennyadsl kennyadsl changed the title [WIP] Always use the default store Always use the default store Jun 16, 2017
@mtomov
Copy link
Contributor

mtomov commented Jun 16, 2017

Thanks!

I haven't used more than 1 store, but if it is a requirement to use the solidus_multi_domain extension to work with more than one store, then I guess the logic should belong in the extension. Then again, I'm not too familiar if core is meant to support multiple stores without the extension.

@jhawthorn
Copy link
Contributor

I think we've been moving more towards having the multi_domain behaviour included in core, just because of how frequent a need it is (and how invasive the changes are otherwise). But I appreciate how wasteful those three queries are on single-store apps.

Maybe we want to include several store selectors side-by-side Spree::StoreSelector::Default/Spree::StoreSelector::ByDomain for users to select. WDYT?

@tvdeyen
Copy link
Member

tvdeyen commented Jun 21, 2017

I like the idea of provided selectable Store selector classes.

Why do we even still have this extension? Shouldn't we deprecate it?

@kennyadsl
Copy link
Member Author

Since the direction is to gradually deprecate solidus_multi_domain, I'm going to change this PR trying to pull stuff from that extension into core instead of the opposite. I'll update this soon!

@kennyadsl kennyadsl changed the title Always use the default store [WIP] Always use the default store Jun 22, 2017
@kennyadsl
Copy link
Member Author

kennyadsl commented Jul 6, 2017

Closing this one in favor of #2041 which is a better solution.

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