Add no_redirect query param for course, script, and puzzle show routes #28311

maddiedierker · 2019-05-01T19:27:46Z

Adds the ability to override our course/script version redirect logic by adding the no_redirect query parameter to the request. This "redirect override" will then be stored in the Rails session to be remembered for the life of the session.

This is most useful for internal users (especially content translators) who need to visit an old or unstable course/script without being redirected. This is primarily useful for logged out users who want to visit these courses/scripts one or more times (since logged out users and students are the only users who are ever redirected away from courses/scripts).

To use this feature, visit a course, script, or level URL with the ?no_redirect=true query param:

studio.code.org/courses/csd-2017?no_redirect=true                 # course
studio.code.org/s/coursea-2017?no_redirect=true                   # script
studio.code.org/s/coursea-2017/stage/1/puzzle/1?no_redirect=true  # level

Redirect overrides are stored in the session as follows:

session[:course_version_overrides] = ['csp-2017', 'csd-2017']
session[:script_version_overrides] = ['coursea-2017', 'csp1-2017']

If a script has an override, its course (if present) is implicitly overridden as well, and vice versa.

Note: the value of this query param is ignored -- we just check for the presence of the no_redirect key in our params hash.

…equest

…quest

maddiedierker · 2019-05-01T20:11:35Z

dashboard/test/controllers/script_levels_controller_test.rb

@@ -382,10 +382,10 @@ def script_level_params(script_level)
  end

  test "show: redirect to latest stable script version in family for logged out user if one exists" do
-    courseg_2018 = create :script, name: 'courseg-2018', family_name: 'courseg', version_year: '2018'
-    Script.stubs(:latest_stable_version).returns(courseg_2018)
+    courseg_2017 = create :script, name: 'courseg-2017', family_name: 'courseg', version_year: '2017', is_stable: true


note: this is just clean-up, not changing test behavior

maddiedierker · 2019-05-01T20:11:39Z

dashboard/test/controllers/script_levels_controller_test.rb

  test "show: redirect to latest assigned script version in family for student if one exists" do
    sign_in @student

-    courseg_2018 = create :script, name: 'courseg-2018', family_name: 'courseg', version_year: '2018'
-    Script.stubs(:latest_assigned_version).returns(courseg_2018)
+    courseg_2017 = create :script, name: 'courseg-2017', family_name: 'courseg', version_year: '2017', is_stable: true


note: this is just clean-up, not changing test behavior

islemaster · 2019-05-01T21:48:49Z

I know Dave suggested an alternative at one point:

...remove the version redirect check on e.g. /s/coursea-2019/next. Once you let the user go to that url once, they'll have progress in that script, and will be allowed to go back to any part of it in the future.

On the surface it sounds simpler. As part of this PR, will you document why we chose this solution instead?

islemaster · 2019-05-01T21:58:25Z

dashboard/app/controllers/courses_controller.rb

@@ -62,7 +62,8 @@ def show
    end

    # Attempt to redirect user if we think they ended up on the wrong course overview page.
-    if redirect_course = redirect_course(course)
+    # Do not redirect users that have added the 'no_redirect' query parameter to the request.
+    if !params[:no_redirect] && redirect_course = redirect_course(course)


I'm tempted to push the no_redirect check down into the redirect_course helper (or redirect_script in other cases). I think it's because the rest of the "do we redirect" logic is hidden down in that layer. I don't feel too bad about referencing params from the helper, since in every case it's still implemented as part of the same controller.

That said, it wouldn't actually remove any duplication and might be considered surprising behavior. Your call.

islemaster · 2019-05-01T22:07:30Z

dashboard/test/controllers/courses_controller_test.rb

+
+    get :show, params: {course_name: 'csp-2017', no_redirect: "true"}
+
+    assert_response :ok


My own ignorance: I had to think carefully and look some things up to understand that this checks for exactly 200 OK, unlike assert_response :success which matches any 2XX, and that either would confirm that the response to this request was not a (3XX) redirect. 🤦‍♂️

maddiedierker · 2019-05-01T23:27:55Z

@islemaster re: your comment --

I know Dave suggested an alternative at one point:

...remove the version redirect check on e.g. /s/coursea-2019/next. Once you let the user go to that url once, they'll have progress in that script, and will be allowed to go back to any part of it in the future.

On the surface it sounds simpler. As part of this PR, will you document why we chose this solution instead?

part of the request from the international team (Amanda and I talked with Suky about it) was to have a single URL that would let the user bypass our redirect logic, so to achieve this, it seemed simplest to just ignore the redirect logic if the URL has ?no_redirect=true. that way, we can share links like https://studio.code.org/courses/csp-2017?no_redirect=true with translators

davidsbailey · 2019-05-02T00:10:51Z

I think the idea with using /next is that we wanted to let the user navigate around the course without getting sent back to the latest stable version. For example, if you send a signed-out user to https://studio.code.org/courses/csp-2019?no_redirect=true and then they click on the first unit, they will get sent back to csp-2018 since that is the latest stable version. is that outcome acceptable?

maddiedierker · 2019-05-02T00:28:45Z

good question, dave. i think that outcome is acceptable because it gives us the flexibility to send a user to a specific level, where they would "make progress" so that they are no longer redirected to the latest stable version (this, to me, is the same as using /next to get around version redirects), or any user can visit a course/script/level once with the no_redirect param if it's a one-time-only need.

davidsbailey · 2019-05-02T16:34:20Z

Hi Maddie, the behavior I'm seeing is that once you go to http://localhost-studio.code.org:3000/s/csd1-2019?no_redirect=true, there's no way to ' "make progress" so that they are no longer redirected to the latest stable version '. These are the steps I'm taking:

go to http://localhost-studio.code.org:3000/s/csd1-2019?no_redirect=true --> no redirect
click on the first puzzle or the continue button --> redirected to /s/csd1-2018

Is there a different way you've found to make progress from http://localhost-studio.code.org:3000/s/csd1-2019?no_redirect=true ? it's ok if this needs to be implemented across multiple PRs, but want to know that this builds in the right direction.

maddiedierker · 2019-05-02T18:23:32Z

ah, i'm sorry, i should have been more specific in my original comment. these are the steps i was thinking:

a user who wants to disable redirects permanently for a course or script would do the following:

go to an individual puzzle -- http://localhost-studio.code.org:3000/s/coursea-2017/stage/1/puzzle/1?no_redirect=true --> no redirect
go to any other course/script page -- http://localhost-studio.code.org:3000/s/coursea-2017 --> no redirect

visiting an individual will provide a permanent override for version redirects, or a user can visit something like http://localhost-studio.code.org:3000/s/coursea-2019?no_redirect=true for a one-time override (curriculum folks and other internal users have asked for this as well, especially for courses/scripts that are still in development like the 2019 versions)

davidsbailey · 2019-05-02T18:50:30Z

go to an individual puzzle -- http://localhost-studio.code.org:3000/s/coursea-2017/stage/1/puzzle/1?no_redirect=true --> no redirect
go to any other course/script page -- http://localhost-studio.code.org:3000/s/coursea-2017 --> no redirect

after pulling down your branch, when i follow these steps while signed out, I am not seeing the same result as you. I am getting redirected to coursea-2018. I think perhaps an additional step is needed, of finishing the level, so that progress is created. can you try the steps you listed in a fresh incognito window to determine if we're seeing the same thing?

maddiedierker · 2019-05-02T19:36:15Z

🤦‍♀ you're right, this solution only works for signed in users. unfortunately, we will have the same issue with the /next solution because the redirect code that checks if a user has progress relies on a user being signed in:

code-dot-org/dashboard/app/models/script.rb

Lines 566 to 567 in a4c8003

    
           has_progress = user.scripts.include?(self) || course&.has_progress?(user) 
        
           return true if has_progress

i'll talk to Amanda to figure out if this is a case we want to solve for (a signed out user who wants to permanently override versioning redirects for a course/script). if we do solve for this case, i see this path forward:

use a before_action to check for the no_redirect param. if it's present, set a cookie for the relevant course/script version
in the controller actions, check for the cookie (rather than the query param) to override redirect logic

the above sounds a little heavy-handed, but i can't think of a better solution... thoughts?

davidsbailey · 2019-05-02T20:30:57Z

ahh, that explains it, I had not tried for signed-in users. Signed-out users do have progress, but unfortunately it is stored in localStorage in their browser, which we can't access on the server.

Based on that, I think you are on the right track with using a cookie to persist this setting. However, it may be easier to use the rails session instead, since they are like cookies but hide more details away. Here is an example of rails session being used:

code-dot-org/dashboard/app/controllers/sections_controller.rb

Line 13 in 2c6c46b

session[:show_pairing_dialog] = true if params[:show_pairing_dialog]
code-dot-org/dashboard/app/controllers/api_controller.rb

Line 105 in 26467ef

show_pairing_dialog = !!session.delete(:show_pairing_dialog)

This article helped me compare the options: https://www.theodinproject.com/courses/ruby-on-rails/lessons/sessions-cookies-and-authentication

If you do go with a cookie instead, here is one thought:

normally we would set one cookie name per environment so that they did not get confused with each other. otherwise, it could cause some confusion for developers when they access a ?no_redirect url in one environment and it affects behavior in another

whichever approach you decide:

may want to ignore the cookie/session setting if the user is signed-in, since in that case we can determine reliably (using the logic already implemented in this PR) where the user belongs.

maddiedierker · 2019-05-02T20:38:20Z

ooo, i like the idea of using the rails session, so i'll try that first. thanks for your help on this!

…ame param

…n session

…irects

maddiedierker · 2019-05-15T18:32:13Z

@davidsbailey @islemaster I refactored the solution to store course/script version redirect overrides in the Rails session -- PTAL!

islemaster

Very cool - the Rails session seems like an elegant solution to this problem.

Will you add to the PR description an example case of where/why we'd want to use this queryparam?

islemaster · 2019-05-15T20:26:22Z

dashboard/lib/version_redirect_overrider.rb

@@ -0,0 +1,37 @@
+module VersionRedirectOverrider


It would be nice to have a file- or module-level comment here giving a prose description of the desired redirect logic, and/or a link out to a spec describing the desired behavior in detail. Even with the good tests you've written, I worry that the desired behavior is complex enough, and the related logic spread out enough, that it'll be challenging to reconstruct a mental model for these redirects in the future.

yes, good call! i'll add a description and some usage info to the module

davidsbailey

nice work Maddie! A few comments inline.

davidsbailey · 2019-05-15T23:27:21Z

dashboard/lib/version_redirect_overrider.rb

+    script_overrides = script_version_overrides(session)
+    course_overrides = course_version_overrides(session)
+
+    script_overrides.include?(script.name) || course_overrides.include?(script.course&.name)


nice call making this a module so it can be used many places.

there's a fair bit of complexity we're adding here, between storing lists of scripts + courses, and checking both before deciding whether to redirect a script or not. the simple alternative would be to just store one "no redirect" setting per user. I think the added complexity is worth it though, since we really want to the limit the chances that students will end up with progress in the wrong version of a course.

lastly, IIRC the docs say that things stored in the rails session expire when the browser window is closed, but that could be worth confirming -- it might be bad if one person accessing a no_redirect url on a shared computer made it behave that way forever for signed-out users.

davidsbailey · 2019-05-15T23:36:14Z

dashboard/test/controllers/script_levels_controller_test.rb

+      id: courseg_2017_stage_1_script_level.position,
+      no_redirect: "true"
+    }
+    assert_response :ok


optional: it's a bit hard to see that this is an apples-to-apples comparison against the following test which does the same but without no_redirect. my recommendation would be to either extract common setup steps, or to just drop this get / assert at the end of the following test so that you can see the no_redirect param is making the difference.

this PR has dragged on a bit already, so feel free to skip and keep in mind for next time.

davidsbailey · 2019-05-15T23:37:40Z

dashboard/test/controllers/scripts_controller_test.rb

+  end
+
+  test "show: do not redirect to latest stable version if no_redirect query param is supplied" do
+    get :show, params: {id: @coursez_2017.name, no_redirect: "true"}


similar to above, it would make this test case more clear to add a get / assert above this line showing the behavior without no_redirect

maddiedierker added 9 commits April 30, 2019 16:09

Do not redirect users from scripts if no_redirect query param is in r…

98a6a60

…equest

Do not redirect users from courses if no_redirect query param is in r…

19080c8

…equest

Do not redirect users from levels if no_redirect query param is in re…

0866676

…quest

Add unit tests for script version redirects

5b410eb

Add unit test for no_redirect query param in scripts#show

cd43a4f

Merge branch 'staging' into version-redirect-workaround

75f2a72

Unit test script_levels#show with no_redirect query param

c6e735e

Add unstable course to script_level#show version redirect tests

507698a

Unit test courses#show with no_redirect query param

abfea3f

maddiedierker commented May 1, 2019

View reviewed changes

maddiedierker requested review from islemaster and davidsbailey May 1, 2019 20:23

islemaster approved these changes May 1, 2019

View reviewed changes

maddiedierker added 6 commits May 13, 2019 13:24

Check session for overridden redirects in script_levels#show

1d00fe8

Add wrapper around set_script_redirect_override to obfuscate script n…

28a014f

…ame param

Check for redirect override in scripts#show

9613126

Check for version override in courses#show

233c2c1

Clean up VersionRedirectOverrider methods/naming

a877717

Check for overridden course scripts in override_course_redirect?

495e205

maddiedierker added 8 commits May 13, 2019 14:50

Merge branch 'staging' into version-redirect-workaround

56a0ef1

Remove outdated comment

475d788

Unit test that setting redirect override for course/script persists i…

12044ab

…n session

Update script_levels#show version redirect tests

e11dadb

Move checks for params[:no_redirect] into controllers

4c1983e

Refactor VersionRedirectOverrider to remove dependency on session

ff4342a

Unit test methods in VersionRedirectOverrider

047926f

Revert versioning redirect logic to if/elsif to avoid unnecessary red…

aa5fc33

…irects

islemaster approved these changes May 15, 2019

View reviewed changes

Add description to VersionRedirectOverrider module

62293d3

davidsbailey approved these changes May 15, 2019

View reviewed changes

Condense test cases for clarity

df196fd

maddiedierker merged commit 83879f5 into staging May 16, 2019

maddiedierker deleted the version-redirect-workaround branch May 16, 2019 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add no_redirect query param for course, script, and puzzle show routes #28311

Add no_redirect query param for course, script, and puzzle show routes #28311

maddiedierker commented May 1, 2019 •

edited

maddiedierker May 1, 2019

maddiedierker May 1, 2019

islemaster commented May 1, 2019

islemaster May 1, 2019

islemaster May 1, 2019

maddiedierker commented May 1, 2019

davidsbailey commented May 2, 2019

maddiedierker commented May 2, 2019

davidsbailey commented May 2, 2019

maddiedierker commented May 2, 2019

davidsbailey commented May 2, 2019

maddiedierker commented May 2, 2019 •

edited

davidsbailey commented May 2, 2019

maddiedierker commented May 2, 2019

maddiedierker commented May 15, 2019

islemaster left a comment

islemaster May 15, 2019

maddiedierker May 15, 2019

davidsbailey left a comment

davidsbailey May 15, 2019

davidsbailey May 15, 2019

davidsbailey May 15, 2019


		get :show, params: {course_name: 'csp-2017', no_redirect: "true"}

		assert_response :ok

Add no_redirect query param for course, script, and puzzle show routes #28311

Add no_redirect query param for course, script, and puzzle show routes #28311

Conversation

maddiedierker commented May 1, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islemaster commented May 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maddiedierker commented May 1, 2019

davidsbailey commented May 2, 2019

maddiedierker commented May 2, 2019

davidsbailey commented May 2, 2019

maddiedierker commented May 2, 2019

davidsbailey commented May 2, 2019

maddiedierker commented May 2, 2019 • edited

davidsbailey commented May 2, 2019

maddiedierker commented May 2, 2019

maddiedierker commented May 15, 2019

islemaster left a 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

Choose a reason for hiding this comment

maddiedierker commented May 1, 2019 •

edited

maddiedierker commented May 2, 2019 •

edited