Repository navigation
Conversation
openedx-webhooks
added
the
open-source-contribution
|
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. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. 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: 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
changed the title
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjwKFzQ6Tfh5HJ5xzgmGKr
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjwKFzQ6Tfh5HJ5xzgmGKr
… tag import Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjwKFzQ6Tfh5HJ5xzgmGKr
…axonomies are skipped Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjwKFzQ6Tfh5HJ5xzgmGKr
…lock cases Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjwKFzQ6Tfh5HJ5xzgmGKr
… tag import contract Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjwKFzQ6Tfh5HJ5xzgmGKr
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjwKFzQ6Tfh5HJ5xzgmGKr
a40f74c to
0b53bc5
Compare
|
I used our backend-code-review skill to review this before sending it on to independent code review, here were the results: Original review with some must fix/improvements/small things: Details# Review of openedx-platform PR 39360: hide archived tagging records in Studio's content_tagging appReview DetailsPrerequisites (skip this section if you know the tagging models and this ticket)
Things the review looked at and is leaving to other tickets
What passedThe head commit merges into the current Action list
Must fix before mergeThe pull request calls openedx-core code that neither the pinned release 1.21.0 nor release 1.21.1 contains, so it cannot pass CI, and it is not marked as blocked. The new code calls A docstring says a library function clears every tag association, which is false for deletion-locked associations: the function archives them and leaves them in the table. The new docstring of The acceptance criterion about copying library content into a course has no test that fails if the behavior breaks. Criterion 9 says that when a library unit with an archived association, and a child component with one, is copied and pasted into a course, neither pasted block carries an archived tag. The new test The acceptance criterion about tags from a disabled taxonomy surviving duplication and copy and paste is not tested for those two cases. Criterion 10 says a block's tags from a disabled taxonomy are still carried when the block is exported, duplicated, or copied and pasted. Only the export has a test, The course import deletes tag associations directly through openedx-core's database models instead of its API functions, so it depends on a delete override that openedx-core does not publish as supported behavior.
Decisions to make or write downRe-importing a course from a file that names a hidden association's value makes that association visible again, and it is undecided whether that is intended. Criterion 14 says that after a course is re-imported into itself, a hidden association still exists and stays hidden. The review archived the course's Tags skipped during import because their taxonomy is archived are not restored later, and some existing placeholders can still be linked to an archived taxonomy. When the import meets a taxonomy that is archived on this instance, On MariaDB, the import's delete may lock tag and taxonomy rows that every course shares. The A block copied before one of its tag associations is archived still pastes that tag afterward. Copying a block stores its tags on the clipboard at copy time. A reviewer changed Improvements worth doingNo single test runs the export and the import on the same data. Chain export and import in one test. Criteria 6 and 11 describe a course that is exported and then imported as a new course. The tests check the export ( The test for one criterion never runs the later taxonomy import that the criterion describes. Run the later taxonomy import that criterion 12 describes. Criterion 12 says tags left out by the import cannot reappear after an administrator later imports a tag file into a taxonomy. One query returns its rows in an order the database is free to choose. Give The import writes one INFO log record for each block and archived taxonomy that lists every skipped value, and one record reached 115,028 characters for 2,000 values. Log the skipped tags once, with a count. The import writes one INFO log record for each block and archived taxonomy, containing every skipped value. The review imported 2,000 values and got a single record of 115,028 characters. The values are taxonomy values chosen by authors, not learner data. Logging once per taxonomy for the whole import, with the number of skipped tags, carries the same information. One test mixes assertions about stored data with assertions about logs and events. Separate the behavior assertions from the log and event assertions. A test helper takes the record to hide but hides a whole tag or taxonomy in two of its three cases. Make the test helper hide only the row under test. The helper One test computes its expected rows the way the production code does, so it does not check the product independently. Write the expected rows of the all-archived export test as a literal. Several tests differ only in their input and could be merged. Merge tests that differ only in their input. Small thingsA docstring says "as before", which describes history rather than the current state. The docstring of A docstring uses different wording for hidden associations than the tests and the ticket do. The same docstring says "keeping what archiving hides" and names The description gives the wrong test count for master. The description says "653 tests on master". 653 is the count on this branch; the merge-base has 634. The description's count of new tests leaves one out, and the testing commands do not run it. The description counts 13 new test functions and 19 cases, which is right for the two files it names. The diff has a fourteenth, The pull request title is not in conventional commit form. The title "Tbain/903 hide archived tagging records" is not a conventional commit. The commitlint check passes because it checks the commits. The documentation style of the new tests is inconsistent. Some new tests have a docstring that restates the test name and others have only comments. Accuracy check of this textA separate accuracy reviewer compared this text with the review's own draft and returned "pass with fixes"; the five fixes it asked for are applied, and the cleanup added 0 definitions that nobody has confirmed. The cleanup added no definitions. It added the one-sentence summary above each finding, and the accuracy reviewer checked each summary against its finding. Four summaries and one list of issue links were corrected as it asked. For the reviewer: the structured review output1. VerdictReady to merge: no. The code needs an openedx-core release that does not exist yet (CI is red on the current pin and the PR is not a draft), three acceptance criteria have a carrier or end state that no test guards, and one committed docstring states something false about 2. Inputs
3. Acceptance criteria
Behavior the change adds that no criterion asked for: an INFO log line per block and archived taxonomy with every skipped value ( 4. Premise audit20 claims checked. FALSE rows:
No UNVERIFIABLE rows. Claim 7 (value order changed with the joins) is true as observed on SQLite; the mechanism is inferred and MySQL was not run. 5. InvariantsThe diff adds no model, constraint, index, or lock of its own. The table had 8 rows for the rules the import relies on. Gap rows:
Backend skill items 10 and 12: no model field is added and no value is built into a bounded column. Item 11: the change adds no lock of its own; it reaches the library's 6. FindingsBugNone. Gap
Question
Suggestion
Nit
7. Tests checked
Edited tests: the old forms of 8. Checks run and skipped
9. Declined to judge
10. Dropped
11. Open questions
re-review after being addressed: Details# Re-review of openedx-platform PR 39360 after the review fixesVerdict: 20 of the 23 findings are fixed, 3 are partly addressed, nothing broke, and criteria 1 to 14 are met and tested against the openedx-core branch. This re-review covers #39360 at head commit The branch was rebased onto the current Was every review finding fixed?I verified the fixes that concern tests by breaking the production code and watching the tests fail. Partly addressed
Fixed
Are the new changes architecturally sound?Yes. Three changes carry design weight, and each is acceptable.
The two direct model reads that remain ( One thing I noticed in the new tests is outside this ticket: openedx-core's Did the changes break anything that worked before?No. With the openedx-core branch mounted (pull request 5's head merged with openedx-core I re-ran five one-line breaks of the production code, and each made the matching tests fail:
I ran everything in a throwaway copy of the head commit inside the CMS container and removed it afterward. The tests ran on SQLite only. CI on the pull request is still red for the same reason as before: the pinned openedx-core release lacks the archive code. That is the declared blocker, not a regression. Are all acceptance criteria met?Against the openedx-core branch, criteria 1 to 14 are implemented and each now has a test that fails when the behavior breaks. That includes the three that were only partly guarded before: criterion 9 (library unit pasted into a course), criterion 10 (disabled taxonomy on duplicate and on copy and paste), and criterion 12 (the later taxonomy import). Criterion 15 (saving a block's tags in Studio keeps hidden associations) is delivered by openedx-core pull request 5, and this pull request adds no test for it. I did not review that pull request's tests. None of the criteria is met on the openedx-core release that openedx-platform pins today, so the pull request cannot merge until a release contains the openedx-core stack and the pin is bumped. What is left to do
|
|
The various static checks for this PR are currently breaking because they are looking at the now current master for openedx-core rather than a version that includes the changes from tbain/openedx-core#5 and its associated stack. The static checks for this PR won't pass until that stack is merged in and this PR can be changed to reference that new version number. |
Description
Studio's
content_taggingapp now respects the archived state that openedx-core gives taxonomies, tags, and tag associations. Three places change: the taxonomy list'stags_countleaves archived tags out;get_all_object_tags, the one read behind the tag export, block duplication, clipboard copy, and library containers, returns only associations that archiving does not hide; and course tag import keeps the associations an author cannot see, and skips tags from a taxonomy that is archived on this instance instead of creating placeholders for them.Why
openedx-core hides archived records on its read paths (openedx/openedx-core#778), and keeps an archived association when
tag_object()replaces an object's tags (tbain/openedx-core#5). Studio'scontent_taggingapp counts tags, reads associations, and imports course tags with its own queries, so without this change an author would still see archived tags in Studio's taxonomy counts and would carry archived associations along when exporting, duplicating, copying, or importing content. openedx/openedx-core#903 describes the gap and the acceptance criteria.What changed
TaxonomyOrgView.get_queryset(rest_api/v1/views.py) annotatestags_count=Count("tag", filter=Q(tag__archived=False), distinct=True), the same expression openedx-core'sTaxonomyViewuses. One annotation covers the no-filter, org, and unassigned branches.get_all_object_tags(api.py) starts its query fromObjectTag.objects.unarchived(). An association is left out when it is archived, or when its tag or taxonomy is archived. Because the CSV header is built from the returned rows, an archived taxonomy drops out of the header too. Associations under a disabled taxonomy are not hidden by archiving and still travel as before. Nothing changes at the call sites ingenerate_csv_rows,copy_object_tags,openedx/core/lib/xblock_serializer/block_serializer.py, orcontent_libraries/api/serializers.py.set_exported_object_tags(api.py):delete_object_tags(object_id, keep_hidden=True), a keyword argument that feat: keep hidden tag associations when tag_object replaces an object's tags tbain/openedx-core#5 adds, so a hidden association (one that is archived, or whose tag or taxonomy is archived) survives a re-import into the same course. Placeholders, associations whose tag was deleted, and associations under a disabled taxonomy are still cleared and re-created from the file. A deletion-locked association is archived instead of deleted, as with everydelete_object_tagscall.Taxonomy.objects.filter(export_id=...).first(), becauseget_taxonomy_by_export_idreturnsNonefor an archived taxonomy and a missing one alike. A live taxonomy is tagged as before. An archived taxonomy is skipped, with no placeholder and no events; a placeholder would be wrong becauseObjectTag.resync()would re-link it to the archived taxonomy. A missing taxonomy still gets placeholders.import_course_tags_from_csvadds those up and writes one INFO log record per archived taxonomy for the whole import, with the count.get_all_object_tagsorders byobject_idand thenid, so the order of values within a taxonomy, and so within atags.csvcell, is stable.content_tagging/tests/test_api.py,rest_api/v1/tests/test_views.py,content_libraries/tests/test_api.py, andcms/djangoapps/contentstore/views/tests/test_clipboard_paste.py, and two extended cms tests (test_duplicate_tags,test_copy_and_paste_unit_with_tags). One pre-existing test,test_get_course_object_tags_with_add_tags, now compares one block's value lists order-insensitively: it builds its expected lists fromget_object_tags(), which sorts differently from theidorderget_all_object_tagsreturns, and the test is about no tag being lost.Who is affected
get_all_object_tagsno longer returns hidden associations.set_exported_object_tagsno longer clears them, and it returns a dict where it returnedNone.delete_object_tags(object_id)still removes hidden associations too; like every delete, it archives a deletion-locked association instead of deleting it.Supporting information
requirements/edx/base.txtpinsopenedx-core==1.21.0. Neither 1.21.0 nor 1.21.1 hasTag.archived,ObjectTag.objects.unarchived(), the archive-awaredelete(), ordelete_object_tags(keep_hidden=...), so CI fails on this PR as it stands. Theopenedx-corepin bump to a release containing the stack above must merge before this change, or be added to it. This PR was developed and tested with the openedx-core branch mounted editable in tutor dev.Testing instructions
Automated, in a tutor dev CMS container with openedx-core mounted from the
tbain/903_keep_hidden_object_tags_on_savebranch (tbain/openedx-core#5) merged with openedx-coremain. The merge is needed because current openedx-platformmasterimportsopenedx_learning.api.roll_up_competency_statuses, which is newer than that branch's base. The tests run on SQLite.The new and changed tests, with the files they live in:
All 670 pass on this branch (run on 2026-11-10, rebased on that day's
master).pylinton the touched files,ruff check openedx/core/djangoapps/content_tagging/, andmypy openedx/core/djangoapps/content_tagging/, withDJANGO_SETTINGS_MODULE=cms.envs.testfor pylint and mypy.Manual, in Studio with the same openedx-core code. These four steps were also run on 2026-11-11 against the tutor dev MySQL database, through the Studio REST endpoints and the tag import function, with the results the steps describe. Nothing in the product archives a row yet, so archive one in a Django shell (
./manage.py cms shell):from openedx_tagging.models import Tag; Tag.objects.filter(taxonomy__name="<your taxonomy>", value="<a tag>").update(archived=True). Then:tags.csvhas no cell with that value.ObjectTag.objects.filter(...)in the shell shows it).Deadline
None.
Other information
test_paste_from_library_unit_leaves_archived_tags_out. On that path the pasted blocks get their tags from openedx-core'scopy_tagsthrough the upstream link, so the end state is delivered by openedx-core's read filter ([BE] Exclude archived tagging records from read paths and block edits to them openedx-core#778);test_copy_container_leaves_archived_tags_outseparately pins what the library serializer and the clipboard carry.tag_object()un-archives a row whose tag is re-applied ([BE] Branch tag association deletes between archiving and hard delete openedx-core#779). A file exported from the same instance after the archive never lists a hidden value, so this needs an older export or a hand-edited file. No test in this PR pins it.test_list_taxonomy_query_countpasses in the full view test file (498 tests pass) but 3 of its 7 cases fail when it is run alone with-k, each counting more queries than expected (17 against 10, 24 against 23, 26 against 25). It depends on earlier tests in the file warming caches. This PR does not edit that test, and its change to the view adds a filter to an existing count, not a query. Checked on 2026-11-11.TaxonomyOrgView.get_querysetinviews.py, thenget_all_object_tagsandset_exported_object_tagsinapi.py, thenTestExportImportTagsintests/test_api.py.