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 script_level URLs from /stage to /lessons and /puzzle to /levels #38802
Changes from 41 commits
f5deb34
7460376
674e59e
835aba6
7e64153
604d6a3
cd895e6
9a04ca9
1d842a2
beb802f
5432081
22f70cb
0f553a6
1cfa2a5
acf71d3
4a80e85
391aa3b
ff4531b
f092ad2
313a74b
30c13bd
1f037ef
79873d7
1e7f736
04176fe
a7ec902
b259334
0cdd2aa
51269e4
1c61a62
e22193c
e8d5c21
7666333
e71b21a
0c92d6e
906fe01
bc6e6f7
c09e554
d3b0db5
1eb7d12
eb5d30e
39d4311
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 |
---|---|---|
|
@@ -23,19 +23,35 @@ def build_script_level_path(script_level, params = {}) | |
flappy_chapter_path(script_level.chapter, params) | ||
elsif params[:puzzle_page] | ||
if script_level.lesson.numbered_lesson? | ||
puzzle_page_script_stage_script_level_path(script_level.script, script_level.lesson, script_level, params[:puzzle_page]) | ||
puzzle_page_script_lesson_script_level_path(script_level.script, script_level.lesson, script_level, params[:puzzle_page]) | ||
else | ||
puzzle_page_script_lockable_stage_script_level_path(script_level.script, script_level.lesson, script_level, params[:puzzle_page]) | ||
end | ||
elsif params[:sublevel_position] | ||
sublevel_script_stage_script_level_path(script_level.script, script_level.lesson, script_level, params[:sublevel_position]) | ||
sublevel_script_lesson_script_level_path(script_level.script, script_level.lesson, script_level, params[:sublevel_position]) | ||
elsif !script_level.lesson.numbered_lesson? | ||
script_lockable_stage_script_level_path(script_level.script, script_level.lesson, script_level, params) | ||
elsif script_level.bonus | ||
query_params = params.merge(level_name: script_level.level.name) | ||
script_stage_extras_path(script_level.script.name, script_level.lesson.relative_position, query_params) | ||
script_lesson_extras_path(script_level.script.name, script_level.lesson.relative_position, query_params) | ||
else | ||
script_stage_script_level_path(script_level.script, script_level.lesson, script_level, params) | ||
script_lesson_script_level_path(script_level.script, script_level.lesson, script_level, params) | ||
end | ||
end | ||
|
||
# This is a a temporary method to help with moving translations onto the new level urls. To start this will | ||
# keep translations on the old URL until foundations can do the work to bring them over to the new url | ||
def build_script_level_path_for_translations(script_level) | ||
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. I noticed this doesn't have the exact same code as the old 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. Yes it is. I noticed that the translations call to build_script_level_path was not using the second paramater |
||
if script_level.script.name == Script::HOC_NAME | ||
hoc_chapter_path(script_level.chapter) | ||
elsif script_level.script.name == Script::FLAPPY_NAME | ||
flappy_chapter_path(script_level.chapter) | ||
elsif !script_level.lesson.numbered_lesson? | ||
script_lockable_stage_script_level_path(script_level.script, script_level.lesson, script_level) | ||
elsif script_level.bonus | ||
`/s/#{script_level.script.name}/stage/#{script_level.lesson.relative_position}/extras?level_name=#{script_level.level.name}` | ||
else | ||
`/s/#{script_level.script.name}/stage/#{script_level.lesson.relative_position}/puzzle/#{script_level.position}` | ||
end | ||
end | ||
|
||
|
@@ -178,7 +194,7 @@ def app_options | |
view_options(server_level_id: @level.id) | ||
if @script_level | ||
view_options( | ||
stage_position: @script_level.lesson.absolute_position, | ||
lesson_position: @script_level.lesson.absolute_position, | ||
level_position: @script_level.position, | ||
next_level_url: @script_level.next_level_or_redirect_path_for_user(current_user, @stage) | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -221,8 +221,8 @@ | |
|
||
# quick links for cartoon network arabic | ||
get '/flappy/lang/ar', to: 'home#set_locale', as: 'flappy/lang/ar', locale: 'ar-SA', user_return_to: '/flappy/1' | ||
get '/playlab/lang/ar', to: 'home#set_locale', as: 'playlab/lang/ar', locale: 'ar-SA', user_return_to: '/s/playlab/stage/1/puzzle/1' | ||
get '/artist/lang/ar', to: 'home#set_locale', as: 'artist/lang/ar', locale: 'ar-SA', user_return_to: '/s/artist/stage/1/puzzle/1' | ||
get '/playlab/lang/ar', to: 'home#set_locale', as: 'playlab/lang/ar', locale: 'ar-SA', user_return_to: '/s/playlab/lessons/1/levels/1' | ||
get '/artist/lang/ar', to: 'home#set_locale', as: 'artist/lang/ar', locale: 'ar-SA', user_return_to: '/s/artist/lessons/1/levels/1' | ||
|
||
# /lang/xx shortcut for all routes | ||
get '/lang/:locale', to: 'home#set_locale', user_return_to: '/' | ||
|
@@ -284,15 +284,15 @@ | |
end | ||
|
||
# CSP 20-21 lockable lessons with lesson plan redirects | ||
get '/s/csp1-2020/lockable/2(*all)', to: redirect(path: '/s/csp1-2020/stage/14%{all}') | ||
get '/s/csp2-2020/lockable/1(*all)', to: redirect(path: '/s/csp2-2020/stage/9%{all}') | ||
get '/s/csp3-2020/lockable/1(*all)', to: redirect(path: '/s/csp3-2020/stage/11%{all}') | ||
get '/s/csp4-2020/lockable/1(*all)', to: redirect(path: '/s/csp4-2020/stage/15%{all}') | ||
get '/s/csp5-2020/lockable/1(*all)', to: redirect(path: '/s/csp5-2020/stage/18%{all}') | ||
get '/s/csp6-2020/lockable/1(*all)', to: redirect(path: '/s/csp6-2020/stage/6%{all}') | ||
get '/s/csp7-2020/lockable/1(*all)', to: redirect(path: '/s/csp7-2020/stage/11%{all}') | ||
get '/s/csp9-2020/lockable/1(*all)', to: redirect(path: '/s/csp9-2020/stage/9%{all}') | ||
get '/s/csp10-2020/lockable/1(*all)', to: redirect(path: '/s/csp10-2020/stage/14%{all}') | ||
get '/s/csp1-2020/lockable/2(*all)', to: redirect(path: '/s/csp1-2020/lessons/14%{all}') | ||
get '/s/csp2-2020/lockable/1(*all)', to: redirect(path: '/s/csp2-2020/lessons/9%{all}') | ||
get '/s/csp3-2020/lockable/1(*all)', to: redirect(path: '/s/csp3-2020/lessons/11%{all}') | ||
get '/s/csp4-2020/lockable/1(*all)', to: redirect(path: '/s/csp4-2020/lessons/15%{all}') | ||
get '/s/csp5-2020/lockable/1(*all)', to: redirect(path: '/s/csp5-2020/lessons/18%{all}') | ||
get '/s/csp6-2020/lockable/1(*all)', to: redirect(path: '/s/csp6-2020/lessons/6%{all}') | ||
get '/s/csp7-2020/lockable/1(*all)', to: redirect(path: '/s/csp7-2020/lessons/11%{all}') | ||
get '/s/csp9-2020/lockable/1(*all)', to: redirect(path: '/s/csp9-2020/lessons/9%{all}') | ||
get '/s/csp10-2020/lockable/1(*all)', to: redirect(path: '/s/csp10-2020/lessons/14%{all}') | ||
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. we should consider whether these redirect requests should be cached. they should be easy enough to add to http_cache.rb, so that's what I would recommend. this could be done in a separate PR after this one. better to do after than before, since this PR changes the redirect destination, so might as well avoid ever telling browsers to cache the old value. 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. Tracked to follow up: https://codedotorg.atlassian.net/browse/PLAT-960 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. these redirects use status code 301, which some browsers cache indefinitely. I think we are good here, but here is me thinking aloud through a couple scenarios:
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. Thanks for thinking this through! 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. |
||
|
||
resources :lessons, only: [:edit, :update] | ||
|
||
|
@@ -320,6 +320,15 @@ | |
end | ||
end | ||
|
||
# Redirects from old /stage/x/extras url to new /lessons/x/extras url | ||
get '/s/:script_name/stage/:position/extras', to: redirect(path: '/s/%{script_name}/lessons/%{position}/extras') | ||
|
||
# Redirects from old /stage/x/puzzle url to new /lessons/x/levels url | ||
get '/s/:script_name/stage/:position/puzzle/(*all)', to: redirect(path: '/s/%{script_name}/lessons/%{position}/levels/%{all}') | ||
|
||
# Redirects from old /lockable/x/puzzle url to new /lockable/x/levels url | ||
get '/s/:script_name/lockable/:position/puzzle/(*all)', to: redirect(path: '/s/%{script_name}/lockable/%{position}/levels/%{all}') | ||
|
||
resources :scripts, path: '/s/' do | ||
# /s/xxx/reset | ||
get 'reset', to: 'script_levels#reset' | ||
|
@@ -335,31 +344,29 @@ | |
get 'instructions' | ||
end | ||
|
||
## TODO: Once we move levels over to /lessons as well combine the routing rules | ||
resources :lessons, only: [:show], param: 'position' do | ||
# /s/xxx/lessons/yyy | ||
resources :lessons, only: [:show], param: 'position', format: false do | ||
get 'student', to: 'lessons#student_lesson_plan' | ||
end | ||
|
||
# /s/xxx/stage/yyy/puzzle/zzz | ||
resources :stages, only: [], path: "/stage", param: 'position', format: false do | ||
get 'extras', to: 'script_levels#stage_extras', format: false | ||
get 'summary_for_lesson_plans', to: 'script_levels#summary_for_lesson_plans', format: false | ||
resources :script_levels, only: [:show], path: "/puzzle", format: false do | ||
|
||
# /s/xxx/lessons/yyy/levels/zzz | ||
resources :script_levels, only: [:show], path: "/levels", format: false do | ||
member do | ||
# /s/xxx/stage/yyy/puzzle/zzz/page/ppp | ||
# /s/xxx/lessons/yyy/levels/zzz/page/ppp | ||
get 'page/:puzzle_page', to: 'script_levels#show', as: 'puzzle_page', format: false | ||
# /s/xxx/stage/yyy/puzzle/zzz/sublevel/sss | ||
# /s/xxx/lessons/yyy/levels/zzz/sublevel/sss | ||
get 'sublevel/:sublevel_position', to: 'script_levels#show', as: 'sublevel', format: false | ||
end | ||
end | ||
end | ||
|
||
# /s/xxx/lockable/yyy/puzzle/zzz | ||
# /s/xxx/lockable/yyy/levels/zzz | ||
resources :lockable_stages, only: [], path: "/lockable", param: 'position', format: false do | ||
get 'summary_for_lesson_plans', to: 'script_levels#summary_for_lesson_plans', format: false | ||
resources :script_levels, only: [:show], path: "/puzzle", format: false do | ||
resources :script_levels, only: [:show], path: "/levels", format: false do | ||
member do | ||
# /s/xxx/stage/yyy/puzzle/zzz/page/ppp | ||
# /s/xxx/lockable/yyy/levels/zzz/page/ppp | ||
get 'page/:puzzle_page', to: 'script_levels#show', as: 'puzzle_page', format: false | ||
end | ||
end | ||
|
@@ -697,8 +704,8 @@ | |
get '/api/section_progress/:section_id', to: 'api#section_progress', as: 'section_progress' | ||
get '/dashboardapi/section_level_progress/:section_id', to: 'api#section_level_progress', as: 'section_level_progress' | ||
get '/api/user_progress/:script', to: 'api#user_progress', as: 'user_progress' | ||
get '/api/user_progress/:script/:stage_position/:level_position', to: 'api#user_progress_for_stage', as: 'user_progress_for_stage' | ||
get '/api/user_progress/:script/:stage_position/:level_position/:level', to: 'api#user_progress_for_stage', as: 'user_progress_for_stage_and_level' | ||
get '/api/user_progress/:script/:lesson_position/:level_position', to: 'api#user_progress_for_stage', as: 'user_progress_for_stage' | ||
get '/api/user_progress/:script/:lesson_position/:level_position/:level', to: 'api#user_progress_for_stage', as: 'user_progress_for_stage_and_level' | ||
put '/api/firehose_unreachable', to: 'api#firehose_unreachable' | ||
namespace :api do | ||
api_methods.each do |action| | ||
|
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.
we should consider whether we want to cache requests for the original
/s/.../stage/...
paths. @sureshc , any ideas how we can figure out if more complex patterns are allowed here other than a trailing wildcard? we'd ideally want something like/s/[a-z0-9-]+/stage/*
.I think this might be https://docs.aws.amazon.com/cloudfront/latest/APIReference/API_CacheBehavior.html --> PathPattern, in which case perhaps we could do
/s/*/stage/*/puzzle/*
. However, I'm not sure if (1) those are the right docs or (2) if we use this same config somewhere else outside of cloudfront.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.
Suresh provided some additional context in slack:
I think this means we could update http_cache.rb to filter on
/s/*/stage/*/puzzle/*
to maximize the caching efficiency of those pages.I am going to suggest that we try to do this in a follow-up task. this PR is already quite large and complex, and I don't think we will suffer any irreversible consequences by failing to cache these responses for a short period of time.
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.
Tracked to follow up: https://codedotorg.atlassian.net/browse/PLAT-982
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.
@wjordan @sureshc for any input on caching