Skip to content

RFC#50-P5 Prepares the admin restructure to be tested#13114

Merged
Ridhwana merged 26 commits intoforem:masterfrom
Ridhwana:Ridhwana/rfc#50-admin-navigation-p5-take-2
Apr 15, 2021
Merged

RFC#50-P5 Prepares the admin restructure to be tested#13114
Ridhwana merged 26 commits intoforem:masterfrom
Ridhwana:Ridhwana/rfc#50-admin-navigation-p5-take-2

Conversation

@Ridhwana
Copy link
Contributor

@Ridhwana Ridhwana commented Mar 24, 2021

NOTE: Travis is bombing out on ActiveRecord::NoDatabaseError: FATAL: database "Forem_test" does not exist. Trying to figure out how to get a Travis build running, but in the meantime this PR can be reviewed.

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

It contains the following changes:

  • It fixes a bug where we was using as: on the scoped routes which caused the path helpers to need to prepended with the scope.
  • It replaces all instances of href or anything manual with link_to so that we're able to use dynamic path helpers
  • It replaces the render method in controllers to reference the action within the same controller instead of referencing a path helper.
  • It uses Rails 6.1 functionality to pull out admin routes into its own file. This allows the changes to be localized and old code can be easily deleted in the future.

What is the goal of this PR and what was the journey
The initial goal of this PR was to allow the old admin structure and the new one to co-exist, and for us to be able to toggle between the two with ease.

However, we encountered a pretty complex problem when trying to do this. Please read this comment to understand the problem we were encountering.

A short overview of the problem is outlined below (optional read):
When we toggle the feature flag it updates all the routes accordingly (i.e it adds the scope to the necessary routes when enabled), however the path helpers are not playing nicely with this change.

When we define scoped routes, it allows us to use the same path helpers as the old routes but a different route. As an example:

  • When the feature flag is enabled the scope is added to the routes admin/content_manager/articles and path helper admin_articles maps to admin/content_manager/articles.
  • When the feature flag is disabled the behaviour is such that the route is without a scope admin/article and path helper admin_articles maps to admin/articles.

However, it seems that with the dynamic routing (using constraints) it contains references to both the old routes and new scoped routes, and Rails is consuming the reference to the first path helper based on order in the routes file. The routes however are being honoured, its the path helpers that aren't.
As a result we obtaining the correct routing structure but the incorrect path helper.

Final decisions
Hence, taking into consideration the following:

  • we will not be running both the new admin routes and the old admin routes together except during the testing period, after which we will delete the old routes.
  • we have exhausted many avenues to try and get the path helpers to take direction from the Feature Flag

We've decided to just use an if statement to load the correct set of routes based on boot time.
This has the obvious problem of requiring an app reload to get the other routes after switching the flag, but since we are testing on a smaller Forem (and not DEV) this will not be an issue, and we can reload the rails app again after toggling the feature flag to get the path helpers pointing correctly.

Related Tickets & Documents

forem/rfcs#50
and 4 other earlier PR"s

QA Instructions, Screenshots, Recordings

  • Double check that the changes I've made to any hrefs and/or renders represent the original code
    -Click around the interface by enabling the admin_restructure feature flag and then restarting your server. Note the description section above for details on what I've covered in this PR.

UI accessibility concerns?

None, non-UI changes

Added tests?

Still need to fix them.

  • Yes
  • No, and there was no new functionality added but just refactors made.
  • I need help with writing tests

[Forem core team only] How will this change be communicated?

Will this PR introduce a change that impacts Forem members or creators, the
development process, or any of our internal teams? If so, please note how you
will share this change with the people who need to know about it.

  • I've updated the Developer Docs and/or
    Admin Guide, or
    Storybook (for Crayons components)
  • I've updated the README or added inline documentation
  • I will share this change in a Changelog
    or in a forem.dev post
  • I will share this change internally with the appropriate teams
  • I'm not sure how best to communicate this change and need help
  • This change does not need to be communicated, and this is why not:This change does not need to be communicated, and this is why not: It will be communicated internally once ready to test.

[optional] Are there any post deployment tasks we need to perform?

Yes the feature flag on a smaller Forem needs to be toggled and the rails app reloaded.

[optional] What gif best describes this PR or how it makes you feel?

alt_text

@pr-triage pr-triage bot added the PR: draft bot applied label for PR's that are a work in progress label Mar 24, 2021
@github-actions
Copy link
Contributor

Thank you for opening this PR! We appreciate you!

For all pull requests coming from third-party forks we will need to
review the PR before we can process it through our CI pipelines.

A Forem Team member will review this contribution and get back to
you as soon as possible!

# rubocop:disable Metrics/BlockLength
router.instance_exec do
# @ridhwana Feature Flag that implements the updated routes for the admin restructure is a work in progress.
constraints(->(_request) { FeatureFlag.enabled?(:admin_restructure) }) do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can remove this, still playing around with the code

@Ridhwana Ridhwana changed the title WIP: Ridhwana/rfc#50 admin navigation p5 take 2 WIP (not ready for review, testing): Ridhwana/rfc#50 admin navigation p5 take 2 Mar 24, 2021
module AdminRoutes
def self.extended(router)
# rubocop:disable Metrics/BlockLength
router.instance_exec do
Copy link
Contributor

Choose a reason for hiding this comment

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

well that's an exciting turn of events

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the old hacky way of splitting routes into several files, which we also use in forem-app. Once we upgraded to 6.1 we can use the new draw DSL method to get this behavior in a more standard way.

@Ridhwana Ridhwana changed the title WIP (not ready for review, testing): Ridhwana/rfc#50 admin navigation p5 take 2 WIP (not ready for review): Ridhwana/rfc#50 admin navigation p5 take 2 Mar 24, 2021
@Ridhwana Ridhwana changed the title WIP (not ready for review): Ridhwana/rfc#50 admin navigation p5 take 2 WIP (not ready for review): RFC#50-P5 Allow inner paths/routes to work with admin_restructure Feature Flag Mar 25, 2021
@Ridhwana
Copy link
Contributor Author

Ridhwana commented Apr 7, 2021

I thought I'd write out the issue I'm experiencing here to outline it for myself as well as see if anyone else has any ideas:

Context

We have the current admin routes (or what I like to call older admin routes) laid out in /routes/current_admin.rb.
We have the new admin routes (based on the new navigation restructure) laid out in /routes/admin.rb
Eventually /routes/current_admin.rb. will be deleted in favour of /routes/admin.rb and thee feature flag will be deleted once we test on production.

We then have a constraint in place that uses the correct set of routes based on whether we have the admin_restructure feature flag in place.

    constraints(->(_request) { !FeatureFlag.enabled?(:admin_restructure) }) do
        draw :current_admin
     end
    constraints(->(_request) { FeatureFlag.enabled?(:admin_restructure) }) do
        draw :admin
    end

Now, this works perfect for the routing!

Keep in mind that we use scopes in /routes/admin which allow us to update the route but with the same path helpers from admin/current_routes.rb.

So for example:

/admin/articles will have path helper admin_articles_path, but
/admin/content_manager/articles will also have path helper admin_articles_path.

It is done this way so that we can avoid going through all the files and changing the paths manually.

Problem

Now the problem is with the path helpers.

Rails load all the routes at the application boot time, however we expect that since we have a constraint in place it will allow us to reference the dynamic segments. And it does! for the routes. i.e. it matches the routes correctly based on the constraint.

However it does honor the constraint with the path helpers. Instead it grabs the path helper of the first matched one from ALL the routes top to bottom.

The way to see this in action is to switch the feature flag on and then load the application. The correct routes will load, but if you go to the overview page, you will notice that the old path helpers (e.g. /admin/tags) are being loaded. This is because draw :current_admin is loaded at the top. If you swap the order around, you will now see the routing still work correctly, however if you disable the Feature Flag you will see the new (scope) path helpers (e.g. /admin/content_manager/tags) on the overview page, because now draw :admin would be at the top.

To understand more - this stack overflow post may do a better job at explaining the problem than I can:

Potential Solutions

Looking at the above stack overflow thread and this one, I can create a file called helpers/admin_restructure_url_helper.rb which would be temporary until we delete all code, and in this file do something like below:

  def admin_tags_path
    if FeatureFlag.enabled?(:admin_restructure)
      '/admin/content_manager/tags'
    else
      '/admin/tags'
    end
  end

This seems like an extremely tedious (and possibly error prone) solution because we'd need to have a corresponding method for EVERY admin path helper.

However, I've been reading through tons of documentation and trying different things in an attempt to perhaps hook into a method each time any admin helper is called, and then somehow have access to both loaded routes route to the correct one based on the Feature Flag.

@rhymes
Copy link
Contributor

rhymes commented Apr 7, 2021

@Ridhwana it seems like Rails doesn't leave you with much of an option since they're loaded at boot time. What might be worth to investigate is: does Rails have a clean interface that calls at boot to generate helpers? Can we somehow call it again at runtime to swap them?

I haven't dug in but Rails 6.1 has an undocumented method that might help

https://github.com/rails/rails/blob/1b455e2e9d6937d4107e19cb32e2f98aa08886b9/actionpack/lib/action_dispatch/routing/route_set.rb#L486

maybe after calling draw() in the constraints you can rebuild the URL helpers with something like that. I imagine it's a bit trickier than this, since the generic url_helpers method caches them:

https://github.com/rails/rails/blob/1b455e2e9d6937d4107e19cb32e2f98aa08886b9/actionpack/lib/action_dispatch/routing/route_set.rb#L478-L484

Hope this can help 🙌

@Ridhwana
Copy link
Contributor Author

Ridhwana commented Apr 7, 2021

Thanks so much for taking the time to provide feedback @rhymes! I'll spend the afternoon looking into your suggestions and see if I can come up with something that works.

If it doesn't work out, I'll just go about using the more manual route and override all the admin path helpers in a separate helper file that can be deleted (along with all the other old code) after testing.

The problem we're experiencing now regarding the first path helper being referenced should not be an issue in production once we remove the Feature Flag (in the next week hopefully - after testing) because we'll be removing the old (i.e. the current) routes, and replacing them with the new scoped ones. Hence, there would not be two of the same path helpers. Hence, I think it would be okay if the solution implemented here would be temporary and localized to one file should an alternate solution be too time consuming.

UPDATE: I'd time boxed this to the afternoon and tried to rebuild the helpers at runtime to swap the path helpers, but I was not winning. For the reasons outlined above, I'm going to work on a manual solution like this tomorrow.

@citizen428
Copy link
Contributor

citizen428 commented Apr 8, 2021

Ugh, this seems like a complicated problem. A few parts that could help:

  • Rails.application.reload_routes! reloads all routes without needing to restart the app
  • Routes can be added to programmatically, e.g.
    Rails.application.routes.draw { draw :current_admin }
  • We already do some routes trickery in config/application.rb in an after_initialize block for adding routes for reserved words.

So I wonder if the following is feasible:

  1. Don't draw any of these routes by default
  2. Add a new after_initialize block to config/application.rb, something like:
    config.after_initialize do
      admin_routes = FeatureFlag.enabled?(:admin_restructure) ? :admin : :current_admin
      Rails.application.routes.draw { draw admin_routes }
      Rails.application.reload_routes!
    end

If this works we could even re-execute this block when the feature flag gets switched, but I think this will happen rarely enough that an app restart to get the other routes wouldn't be the end of the world.

@Ridhwana
Copy link
Contributor Author

Ridhwana commented Apr 8, 2021

Thanks @citizen428! I tried this, googled some more and read more documentation but couldn't get it working. It doesn't seem to load the routes from the after initialize block and spits other related errors as well :( .

@citizen428
Copy link
Contributor

citizen428 commented Apr 9, 2021

@Ridhwana Shame this didn't work, I'm tempted to play around with it a bit myself but mostly just because I'm curious what the problem is. On a related note: we currently have two blocks with two different constraints:

      constraints(->(_request) { !FeatureFlag.enabled?(:admin_restructure) }) do
        draw :current_admin
      end
      constraints(->(_request) { FeatureFlag.enabled?(:admin_restructure) }) do
        draw :admin
      end

Have you considered a single block instead?

admin_routes = FeatureFlag.enabled?(:admin_restructure) ? :admin : :current_admin
draw admin_routes

This has the obvious problem of requiring an app reload to get the other routes after switching the flag, but how likely is this to happen? If we are primarily using new admin on DEV, maybe this is a feasible solution that keeps all other Forem on the old admin without needing any path helper shenanigans.

@Ridhwana
Copy link
Contributor Author

Ridhwana commented Apr 11, 2021

I'm tempted to play around with it a bit myself

Please feel free to if you feel like it :) This one has been a tough one to crack.

This has the obvious problem of requiring an app reload to get the other routes after switching the flag, but how likely is this to happen? If we are primarily using new admin on DEV, maybe this is a feasible solution that keeps all other Forem on the old admin without needing any path helper shenanigans.

@citizen428 I did consider this, however during the testing phase it would be ideal for the community success team to be able to toggle the feature flag to the old admin interface and continue their work in the case of finding a bug on the new routes. In the case that the feature flag cannot be toggled (without an app reload) and something critical requires admin intervention (and if the new routes don't work) then we are put at a possible disadvantage.

This may not be critical though if its on a Forem like forem.team. We could test thoroughly on there and then if necessary move this over to DEV and finally remove the routes. So perhaps this could be an option.

I spent some more time this weekend trying different solutions to solve the problem and came up with the latest WIP commit. Its a bit of a hacky workaround where the new admin paths are prepended with the scope and then when the feature flag is toggled I call the new helper methods instead of reusing the same helper methods across both sets of routes. Its also all confined to one file which we can then remove once testing is over.

…old helpers to the new ones if the FF is toggled
@citizen428
Copy link
Contributor

I did consider this, however during the testing phase it would be ideal for the community success team to be able to toggle the feature flag to the old admin interface and continue their work in the case of finding a bug on the new routes

I agree. But given the relative complexity of workarounds explored so far I was wondering how much more effort we should spend on this or if it's good enough to leave the flag off on DEV for a bit longer, then get this tested on a different community and only enable it on DEV once we're reasonably sure that no/only small bugs are left.


def deduced_controller(request)
request.path.split("/").last
request.path.split("/").fourth
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though this seems a bit hacky it makes more sense to get the deduced controller and scope from left to right to account for nested routes

<img class="mx-auto mt-3" width="40" height="40" src="<%= badge_achievement.badge.badge_image %>" alt="badge image">
</td>
<td><%= link_to "Remove", url_for(action: :destroy, id: badge_achievement.id), method: :delete, data: { confirm: "Are you sure?" }, class: "btn btn-danger" %></td>
<td><%= link_to "Remove", admin_badge_achievement_path(badge_achievement), class: "crayons-btn crayons-btn--danger", method: :delete, data: { confirm: "Are you sure?" } %></td>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed most things to use the path helpers which makes it a lot easier to refactor routes

<h3 class="crayons-subtitle-1">Welcome Checklist</h3>
<p class="crayons-subtitle-4 color-base-70 mb-3">Here are a few things we recommend doing now that you're using Forem!</p>
<ol>
<li class="mb-1 -ml-4">Set up and configure your Forem under <a href="/admin/config" data-action="click->ahoy#trackOverviewLink">Config</a></li>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, removed hrefs in favor of link_to

config/routes.rb Outdated
# It will require the rails app to be reloaded when the feature flag is toggled
# You can find more details on why we had to implement it this way in this PR
# https://github.com/forem/forem/pull/13114
admin_routes = FeatureFlag.enabled?(:admin_restructure) ? :admin : :current_admin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used rails 6.1 draw routes to maintain separate files of the current admin routes and the new admin routes. It will make it easier to delete the old routes post testing.

@Ridhwana Ridhwana changed the title WIP (not ready for review): RFC#50-P5 Allow inner paths/routes to work with admin_restructure Feature Flag RFC#50-P5 Prepares the codebase to be tested Apr 13, 2021
@Ridhwana Ridhwana marked this pull request as ready for review April 13, 2021 16:44
@Ridhwana Ridhwana requested a review from a team as a code owner April 13, 2021 16:44
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: draft bot applied label for PR's that are a work in progress labels Apr 13, 2021
@Ridhwana Ridhwana changed the title RFC#50-P5 Prepares the codebase to be tested RFC#50-P5 Prepares the admin restructure to be tested Apr 13, 2021
Copy link
Contributor

@mstruve mstruve left a comment

Choose a reason for hiding this comment

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

Some really great refactoring happening here. The deduced scope method makes me a little leary but I can deal with it.

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Apr 13, 2021
Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

Clicked around, flipped the swiched, clicked around. Everything works!

Great job @Ridhwana!

@Ridhwana
Copy link
Contributor Author

Ridhwana commented Apr 14, 2021

Soooo I looked deeper into the Travis builds,
I could run my test suite fine and re-create my test db too. However, after trying again today to drop and recreate my db it bombed out locally with the ffg:

DEPRECATION WARNING: Devise::Models::Authenticatable::BLACKLIST_FOR_SERIALIZATION is deprecated! Use Devise::Models::Authenticatable::UNSAFE_ATTRIBUTES_FOR_SERIALIZATION instead. (called from const_get at /Users/ridhwana/Projects/DEV/forem/vendor/cache/devise-0cd72a56f984/lib/devise/models.rb:90)
rake aborted!
ActiveRecord::NoDatabaseError: FATAL:  database "Forem_test" does not exist
/Users/ridhwana/Projects/DEV/forem/app/services/feature_flag.rb:3:in `enabled?'
/Users/ridhwana/Projects/DEV/forem/config/routes.rb:87:in `block (3 levels) in <main>'
/Users/ridhwana/Projects/DEV/forem/config/routes.rb:40:in `block (2 levels) in <main>'
/Users/ridhwana/Projects/DEV/forem/config/routes.rb:26:in `block in <main>'
/Users/ridhwana/Projects/DEV/forem/config/routes.rb:3:in `<main>'
/Users/ridhwana/Projects/DEV/forem/config/application.rb:75:in `block in <class:Application>'
/Users/ridhwana/Projects/DEV/forem/config/environment.rb:5:in `<main>'
/Users/ridhwana/.rbenv/versions/2.7.2/bin/bundle:23:in `load'
/Users/ridhwana/.rbenv/versions/2.7.2/bin/bundle:23:in `<main>'

Caused by:
PG::ConnectionBad: FATAL:  database "Forem_test" does not exist
/Users/ridhwana/Projects/DEV/forem/app/services/feature_flag.rb:3:in `enabled?'
/Users/ridhwana/Projects/DEV/forem/config/routes.rb:87:in `block (3 levels) in <main>'
/Users/ridhwana/Projects/DEV/forem/config/routes.rb:40:in `block (2 levels) in <main>'
/Users/ridhwana/Projects/DEV/forem/config/routes.rb:26:in `block in <main>'
/Users/ridhwana/Projects/DEV/forem/config/routes.rb:3:in `<main>'
/Users/ridhwana/Projects/DEV/forem/config/application.rb:75:in `block in <class:Application>'
/Users/ridhwana/Projects/DEV/forem/config/environment.rb:5:in `<main>'
/Users/ridhwana/.rbenv/versions/2.7.2/bin/bundle:23:in `load'
/Users/ridhwana/.rbenv/versions/2.7.2/bin/bundle:23:in `<main>'
Tasks: TOP => db:create => db:load_config => environment
(See full trace by running task with --trace)

After some more tracing it seems to be these lines of code causing the issue:

 admin_routes = FeatureFlag.enabled?(:admin_restructure) ? :admin : :current_admin
 draw admin_routes

☝️ kind of the main purpose of the PR.

Because if I change it to this:

admin_routes = true ? :admin : :current_admin
draw admin_routes

I'm able to drop and recreate the db.

My guess is that since the above lines are not in a constraint its trying to reference FeatureFlag.enabled? before its somehow initialized in the test env. I'm not certain of this as yet.

I need to go back to the drawing board and figure out how to fix this.
(I can't wrap it in a constraint because then it would load all the path helpers again on boot and we'd be back at the start)

"This PR is the gift that keeps giving" 😅

@citizen428
Copy link
Contributor

citizen428 commented Apr 15, 2021

@Ridhwana You're in luck because when I hit a similar problem during profile work I built a solution we can use for all scenarios where DB setup fails because some code relies on the presence of a table.

diff --git a/config/routes.rb b/config/routes.rb
index 40ff964a8..59d275779 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -84,8 +84,10 @@
       # It will require the rails app to be reloaded when the feature flag is toggled
       # You can find more details on why we had to implement it this way in this PR
       # https://github.com/forem/forem/pull/13114
-      admin_routes = FeatureFlag.enabled?(:admin_restructure) ? :admin : :current_admin
-      draw admin_routes
+      if Database.table_exists?("flipper_features")
+        admin_routes = FeatureFlag.enabled?(:admin_restructure) ? :admin : :current_admin
+        draw admin_routes
+      end
     end

This way loading these routes will be skipped when the backing Flipper table doesn't exist, so during tasks like db:create and db:setup. If you start the app afterward the routes will be added. Here you see me hitting a breakpoint after I dropped and recreated the database:

image

You probably should add a comment about Database.table_exists? and why it is necessary here.

@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Apr 15, 2021
@Ridhwana
Copy link
Contributor Author

Ridhwana commented Apr 15, 2021

@Ridhwana You're in luck because when I hit a similar problem during profile work I built a solution we can use for all scenarios where DB setup fails because some code relies on the presence of a table.

@citizen428 you're the best - for hitting this problem before, building this solution, and then pointing me to it 😁 - thank you!!

@Ridhwana Ridhwana merged commit cf52075 into forem:master Apr 15, 2021
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: unreviewed bot applied label for PR's with no review labels Apr 15, 2021
citizen428 pushed a commit that referenced this pull request Apr 19, 2021
* feat: make the sidebar more dynamic

* refactor: use the action in the same controller instead of the whole path

* feat: remove hardcoded routes

* feat: move routes into file

* feat: use rails 6 draw for the admin routes

* add a helper method

* oops: fix super

* feat: add the hacky helper methods :( )

* WIP: created different path helpers for the new routes and point the old helpers to the new ones if the FF is toggled

* feat: update the module

* chore: add new paths

* feat: change link_to's use paths instead

* feat: feedback_messages to scoped admin route

* chore: update the feature flag urls helpers

* feat: feedback_messages issue

* chore: remove all the workarounds

* chore: rubucop

* fix: oops remove helper

* chore: comment out the tests that touch the tabbed navbar which is affected by the rails application needing to be reloaded

* feat: ensure that we chcek if the db table exists
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: merged bot applied label for PR's that are merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants