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

Allow to easily extend the TaxonsController #2782

Merged

Conversation

coorasse
Copy link
Contributor

@coorasse coorasse commented Jun 21, 2018

Scenario
solidus_multi_domain wants to extend the TaxonsController. Since there is no way to override only the loading of the Taxon, it ends up overriding the entire show method.

Problem
solidus_frontend changes the show method to preload images: https://github.com/solidusio/solidus/blob/master/frontend/app/controllers/spree/taxons_controller.rb#L13

solidus_multi_domain does not receive the changes and we end up with a N+1 query when using both.

Solution
Extract the load_taxon method so that solidus_multi_domain can override only that specific method.

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, I like the change. I'm just a little bit afraid that if users have overwrote this action they could end up having an extra query. Maybe a line into the Changelog would be enough.

respond_to :html

def show
@taxon = Spree::Taxon.find_by!(permalink: params[:id])
return unless @taxon
Copy link
Member

Choose a reason for hiding this comment

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

This line was useless, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. exactly. I also double checked to be sure but the find_by! raises an exception if the element is not found, returning 404.

@coorasse
Copy link
Contributor Author

Good catch. Yes, that would happen.
Do you want me to add a note to the Changelog? Another option would be to call @taxon = load_taxon directly inside the action...even though I would prefer the current version.

@kennyadsl
Copy link
Member

@coorasse let's go with just adding a line to the Changelog, thanks!

@coorasse coorasse force-pushed the feature/extend_taxons_controller branch from ec46f2a to a1c3530 Compare June 22, 2018 08:15
@coorasse
Copy link
Contributor Author

Good. I updated the CHANGELOG and squashed. 👍

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 475d9db into solidusio:master Jun 27, 2018
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.

3 participants