Update script_level URLs from /stage to /lessons and /puzzle to /levels #38802

dmcavoy · 2021-01-29T04:05:42Z

Proposal Of URL Changes

As we bring content over from CB to LB we have been considering how to improve our URL scheme to better support our curriculum model. The new lesson plans which have been brought over are going to live at /s/<script name and year>/lesson/<relative_position>. However current the levels that will be in these lessons are at /s/<script name and year>/stage/<relative_position>/puzzle/<position>. In order to make our URLs more consistent we would like to move the levels to be at /s/<script name and year>/lessons/<relative_position>/levels/<position>.

Summary

Levels in lessons that are not lockable or have a lesson plan will move from .../stage/... to .../lessons/... and .../puzzle/... to .../levels/...

Overview of Code Changes

The core of the change in this PR is in routes.rb. That is where the updates to the script_level routes can be found as well as the new redirects that will redirect old URLs to the new URL.

0cdd2aa creates a helper method so that level keys for translations stay on the old URLs for now until foundations has a chance to move them over to the new system.

All of the other changes were to get tests passing. There are A LOT of UI tests where we are updating the URL that we are navigating to.

Links

spec
jira

Testing story

Added a couple new tests for the translations helper method
Updated URLs in UI tests to the new URLs to get tests passing
Other tests updates in order to get tests to pass

We have a lot of existing tests covering script levels especially in script_levels_controller_test.rb so I am relying on those for unit testing. We will also be doing 2 bug bashes of this before launching.

Reviewer 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

This reverts commit 2f189d4.

dashboard/config/routes.rb

dmcavoy · 2021-04-07T03:21:09Z

Adhoc: https://adhoc-update-script-level-url-studio.cdn-code.org

jamescodeorg

I don't really understand what's going on with the translation stuff but the core route changes look good to me and I'm assuming drone will catch any issues with the tests.

Thank you so much for doing this! This is difficult, tedious work and this is a huge step forward towards unifying our urls and getting rid of the 'stage' terminology in our codebase.

jamescodeorg · 2021-04-07T17:06:25Z

dashboard/config/routes.rb

-    ## 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


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

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

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.

davidsbailey

Nice job pulling this all together, Dani! I have some comments around testing and caching.

I think it will also be important to think about "deployment strategy", i.e. is it safe to deploy this entire PR at once, or is there some place where we need to point people to a new route before taking away an old one. since you are replacing the old routes with redirects, I think we are generally good there. Just thought I'd mention this in case it gives other reviewers any ideas.

dashboard/test/integration/caching_test.rb

davidsbailey · 2021-04-08T00:23:35Z

cookbooks/cdo-varnish/libraries/http_cache.rb

@@ -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/*"]


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.

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

code-dot-org/lib/cdo/aws/cloudfront.rb

Line 229 in 98b78da

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.

Tracked to follow up: https://codedotorg.atlassian.net/browse/PLAT-982

@wjordan @sureshc for any input on caching

davidsbailey · 2021-04-08T00:26:16Z

dashboard/config/routes.rb

+  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}')


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.

Tracked to follow up: https://codedotorg.atlassian.net/browse/PLAT-960

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:

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.

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.

Thanks for thinking this through!

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

dashboard/app/helpers/view_options_helper.rb

dashboard/test/integration/redirects_test.rb

dmcavoy · 2021-04-13T17:50:58Z

@daynew @sureshc Just wanted to check in. Do you think you will have a chance to look at this PR this sprint?

davidsbailey

Great work Dani! I am nervous about this change, but I've checked all the things i can think of to check and I can't think of any other precautions to take. LGTM!

davidsbailey · 2021-04-16T00:09:19Z

cookbooks/cdo-varnish/libraries/http_cache.rb

@@ -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/*"]


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

code-dot-org/lib/cdo/aws/cloudfront.rb

Line 229 in 98b78da

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.

dashboard/test/integration/redirects_test.rb

davidsbailey · 2021-04-16T00:59:50Z

dashboard/config/routes.rb

+  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}')


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:

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.

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.

daynew · 2021-04-16T19:02:16Z

dashboard/app/helpers/levels_helper.rb

+
+  # 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)


I noticed this doesn't have the exact same code as the old build_script_level_path. Is that intentional?

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

dmcavoy · 2021-04-19T20:49:32Z

@daynew @davidsbailey @wjordan @sureshc @jamescodeorg I'm planning to ship this change this week once PM gives the thumbs up on one last thing. Is there anything from you all that you would like me to check before merging? Any other considerations here?

davidsbailey

LGTM! I've tagged Suresh and Will in places related to caching. If they don't have time to look this week, then it should be ok to proceed, and we can address any concerns in follow-up work.

davidsbailey · 2021-04-19T21:20:06Z

cookbooks/cdo-varnish/libraries/http_cache.rb

@@ -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/*"]


@wjordan @sureshc for any input on caching

davidsbailey · 2021-04-19T21:20:38Z

dashboard/config/routes.rb

+  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}')


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

dmcavoy requested a review from a team as a code owner January 29, 2021 20:25

dmcavoy added 9 commits February 2, 2021 21:51

Update script level urls from stage to lesson and lockable to survey

f5deb34

Improve the redirect routes

7460376

Update a bunch of tests that reference /stage

674e59e

Update all references to /stage or /lockable in code base

835aba6

Revert "Update all references to /stage or /lockable in code base"

7e64153

This reverts commit 2f189d4.

Update the URLs in UI tests

604d6a3

Update a bunch of hard coded references to /stage

cd895e6

Remove unwanted change

9a04ca9

More updates to UI tests

1d842a2

dmcavoy force-pushed the update-script-level-url branch from e1acc7c to 1d842a2 Compare February 3, 2021 02:56

dmcavoy removed the request for review from a team February 3, 2021 02:57

Fix file that got messed up in rebase

beb802f

dmcavoy commented Feb 3, 2021

View reviewed changes

dashboard/config/routes.rb Outdated Show resolved Hide resolved

dmcavoy commented Feb 3, 2021

View reviewed changes

dashboard/config/routes.rb Outdated Show resolved Hide resolved

dmcavoy changed the title ~~Update script level url~~ Update script_level URLs from /stage to /lesson Feb 3, 2021

dmcavoy added 3 commits February 2, 2021 23:13

Merge branch 'staging' into update-script-level-url

5432081

Merge branch 'staging' into update-script-level-url

22f70cb

Make sure factory for lesson has a default for has_lesson_plan

0f553a6

dmcavoy marked this pull request as draft February 9, 2021 04:06

dmcavoy added 8 commits March 30, 2021 13:24

Merge branch 'staging' into update-script-level-url

1cfa2a5

Put lockable back since we decided against survey

acf71d3

Clean up routes file

4a80e85

Change lesson to lessons

391aa3b

Update comments and student lesson plan route

ff4531b

Move over from stages to lessons

f092ad2

Fix up stage extras to make the path work

313a74b

Fix up the redirect from stage to lessons

30c13bd

dmcavoy changed the title ~~Update script_level URLs from /stage to /lesson~~ Update script_level URLs from /stage to /lessons and /puzzle to /levels Mar 30, 2021

Move /puzzle to /levels

1f037ef

dmcavoy requested a review from a team April 7, 2021 01:59

Update a couple new places that urls were added in UI tests

51269e4

jamescodeorg approved these changes Apr 7, 2021

View reviewed changes

Remove path since it now matches the automatic one from rails

1c61a62

davidsbailey reviewed Apr 8, 2021

View reviewed changes

dmcavoy added 7 commits April 9, 2021 15:58

Fix route to work for redirect

e22193c

Update references to stagePosition in apps

e8d5c21

Add a test to make sure we don't hit database when redirecting

7666333

Add some checks that redirects are working

e71b21a

Merge branch 'staging' into update-script-level-url

0c92d6e

Add test for lockable redirect and add lockable redirect

906fe01

Add test for stage extras and make sure they redirect

bc6e6f7

dmcavoy requested a review from davidsbailey April 12, 2021 21:55

Merge branch 'staging' into update-script-level-url

c09e554

davidsbailey approved these changes Apr 16, 2021

View reviewed changes

Add test for page redirect

d3b0db5

dmcavoy requested review from wjordan and removed request for sureshc April 16, 2021 16:38

daynew reviewed Apr 16, 2021

View reviewed changes

daynew approved these changes Apr 16, 2021

View reviewed changes

dmcavoy added 2 commits April 16, 2021 17:26

Merge branch 'staging' into update-script-level-url

1eb7d12

Merge branch 'staging' into update-script-level-url

eb5d30e

davidsbailey approved these changes Apr 19, 2021

View reviewed changes

Merge branch 'staging' into update-script-level-url

39d4311

dmcavoy merged commit 00a06c3 into staging Apr 20, 2021

dmcavoy deleted the update-script-level-url branch April 20, 2021 18:51

dmcavoy mentioned this pull request Apr 23, 2021

update urls to use lessons and levels instead of stage and puzzle code-dot-org/curriculumbuilder#358

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update script_level URLs from /stage to /lessons and /puzzle to /levels #38802

Update script_level URLs from /stage to /lessons and /puzzle to /levels #38802

dmcavoy commented Jan 29, 2021 •

edited

dmcavoy commented Apr 7, 2021

jamescodeorg left a comment

jamescodeorg Apr 7, 2021

dmcavoy Apr 7, 2021

dmcavoy Apr 7, 2021

davidsbailey left a comment

davidsbailey Apr 8, 2021

davidsbailey Apr 16, 2021

dmcavoy Apr 16, 2021 •

edited

davidsbailey Apr 19, 2021

davidsbailey Apr 8, 2021

dmcavoy Apr 10, 2021

davidsbailey Apr 16, 2021

dmcavoy Apr 16, 2021

davidsbailey Apr 19, 2021

dmcavoy commented Apr 13, 2021

davidsbailey left a comment

davidsbailey Apr 16, 2021

davidsbailey Apr 16, 2021

daynew Apr 16, 2021

dmcavoy Apr 16, 2021

dmcavoy commented Apr 19, 2021

davidsbailey left a comment

davidsbailey Apr 19, 2021

davidsbailey Apr 19, 2021

Update script_level URLs from /stage to /lessons and /puzzle to /levels #38802

Update script_level URLs from /stage to /lessons and /puzzle to /levels #38802

Conversation

dmcavoy commented Jan 29, 2021 • edited

Proposal Of URL Changes

Summary

Overview of Code Changes

Links

Testing story

Reviewer Checklist:

dmcavoy commented Apr 7, 2021

jamescodeorg 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

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

dmcavoy Apr 16, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmcavoy commented Apr 13, 2021

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

Choose a reason for hiding this comment

dmcavoy commented Apr 19, 2021

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

dmcavoy commented Jan 29, 2021 •

edited

dmcavoy Apr 16, 2021 •

edited