-
Notifications
You must be signed in to change notification settings - Fork 5
Support multiple teachers per class #456
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
Conversation
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.
Approved👍
@@ -105,6 +106,10 @@ def school_owner? | |||
school && current_user.school_owner?(school) | |||
end | |||
|
|||
def class_teacher?(project) | |||
project.lesson_id.present? && project.lesson.school_class.present? && project.lesson.school_class.teacher_ids.include?(current_user.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.
do you need project.lesson_id.present?
? I think that's implicit by project.lesson
existing, since the relationship wouldn't exist otherwise
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.
This was more to be on the safe side to cover off the cases where it's not a school project
app/models/school_class.rb
Outdated
@@ -2,12 +2,15 @@ | |||
|
|||
class SchoolClass < ApplicationRecord | |||
belongs_to :school | |||
has_many :members, class_name: :ClassMember, inverse_of: :school_class, dependent: :destroy | |||
has_many :students, class_name: :ClassStudent, inverse_of: :school_class, dependent: :destroy | |||
has_many :class_teachers, class_name: :ClassTeacher, inverse_of: :school_class, dependent: :destroy |
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.
why use :students
and then :class_teachers
instead of :teachers
or vice versa? going with one or the other would be nice for consistency
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 yes, that would simplify things, thanks 👍
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 great, just a few fairly minor comments
:name, | ||
:created_at, | ||
:updated_at | ||
) | ||
|
||
json.teacher_name(teacher&.name) | ||
json.teachers(teachers) do |teacher| | ||
if teacher.present? |
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.
there is already a _school_teacher.json.builder template you can use here and in the show template, you might just need to make the email optional
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.
👍
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 great! and well done, this was a prickly bit of work :)
What's Changed?
ClassTeacher
model which allows classes to have multiple teachersClass
model now accepts nested attributes forClassTeacher
sClassMember
model has been renamed toClassStudent
to provide consistency with the approach forSchoolStudent
,SchoolMember
etc.ClassMember
operations will now deal with both teachers and students in keeping with theSchoolMember
modulecloses #463