Repository navigation
716 archived field cc group models - #857
ufedaseyeuconsultant wants to merge 5 commits into
Conversation
|
Thanks for the pull request, @ufedaseyeuconsultant! This repository is currently maintained by 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 approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere 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: 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
@tbain Same as PR before |
tbain
left a comment
There was a problem hiding this comment.
One minor comment from me:
tests/openedx_learning/applets/cbe/test_criteria_group.py: 32 andtests/openedx_learning/applets/cbe/test_criterion.py:35 These docstrings are unnecessary, we can remove them to reduce the noise
Otherwise nothing from Codex
Details
## VerdictNo actionable bugs found in PR #857 against issue #716.
The PR correctly:
- Adds
archived = BooleanField(default=False)to both models. - Adds one migration covering live and historical tables.
- Omits
archived_at. - Preserves existing PII annotations and history tracking.
- Adds no runtime archive behavior, as required.
Verification:
- Targeted tests: 19 passed.
- Full CBE suite: 144 passed.
- Migration drift check: passed.
git diff --check: passed.- PII check is blocked by pre-existing repository-wide safelist/coverage issues unrelated to this PR.
One non-blocking question: the new help_text says the field hides rows from a criteria-tree read endpoint, although no such filtering exists yet. This appears intended as documentation for downstream work, but may be worth confirming.
…terion (openedx#716) Adds archived = models.BooleanField(default=False) to both models, in one migration, per ADR 0002 Decision 7's archive-only retirement rule. No archived_at companion field: django-simple-history (ADR 0003) already tracks both models, covering the "when" without a separate timestamp. Both models' existing `.. no_pii:` class-level docstring annotation already covers the new field, matching CompetencyRuleProfile.archived's existing non-PII precedent; no separate registry change needed. This ticket makes no behavior changes: nothing here sets, reads, or filters on the field. That is downstream tickets' scope (openedx#674, openedx#675, openedx#681). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pins the same contract CompetencyRuleProfile.archived already has a test for (test_rule_profile.py's test_creating_a_rule_profile_persists_its_columns): a freshly created row is not archived unless told otherwise. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Cherry-picked 39561a6 and 0e9513b (openedx#716: add archived field to CompetencyCriteriaGroup and CompetencyCriterion) onto upstream/main to open a direct PR there instead of against this fork's lagging main. The original migration depended on 0008_criteria_group_unique_constraints, a migration from an unrelated, unmerged branch that doesn't exist on upstream/main. Deletes it and regenerates via makemigrations against upstream's current tip (0012_studentcompetencycriteriagroupstatus, from 48a04ee and 5984e3e), landing as 0013 with the identical field operations, just renumbered and re-based. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review nit: test_group_archived_defaults_to_false and test_criterion_archived_defaults_to_false's docstrings add no information the function name doesn't already give. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rebased onto upstream/main, which merged openedx#853 since this branch was last renumbered, adding its own 0013 migration depending on the same 0012_studentcompetencycriteriagroupstatus this branch's migration also depended on. Deletes the old 0013 and regenerates via makemigrations, landing as 0014; the field operations are otherwise identical to the deleted file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
70421ca to
a684d66
Compare
|
@tbain Done |
|
Approved, thanks :) |
Description
Adds
archived = models.BooleanField(default=False)toCompetencyCriteriaGroupandCompetencyCriterion, in one migration, per ADR-0002Decision 7's archive-only retirement rule (a group or criterion referenced by learner status is
retired by archiving, never hard-deleted). No
archived_atcompanion field:django-simple-history(ADR-0003) already tracks both models, covering the "when" without a separate timestamp.
This PR makes no behavior changes. Nothing here sets, reads, or filters on
archived; that'sdownstream tickets' scope (#674, #675, #681). Both models' existing
.. no_pii:class-leveldocstring annotation already covers the new field, matching
CompetencyRuleProfile.archived'sexisting non-PII precedent, so no separate PII registry change was needed.
Impacted roles: Developer only. This is a schema-only change with no reachable behavior yet;
no Course Author, Learner, or Operator-facing effect until a downstream ticket starts using the
field.
Supporting information
lands first adds it" language in their own tickets)
its own branch/PR so the model change lands and reviews independently of [BE] GET Competency Criteria & Groups #681's endpoint logic
Testing instructions
python manage.py migrate openedx_learningpython manage.py makemigrations --check --dry-run(shouldreport nothing pending for
openedx_learning)pytest tests/openedx_learning/applets/cbe/test_criteria_group.py tests/openedx_learning/applets/cbe/test_criterion.py --no-cov— includes
test_group_archived_defaults_to_falseandtest_criterion_archived_defaults_to_false,pinning the same "defaults to False" contract
CompetencyRuleProfile.archivedalready has a testfor
pytest tests/openedx_learning/applets/cbe/ --no-covtox -e pii_check(or directly:code_annotations django_find_annotations --config_file .pii_annotations.yml --report --coverage)and confirm
CompetencyCriteriaGroup/CompetencyCriterionare not listed among uncovered modelsOther information
AddFieldon four tables — the two models plus theirdjango-simple-historyhistorical counterparts); no data migration involved.🤖 Generated with Claude Code