Change creator_id to only need to be unique for schools where rejecte…#742
Change creator_id to only need to be unique for schools where rejecte…#742jamiebenstead wants to merge 1 commit intomainfrom
Conversation
Test coverage89.79% line coverage reported by SimpleCov. |
There was a problem hiding this comment.
Pull request overview
This PR updates the School model so a creator_id only needs to be unique among non-rejected schools, enabling a user to create a new school after their previous one was rejected.
Changes:
- Scope
creator_iduniqueness validation to records whererejected_atisnil.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 |
There was a problem hiding this comment.
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).
| 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 |
| validates :creator_id, | ||
| presence: true, | ||
| uniqueness: { | ||
| conditions: -> { where(rejected_at: nil) } | ||
| } |
There was a problem hiding this comment.
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).
| validates :creator_id, | ||
| presence: true, | ||
| uniqueness: { | ||
| conditions: -> { where(rejected_at: nil) } | ||
| } |
There was a problem hiding this comment.
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).
Status
What's changed?
creator_idcan be reused ifrejected_atis not nil