Simplify class links by removing special cases for teachers & owners

app/controllers/api/join_controller.rb

@@ -16,14 +16,11 @@ def create
       case action_status
       when :wrong_school, :domain_mismatch, :not_a_student
         render json: { error: action_status.to_s }, status: :forbidden
-      when :already_member, :owner
-        render json: { redirect_url: class_redirect_path }, status: :ok
-      when :joinable_as_teacher
-        add_user_to_class_as_teacher
-        render json: { redirect_url: class_redirect_path }, status: :ok
+      when :already_member
+        render json: {}, status: :ok
       when :joinable
         add_student_to_school_and_class
-        render json: { redirect_url: class_redirect_path }, status: :ok
+        render json: {}, status: :ok
       else
         raise "Unexpected join action_status: #{action_status.inspect}"
       end
@@ -46,10 +43,6 @@ def action_status
       @action_status ||= JoinStatusService.new(school: @school, school_class: @school_class, user: current_user).call
     end
 
-    def class_redirect_path
-      "/school/#{@school.code}/class/#{@school_class.code}"
-    end
-
     def add_student_to_school_and_class
       ActiveRecord::Base.transaction do
         Role.find_or_create_by!(school: @school, user_id: current_user.id, role: :student)
@@ -64,16 +57,5 @@ def add_student_to_school_and_class
       raise unless e.record.errors.of_kind?(:student_id, :taken)
       # Concurrent join request raced the in-memory uniqueness validator. Already enrolled.
     end
-
-    def add_user_to_class_as_teacher
-      ClassTeacher.find_or_create_by!(school_class: @school_class, teacher_id: current_user.id) do |class_teacher|
-        class_teacher.teacher = current_user
-      end
-    rescue ActiveRecord::RecordNotUnique
-      # Concurrent join request for the same teacher/class — already enrolled.
-    rescue ActiveRecord::RecordInvalid => e
-      raise unless e.record.errors.of_kind?(:teacher_id, :taken)
-      # Concurrent join raced the in-memory uniqueness validator. Already enrolled.
-    end
   end
 end

app/services/join_status_service.rb

@@ -6,10 +6,8 @@
 #
 # Possible return values:
 #   :already_member       — user is already in this class
-#   :owner                — user owns this school (redirect, no enrolment)
-#   :joinable_as_teacher  — user teaches this school but isn't in the class
 #   :joinable             — user can be enrolled as a student of this class
-#   :not_a_student        — user has a non-student role in some other school
+#   :not_a_student        — user has a non-student role
 #   :wrong_school         — user is a student of a different school
 #   :domain_mismatch      — user's email domain isn't registered for the school
 class JoinStatusService
@@ -21,21 +19,13 @@ def initialize(school:, school_class:, user:)
 
   def call
     return :already_member if user_is_member_of_class?
-    return existing_user_join_status if user_has_role_in_school?
+    return :joinable if user_is_a_student_in_school?
 
     new_user_join_status
   end
 
   private
 
-  # The user already has a role in this school: which one decides the status.
-  def existing_user_join_status
-    return :owner if user_is_owner_of_school?
-    return :joinable_as_teacher if user_is_teacher_of_school?
-
-    :joinable # student is the only remaining role for this school
-  end
-
   # The user has no role in this school yet: may they join as a new student?
   def new_user_join_status
     return :not_a_student if user_has_non_student_account_type?
@@ -46,20 +36,11 @@ def new_user_join_status
   end
 
   def user_is_member_of_class?
-    ClassStudent.exists?(school_class: @school_class, student_id: @user.id) ||
-      ClassTeacher.exists?(school_class: @school_class, teacher_id: @user.id)
-  end
-
-  def user_is_owner_of_school?
-    Role.exists?(school: @school, user_id: @user.id, role: Role.roles[:owner])
-  end
-
-  def user_is_teacher_of_school?
-    Role.exists?(school: @school, user_id: @user.id, role: Role.roles[:teacher])
+    ClassStudent.exists?(school_class: @school_class, student_id: @user.id)
   end
 
-  def user_has_role_in_school?
-    Role.exists?(school: @school, user_id: @user.id)
+  def user_is_a_student_in_school?
+    Role.exists?(school: @school, user_id: @user.id, role: Role.roles[:student])
   end
 
   def user_has_non_student_account_type?

spec/requests/join_controller_spec.rb

@@ -96,23 +96,23 @@
     context 'when the user is authenticated as a teacher' do
       before { authenticated_in_hydra_as(teacher) }
 
-      it 'returns status: joinable_as_teacher when the user is a teacher of this school not yet in the class' do
+      it 'returns status: :not_a_student when the user is a teacher of this school not yet in the class' do
         create(:teacher_role, school:, user_id: teacher.id)
 
         get "/api/join/#{school_class.join_code}", headers: headers
 
         data = JSON.parse(response.body, symbolize_names: true)
-        expect(data[:status]).to eq('joinable_as_teacher')
+        expect(data[:status]).to eq('not_a_student')
       end
 
-      it 'returns status: already_member when the user is already a teacher in the class' do
+      it 'returns status: not_a_student when the user is already a teacher in the class' do
         create(:teacher_role, school:, user_id: teacher.id)
         ClassTeacher.create!(school_class:, teacher_id: teacher.id)
 
         get "/api/join/#{school_class.join_code}", headers: headers
 
         data = JSON.parse(response.body, symbolize_names: true)
-        expect(data[:status]).to eq('already_member')
+        expect(data[:status]).to eq('not_a_student')
       end
 
       it 'returns status: not_a_student for a teacher of a different school (not wrong_school)' do
@@ -129,13 +129,13 @@
     context 'when the user is authenticated as an owner' do
       before { authenticated_in_hydra_as(owner) }
 
-      it 'returns status: owner when the user is an owner of this school' do
+      it 'returns status: not_a_student' do
         create(:owner_role, school:, user_id: owner.id)
 
         get "/api/join/#{school_class.join_code}", headers: headers
 
         data = JSON.parse(response.body, symbolize_names: true)
-        expect(data[:status]).to eq('owner')
+        expect(data[:status]).to eq('not_a_student')
       end
     end
   end
@@ -151,14 +151,12 @@
     context 'when the user is authenticated as a student' do
       before { authenticated_in_hydra_as(student, :student) }
 
-      it 'adds the user to the school and class and returns a redirect URL' do
+      it 'adds the user to the school' do
         expect do
           post "/api/join/#{school_class.join_code}", headers: headers
         end.to change(ClassStudent, :count).by(1).and change(Role, :count).by(1)
 
         expect(response).to have_http_status(:ok)
-        data = JSON.parse(response.body, symbolize_names: true)
-        expect(data[:redirect_url]).to eq("/school/#{school.code}/class/#{school_class.code}")
 
         created_role = Role.find_by(user_id: student.id, school:)
         expect(created_role.role).to eq('student')
@@ -173,8 +171,6 @@
         end.not_to change(ClassStudent, :count)
 
         expect(response).to have_http_status(:ok)
-        data = JSON.parse(response.body, symbolize_names: true)
-        expect(data[:redirect_url]).to eq("/school/#{school.code}/class/#{school_class.code}")
       end
 
       it 'does not duplicate the school role if the user is already in the school' do
@@ -234,60 +230,32 @@
           end.to change(ClassStudent, :count).by(1)
 
           expect(response).to have_http_status(:ok)
-          data = JSON.parse(response.body, symbolize_names: true)
-          expect(data[:redirect_url]).to eq("/school/#{school.code}/class/#{school_class.code}")
         end
       end
     end
 
     context 'when the user is authenticated as a teacher' do
       before { authenticated_in_hydra_as(teacher) }
 
-      it 'adds the user to the class as a teacher and returns a redirect URL' do
+      it 'does not add the teacher to the school' do
         create(:teacher_role, school:, user_id: teacher.id)
         school_class # force creation before the request
 
-        expect do
-          post "/api/join/#{school_class.join_code}", headers: headers
-        end.to change(ClassTeacher, :count).by(1)
-
-        expect(response).to have_http_status(:ok)
-        data = JSON.parse(response.body, symbolize_names: true)
-        expect(data[:redirect_url]).to eq("/school/#{school.code}/class/#{school_class.code}")
-        expect(ClassTeacher.exists?(school_class:, teacher_id: teacher.id)).to be(true)
-        expect(ClassStudent.exists?(school_class:, student_id: teacher.id)).to be(false)
-        expect(Role.where(user_id: teacher.id, school:).pluck(:role)).to eq(['teacher'])
-      end
-
-      it 'is idempotent when the user is already a teacher in the class' do
-        create(:teacher_role, school:, user_id: teacher.id)
-        ClassTeacher.create!(school_class:, teacher_id: teacher.id)
-
-        expect do
-          post "/api/join/#{school_class.join_code}", headers: headers
-        end.not_to change(ClassTeacher, :count)
+        post "/api/join/#{school_class.join_code}", headers: headers
 
-        expect(response).to have_http_status(:ok)
-        data = JSON.parse(response.body, symbolize_names: true)
-        expect(data[:redirect_url]).to eq("/school/#{school.code}/class/#{school_class.code}")
+        expect(response).to have_http_status(:forbidden)
       end
     end
 
     context 'when the user is authenticated as an owner' do
       before { authenticated_in_hydra_as(owner) }
 
-      it 'redirects an owner into the class without adding them to it' do
+      it 'returns a forbidden status' do
         create(:owner_role, school:, user_id: owner.id)
 
-        expect do
-          post "/api/join/#{school_class.join_code}", headers: headers
-        end.not_to change(ClassStudent, :count)
+        post "/api/join/#{school_class.join_code}", headers: headers
 
-        expect(response).to have_http_status(:ok)
-        data = JSON.parse(response.body, symbolize_names: true)
-        expect(data[:redirect_url]).to eq("/school/#{school.code}/class/#{school_class.code}")
-        expect(ClassTeacher.exists?(school_class:, teacher_id: owner.id)).to be(false)
-        expect(Role.where(user_id: owner.id, school:).pluck(:role)).to eq(['owner'])
+        expect(response).to have_http_status(:forbidden)
       end
     end
   end

spec/services/join_status_service_spec.rb

@@ -28,8 +28,8 @@
 
       before { ClassTeacher.create!(school_class:, teacher_id: user.id) }
 
-      it 'returns :already_member' do
-        expect(service.call).to eq(:already_member)
+      it 'returns :not_a_student' do
+        expect(service.call).to eq(:not_a_student)
       end
     end
 
@@ -38,16 +38,16 @@
 
       before { create(:owner_role, school:, user_id: user.id) }
 
-      it 'returns :owner' do
-        expect(service.call).to eq(:owner)
+      it 'returns :not_a_student' do
+        expect(service.call).to eq(:not_a_student)
       end
     end
 
-    context 'when the user is a teacher of the school but not in this class' do
+    context 'when the user is a teacher of the school' do
       let(:user) { create(:teacher, school:) }
 
-      it 'returns :joinable_as_teacher' do
-        expect(service.call).to eq(:joinable_as_teacher)
+      it 'returns :not_a_student' do
+        expect(service.call).to eq(:not_a_student)
       end
     end