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

redirect students to course overview page #17470

Merged
merged 2 commits into from
Sep 12, 2017
Merged

Conversation

Erin007
Copy link
Contributor

@Erin007 Erin007 commented Sep 1, 2017

In #17350 I redirected students with course progress to the next lesson upon sign-in, when I should have redirected them to the course overview page for their top course. Unexpectedly, this caused a bug where students who don't have an age set couldn't access their accounts because they weren't prompted with the age interstitial. Eric tracked down this bug and temporarily fixed it in #17461. (Thank you!!) Long term, we still want all students to land on their course overview page upon sign in.

@Erin007
Copy link
Contributor Author

Erin007 commented Sep 1, 2017

I spoke with @tanyaparker briefly and discussed the redirect that needed to be made in the first place. Apologies to all for the confusion! @ewjordan will this PR work or does any student without an age set have to ONLY go to /home?

if current_user.student? && current_user.primary_script && current_user.age.present?
redirect_to script_next_path(current_user.primary_script)
if current_user.student? && current_user.primary_script
redirect_to script_path(current_user.primary_script)
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll have to test this with yesterday's problem scenario for new google classroom students. @ewjordan has the details and knows how to set up a repro. It might be worth still skipping the redirect when age is missing (unless of course we can confirm it works with this new path). If we do remove that check, also remove the associated comment above ;)

Copy link
Contributor

@ewjordan ewjordan Sep 1, 2017

Choose a reason for hiding this comment

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

The repro is pretty easy, with a user in the right kind of section to trigger this redirect, from the dashboard-console you can do the same thing the test does:

student.birthday = nil
student.age = nil
student.save(validate: false)

and then just navigate to the root (not home) and make sure that you can enter an age on whatever page you land on.

get :index

assert_redirected_to '/home'
assert_redirected_to script_path(script)
Copy link
Contributor

Choose a reason for hiding this comment

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

like above, verify with @ewjordan that this scenario works and doesn't cause the CSRF error we were seeing yesterday.

@ewjordan
Copy link
Contributor

ewjordan commented Sep 1, 2017

I believe this should work, though we should definitely verify on staging - the course overview pages do have the CSRF token present, so the age interstitial should work just fine there. Longer term I'm probably also going to set up that interstitial to work on any page so we don't have to worry about this.

@Erin007 Erin007 merged commit 0abd694 into staging Sep 12, 2017
@Erin007 Erin007 deleted the fix-student-redirect branch September 12, 2017 16:17
@tanyaparker
Copy link
Contributor

WOOO!!! Lots of people are going to be so excited about this. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants