MPT-23347 refactor BucketDeleter scope resolution and trace scope deletes

[albertsola]

🤖 AI-generated PR — Please review carefully.

What was done

BucketDeleter mixed scope dispatch, deletion mechanics, commerce-API resolution, and mutable statement_agreements side-state read by callers after the delete returned. It is now three collaborating pieces:

• ScopeResolver (services/scope_resolver.py, new) — scope → target ids: the streaming commerce-API agreement-id lookup with its MPTError → UpstreamSubscriptionError mapping, the product/seller RQL query building, and the stored-agreement lookup for a subscription scope. Same split as the push-estimates resolvers.
• BucketDeleter — the delete actions only: delete_subscription, delete_agreement, delete_agreements_by_query, delete_all (public, was _delete_all). No Selector knowledge.
• ScopeBucketDeleter (new, same module) — converts a selector scope into delete actions: owns the delete(scope) entrypoint with the match dispatch, outcome logging, and DeleteReport rendering; uses BucketDeleter to delete data.

DeleteOutcome gains a statement_agreements frozenset returned by the delete calls; the mutable property and its cross-call |= accumulation are gone. pipeline.py's reset flow and the delete CLI command wire ScopeBucketDeleter(BucketDeleter(...), resolver).

Tracing — trace_span on the public delete actions, child spans under usage_reporting.delete_buckets (which sits on ScopeBucketDeleter.delete):

• usage_reporting.delete_subscription with the subscription id
• usage_reporting.delete_agreement with the agreement id
• usage_reporting.delete_agreements_by_query with the RQL query

The resolver's streaming methods stay untraced — a span around an async generator would close at creation, before any row is fetched.

Testing

• make check and make test pass (260 tests).
• New test_scope_resolver.py suite; test_bucket_delete.py reworked around a scope_deleter fixture, including a dispatch-routing test (each selector kind → its BucketDeleter action, None → delete_all).

Jira: MPT-23347

🤖 Generated with Claude Code

• Refactored bucket deletion into scope resolution, deletion actions, and scope dispatch components.
• Added deduplicated agreement resolution with upstream error handling.
• Returned immutable statement_agreements in DeleteOutcome.
• Updated CLI and reset flows to use ScopeBucketDeleter.
• Added deletion tracing and expanded resolver and selector-routing test coverage.
• Verified with 260 passing tests and make check.

[coderabbitai]

📝 Walkthrough

Walkthrough

The deletion flow now separates scope resolution from bucket deletion. ScopeResolver resolves selector scopes, ScopeBucketDeleter dispatches deletion operations, and DeleteOutcome carries statement agreement targets through CLI and pipeline reset flows.

Changes

Scoped deletion flow

Layer / File(s)	Summary
Scope resolution and deletion outcome backend/mpt_usage_reporting_extension/services/scope_resolver.py, backend/mpt_usage_reporting_extension/services/bucket_delete.py	Adds selector query construction, agreement ID streaming and deduplication, upstream error translation, and statement_agreements on DeleteOutcome.
Target-specific deletion and selector dispatch backend/mpt_usage_reporting_extension/services/bucket_delete.py	Refactors bucket deletion into target-specific methods and adds ScopeBucketDeleter for selector-based routing.
Pipeline and CLI integration backend/mpt_usage_reporting_extension/pipeline.py, backend/mpt_usage_reporting_extension/cli/commands/delete.py	Constructs the scoped deleter and propagates returned statement agreement targets through reset and delete operations.
Deletion and integration test coverage backend/tests/services/test_bucket_delete.py, backend/tests/services/test_scope_resolver.py, backend/tests/test_pipeline.py, backend/tests/cli/commands/test_delete.py	Updates deletion, resolver, pipeline, CLI, selector-routing, error, and dry-run tests for the new composition and outcome shape.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• softwareone-platform/mpt-usage-reporting-extension#27: Changes the CLI delete command’s selector-driven bucket deletion flow.
• softwareone-platform/mpt-usage-reporting-extension#28: Modifies deletion outcomes and pipeline reset handling for agreement-only resets.
• softwareone-platform/mpt-usage-reporting-extension#40: Adds upstream error handling connected to agreement ID resolution in bucket deletion.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Documentation Up To Date	⚠️ Warning	The PR adds ScopeResolver/ScopeBucketDeleter and changes bucket-delete boundaries, but no docs file was updated; docs/architecture.md still only describes BucketDeleter.	Update docs/architecture.md to describe the new scope/deletion split (ScopeResolver, BucketDeleter actions, ScopeBucketDeleter) and any affected CLI/reset behavior if intended.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

	Warnings
⚠️	This PR changes 644 lines across 8 files (threshold: 600). Please consider splitting it into smaller PRs for easier review.

✅ Found Jira issue key in the title: MPT-23347

Generated by 🚫 dangerJS against 622c329

[coderabbitai]

🧹 Nitpick comments (2)

backend/mpt_usage_reporting_extension/pipeline.py (1)

394-401: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Unjustified # noqa: WPS476 suppression.

WPS476 flags sequential await in a for loop (recommends asyncio.gather for independent coroutines) — here each product's delete runs one after another with no comment explaining why sequential processing is required (e.g., tracing/ordering constraints). As per path instructions, inline linter ignores must be "a narrow, justified last resort"; please add a rationale comment, or consider parallelizing the per-product deletes if they are independent.

🤖 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 `@backend/mpt_usage_reporting_extension/pipeline.py` around lines 394 - 401,
The sequential await in _reset_products is suppressed without justification.
Determine whether deleter.delete calls for each product_id are independent; if
so, parallelize them with asyncio.gather and merge all outcomes afterward,
otherwise retain the loop and add a concise rationale explaining the required
ordering or tracing constraint next to the WPS476 suppression.

Source: Path instructions

backend/tests/services/test_bucket_delete.py (1)

23-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated _StubSubscriptions/_aiter test doubles across files.

This stub (and _aiter) is duplicated verbatim in the new backend/tests/services/test_scope_resolver.py, and a subscriptions() fixture returning the same stub also appears in backend/tests/cli/commands/test_push_estimates_by_id.py per the graph evidence. Consider hoisting these into a shared fixtures module (e.g. a services-scoped conftest or fixtures package registered via pytest_plugins) so all consumers share one definition.

As per path instructions, backend tests should "share setup through fixtures (splitting a large conftest into a fixtures package registered via pytest_plugins)".

🤖 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 `@backend/tests/services/test_bucket_delete.py` around lines 23 - 51, The test
doubles _StubSubscriptions and _aiter are duplicated across backend test
modules. Move their shared definitions into the backend test fixtures package or
a services-scoped conftest registered through pytest_plugins, expose the
subscriptions fixture there, and update test_bucket_delete.py,
test_scope_resolver.py, and test_push_estimates_by_id.py to consume the shared
fixtures instead of defining local copies.

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.

Nitpick comments:
In `@backend/mpt_usage_reporting_extension/pipeline.py`:
- Around line 394-401: The sequential await in _reset_products is suppressed
without justification. Determine whether deleter.delete calls for each
product_id are independent; if so, parallelize them with asyncio.gather and
merge all outcomes afterward, otherwise retain the loop and add a concise
rationale explaining the required ordering or tracing constraint next to the
WPS476 suppression.

In `@backend/tests/services/test_bucket_delete.py`:
- Around line 23-51: The test doubles _StubSubscriptions and _aiter are
duplicated across backend test modules. Move their shared definitions into the
backend test fixtures package or a services-scoped conftest registered through
pytest_plugins, expose the subscriptions fixture there, and update
test_bucket_delete.py, test_scope_resolver.py, and test_push_estimates_by_id.py
to consume the shared fixtures instead of defining local copies.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: e7062235-c143-4f2b-8030-be161b045603

📥 Commits

Reviewing files that changed from the base of the PR and between 6eb0bc1 and 622c329.

📒 Files selected for processing (8)

• backend/mpt_usage_reporting_extension/cli/commands/delete.py
• backend/mpt_usage_reporting_extension/pipeline.py
• backend/mpt_usage_reporting_extension/services/bucket_delete.py
• backend/mpt_usage_reporting_extension/services/scope_resolver.py
• backend/tests/cli/commands/test_delete.py
• backend/tests/services/test_bucket_delete.py
• backend/tests/services/test_scope_resolver.py
• backend/tests/test_pipeline.py

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• softwareone-platform/mpt-extension-skills (manual)

📜 Review details

⏰ Context from checks skipped due to timeout. (6)

• GitHub Check: mpt-extension-usage-reporting (Prerequisites Create standard build artifact)
• GitHub Check: mpt-extension-usage-reporting (Prerequisites Set the version)
• GitHub Check: mpt-extension-usage-reporting (Helm Build Helm Chart)
• GitHub Check: mpt-extension-usage-reporting (Build Build Image)
• GitHub Check: mpt-extension-usage-reporting (Prerequisites Create standard build artifact)
• GitHub Check: mpt-extension-usage-reporting (Prerequisites Set the version)

⚠️ CI failures not shown inline (1)

GitHub Actions: PR build and merge / 0_build.txt: MPT-23347 refactor BucketDeleter scope resolution and trace scope deletes

Conclusion: failure

View job details

##[group]Run SonarSource/sonarqube-scan-action@v8.2.0
 with:
   projectBaseDir: backend
   scannerVersion: 8.1.0.6389
   scannerBinariesUrl: https://binaries.sonarsource.com/Distribution/sonar-scanner-cli
   skipSignatureVerification: false
 env:
   GITHUB_***REDACTED***
   SONAR_***REDACTED***
 ##[endgroup]
 Installing Sonar Scanner CLI 8.1.0.6389 for linux-x64...
 Downloading from: https://binaries.sonarsource.com/Distribution/sonar-scanner-cli/sonar-scanner-cli-8.1.0.6389-linux-x64.zip
 Downloading signature from: https://binaries.sonarsource.com/Distribution/sonar-scanner-cli/sonar-scanner-cli-8.1.0.6389-linux-x64.zip.asc
 Importing SonarSource public key from hkps://keyserver.ubuntu.com...
 [command]/usr/bin/gpg --homedir /home/runner/work/_temp/gpg-home-1784649246563-5205 --batch --keyserver hkps://keyserver.ubuntu.com --recv-keys 679F1EE92B19609DE816FDE81DB198F93525EC1A
 gpg: keybox '/home/runner/work/_temp/gpg-home-1784649246563-5205/pubring.kbx' created
 gpg: /home/runner/work/_temp/gpg-home-1784649246563-5205/trustdb.gpg: trustdb created
 gpg: key 1DB198F93525EC1A: public key "SonarSource S.A. <infra@sonarsource.com>" imported
 gpg: Total number processed: 1
 gpg:               imported: 1
 Successfully imported key from hkps://keyserver.ubuntu.com
 ✓ SonarSource public key imported successfully
 Verifying GPG signature...
 [command]/usr/bin/gpg --homedir /home/runner/work/_temp/gpg-home-1784649246563-5205 --batch --verify /home/runner/work/_temp/2c648082-42cf-4c3f-8687-d7fc8481d90e /home/runner/work/_temp/bf66cb32-77cc-4a65-bd43-b21a213828f2
 gpg: Signature made Tue Apr 21 07:20:26 2026 UTC
 gpg:                using RSA key D1436C0DBACEA48702AF97C363F1DD7753B8B315
 gpg: Good signature from "SonarSource S.A. <infra@sonarsource.com>" [unknown]
 gpg: WARNING: This key is not certified with a trusted signature!
 gpg:          There is no indication that the signature belongs to the owner.
 Primary key fingerprint: 679F 1EE9 2B19 609D E816  FDE8 1DB1 98F9 ...

🧰 Additional context used

📓 Path-based instructions (4)

**/*

⚙️ CodeRabbit configuration file

**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved

Files:

• backend/tests/cli/commands/test_delete.py
• backend/mpt_usage_reporting_extension/services/scope_resolver.py
• backend/mpt_usage_reporting_extension/cli/commands/delete.py
• backend/tests/services/test_scope_resolver.py
• backend/mpt_usage_reporting_extension/pipeline.py
• backend/tests/services/test_bucket_delete.py
• backend/tests/test_pipeline.py
• backend/mpt_usage_reporting_extension/services/bucket_delete.py

backend/**/*.py

⚙️ CodeRabbit configuration file

backend/**/*.py: Follow the linting rules defined in backend/pyproject.toml under [tool.ruff] and [tool.flake8].
For formatting, use Ruff instead of Black. Do not suggest Black formatting changes.
Review code against the linked repository's standards/python-coding.md.
Flag any code artifact not written in English: identifiers, comments, docstrings, log messages, error messages, or test names in any other language must be reported and translated to English.
Flag module-level docstrings in __init__.py files, and redundant module-level docstrings that only restate the module name or path.
Verify modules are organized into cohesive packages instead of flat or grab-bag utils/helpers modules, and flag inline linter or type-checker ignores (# noqa, # type: ignore) that are not a narrow, justified last resort.

Files:

• backend/tests/cli/commands/test_delete.py
• backend/mpt_usage_reporting_extension/services/scope_resolver.py
• backend/mpt_usage_reporting_extension/cli/commands/delete.py
• backend/tests/services/test_scope_resolver.py
• backend/mpt_usage_reporting_extension/pipeline.py
• backend/tests/services/test_bucket_delete.py
• backend/tests/test_pipeline.py
• backend/mpt_usage_reporting_extension/services/bucket_delete.py

backend/**

⚙️ CodeRabbit configuration file

backend/**: Review backend changes against AGENTS.md, docs/architecture.md, docs/contributing.md, and docs/testing.md.
Use relevant linked shared standards and operational guidance when those local documents reference them.
If the change adds or alters behaviour, components, configuration, or commands, verify the corresponding documentation (docs/*, README.md, AGENTS.md) is updated per standards/documentation.md.

Files:

• backend/tests/cli/commands/test_delete.py
• backend/mpt_usage_reporting_extension/services/scope_resolver.py
• backend/mpt_usage_reporting_extension/cli/commands/delete.py
• backend/tests/services/test_scope_resolver.py
• backend/mpt_usage_reporting_extension/pipeline.py
• backend/tests/services/test_bucket_delete.py
• backend/tests/test_pipeline.py
• backend/mpt_usage_reporting_extension/services/bucket_delete.py

backend/tests/**

⚙️ CodeRabbit configuration file

backend/tests/**: Review backend test changes against docs/testing.md and the linked repository's standards/unittests.md.
Verify that repository-specific test behavior and shared unit-test rules are followed.
Verify tests are written as functions (not classes), are grouped into packages mirroring the source, share setup through fixtures (splitting a large conftest into a fixtures package registered via pytest_plugins), keep fixture dependency depth at 3 or fewer levels, and use freezegun instead of patching datetime.

Files:

• backend/tests/cli/commands/test_delete.py
• backend/tests/services/test_scope_resolver.py
• backend/tests/services/test_bucket_delete.py
• backend/tests/test_pipeline.py

🔇 Additional comments (12)

backend/mpt_usage_reporting_extension/services/scope_resolver.py (1)

1-57: LGTM!

backend/mpt_usage_reporting_extension/services/bucket_delete.py (1)

19-69: LGTM!

Also applies to: 106-217, 219-258

backend/mpt_usage_reporting_extension/pipeline.py (1)

26-26: LGTM!

Also applies to: 44-44, 133-147

backend/mpt_usage_reporting_extension/cli/commands/delete.py (1)

15-20: LGTM!

Also applies to: 71-86

backend/tests/services/test_scope_resolver.py (2)

13-39: Duplicate of the _StubSubscriptions/_aiter test double already defined in test_bucket_delete.py.

Same root cause as flagged in backend/tests/services/test_bucket_delete.py — will be covered by the consolidated comment.

---

60-97: LGTM!

query_for, agreement_ids dedupe/skip-missing, upstream error wrapping, and subscription_agreements delegation are all correctly exercised against the ScopeResolver contract shown in the snippets. Tests are plain functions with shallow fixture dependency depth, consistent with test guidance.

backend/tests/services/test_bucket_delete.py (3)

15-20: LGTM!

Fixture wiring (resolver → deleter → scope_deleter) and the new multi-line imports correctly reflect the ScopeResolver/BucketDeleter/ScopeBucketDeleter composition shown in bucket_delete.py.

Also applies to: 77-88

---

91-343: Test assertions correctly track the DeleteOutcome.statement_agreements contract.

All the refactored scope_deleter.delete(...) calls, dedupe/skip-missing/zero-delete scenarios, and dry-run variants match the upstream BucketDeleter/ScopeBucketDeleter behavior shown in the referenced snippets (e.g. statement_agreements bootstrapped even on zero-delete outcomes).

---

246-257: 🎯 Functional Correctness

No change needed. DeleteReport.render() logs the per-subscription and per-agreement lines, plus the summary, so test_delete_reports_the_summary matches the emitted log text.

			> Likely an incorrect or invalid review comment.

backend/tests/cli/commands/test_delete.py (1)

44-50: LGTM!

Patching ScopeBucketDeleter directly matches _build_scope_deleter's construction (ScopeBucketDeleter(BucketDeleter(...), resolver)), and the assertion on deleter.delete is unaffected by the internal BucketDeleter/ScopeResolver wiring.

backend/tests/test_pipeline.py (2)

61-64: LGTM!

Patching pipeline.ScopeBucketDeleter instead of pipeline.BucketDeleter correctly targets the class pipeline.reset()/_reset_products() actually instantiate.

---

157-160: LGTM!

Each updated DeleteOutcome(...) mock return correctly reflects the new statement_agreements propagation through pipeline.reset()/_reset_products() for the corresponding scope (subscription/agreement/product/None), matching the ResetScope construction shown in the referenced pipeline.py snippets.

Also applies to: 169-173, 187-191, 208-214, 245-252, 284-287, 310-315, 359-362, 393-395, 426-427, 461-462, 487-490

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud