CDRH / orchid

Rails Engine for site integration with CDRH API
MIT License
3 stars 0 forks source link

Override Orchid Routes in Apps #16

Closed jduss4 closed 7 years ago

jduss4 commented 7 years ago

Right now I don't know how to override or alter routes that were drawn by the orchid engine. I haven't read too much about it, yet, but spitballing ideas

http://edgeguides.rubyonrails.org/engines.html#routes

techgique commented 7 years ago

From Jess: "... you can't override a named route. Like item_path is going to always go to items#show at /items/:id because we can't override it to be /:id or whatever the WCA needs"

jduss4 commented 7 years ago

This thread is for rails 3, but might have something we can try out: https://stackoverflow.com/questions/5997527/overriding-named-routes-provided-by-rails-3-engines

jduss4 commented 7 years ago

Okay, it looks like the app's routes load before the generator's routes load. This means that any gnarly code can be done on the engine's side and most casual users won't need to know about it.

There is likely a better way to do this, but I'm thinking...what if we run each of orchid's routes through a check. If there isn't currently a route with that name, we add it. If there is, we skip the orchid version (maybe outputting a warning about it being overridden?).

jduss4 commented 7 years ago

So, using the above idea about only loading orchid routes if no similarly named app route, I mocked up a branch:

https://github.com/CDRH/orchid/commit/47acaaf6a7c05265e95cec57c777314dc9cd2ed4

This does appear to work in general cases. The bad news is that Cather specifically wants the letters at /letters/let0183 which currently means the root of the rails app. This means that if we add the route get /:id, it's going to match on EVERYTHING. We ideally need it to be loaded last. My mockup branch wouldn't address that. I think we could probably use "match" instead of "get" to make sure that only urls matching letter ids would be caught in that first overridden rule in the app routes?

http://guides.rubyonrails.org/routing.html#advanced-constraints

jduss4 commented 7 years ago

I believe that this now addresses the above concern, also. My questions now are:

https://github.com/CDRH/orchid/tree/route_test

techgique commented 7 years ago

Code is reviewed and ready, but Jess said she'll move documentation to README.

We had discussed it before and I can't remember if we came to any conclusion. Do we want to add further constraints on the :id value? I recall not thinking it a good idea because one can't add anchors, but I read that all the constraints are anchored to the beginning of the string (http://guides.rubyonrails.org/routing.html#segment-constraints) so that's no longer a reason to dismiss them. I think we were just hoping to avoid all the unnecessary API calls and return 404s for bad IDs. Are there any or only a handfull of non-#### documents for it to retrieve?

jduss4 commented 7 years ago

I think I would prefer that the item path's id stays fairly open in orchid, but we could lock it down by app / project as needed?

techgique commented 7 years ago

Excellent point. I agree.

jduss4 commented 7 years ago

Can this issue be closed?

techgique commented 7 years ago

After I add documentation to the PR and we merge it :)

jduss4 commented 7 years ago

Oh! I thought all the routes stuff had already been merged, my bad

techgique commented 7 years ago

Oh, sorry I misread which issue this was when I posted that. There is the routing with Passenger deployment bug I'm working on now though, so that's a reason to leave it open. Will close this once that's sorted out.

techgique commented 7 years ago

Orchid routes don't work when the main app is deployed via Phusion Passenger because of how it spawns processes. It must be set to the 'direct' SpawnMethod for the routes to be present in the app. This can only be set vhost-wide and we don't want to do that for all future Orchid-based Rails apps. Looking into how to modify the code to be compatible.