Skip to content

Make sure @resource is also set for nested fields#83

Open
pedantic-git wants to merge 1 commit into
nickcharlton:mainfrom
fishpercolator:bug/resource_not_set
Open

Make sure @resource is also set for nested fields#83
pedantic-git wants to merge 1 commit into
nickcharlton:mainfrom
fishpercolator:bug/resource_not_set

Conversation

@pedantic-git
Copy link
Copy Markdown

Hey @nickcharlton - long time since we've interacted on Administrate-related things!

I love this gem and it's been really useful in my latest project. One bug I found is that if any of the fields from the associated dashboard use self.resource, they end up picking up the parent resource instead of the nested one.

You'd already used instance_variable_set to set the self.data and so I propose that the gem uses the same methodology to set resource.

I've tested this fix in my app that has a full test suite and it hasn't broken any of the tests in that app. I couldn't get the gem's tests to run because they depend on a postgres database, which I don't have, but I couldn't see a place in the tests for defending this change anyway - the earlier @data setting doesn't appear to have a test defending it.

Let me know what you think!

Copy link
Copy Markdown
Owner

@nickcharlton nickcharlton left a comment

Choose a reason for hiding this comment

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

Ah, nice! Is there a way we could reasonably test, mostly so we don't introduce a regression at some point?

@pedantic-git
Copy link
Copy Markdown
Author

I couldn't see an easy win there, unfortunately, and I couldn't get the tests to run locally because of their dependency on Postgres. If you can think of a way to test it I'll happily add!

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.

2 participants