Repository navigation
Conversation
|
Thanks for the pull request, @tbain! 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. |
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Claude and I collaborated on this code review. Claude wrote this comment:
Thanks for this, the model and migration changes look good. The ObjectTag.copy() fix is a good catch, and I confirmed that test_cast_copies_archived_and_deletion_locked fails when those two copy() lines are reverted, so that test is doing real work. I also ran the tagging model and migration tests, makemigrations --check, pylint, pycodestyle, isort, mypy, and lint-imports against the PR head, and everything passes.
My requests below are all about the tests. Most of the new tests check things that the schema already guarantees, so they can't fail.
1. Make the test_migrations.py module docstring clearer (line 2). "Run against existing data" is vague. What the tests actually do is create rows on the schema from before a migration, run the real migration, and then check those rows. Something like "Tests that run openedx_tagging migrations over rows created on the earlier schema." would say that directly.
2. In test_archived_and_deletion_locked_default_to_false_for_existing_rows, remove lines 140-147, and with them the line 121 comment and the explicit _export_id/_value arguments on line 123. Lines 140-147 check that the migration left every other field unchanged. Migration 0023 only adds four columns with a default, and adding a column can't change the values in other columns, so these assertions have no way to fail. Lines 146-147 in particular only check the test's own setup. I tried removing the _export_id/_value arguments from line 123: the only failure was assert '' == 'flags_test' on line 146, which comes from the test's setup, not from the migration. With lines 140-147, the comment, and those two arguments all removed, the test still passes. It still checks what matters, lines 135-138: pre-existing rows come out unarchived and unlocked.
3. Remove test_archived_defaults_to_false (test_models.py, line 158) and test_archived_and_deletion_locked_default_to_false (test_models.py, line 611). Both tests check that a newly created row has the default=False declared on the field, so they test the schema itself rather than any behavior. The defaults for pre-existing rows, which are the case #776 actually asks about, are already covered by the migration test.
4. Remove test_setting_one_flag_leaves_the_other_and_identity_unchanged (test_models.py, line 623). archived and deletion_locked are separate columns, so saving one can't change the other, and neither can change object_id, taxonomy_id, tag_id, or the cached fields. I know #776's acceptance criteria list "can be set independently ... with no other field affected", but not every acceptance criterion needs a unit test, and the schema already guarantees this one.
5. Nit, not required: the docstrings on the new tests in test_models.py could be dropped. Pylint doesn't require docstrings on tests here, and each one mostly restates the test name. Keeping them is fine too, since the file has both styles today. This applies to whichever of those tests remain after items 3 and 4, which would be only test_cast_copies_archived_and_deletion_locked.
|
Updated to address all the code review comments, handled with Claude 5.5 Opus. I also updated Step 3 of the testing instructions since we changed/removed some of the tests. |
Description
Adds
archived = models.BooleanField(default=False)toTaxonomy,Tag, andObjectTag, anddeletion_locked = models.BooleanField(default=False)toObjectTagonly, in one schema-onlymigration (
oel_tagging0023_archived_and_deletion_locked).Today these three models can only be hard-deleted, which would destroy the learner mastery history
that points at them.
archivedrecords that a row has been retired: hidden from active use, butkept.
deletion_lockedrecords that something outside the tagging app depends on an association,so a future delete must archive it instead of removing it.
TagandTaxonomyhave no lock fieldof their own; whether one is locked is derived from the associations beneath it, which #777 adds.
This PR makes no behavior changes. Nothing sets, reads, or filters on either field. #777 adds the
lock API, #778 makes archived records inert on read and write paths, and #779 through #781 add the
delete branches that use both fields.
Impacted roles: Developer only. This is a schema-only change with no reachable behavior yet.
Choices that differ from the ticket's suggestions
Tag.archived,ObjectTag.archived, andObjectTag.deletion_locked, to be confirmed against the queries in [BE] Exclude archived tagging records from read paths and block edits to them #778 and [BE] Branch tag and taxonomy deletes between archiving and hard delete #780. Those queriesdon't benefit from them. [BE] Exclude archived tagging records from read paths and block edits to them #778 filters reads on
archived=False, which nearly every row matches,so the database would not use an index on that column; the existing
(taxonomy, object_id)and(taxonomy, _value)indexes drive those reads. [BE] Add deletion-lock functions to openedx_tagging #777's lock check filtersObjectTagbytag_id IN (...), which the existing foreign key index ontagalready narrows to a few rows.This also matches [BE] Add archived field to Competency Criteria & Group models #716, which adds
archivedwithout an index.[BE] Add archived field to Competency Criteria & Group models #716's criteria-tree wording, which names an endpoint that doesn't exist in the tagging app.
Taxonomy.archivedalso says how archiving differs from the existingTaxonomy.enabledflag,since both hide a taxonomy from authors.
has one because [BE] Add an archived field to Tag, Taxonomy, and ObjectTag, and a deletion_locked field to ObjectTag #776's acceptance criteria ask for proof that the migration applies cleanly over
existing taxonomies, tags, and associations and leaves every row unarchived and unlocked.
ObjectTag.copy()carries the new fields.copy()(used byObjectTag.cast()) copiesan object tag's fields one by one, including its
id. Without the new fields, saving a castinstance would reset
archivedanddeletion_lockedtoFalseon the original row, whichwould silently clear a lock once [BE] Add deletion-lock functions to openedx_tagging #777 starts setting them. This is the only code that reads
the new fields, and it copies them; nothing filters or branches on them.
Supporting information
docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst, Decision 3archivedtoCompetencyCriteriaGroupandCompetencyCriterionTesting instructions
python manage.py migrate oel_taggingpython manage.py makemigrations --check --dry-runpytest tests/openedx_tagging/test_models.py tests/openedx_tagging/test_migrations.py --no-covtest_archived_and_deletion_locked_default_to_false_for_existing_rowsruns the real migrationover a taxonomy, tag, and association created on
0022and checks they come out unarchivedand unlocked.
test_cast_copies_archived_and_deletion_lockedchecks thatObjectTag.cast()carries bothflags over.
Taxonomy, aTag, and anObjectTag, and confirmarchivedanddeletion_lockedareFalse. Setdeletion_locked=Trueon theObjectTag,save it, reload it, and confirm
archivedis stillFalse.grep -rn "archived\|deletion_locked" src/openedx_taggingmatches only the model definitions, the migration, and
ObjectTag.copy().Other information
no_piiin.annotation_safe_list.yml, so the newfields are covered without new annotations.
make pii_checkcurrently fails onmainforreasons unrelated to this PR:
openedx_content.Draftandopenedx_content.PublishableEntityVersionare both annotated and in the safe list, and 20 models in other apps are uncovered. None of
those failures involve the tagging models.
pydocstylereports the same pre-existing errors insrc/openedx_tagging/models/base.pyonmainand on this branch; this PR adds none.AddFieldoperations, no data migration).change and ran all tests and checks locally.
Generated with Claude Code