Enhances metadata, notifications, and service management - #23
Merged
Conversation
- Added audio summary, torrent health, access labels, added date, and identifiers (ASIN/ISBN) to Discord, Gotify, NTFY, and Pushover notifications. - Improved the handling of metadata in the `get_notification_fields` function to include MAM enrichment details such as audio properties, torrent health, and uploader information. - Updated the web UI to display additional metadata fields including audio summary, torrent health, uploader, access labels, and added date. - Enhanced tests to cover new MAM enrichment fields and ensure proper formatting in notifications and web UI.
…adata coordinator and qbittorrent modules
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds MAM enrichment normalization and propagation, expands notification and approval-page metadata, standardizes qBittorrent add-result handling, introduces a systemd service script, extends reply-token TTL defaults, updates dependencies, and revises related tests. ChangesRuntime operations and defaults
MAM normalization and coordinator flow
Enriched notification and approval rendering
qBittorrent result classification
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Webhook
participant MetadataCoordinator
participant MAMApiAdapter
participant Audnex
participant Audible
Webhook->>MetadataCoordinator: submit MAM URL
MetadataCoordinator->>MAMApiAdapter: get_full_metadata(url)
MAMApiAdapter-->>MetadataCoordinator: return asin and mam_enrichment
MetadataCoordinator->>Audnex: request metadata using asin
MetadataCoordinator->>Audible: fallback after missing MAM or Audnex metadata
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f25a90598b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/metadata_coordinator.py (1)
74-94: 🧹 Nitpick | 🔵 TrivialMAM auth failures now silently fall back instead of surfacing.
MamApiError(including auth errors thatget_torrent_datadeliberately re-raises) is caught and swallowed here, degrading gracefully to Audible fallback with only a log entry. An expired/invalidMAM_IDwill now silently degrade metadata quality indefinitely instead of being surfaced to the operator.Recommend adding a metric/alert on the
coordinator.step1.mam_api_errorlog event (or a periodic health check) so a persistent auth misconfiguration doesn't go unnoticed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/metadata_coordinator.py` around lines 74 - 94, Update the MamApiError handling in the coordinator flow around mam_adapter.get_full_metadata so authentication-related failures are surfaced to operators instead of only being logged and swallowed. Add a metric or alert tied to the coordinator.step1.mam_api_error event, preserving graceful fallback while enabling detection of persistent MAM_ID/auth misconfiguration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mam_api/models.py`:
- Around line 118-120: Add a concise comment immediately above the BitRate and
Bitrate fields explaining that they intentionally map to distinct raw
MAM/MediaInfo key casings and are both used by bitrate_value()'s fallback chain.
Do not rename, remove, or otherwise alter either field.
In `@src/qbittorrent.py`:
- Around line 220-276: Update add_torrent_by_url to route every non-string
torrents_add result through _torrent_add_result_succeeded(result) instead of
treating it as successful, while preserving the existing conflict and error
handling. Add coverage for metadata with success_count=0, empty
added_torrent_ids, and failure_count>0, asserting the URL add returns False.
In `@src/utils.py`:
- Around line 183-216: Update `_format_sampling_rate` so its numeric `<1000`
branch appends the appropriate `" Hz"` fallback unit, matching
`_format_bitrate`’s labeled output while preserving existing kHz formatting and
nonnumeric handling; add a direct test covering a sub-1000 sampling-rate value.
In `@src/webui.py`:
- Around line 102-109: Update the template context construction around the
context dictionary so the explicit "token": token assignment occurs after the
payload, metadata, and fields spreads. Preserve the existing narrator
assignment, and add coverage for payload or metadata containing a token to
verify the rendered approval link still uses the URL-path token.
---
Outside diff comments:
In `@src/metadata_coordinator.py`:
- Around line 74-94: Update the MamApiError handling in the coordinator flow
around mam_adapter.get_full_metadata so authentication-related failures are
surfaced to operators instead of only being logged and swallowed. Add a metric
or alert tied to the coordinator.step1.mam_api_error event, preserving graceful
fallback while enabling detection of persistent MAM_ID/auth misconfiguration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3ae5c1c3-2005-46fc-8534-4cf923734412
📒 Files selected for processing (25)
.github/workflows/ci.ymlconfig/config.yaml.exampledocs/vendor/audible/config/config.yaml.examplerequirements.txtservice.shsrc/db.pysrc/mam_api/adapter.pysrc/mam_api/models.pysrc/metadata_coordinator.pysrc/notify/discord.pysrc/notify/gotify.pysrc/notify/ntfy.pysrc/notify/pushover.pysrc/qbittorrent.pysrc/utils.pysrc/webui.pytemplates/approval.htmltests/conftest.pytests/test_mam_api.pytests/test_metadata_coordinator.pytests/test_metadata_extended.pytests/test_notify_formatting.pytests/test_qbittorrent.pytests/test_utils_extra.pytests/test_webui_extended.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_metadata_coordinator.py (2)
273-287: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
MAM_ID not configuredauthentication branch.
- Issue: The new test exercises
MAM_AUTH_ERROR_MESSAGEbut not the alternate marker checked onsrc/metadata_coordinator.pyLine 88.- Impact: Missing MAM configuration could regress to non-alert logging without failing the suite.
- Fix: Add a parameterized case or test using
MamApiError("MAM_ID not configured")and assertalert=Trueplus the auth-alert log.- Test: Verify the Audible fallback still occurs.
As per path instructions,
**/tests/**requires pytest best practices, proper mocking of external services, and adequate coverage of edge cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_metadata_coordinator.py` around lines 273 - 287, Add coverage for the alternate MAM authentication marker by parameterizing or adding a test alongside test_webhook_mam_auth_error_falls_back_to_audible using MamApiError("MAM_ID not configured"). Assert the Audible fallback succeeds and verify both the exception log with alert=True and the auth-alert log containing the marker.Source: Path instructions
188-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise MAM enrichment propagation in both webhook outcomes.
- Issue: The MAM mocks return only
asinorNone; neither path asserts thatmam_enrichmentis copied to the Audnex or Audible result.- Impact: A regression in
src/metadata_coordinator.pyLines 120 or 146 could silently remove metadata used by notifications and the approval page.- Fix: Return a small
mam_enrichmentmapping and assert exact propagation in both tests.- Test: Cover both the Audnex-success and Audible-fallback paths.
As per path instructions,
**/tests/**requires pytest best practices, proper mocking of external services, and adequate coverage of edge cases.Also applies to: 206-214
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_metadata_coordinator.py` around lines 188 - 198, Update the webhook metadata tests around get_metadata_from_webhook to have the MAM mock return a small mam_enrichment mapping alongside the existing ASIN or None values, then assert exact mam_enrichment propagation in both the Audnex-success and Audible-fallback cases. Preserve the existing source, ASIN, and mock-call assertions while covering both outcomes.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/qbittorrent.py`:
- Line 522: Update the string-response branch in add_torrent_by_url to route the
complete qBittorrent response through _torrent_add_result_succeeded, or
explicitly return False for unrecognized strings such as "Unexpected.". Add a
pytest case using the existing mock to verify add_torrent_by_url returns False
for that response.
---
Outside diff comments:
In `@tests/test_metadata_coordinator.py`:
- Around line 273-287: Add coverage for the alternate MAM authentication marker
by parameterizing or adding a test alongside
test_webhook_mam_auth_error_falls_back_to_audible using MamApiError("MAM_ID not
configured"). Assert the Audible fallback succeeds and verify both the exception
log with alert=True and the auth-alert log containing the marker.
- Around line 188-198: Update the webhook metadata tests around
get_metadata_from_webhook to have the MAM mock return a small mam_enrichment
mapping alongside the existing ASIN or None values, then assert exact
mam_enrichment propagation in both the Audnex-success and Audible-fallback
cases. Preserve the existing source, ASIN, and mock-call assertions while
covering both outcomes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d7ed7bd-6349-4a04-93c8-158875f3af71
📒 Files selected for processing (9)
src/mam_api/models.pysrc/metadata_coordinator.pysrc/qbittorrent.pysrc/utils.pysrc/webui.pytests/test_metadata_coordinator.pytests/test_qbittorrent.pytests/test_utils_extra.pytests/test_webui_extended.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/test_metadata_coordinator.py (2)
297-309: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert that non-auth errors do not emit an auth alert.
- Issue: The test checks
alert=Falsebut never asserts thatcoordinator.step1.mam_auth_alertis absent.- Impact: A regression could emit an auth-specific alert for every MAM API error while this test still passes.
- Fix/Test: Add a negative assertion for the auth-alert log event.
As per path instructions, tests should cover the relevant edge-case distinction between authenticated and non-authenticated failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_metadata_coordinator.py` around lines 297 - 309, Extend test_webhook_mam_api_error_falls_back_to_audible to assert mock_log does not emit the coordinator.step1.mam_auth_alert event, while preserving the existing assertion that the generic mam_api_error event uses alert=False.Source: Path instructions
245-249: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winProve that the new MAM mock is exercised.
- Issue: These fallback/error tests configure
get_full_metadatabut do not assert it was called.- Impact: They can pass if the coordinator skips MAM or regresses to another method; injected MAM failures would never be exercised.
- Fix/Test: Add
assert_called_once_with(sample_webhook_payload["url"])to the affected MAM-path tests, and assert Audnex was called where its failure path is under test.Suggested assertion
result = await coordinator.get_metadata_from_webhook(sample_webhook_payload) +coordinator.mam_adapter.get_full_metadata.assert_called_once_with( + sample_webhook_payload["url"] +)As per path instructions, tests must verify proper mocking of external services and adequate edge-case coverage.
Also applies to: 259-260, 269-270, 314-316, 326-328, 338-340, 350-351, 359-360, 368-369, 400-401, 411-412, 422-423, 980-982
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_metadata_coordinator.py` around lines 245 - 249, Add interaction assertions to each affected MAM-path test around the configured coordinator.mam_adapter.get_full_metadata mock, verifying it was called exactly once with sample_webhook_payload["url"]. In tests covering Audnex failure, also assert coordinator.audnex.get_book_by_asin was called with the expected ASIN, while preserving the existing fallback assertions.Source: Path instructions
src/qbittorrent.py (1)
505-518: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog count-based metadata successes consistently.
- Issue: URL adds log success only when
result.hashexists. Metadata responses recognized through counts, IDs, or pending state returnTruewithout emitting the success event.- Impact: Successful URL additions can be missing from operational telemetry, unlike file and data additions.
- Fix: Compute
successonce, then emit the generic success log for every successful result; includehashonly when available.- Test: Add a metadata-success fixture without
hashand assert the success log is emitted.As per path instructions, maintain logging consistency for Python API-call paths.
Suggested adjustment
else: + success = _torrent_add_result_succeeded(result) torrent_hash = getattr(result, "hash", None) - if torrent_hash: - log.info("qbittorrent.torrent.add.success", hash=torrent_hash) - return _torrent_add_result_succeeded(result) + if success: + if torrent_hash: + log.info("qbittorrent.torrent.add.success", hash=torrent_hash) + else: + log.info("qbittorrent.torrent.add.success") + return success🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/qbittorrent.py` around lines 505 - 518, Update the metadata branch of the torrent-add handling around _torrent_add_result_succeeded to compute success once, then emit qbittorrent.torrent.add.success for every successful result, including metadata recognized by counts, IDs, or pending state. Include the hash field only when available, preserve rejection handling for string responses, and add coverage for a successful metadata result without hash.Source: Path instructions
tests/test_qbittorrent_coverage.py (1)
241-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining qBittorrent metadata branches
- Issue: this test only hits the unrecognised-object fallback.
- Impact:
add_torrent_by_url()can still drift onsuccess_count,added_torrent_ids, orpending_count/failure_countresponses without a direct guard here.- Fix: add a few parametrized cases for positive success count, non-empty IDs, pending with zero failures, and failure/empty boundaries; use realistic metadata objects instead of a generic mock where possible.
- Test: keep the current hash and fallback assertions, and add the remaining shapes in this file or the nearest qBittorrent coverage test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_qbittorrent_coverage.py` around lines 241 - 258, Expand test_unrecognised_metadata_response_returns_false into parametrized coverage for add_torrent_by_url metadata handling, retaining the existing hash and unrecognised-object fallback assertions. Add realistic metadata cases for positive success_count, non-empty added_torrent_ids, pending_count with zero failure_count, and failure/empty boundary values, asserting each expected result directly.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/qbittorrent.py`:
- Around line 505-518: Update the metadata branch of the torrent-add handling
around _torrent_add_result_succeeded to compute success once, then emit
qbittorrent.torrent.add.success for every successful result, including metadata
recognized by counts, IDs, or pending state. Include the hash field only when
available, preserve rejection handling for string responses, and add coverage
for a successful metadata result without hash.
In `@tests/test_metadata_coordinator.py`:
- Around line 297-309: Extend test_webhook_mam_api_error_falls_back_to_audible
to assert mock_log does not emit the coordinator.step1.mam_auth_alert event,
while preserving the existing assertion that the generic mam_api_error event
uses alert=False.
- Around line 245-249: Add interaction assertions to each affected MAM-path test
around the configured coordinator.mam_adapter.get_full_metadata mock, verifying
it was called exactly once with sample_webhook_payload["url"]. In tests covering
Audnex failure, also assert coordinator.audnex.get_book_by_asin was called with
the expected ASIN, while preserving the existing fallback assertions.
In `@tests/test_qbittorrent_coverage.py`:
- Around line 241-258: Expand test_unrecognised_metadata_response_returns_false
into parametrized coverage for add_torrent_by_url metadata handling, retaining
the existing hash and unrecognised-object fallback assertions. Add realistic
metadata cases for positive success_count, non-empty added_torrent_ids,
pending_count with zero failure_count, and failure/empty boundary values,
asserting each expected result directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d34aeb9-6c03-4c14-9af7-b19a5ebdb634
📒 Files selected for processing (3)
src/qbittorrent.pytests/test_metadata_coordinator.pytests/test_qbittorrent_coverage.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces significant enhancements to metadata processing, notification content, and system management capabilities.
service.shscript to streamline systemd service operations, allowing users to easily start, stop, restart, check status, view logs, and manage the service without manualsystemctlcommands.qbittorrent-apito a newer version and refines CI workflows for improved stability and maintenance.Summary by CodeRabbit
service.shcontrol script for managing audiobook-style services and viewing logs.