I35 valkyrize hyku routes and redirects #2149
Conversation
The Hyrax generator does not generate presenters for resource work types so I'm adding them here and ensuring the respective resource work type controllers use them.
This commit will add redirects for the the work types so when viewing or editing them, the user will be redirected to the Valkyrie (Resource) version of the work type. The won't work unless turbolinks is disabled. We are opting for using an ENV variable to disable turbolinks for now so it can be easily removed in the future.
This allows for the redirects of AF works to Valkyrie works to function correctly because turbolinks was not allowing the redirect to happen. Ref: - notch8/hykuup_knapsack#100
This commit will move the turbolinks logic to a helper so that it can be be a little more sturdier than checking for the string values of the ENV variable especially because it is a boolean.
kirkkwang
force-pushed
the
i35-valkyrize-hyku-routes-and-redirects
branch
from
dfb7aa7 to
751b97c
Compare
kirkkwang
force-pushed
the
i35-valkyrize-hyku-routes-and-redirects
branch
from
9a6b1b6 to
dff0efd
Compare
|
This may or may not be related to this work. I took this branch for a spin and the routes are working now 🎉 But when I try to save a GenericWorkResource it fails because @form doesn't exist. Should valkyrie objects be routed to a different create? We can dig in tomorrow if you'd like. 
|
x 
|
kirkkwang
force-pushed
the
i35-valkyrize-hyku-routes-and-redirects
branch
from
dff0efd to
4137515
Compare
This commit will correct the logic because when our ENV var is true, turbolinks needs to be false and vice versa. Co-authored-by: Benjamin Kiah Stroud <32469930+bkiahstroud@users.noreply.github.com>
kirkkwang
force-pushed
the
i35-valkyrize-hyku-routes-and-redirects
branch
from
4137515 to
7c7699b
Compare
This refactor will simply remove the ENV variable have the helper method return false with a TODO. This should give us enough documentation in the future.
basically it introduced a lot of extra complexity that wasn't needed, instead we document why in the code comments |
There was a problem hiding this comment.
basically it introduced a lot of extra complexity that wasn't needed, instead we document why in the code comments
It certainly increases complexity, I agree with you there. However, there are significant benefits to using an ENV variable in a situation like this:
- We can toggle it on/off in rancher without changing any code. Even if we did want to change it in the code, it would be a single line, which we could do quickly and safely.
- Because this is Hyku, other institutions who are mid-migration can use this. In its current form, an institution mid-migration will not be able to upgrade their Hyku once this changes is removed.
There may be a couple other benefits, but those two are the most significant and relevant.
|
Cool that sounds good too. I'm going to have to bring in @jeremyf on this because I can go either way and I'll let the difference of opinions be put forth here so we have it tracked. I'll put this PR back into draft for now. |
There was a problem hiding this comment.
For the record, I agree with @jeremyf's suggested rename of the ENV variable to HYKU_BLOCK_VALKYRIE_REDIRECT. That way we don't have to flip the polarity
|
|
||
| resource = resource.to_s.underscore | ||
|
|
||
| get "/concern/#{resource}s/:id/edit", to: redirect("/concern/#{resource}_resources/%{id}/edit") |
There was a problem hiding this comment.
It's possible that we don't need to use a redirect but could instead specify the controller/action pair. Since this route is declared earlier than Hyrax, this route will take precedence.
Why avoid using a redirect? It saves at least one HTTP request.
There was a problem hiding this comment.
cool i'm not sure what that means but i'll do some research
This commit will bring back the ENV variable so that we have the option to turn back on the turbolinks without having to redeploy code.
🧹 Add custom presenters for resource work types
1ff3036
The Hyrax generator does not generate presenters for resource work
types so I'm adding them here and ensuring the respective resource work
type controllers use them.
🧹 Add redirects for work types
83e7dab
This commit will add redirects for the the work types so when viewing or
editing them, the user will be redirected to the Valkyrie (Resource)
version of the work type. The won't work unless turbolinks is disabled.
We are opting for using an ENV variable to disable turbolinks for now so
it can be easily removed in the future.
⚙️ Set TURBOLINKS_FOR_VALKYRIE to false
3f37899
This allows for the redirects of AF works to Valkyrie works to function
correctly because turbolinks was not allowing the redirect to happen.
Ref: