Fix routing issue with engine mounted to root_path

[ipmsteven]

No description provided.

[dtognazzini]

This code is similar to Rails3#recognizes?. Please reuse code as much as possible. In general, the CodeClimate score should not decrease:
https://codeclimate.com/github/appfolio/ae_page_objects/compare/fixRouterEngineRoot

[dtognazzini]

The refactoring in f8a75e0 is pretty good. However, the resulting method is still a bit difficult to read. I took a shot at refactoring here: fixRouterEngineRoot...refactorRecognize

It's still unclear to me what the path_route_result holds per my earlier comments. What do you think about incorporating some of the changes on that branch?

[ipmsteven]

@dtognazzini good refactoring 👍 👍 👍

path_route_result stores the resolved hash which contains controller and action a named_path maps to by calling router.named_routes[path].requirements,

For example:
If we have a named_path called new_book, passing it to router.named_routes[:new_book].requirements we get {:controller: :books, action: :new}.

Also we can ask a rails router to get the resolved hash by passing a url('/books/new') and a http verb(:get), like recognized_route = recognize_path(router, url, method), the returned recognized_route hold a hash that contains controller and action a url and http verb pair maps to.

If the recognized_route and path_route_result are the same, then we can say the path matches url.

Above is the idea for the recognizes? method, however the recognized_route and path_route_result are not good names to indicate what kind of data behind those variables.

Maybe we should explicitly call them controller_and_action_from_named_path and controller_and_action_from_url_and_http_verb? 😨

[dtognazzini]

Ok. I'm not convinced that router.named_routes[path].requirements always returns a hash containing only :controller and :action keys. The code in recognize_path assumes that's always true.

That said, we can still merge this PR ahead of answering that question.

One thing that would make this easier to read by making the above assumption clear as day is to use a value object:

RouteTarget = Struct.new(:controller, :action)

We could use RouteTarget (I'm not in love with the name) in both places. You could rename the local variables accordingly. If/when the comparison needs to take into account more keys (or whatever) from Rails' routing, we'd just need to update the value object with this data.

[ipmsteven]

@dtognazzini requirements is a method from https://github.com/rails/journey/blob/master/lib/journey/route.rb#L39

I didn't dive into the rails routing code too much but I agree that it's hard to say if requirements always returns a hash containing only :controller and :action keys or not.

However, the comment for requirements method in https://github.com/rails/journey/blob/master/lib/journey/route.rb#L40 guide me to the implementation of rake routes task https://github.com/rails/docrails/blob/master/railties/lib/rails/tasks/routes.rake#L4

Then I found routes.rake use action_dispatch/routing/inspector under the hood and found serval usages of requirements method inside inspector https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/routing/inspector.rb#L56-L62

I think that would be a good hint for adding engine name_path support inside aepos-rails in future.

And for now, let's try the RouteTarget = Struct.new(:controller, :action) way 👲

[dtognazzini]

Based on the last 2 commits, I refactored a bit more here: fixRouterEngineRoot...refactorApplicationRouter

The commits are focused with descriptive messages. Let me know what you think.

[ipmsteven]

@dtognazzini that looks pretty good for me 😄 😃 😀

I only have two concerns:

1. Regarding the test failure here(https://travis-ci.org/appfolio/ae_page_objects/jobs/42222265), it seems that we need return a NullObject for ResolvedRoute after line 55?
   

   ae_page_objects/lib/ae_page_objects/core/application_router.rb

   Line 55 in 1417f24

   rescue ActionController::RoutingError, ActionController::MethodNotAllowed

2. Can we memorize router function? like

def router
  @router ||= ::Rails.application.routes
end

[dtognazzini]

I made a few more changes (see the comparison):

1. Check for unresolvable routes.
2. Remove unnecessary == method as Struct provides that for you.

Regarding memoizing: Yes, you could do this. However, I'm not sure it buys you much as Rails.application.routes (like the other implementations of router) is already "memoized".

[dtognazzini]

Looks good.

[dtognazzini]

Addresses #63

[ipmsteven]

yeah ✋