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.

Sign up for GitHub

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

Jump to bottom

Fix Spree::Admin::VariantsController#index for deleted products #1804

Merged
merged 4 commits into from
Jun 7, 2017
Merged

Fix Spree::Admin::VariantsController#index for deleted products #1804

merged 4 commits into from
Jun 7, 2017

Conversation

bbuchalter
Copy link
Contributor

@bbuchalter bbuchalter commented Mar 28, 2017 •
edited
Loading

Fixes #1803

When a Product was deleted, @parent would not be set correctly when accessing Spree::Admin::VariantsController#index. Adding the with_deleted scope for the assignment to Spree::Admin::ResourceController#parent addresses this problem.

The heart of the change happens in dec876c while the rest of the commits clean up the specs for Spree::Admin::VariantsController.

@bbuchalter
Copy link
Contributor Author

Obviously this isn't as simple as adding with_deleted, as this causes 111 specs to fail. I could add a check to see if parent_data[:model_class].respond_to?(:with_deleted), but that's pretty ugly. However, I don't have any alternative suggestions. Going to let this sit for a day to see if I get any feedback on how we'd like to move forward with this, or come up with else.

@jordan-brough
Copy link
Contributor

jordan-brough commented Mar 28, 2017 •
edited
Loading

Seems worth fixing in some way. Some thoughts:

I could add a check to see if parent_data[:model_class].respond_to?(:with_deleted), but that's pretty ugly

parent_data[:model_class].paranoid? is maybe a little better?

Any chance this might open up any vulnerabilities for custom controllers that stores have that inherit from ResourceController?

We could also just override the required methods in Spree::Admin::VariantsController itself?

@bbuchalter
Copy link
Contributor Author

We could also just override the required methods in Spree::Admin::VariantsController itself?

As I was thinking about it more, this was the direction I was taking. The question now is to what degree: do I just override #parent or do I stop trying to use ResourceController as the base of VariantsController? Is there an interest in moving away from ResourceController?

@mamhoff
Copy link
Contributor

mamhoff commented Mar 29, 2017

I think the change with the smallest impact that fixes the bug would be overriding #parent on the VariantsController.

@jordan-brough
Copy link
Contributor

Another thought: Would it work to just do:

belongs_to 'spree/product', find_by: :slug, model_class: Spree::Product.with_deleted

?

@jordan-brough
Copy link
Contributor

I'm not sure I love the thought of setting an option called model_class to a scope, but it seems like it could be OK.

@bbuchalter
Copy link
Contributor Author

@jordan-brough your solution is clever, but I think overriding parent is more clear.

@bbuchalter
Copy link
Contributor Author

Poke.

jhawthorn
jhawthorn suggested changes Apr 18, 2017
View reviewed changes
Copy link
Contributor

@jhawthorn jhawthorn left a comment

Choose a reason for hiding this comment

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

I have one suggestion, otherwise LGTM.

I like @jordan-brough's suggestion, but I agree that it's too clever and makes assumptions about how model_class is used. I'd be okay if we introduced a model_scope (or scope) option, but such a change is outside the scope of this PR.

backend/app/controllers/spree/admin/variants_controller.rb Outdated
@parent ||= parent_data[:model_class].with_deleted.send("find_by_#{parent_data[:find_by]}", params["#{model_name}_id"])
instance_variable_set("@#{model_name}", @parent)
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

We know what all these values are, so we can probably just

def parent
  @parent ||= Spree::Product.with_deleted.find_by(slug: params[:product_id])
  @product = @parent
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree this is an improvement. Will fix.

@bbuchalter
Use subject in Admin::VariantsController spec
e04069f 
By using subject we can eliminate duplication in our specs.
@bbuchalter
Better organize Admin::VariantsController spec
9081d51 
With no context blocks, this spec was setup to only test one thing.
Let's wrap the current tests in some context so we can add more tests
without having unneeded setup.
@bbuchalter
Support deleted products in Admin::VariantsCtrl
d134f3a 
Per issue solidusio#1803 deleted
products could not show their variants due to a missing `with_deleted`
scope on `ResourceController#parent`.
@bbuchalter
Improve context names in VariantsController spec
f6308b6 
These context descriptions are more descriptive and succinct.
@bbuchalter
Copy link
Contributor Author

Feedback implemented. Ready for another review.

jhawthorn
jhawthorn approved these changes Apr 20, 2017
View reviewed changes
Copy link
Contributor

@jhawthorn jhawthorn left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks again Brian

@jhawthorn jhawthorn added changelog:solidus_backend Changes to the solidus_backend gem type:bug Error, flaw or fault Code review needed labels Apr 20, 2017
mamhoff
mamhoff approved these changes May 18, 2017
View reviewed changes
Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

Well done, thank you!

@bbuchalter
Copy link
Contributor Author

Poke.

@tvdeyen tvdeyen merged commit 567fa49 into solidusio:master Jun 7, 2017
@mamhoff mamhoff mentioned this pull request Jun 7, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mamhoff mamhoff mamhoff approved these changes

@jhawthorn jhawthorn jhawthorn approved these changes

@tvdeyen tvdeyen tvdeyen approved these changes

Assignees
No one assigned
Labels
changelog:solidus_backend Changes to the solidus_backend gem type:bug Error, flaw or fault
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bbuchalter @jordan-brough @mamhoff @tvdeyen @jhawthorn