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
Get levels in last lesson of CSP 20-21 units (except unit 8) by absolute position #39035
Conversation
@davidsbailey can you take a look at this? I know I need to add the experiment flag here but wanted to check this is the right direction |
Reminder - Don't forget to check with Dayne on the translations for this |
Actually @davidsbailey do you think we can go without the experiment? You mentioned that in your initial comment. |
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.
@dmcavoy if the steps without an experiment are:
- make new urls start working using absolute_position, not relative_position
- start sending users to new urls (e.g. users who click a link on script overview page)
- redirect from old urls to new urls (e.g. users who have an old url bookmarked or stored in an LMS)
- update relative_position in the DB
- go back to routing via relative_position, not absolute_position
then it looks like this PR does steps 1 and 2. This seems risky to me, because if you had to revert this PR after it reached production, you'd end up with some CSP test takers pointing at the new URLs, which would then stop working when you reverted.
I think the safest bet will be to do step 1 first in isolation, which in this case would be just the changes to get_script_level
, but not the changes in build_script_level_path
. then test in production to make sure everything is working. that way, when you go to do steps 2 and 3 (either together or in isolation), if anything goes wrong you can simply revert them, and both old and new urls will keep working.
@davidsbailey I think I have updated this PR to just do step 1. Let me know what you think and also what you think might be the best tests for this PR. You can also see the 4 follow up PRs now linked in the description if you want to look what follows this. |
id: @lockable_level_group_sl.position | ||
} | ||
|
||
assert_redirected_to '/s/csp2-2020/lockable/1/puzzle/1/page/1' |
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.
ahh, I had not thought about the subpage urls. these urls will need to also work at their new locations before the step of "make new urls start working" can be considered to be complete. can you please add tests showing that those urls work? I think the way you've written the above unit tests looks good enough given what we're testing and how long we'll need them for.
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'm not sure what you are looking for that is different from what is here. Can you clarify?
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.
please correct me if I am mistaken but it looks like you are testing what happens when you go to /s/csp2-2020/stage/15/puzzle/1/ . I am asking what happens at /s/csp2-2020/stage/15/puzzle/1/page/1 .
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.
Ah I see what you are saying. Going directly to that URL instead of the redirect. I'll add something
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.
Looks good, Dani!
My concern about multiple get requests within a single unit tests appears to be unfounded.
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.
Makes sense to me! What's special about unit 8?
Unit 8 does not have a lockable lesson at the end (its the Create PT unit) |
Manual Testing Report
|
For the 9 special case lessons in CSP 20-21 start looking up the scripts_levels using the lessons absolute position instead of relative_position. This results in if a user goes to
/s/csp1-2020/stage/14/puzzle/1/page/1
for example they will get redirected to/s/csp1-2020/lockable/2/puzzle/1/page/1
but the/lockable
url still remains the one we send users to through out the site.This is part 1 of 5 for moving CSP 20-21 lockable lessons with lesson plans over to new URLs at
/stage
instead of/lockable
. The steps are:/stage
URLs for CSP 20-21 lockable lesson with lesson plans and redirect old/lockable
URL to the new/stage
URL.Links
Testing story