openedx / openedx/openedx-core
[BE] Enforce archived in Competency Criteria creation
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 10
- Forks
- 32
- Avg merge
- 2d 17h
- Merged PRs (30d)
- 12
Description
Blocked by: #665 (defines resolve_or_create_leaf_group() / resolve_supplied_leaf_group(), the two functions this ticket modifies) and the archived-field ticket (no GitHub issue yet — adds archived to both CompetencyCriteriaGroup and CompetencyCriterion in one migration, so this ticket, #674, and #675 don't have to race to add it themselves).
Repo: openedx-core, single-repo. No openedx-platform changes: no new endpoint, this only adds validation inside #665's existing create-criterion flow.
Use Case
As a course author associating a gradeable subsection with a competency, via the same create-criterion flow #665 builds, I want the system to reject my request when it targets or derives into an archived branch, and to never let an already-archived criterion block me from creating a fresh one for the same content, so that "archived" consistently means retired from new authoring, in both directions.
Description
This ticket makes archived mean what ADR 0002 promises for new Competency Criteria authoring, in both directions: #665's group-resolution functions must reject creating against an archived branch, and #665's duplicate-association check must stop letting an archived criterion permanently block a fresh one for the same content.
Current state
Once #674/#675 add archived to CompetencyCriteriaGroup/CompetencyCriterion, a branch or a criterion can be permanently retired (cascade-archived, or archived directly) when a learner-status row blocks hard deletion. ADR 0002 Decision 7 names CompetencyCriteriaGroup and CompetencyCriteria directly among the models whose retirement "may be archive-only (hidden from authoring and new associations)", the same guarantee Decision 3 already gives CompetencyRuleProfile.archived. #665 predates archived and gets both sides of that guarantee wrong:
resolve_supplied_leaf_group()validates only that the suppliedgroup_idnames a leaf belonging to the right tag and course;resolve_or_create_leaf_group()'sget_or_create()calls for the root and course-level groups return an existing row regardless of itsarchivedstate. An author can explicitly target an archived leaf, or unknowingly derive into and grow an archived root or course-level branch.- The duplicate-association check rejects a
(tag_id, object_id)pairing whenever anyCompetencyCriterionreferences the existingObjectTag, active or archived. An archived criterion'sObjectTagis deliberately preserved (per #674's ADR 0002 Decision 7 protection, so its history stays resolvable), but that same preserved reference now permanently blocks ever creating a fresh criterion for that content, even though retiring the old one was never meant to have that effect.
Requested change
resolve_supplied_leaf_group()rejects with a 400 when the resolved leaf is archived.resolve_or_create_leaf_group()rejects with a 400 when the root group it would reuse is archived, and independently when the course-level group it would reuse is archived.- Neither check falls back to creating a fresh sibling: hitting an archived root or course-level group is always a rejection, never a silent substitution.
- The derive path's leaf creation needs no new check: a leaf is always freshly created there, never reused, so it's never already archived.
- The duplicate-association check's
CompetencyCriterion.objects.filter(oel_tagging_objecttag=existing_object_tag).exists()query is scoped toarchived=False: only an active criterion referencing the sameObjectTagblocks a new association; an archived one no longer does. - Every rejection this ticket adds returns a message distinguishable from #665's other 400 causes (non-leaf, competency mismatch, course mismatch, duplicate association), not just "some validation failed."
Explicitly out of scope
- Reactivating (unarchiving) a retired branch or criterion. Archiving is one-directional everywhere else in this backlog; once a competency's entire root branch is archived, there is no way back. Accepted as permanent for MVP, matching
CompetencyRuleProfile's own archive-only, no-unarchive precedent. - Any change to #665's
UniqueConstraints. This ticket assumes they stand as defined. - Rejecting a
group_idthat doesn't exist or was hard-deleted: already a 404 via #665's existingget_object_or_404, unaffected by this ticket. - Surfacing archived state on any GET/list endpoint. None exists yet for groups or criteria (a separate ticket); this only changes create-time behavior.
Acceptance Criteria
These scenarios are verifiable via Postman.
Scenario: Reject supplied-group creation targeting an archived leaf
Given a leaf CompetencyCriteriaGroup exists for a competency and course, with archived=true
And the requesting user has studio write access to the subsection's course and view access to the competency's taxonomy
When a POST request to the create-criterion endpoint supplies that leaf's group_id together with a matching object_id
Then the response returns status code 400
And the response body identifies the group as archived and no longer eligible for new associations
And no CompetencyCriterion is created
Scenario: Reject derive-path creation when the competency's root group is archived
Given a competency's root CompetencyCriteriaGroup exists with archived=true
When a POST request to the create-criterion endpoint for that competency supplies a valid object_id and no group_id
Then the response returns status code 400
And the response body identifies the competency's branch as archived
And no new CompetencyCriteriaGroup or CompetencyCriterion rows are created
Scenario: Reject derive-path creation when the course-level group for the target course is archived
Given a competency's root group is active (archived=false), but its course-level group for Course X is archived=true
When a POST request to the create-criterion endpoint for that competency supplies an object_id whose parsed course is Course X, and no group_id
Then the response returns status code 400
And the response body identifies the course-level branch as archived
And no new CompetencyCriteriaGroup or CompetencyCriterion rows are created
Scenario: Derive-path creation still succeeds for a different, non-archived course under the same competency
Given a competency's root group is active, its course-level group for Course X is archived=true, and it has no existing group for Course Y
When a POST request to the create-criterion endpoint for that competency supplies an object_id whose parsed course is Course Y, and no group_id
Then the response returns status code 201
And a new course-level CompetencyCriteriaGroup is created for Course Y, parented under the existing active root
And a new leaf CompetencyCriteriaGroup and CompetencyCriterion are created under it
Scenario: Supplied-group creation still succeeds for a non-archived leaf in a partially-archived tree
Given a competency's course-level group has two leaf groups: one archived=true, one archived=false
When a POST request to the create-criterion endpoint supplies the non-archived leaf's group_id together with a matching object_id
Then the response returns status code 201
And a CompetencyCriterion is created, associated with that non-archived leaf group
Scenario: Re-associating content after its prior criterion was archived succeeds
Given a CompetencyCriterion previously associated object_id X with tag_id T, was later archived (archived=true) rather than deleted, and its oel_tagging_objecttag row is still in place
When a POST request to competency T's endpoint associates object_id X again
Then the response returns status code 201
And a new CompetencyCriterion is created, associated with the existing oel_tagging_objecttag row
And the archived CompetencyCriterion is unaffected
Technical Details
Data Structures
No new types. This depends on CompetencyCriteriaGroup.archived and CompetencyCriterion.archived (both BooleanFields, added by the archived-field ticket) and reuses #665's resolve_or_create_leaf_group(), resolve_supplied_leaf_group(), and the duplicate-association check inside associate_competency_criterion(), all with signatures unchanged.
Logic
Add a private _reject_if_archived(group: CompetencyCriteriaGroup) -> None helper that raises rest_framework.exceptions.ValidationError when group.archived is True, identifying the group's id and that archived groups can't accept new associations. It must be DRF's ValidationError, not Django's core one: this app's exception handling (src/openedx_tagging/rest_api/v1/exception_handlers.py) only maps APIException/Http404/PermissionDenied to a clean response, so the wrong type would surface as an unhandled 500. Its message must be distinguishable from #665's other 400 causes (non-leaf, competency mismatch, course mismatch, duplicate association) — do not key it archived, since that's already the model's boolean-state field name in this backlog's success responses (#674/#675); a distinct message string, or a differently-named key, is enough.
- In
resolve_or_create_leaf_group(): call the helper immediately after the rootget_or_create(), and again immediately after the course-levelget_or_create(), before the leaf is touched. Both checks are independent: an active root doesn't imply an active course-level branch, and vice versa (a course-level branch can be archived while the root and its other course-level siblings stay active, per #675's own "cascade stops at an ancestor with a surviving sibling" behavior). Do not add a "create a fresh sibling instead" fallback: #665's own partialUniqueConstraints (one root per competency; one course-level group per competency+course) make that schema-illegal. The leaf itself is always freshly created here and never needs the check. - In
resolve_supplied_leaf_group(): add one more step to the existing ordered validation (after leaf-shape, tag-match, course-match) calling_reject_if_archived()on the resolved group. No ancestor walk needed: given #674/#675's cascade semantics (an ancestor archives only once every child is already archived), a leaf can never be non-archived while an ancestor is archived. - In
associate_competency_criterion()'s duplicate-association check: changeCompetencyCriterion.objects.filter(oel_tagging_objecttag=existing_object_tag).exists()to also filterarchived=False. An archived criterion's preservedObjectTagreference no longer counts as a blocking duplicate. - All three checks run inside
associate_competency_criterion()'s existingtransaction.atomic()block (already true, since they sit inside the functions it calls), so a rejection after a root/course-level row was freshly resolved, but not newly created, leaves nothing to roll back.
No public-API signature changes: both functions keep #665's exact contracts, this only adds a rejection path inside them. No migration, no new endpoint, no serializer or view changes, no .importlinter impact.
Test strategy: unit tests for resolve_or_create_leaf_group() (archived root rejected before course-level lookup runs; archived course-level group rejected with an active root; happy path unaffected), resolve_supplied_leaf_group() (archived leaf rejected; happy path unaffected), and the duplicate-association check (archived criterion no longer blocks a fresh one; a still-active criterion still does), plus DRF integration tests on the create-criterion endpoint for the supplied-archived-leaf 400, the derive-path archived-course-level-group 400, and the archived-criterion re-association 201.
Example Resolution Prompt
Implement the archived-branch protections described in Technical Details for
resolve_or_create_leaf_group(),resolve_supplied_leaf_group(), and the duplicate-association check, all insrc/openedx_learning/applets/cbe/api.py. Assume #665 has landed all three andassociate_competency_criterion()as scoped, and that the archived-field ticket has already addedarchived = models.BooleanField(default=False)to bothCompetencyCriteriaGroupandCompetencyCriterion.Follow Technical Details for the
_reject_if_archived()helper's behavior and exception type, where each function calls it, the no-fallback / no-ancestor-walk invariants, the duplicate-check'sarchived=Falsefilter, and the distinguishable-message requirement. Follow the Acceptance Criteria for the exact response shape and scenario coverage. Stay insideapi.py: no signature, migration, serializer, or view changes.Add unit tests to
src/openedx_learning/applets/cbe/tests/test_api.pyand DRF integration tests tosrc/openedx_learning/applets/cbe/rest_api/v1/tests/test_views.pyper the Acceptance Criteria, and confirm none of #665's existing happy-path scenarios regress.
Context
- #665: defines
resolve_or_create_leaf_group(),resolve_supplied_leaf_group(), andassociate_competency_criterion(), the functions this ticket modifies, plus the two partialUniqueConstraints (one root per competency; one course-level group per competency+course) that make "create a fresh sibling instead of erroring" schema-illegal once the existing row is archived. Also the source of this ticket's actor and permission model (oel_tagging.can_tag_object), unchanged here. - Archived-field ticket (no GitHub issue yet): adds the
archivedfield this ticket checks, ahead of #674 and #675 too. - #674 / #675: their cascade mechanics (an ancestor archives only once every child is already archived) are in Technical Details, where they justify this ticket's check placement.
- ADR 0002 Decision 7 (
docs/openedx_learning/decisions/0002-competency-criteria-model.rst): namesCompetencyCriteriaGroup/CompetencyCriteriadirectly under archive-only, hidden-from-new-associations retirement, the primary citation for this ticket. - ADR 0002 Decision 3: the same guarantee already implemented for
CompetencyRuleProfile.archived, the precedent this ticket extends, including its own no-unarchive-path precedent. src/openedx_tagging/rest_api/v1/exception_handlers.py: confirms onlyAPIException/Http404/PermissionDeniedbecome clean error responses here, why the new check must raiserest_framework.exceptions.ValidationError, not Django's core one.src/openedx_tagging/rest_api/v1/serializers.py'svalidate_tag_value: this repo's existing state/uniqueness-rejection precedent, a 400 (no 409 usage anywhere in this codebase).
Files to create and modify
Modified files
| File | Nature of modification |
|---|---|
src/openedx_learning/applets/cbe/api.py |
Add the _reject_if_archived() helper, call it from resolve_or_create_leaf_group() and resolve_supplied_leaf_group(), and scope the duplicate-association check to archived=False, per Technical Details. |
src/openedx_learning/applets/cbe/tests/test_api.py |
Unit tests: archived root rejected, archived course-level group rejected (active root), archived supplied leaf rejected, archived criterion no longer blocks a fresh duplicate, happy paths unaffected. |
src/openedx_learning/applets/cbe/rest_api/v1/tests/test_views.py |
DRF integration tests: POST to the create-criterion endpoint returns 400 for a supplied archived leaf and for a derive-path archived course-level group, and 201 when re-associating content whose prior criterion was archived. |
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with resolve_or_create_leaf_group(), resolve_supplied_leaf_group(), and the duplicate-association check inside associate_competency_criterion(), using the existing create-criterion flow from #665. Review src/openedx_tagging/rest_api/v1/exception_handlers.py and the stated unit and DRF integration test areas. Done means archived branches reject creation with distinct 400 responses, while archived criteria no longer block valid re-association and active behavior remains unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- django, python
- Domain
- api, backend, databases, testing-qa
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100