[alezconsultant]

Description

Ticket #816 asks that a grade recorded against an archived competency criterion writes no new learner status for that criterion. That behavior already exists: the criteria-resolution query in record_graded_object_statuses filters on CompetencyCriterion.archived=False, and has since #862 landed for #699. This PR adds the tests that pin each acceptance-criteria scenario of #816 to that filter, so a later change to the query cannot drop the exclusion unnoticed. It changes no production code.

Changes

One file, tests/openedx_learning/applets/cbe/test_record_graded_object_statuses.py.

• test_archived_criterion_is_skipped (existing, extended) covers the first two scenarios. Both grade calls now run under freeze_time, and the test asserts that the status row recorded before archiving keeps its created and modified values after a later, higher grade for the same content. It already asserted that the archived criterion with no prior status gets no row and that the call returns 0.
• test_active_criterion_sharing_an_object_tag_is_evaluated (new) covers the third scenario. One ObjectTag is referenced by two criteria in two groups of the same competency tree: an archived one under a child group at a 0.9 threshold, created first, and an active one under the root at 0.75. A 0.78 grade writes exactly one row, demonstrated, for the active criterion, and none for the archived one. The score meets only the active criterion's 0.75 threshold, and falls short of both the default profile's 0.8 and the archived criterion's 0.9, so the test shows the active criterion is evaluated at its own threshold. The create endpoint produces this state when group_id names a second group for the same content and competency; the test builds it directly for brevity.
• test_criterion_archived_with_its_group_is_skipped (new) covers the fourth scenario. A child group and the criterion beneath it both carry archived=True, the end state that [BE] Build endpoint for removing a Competency Criteria Group #675 scenario 3 specifies for archiving a group. The endpoint that archives a group is not implemented yet, so the test builds that state directly. A grade for that criterion's object writes no row, while an active criterion under the non-archived root on a second object is evaluated as usual.

The second group in the last two tests is a child of the root fixture group rather than a second root, because the database allows one root group per competency.

Verification

• pytest tests/openedx_learning/applets/cbe/test_record_graded_object_statuses.py --no-cov: 29 passed on SQLite.
• Removing the archived=False condition from the query in api.py makes each of the three tests fail on its changed assertion (the return value) with an observed value of 2; restoring it makes them pass.
• pylint, pycodestyle, isort --check-only, mypy: clean on the changed file.
• No manual QA path: the change is test-only, so verification is the automated runs above.

AI usage: implemented with Claude Code (Claude Fable 5.1 for the first commit, Claude Fable 5.2 for the review fixes), then reviewed and tested by the author.

Related to #816

🤖 Generated with Claude Code

[openedx-webhooks]

Thanks for the pull request, @alezconsultant!

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.

[tbain]

To my humans eyes, everything looks good; T3 with Claude Fable 5.2 noted one "must fix" item that seems pretty relevant and simple to address since it's a documentation error and a few nice to have that should probably be done since we have to revisit the code slightly for the "must fix" item.

Raw T3 Review:

Review of PR 926: tests that pin grade recording for archived competency criteria

Pull request: #926. Ticket: #816.

Review Details

Prerequisites, items left to other tickets, what passed, and the action list

Prerequisites (skip this section if you know the CBE models and how this review tested the tests)

• ADR. An architecture decision record, a short repository document that records one design decision.
• Criterion. A CompetencyCriterion row is one mastery rule: it says that a piece of graded content counts toward a competency when the learner's grade meets a threshold. The model is in src/openedx_learning/applets/cbe/models/criteria.py.
• Group. A CompetencyCriteriaGroup row holds criteria and child groups, forming a tree per competency. The database allows one root group per competency. The authoring API places criteria in leaf groups that sit under a course-level group, which sits under the root.
• Tagged association, or ObjectTag. An ObjectTag row (from openedx_tagging) records that one piece of content is tagged with one competency tag. A criterion points at an ObjectTag. The ticket calls this row the "tagged association".
• The archived flag. Both the criterion and the group have a boolean archived field, added by ticket 716. Archiving is what happens instead of deletion once a learner has a recorded status.
• Status row. A StudentCompetencyCriterionStatus row stores one learner's mastery status for one criterion, with created and modified timestamps that the application sets from Python's clock.
• The function under test. record_graded_object_statuses in src/openedx_learning/applets/cbe/api.py is called by openedx-platform inside the transaction that saves a grade. It finds the criteria attached to the graded content with one query, evaluates each one against the grade, and creates or raises status rows. It returns the number of rows it created or raised, and 0 means nothing changed. That query already contains the condition archived=False on the criterion. The condition arrived with pull request 862 for ticket 699.
• What this pull request does. It changes no production code. It extends test_archived_criterion_is_skipped and adds test_active_criterion_on_a_shared_tag_is_evaluated and test_criterion_archived_with_its_group_is_skipped, all in tests/openedx_learning/applets/cbe/test_record_graded_object_statuses.py.
• Default threshold. Criteria in this test file that use the seeded default rule profile demonstrate mastery at a grade of 0.8 or higher. The shared-association test gives its active criterion a 0.75 threshold and its archived criterion a 0.9 threshold.
• Sibling tickets. Ticket 675 (open) will add the endpoint that removes a group; when a learner status exists under the group, it archives the group and everything beneath it, including each criterion. Ticket 817 (open) will stop content from showing as tagged once its last criterion is archived. Ticket 699 defined grade-time evaluation. Ticket 723 is the parent epic and has no criteria of its own.
• How the review tested the tests. For each test, a reviewer agent changed the production code in a temporary copy of the repository so that the behavior the test names was broken, ran the test, confirmed that it failed and on which assertion, and restored the code. A test that still passes against broken code does not protect the behavior.

Things the review looked at and is leaving to other tickets

• The upward rollup in src/openedx_learning/applets/cbe/rollup.py counts archived criteria as children of their group. A learner with no status on an archived criterion in a group that requires all of its children can therefore never reach Demonstrated on that group. Ticket 816's criteria cover only the status of a single criterion, and ticket 699 assigns group computation to another ticket. The review could not determine which ticket that is, so the question is open: which ticket owns excluding archived children from the rollup?
• Rejecting new criteria under an archived group. The extracts of tickets 675 and 716 say this work has no GitHub issue. It can be built without undoing anything in this pull request.

What passed

The head merges cleanly into the current main, the migration check reports no changes, and the import linter keeps all three contracts. On the merged tree the changed file gives 29 passed and the whole CBE test directory gives 580 passed, on SQLite. pylint, pycodestyle, isort --check-only, and mypy are clean. Removing the archived=False condition makes exactly the three tests of this pull request fail, each on its changed assertion with an observed value of 2, as the description says, and with that assertion disabled each test's row-level assertions fail on their own. The assertion that modified equals the first frozen time fails when the code is changed to touch modified on every status row of the learner, and the two different frozen times are what make it able to fail. Twelve of the thirteen factual statements in the description, commit message, and docstrings are true. The tests could not be run against MySQL locally because the local client cannot authenticate; the pull request's MySQL CI job is green, and the timestamps are whole-second UTC values written from Python.

Action list

1. Remove or correct the sentence "No authoring endpoint puts two criteria on one ObjectTag" in the test docstring and in the pull request description.
2. Ask the owner of ticket 816 whether the ticket relies on ticket 675 archiving every criterion under an archived group, record the answer in the ticket, and note the dependency in the group test's docstring.
3. Decide whether the pull request should say "Closes" for ticket 816.
4. Tell the owner of ticket 817 that the state in its second and fourth criteria is reachable through the create endpoint.
5. Change the grade in the shared-association test to 0.78.
6. Create the archived criterion before the active one in the shared-association test.
7. Consider an ADR 0002 amendment, in a separate change, stating that archived criteria are skipped at grade time.
8. Optionally rename the shared-association test, shorten the two docstrings, add an archived keyword to _make_criterion, and build the shared state through the authoring API.
9. Find out which ticket owns excluding archived children from the rollup.

Must fix before merge

The pull request wrongly claims that a particular data state cannot be produced through the authoring API, when the create endpoint produces it.

Details

A docstring and the pull request description say a state is unreachable when the create endpoint produces it

The docstring of test_active_criterion_on_a_shared_tag_is_evaluated says "No authoring endpoint puts two criteria on one ObjectTag, so the test builds that state directly." The pull request description repeats the statement. The statement is false. In api.py, _reject_duplicate_criterion rejects a second criterion for the same content and competency only when it is in the same group, and its comment reads "A different explicit group is a deliberate second association (ADR-0002), not a duplicate." The create view passes the client's group_id straight to associate_competency_criterion. Two existing tests exercise this path: test_associate_competency_criterion_allows_a_different_explicit_group_for_the_same_pair in test_api.py asserts that both criteria have the same object_tag_id, and a test in test_views.py does the same over REST and receives a 201 response. CompetencyCriterion has no unique constraint on object_tag. Four reviewer agents, including one from a different model family, reached this conclusion independently. It matters because the sentence tells the next maintainer that the third acceptance criterion is a corner case that cannot occur, when it is the multi-group pattern ADR 0002 describes. Delete the sentence, or replace it with one that says the create endpoint produces this state when group_id names a second leaf group and the test builds it directly for brevity. Correct the same sentence in the pull request description.

Decisions to make or write down

It is an open decision whether ticket 816 counts on ticket 675 to archive everything under an archived group, because grade recording ignores whether the group is archived.

Details

Whether ticket 816 depends on group archiving also archiving every criterion beneath the group

The query in record_graded_object_statuses reads only the criterion's own archived flag. Three reviewer agents each ran a probe with a criterion whose archived is false under a group whose archived is true: the function wrote a status row and returned 1. Nothing on main sets a group's archived flag today, so no user can reach that state yet. It becomes reachable when ticket 675 ships, because associate_competency_criterion creates a live criterion under an archived group on both of its paths (one reviewer reproduced this), and a comment in the bulk-update function in api.py on main says "Archiving a group doesn't archive its criteria". The other readers in api.py, the tree read and the bulk update, check ancestor groups. The title of ticket 816 says "and criteria groups", but its fourth criterion describes only a criterion whose own flag was set by the group's archiving, and test_criterion_archived_with_its_group_is_skipped passes whether the filter reads the criterion's flag or the group's. If the answer is that 816 relies on ticket 675 archiving the whole subtree, a rule the author cannot see in authoring would still record mastery in the period after 675 ships and before new authoring under an archived group is rejected, which is the outcome 816 exists to prevent. Ask the owner of ticket 816 to decide, record the dependency on ticket 675 in the ticket, and check whether rejecting new criteria under an archived group has a GitHub issue. In this pull request, say in the docstring of the group test that the criterion is archived too because ticket 675 archives the whole subtree.

The pull request description says "Related to" for ticket 816, which leaves it open after merge. The pull request covers every criterion that can be tested here, so whether to say "Closes" depends on the previous decision.

Details

Whether this pull request closes ticket 816

The pull request description says "Related to #816", which leaves the ticket open after merge. The pull request adds a test for every criterion this repository can test and says the behavior already shipped, so nothing of 816 remains unless the previous decision adds work. No document in the repository states a rule for closing keywords, and pull request 862 also used "Related to". Decide once the previous question is answered, and use "Closes" if the ticket is done.

A sibling ticket repeats the same false claim that a data state cannot be created.

Details

Ticket 817 repeats the false statement about unreachable states

The second and fourth criteria of ticket 817 describe two rules measuring one assignment as "a state no authoring endpoint can produce". The evidence in the must-fix item shows that the create endpoint produces that state. This is a defect in the ticket and requires no change in this pull request. Tell the owner of ticket 817.

Improvements worth doing

One test cannot tell whether a criterion is judged by its own threshold or by the default one.

Details

The shared-association test cannot tell the active criterion's own threshold from the default threshold

test_active_criterion_on_a_shared_tag_is_evaluated records a grade of 0.8. The active criterion's threshold is 0.75, the archived criterion's is 0.9, and the default profile's is 0.8. A reviewer agent changed the code so that the active criterion was evaluated at the default 0.8 threshold, and the test still passed, because 0.8 meets both 0.75 and 0.8. The test did fail when the active criterion was given the archived criterion's 0.9 rule. The third acceptance criterion says the active criterion "reports the status its own threshold implies", so the test holds a weaker property than the criterion states. With a grade of 0.78 both breaks made the status assertion fail. Change the grade to Decimal("0.78").

One test always creates its two criteria in the same order, which lets it pass against a kind of broken production code that a reversed order would catch.

Details

The shared-association test always creates the active criterion first

The reviewer agent from a different model family, and only that agent, raised this. It rewrote the production code to drop the archived condition from the query, keep only the first criterion per ObjectTag in primary-key order, and check archived inside the loop. All 29 tests in the file passed against that rewrite. A copy of the shared-association test with the archived criterion created first failed against it, returning 0 changes, because the archived criterion took the association's only slot. The reversed order passes against the real code, so this is a hole in the test and not a production failure. Create the archived criterion before the active one.

No architecture decision record states that archived criteria are skipped when a grade is recorded.

Details

No ADR says archived criteria are skipped when a grade is recorded

ADR 0002 Decision 4 item 7 says archived criteria are "hidden from authoring and new associations but remain queryable". It says nothing about evaluation. The docstring this pull request edits used to cite "ADR-0002 Decision 4: archiving removes it from the tree"; the ADR does not say that, so removing the citation was correct. The rule that grade recording skips archived criteria is now written only in ticket 816 and in the docstring of record_graded_object_statuses. Once the group question above is answered, consider a short amendment to ADR 0002 in a separate documentation change that states the rule and which flag decides it.

Small things

One test's name uses a term that names a different object than the one the test is about.

Details

A test name says "tag" where it means the tagged association

In test_active_criterion_on_a_shared_tag_is_evaluated, every criterion in the file uses the same competency Tag; what the two criteria share is one ObjectTag. test_active_criterion_sharing_an_object_tag_is_evaluated matches the test's own docstring and the ticket's "single tagged association".

One test's docstring repeats what the test body already shows.

Details

The group test's docstring restates its setup

In test_criterion_archived_with_its_group_is_skipped, the clause "while a criterion under the non-archived root is evaluated" describes what the test body already shows. One line is enough, for example "A criterion archived with its group is skipped like one archived directly", plus the note about ticket 675 from the group question above.

One test's docstring describes how the code is written instead of the behavior it checks.

Details

The shared-association test's docstring describes how the code is written

After the false sentence is removed, the clause "the filter reads CompetencyCriterion.archived, nothing on ObjectTag" remains, and it describes the implementation. One line is enough: "Archiving one of two criteria on a shared ObjectTag leaves the other evaluated at its own threshold."

A two-line pattern is repeated three times in the test file, which the review marks as optional to fix.

Details

The archive-after-create pair now appears three times

The two lines criterion.archived = True and criterion.save() appear at three places in the test file, two of them new. An archived: bool = False keyword on the _make_criterion helper would replace the two new ones. The neighboring test_archived_rule_profile_still_evaluates uses the same save-after-create pattern, so this is optional.

One test builds a data shape that the authoring API never creates.

Details

The shared-association test puts a criterion directly on the root group

The authoring API never creates that shape, since it places criteria in leaf groups under a course-level group. The file's existing criterion fixture does the same thing, so the new test follows precedent. Building the state through associate_competency_criterion with a second leaf group, as the test in test_api.py does, would use the real shape.

Accuracy check of this text

The accuracy gate returned PASS WITH FIXES, and its two wording fixes are applied; the cleanup added one definition that nobody has confirmed.

Details

• "ADR": "an architecture decision record, a short repository document that records one design decision." The definition comes from general knowledge, not from the repository.

For the reviewer: the structured review output

Details

1. Verdict

Ready to merge: with fixes. The three tests hold all four acceptance criteria of ticket 816 and fail when the archived=False filter is removed, but a docstring the PR adds, and the PR body, state that no authoring endpoint can put two criteria on one ObjectTag, and that statement is false.

2. Inputs

• Repository: openedx/openedx-core. Target: #926 (open, not draft, one commit, author alezconsultant).
• Base and merge-base: 7f52bb6330213d620fb4c7cb5b61eec789fe2669. Head: e95cc25e68545a6e4e8c9eb937befc1ef596dfde. main at review time: 7e150e7d631fe1fcbe5ea06d5a10521608b63f0c.
• Ticket: #816, updated_at 2026-10-16T21:43:39Z. Parent: #723 (epic, no criteria). Siblings read as extracts: 675, 699, 716, 817.
• Commits on main after the merge-base that touch src/openedx_learning, tests/openedx_learning, or src/openedx_tagging: none.
• Repositories checked for companion changes: openedx-platform (the caller, lms/djangoapps/grades/subsection_grade.py:52, already branches on the return value; no companion change is needed).

3. Acceptance criteria

All line numbers are in tests/openedx_learning/applets/cbe/test_record_graded_object_statuses.py at the head unless a path is given.

Criterion (quoted)	In scope	Implemented at	Tested by	Verdict
AC1 "Then the grade is recorded"	No. The grade is written by openedx-platform; this library function only runs inside that transaction.	n/a	n/a	Out of scope
AC1 "no status is recorded for that learner against that criterion"	Yes	src/openedx_learning/applets/cbe/api.py:820	test_archived_criterion_is_skipped, the .exists() assertion at line 524 fails with the filter removed	Covered
AC1 "the caller is told that nothing changed"	Yes	api.py:820, return value documented at api.py:797-798	test_archived_criterion_is_skipped, assert changed == 0 at line 519 fails with 2 == 0	Covered
AC2 "that learner's status for that criterion is unchanged, including when it was recorded"	Yes	api.py:820; created and modified are written only in statuses.py:36 and :41	test_archived_criterion_is_skipped, status_id at line 521 and modified == first_time at line 523 each fail on their own under a break	Covered
AC3 "the active criterion reports the status its own threshold implies"	Yes	api.py:816-821 (per-criterion filter) and api.py:829 (_effective_rule(criterion))	test_active_criterion_on_a_shared_tag_is_evaluated, line 549 fails when the active criterion is given the archived sibling's rule; it does not fail when the active criterion is evaluated at the default 0.8 threshold	Covered, with the weakness in Suggestion S1
AC3 "no status is recorded for that learner against the archived criterion"	Yes	api.py:820	same test, .exists() at line 550, and changed == 1 at line 548 (2 == 1)	Covered
AC4 "no status is recorded for that learner against that criterion"	Yes	api.py:820	test_criterion_archived_with_its_group_is_skipped, .exists() at line 579 and changed == 1 at line 578 (2 == 1)	Covered for the state the criterion describes (the criterion's own flag is set); see Question Q1

Behavior the change adds that no criterion asked for: none. The change is test-only.

4. Premise audit

13 claims checked. 12 TRUE (the MySQL half of claim 10 is inferred locally; the PR's CI tests (ubuntu-latest, 3.12, django52, 8) job is green at the head). 1 FALSE:

Claim	Where	Checked against	Verdict
"No authoring endpoint puts two criteria on one ObjectTag"	Docstring at test file line 533; PR body, bullet for the shared-tag test	api.py:486-487 ("A different explicit group is a deliberate second association (ADR-0002), not a duplicate"); rest_api/v1/views.py:111-114 passes group_id through; tests/openedx_learning/applets/cbe/test_api.py:489-503 asserts second.object_tag_id == first.object_tag_id; test_views.py:505-532 does it over REST and gets 201; CompetencyCriterion has no unique constraint on object_tag	FALSE

5. Invariants

The diff touches no model, constraint, index, or lock. B5's table had 7 rows covering the rules the tests' hand-built states rely on. One gap row bears on this change:

Invariant	Enforcement	Gap
A criterion under an archived group is itself archived	Nothing on main. Ticket 675 (open) plans a cascade. The create path does not check archived on the group (api.py:296-298, api.py:317-335).	A criterion with archived=False under a group with archived=True is evaluated by record_graded_object_statuses and hidden by the tree read. See Q1.

6. Findings

Gap

G1. False statement in a committed docstring and in the PR body. test_record_graded_object_statuses.py:533 says "No authoring endpoint puts two criteria on one ObjectTag, so the test builds that state directly." The create endpoint does this by design when group_id names a second leaf group (api.py:486-487, views.py:111-114, test_api.py:489-503). Raised independently by B2, B3, B5, and the B6 agent. The reader is told AC3 is an unreachable corner case when it is ADR 0002's own multi-group pattern. Fix: delete the sentence, or replace it with one that says the create endpoint produces this state when group_id names another leaf and the test builds it directly for brevity; fix the same sentence in the PR body.

Question

Q1. Does ticket 816 rely on the archive cascade for groups, and where is that written down? api.py:820 reads only the criterion's own flag. B4, B5, and the B6 agent each ran a probe: a criterion with archived=False under a group with archived=True gets a status row and changed == 1. Nothing on main sets a group's archived flag, so the state is not reachable today (reachability unconfirmed, hence a Question). It becomes reachable when ticket 675 ships, because associate_competency_criterion creates live criteria under an archived group (B5 reproduced both the derived path and the explicit group_id path), and the comment at api.py:719 on main says "Archiving a group doesn't archive its criteria". The other readers in api.py check ancestors (api.py:350-352, :719-726). The ticket title says "and criteria groups", but AC4 only covers a criterion whose own flag is set, and test_criterion_archived_with_its_group_is_skipped passes whether the filter reads the criterion's flag or the group's. Ask the ticket 816 owner to decide, and until then keep "Related to" in the PR and say in the cascade test's docstring that the criterion is archived too because ticket 675 archives the whole subtree.

Q2. Should the PR close ticket 816? The body says "Related to #816". The PR adds a test for every in-scope criterion and says the behavior already shipped, so after merge nothing remains of 816 unless Q1 adds work. No repository document states a closing-keyword rule. Decide after Q1.

Q3. Ticket 817's criteria 2 and 4 describe two rules measuring one assignment as "a state no authoring endpoint can produce". The same evidence as G1 shows the create endpoint produces it. This is a ticket defect, not a PR defect; tell the 817 owner.

Suggestion

S1. The shared-tag test does not distinguish the active criterion's own 0.75 threshold from the default 0.8 threshold. test_record_graded_object_statuses.py:545 grades 0.8. B4 broke the code so the active criterion was evaluated at the default profile's 0.8 and the test still passed; with a score of 0.78 the same break fails line 549, as does giving the active criterion the archived sibling's 0.9 rule. AC3 says "the status its own threshold implies". Change the score to Decimal("0.78").

S2. (Raised only by the B6 agent.) The shared-tag test always creates the active criterion first. The agent ran a mutant that drops the query filter, keeps only the first criterion per ObjectTag in pk order, and checks archived in the loop: all 29 tests pass, and a copy of the test with the archived criterion created first fails with changed == 0. The reversed order passes on the real code. Creating the archived criterion first costs nothing and closes that hole.

S3. No ADR says archived criteria are skipped in evaluation or which flag decides. ADR 0002 Decision 4 item 7 (docs/openedx_learning/decisions/0002-competency-criteria-model.rst:169) says archived criteria are "hidden from authoring and new associations but remain queryable". The docstring this PR edits used to cite "ADR-0002 Decision 4: archiving removes it from the tree", which the ADR does not say, so removing the citation is correct. The rule now lives only in ticket 816 and the api.py:797 docstring. Consider a short ADR 0002 amendment in a separate docs change, once Q1 is answered.

Nit

N1. test_active_criterion_on_a_shared_tag_is_evaluated (line 527): what the two criteria share is one ObjectTag, not a Tag. test_active_criterion_sharing_an_object_tag_is_evaluated matches the docstring and AC3's "single tagged association".

N2. Docstring of the cascade test (lines 556-559): the second half ("while a criterion under the non-archived root is evaluated") restates setup. One line is enough: "A criterion archived with its group is skipped like one archived directly."

N3. Docstring of the shared-tag test (lines 530-533): after G1's sentence goes, "the filter reads CompetencyCriterion.archived, nothing on ObjectTag" describes how the code is written. One line is enough: "Archiving one of two criteria on a shared ObjectTag leaves the other evaluated at its own threshold."

N4. The pair criterion.archived = True; criterion.save() now appears at three sites in the file (lines 507-509, 541-542, 563-564). An archived: bool = False keyword on _make_criterion (line 29) would replace the two new ones. The neighboring test_archived_rule_profile_still_evaluates uses save-after-create, so this is optional.

N5. The shared-tag test puts the active criterion directly on the root group, a shape the authoring API never creates (criteria live in leaf groups under a course-level group). The file's criterion fixture does the same, so this follows precedent.

7. Tests checked (B4's break-and-restore table)

Test	How the code was broken	Failed?	For the stated reason?
archived_criterion_is_skipped	(a) archived=False removed	Yes, line 519, 2 == 0	Yes
shared_tag	(a)	Yes, line 548, 2 == 1	Yes
cascade	(a)	Yes, line 578, 2 == 1	Yes
archived_criterion_is_skipped	(b) changed assert neutralized	Yes, line 521 (status raised)	Yes
shared_tag	(b)	Yes, line 550 (.exists())	Yes
cascade	(b)	Yes, line 579 (.exists())	Yes
archived_criterion_is_skipped	(b2) changed and status asserts neutralized	Yes, line 523 (modified is 2026-01-02)	Yes
archived_criterion_is_skipped	line 522 created alone under (a)	No	Never fails under the breaks tried
archived_criterion_is_skipped	line 524 alone under (a)	Yes	Yes
shared_tag	line 549 alone under (a)	No	Fails only under (c) and (d)
cascade	line 580 alone under (a)	No	Positive control
shared_tag	(c) exclude by the ObjectTag having any archived criterion	Yes, line 548, 0 == 1	Yes
shared_tag	(d) active takes the archived sibling's rule	Yes, line 549	Yes
shared_tag	(d) same, archived override 0.5	No	Not detected
shared_tag	(d) active evaluated at default 0.8	No	Not detected (S1)
shared_tag	(d) default 0.8 or cross-wire, score 0.78	Yes, line 549	Yes
archived_criterion_is_skipped	(e) update(modified=now) on all the learner's rows	Yes, line 523	Yes
archived_criterion_is_skipped	(e) with second_time == first_time	No	The distinct times are load-bearing
archived_criterion_is_skipped	(f) filter on group__archived=False instead	Yes, line 519	Yes
shared_tag	(f)	Yes, line 548	Yes
cascade	(f)	No	Cannot tell group flag from criterion flag (Q1)

The pre-PR form of test_archived_criterion_is_skipped passes against the head, as expected for a test-only change; the edit adds the modified and created assertions.

8. Checks run and skipped

• Merge of the head into main at 7e150e7: clean, one file. makemigrations --check --dry-run: no changes. lint-imports: 3 contracts kept.
• pytest on the changed file: 29 passed. pytest tests/openedx_learning/applets/cbe: 580 passed. Both on SQLite, on the merged tree.
• pylint, pycodestyle, isort --check-only on the changed file, and repo-wide mypy: clean.
• Skipped: migrate --check (not meaningful on in-memory SQLite; the PR has no migration). pydocstyle (runs on src only). Local MySQL run (the local client cannot load mysql_native_password); the PR's CI job on MySQL is green at the head, and the frozen datetimes have zero microseconds and are UTC-aware.
• B6 ran (GPT-6.1-Sol through Codex).

9. Declined to judge

• The upward rollup counts archived criteria as children (src/openedx_learning/applets/cbe/rollup.py:76-78, :234-238), so a learner with no status on an archived criterion in an AND group cannot reach Demonstrated on that group. Ticket 816's criteria cover leaf status only, and ticket 699 names another ticket as the owner of group computation. That ticket must filter archived children in rollup.py; nothing in this PR's tests constrains it.
• The archived-branch authoring guard (reject new criteria under an archived group). The 675 and 716 extracts say it has no GitHub issue. It can be built without undoing anything here, but see Q1 for the window between 675 shipping and the guard shipping.

10. Dropped

• "Drop assert row.created == first_time because no break makes it fail" (B4): AC2 says "including when it was recorded", and created is that value; the assertion is the direct statement of the criterion.
• "changed == 0 cannot attribute a failure to AC1 or AC2" (B4): the row assertions at lines 521 to 524 do attribute it, and no change was proposed.
• "tag = object_tag.tag is unneeded" (B4): the line predates the PR.
• "ADRs 0002 and 0003 have no Status line" (B1, B3, B5): pre-existing, not touched.
• "The docstring claim is a Bug" (B5): a false statement in committed text is a Gap under this review's rules; no behavior fails.

11. Open questions for the developer

1. Q1: does 816 rely on ticket 675's cascade for groups, and is there an issue for the authoring guard?
2. Q2: should this PR close 816?
3. Which ticket owns excluding archived children from the rollup?

[alezconsultant]

@tbain
Thanks. The fixes are pushed.

1. Fixed. The sentence is removed from the docstring and corrected in the description: the create endpoint produces this state when group_id names a second group.
2. Ticket [BE] Exclude archived competency criteria and criteria groups from evaluation when a grade is recorded #816 relies on [BE] Build endpoint for removing a Competency Criteria Group #675 archiving every criterion under an archived group. Creating a new criterion under an archived group is rejected by [BE] Enforce archived in Competency Criteria creation #717, implemented in feat: reject new criteria under archived competency groups (#717) #899. I did not add the docstring note, because ticket 675 is not implemented yet.
3. The description keeps "Related to". Nothing of ticket 816 remains after this merge, so I will close it by hand.
4. Confirmed. I've notified owner of 817 ticket about it.
5. Fixed. The grade is now 0.78.
6. Fixed. The archived criterion is created first.
7. feat: exclude archived criteria and groups from competency rollup #929 adds this rule to ADR 0002 Decision 6.
8. The test is renamed and both docstrings are one line. I skipped the _make_criterion keyword and the authoring API setup, because the current code follows the pattern of the rest of the file.
9. [BE] Exclude archived criteria and groups when rolling up a learner's competency status #815 owns it, implemented in feat: exclude archived criteria and groups from competency rollup #929.

[tbain]

Approved from me - T3/Claude also approves, though notes that the commit stated that the code was performed with Fable 5.2, but the description still states just 5.1. Small thing but would be good to update that for technicalities sake I suppose.

Raw T3/Claude output:

Details

# Re-review of PR 926 after the author's fixes

The pull request is ready to merge. Nothing in it needs to change. Pull request: #926. Ticket: #816. The first review looked at commit e95cc25. This re-review looks at the new head b09a15f, which adds one commit, "test: tighten the shared-association test for archived criteria".

Was every review comment fixed?

The first review ended with nine numbered actions. The author answered each one in a pull request comment on 2026-11-11. I checked each answer against the code and GitHub.

Action from the first review	Outcome	What I checked
1. Remove or correct the sentence "No authoring endpoint puts two criteria on one ObjectTag" in the docstring and the description.	Fixed.	The sentence is gone from the docstring. The description now says the create endpoint produces this state when group_id names a second group, which is true.
2. Decide whether ticket 816 relies on ticket 675 archiving every criterion under an archived group, record the answer in the ticket, and note it in the group test's docstring.	Answered, with two parts declined.	The author states that ticket 816 relies on #675, and that #717, implemented in #899, rejects creating a criterion under an archived group. Both exist and both are open. The answer is recorded in the pull request comment only: ticket 816 was last updated on 2026-10-16. The docstring note was declined because ticket 675 is not implemented yet. The pull request description already states that dependency, so I accept this.
3. Decide whether the pull request should say "Closes" for ticket 816.	Decided.	The description keeps "Related to", and the author will close ticket 816 by hand after merge.
4. Tell the owner of ticket 817 that the state in its second and fourth criteria is reachable.	Done, according to the author.	#817 was updated on 2026-11-11. I did not read what changed in it.
5. Change the grade in the shared-association test to 0.78.	Fixed.	I changed the code so every criterion is evaluated at 0.8, and the test now fails on its status assertion.
6. Create the archived criterion before the active one.	Fixed.	I rebuilt the broken version of the code from the first review, which keeps only the first criterion per ObjectTag. The test now fails with 0 changes where it expects 1. All 29 tests passed against that broken code before this fix.
7. Consider an ADR 0002 amendment stating that archived criteria are skipped at grade time.	Handled in another pull request.	#929 is open and changes docs/openedx_learning/decisions/0002-competency-criteria-model.rst. Its description says it adds a Decision 6 bullet. I did not review that pull request.
8. Optional: rename the test, shorten two docstrings, add an archived keyword to _make_criterion, build the shared state through the authoring API.	Two done, two declined.	The test is now test_active_criterion_sharing_an_object_tag_is_evaluated, and both docstrings are one line. The author declined the helper keyword and the authoring API setup because the file's other tests use the same pattern. Both were optional, and the reason is sound.
9. Find out which ticket owns excluding archived children from the rollup.	Answered.	#815 owns it, and pull request 929 implements it. Both exist and are open.

Are the changes since the last review architecturally sound?

Yes. The new commit changes 18 lines in one test file and no production code. It adds one comment, "0.78 meets only the active criterion's 0.75 threshold, not the default 0.8 or the archived 0.9", which explains the choice of number and is worth keeping.

One risk sits outside this pull request. Grade recording reads only the criterion's own archived flag. That is safe only when two other changes both hold: ticket 675 archives every criterion under an archived group, and pull request 899 rejects new criteria under an archived group. Both are open. If ticket 675 merges before pull request 899, an author can create a live criterion under an archived group, and grade recording will evaluate it. I suggest writing on ticket 675 that it must not merge before pull request 899.

Did the changes break anything that worked before?

No. On the new head merged with the current main, the changed file gives 29 passed and the whole CBE test directory gives 580 passed, on SQLite. Removing the archived=False condition still makes exactly the three tests of this pull request fail, with the same values as before. pylint, pycodestyle, and isort --check-only are clean on the changed file. I did not rerun mypy locally; the CI quality job and the MySQL test job are green on the new head.

Are all acceptance criteria met?

Yes, for everything this repository can test.

Criterion of ticket 816	Met?	Test
1. An archived criterion records no new status, and the caller is told nothing changed.	Yes.	test_archived_criterion_is_skipped
2. A status recorded before archiving is unchanged, including when it was recorded.	Yes.	test_archived_criterion_is_skipped
3. An active criterion sharing a tagged association with an archived one is evaluated at its own threshold.	Yes, and more strictly than at the first review.	test_active_criterion_sharing_an_object_tag_is_evaluated
4. A criterion archived because its group was archived is skipped.	Yes.	test_criterion_archived_with_its_group_is_skipped

The first criterion also says "the grade is recorded". The grade is saved by openedx-platform, which calls this library function, so that clause cannot be tested here.

One line in the description to update

The description says the work was implemented with Claude Fable 5.1, and the new commit is co-authored by Claude Fable 5.2. The author may want to update that line.

[alezconsultant]

@tbain ai model fixed.