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

Pass page local when rendering fields #1788

Merged
merged 1 commit into from
Nov 26, 2020
Merged

Pass page local when rendering fields #1788

merged 1 commit into from
Nov 26, 2020

Conversation

bobcats
Copy link
Contributor

@bobcats bobcats commented Oct 8, 2020

Hi there!

We live dangerously and run on edge for administrate, and seems like a recent change (#1259) has caused HasOne's to not render in our app with a undefined local variable or method 'page'

The proposed change in the PR seems to resolve the error.

Let me know if I need to change anything, or if this is too naive of a solution.

Thanks!

@nickcharlton
Copy link
Member

Ah nice, thanks! It's fairly likely that me merging that caused this.

To stop this happening again, can you think of a way we can test this? Ideally the ideal solution to the problem.

@nickcharlton nickcharlton added bug breakages in functionality that is implemented fields new fields, displaying and editing data labels Oct 12, 2020
@bobcats
Copy link
Contributor Author

bobcats commented Oct 12, 2020

Totally! I think the reproduction ended up being trying to render has_ones with has_ones. Setting up Payment with a has_one without passing the page local reproduced the error.

I'm used to using render_views, but it seems like this isn't used much in the codebase. Is there a different approach you'd like to take with the test? Just checking for not raising an error seems not ideal. Also, trying to figure out how to get the Hound rule to pass, as with positional args it seems to cause the test to fail.

@pablobm
Copy link
Collaborator

pablobm commented Oct 15, 2020

Issue also reported at #1792

@pablobm
Copy link
Collaborator

pablobm commented Oct 15, 2020

@bobcats - I agree that testing for the page not to error is not ideal. I wonder if this could be done with a view spec instead? Eg: trying to render something at https://github.com/thoughtbot/administrate/blob/master/spec/administrate/views/fields/has_one/_show_spec.rb and checking that the expected text is there.

@hound hound bot deleted a comment from pablobm Oct 22, 2020
@bobcats
Copy link
Contributor Author

bobcats commented Oct 22, 2020

Thanks for the feedback folks! I've tried testing the scenario as a view spec, with albeit a lot of double-ifying. Let me know what you all think.

@sedubois sedubois mentioned this pull request Oct 27, 2020
Copy link
Collaborator

@pablobm pablobm left a comment

Choose a reason for hiding this comment

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

Thank you for this. It looks pretty good. Regarding those stubs, I think let's try use the real objects (left some comments on that). I see that the original file was already using many stubs and you followed the same pattern, but I'd rather start do differently here.

@bobcats
Copy link
Contributor Author

bobcats commented Nov 13, 2020

@pablobm Thanks for the feedback again! So, I tried using real objects, but end up getting being unable to reproduce the original error because the combinations of associations that show this issue don't seem to exist in the sample app's models.

Should I create that particular combination of associations in the sample app and then use the real models to do so? I was trying to avoid changing the models, but since there's a fair amount of reflecting going on, it seems more straightforward. I think I need a Parent has_one Child has_many Children.

@pablobm
Copy link
Collaborator

pablobm commented Nov 26, 2020

@bobcats - It's ok, thanks for trying. I think we can merge this as-is then. No need to make it more complicated than it needs be.

@pablobm pablobm merged commit f27396b into thoughtbot:master Nov 26, 2020
pablobm added a commit that referenced this pull request Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug breakages in functionality that is implemented fields new fields, displaying and editing data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants