Student sign in redirects

[Erin007]

There has been confusion among teachers about where their students are redirected upon signing in, which has caused a flurry of Zendesk tickets. To help alleviate the issue, students will only be redirected to a script overview page if they have been assigned a script and have not made progress in any scripts other than the one assigned since the time of assignment. Otherwise, the student will be redirected to /home on signing in where they will see course tiles for all scripts/courses to which they have been assigned and/or have made progress in.

A few notes:

• This work intentionally does not address what is shown as the student's TopCourse tile on their homepage. Future work will likely address this, but it is lower priority than HoC-related work items at this time.
• A quick of this part of the code base is that we, unfortunately, do not track timing of when courses are assigned. Future work to include a UserCourse model, similar to UserScript would be ideal, but is also not a priority at this time. Therefore, this change only applies to assigned scripts, not assigned courses.

[islemaster]

This is great! I have suggested improvements but nothing blocking. Great tests!

[islemaster]

Some possible subtle improvements to your query here:

When trying to get the first or last of a thing by date, try to use order instead of sort_by and first instead of last. Differences: sort_by is a Ruby operation that runs after the query, where order is a database operation that runs as part of the query (source), and first can be interpreted to add a LIMIT 1 clause to your query while last will require you to load the whole recordset.

In practice here the difference is small (there are usually few userscripts per student) but does show up in the query.

[staging] dashboard > u.user_scripts.where('assigned_at').sort_by(&:assigned_at).last
  UserScript Load (0.6ms)
SELECT `user_scripts`.* FROM `user_scripts` 
WHERE `user_scripts`.`user_id` = 122 AND (assigned_at) 
ORDER BY 
  -completed_at asc, 
  greatest(coalesce(started_at, 0), coalesce(assigned_at, 0), coalesce(last_progress_at, 0)) desc, 
  user_scripts.id asc

[staging] dashboard > u.user_scripts.where('assigned_at').order(assigned_at: :desc).first
  UserScript Load (0.8ms)  
SELECT  `user_scripts`.* FROM `user_scripts` 
WHERE `user_scripts`.`user_id` = 122 AND (assigned_at) 
ORDER BY 
  -completed_at asc, 
  greatest(coalesce(started_at, 0), coalesce(assigned_at, 0), coalesce(last_progress_at, 0)) desc, 
  user_scripts.id asc, 
  `user_scripts`.`assigned_at` DESC
LIMIT 1

The difference here is more subtle than it would otherwise be because the user_scripts relation on User has a default ordering clause:

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

Line 439 in 2c7c69b

has_many :user_scripts, -> {order "-completed_at asc, greatest(coalesce(started_at, 0), coalesce(assigned_at, 0), coalesce(last_progress_at, 0)) desc, user_scripts.id asc"}

[islemaster]

It might feel like overkill, but I think you could push most of these conditions down into should_redirect_to_script_overview? so that this logic simply reads

if should_redirect_to_script_overview?
  redirect_to script_path current_user.most_recently_assigned_script
else
  redirect_to '/home'
end

[islemaster]

Great testing! One thing I'd consider adding is precondition assertions; for example, this case "student without progress or assigned course/script redirected to index" could check that the student really does have no progress or assigned course/script (in case we change our factory in the future) so it proves it's testing what it says it's testing.

# Given...
student = create :student
assert_nil student.script_with_most_recent_progress
assert_nil student.most_recently_assigned_script

# When...
sign_in student
get :index

# Then...
assert_redirected_to '/home'

[davidsbailey]

students will only be redirected to a script overview page if they have been assigned a script and have not made progress in any scripts other than the one assigned since the time of assignment

this doesn't seem right to me -- if they are assigned to csp1-2018, and they do frozen for a bit, then come back and do csp1-2018 again, the next time they sign in I'd want them to go to csp1-2018. Otherwise, the assignment seems like it will almost never cause the student to be redirected to the right place. I think we would want to redirect them as long as "last progress" and "last assigned at" are both pointing to the same unit.

If you're sure this is the behavior you want, then ok to proceed.

[Erin007]

Merging to get the bug fix in ASAP, will address suggestions in a follow up.

[Erin007]

I think this behaves the way I want, but I phrased poorly in the description. In the case you describe @davidsbailey the student would end up on csp1-2018 because last progress and last assigned script match. The scenario I was attempting to explain is when a student makes progress in Frozen and then later in time the student is assigned csp1-2018, they should go to csp1-2018 on sign in even though the last progress and last assigned script don't match.

[davidsbailey]

ok, great! yes, the logic in should_redirect_to_script_overview? looks like it matches what I was asking for / what you're saying.