[ufedaseyeuconsultant]

Description

Closes #717.

POST /api/cbe/v1/competencies/<tag_id>/criteria/ creates a CompetencyCriterion for a piece of course content. It either uses a leaf group the client supplies as group_id (the supplied path), or derives the root, course-level, and leaf groups for the competency and the content's course (the derive path). Until now that flow ignored the archived flag on CompetencyCriteriaGroup and CompetencyCriterion. ADR 0002 says archived groups and criteria are hidden from authoring and new associations. With this PR, creating a criterion behaves as follows:

• A supplied group_id that names an archived leaf is rejected with a 400 keyed group_id: "group_id references an archived CompetencyCriteriaGroup, which no longer accepts new associations."
• On the derive path, an archived course-level group for the content's course is rejected with a 400 keyed object_id: "The course-level CompetencyCriteriaGroup for this competency and course is archived and no longer accepts new associations." No replacement course-level group is created, because the unique constraint oel_cbe_criteria_group_one_course_group_per_tag_course allows only one per competency and course.
• On the derive path, an archived root group is reactivated (archived=False) and reused. A root is archived once every course-level group under it is archived, so without this, archiving a competency's last course would block new criteria for every course. The root's archived children stay archived. If a later check in the same request rejects it, the transaction rolls the reactivation back.
• An archived criterion keeps blocking a new criterion for the same content: in its group when group_id is supplied, and anywhere under the competency on the derive path. When every blocking criterion is archived, the 400 says the content was previously associated and the association archived, so the author is not left staring at a duplicate error for content they cannot see. A different active group still accepts the content, as a deliberate second association. Decided by the ticket owner in [BE] Enforce archived in Competency Criteria creation #717 (comment): archiving is permanent below the root for MVP, and one criterion row per content per group keeps the data clean for a post-MVP unarchive.

How it works

All changes are in src/openedx_learning/applets/cbe/api.py:

• _create_leaf_group() (the derive path, which the ticket calls resolve_or_create_leaf_group()) reactivates an archived root right after its get_or_create(), and rejects an archived course-level group right after its get_or_create(). The leaf is always newly created there, so it needs no check. The reactivation uses save(), so simple_history records it.
• resolve_supplied_leaf_group() adds the archived checks after its existing leaf, tag, and course checks, so a structurally wrong group_id keeps its existing error. It rejects an archived leaf and also a leaf whose course-level parent is archived: the parent is already loaded, so the check costs no query, and it defends against rows written outside the API, mirroring get_courses_with_relative_criteria().
• _reject_duplicate_criterion() counts every criterion on the content's ObjectTag, archived or not, and picks the 400 message by whether an active one is among the blockers.
• All three run inside the existing transaction.atomic() block in associate_competency_criterion(), under the existing row lock on the competency Tag.

No signature, migration, serializer, view, or import-layer change. associate_competency_criterion() and resolve_supplied_leaf_group() are marked UNSTABLE, so the new rejections need no deprecation.

Three details differ from the ticket's Technical Details:

• The rejections raise Django's ValidationError, as associate_competency_criterion()'s other rejections do (the module also raises CompetencyCriterionArchivedError and PermissionDenied elsewhere), not DRF's. The ticket expected a 500 from Django's exception, but CompetencyCriterionCreateView does not use the tagging exception handler the ticket cites. It translates Django's ValidationError into a DRF 400 itself (rest_api/v1/views.py), so the client gets the same 400 with the field key, and api.py stays free of HTTP-layer types.
• The two checks are inline instead of a shared _reject_if_archived() helper, because they use different keys and messages and there are only two callers.
• The derive-path rejection is keyed object_id, because the request has no group_id and the content's usage key is what selects the archived course-level group.
• An archived group is a 400 here while the group PATCH endpoint returns a 409 for the same state. The difference is deliberate: this ticket's scenarios specify a 400 validation error on create, and the 409 is the update endpoint's conflict convention from [BE] Update/Edit Competency Criteria Group #760.

Who is affected

• Course Authors creating competency criteria: a request that targets an archived leaf, or the archived course-level branch of a course, now gets a 400 instead of silently adding to archived rules. Re-adding content whose earlier criterion was archived is rejected with a 400 naming the archived association; the content can still be added to a different active group.
• Developers calling associate_competency_criterion(): the same behaviors, raised as Django ValidationError.

Supporting information

• [BE] Enforce archived in Competency Criteria creation #717: this ticket.
• [BE] Create CompetencyCriteria #665: the create-criterion flow this PR changes (merged).
• [BE] Add archived field to Competency Criteria & Group models #716: the archived fields this PR checks (merged).
• [BE] Delete Competency Criterion #674 and [BE] Build endpoint for removing a Competency Criteria Group #675: delete or archive a criterion or group, with the upward archive cascade this PR's behavior is designed around (in review).
• ADR 0002, docs/openedx_learning/decisions/0002-competency-criteria-model.rst, Decisions 2 and 7: archived groups and criteria are hidden from authoring and new associations.
• AI tools: Claude Code. Claude Opus 5.5 (claude-opus-5-5) planned the change and reviewed it; Claude Sonnet 5.5 sub-agents wrote the code and tests under that plan. I reviewed the result.

Testing instructions

Automated:

1. pytest tests/openedx_learning/applets/cbe -o addopts="" (594 tests at this branch's head; -o addopts="" skips the coverage flags tox.ini adds).
2. The quality checks as CI runs them: lint-imports, pylint, pycodestyle, isort --check-only, pydocstyle and mypy --show-traceback (mypy also checks the tests).

New tests (16 across test_api.py and test_views.py), one per ticket scenario plus regression guards:

• Supplied archived leaf: rejected with a group_id error (unit and view). An active leaf next to an archived one in the same course-level group still resolves.
• Archived root on the derive path: reactivated, still exactly one root, a new course-level group and leaf for the new course under it, and the other course's archived course-level group untouched (unit and view); the reactivation history row is asserted by the unit test.
• Archived course-level group on the derive path: rejected with an object_id error and nothing created, under an active root (unit and view) and under an archived root, where the root is still archived afterwards because the transaction rolled back.
• A different course under the same competency still succeeds when another course's course-level group is archived.
• Re-association: with the earlier criterion archived, re-adding the same content is rejected with the archived-association 400, on the derive path (unit and view) and on the supplied path into the same group (view); the duplicate check fires before any group state is consulted. A different active group still accepts the content (view), and a mix of active and archived blockers keeps the plain duplicate message (unit).
• An active leaf under an archived course-level group is rejected on the supplied path, and a request rejected by the hierarchy-dominance check after the root was reactivated rolls the reactivation back. With the criterion, its leaf, its course-level group, and the root all archived, re-association is rejected with the course-level object_id error.

Each new test was checked by temporarily breaking the code line it covers and confirming it fails for the stated reason.

Manual, against a devstack, with a competency tag <tag_id> and two course runs X and Y. The archived state can only be set from a Django shell today, because the archive endpoints are in #674 and #675.

1. POST {"object_id": "<usage key in X>"} to /api/cbe/v1/competencies/<tag_id>/criteria/. Expect 201.
2. In a shell, set archived=True with save() on that criterion's leaf. POST again with {"object_id": "<another usage key in X>", "group_id": <that leaf id>}. Expect 400 with the group_id archived message.
3. Archive X's course-level group and the root the same way. POST {"object_id": "<usage key in X>"}. Expect 400 with the object_id archived message, and the root still archived.
4. POST {"object_id": "<usage key in Y>"}. Expect 201, the root reactivated, and X's course-level group still archived.

Other information

• The one-course closure stands: when the last live criterion in a course is archived, the cascade archives the course-level group, and new content from that course gets the course-level 400. The ticket owner confirmed this in [BE] Enforce archived in Competency Criteria creation #717 (comment), matching the decision on [BE] Delete Competency Criterion #674 (comment), and is updating the ticket's user story and scenarios to say so.
• Supplied path: an active leaf that holds an archived criterion for the same content now accepts a second criterion for it, because the duplicate check ignores archived criteria. The ticket owner overruled re-association in [BE] Enforce archived in Competency Criteria creation #717 (comment); the tests pin the 400 for the same group and the derive path, and the 201 for a different group.
• The ticket's open question about StudentCompetencyCriteriaGroupStatus rows on a reactivated root: the roll-up on main reads and writes these rows with no archived filter, so a reactivated root's old roll-up rows carry over and are read like any other root's. ADR 0004 only ever raises statuses automatically, and the competency-level status is keyed by user and tag, so the reactivation itself changes nothing for recorded learners. Whether old rows should be recomputed against rebuilt rules belongs to the roll-up work.
• ADR 0002 already records root reactivation as the one reversible exception to permanent archiving: the amendment landed with [BE] Enforce one root Competency Criteria Group per competency in the database #872 (Decisions 2 and 7 and Rejected Alternative 7), and the reactivation comment in _create_leaf_group() cites it.
• The delete paths of [BE] Delete Competency Criterion #674 and [BE] Build endpoint for removing a Competency Criteria Group #675 now take the same competency Tag lock this flow holds, so a concurrent create and archive serialize once those PRs land; until then the reactivation and the archived checks are serialized only against other creates.
• No migrations.

🤖 Generated with Claude Code

[openedx-webhooks]

Thanks for the pull request, @ufedaseyeuconsultant!

This repository is currently maintained by @axim-engineering.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

---

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[mphilbrick211]

Hi @ufedaseyeuconsultant! Just flagging that some branch conflicts have popped up. Would you mind taking a look? Thanks!

[jesperhodge]

review-pr-899.md

Hi @ufedaseyeuconsultant , this is the AI-generated Code Review (raw - I haven't edited / triaged it yet).
I don't see any meaningful finding under must-haves, so there is no action needed on your side right now. I will proceed with a human code review.

[jesperhodge]

Hi @ufedaseyeuconsultant , thanks for the PR. Looks good!

I've reviewed the PR. Everything tested well.

The AI code review found some things that look worth addressing. Can you go through review-pr-899.md and address its findings?

Also the AI code review did flag a point that I think is worth a look:

"Ticket #717's user story and scenario 7 promise that an old archived association never blocks a new one, but the ticket's description, its out-of-scope list, and ADR 0002 keep a course with an archived course-level group closed, and the code follows the closed-course side."

The ticket itself contradicts itself here, so this is probably best settled in the ticket discussion, and the answer may affect the PR.

One more thing: the PR description says you left a comment on #717 about this, but I couldn't find one. Could you double-check the description and make sure everything is correct? Sometimes AI tools can slip details like this in.

Another thing in the PR description is the point:
"Supplied path: an active leaf that holds an archived criterion for the same content now accepts a second criterion for it, because the duplicate check ignores archived criteria. The ticket describes re-association only on the derive path. I've asked on #717 whether this is intended. No test pins it yet."
I think there's no question for that in 717 either, could you check that as well?

Thank you!

[ufedaseyeuconsultant]

@jesperhodge Addressed everything from the review file: rebased onto current main (the conflict was the one docstring; it now also says what the reactivation relies on for serialization), the stale duplicate-check comment is fixed, the supplied path now also rejects a leaf under an archived course-level group, the get_or_create rationale is back with the ADR 0002 / #872 citation, and the supplied-path re-association got its view test plus a rollback test through main's dominance check. The two ticket questions you couldn't find are now actually posted on #717 - sorry about that, the description claimed them before they existed. The description is rewritten: stale counts and claims fixed, the planned ADR amendment dropped (it already landed via #872), and the 400-on-create vs 409-on-update difference recorded as deliberate. On the contradiction: Mary already accepted the closed-course consequence for MVP on #674, so I proposed on #717 to narrow the two user-story bullets and scenario 7 accordingly.

[mgwozdz-unicon]

@ufedaseyeuconsultant Please see my comment response on the ticket since I am requesting that an archived criterion blocks the creation of a new criterion for the same content in the same group instead of allowing it. #717 (comment)

[ufedaseyeuconsultant]

@mgwozdz-unicon @jesperhodge Implemented Mary's decision from the ticket: the archived=False filter is gone from the duplicate check, so an archived criterion blocks a new criterion for the same content again - in its group when group_id is supplied, and anywhere under the competency on the derive path. When every blocker is archived, the 400 now says the association was archived instead of the plain duplicate message, so an author who can't see the content doesn't read it as a bug. Tests flipped accordingly: same-group re-add is a 400, the derive-path re-add is a 400, a different active group still gets a 201, and a mixed active-plus-archived case keeps the plain duplicate message. This supersedes the re-association part of my previous comment - the view test that pinned the 201 there now pins the 400. The PR description is updated to match, and the suite is green

[jesperhodge]

approved it and moved it to @mgwozdz-unicon since you added a comment there.