Make NCES ID mandatory for US schools #638

Mohamed-RPF · 2025-12-08T14:07:31Z

Make NCES ID mandatory for US schools

Add conditional presence validation for US NCES ID (district_nces_id) fields, with format validation and scoped uniqueness to allow reuse after school rejection. This improves validation error handling so users receive specific, actionable error messages instead of generic form errors.

Status

Closes: https://github.com/orgs/RaspberryPiFoundation/projects/51/views/11?pane=issue&itemId=120513360&issue=RaspberryPiFoundation%7Cdigital-editor-issues%7C780

Related to: (standalone url)

Points for consideration:

Security

Partial unique indexes prevent duplicate entries among active (non-rejected) schools
Format validation enforces strict digit patterns (5-6 digits for URN, 12 digits for NCES ID)
Case-insensitive uniqueness check prevents bypass attempts

What's changed?

Database:

Converted reference index to partial unique index (WHERE rejected_at IS NULL)
Converted district_nces_id index to partial unique index (WHERE rejected_at IS NULL)
This allows rejected schools to release their identifiers for reuse by legitimate schools

Model (School):

Added conditional presence validation: reference required for UK (country_code == 'GB')
Added conditional presence validation: district_nces_id required for US (country_code == 'US')
Added format validation: URN must be 5-6 digits, NCES ID must be 12 digits
Added scoped uniqueness: only active (non-rejected) schools are checked for duplicates
Added united_kingdom? and united_states? helper methods

API Error Responses:

Field	Validation	Error Message
`reference`	presence	`"can't be blank"`
`reference`	uniqueness	`"has already been taken"`
`reference`	format	`"must be 5-6 digits (e.g., 100000)"`
`district_nces_id`	presence	`"can't be blank"`
`district_nces_id`	uniqueness	`"has already been taken"`
`district_nces_id`	format	`"must be 12 digits (e.g., 010000000001)"`

Tests:

16 comprehensive validation tests covering:
- Conditional presence (UK/US specific)
- Format validation (valid/invalid patterns)
- Scoped uniqueness (allows reuse after rejection)
Updated factory to include valid reference for default GB schools

…number-validation-error-message-and-form-state-api

lib/concepts/school/operations/create.rb

      rescue StandardError => e
        Sentry.capture_exception(e)
        response[:error] = response[:school].errors
+        response[:error_types] = response[:school].errors.details


lib/concepts/school/operations/create.rb

      rescue StandardError => e
        Sentry.capture_exception(e)
        response[:error] = response[:school].errors
+        response[:error_types] = response[:school].errors.details


Copilot

Pull request overview

This PR aims to make URN (reference) mandatory for UK schools and NCES ID (district_nces_id) mandatory for US schools, with format validation and scoped uniqueness constraints. However, there's a critical bug: the reference field is missing presence: true validation in the model, meaning URN is not actually mandatory for UK schools despite the PR's stated purpose.

Key changes:

Converted database indexes for reference and district_nces_id to partial unique indexes allowing rejected schools to release identifiers for reuse
Added format validation for URN (5-6 digits) and NCES ID (12 digits) fields
Added district_name as required field for US schools
Enhanced error responses to include detailed error types for better API feedback

Reviewed changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
app/models/school.rb	Adds conditional format and uniqueness validations for reference/district_nces_id, plus united_kingdom?/united_states? helper methods. Missing presence validation for reference.
db/migrate/20251208134354_change_reference_and_nces_id_indexes_to_partial.rb	Converts reference and district_nces_id indexes to partial unique indexes (WHERE rejected_at IS NULL)
db/schema.rb	Reflects updated partial index constraints
spec/models/school_spec.rb	Adds comprehensive validation tests, but test on line 143-147 incorrectly asserts reference is not required for UK schools
spec/factories/school.rb	Adds sequential reference generation for default GB schools
spec/features/school/creating_a_school_spec.rb	Updates school creation to include reference field
spec/features/admin/schools_spec.rb	Sets reference to nil when testing US school scenarios
spec/concepts/school/create_spec.rb	Updates school creation params to include reference
spec/jobs/school_import_job_spec.rb	Adds district_name and district_nces_id to US school test data
lib/tasks/seeds_helper.rb	Adds conditional population of country-specific identifier fields, but missing district_name for US schools
lib/concepts/school/operations/create.rb	Adds error_types to operation response for detailed error handling
app/jobs/school_import_job.rb	Passes through district_name, district_nces_id, and school_roll_number fields
app/controllers/api/schools_controller.rb	Returns error_types in addition to error messages for create endpoint
config/locales/en.yml	Adds custom error messages for reference, district_nces_id, and school_roll_number validations
Gemfile.lock	Adds arm64-darwin-25 platform (developer environment update)

spec/models/school_spec.rb

app/models/school.rb

lib/tasks/seeds_helper.rb

spec/models/school_spec.rb

Copilot

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 7 comments.

Copilot · 2026-01-06T14:10:53Z

config/locales/en.yml

    school:
      website: "must be a valid URL"
+      reference: "must be 5-6 digits (e.g., 100000)"
+      reference_urn_exists: "URN number already exists"


The custom uniqueness error message "URN number already exists" is inconsistent with the standard Rails error messages used elsewhere. According to the PR description's API Error Responses table, the uniqueness error should be "has already been taken" to match Rails conventions.

Additionally, using "URN number" is redundant since URN already stands for "Unique Reference Number". Consider using either "URN has already been taken" or "reference has already been taken" for consistency.

Suggested change

reference_urn_exists: "URN number already exists"

reference_urn_exists: "URN has already been taken"

Copilot · 2026-01-06T14:10:54Z

config/locales/en.yml

+      district_nces_id: "must be 12 digits (e.g., 010000000001)"
+      district_nces_id_exists: "NCES ID already exists"
      school_roll_number: "must be numbers followed by letters (e.g., 01572D)"
+      school_roll_number_exists: "School roll number already exists"


The custom uniqueness error message "School roll number already exists" is inconsistent with the standard Rails error messages. According to the PR description's API Error Responses table and Rails conventions, the uniqueness error should be "has already been taken".

Consider changing this to "has already been taken" for consistency with Rails standards.

Suggested change

school_roll_number_exists: "School roll number already exists"

school_roll_number_exists: "has already been taken"

Copilot · 2026-01-06T14:10:54Z

app/models/school.rb

+            format: { with: /\A\d{5,6}\z/, allow_nil: true, message: I18n.t('validations.school.reference') },
+            if: :united_kingdom?
+  validates :district_nces_id,
+            uniqueness: { conditions: -> { where(rejected_at: nil) }, case_sensitive: false, allow_blank: true, message: I18n.t('validations.school.district_nces_id_exists') },


The format validation uses allow_nil: true, but the uniqueness validation uses allow_blank: true. This creates an inconsistency where an empty string would pass uniqueness validation but fail format validation.

Since there's a normalize_district_fields callback (lines 114-117) that converts blank values to nil, both validations should use allow_nil: true for consistency, or the format validation should also use allow_blank: true.

Suggested change

uniqueness: { conditions: -> { where(rejected_at: nil) }, case_sensitive: false, allow_blank: true, message: I18n.t('validations.school.district_nces_id_exists') },

uniqueness: { conditions: -> { where(rejected_at: nil) }, case_sensitive: false, allow_nil: true, message: I18n.t('validations.school.district_nces_id_exists') },

Copilot · 2026-01-06T14:10:55Z

spec/models/school_spec.rb

+    it 'returns error if school_roll_number is not unique' do
+      duplicate_school = build(:school, school_roll_number: '01572D', country_code: 'IE')
+      duplicate_school.valid?
+      expect(duplicate_school.errors.details[:school_roll_number]).to include(hash_including(error: :taken))


The error message assertion expects the :taken error symbol, but the model validation on line 30 of school.rb specifies a custom message I18n.t('validations.school.school_roll_number_exists') which translates to "School roll number already exists" according to config/locales/en.yml line 23.

The test should expect the custom error message instead of checking for the :taken error symbol to accurately reflect the actual validation behavior.

Suggested change

expect(duplicate_school.errors.details[:school_roll_number]).to include(hash_including(error: :taken))

expect(duplicate_school.errors[:school_roll_number]).to include(I18n.t('validations.school.school_roll_number_exists'))

Copilot · 2026-01-06T14:10:55Z

config/locales/en.yml

+      reference: "must be 5-6 digits (e.g., 100000)"
+      reference_urn_exists: "URN number already exists"
+      district_nces_id: "must be 12 digits (e.g., 010000000001)"
+      district_nces_id_exists: "NCES ID already exists"


The custom uniqueness error message "NCES ID already exists" is inconsistent with the standard Rails error messages. According to the PR description's API Error Responses table, the uniqueness error should be "has already been taken" to match Rails conventions.

Consider changing this to "has already been taken" for consistency with Rails standards and the documented API response format.

Suggested change

district_nces_id_exists: "NCES ID already exists"

district_nces_id_exists: "has already been taken"

Copilot · 2026-01-06T14:10:55Z

spec/models/school_spec.rb

+
+      duplicate_school = build(:school, reference: '100000')
+      duplicate_school.valid?
+      expect(duplicate_school.errors.details[:reference]).to include(hash_including(error: :taken))


The error message assertion expects the Rails default message "has already been taken", but the model validation on line 20 of school.rb specifies a custom message I18n.t('validations.school.reference_urn_exists') which translates to "URN number already exists" according to config/locales/en.yml line 19.

The test should expect the custom error message instead of the default Rails message to accurately reflect the actual validation behavior.

Suggested change

expect(duplicate_school.errors.details[:reference]).to include(hash_including(error: :taken))

expect(duplicate_school.errors[:reference]).to include(I18n.t('validations.school.reference_urn_exists'))

Copilot · 2026-01-06T14:10:56Z

spec/models/school_spec.rb

+    it 'returns error if district_nces_id is not unique' do
+      duplicate_school = build(:school, country_code: 'US', district_nces_id: '010000000001')
+      duplicate_school.valid?
+      expect(duplicate_school.errors.details[:district_nces_id]).to include(hash_including(error: :taken))


The error message assertion expects the Rails default message "has already been taken" via the :taken symbol, but the model validation on line 24 of school.rb specifies a custom message I18n.t('validations.school.district_nces_id_exists') which translates to "NCES ID already exists" according to config/locales/en.yml line 21.

The test should expect the custom error message instead of checking for the :taken error symbol to accurately reflect the actual validation behavior.

Suggested change

expect(duplicate_school.errors.details[:district_nces_id]).to include(hash_including(error: :taken))

expect(duplicate_school.errors[:district_nces_id]).to include(I18n.t('validations.school.district_nces_id_exists'))

Mohamed-RPF self-assigned this Dec 8, 2025

Mohamed-RPF added enhancement New feature or request API labels Dec 8, 2025

cla-bot bot added the cla-signed label Dec 8, 2025

Mohamed-RPF added 2 commits December 8, 2025 14:31

Make URN mandatory for UK schools and NCES ID mandatory for US schools

980647f

Fix tests: add required reference/district_nces_id for UK/US schools

27313f4

Mohamed-RPF force-pushed the 780-improve-urn-nces-id-and-and-school-roll-number-validation-error-message-and-form-state-api branch from 8994994 to 27313f4 Compare December 8, 2025 14:31

Mohamed-RPF and others added 6 commits December 8, 2025 14:37

Fix tests: add required reference/district_nces_id for UK/US schools

a791df2

Fix seeds helper: add required country-specific fields

37847a1

Merge branch 'main' into 780-improve-urn-nces-id-and-and-school-roll-…

9313869

…number-validation-error-message-and-form-state-api

Fix whitespace

0798d34

Update schema

0559048

Add platform to build process

b54866d

raspberrypiherokubot temporarily deployed to editor-api-p-780-improv-u1dpka December 17, 2025 11:54 Inactive

jamiebenstead added 2 commits December 17, 2025 12:01

Update test

508b66f

Add mapping, fix test data

dae0327

adrian-rpf assigned jamiebenstead and unassigned Mohamed-RPF Dec 17, 2025

jamiebenstead added 4 commits December 17, 2025 16:18

Add validation and tests

e953dd7

Return error message if ID exists

aba460d

Return an error code if not unique

6877786

Add model tests

f059d13

github-code-quality bot found potential problems Jan 5, 2026

View reviewed changes

jamiebenstead added 2 commits January 6, 2026 10:29

Add now required district_name and district_nces_id to tests

bef5de5

Update test to add required field

023a0bd

jamiebenstead requested a review from Copilot January 6, 2026 12:49

Copilot started reviewing on behalf of jamiebenstead January 6, 2026 12:49 View session

Copilot AI reviewed Jan 6, 2026

View reviewed changes

jamiebenstead changed the title ~~Make URN mandatory for UK schools and NCES ID mandatory for US schools~~ Make NCES ID mandatory for US schools Jan 6, 2026

Add required seed data

581db1d

jamiebenstead added 2 commits January 6, 2026 14:04

Fix typo

778280a

Fix test data

1785e13

jamiebenstead requested a review from Copilot January 6, 2026 14:06

Copilot started reviewing on behalf of jamiebenstead January 6, 2026 14:06 View session

Copilot AI reviewed Jan 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make NCES ID mandatory for US schools #638

Make NCES ID mandatory for US schools #638

Uh oh!

Mohamed-RPF commented Dec 8, 2025 •

edited by jamiebenstead

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	reference_urn_exists: "URN number already exists"
	reference_urn_exists: "URN has already been taken"

	school_roll_number_exists: "School roll number already exists"
	school_roll_number_exists: "has already been taken"

	uniqueness: { conditions: -> { where(rejected_at: nil) }, case_sensitive: false, allow_blank: true, message: I18n.t('validations.school.district_nces_id_exists') },
	uniqueness: { conditions: -> { where(rejected_at: nil) }, case_sensitive: false, allow_nil: true, message: I18n.t('validations.school.district_nces_id_exists') },

	expect(duplicate_school.errors.details[:school_roll_number]).to include(hash_including(error: :taken))
	expect(duplicate_school.errors[:school_roll_number]).to include(I18n.t('validations.school.school_roll_number_exists'))

	district_nces_id_exists: "NCES ID already exists"
	district_nces_id_exists: "has already been taken"

	expect(duplicate_school.errors.details[:reference]).to include(hash_including(error: :taken))
	expect(duplicate_school.errors[:reference]).to include(I18n.t('validations.school.reference_urn_exists'))

	expect(duplicate_school.errors.details[:district_nces_id]).to include(hash_including(error: :taken))
	expect(duplicate_school.errors[:district_nces_id]).to include(I18n.t('validations.school.district_nces_id_exists'))

Make NCES ID mandatory for US schools #638

Are you sure you want to change the base?

Make NCES ID mandatory for US schools #638

Uh oh!

Conversation

Mohamed-RPF commented Dec 8, 2025 • edited by jamiebenstead Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Make NCES ID mandatory for US schools

Status

Points for consideration:

Security

What's changed?

Database:

Model (School):

API Error Responses:

Tests:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Mohamed-RPF commented Dec 8, 2025 •

edited by jamiebenstead

Loading