Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f6f5c2c
Add JoinCodeGenerator utility
fspeirs May 6, 2026
4d5bb58
Add join_code to SchoolClass
fspeirs May 6, 2026
d5dfd16
Add regenerate_join_code endpoint
fspeirs May 6, 2026
2bb19a3
Add School#valid_email?
fspeirs May 6, 2026
83f836f
Add /api/join endpoint for student class enrollment
fspeirs May 6, 2026
4110c68
Expose School#sso_enabled? on school JSON
fspeirs May 14, 2026
3007552
Simplify join code format to CDDD-CDDD
fspeirs May 18, 2026
f97754f
refactor: rename School#sso_enabled? to auto_join_enabled?
fspeirs May 19, 2026
348c303
feat: let teachers and owners use class join links
fspeirs May 19, 2026
7684548
perf: use exists? for School#auto_join_enabled?
fspeirs May 20, 2026
9c00b3a
fix: bound join code generation and rescue unique-index races
fspeirs May 20, 2026
a9e8ff6
fix: make join enrollment idempotent under concurrent requests
fspeirs May 20, 2026
6aaf598
Update test harness description to match.
fspeirs May 20, 2026
3e34cc2
Use a similar approach to teacher adding as student.
fspeirs May 20, 2026
3339389
chore: add frozen_string_literal to add_join_code migration
fspeirs May 20, 2026
f2ed0f2
fix: harden backfill migration against retry exhaustion and races
fspeirs May 20, 2026
7f73c09
fix: keep ClassStudent role validator active and handle concurrent race
fspeirs May 20, 2026
3dbd8c8
fix: rescue RecordInvalid in teacher join path for concurrent races
fspeirs May 20, 2026
3576349
fix: return model errors instead of raw exception in regenerate_join_…
fspeirs May 20, 2026
35b5567
test: drop duplicate regenerate_join_code 200 example
fspeirs May 20, 2026
40452ea
refactor: make join#create case statement exhaustive
fspeirs May 21, 2026
1e53dcc
refactor: rename School#valid_email? to email_domain_in_school_domains?
fspeirs May 21, 2026
c055342
refactor: extract JoinStatusService from JoinController
fspeirs May 21, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions app/controllers/api/join_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
# frozen_string_literal: true

module Api
class JoinController < ApiController
Comment thread
fspeirs marked this conversation as resolved.
before_action :authorize_user, only: :create
before_action :find_school_and_class

def show
@status = show_status
render :show, formats: [:json], status: :ok
end

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 :joinable
add_student_to_school_and_class
render json: { redirect_url: class_redirect_path }, status: :ok
else
raise "Unexpected join action_status: #{action_status.inspect}"
end
end

private

def find_school_and_class
@school_class = SchoolClass.find_by!(join_code: JoinCodeGenerator.normalize(params[:join_code]))
@school = @school_class.school
end

def show_status
return :unauthenticated unless current_user

action_status
end

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)
ClassStudent.find_or_create_by!(school_class: @school_class, student_id: current_user.id) do |class_student|
class_student.student = current_user
end
end
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.
rescue ActiveRecord::RecordInvalid => e
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
8 changes: 8 additions & 0 deletions app/controllers/api/school_classes_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,14 @@ def destroy
end
end

def regenerate_join_code
@school_class.regenerate_join_code!
@school_class_with_teachers = @school_class.with_teachers
render :show, formats: [:json], status: :ok
rescue ActiveRecord::RecordInvalid
render json: { error: @school_class.errors.full_messages.to_sentence }, status: :unprocessable_content
end

private

def render_student_index(school_classes)
Expand Down
4 changes: 2 additions & 2 deletions app/models/ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def define_authenticated_non_student_abilities(user)
def define_school_owner_abilities(school:)
can(%i[read update destroy], School, id: school.id)
can(%i[read], :school_member)
can(%i[read create import update destroy], SchoolClass, school: { id: school.id })
can(%i[read create import update destroy regenerate_join_code], SchoolClass, school: { id: school.id })
can(%i[read show_context], Project, school_id: school.id, lesson: { visibility: %w[teachers students] })
can(%i[read create create_batch destroy], ClassStudent, school_class: { school: { id: school.id } })
can(%i[read create destroy], :school_owner)
Expand All @@ -78,7 +78,7 @@ def define_school_teacher_abilities(user:, school:)
can(%i[read], School, id: school.id)
can(%i[read], :school_member)
can(%i[create import], SchoolClass, school: { id: school.id })
can(%i[read update destroy], SchoolClass, school: { id: school.id }, teachers: { teacher_id: user.id })
can(%i[read update destroy regenerate_join_code], 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)
Expand Down
13 changes: 13 additions & 0 deletions app/models/school.rb
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,26 @@ def import_in_progress?
.exists?(description: id)
end

def auto_join_enabled?
school_email_domains.exists?
end

def valid_domain?(candidate_domain)
validated_domain = SchoolEmailDomainValidator.call(candidate_domain)
school_email_domains.exists?(domain: validated_domain)
rescue ::SchoolEmailDomainValidator::Error
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
Expand Down
27 changes: 27 additions & 0 deletions app/models/school_class.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@ class SchoolClass < ApplicationRecord
scope :with_teachers, ->(user_id) { joins(:teachers).where(teachers: { id: user_id }) }

before_validation :assign_class_code, on: %i[create import]
before_validation :assign_join_code, on: %i[create import]

validates :name, presence: true
validates :code, uniqueness: { scope: :school_id }, presence: true, format: { with: /\d\d-\d\d-\d\d/, allow_nil: false }
validates :join_code, uniqueness: true, presence: true, format: { with: JoinCodeGenerator::FORMAT_REGEX, allow_nil: false }
Comment thread
fspeirs marked this conversation as resolved.
validate :code_cannot_be_changed
validate :school_class_has_at_least_one_teacher

Expand Down Expand Up @@ -58,6 +60,27 @@ def assign_class_code
errors.add(:code, 'could not be generated')
end

def assign_join_code
return if join_code.present?

5.times do
self.join_code = JoinCodeGenerator.generate
return if join_code_is_unique?
end

errors.add(:join_code, 'could not be generated')
end

def regenerate_join_code!
self.join_code = nil
assign_join_code
save!
rescue ActiveRecord::RecordNotUnique
self.join_code = nil
assign_join_code
save!
end

def submitted_projects_count
lessons.to_a.sum(&:submitted_projects_count)
end
Expand All @@ -77,4 +100,8 @@ def code_cannot_be_changed
def code_is_unique_within_school?
code.present? && SchoolClass.where(code:, school:).none?
end

def join_code_is_unique?
join_code.present? && SchoolClass.where(join_code:).where.not(id:).none?
Comment thread
fspeirs marked this conversation as resolved.
end
end
72 changes: 72 additions & 0 deletions app/services/join_status_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
# frozen_string_literal: true

# Computes the join "status" symbol that describes the relationship between
# `user` and `school_class` — the controller uses it to decide what HTTP
# response to return and whether to enrol the user.
#
# 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
# :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
@school_class = school_class
@user = user
end

def call
return :already_member if user_is_member_of_class?
return existing_user_join_status if user_has_role_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_role?
return :wrong_school if user_in_different_school?
return :domain_mismatch unless @school.email_domain_in_school_domains?(@user.email)

:joinable
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])
end

def user_has_role_in_school?
Role.exists?(school: @school, user_id: @user.id)
end

def user_has_non_student_role?
Role.where(user_id: @user.id).where.not(role: Role.roles[:student]).exists?
end

def user_in_different_school?
Role.where(user_id: @user.id).where.not(school_id: @school.id).exists?
end
end
11 changes: 11 additions & 0 deletions app/views/api/join/show.json.jbuilder
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# frozen_string_literal: true

json.status @status.to_s
json.school do
json.code @school.code
json.name @school.name
end
json.school_class do
json.code @school_class.code
json.name @school_class.name
end
3 changes: 2 additions & 1 deletion app/views/api/school_classes/_school_class.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ json.call(
:created_at,
:updated_at,
:import_origin,
:import_id
:import_id,
:join_code
)

json.teachers(teachers) do |teacher|
Expand Down
1 change: 1 addition & 0 deletions app/views/api/schools/_school.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,4 @@ include_user_origin = local_assigns.fetch(:user_origin, false)
json.user_origin(school.user_origin) if include_user_origin

json.import_in_progress school.import_in_progress?
json.auto_join_enabled school.auto_join_enabled?
Comment thread
fspeirs marked this conversation as resolved.
4 changes: 4 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
resources :members, only: %i[index], controller: 'school_members'
resources :classes, only: %i[index show create update destroy], controller: 'school_classes' do
post :import, on: :collection
post :regenerate_join_code, on: :member
resources :members, only: %i[index create destroy], controller: 'class_members' do
Comment thread
fspeirs marked this conversation as resolved.
post :batch, on: :collection, to: 'class_members#create_batch'
end
Expand Down Expand Up @@ -100,6 +101,9 @@

resources :profile_auth_check, only: %i[index]
resources :subscriptions, only: %i[create]

get '/join/:join_code', to: 'join#show'
post '/join/:join_code', to: 'join#create'
end

resource :github_webhooks, only: :create, defaults: { formats: :json }
Expand Down
8 changes: 8 additions & 0 deletions db/migrate/20260420104938_add_join_code_to_school_classes.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# frozen_string_literal: true

class AddJoinCodeToSchoolClasses < ActiveRecord::Migration[7.2]
def change
add_column :school_classes, :join_code, :string
Comment thread
fspeirs marked this conversation as resolved.
add_index :school_classes, :join_code, unique: true
end
end
35 changes: 35 additions & 0 deletions db/migrate/20260420104939_backfill_join_code_for_school_classes.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# frozen_string_literal: true

class BackfillJoinCodeForSchoolClasses < ActiveRecord::Migration[7.2]
MAX_ATTEMPTS = 10

def up
SchoolClass.where(join_code: nil).find_each do |school_class|
backfill_with_retries(school_class)
end
end

def down
# No need to revert - join codes can stay
end
Comment thread
fspeirs marked this conversation as resolved.

private

def backfill_with_retries(school_class)
MAX_ATTEMPTS.times do
school_class.join_code = nil
school_class.assign_join_code
next if school_class.errors[:join_code].any?

begin
school_class.save!(validate: false)
return
rescue ActiveRecord::RecordNotUnique
school_class.errors.clear
next
end
end

raise "Could not generate a unique join_code for SchoolClass##{school_class.id} after #{MAX_ATTEMPTS} attempts"
end
end
2 changes: 2 additions & 0 deletions db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 29 additions & 0 deletions lib/join_code_generator.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# frozen_string_literal: true

class JoinCodeGenerator
# Omit K, X, Z — commonly confused or offensive in short codes.
CONSONANTS = %w[B C D F G H J L M N P Q R S T V W Y].freeze

# Format: CDDD-CDDD (e.g., B123-C456). C = consonant from CONSONANTS, D = digit.
FORMAT_REGEX = Regexp.new("\\A(?:#{CONSONANTS.join('|')})\\d{3}-(?:#{CONSONANTS.join('|')})\\d{3}\\z").freeze
Comment thread
fspeirs marked this conversation as resolved.

cattr_accessor :random

self.random ||= Random.new

def self.generate
seg = lambda do
"#{CONSONANTS.sample(random: random)}#{format('%03d', random.rand(1000))}"
end

"#{seg.call}-#{seg.call}"
end

# Canonical hyphenated form for DB lookup; accepts typed codes with or without a hyphen.
def self.normalize(raw)
alnum = raw.to_s.upcase.gsub(/[^A-Z0-9]/, '')
return alnum if alnum.length != 8

"#{alnum[0]}#{alnum[1, 3]}-#{alnum[4]}#{alnum[5, 3]}"
end
end
1 change: 1 addition & 0 deletions spec/factories/school_class.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
factory :school_class do
sequence(:name) { |n| "Class #{n}" }
code { ForEducationCodeGenerator.generate }
join_code { JoinCodeGenerator.generate }

transient do
teacher_ids { [SecureRandom.uuid] }
Expand Down
Loading
Loading