From b04a3b3f93c282c4c741bbb72c4814e58889c695 Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Mon, 7 Sep 2026 14:55:40 +0100 Subject: [PATCH 1/2] Simplify join codes stopping teachers being able to follow links When taking to Jason, he was unsure if it was safe for teachers to be able to use the same join codes as students. I've decided to remove it handling teachers adds more complexity to the frontend. Instead all non-student accounts will be a shown a link to get to the class which they can use if they are already a teacher or owner. This also removes the owner status. Now all non-students are treated the same. We may think this is a useful feature in the future so could add it back in if we want. fixup --- app/controllers/api/join_controller.rb | 16 +------- app/services/join_status_service.rb | 29 +++---------- spec/requests/join_controller_spec.rb | 50 ++++++----------------- spec/services/join_status_service_spec.rb | 14 +++---- 4 files changed, 25 insertions(+), 84 deletions(-) diff --git a/app/controllers/api/join_controller.rb b/app/controllers/api/join_controller.rb index 90bedfcdb..860753908 100644 --- a/app/controllers/api/join_controller.rb +++ b/app/controllers/api/join_controller.rb @@ -16,10 +16,7 @@ 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 + when :already_member render json: { redirect_url: class_redirect_path }, status: :ok when :joinable add_student_to_school_and_class @@ -64,16 +61,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..1bd466de1 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 @@ -243,51 +243,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 From 13b5eece908d94ec156509d416f3a6ed13c6400c Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Mon, 7 Sep 2026 15:01:39 +0100 Subject: [PATCH 2/2] Remove confusing param This is not a URL, it's not even full root-relative path as the locale needs to be added. The frontend knows all that it needs to construct the URL from the other params in the get response so this can be removed. --- app/controllers/api/join_controller.rb | 8 ++------ spec/requests/join_controller_spec.rb | 8 +------- 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/app/controllers/api/join_controller.rb b/app/controllers/api/join_controller.rb index 860753908..bd56e04c2 100644 --- a/app/controllers/api/join_controller.rb +++ b/app/controllers/api/join_controller.rb @@ -17,10 +17,10 @@ def create when :wrong_school, :domain_mismatch, :not_a_student render json: { error: action_status.to_s }, status: :forbidden when :already_member - render json: { redirect_url: class_redirect_path }, status: :ok + 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 @@ -43,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) diff --git a/spec/requests/join_controller_spec.rb b/spec/requests/join_controller_spec.rb index 1bd466de1..ed6817390 100644 --- a/spec/requests/join_controller_spec.rb +++ b/spec/requests/join_controller_spec.rb @@ -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