Router rewrite and support of singleton belongs to resources

[dmitry]

It's WIP branch, which will be squashed and completed, after you give me a green light on it.

• Rewrite of the router definitions
• Add`s ability to define singleton resource

Related

• Support multiple belongs_to (Support multiple belongs_to #221)
• Support nested belongs_to (Adding nested belongs_to #3280)

Singleton resource

ActiveAdmin.register Place
ActiveAdmin.register Location do
  belongs_to :place, singleton: true
end

Generates correct route with resource DSL command, instead of resources, so it's possible to get access location using the url: /admin/users/1/location instead of /admin/users/1/locations.

[dmitry]

/ping @seanlinsley

Can you please check this pull-request and if it's ok, I will complete it. Thanks!

[seanlinsley]

I'd prefer that you remove style changes like this

[dmitry]

OK. I will do it.

PS. Any styleguide you are following?

[seanlinsley]

What still needs to be done? I'm hesitant to do an in-depth review of this if there are still a lot of things you want to change.

[dmitry]

Rename some methods and add documentation, other things overall will stay the same.

[dmitry]

Rebased and updated according to a requirements from the @seanlinsley . Now can be checked.

[dmitry]

If everything is ok and can be merged, I can write some more specs for newly introduced class ResourceRoutes.

[dmitry]

Can it be reviewed?

[dmitry]

Why does it takes so much time? Can I assist some how to get it to be merged faster? I really need it for my few projects.

[dmitry]

/ping @gregbell @timoschilling

[Fivell]

@dmitry , can it be used for this feature #2825 ?

[dmitry]

@Fivell yes, that's what this PR actually does.

[bamorim]

@dmitry Just a style question: Why inside the ResourceRoutes you do the case config?
Isn't better to have two classes ResourceRoutes and PageRoutes and do the case on the define_resource_routes method?

That way we could subclass and have shared behaviour with ease.

By the way. 👍

[dmitry]

@bamorim it was left from the previous version of the file. That's a nice idea, I will work on it, once I will have some time and anyone from the maintainers will agree to include this PR into the master :)

[dmitry]

@seanlinsley can this feature be merged or at least reviewed?

[timoschilling]

@dmitry I will do a review, in the next days. But please revert the style changes and code refactorings, as @seanlinsley already said. Your PR has to many noise of style changes. It's not about the style guide, it's about the mix of feature implementation and refactoring. You should never do both at the same time!

[dmitry]

@timoschilling I only found one style change: https://github.com/activeadmin/activeadmin/pull/3170/files#diff-55e5e7154c56512a3495ca1876501d08R18 Just two lines. Please point me, where I've changed the style anywhere else (I guess improvement of just 2 lines isn't a real change)?

[timoschilling]

@dmitry your right that are not so many on a second look.

1. that one you named
2. the application accessor
3. the last line

But the there is one other thing that make it hard to track the change. The code of the ResourceRoutes Line #47 and down are a wild mix of old and new code. In my option ResourceRoutes should move in a own file lib/active_admin/router/resource_routes.rb. That the change is clear to see.

[dmitry]

I've moved ResourceRoutes class in it's own file.

[samvincent]

Good stuff. Looking forward to this one.

[dmitry]

Can it be checked/reviewed and declined or accepted? I will rebase it only after it will be reviewed and accepted. I don't think this should be a show-stopper: 46f94a1

[timoschilling]

46f94a1 is not a show stopper! But can you make a rebase please. In your discripten you write 4 Tasks, but only checked 2 as done, what about the last 2? I need some time to review it. But I will do it in the next days.

[dmitry]

Updated description and rebased.

[timoschilling]

Please take a look on the router_refactoring branch. I have split your commit into 2 separate one, to get a better overview. Please go this way.

I like the idea from @bamorim:

@dmitry Just a style question: Why inside the ResourceRoutes you do the case config?
Isn't better to have two classes ResourceRoutes and PageRoutes and do the case on the define_resource_routes method?
Can you do that?

I like your work 👍 and will merge it after that changes.

[dmitry]

@timoschilling shoud I use your router_refactoring in my activeadmin fork to split into a PageRoutes and ResourceRoutes?

[timoschilling]

@dmitry you can use my or write your one. The important think is that shouldn't be in one commit.

[dmitry]

Can't find router_refactoring anymore.

[timoschilling]

it's back again, sorry for that
we should add a changelog entry for this
and even some line in the docs

[timoschilling]

@dmitry (ping)

[dmitry]

@timoschilling I will work on this PR today. Not easy to extract Page and Resource (not a 10 mins task).

[timoschilling]

@dmitry any updates?

[dmitry]

@timoschilling want to complete today in the evening.

[timoschilling]

👍

[timoschilling]

@dmitry any updates?

[datenimperator]

Any news regarding this?

[dmitry]

Guys, still don't have a time to complete this PR... to much work currently.

[samvincent]

Let me know if I can help with anything. I could write a cucumber example for this configuration if it helps getting this patch to completion. Not too sure what the remaining tasks are.

[dmitry]

Remaining task is refactoring of the code. :)

[timoschilling]

Maybe I found some time for it.

[timoschilling]

One other thing, related to this would be to have the possibility to define a resource on the main level.

[bamorim]

I'll try to handle this soon if I finish my week tasks on my job.

However, what refactoring you think should be applied?

I'm thinking about an elegant way to do this and #3280. I'll first finish this and then I'd copy my tests and rewrite it and update my PR.
Suggestions are welcome.

[timoschilling]

The plan of @dmitry was to split define_routes into 2 different ResourceRouter's, one for each kind.

[timoschilling]

And we need a rebase

[timoschilling]

@bamorim the best think should be you create a new branch based on origin/pr/3170, continue the work and create a new PR with refs this one.

[bamorim]

@timoschilling wow, I was going to suggest it anyway. hahaha
So it's fine. I'll see if I can do this in the weekend.

Another thing, maybe we shouldn't have another type of resource? just like Resouce and Page?
Something like register_singleton.
I mean, this have completely diferent use cases than a normal resource. Also, removing the singleton: true from the belongs_to we will be able to do root singletons.

But this is discussion for another PR, I think.

Actually, I think we have to rethink the architecture. Just passing belongs_to to inherited resources is not going to help. We are strongly dependent on them, and valim just stopped supporting it.

Probably we should have some kind of adapter for the Inherited Resources. I don't know.
IMO we really consider dropping inherited resources or still using it through that adapter. This will allow us to move quickly.

[timoschilling]

@bamorim I agree with you, but as you say it's all for new PR's / Issues.

@rafaelfranca is supporting inherited_resources until Rails 5.0 for us. But after that we need to replace it, I think with a own solution. Maybe @bamorim and @rafaelfranca can work on that. There is already a Issue for that #3604.

[timoschilling]

@bamorim any updates?

[nileshtrivedi]

What's the status of this PR?

[seanlinsley]

Given the limited time we have to review PRs and that we'll have to remove Inherited Resources soon (#3604), I don't think we can afford to accept this PR.

I'm really sorry it turned out this way 😞