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

View example group inserts controller path into lookup_context in the wrong order #2729

Open
sfnelson opened this issue Feb 5, 2024 · 2 comments · May be fixed by #2749
Open

View example group inserts controller path into lookup_context in the wrong order #2729

sfnelson opened this issue Feb 5, 2024 · 2 comments · May be fixed by #2749

Comments

@sfnelson
Copy link

sfnelson commented Feb 5, 2024

What Ruby, Rails and RSpec versions are you using?

Ruby version: 3.2.3
Rails version: 7.1.3
RSpec version: 3.12.2

Observed behaviour

Testing a controller where the 'prefixes' logic in ActionView::Renderer::AbstractRenderer#merge_prefix_into_object_path applies fails to find view partials when called from inside a nested controller.

This happens because the logic uses @context_prefix = lookup_context.prefixes.first, which in view specs is "", instead of the controller path.

Expected behaviour

@context_prefix = lookup_context.prefixes.first should return the controller's path

Can you provide an example reproduction?

Hard to reproduce as it relates to rails/rails#50916 – the logic in merge_prefix_into_object_path isn't very useful. I've got a monkey patch for that logic that address the issue, which is how I came across this issue.

I believe the logic in view_example_group is faulty because it uses append instead of prepend when setting up the lookup context prefixes.

view.lookup_context.prefixes << _controller_path

sfnelson added a commit to katalyst/koi that referenced this issue Feb 5, 2024
@pirj
Copy link
Member

pirj commented Feb 6, 2024

So if you change it to prepend (unshift) instead of << in rspec-rails code itself, would it fix your issue?

Controller path taking precedence over a zero prefix sounds logical.
But we’ll need to figure out a spec to indicate what we’re fixing.

Would you like to submit a PR?

@sfnelson
Copy link
Author

sfnelson commented Feb 6, 2024

@pirj yes, changing line 189 to unshift instead of << will fix the issue. I'll see if I can create a reproduction and a PR.

sfnelson added a commit to katalyst/rspec-rails that referenced this issue Mar 27, 2024
Rails rendering assumes that the first prefix encountered is 'special', specifically, it's the controller's prefix. This can be seen in `ActionView::AbstractRenderer::ObjectRendering#initialize`. This change injects the controller path at the start of the prefix list instead of the end to make sure this assumption is satisfied.

Resolves rspec#2729.
@sfnelson sfnelson linked a pull request Mar 27, 2024 that will close this issue
sfnelson added a commit to katalyst/rspec-rails that referenced this issue Apr 10, 2024
Rails rendering assumes that the first prefix encountered is 'special', specifically, it's the controller's prefix. This can be seen in `ActionView::AbstractRenderer::ObjectRendering#initialize`. This change injects the controller path at the start of the prefix list instead of the end to make sure this assumption is satisfied.

Resolves rspec#2729.
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 a pull request may close this issue.

2 participants