diff --git a/app/controllers/api/join_controller.rb b/app/controllers/api/join_controller.rb index 90bedfcdb..bd56e04c2 100644 --- a/app/controllers/api/join_controller.rb +++ b/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 diff --git a/app/services/join_status_service.rb b/app/services/join_status_service.rb index ae4402556..0e03ee2b4 100644 --- a/app/services/join_status_service.rb +++ b/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? diff --git a/spec/requests/join_controller_spec.rb b/spec/requests/join_controller_spec.rb index 9ffa73d9b..ed6817390 100644 --- a/spec/requests/join_controller_spec.rb +++ b/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,8 +230,6 @@ 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 @@ -243,51 +237,25 @@ 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 diff --git a/spec/services/join_status_service_spec.rb b/spec/services/join_status_service_spec.rb index 65528cea1..3b2bcfea3 100644 --- a/spec/services/join_status_service_spec.rb +++ b/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