Skip to content

Convert management command to address licensing metadata issues#5760

Open
bjester wants to merge 1 commit intolearningequality:hotfixesfrom
bjester:nsync-back-back-back
Open

Convert management command to address licensing metadata issues#5760
bjester wants to merge 1 commit intolearningequality:hotfixesfrom
bjester:nsync-back-back-back

Conversation

@bjester
Copy link
Copy Markdown
Member

@bjester bjester commented Mar 16, 2026

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.

  • Uses a CSV for applying licensing metadata on a per-channel basis, all for public channels
  • Limits usage of .iterator() to when it handles a channel with imported nodes, to reduce the duration of how long a transaction is open

References

Reviewer guidance

  • Review the tests, do they make sense?
  • You can run the command locally, it shouldn't fail!
  • We'll do a test run on hotfixes!

AI usage

  • Changes to the management command were implemented, then AI was instructed to create tests for 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.

Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid conversion of the audit-only command into one that actually fixes licensing metadata. CI passing.

  • suggestion (1): get_info could crash if a channel isn't in the CSV lookup
  • nitpick (1): Type hint mismatch on license_id parameter
  • praise (2): Clean LicensingFixesLookup extraction; 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

@bjester bjester force-pushed the nsync-back-back-back branch from 00e2ef2 to 60ded2b Compare March 17, 2026 23:11
Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 AttributeError on license_obj — see inline
  • praise (2): prior praise still stands — good LicensingFixesLookup encapsulation 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

@bjester bjester force-pushed the nsync-back-back-back branch from 60ded2b to e4a5bd4 Compare April 1, 2026 22:34
@bjester bjester marked this pull request as ready for review April 1, 2026 22:36
Copy link
Copy Markdown
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one possible issue with kind vs kind_id that I think will break.

@bjester bjester force-pushed the nsync-back-back-back branch from e4a5bd4 to 1fe2410 Compare April 13, 2026 20:27
Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior findings

All 2 prior findings remain resolved — no changes since last review. The kind/kind_id bug flagged by @rtibbles has been fixed (node.kind_id at line 246) and the test now validates the exact arguments passed to get_info.

CI passing. No UI files changed.

  • nitpick (inline): info["license_id"] key access in get_info — see comment
  • praise (inline): test arg validation catches the kind/kind_id bug cleanly

@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


call_command("fix_missing_import_sources")

lookup.get_info.assert_called_once_with(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Adding the exact-args assertion here is what exposed the node.kind vs node.kind_id bug — good example of a test validating the call contract rather than just that the call happened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants