Create roles for stranded students

app/controllers/api/join_controller.rb

@@ -14,12 +14,12 @@ def show
 
     def create
       case action_status
-      when :wrong_school, :domain_mismatch, :not_a_student
+      when :wrong_school, :not_a_student
         render json: { error: action_status.to_s }, status: :forbidden
       when :already_member
         render json: {}, status: :ok
       when :joinable
-        add_student_to_school_and_class
+        add_student_to_class
         render json: {}, status: :ok
       else
         raise "Unexpected join action_status: #{action_status.inspect}"
@@ -43,13 +43,8 @@ def action_status
       @action_status ||= JoinStatusService.new(school: @school, school_class: @school_class, user: current_user).call
     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)
-        ClassStudent.find_or_create_by!(school_class: @school_class, student_id: current_user.id) do |class_student|
-          class_student.student = current_user
-        end
-      end
+    def add_student_to_class
+      @school_class.students.find_or_create_by!(student_id: current_user.id)
     rescue ActiveRecord::RecordNotUnique
       # Concurrent join request for the same user/class — DB unique index
       # caught a race we couldn't catch at validation time. Already enrolled.

app/controllers/concerns/identifiable.rb

@@ -16,6 +16,8 @@ def load_current_user
 
     @current_user = User.from_token(token:)
     return if @current_user.blank?
+
+    StudentRoleService.ensure_student_role(@current_user)
     return unless RequestStore.respond_to?(:active?) && RequestStore.active?
 
     RequestStore.store[:safeguarding_flag_users_by_token] ||= {}

app/models/school.rb

@@ -156,15 +156,6 @@ def valid_domain?(candidate_domain)
     false
   end
 
-  def email_domain_in_school_domains?(email)
-    return false if email.blank?
-
-    local, separator, domain = email.to_s.rpartition('@')
-    return false if separator.empty? || local.blank? || domain.blank?
-
-    valid_domain?(domain.strip.downcase)
-  end
-
   private
 
   # Ensure the reference is nil, not an empty string

app/models/user.rb

@@ -17,6 +17,7 @@ class User
     picture
     postcode
     profile
+    school_id
     token
     username
     roles
@@ -54,6 +55,10 @@ def student?
     Role.student.exists?(user_id: id)
   end
 
+  def school_id
+    @school_id if student_account_type?
+  end
+
   def student_account_type?
     sub.to_s.starts_with?('student:')
   end

app/services/join_status_service.rb

@@ -9,7 +9,6 @@
 #   :joinable             — user can be enrolled as a student of this class
 #   :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
   def initialize(school:, school_class:, user:)
     @school = school
@@ -30,7 +29,6 @@ def call
   def new_user_join_status
     return :not_a_student if user_has_non_student_account_type?
     return :wrong_school if user_in_different_school?
-    return :domain_mismatch unless @school.email_domain_in_school_domains?(@user.email)
 
     :joinable
   end

app/services/student_role_service.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class StudentRoleService
+  class << self
+    def ensure_student_role(user)
+      return unless user.student_account_type? && user.school_id.present?
+
+      Role.student.find_or_create_by!(school_id: user.school_id, user_id: user.id)
+    end
+  end
+end

spec/factories/user.rb

@@ -18,23 +18,24 @@
     end
 
     factory :student do
+      transient do
+        school { nil }
+      end
+
       email { nil }
       username { Faker::Internet.username }
       sso_providers { [] } # standard students have no SSO providers
       sub { "student:#{id}" }
+      school_id { school&.id || create(:school).id }
 
       trait :sso do
         email { Faker::Internet.email }
         username { nil }
         sso_providers { ['google'] } # SSO students have SSO providers
       end
 
-      transient do
-        school { nil }
-      end
-
-      after(:create) do |user, context|
-        create(:student_role, user_id: user.id, school: context.school)
+      after(:create) do |user|
+        create(:student_role, user_id: user.id, school_id: user.school_id)
       end
     end
 

spec/features/school/listing_schools_spec.rb

@@ -3,37 +3,56 @@
 require 'rails_helper'
 
 RSpec.describe 'Listing schools', type: :request do
-  before do
-    school = create(:school, name: 'Test School')
-    owner = create(:owner, school:)
-    authenticated_in_hydra_as(owner)
-  end
-
   let(:headers) { { Authorization: UserProfileMock::TOKEN } }
+  let(:school) { create(:school, name: 'Test School') }
 
-  it 'responds 200 OK' do
-    get('/api/schools', headers:)
-    expect(response).to have_http_status(:ok)
+  it 'responds 401 Unauthorized when no token is given' do
+    get '/api/schools'
+    expect(response).to have_http_status(:unauthorized)
   end
 
-  it 'responds with the schools JSON' do
-    get('/api/schools', headers:)
-    data = JSON.parse(response.body, symbolize_names: true)
+  context 'when the user is a school owner' do
+    let(:user) { create(:owner, school:) }
 
-    expect(data.first[:name]).to eq('Test School')
-  end
+    before do
+      authenticated_in_hydra_as(user)
+    end
 
-  it 'only includes schools the user belongs to' do
-    create(:school, id: SecureRandom.uuid)
+    it 'responds 200 OK' do
+      get('/api/schools', headers:)
+      expect(response).to have_http_status(:ok)
+    end
 
-    get('/api/schools', headers:)
-    data = JSON.parse(response.body, symbolize_names: true)
+    it 'responds with the schools JSON' do
+      get('/api/schools', headers:)
+      data = JSON.parse(response.body, symbolize_names: true)
+
+      expect(data.first[:name]).to eq('Test School')
+    end
 
-    expect(data.size).to eq(1)
+    it 'only includes schools the user belongs to' do
+      create(:school, id: SecureRandom.uuid)
+
+      get('/api/schools', headers:)
+      data = JSON.parse(response.body, symbolize_names: true)
+
+      expect(data.size).to eq(1)
+    end
+
+    it 'responds 401 Unauthorized when no token is given' do
+      get '/api/schools'
+      expect(response).to have_http_status(:unauthorized)
+    end
   end
 
-  it 'responds 401 Unauthorized when no token is given' do
-    get '/api/schools'
-    expect(response).to have_http_status(:unauthorized)
+  it 'creates a student role when the students token includes a school claim but they have no school already' do
+    user = build(:student, school_id: school.id)
+    authenticated_in_hydra_as(user, :student)
+
+    expect { get('/api/schools', headers:) }
+      .to change { Role.student.where(school:, user_id: user.id).count }.by(1)
+    data = JSON.parse(response.body, symbolize_names: true)
+
+    expect(data.first[:name]).to eq('Test School')
   end
 end

spec/lib/profile_api_client_spec.rb

@@ -410,8 +410,8 @@ def list_school_students
     let(:username) { 'username' }
     let(:password) { 'password' }
     let(:name) { 'name' }
-    let(:school) { build(:school, id: SecureRandom.uuid) }
-    let(:student) { create(:student, school:) }
+    let(:school) { create(:school, id: SecureRandom.uuid) }
+    let(:student) { create(:student, school_id: school.id) }
     let(:update_student_url) { "#{api_url}/api/v1/schools/#{school.id}/students/#{student.id}" }
 
     before do

spec/models/school_spec.rb

@@ -775,36 +775,4 @@
       expect(school.valid_domain?(unregistered_domain)).to be(false)
     end
   end
-
-  describe '#email_domain_in_school_domains?' do
-    before do
-      SchoolEmailDomain.create!(school:, domain: 'valid.edu')
-    end
-
-    it 'returns true when the email domain is registered for the school' do
-      expect(school.email_domain_in_school_domains?('user@valid.edu')).to be(true)
-    end
-
-    it 'returns false when the email domain is not registered for the school' do
-      expect(school.email_domain_in_school_domains?('user@other.edu')).to be(false)
-    end
-
-    it 'normalizes case and surrounding whitespace on the domain' do
-      expect(school.email_domain_in_school_domains?('User@VALID.EDU  ')).to be(true)
-    end
-
-    it 'returns false when the email is blank' do
-      expect(school.email_domain_in_school_domains?('')).to be(false)
-      expect(school.email_domain_in_school_domains?(nil)).to be(false)
-    end
-
-    it 'returns false when the email has no @ separator' do
-      expect(school.email_domain_in_school_domains?('not-an-email')).to be(false)
-    end
-
-    it 'returns false when the local part or domain is missing' do
-      expect(school.email_domain_in_school_domains?('@valid.edu')).to be(false)
-      expect(school.email_domain_in_school_domains?('user@')).to be(false)
-    end
-  end
 end