-
Notifications
You must be signed in to change notification settings - Fork 479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update routes for code docs launch, using a dcdo flag #45145
Changes from all commits
7c69b1b
1bf7626
9e04b83
25923f4
e40af56
2d3d6ca
94cffd3
9a02677
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,7 +62,16 @@ | |
|
||
get 'redirected_url', to: 'redirect_proxy#get', format: false | ||
|
||
get 'docs/', to: 'curriculum_proxy#get_doc_landing' | ||
# We moved code docs off of curriculum builder in spring 2022. | ||
# In that move, we wanted to preserve the previous /docs routes for these | ||
# pages. However, there are a lot of other /docs URLs that did not move over | ||
# so we're allow-listing the four IDEs that lived on curriculum builder to be | ||
# served by ProgrammingEnvironmentsController and ProgrammingExpressionsController, | ||
# with the rest falling back to the old proxying logic. | ||
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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. am I correct in understanding that this line handles the following routes mentioned in the PR description?
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
get 'curriculum/*path', to: 'curriculum_proxy#get_curriculum' | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to add one for this route but the fact that this test still passes is evidence that concept maps aren't affected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Thanks for pointing to existing tests that are covering this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
code-dot-org/dashboard/test/integration/redirects_test.rb
Line 5 in 6a9ea77
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.