Skip to content

[F] Add external identifiers#3968

Merged
timfrazee merged 3 commits intotf/saml-entitlementsfrom
ts/ad-external-identifiers
Nov 25, 2025
Merged

[F] Add external identifiers#3968
timfrazee merged 3 commits intotf/saml-entitlementsfrom
ts/ad-external-identifiers

Conversation

@timbot1789
Copy link
Contributor

@timbot1789 timbot1789 commented Nov 21, 2025

Resolves PROJ-3275

This PR adds an external_identifier attribute to the Projects, ProjectCollections, Journals, and UserGroups APIs. This attribute is backed by a database table and ActiveRecord model, but is not exposed as an API endpoint itself.

@timbot1789 timbot1789 requested a review from timfrazee November 21, 2025 20:47
@timbot1789 timbot1789 force-pushed the ts/ad-external-identifiers branch from baa1702 to 790493e Compare November 21, 2025 20:48
Copy link
Contributor

@timfrazee timfrazee left a comment

Choose a reason for hiding this comment

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

All in all, looks good. 2 requested changes:

  1. Please move the strong params back to concerns/validation. I know the validation concern is a strange/outdated pattern, but unless we make a decision to move all strong params into their respective controllers, we should follow the existing pattern.

  2. Please add a unique index to external_identifiers#identifier in the migration. This will ensure uniqueness if the validations are ever bypassed, and the index will help since we'll often be querying by the identifier string.

@timfrazee timfrazee force-pushed the ts/ad-external-identifiers branch from 790493e to 7025a33 Compare November 25, 2025 18:52
@timfrazee timfrazee merged commit 622ab91 into tf/saml-entitlements Nov 25, 2025
1 of 3 checks passed
@timfrazee timfrazee deleted the ts/ad-external-identifiers branch November 25, 2025 18:55
@timfrazee
Copy link
Contributor

I've made the requested changes above so I can merge this into the saml-entitlements branch and continue work there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants