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
Remove references to Follower.user_id. #14115
Conversation
FYI @davidwufer. |
323e83d
to
d191972
Compare
d191972
to
fed77ba
Compare
@@ -1207,7 +1207,7 @@ def can_edit_password? | |||
end | |||
|
|||
def section_for_script(script) | |||
followeds.collect(&:section).find {|section| section.script_id == script.id} | |||
sections_as_student.find {|section| section.script_id == script.id} |
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.
Tangentially related, this line change could be part of a separate PR.
dashboard/app/models/user.rb
Outdated
collect {|followed| followed.user.try(:terms_of_service_version)}. | ||
compact. | ||
max | ||
teachers.map {|teacher| teacher.try(:terms_of_service_version)}.try(:compact).try(:max) |
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.
Tangentially related, this line change could be part of a separate PR.
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.
Can teacher
be nil here? I don't think you need the first .try
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.
Done. Good catch.
dashboard/app/models/user.rb
Outdated
collect {|followed| followed.user.try(:terms_of_service_version)}. | ||
compact. | ||
max | ||
teachers.map {|teacher| teacher.try(:terms_of_service_version)}.try(:compact).try(:max) |
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.
Can teacher
be nil here? I don't think you need the first .try
return if Dashboard.db[:sections]. | ||
join(:followers, section_id: :sections__id). | ||
where(followers__student_user_id: params[:id], sections__user_id: dashboard_user_id). | ||
where(followers__deleted_at: nil). |
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.
nit: any reason why the first 2 conditions are in one where
, and the 3rd condition is in a 2nd where
? I'd prefer consistency, either all comma-separated args in a single where
or all separate.
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.
Done.
This PR finishes making
Followers.user_id
truly optional: at the DB level (#13878), at the Rails validation level (#13945), at the controller level (this PR), and at the Pegasus level (this PR).After this PR reaches production (safely), further PRs will remove the column from the DB.