feat: add tenant-safe enrollment vertical - #5
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Please perform an independent review of the exact current head |
7c7a846 to
b832ed6
Compare
5742e11 to
a3e2269
Compare
|
@opencode-agent Please review the current exact head |
|
@opencode-agent Please review the current exact head |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-head review request for ae4c9c8. Re-verify the exact-head diff, required Checks, tenant/security boundaries, and any valid review findings after the shared Rust component-install fix. COMMENT only; no approval or protected-merge bypass is requested.
…to HEAD # Conflicts: # .github/workflows/quality.yml # CHANGELOG.md # README.md # docs/ARCHITECTURE.md # docs/DATA_MODEL.md # docs/product-technical-gap-baseline.md
|
@OpenCode review only the exact current head b92fc86 against base 4ce89f9. Re-check enrollment tenant isolation, entitlement/membership validity, registration cardinality, rollback dependency order, and the merged PR4 workflow. Leave an independent review or approval for this exact head only. Do not update code, merge, self-approve, bypass protections, or treat queued Checks as proof. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| CONSTRAINT learning_registration_enrollment_fk | ||
| FOREIGN KEY (tenant_id, enrollment_record_id) | ||
| REFERENCES enrollment_record (tenant_id, enrollment_record_id), | ||
| CONSTRAINT learning_registration_enrollment_unique UNIQUE (tenant_id, enrollment_record_id), |
There was a problem hiding this comment.
🟡 Learners can never re-register for an enrollment they already registered once
A learning registration is limited to one per enrollment by a uniqueness rule (learning_registration_enrollment_unique at migrations/0001_learning_kernel.sql:199), which contradicts the platform's own documented model that allows an enrollment to have many registrations, so a learner whose registration is closed can never register again.
Impact: Any attempt to register a second time against the same enrollment (for example after a prior registration is closed) is permanently rejected, blocking the documented retry/re-registration journey.
Constraint vs documented cardinality mismatch
migrations/0001_learning_kernel.sql:199 declares CONSTRAINT learning_registration_enrollment_unique UNIQUE (tenant_id, enrollment_record_id), forcing a 1:1 relationship between enrollment_record and learning_registration. However docs/DATA_MODEL.md:49 states the cardinality baseline as enrollment_record 1 ---- * learning_registration (one-to-many). The API handler create_registration (crates/lms_kernel/src/bin/lms_api.rs:423-468) relies on this uniqueness so that a second registration attempt raises a 23505 and is mapped to 409 (crates/lms_kernel/src/bin/lms_api.rs:43-52). Given the registration_status lifecycle includes closed (migrations/0001_learning_kernel.sql:190-191) and the acceptance slice requires retry/out-of-order behavior to be tested (docs/product-technical-gap-baseline.md), the 1:1 constraint is likely stricter than intended. Either the cardinality baseline should be corrected to 1:1, or the constraint should be relaxed to permit the documented one-to-many relationship.
Prompt for agents
The new learning_registration table (migrations/0001_learning_kernel.sql:184-201) adds CONSTRAINT learning_registration_enrollment_unique UNIQUE (tenant_id, enrollment_record_id), which enforces exactly one learning_registration per enrollment_record. This contradicts the documented cardinality baseline in docs/DATA_MODEL.md:49 which states enrollment_record 1 ---- * learning_registration (one-to-many), and conflicts with the acceptance requirement that retry / re-registration journeys be exercised. Decide the intended cardinality: if one registration per enrollment is truly intended, update docs/DATA_MODEL.md line 49 to reflect a 1:1 relationship; if many registrations per enrollment are intended, remove or loosen the learning_registration_enrollment_unique constraint (and reconsider the create_registration handler in crates/lms_kernel/src/bin/lms_api.rs which currently depends on the uniqueness to return 409 on duplicates).
Was this helpful? React with 👍 or 👎 to provide feedback.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| --header 'content-type: application/json' --data "$payload" \ | ||
| "http://127.0.0.1:8080/v1/tenants/$tenant_id/learners")" = '409' | ||
| learner_id=$(python3 -c 'import json, sys; print(json.load(open(sys.argv[1]))["learner_id"])' "$RUNNER_TEMP/lms_api_response.json") | ||
| offering_id=$(curl --fail --silent --show-error --request POST \ |
| --data '{"offering_name":"Partner Academy Basics","content_release_reference":"content-release/v1/partner-basics"}' \ | ||
| "http://127.0.0.1:8080/v1/tenants/$tenant_id/offerings" \ | ||
| | python3 -c 'import json, sys; print(json.load(sys.stdin)["course_offering_id"])') | ||
| entitlement_id=$(curl --fail --silent --show-error --request POST \ |
| --data '{"source_authority":"billing-control-plane","external_entitlement_reference":"entitlement-ci-1","source_digest":"sha256:ci","source_version":"v1"}' \ | ||
| "http://127.0.0.1:8080/v1/tenants/$tenant_id/learners/$learner_id/entitlements" \ | ||
| | python3 -c 'import json, sys; print(json.load(sys.stdin)["access_entitlement_id"])') | ||
| enrollment_id=$(curl --fail --silent --show-error --request POST \ |
| --data "{\"course_offering_id\":\"$offering_id\",\"access_entitlement_id\":\"$entitlement_id\"}" \ | ||
| "http://127.0.0.1:8080/v1/tenants/$tenant_id/learners/$learner_id/enrollments" \ | ||
| | python3 -c 'import json, sys; print(json.load(sys.stdin)["enrollment_record_id"])') | ||
| curl --fail --silent --show-error --request POST \ |
2 participants
Summary
Evidence
b92fc86438b881c2d83cb2d98f8d21337811623f4ce89f96310d41eaa3b12cf4afa12c36a72462ddactionlint .github/workflows/quality.ymlcargo +1.97.1 fmt --all -- --checkcargo +1.97.1 test --workspace --all-targets --lockedcargo +1.97.1 clippy --workspace --all-targets --locked -- -D warningsRUSTDOCFLAGS='-D warnings' cargo +1.97.1 doc --workspace --no-deps --lockedNOSUPERUSER NOBYPASSRLSrole: 12 tenant policies, learner201, duplicate learner409, offering/entitlement/enrollment/registration201, rollback tables0, and re-applied policies12git diff --checkScope boundary
This is the registration/enrollment slice of issue #3. Progress projection, assessment, completion persistence, credential issuance, released external contracts, browser E2E, and production readiness remain follow-up work.
Review request
Please review the exact current head
b92fc86438b881c2d83cb2d98f8d21337811623fagainst4ce89f96310d41eaa3b12cf4afa12c36a72462dd. Do not treat local or green CI checks as semantic approval.