Skip to content
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

Merged
merged 42 commits into from Apr 20, 2021
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
f5deb34
Update script level urls from stage to lesson and lockable to survey
dmcavoy Jan 29, 2021
7460376
Improve the redirect routes
dmcavoy Jan 29, 2021
674e59e
Update a bunch of tests that reference /stage
dmcavoy Jan 29, 2021
835aba6
Update all references to /stage or /lockable in code base
dmcavoy Jan 29, 2021
7e64153
Revert "Update all references to /stage or /lockable in code base"
dmcavoy Jan 29, 2021
604d6a3
Update the URLs in UI tests
dmcavoy Jan 30, 2021
cd895e6
Update a bunch of hard coded references to /stage
dmcavoy Jan 30, 2021
9a04ca9
Remove unwanted change
dmcavoy Jan 30, 2021
1d842a2
More updates to UI tests
dmcavoy Jan 30, 2021
beb802f
Fix file that got messed up in rebase
dmcavoy Feb 3, 2021
5432081
Merge branch 'staging' into update-script-level-url
dmcavoy Feb 3, 2021
22f70cb
Merge branch 'staging' into update-script-level-url
dmcavoy Feb 4, 2021
0f553a6
Make sure factory for lesson has a default for has_lesson_plan
dmcavoy Feb 4, 2021
1cfa2a5
Merge branch 'staging' into update-script-level-url
dmcavoy Mar 30, 2021
acf71d3
Put lockable back since we decided against survey
dmcavoy Mar 30, 2021
4a80e85
Clean up routes file
dmcavoy Mar 30, 2021
391aa3b
Change lesson to lessons
dmcavoy Mar 30, 2021
ff4531b
Update comments and student lesson plan route
dmcavoy Mar 30, 2021
f092ad2
Move over from stages to lessons
dmcavoy Mar 30, 2021
313a74b
Fix up stage extras to make the path work
dmcavoy Mar 30, 2021
30c13bd
Fix up the redirect from stage to lessons
dmcavoy Mar 30, 2021
1f037ef
Move /puzzle to /levels
dmcavoy Mar 30, 2021
79873d7
Update some routes with stage_position to lesson_position
dmcavoy Mar 30, 2021
1e7f736
Fix some tests for script and script levels
dmcavoy Mar 30, 2021
04176fe
Fix lockable_stage_position
dmcavoy Mar 31, 2021
a7ec902
change puzzle to levels in another couple places
dmcavoy Mar 31, 2021
b259334
Merge branch 'staging' into update-script-level-url
dmcavoy Apr 7, 2021
0cdd2aa
Create helper method to keep translation keys on old urls
dmcavoy Apr 7, 2021
51269e4
Update a couple new places that urls were added in UI tests
dmcavoy Apr 7, 2021
1c61a62
Remove path since it now matches the automatic one from rails
dmcavoy Apr 7, 2021
e22193c
Fix route to work for redirect
dmcavoy Apr 9, 2021
e8d5c21
Update references to stagePosition in apps
dmcavoy Apr 10, 2021
7666333
Add a test to make sure we don't hit database when redirecting
dmcavoy Apr 10, 2021
e71b21a
Add some checks that redirects are working
dmcavoy Apr 10, 2021
0c92d6e
Merge branch 'staging' into update-script-level-url
dmcavoy Apr 10, 2021
906fe01
Add test for lockable redirect and add lockable redirect
dmcavoy Apr 12, 2021
bc6e6f7
Add test for stage extras and make sure they redirect
dmcavoy Apr 12, 2021
c09e554
Merge branch 'staging' into update-script-level-url
dmcavoy Apr 14, 2021
d3b0db5
Add test for page redirect
dmcavoy Apr 16, 2021
1eb7d12
Merge branch 'staging' into update-script-level-url
dmcavoy Apr 16, 2021
eb5d30e
Merge branch 'staging' into update-script-level-url
dmcavoy Apr 17, 2021
39d4311
Merge branch 'staging' into update-script-level-url
dmcavoy Apr 20, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/i18n/i18n_script_utils.rb
Expand Up @@ -128,7 +128,7 @@ def self.contains_malformed_link_or_image(translation)

def self.get_level_url_key(script, level)
script_level = level.script_levels.find_by_script_id(script.id)
path = script_level.build_script_level_path(script_level)
path = script_level.build_script_level_path_for_translations(script_level)
URI.join("https://studio.code.org", path)
end

Expand Down
6 changes: 3 additions & 3 deletions cookbooks/cdo-varnish/libraries/http_cache.rb
Expand Up @@ -20,8 +20,8 @@ class HttpCache
# A list of script levels that should not be cached, even though they are
# in a cacheable script, because they are project-backed.
UNCACHED_SCRIPT_LEVEL_PATHS = [
'/s/dance/stage/1/puzzle/13',
'/s/dance-2019/stage/1/puzzle/10'
'/s/dance/lessons/1/levels/13',
'/s/dance-2019/lessons/1/levels/10'
]

# A map from script name to script level URL pattern.
Expand All @@ -41,7 +41,7 @@ class HttpCache
oceans
).map do |script_name|
# Most scripts use the default route pattern.
[script_name, "/s/#{script_name}/stage/*"]
[script_name, "/s/#{script_name}/lessons/*"]
Copy link
Member

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.

Copy link
Member

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:

Suresh (he/him):lightning: 6 hours ago
I believe so. Here’s where we generate the cache behavior CloudFormation template logic

def self.cache_behavior(behavior_config, path=nil)

Suresh (he/him):lightning: 6 hours ago
I always start with the CloudFormation template reference for a particular AWS service https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-distribution-cachebehavior.html

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.

Copy link
Contributor Author

@dmcavoy dmcavoy Apr 16, 2021

Choose a reason for hiding this comment

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

Copy link
Member

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

end.to_h.merge(
# Add the "special case" routes here.
'hourofcode' => '/hoc/*',
Expand Down
12 changes: 6 additions & 6 deletions cookbooks/cdo-varnish/test/shared/shared.rb
Expand Up @@ -253,7 +253,7 @@ def self.setup(environment, helpers, cloudfront=false)
it 'Handles Accept-Language behaviors' do
skip 'Not implemented in Rack yet' unless environment.to_s == 'integration'
# URL contains whitelisted 'Accept-Language' header
url = build_url 's/starwars/stage/1/puzzle'
url = build_url 's/starwars/lessons/1/levels'
text_en = 'Hello World!'
text_fr = 'Bonjour le Monde!'
mock_response url, text_en, {'X-Varnish-Accept-Language' => 'en'}
Expand Down Expand Up @@ -490,17 +490,17 @@ def does_not_strip_cookies_from_png_in_path(path)
end

it 'Strips cookies from the penultimate dance level' do
assert strips_session_specific_cookies_from_request? '/s/dance/stage/1/puzzle/12'
assert strips_session_specific_cookies_from_request? '/s/dance-2019/stage/1/puzzle/9'
assert strips_session_specific_cookies_from_request? '/s/dance/lessons/1/levels/12'
assert strips_session_specific_cookies_from_request? '/s/dance-2019/lessons/1/levels/9'
end

it 'Does not strip cookies from the last dance level' do
refute strips_session_specific_cookies_from_request? '/s/dance/stage/1/puzzle/13'
refute strips_session_specific_cookies_from_request? '/s/dance-2019/stage/1/puzzle/10'
refute strips_session_specific_cookies_from_request? '/s/dance/lessons/1/levels/13'
refute strips_session_specific_cookies_from_request? '/s/dance-2019/lessons/1/levels/10'
end

it 'Strips cookies from an aquatic level' do
assert strips_session_specific_cookies_from_request? '/s/aquatic/stage/1/puzzle/5'
assert strips_session_specific_cookies_from_request? '/s/aquatic/lessons/1/levels/5'
end

# rubocop:disable Lint/NestedMethodDefinition
Expand Down
2 changes: 1 addition & 1 deletion dashboard/app/controllers/api_controller.rb
Expand Up @@ -422,7 +422,7 @@ def user_progress_for_stage
response[:signedIn] = !current_user.nil?

script = Script.get_from_cache(params[:script])
stage = script.lessons[params[:stage_position].to_i - 1]
stage = script.lessons[params[:lesson_position].to_i - 1]
script_level = stage.cached_script_levels[params[:level_position].to_i - 1]
level = params[:level] ? Script.cache_find_level(params[:level].to_i) : script_level.oldest_active_level

Expand Down
12 changes: 6 additions & 6 deletions dashboard/app/controllers/script_levels_controller.rb
Expand Up @@ -165,8 +165,8 @@ def show
def self.get_script_level(script, params)
if params[:chapter]
script.get_script_level_by_chapter(params[:chapter])
elsif params[:stage_position]
script.get_script_level_by_relative_position_and_puzzle_position(params[:stage_position], params[:id], false)
elsif params[:lesson_position]
script.get_script_level_by_relative_position_and_puzzle_position(params[:lesson_position], params[:id], false)
elsif params[:lockable_stage_position]
script.get_script_level_by_relative_position_and_puzzle_position(params[:lockable_stage_position], params[:id], true)
else
Expand Down Expand Up @@ -231,7 +231,7 @@ def stage_extras
return head :not_found if ScriptConstants::FAMILY_NAMES.include?(params[:script_id])

@script = Script.get_from_cache(params[:script_id])
@stage = @script.lesson_by_relative_position(params[:stage_position].to_i)
@stage = @script.lesson_by_relative_position(params[:lesson_position].to_i)

if params[:id]
@script_level = Script.cache_find_script_level params[:id]
Expand All @@ -247,7 +247,7 @@ def stage_extras
return
end

@stage = Script.get_from_cache(params[:script_id]).lesson_by_relative_position(params[:stage_position].to_i)
@stage = Script.get_from_cache(params[:script_id]).lesson_by_relative_position(params[:lesson_position].to_i)
@script = @stage.script
@stage_extras = {
next_stage_number: @stage.next_level_number_for_lesson_extras(current_user),
Expand All @@ -269,8 +269,8 @@ def summary_for_lesson_plans
script = Script.get_from_cache(params[:script_id])

stage =
if params[:stage_position]
script.lesson_by_relative_position(params[:stage_position])
if params[:lesson_position]
script.lesson_by_relative_position(params[:lesson_position])
else
script.lesson_by_relative_position(params[:lockable_stage_position], true)
end
Expand Down
26 changes: 21 additions & 5 deletions dashboard/app/helpers/levels_helper.rb
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The 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 build_script_level_path. Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 param and that means it would never hit certain cases. So I just kept the cases that it would hit

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

Expand Down Expand Up @@ -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)
)
Expand Down
4 changes: 2 additions & 2 deletions dashboard/app/helpers/script_levels_helper.rb
Expand Up @@ -20,9 +20,9 @@ def script_level_solved_response(response, script_level)
stage_extras: true
).any?
if enabled_for_stage && (enabled_for_user || enabled_for_teacher)
response[:redirect] = script_stage_extras_path(
response[:redirect] = script_lesson_extras_path(
script_id: script_level.script.name,
stage_position: (@stage || script_level.lesson).absolute_position
lesson_position: (@stage || script_level.lesson).absolute_position
)
end
end
Expand Down
2 changes: 1 addition & 1 deletion dashboard/app/helpers/view_options_helper.rb
Expand Up @@ -25,7 +25,7 @@ module ViewOptionsHelper
:server_project_level_id,
:game_display_name,
:script_name,
:stage_position,
:lesson_position,
dmcavoy marked this conversation as resolved.
Show resolved Hide resolved
:level_position,
:public_caching,
:is_13_plus,
Expand Down
2 changes: 1 addition & 1 deletion dashboard/app/models/lesson.rb
Expand Up @@ -303,7 +303,7 @@ def summarize(include_bonus_levels = false, for_edit: false)
lesson_data[:finishText] = I18n.t('nav.header.finished_hoc')
end

lesson_data[:lesson_extras_level_url] = script_stage_extras_url(script.name, stage_position: relative_position) unless unplugged_lesson?
lesson_data[:lesson_extras_level_url] = script_lesson_extras_url(script.name, lesson_position: relative_position) unless unplugged_lesson?

lesson_data
end
Expand Down
2 changes: 1 addition & 1 deletion dashboard/app/models/levels/bubble_choice.rb
Expand Up @@ -55,7 +55,7 @@ def sublevel_at(index)
end

# Returns a sublevel's position in the parent level. Can be used for generating
# a sublevel URL (/s/:script_name/stage/:stage_pos/puzzle/:puzzle_pos/sublevel/:sublevel_pos).
# a sublevel URL (/s/:script_name/lessons/:stage_pos/levels/:puzzle_pos/sublevel/:sublevel_pos).
# @param [Level] sublevel
# @return [Integer] The sublevel's position (i.e., its index + 1) under the parent level.
def sublevel_position(sublevel)
Expand Down
4 changes: 2 additions & 2 deletions dashboard/app/models/script_level.rb
Expand Up @@ -256,7 +256,7 @@ def next_level_or_redirect_path_for_user(
elsif bonus
# If we got to this bonus level from another lesson's lesson extras, go back
# to that lesson
script_stage_extras_path(script.name, (extras_lesson || lesson).relative_position)
script_lesson_extras_path(script.name, (extras_lesson || lesson).relative_position)
else
level_to_follow ? build_script_level_path(level_to_follow) : script_completion_redirect(script)
end
Expand Down Expand Up @@ -527,7 +527,7 @@ def self.summarize_as_bonus_for_teacher_panel(script, bonus_level_ids, student)
{
# Some lessons have a lesson extras option without any bonus levels. In
# these cases, they just display previous lesson challenges. These should
# be displayed as "perfect." Example level: /s/express-2020/stage/28/extras
# be displayed as "perfect." Example level: /s/express-2020/lessons/28/extras
id: '-1',
bonus: true,
user_id: student.id,
Expand Down
51 changes: 26 additions & 25 deletions dashboard/config/routes.rb
Expand Up @@ -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: '/'
Expand Down Expand Up @@ -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}')
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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:

  1. the redirect from /s/csp1-2020/lockable/2/puzzle/1 to /s/csp1-2020/stage/14/puzzle/1 may be cached by some browsers. the other routes you are adding in this PR will then redirect from /s/csp1-2020/stage/14/puzzle/1 to /s/csp1-2020/lessons/14/levels/1. this seems correct.

  2. if someone hits /s/csp1-2020/lockable/2/levels/1, it would be bad if the old redirect were cached, because that would send them to /s/csp1-2020/stage/14/levels/1, which would 404, because stage + levels is never handled. It seems very unlikely for someone to have this redirect cached though, because they would only be able to cache the response to an individual request, not the entire pattern of /s/csp1-2020/lockable/2(*all), and there is no point in time at which our servers should ever have sent anyone to /s/csp1-2020/lockable/2/levels/1. This is because we stopped sending people anywhere within /s/csp1-2020/lockable/2/ in Send users to new CSP 20-21 URLs for lockable lessons with lesson plans and redirect old URL #39128, and we won't be sending anyone to /levels/ urls until after this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for thinking this through!

Copy link
Member

Choose a reason for hiding this comment

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

@wjordan @sureshc again for any input on caching the "lockable" routes


resources :lessons, only: [:edit, :update]
resources :resources, only: [:create, :update]
Expand All @@ -308,6 +308,9 @@
end
end

# Redirects from old /stage url to new /lessons url for levels
get '/s/:script_name/stage/:position/puzzle/:number(*all)', to: redirect(path: '/s/%{script_name}/lessons/%{position}/levels/%{number}%{all}')

resources :scripts, path: '/s/' do
# /s/xxx/reset
get 'reset', to: 'script_levels#reset'
Expand All @@ -323,31 +326,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], path: "/lessons", param: 'position', format: false do
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, is it necessary to specify path here (since it matches the resource name)?

Also, I noticed that we also have edit and update actions defined for lessons above on 297. Is it intentional that these are separate? (I could imagine us wanting to keep those closer to the other edit/update entries.)

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'll try removing path since we are no longer using stage you are probably right its no longer needed. I'll also take a look at the other lesson entries to see if I can bring them closer together

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I know why I left the other lessons stuff separate. Its because when you are editing or updating you are not using the nested route inside the script like we do for showing.

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
Expand Down Expand Up @@ -685,8 +686,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|
Expand Down
Expand Up @@ -251,7 +251,7 @@ class Api::V1::AssessmentsControllerTest < ActionController::TestCase
"stage" => script.name,
"puzzle" => 1,
"question" => "Long assessment 1",
"url" => "http://test.host/s/#{script.name}/stage/1/puzzle/1?section_id=#{@section.id}&user_id=#{@student_1.id}",
"url" => "http://test.host/s/#{script.name}/lessons/1/levels/1?section_id=#{@section.id}&user_id=#{@student_1.id}",
"multi_correct" => 1,
"multi_count" => 4,
"match_correct" => 1,
Expand Down Expand Up @@ -335,7 +335,7 @@ class Api::V1::AssessmentsControllerTest < ActionController::TestCase
"stage" => script.name,
"puzzle" => 1,
"question" => "Long assessment 1",
"url" => "http://test.host/s/#{script.name}/stage/1/puzzle/1?section_id=#{@section.id}&user_id=#{@student_1.id}",
"url" => "http://test.host/s/#{script.name}/lessons/1/levels/1?section_id=#{@section.id}&user_id=#{@student_1.id}",
"multi_correct" => 1,
"multi_count" => 2,
"match_correct" => 0,
Expand Down