Update routes for code docs launch, using a dcdo flag #45145

bethanyaconnor · 2022-03-03T22:24:25Z

Adds a DCDO flag to toggle where we serve code docs from. At first, it will continue to proxy these pages from curriculum builder. When the DCDO flag is flipped, it will use the levelbuilder versions of this page. I chose to use a DCDO flag as with my team moves, it's a lower lift to launch these pages whenever we like.

The implication of the way I've done this change is that the /docs URLs will only work for applab, gamelab, spritelab, and weblab code docs. These urls are difficult to handle in our Rails application, as we will continue to proxy /docs/concepts, /docs/csd, /docs/csd-1718, etc and I was having trouble coming up with a good allow-list for all the various routes we use (but I could if needed). Starting with javalab, I'd like to start using more Rails-friendly routes for these pages.

This PR will cause an eyes diff as I switched out one of the allthethings levels. The only time we have used a code docs page in a CurriculumReference/Map level was this allthethings level, so I switched it to use a reference guide instead.

Testing story

a fair number of existing tests tested this functionality, added more unit tests, manual testing

Deployment strategy

This should be safe to deploy as the page doesn't change, just the controller action. This is also not a route that is used for ajax requests on a page.

Follow-up work

after these pages are launched, there's a bit of clean up to do here

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

… and grabbing the title in the level

…d of a code doc

dmcavoy · 2022-03-28T19:19:57Z

This PR will cause an eyes diff as I switched out one of the allthethings levels. The only time we have used a code docs page in a CurriculumReference/Map level was this allthethings level, so I switched it to use a reference guide instead.

Since we will continue to have old Map levels which point to CB I think it would be better to leave this level and then add a new one that uses the new reference guides

dmcavoy · 2022-03-28T19:25:29Z

dashboard/app/controllers/programming_environments_controller.rb

-      return
-    end
-    programming_environment.assign_attributes(programming_environment_params.except(:categories))
+    return render :not_found unless @programming_environment


love that you moved to using @programming_environment here. You can also move this not_found check into set_programming_environment so you don't have to check it in each controller.

dmcavoy · 2022-03-28T19:28:29Z

dashboard/config/routes.rb

@@ -62,7 +62,10 @@

  get 'redirected_url', to: 'redirect_proxy#get', format: false

-  get 'docs/', to: 'curriculum_proxy#get_doc_landing'
+  get 'docs/', to: 'programming_environments#docs_index'


Are there unit tests that could be added to check that all of these URLs go to the right place and that ones that should go to CB for concept maps aren't impacted?

I forgot to add one for this route but the fact that this test still passes is evidence that concept maps aren't affected.

Great. Thanks for pointing to existing tests that are covering this

hmm, my understanding is that controller tests do not look at our routing logic, so I think that test is verifying that the curriculum proxy controller does the right thing, but not that the right requests will get routed to the curriculum proxy controller. to test the routing, you'd need an integration test which operates on urls, maybe like one of these:

code-dot-org/dashboard/test/integration/home_test.rb

Line 19 in 6040b86

get '/home'

code-dot-org/dashboard/test/integration/redirects_test.rb

Line 5 in 6a9ea77

get '/beta'

given how much complexity there is in the routing logic, I think it would be a good idea to have at least one integration test verifying that the fallback to /docs/concepts on CB is working end-to-end.

bethanyaconnor · 2022-03-29T16:38:30Z

Since we will continue to have old Map levels which point to CB I think it would be better to leave this level and then add a new one that uses the new reference guides

I just updated this level to point to a /docs/concepts route off curriculum builder. The reason I did this is that no levels use /docs/ in a map level so it didn't seem super helpful to test. I can add a new level though if it would help to keep this consistent, but I definitely think we need a Map level for /docs/concepts to be tested

…missing tests

dmcavoy · 2022-03-29T17:09:25Z

Since we will continue to have old Map levels which point to CB I think it would be better to leave this level and then add a new one that uses the new reference guides

I just updated this level to point to a /docs/concepts route off curriculum builder. The reason I did this is that no levels use /docs/ in a map level so it didn't seem super helpful to test. I can add a new level though if it would help to keep this consistent, but I definitely think we need a Map level for /docs/concepts to be tested

Ah ok I understand the difference now. It's a Map Level with a Code Doc. My head had not switched out of Ref Guides world so thank you for explaining

dmcavoy · 2022-03-29T17:11:35Z

@davidsbailey I would love for you to take a look too as I'm not super familiar with DCDO flags and proxied urls.

davidsbailey

sorry for the long delay. DCDO code and tests look good!

davidsbailey · 2022-03-30T17:46:53Z

dashboard/config/routes.rb

+  get 'docs/', to: 'programming_environments#docs_index'
+  get 'docs/:programming_environment_name', to: 'programming_environments#docs_show', constraints: {programming_environment_name: /(applab|gamelab|spritelab|weblab)/}
+  get 'docs/:programming_environment_name/:programming_expression_key', constraints: {programming_environment_name: /(applab|gamelab|spritelab|weblab)/, programming_expression_key: /#{CurriculumHelper::KEY_CHAR_RE}+/}, to: 'programming_expressions#docs_show'
+  get 'docs/:programming_environment_name/:programming_expression_key/index.html', constraints: {programming_environment_name: /(applab|gamelab|spritelab|weblab)/, programming_expression_key: /#{CurriculumHelper::KEY_CHAR_RE}+/}, to: 'programming_expressions#docs_show'
  get 'docs/*path', to: 'curriculum_proxy#get_doc'


am I correct in understanding that this line handles the following routes mentioned in the PR description?

we will continue to proxy /docs/concepts, /docs/csd, /docs/csd-1718, etc and I was having trouble coming up with a good allow-list for all the various routes we use (but I could if needed).

if so, this would be helpful to note in a comment on this line. also, what is the endgame for /docs/csd and /docs/csd-1718 -- will we continue to proxy these requests to curriculum builder indefinitely?

@tim-dot-org , heads up that you'll need to modify this routing when you launch concept maps. it looks like you should just be able to add any new routing rules above this line.

Yep these routes only direct requests to /docs/{applab,gamelab,spritelab,weblab} to the new controllers. Everything else will still go to curriculum_proxy. Because there are all these other routes in our curriculum, I figured it would be better to explicitly list these exceptions.

I don't know what the endgame is for those routes. Older levels, such as https://levelbuilder-studio.code.org/s/csd2-2019/lessons/3/levels/7, link to these routes in the "Help and Tips" tab, but it looks like some of them were replaced by concept maps (eg the same level in 2020). It might be possible to pull support for these routes but a. I'm not sure and b. this doesn't seem like something we'll have time for before curriculum launch.

davidsbailey · 2022-03-30T17:52:40Z

dashboard/config/routes.rb

@@ -62,7 +62,10 @@

  get 'redirected_url', to: 'redirect_proxy#get', format: false

-  get 'docs/', to: 'curriculum_proxy#get_doc_landing'
+  get 'docs/', to: 'programming_environments#docs_index'


hmm, my understanding is that controller tests do not look at our routing logic, so I think that test is verifying that the curriculum proxy controller does the right thing, but not that the right requests will get routed to the curriculum proxy controller. to test the routing, you'd need an integration test which operates on urls, maybe like one of these:

code-dot-org/dashboard/test/integration/home_test.rb

Line 19 in 6040b86

get '/home'

code-dot-org/dashboard/test/integration/redirects_test.rb

Line 5 in 6a9ea77

get '/beta'

given how much complexity there is in the routing logic, I think it would be a good idea to have at least one integration test verifying that the fallback to /docs/concepts on CB is working end-to-end.

bethanyaconnor · 2022-03-30T21:05:46Z

hmm, my understanding is that controller tests do not look at our routing logic, so I think that test is verifying that the curriculum proxy controller does the right thing, but not that the right requests will get routed to the curriculum proxy controller.

If I change these tests to have a random name for the programming environments, the tests actually fail with ActionController::UrlGenerationError: No route matches {:action=>"docs_show", :controller=>"programming_environments", :programming_environment_name=>"programming-environment-7"}, possible unmatched constraints: [:programming_environment_name]. That being said, an integration test sounds like a good move to make sure this works long term so I'll add that

davidsbailey · 2022-03-30T23:03:20Z

dashboard/test/integration/curriculum_docs_test.rb

+  test '/docs/applab is routed to ProgrammingEnvironmentsController' do
+    assert_recognizes(
+      {controller: 'programming_environments', action: 'docs_show', programming_environment_name: 'applab'},
+      {path: '/docs/applab', method: :get}


nice pattern! TIL

bethanyaconnor force-pushed the launch-code-docs-dcdo branch from d6f9e3e to 2059beb Compare March 17, 2022 18:42

bethanyaconnor force-pushed the launch-code-docs-dcdo branch from 05844e2 to f6c5f01 Compare March 25, 2022 15:23

bethanyaconnor added 5 commits March 28, 2022 07:41

Update routes for code docs launch, using a dcdo flag

7c69b1b

Various fixes to handle the /docs/csf routes, the /index.html routes,…

1bf7626

… and grabbing the title in the level

Restrict /docs URLs to just applab, gamelab, spritelab, and weblab

9e04b83

Fix proxied environment landing pages and add some tests

25923f4

Change the allthethings map level to be an old reference guide instea…

e40af56

…d of a code doc

bethanyaconnor force-pushed the launch-code-docs-dcdo branch from 8ee6ccc to e40af56 Compare March 28, 2022 14:42

bethanyaconnor marked this pull request as ready for review March 28, 2022 16:12

bethanyaconnor requested a review from a team March 28, 2022 16:12

dmcavoy reviewed Mar 28, 2022

View reviewed changes

PR feedback: move not_found into set_programming_environment and add …

2d3d6ca

…missing tests

davidsbailey self-requested a review March 29, 2022 18:27

davidsbailey reviewed Mar 30, 2022

View reviewed changes

Merge branch 'staging' into launch-code-docs-dcdo

94cffd3

Add comments and tests

9a02677

bethanyaconnor requested a review from davidsbailey March 30, 2022 22:23

davidsbailey approved these changes Mar 30, 2022

View reviewed changes

bethanyaconnor merged commit 42d98de into staging Mar 31, 2022

bethanyaconnor deleted the launch-code-docs-dcdo branch March 31, 2022 15:36

bethanyaconnor mentioned this pull request Mar 31, 2022

Revert "Update routes for code docs launch, using a dcdo flag" #45598

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update routes for code docs launch, using a dcdo flag #45145

Update routes for code docs launch, using a dcdo flag #45145

bethanyaconnor commented Mar 3, 2022 •

edited

dmcavoy commented Mar 28, 2022

dmcavoy Mar 28, 2022

dmcavoy Mar 28, 2022

bethanyaconnor Mar 29, 2022

dmcavoy Mar 29, 2022

davidsbailey Mar 30, 2022

bethanyaconnor commented Mar 29, 2022

dmcavoy commented Mar 29, 2022

dmcavoy commented Mar 29, 2022

davidsbailey left a comment

davidsbailey Mar 30, 2022

bethanyaconnor Mar 30, 2022

davidsbailey Mar 30, 2022

bethanyaconnor commented Mar 30, 2022

davidsbailey Mar 30, 2022

Update routes for code docs launch, using a dcdo flag #45145

Update routes for code docs launch, using a dcdo flag #45145

Conversation

bethanyaconnor commented Mar 3, 2022 • edited

Testing story

Deployment strategy

Follow-up work

PR Checklist:

dmcavoy commented Mar 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bethanyaconnor commented Mar 29, 2022

dmcavoy commented Mar 29, 2022

dmcavoy commented Mar 29, 2022

davidsbailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bethanyaconnor commented Mar 30, 2022

Choose a reason for hiding this comment

bethanyaconnor commented Mar 3, 2022 •

edited