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

Improve multiple belongs_to hack #5938

Merged
merged 2 commits into from
Nov 21, 2019
Merged

Improve multiple belongs_to hack #5938

merged 2 commits into from
Nov 21, 2019

Conversation

leio10
Copy link
Contributor

@leio10 leio10 commented Nov 18, 2019

This PR slightly modifies polymorphic routes generation. When mapping the route parameters, instead of checking each class of the given parameters against the parent class, it does it against the configured belongs_to resource class. This has sense because if this check is successful it won't use the parent but the configured belongs_to resource.

As explained in #221, when you define several belongs_to for a resource:

ActiveAdmin.register Movie do
  belongs_to :provider, optional: true
  belongs_to :actor, optional: true
end

And then add the missing routes:

namespace :admin do
    resources :movies
    resources :providers do
        resources :movies
    end
    resources :actors do
        resources :movies
    end
end

Everything seems to work well, but when browsing the movies for the provider 123, some links are generated as /actors/123/movies instead of /providers/123/movies.

This change fixes this issue and keeps the same behavior when having only one belongs_to declaration.

Also, I've added some tests to avoid regression, even when multiple belongs_to is still not supported, since it can be useful for many people, and could be a little step further toward supporting this 😃 .

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Hi @leio10! Thanks, this makes sense to me!

Can you add a change log entry following the existing format and squash everything into a single commit?

spec/unit/resource_controller/polymorphic_routes_spec.rb Outdated Show resolved Hide resolved
@leio10
Copy link
Contributor Author

leio10 commented Nov 20, 2019

Hi @deivid-rodriguez, thanks for the review!

I did what you asked for, but now seems that JS linting is failing for some strange reason 🤕

@leio10
Copy link
Contributor Author

leio10 commented Nov 20, 2019

Ok, @deivid-rodriguez, it was my mistake. I got confused with the Gherkin error, which is actually ignored. The real issue was that my changelog entry wasn't pointing to the right URL. I'm impressed that you have a test for that. 😍 🙌

If we are going to use the configured `belongs_to` resource to generate the path part, is better to compare the given record with that class, instead of doing it with the `parent` class.
This change shouldn't affect when working with just one `belongs_to` sentence, but it should fix the paths generation with multiple `belongs_to`.

Added tests for polymorphic routes with multiple belongs_to

Changelog entry added
@deivid-rodriguez
Copy link
Member

Thanks @leio10! Still gherkin-lint failing is unexpected, trying to fix it in #5941.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

💜

@javierjulio
Copy link
Member

@deivid-rodriguez I see you pushed the latest master changes to get this to pass. Thanks again! Just wanted to confirm this was ready in case you may have needed to do something else?

@deivid-rodriguez
Copy link
Member

Yeah, I fixed changelog conflicts through github's UI in order to not bother @leio10 anymore. It's ready (I would "Squash and merge" to get rid of the merge commit).

@javierjulio
Copy link
Member

Oh don't you worry, I'll be getting rid of that merge commit. 😁 Thanks again!

Copy link
Member

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

Thanks you two! ❤️

@javierjulio javierjulio merged commit a17cd9c into activeadmin:master Nov 21, 2019
@leio10 leio10 deleted the improve-multiple-belongs_to-hack branch November 21, 2019 15:59
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