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

Merged
jamiebenstead merged 6 commits into from
Mar 24, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
400d122
Change creator_id to only need to be unique for schools where rejecte…
jamiebenstead Mar 20, 2026
aa91052
Update find_for_user to only return user where rejected_at is nil
jamiebenstead Mar 23, 2026
c84d839
Add migration so creator_id is only unique on table where rejected_at…
jamiebenstead Mar 23, 2026
d5b80e7
Change migration to be reversible if needed
jamiebenstead Mar 23, 2026
9c5b01c
Run db migration
jamiebenstead Mar 24, 2026
4e3a399
Add test for not returning user if user is creator of rejected school
jamiebenstead Mar 24, 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
8 changes: 6 additions & 2 deletions 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 thread
jamiebenstead marked this conversation as resolved.
Comment thread
jamiebenstead marked this conversation as resolved.
Comment on lines +34 to +38

Copilot AI Mar 23, 2026

Copy link

Choose a reason for hiding this comment

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

Test coverage: this changes creator_id uniqueness semantics to allow reuse when the existing school is rejected. Please add/adjust specs to cover the new behavior (duplicate creator_id allowed when the other record has rejected_at set, but still rejected when both are active).

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 thread
jamiebenstead marked this conversation as resolved.
Expand All @@ -53,7 +57,7 @@ class School < ApplicationRecord
after_create :generate_code!, if: -> { FeatureFlags.immediate_school_onboarding? }

def self.find_for_user!(user)
school = Role.find_by(user_id: user.id)&.school || find_by(creator_id: user.id)
school = Role.find_by(user_id: user.id)&.school || find_by(creator_id: user.id, rejected_at: nil)
raise ActiveRecord::RecordNotFound unless school
Comment on lines 59 to 61

Copilot AI Mar 23, 2026

Copy link

Choose a reason for hiding this comment

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

Test coverage: .find_for_user! now excludes schools created by the user if they are rejected (rejected_at set). Add a spec for the rejected-school case (e.g., creator has only a rejected school -> RecordNotFound, and creator has both rejected + active -> returns the active one).

Copilot uses AI. Check for mistakes.
Comment on lines 59 to 61

Copilot AI Mar 23, 2026

Copy link

Choose a reason for hiding this comment

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

This change introduces new behavior (creator_id can be reused when the existing school is rejected, and .find_for_user! should ignore rejected schools). There are existing model specs for creator_id uniqueness and .find_for_user! (spec/models/school_spec.rb), but they don't appear to cover the rejected-school cases. Please add/adjust tests to assert that a second school with the same creator_id is valid when the first has rejected_at set, and that .find_for_user! returns the active school (or raises) when only a rejected school exists.

Copilot uses AI. Check for mistakes.

school
Expand Down
20 changes: 20 additions & 0 deletions db/migrate/20260323152328_update_creator_id_index_on_schools.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
class UpdateCreatorIdIndexOnSchools < ActiveRecord::Migration[7.2]
def up
remove_index :schools, name: "index_schools_on_creator_id"

add_index :schools,
:creator_id,
unique: true,
where: "rejected_at IS NULL",
name: "index_schools_on_creator_id_active_only"

Copilot AI Mar 23, 2026

Copy link

Choose a reason for hiding this comment

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

The new partial index is given a non-standard name (index_schools_on_creator_id_active_only). For consistency with the existing partial unique indexes on schools (e.g. index_schools_on_reference), consider reusing the conventional name index_schools_on_creator_id for the replacement index to reduce future migration complexity.

Suggested change
name: "index_schools_on_creator_id_active_only"
name: "index_schools_on_creator_id"

Copilot uses AI. Check for mistakes.

@zetter-rpf zetter-rpf Mar 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't have a preference either way. I don't see any problem with the more descriptive name as you only tend to refer to index names from within other migrations. I think there is a limit to the name length but this would likely be within it.

I would choose what you think is clearer.

end

def down
remove_index :schools, name: "index_schools_on_creator_id_active_only"

add_index :schools,
:creator_id,
unique: true,
name: "index_schools_on_creator_id"
Comment on lines +13 to +18

Copilot AI Mar 23, 2026

Copy link

Choose a reason for hiding this comment

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

The down migration recreates a global unique index on creator_id. After this change is deployed, the table may legitimately contain multiple rows with the same creator_id (as long as only one is active), so rolling back would fail when re-adding this index. Consider marking the migration as irreversible, or adding a rollback strategy (e.g., clean up/merge duplicate rows or raise with a clear message before attempting to add the unique index).

Suggested change
remove_index :schools, name: "index_schools_on_creator_id_active_only"
add_index :schools,
:creator_id,
unique: true,
name: "index_schools_on_creator_id"
raise ActiveRecord::IrreversibleMigration,
"Cannot safely restore global unique index on creator_id because " \
"multiple rows per creator_id may exist after this migration."

Copilot uses AI. Check for mistakes.

@zetter-rpf zetter-rpf Mar 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This risk here is that if we did roll back, and the remove_index succeeded, but the add_index didn't, we would could get duplicates.

This is a small risk as it only applies if we decide to manually roll back the code and migrations so I don't have a strong preference as to if it's worth changing or not - up to you.

end
end
4 changes: 2 additions & 2 deletions db/schema.rb
View file
Open in desktop

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

8 changes: 8 additions & 0 deletions spec/models/school_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,14 @@
user = build(:user)
expect { described_class.find_for_user!(user) }.to raise_error(ActiveRecord::RecordNotFound)
end

it('raises ActiveRecord::RecordNotFound if the user is the creator of a rejected school') do
creator = create(:user)
school.update!(creator_id: creator.id)
school.reject

expect { described_class.find_for_user!(creator) }.to raise_error(ActiveRecord::RecordNotFound)
end
end

describe '#verified?' do
Expand Down
Loading