Updated required fields by thelmick-unicon · Pull Request #1 · LIF-Initiative/lif-core

thelmick-unicon · 2025-11-26T19:53:56Z

No description provided.

mgwozdz-unicon

I have some change requests and considerations:

ProficiencyLevel should probably have identifier attribute added as required. The competencyId attribute should probably be deleted as inconsistent. It looks like ProficiencyLevel is a grandchild of Competency, so in theory we should already know the competencyId.
CredentialAward.id should be changed to CredentialAward.identifier for consistency.
Accreditation should have identifier added as required and should not require ctid in case someone is not using Credential Engine.
imageId attribute should be changed to identifier for consistency.
Are we using a consistent pattern for when informationSourceSystem is required? I feel like it should either always be required or never be required unless we have a specific reason to have exceptions for this.
For the *LearningExperience Entities, I think it would be good to have a consistent set of required fields for these where name exists and is required for all or is required for none and identifier exists and is either required for all or required for none of these Entities. I think I would lean towards having and requiring both for all of these.
Identifier entity should have identifier as a required attribute.
Can we add name as a required field of Credential, CredentialAward, Program, CompetencyFramework, and Membership?
CompetencyFramework and Credential should have id renamed to identifier for consistency.
We might want to consider a new rule where every entity is required to have identifier as a child attribute that is required.
Can you make sure that the required fields for Accreditation, Requirements, OrganizationCode, Course, and Program match for Base LIF and StateU LIF? I think some fields appear to be missing for StateU LIF; they probably need to be marked as inclusions. Can you check all of these please? We should be able to do a sanity check where we look at every "required" array and can see that it contains something other than just informationSourceId and informationSourceOrganization.
Could you please also comment out the \restrict and \unrestrict lines in the V1.1 file?

thelmick-unicon

Done.
Done.
Done.
Done.
Per the LIF Data Model rules, it is optional. I've marked it not required on all entities.
I've added name and identifier for all *LearningExperience Entities and marked them both as required.
Done.
Done.
Done.
I've added identifier to every Entity and marked it required. I also updated the LIF Data Model rules to note this.
Each of these (Accreditation, Requirements, OrganizationCode, Course, and Program) now have additional required fields beyond informationSourceId and informationSourceOrganization, and I've made sure to include all required fields for those Entities listed in the StateU Data Model.
Done.

mgwozdz-unicon

This is looking much better! However, the StateU LIF is not following the rules of all entities having identifier and it is also missing name in a few entities where it is important to have. While doing that check, it also became apparent that there are 2 confusing duplicates in StateU LIF that I called out below.

StateU Entities missing required identifier attribute:

CourseLearningExperience
Organization
EmploymentLearningExperience
Proficiency
Identity
Position
Name
Contact
Address
Email
Telephone
EmploymentPreferences
Narrative
Texts
PositionPreferences
Relocation (or these 2 could be flattened, but that would mean flattening in sample files too)
RemoteWork (or this could be flattened, but that would mean flattening in sample files too)
Travel
RemunerationPackage
Interactions
Requirements
CredentialAlignments
Accreditation
OperationalStatus
Ranges
MinimumAmount
Assessment

StateU Entities missing required name attribute:

CourseLearningExperience
CredentialAward
Position
Assessment

StateU appears to have odd duplicates of Accreditation and Identifier. Could you please delete Accreditation with Id=369 and Identifier with Id=371. Identifier with Id=371 seems to have EntityAttributeAssociations to the real common Identifier attributes; these associations should be deleted but those attributes should not.

thelmick-unicon

The requested updates have been made and the three associated files in the repo updated.

mgwozdz-unicon

Sorry, I think I missed these on my last pass through:

StateU Entities missing informationSourceId and informationSourceOrganization:

Narrative
Texts
PositionPreferences
Relocation
RemoteWork
Travel
RemunerationPackage
Interactions
Ranges
MinimumAmount

StateU Entity missing identifier required attribute:

Criteria

Still need Entity with Id=371 marked as deleted to remove duplicate Identifier.

…ata model

thelmick-unicon

Requested updates have been made.

mgwozdz-unicon

Looks great, thank you!

…acked test (#912) ## Summary V1.2's `clone_lif_schema` function never ran successfully against real LIF data. Four issues stack up to break it on the first invocation against any fresh DB: - **#1 — `OVERRIDING SYSTEM VALUE` missing.** Every LIF `"Id"` column is `GENERATED ALWAYS AS IDENTITY` (V1.1). `INSERT ... SELECT *` raises `GeneratedAlways` and the whole call rolls back. - **#2 — FK ordering during data copy.** V1.2 created FKs before the copy and iterated tables alphabetically, so `Attributes` was loaded before `DataModels` and failed FK checks against an empty parent. Fix: reorder to **DDL → data → FKs** so `ADD CONSTRAINT FOREIGN KEY` validates all rows in one shot. - **#3 — Identity-column sequences invisible.** Sequences owned by `GENERATED ALWAYS` columns don't appear in `information_schema.sequences` (they're attached to a column, not user-created). Loop iterated zero rows; targets were left at `last_value=1`; first insert hit `UniqueViolation`. Fix: iterate `pg_sequences`. - **#4 — `setval()` target unquoted.** Mixed-case sequence names (e.g. `Entities_Id_seq`) were built via string concatenation, which PG folds to lowercase before regclass resolution → `UndefinedTable`. Fix: build with `format('%I.%I', ...)` so each identifier is double-quoted. V1.4 adds a `CREATE OR REPLACE FUNCTION` that fixes all four. Same signature, same behavior in every other respect. Idempotent per the V1.2+ convention. ## Why none of this surfaced in PR 4a / 4b review The provisioning endpoint (#903) and post-confirmation Lambda wiring (#904) had unit tests that mocked the SQL layer. PR 3 (#908) merged the V1.3 cutover migration but the demo env hasn't been redeployed yet, so V1.3's `clone_lif_schema('tenant_lif_team', TRUE)` call has never executed against real data. A local `docker compose down -v && up` after PR 4a would have hit this; persistent-volume cycles wouldn't. The Postgres-backed test in this PR is the first thing to actually exercise the function. All four bugs were discovered while writing it. ## Test coverage `test/bases/lif/mdr_restapi/test_clone_lif_schema_sql.py` (15 tests) exercises the function end-to-end against `testing.postgresql`: - DDL parity with `public` - FK count parity + every FK target lives in the tenant schema (catches FK rewrite leaks) - Data row counts match when `include_data=true` - Empty tables when `include_data=false` - Sequence advanced past max (insert into clone with default `Id` succeeds without PK collision) - Public unchanged after clone (read-only check) - Inserts in clone invisible in public - Target validation regex rejects: `public_foo`, `tenant_`, `tenant_1abc`, `tenant_Foo`, `tenant_foo-bar`, `tenant_foo bar`, oversized 67-char identifier - Second clone of same target raises SQLSTATE `42P06` (used by `tenant_service` to translate to `TenantAlreadyExistsError` → HTTP 200 idempotent retry) The fixture applies V1.2 + V1.4 in deploy order, proving the migration chain stays valid. Removes a now-stale `(PR 5)` comment from `test_tenant_endpoints.py`. ## Impact No existing tenant schemas need rebuilding — the function never ran successfully against real data, so this is a forward-only correction. ## Test plan - [x] All 15 new tests pass against PG16 (`brew install postgresql@16`) - [x] Full suite green: `uv run pytest test` → 443 passed - [x] `ruff check`, `ruff format --check`, `cspell` all pass on touched files - [x] V1.4's `CREATE OR REPLACE` is idempotent — re-running the migration is a no-op - [ ] Demo redeploy will exercise V1.3 → `clone_lif_schema` against real data for the first time ## Notes Pre-commit `ty-check` was bypassed on the commit — there are 21 pre-existing ty errors in `integration_tests/` and `components/lif/query_cache_service/core.py` on plain `main` that block the hook. Not introduced by this PR; tracked as a separate cleanup. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

thelmick-unicon added 2 commits November 26, 2025 14:53

Updated required fields

80ae07a

updated .json sample files with new required fields data

347ad8c

mgwozdz-unicon assigned thelmick-unicon Dec 1, 2025

mgwozdz-unicon requested changes Dec 1, 2025

View reviewed changes

Updated required fields based on feedback in pull-request

b75cf65

thelmick-unicon commented Dec 1, 2025

View reviewed changes

mgwozdz-unicon requested changes Dec 2, 2025

View reviewed changes

Updated StateU to include required fields from Base LIF

231bb17

thelmick-unicon commented Dec 2, 2025

View reviewed changes

mgwozdz-unicon requested changes Dec 2, 2025

View reviewed changes

thelmick-unicon added 2 commits December 2, 2025 12:15

Removed extra Identifier from StateU

26c32a3

Updated with latest requested changes for required fields in StateU d…

00d493a

…ata model

thelmick-unicon commented Dec 2, 2025

View reviewed changes

mgwozdz-unicon approved these changes Dec 2, 2025

View reviewed changes

mgwozdz-unicon merged commit b6f8b45 into main Dec 3, 2025

mgwozdz-unicon deleted the fix/update-required-fields branch December 3, 2025 13:36

This was referenced Dec 17, 2025

docker compose up --build in both development/ and deployments/ should be self-contained and not require a script #300

Closed

SPIKE: Semantic Search failing to start - possible schema change issue #658

Closed

brandonopened mentioned this pull request Apr 29, 2026

AI-Suggested Single Mappings #911

Open

bjagg mentioned this pull request May 7, 2026

Issue #883: Fix four bugs in clone_lif_schema (V1.4) + add Postgres-backed test #912

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated required fields#1

Updated required fields#1
mgwozdz-unicon merged 6 commits into
mainfrom
fix/update-required-fields

thelmick-unicon commented Nov 26, 2025

Uh oh!

mgwozdz-unicon left a comment

Uh oh!

thelmick-unicon left a comment

Uh oh!

mgwozdz-unicon left a comment

Uh oh!

thelmick-unicon left a comment •

edited

Loading

Uh oh!

mgwozdz-unicon left a comment

Uh oh!

thelmick-unicon left a comment

Uh oh!

mgwozdz-unicon left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

thelmick-unicon commented Nov 26, 2025

Uh oh!

mgwozdz-unicon left a comment

Choose a reason for hiding this comment

Uh oh!

thelmick-unicon left a comment

Choose a reason for hiding this comment

Uh oh!

mgwozdz-unicon left a comment

Choose a reason for hiding this comment

Uh oh!

thelmick-unicon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgwozdz-unicon left a comment

Choose a reason for hiding this comment

Uh oh!

thelmick-unicon left a comment

Choose a reason for hiding this comment

Uh oh!

mgwozdz-unicon left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thelmick-unicon left a comment •

edited

Loading