Convert management command to address licensing metadata issues#5760
Convert management command to address licensing metadata issues#5760bjester wants to merge 1 commit intolearningequality:hotfixesfrom
Conversation
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Solid conversion of the audit-only command into one that actually fixes licensing metadata. CI passing.
- suggestion (1):
get_infocould crash if a channel isn't in the CSV lookup - nitpick (1): Type hint mismatch on
license_idparameter - praise (2): Clean
LicensingFixesLookupextraction; good transaction scoping
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
contentcuration/contentcuration/management/commands/fix_missing_import_sources.py
Show resolved
Hide resolved
contentcuration/contentcuration/management/commands/fix_missing_import_sources.py
Outdated
Show resolved
Hide resolved
| total_node_count += node_count | ||
| channel_count += 1 | ||
| # skip using an iterator here, to limit transaction duration to `handle_channel` | ||
| for channel in destination_channels: |
There was a problem hiding this comment.
praise: Good call not using .iterator() on the outer channel loop — keeps the long-lived transaction scoped to handle_channel instead of spanning the entire command.
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class LicensingFixesLookup(object): |
There was a problem hiding this comment.
praise: Nice separation of LicensingFixesLookup — the CSV-to-license resolution logic is cleanly encapsulated and independently testable.
bjester
force-pushed
the
nsync-back-back-back
branch
from
00e2ef2 to
60ded2b
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean conversion from audit-only to fix-apply command. CI passing.
Delta from prior review: Both prior findings (suggestion: None guard in get_info, nitpick: Optional[int] type hint) are RESOLVED in the current code.
- suggestion (1): potential
AttributeErroronlicense_obj— see inline - praise (2): prior praise still stands — good
LicensingFixesLookupencapsulation and deliberate transaction scoping
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
contentcuration/contentcuration/management/commands/fix_missing_import_sources.py
Show resolved
Hide resolved
| :param copyright_holder: The current copyright_holder of the node | ||
| :return: A tuple of (license_id, license_description, copyright_holder) to use on the node | ||
| """ | ||
| info = self._lookup.get(channel_id, None) |
There was a problem hiding this comment.
praise: Nice handling of the unknown-channel case — logging a warning and passing through the original values is exactly right for a data-fix command where you want visibility without crashing.
2 participants
Summary
Converts the management command that was previously used to export data for analyzing the issue of incomplete licensing metadata for nodes that have missing source nodes that were in public channels.
.iterator()to when it handles a channel with imported nodes, to reduce the duration of how long a transaction is openReferences
Reviewer guidance
…
AI usage
LicensingFixesLookup. Those tests were refactored to be more robust, then AI was told to update the management command tests for the same scenarios. Corrections were made to thoses tests until they passed.