Skip to content

Change creator_id to only need to be unique for schools where rejecte… #742

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jamiebenstead wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Change creator_id to only need to be unique for schools where rejecte… #742

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion app/models/school.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,11 @@ class School < ApplicationRecord
format: { with: /\A[0-9]+[A-Z]+\z/, allow_nil: true, message: I18n.t('validations.school.school_roll_number') },
presence: true,
if: :ireland?
validates :creator_id, presence: true, uniqueness: true
validates :creator_id,
presence: true,
uniqueness: {
conditions: -> { where(rejected_at: nil) }
}
Comment on lines +34 to +38
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allowing multiple schools per creator_id (when prior ones are rejected) makes School.find_for_user! ambiguous: it currently does find_by(creator_id: user.id) without preferring the active (non-rejected) school. This can return an older rejected school, causing /api/my_school to surface the wrong record. Consider scoping the lookup to rejected_at: nil (or otherwise selecting the most relevant school deterministically).

Copilot uses AI. Check for mistakes.
Comment on lines +34 to +38
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are specs covering reuse for reference, district_nces_id, and school_roll_number when a school is rejected, but no equivalent coverage for the new creator_id reuse rule. Add a model spec that rejects the original school and asserts a second school with the same creator_id is valid/saves (and ideally a spec asserting find_for_user! returns the non-rejected one when multiple exist).

Copilot uses AI. Check for mistakes.
validates :creator_agree_authority, presence: true, acceptance: true
validates :creator_agree_terms_and_conditions, presence: true, acceptance: true
validates :creator_agree_responsible_safeguarding, presence: true, acceptance: true
Comment on lines +36 to 41
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The model validation now scopes creator_id uniqueness to rejected_at: nil, but the database still enforces a global unique index on schools.creator_id (index_schools_on_creator_id is unique in db/schema.rb). As a result, creating a new school for a creator whose previous school was rejected will still fail at the DB layer. Add a migration to replace the existing unique index with a partial unique index (WHERE rejected_at IS NULL) (and ensure the index name is stable / removal is idempotent).

Suggested change
uniqueness: {
conditions: -> { where(rejected_at: nil) }
}
validates :creator_agree_authority, presence: true, acceptance: true
validates :creator_agree_terms_and_conditions, presence: true, acceptance: true
validates :creator_agree_responsible_safeguarding, presence: true, acceptance: true
uniqueness: true
validates :creator_agree_authority, presence: true, acceptance: true
validates :creator_agree_terms_and_conditions, presence: true, acceptance: true
validates :creator_agree_responsible_safeguarding, presence: true, acceptance: true
validates :creator_agree_terms_and_conditions, presence: true, acceptance: true
validates :creator_agree_responsible_safeguarding, presence: true, acceptance: true

Copilot uses AI. Check for mistakes.
Expand Down
Loading