Support multiple teachers per class (#456)

## What's Changed?

- Added a `ClassTeacher` model which allows classes to have multiple
teachers
- `Class` model now accepts nested attributes for `ClassTeacher`s
- `ClassMember` model has been renamed to `ClassStudent` to provide
consistency with the approach for `SchoolStudent`, `SchoolMember` etc.
- `ClassMember` operations will now deal with both teachers and students
in keeping with the `SchoolMember` module

closes #463

---------

Co-authored-by: Kristina <[email protected]>

Author: loiswells97

app/controllers/api/class_members_controller.rb

@@ -5,11 +5,11 @@ class ClassMembersController < ApiController
     before_action :authorize_user
     load_and_authorize_resource :school
     load_and_authorize_resource :school_class, through: :school, through_association: :classes, id_param: :class_id
-    load_and_authorize_resource :class_member, through: :school_class, through_association: :members
+    load_and_authorize_resource :class_student, through: :school_class, through_association: :students
 
     def index
-      @class_members = @school_class.members.accessible_by(current_ability)
-      result = ClassMember::List.call(school_class: @school_class, class_members: @class_members, token: current_user.token)
+      @class_students = @school_class.students.accessible_by(current_ability)
+      result = ClassMember::List.call(school_class: @school_class, class_students: @class_students, token: current_user.token)
 
       if result.success?
         @class_members = result[:class_members]

app/controllers/api/projects_controller.rb

@@ -22,6 +22,7 @@ def show
         @user = project_with_user[1]
       end
 
+      @project.user_id = @current_user.id if class_teacher?(@project)
       render :show, formats: [:json]
     end
 
@@ -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)
+    end
+
     def school
       @school ||= @project&.school || School.find_by(id: base_params[:school_id])
     end

app/controllers/api/school_classes_controller.rb

@@ -8,21 +8,21 @@ class SchoolClassesController < ApiController
 
     def index
       school_classes = @school.classes.accessible_by(current_ability)
-      school_classes = school_classes.where(teacher_id: current_user.id) if params[:my_classes] == 'true'
+      school_classes = school_classes.joins(:teachers).where(teachers: { teacher_id: current_user.id }) if params[:my_classes] == 'true'
       @school_classes_with_teachers = school_classes.with_teachers
       render :index, formats: [:json], status: :ok
     end
 
     def show
-      @school_class_with_teacher = @school_class.with_teacher
+      @school_class_with_teachers = @school_class.with_teachers
       render :show, formats: [:json], status: :ok
     end
 
     def create
-      result = SchoolClass::Create.call(school: @school, school_class_params:)
+      result = SchoolClass::Create.call(school: @school, school_class_params:, current_user:)
 
       if result.success?
-        @school_class_with_teacher = result[:school_class].with_teacher
+        @school_class_with_teachers = result[:school_class].with_teachers
         render :show, formats: [:json], status: :created
       else
         render json: { error: result[:error] }, status: :unprocessable_entity
@@ -34,7 +34,7 @@ def update
       result = SchoolClass::Update.call(school_class:, school_class_params:)
 
       if result.success?
-        @school_class_with_teacher = result[:school_class].with_teacher
+        @school_class_with_teachers = result[:school_class].with_teachers
         render :show, formats: [:json], status: :ok
       else
         render json: { error: result[:error] }, status: :unprocessable_entity
@@ -55,7 +55,7 @@ def destroy
 
     def school_class_params
       # A school teacher may only create classes they own.
-      params.require(:school_class).permit(:name, :description).merge(teacher_id: current_user.id)
+      params.require(:school_class).permit(:name, :description)
     end
 
     def school_owner?

app/models/ability.rb

@@ -61,7 +61,7 @@ def define_school_owner_abilities(school:)
     can(%i[read], :school_member)
     can(%i[read create update destroy], SchoolClass, school: { id: school.id })
     can(%i[read], Project, school_id: school.id, lesson: { visibility: %w[teachers students] })
-    can(%i[read create create_batch destroy], ClassMember, school_class: { school: { id: school.id } })
+    can(%i[read create create_batch destroy], ClassStudent, school_class: { school: { id: school.id } })
     can(%i[read create destroy], :school_owner)
     can(%i[read create destroy], :school_teacher)
     can(%i[read create create_batch update destroy], :school_student)
@@ -73,9 +73,8 @@ def define_school_teacher_abilities(user:, school:)
     can(%i[read], School, id: school.id)
     can(%i[read], :school_member)
     can(%i[create], SchoolClass, school: { id: school.id })
-    can(%i[read update destroy], SchoolClass, school: { id: school.id }, teacher_id: user.id)
-    can(%i[read create create_batch destroy], ClassMember,
-        school_class: { school: { id: school.id }, teacher_id: user.id })
+    can(%i[read update destroy], SchoolClass, school: { id: school.id }, teachers: { teacher_id: user.id })
+    can(%i[read create create_batch destroy], ClassStudent, school_class: { school: { id: school.id }, teachers: { teacher_id: user.id } })
     can(%i[read], :school_owner)
     can(%i[read], :school_teacher)
     can(%i[read create create_batch update], :school_student)
@@ -88,24 +87,24 @@ def define_school_teacher_abilities(user:, school:)
     end
     can(%i[read update], Project, school_id: school.id, lesson: { visibility: %w[teachers students] })
     can(%i[read], Project,
-        remixed_from_id: Project.where(user_id: user.id, school_id: school.id, remixed_from_id: nil).pluck(:id))
+        remixed_from_id: Project.where(school_id: school.id, remixed_from_id: nil, lesson_id: Lesson.where(school_class_id: ClassTeacher.where(teacher_id: user.id).select(:school_class_id))).pluck(:id))
   end
 
   def define_school_student_abilities(user:, school:)
     can(%i[read], School, id: school.id)
-    can(%i[read], SchoolClass, school: { id: school.id }, members: { student_id: user.id })
+    can(%i[read], SchoolClass, school: { id: school.id }, students: { student_id: user.id })
     # Ensure no access to ClassMember resources, relationships otherwise allow access in some circumstances.
-    can(%i[read], Lesson, school_id: school.id, visibility: 'students', school_class: { members: { student_id: user.id } })
+    can(%i[read], Lesson, school_id: school.id, visibility: 'students', school_class: { students: { student_id: user.id } })
     can(%i[read create update], Project, school_id: school.id, user_id: user.id, lesson_id: nil)
-    can(%i[read], Project, lesson: { school_id: school.id, school_class: { members: { student_id: user.id } } })
+    can(%i[read], Project, lesson: { school_id: school.id, school_class: { students: { student_id: user.id } } })
     can(%i[show_finished set_finished], SchoolProject, project: { user_id: user.id, lesson_id: nil }, school_id: school.id)
   end
 
   def school_teacher_can_manage_lesson?(user:, school:, lesson:)
     is_my_lesson = lesson.school_id == school.id && lesson.user_id == user.id
-    is_my_class = lesson.school_class && lesson.school_class.teacher_id == user.id
+    is_my_class = lesson.school_class&.teacher_ids&.include?(user.id)
 
-    is_my_lesson && (is_my_class || !lesson.school_class)
+    is_my_class || (is_my_lesson && !lesson.school_class)
   end
 
   def school_teacher_can_manage_project?(user:, school:, project:)

app/models/class_member.rb renamed to app/models/class_student.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-class ClassMember < ApplicationRecord
+class ClassStudent < ApplicationRecord
   belongs_to :school_class
   delegate :school, to: :school_class
   attr_accessor :student

app/models/class_teacher.rb

@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+class ClassTeacher < ApplicationRecord
+  belongs_to :school_class
+  delegate :school, to: :school_class
+  attr_accessor :teacher
+
+  validates :teacher_id, presence: true, uniqueness: {
+    scope: :school_class_id,
+    case_sensitive: false
+  }
+
+  validate :teacher_has_the_school_teacher_role_for_the_school
+
+  has_paper_trail(
+    meta: {
+      meta_school_id: ->(cm) { cm.school_class&.school_id }
+    }
+  )
+
+  private
+
+  def teacher_has_the_school_teacher_role_for_the_school
+    return unless teacher_id_changed? && errors.blank? && teacher.present?
+
+    return if teacher.school_teacher?(school)
+
+    msg = "'#{teacher.id}' does not have the 'school-teacher' role for organisation '#{school.id}'"
+    errors.add(:teacher, msg)
+  end
+end

app/models/lesson.rb

@@ -70,7 +70,7 @@ def user_has_the_school_owner_or_school_teacher_role_for_the_school
   end
 
   def user_is_the_school_teacher_for_the_school_class
-    return if !school_class || user_id == school_class.teacher_id
+    return if !school_class || school_class.teacher_ids.include?(user_id)
 
     errors.add(:user, "'#{user_id}' is not the 'school-teacher' for school_class '#{school_class.id}'")
   end

app/models/project.rb

@@ -21,7 +21,7 @@ class Project < ApplicationRecord
   validate :identifier_cannot_be_taken_by_another_user
   validates :locale, presence: true, unless: :user_id
   validate :user_has_a_role_within_the_school
-  validate :user_is_a_member_or_the_owner_of_the_lesson
+  validate :user_is_class_teacher_or_student
   validate :project_with_instructions_must_belong_to_school
   validate :project_with_school_id_has_school_project
   validate :school_project_school_matches_project_school
@@ -96,11 +96,22 @@ def user_has_a_role_within_the_school
     errors.add(:user, msg)
   end
 
-  def user_is_a_member_or_the_owner_of_the_lesson
+  def user_is_class_teacher_or_student
     # TODO: Revisit the case where the lesson is not associated to a class i.e. when we build a lesson library
-    return if !lesson || user_id == lesson.user_id || !lesson.school_class || lesson.school_class&.members&.exists?(student_id: user_id)
+    no_lesson = !lesson
+    no_school_class = lesson && !lesson.school_class
 
-    errors.add(:user, "'#{user_id}' is not the owner or a member of the lesson '#{lesson_id}'")
+    return if no_lesson || no_school_class || user_is_class_student || user_is_class_teacher
+
+    errors.add(:user, "'#{user_id}' is not a class member or the owner of the lesson '#{lesson_id}'")
+  end
+
+  def user_is_class_student
+    lesson&.school_class&.students&.exists?(student_id: user_id)
+  end
+
+  def user_is_class_teacher
+    lesson&.school_class&.teachers&.exists?(teacher_id: user_id)
   end
 
   def project_with_instructions_must_belong_to_school

app/models/school_class.rb

@@ -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 :teachers, class_name: :ClassTeacher, inverse_of: :school_class, dependent: :destroy
   has_many :lessons, dependent: :nullify
+  accepts_nested_attributes_for :teachers
+
+  scope :with_teachers, ->(user_id) { joins(:teachers).where(teachers: { id: user_id }) }
 
-  validates :teacher_id, presence: true
   validates :name, presence: true
-  validate :teacher_has_the_school_teacher_role_for_the_school
+  validate :school_class_has_at_least_one_teacher
 
   has_paper_trail(
     meta: {
@@ -16,27 +19,28 @@ class SchoolClass < ApplicationRecord
   )
 
   def self.teachers
-    User.from_userinfo(ids: pluck(:teacher_id))
+    teacher_ids = all.map(&:teacher_ids).flatten.uniq
+    User.from_userinfo(ids: teacher_ids)
   end
 
   def self.with_teachers
     by_id = teachers.index_by(&:id)
-    all.map { |instance| [instance, by_id[instance.teacher_id]] }
+    all.map { |instance| [instance, instance.teacher_ids.map { |teacher_id| by_id[teacher_id] }] }
   end
 
-  def with_teacher
-    [self, User.from_userinfo(ids: teacher_id).first]
+  def teacher_ids
+    teachers.pluck(:teacher_id)
   end
 
-  private
+  def with_teachers
+    [self, User.from_userinfo(ids: teacher_ids)]
+  end
 
-  def teacher_has_the_school_teacher_role_for_the_school
-    return unless teacher_id_changed? && errors.blank?
+  private
 
-    user = User.new(id: teacher_id)
-    return if user.school_teacher?(school)
+  def school_class_has_at_least_one_teacher
+    return if teachers.present?
 
-    msg = "'#{teacher_id}' does not have the 'school-teacher' role for organisation '#{school.id}'"
-    errors.add(:user, msg)
+    errors.add(:teachers, 'must have at least one teacher')
   end
 end

app/views/api/school_classes/index.json.jbuilder

@@ -1,16 +1,17 @@
 # frozen_string_literal: true
 
-json.array!(@school_classes_with_teachers) do |school_class, teacher|
+json.array!(@school_classes_with_teachers) do |school_class, teachers|
   json.call(
     school_class,
     :id,
     :description,
     :school_id,
-    :teacher_id,
     :name,
     :created_at,
     :updated_at
   )
 
-  json.teacher_name(teacher&.name)
+  json.teachers(teachers) do |teacher|
+    json.partial! '/api/school_teachers/school_teacher', teacher:, include_email: false if teacher.present?
+  end
 end

app/views/api/school_classes/show.json.jbuilder

@@ -1,16 +1,17 @@
 # frozen_string_literal: true
 
-school_class, teacher = @school_class_with_teacher
+school_class, teachers = @school_class_with_teachers
 
 json.call(
   school_class,
   :id,
   :description,
   :school_id,
-  :teacher_id,
   :name,
   :created_at,
   :updated_at
 )
 
-json.teacher_name(teacher&.name)
+json.teachers(teachers) do |teacher|
+  json.partial! '/api/school_teachers/school_teacher', teacher:, include_email: false
+end

app/views/api/school_teachers/_school_teacher.json.jbuilder

@@ -3,6 +3,9 @@
 json.call(
   teacher,
   :id,
-  :email,
   :name
 )
+
+include_email = local_assigns.fetch(:include_email, true)
+
+json.email(teacher.email) if include_email

db/migrate/20241101125219_create_class_teachers.rb

@@ -0,0 +1,12 @@
+class CreateClassTeachers < ActiveRecord::Migration[7.1]
+  def change
+    create_table :class_teachers, id: :uuid do |t|
+      t.references :school_class, type: :uuid, foreign_key: true, index: true, null: false
+      t.uuid :teacher_id, null: false
+      t.timestamps
+    end
+
+    add_index :class_teachers, :teacher_id
+    add_index :class_teachers, %i[school_class_id teacher_id], unique: true
+  end
+end

db/migrate/20241101125505_move_teacher_data_to_class_teachers_table.rb

@@ -0,0 +1,11 @@
+class MoveTeacherDataToClassTeachersTable < ActiveRecord::Migration[7.1]
+  def up
+    SchoolClass.find_each do |school_class|
+      ClassTeacher.create!(school_class: school_class, teacher_id: school_class.teacher_id)
+    end
+  end
+
+  def down
+    ClassTeacher.destroy_all
+  end
+end

db/migrate/20241101125814_remove_class_teacher_id.rb

@@ -0,0 +1,14 @@
+class RemoveClassTeacherId < ActiveRecord::Migration[7.1]
+  def up
+    remove_column :school_classes, :teacher_id
+  end
+
+  def down
+    add_column :school_classes, :teacher_id, :uuid
+    ClassTeacher.find_each do |class_teacher|
+      school_class = class_teacher.school_class
+      school_class.update!(teacher_id: class_teacher.teacher_id)
+    end
+    change_column_null :school_classes, :teacher_id, false
+  end
+end

db/migrate/20241101142545_rename_class_members_to_class_students.rb

@@ -0,0 +1,9 @@
+class RenameClassMembersToClassStudents < ActiveRecord::Migration[7.1]
+  def up
+    rename_table :class_members, :class_students
+  end
+
+  def down
+    rename_table :class_students, :class_members
+  end
+end