Conversation
📝 WalkthroughWalkthroughThis PR introduces PLS (Partner Logic Support) charge processing by refactoring the line processor architecture, adding marketplace-specific charge filtering, and integrating a new PLS charge manager into billing journal generation. Configuration is extended to support percentage-based PLS charges via Helm values and application context. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
swo_aws_extension/flows/jobs/billing_journal/generators/journal_line.py (1)
34-34: Prefer a named constant/enum for the marketplace record-type key.Line 34 uses a raw string literal (
"MARKETPLACE"), which is easy to drift from upstream record-type values. Use a shared constant (or enum entry) to reduce typo and mismatch risk.♻️ Suggested tweak
+MARKETPLACE_RECORD_TYPE = "MARKETPLACE" + def _build_processor_registry(*, is_pls: bool = False) -> dict[str, JournalLineProcessor]: @@ - "MARKETPLACE": marketplace, + MARKETPLACE_RECORD_TYPE: marketplace,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swo_aws_extension/flows/jobs/billing_journal/generators/journal_line.py` at line 34, Replace the raw string literal "MARKETPLACE" used as the record-type key in the dict with a shared constant or enum entry to avoid drift/typos; locate the literal in journal_line.py (the dict entry with "MARKETPLACE": marketplace) and import/use the project-wide record-type identifier (for example RECORD_TYPE_MARKETPLACE or RecordType.MARKETPLACE) instead of the string so the key is tied to the canonical constant used across the billing_journal codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@swo_aws_extension/flows/jobs/billing_journal/generators/pls_charge_manager.py`:
- Around line 60-68: Replace the hardcoded placeholder invoice_entity value with
the established empty-string fallback used in base.py: when constructing
InvoiceDetails in pls_charge_manager (the InvoiceDetails(...) call used to build
JournalLine via JournalLine.build), set invoice_entity="" instead of
"invoice_entity_name" so the PLS charge follows the same fallback pattern as
other generators.
In `@tests/flows/jobs/billing_journal/generators/test_pls_charge_manager.py`:
- Around line 31-38: build_usage_result currently constructs an
OrganizationUsageResult with reports set to an empty dict which mismatches the
typed OrganizationReport; change build_usage_result to create and pass a proper
OrganizationReport instance (e.g., instantiate OrganizationReport with an
empty/internal-empty payload) instead of {} so
OrganizationUsageResult(reports=OrganizationReport(...), usage_by_account={...})
preserves type safety; update the call site in build_usage_result and ensure
AccountUsage and OrganizationUsageResult usage remains unchanged.
---
Nitpick comments:
In `@swo_aws_extension/flows/jobs/billing_journal/generators/journal_line.py`:
- Line 34: Replace the raw string literal "MARKETPLACE" used as the record-type
key in the dict with a shared constant or enum entry to avoid drift/typos;
locate the literal in journal_line.py (the dict entry with "MARKETPLACE":
marketplace) and import/use the project-wide record-type identifier (for example
RECORD_TYPE_MARKETPLACE or RecordType.MARKETPLACE) instead of the string so the
key is tied to the canonical constant used across the billing_journal codebase.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: aaf063c5-634b-4bb6-83da-3a0e34c40d24
📒 Files selected for processing (14)
swo_aws_extension/flows/jobs/billing_journal/generators/agreement.pyswo_aws_extension/flows/jobs/billing_journal/generators/currency.pyswo_aws_extension/flows/jobs/billing_journal/generators/journal_line.pyswo_aws_extension/flows/jobs/billing_journal/generators/line_processors/base.pyswo_aws_extension/flows/jobs/billing_journal/generators/line_processors/credit.pyswo_aws_extension/flows/jobs/billing_journal/generators/line_processors/marketplace.pyswo_aws_extension/flows/jobs/billing_journal/generators/pls_charge_manager.pyswo_aws_extension/parameters.pytests/flows/jobs/billing_journal/generators/line_processors/test_base.pytests/flows/jobs/billing_journal/generators/line_processors/test_credit.pytests/flows/jobs/billing_journal/generators/line_processors/test_marketplace.pytests/flows/jobs/billing_journal/generators/test_agreement.pytests/flows/jobs/billing_journal/generators/test_journal_line.pytests/flows/jobs/billing_journal/generators/test_pls_charge_manager.py
swo_aws_extension/flows/jobs/billing_journal/generators/pls_charge_manager.py
Show resolved
Hide resolved
ruben-sebrango
force-pushed
the
MPT-18440-aws-billing-transfer-support-pls-additional-discount
branch
from
5ede6b1 to
41b324f
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@swo_aws_extension/config.py`:
- Line 6: Update the config so DEFAULT_PLS_CHARGE_PERCENTAGE and the
EXTENSION_CONFIG lookup return a Decimal instead of float and treat empty or
whitespace values as missing (fall back to Decimal('5.0')); specifically, when
reading PLS_CHARGE_PERCENTAGE from EXTENSION_CONFIG parse/validate using Decimal
(strip whitespace and check emptiness) and default to Decimal('5.0') if blank or
invalid, and then remove the redundant Decimal(str(...)) wrapping in
generate_billing_journals.py (around the value used at the previous line 89)
since the config property now already yields a Decimal.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 44c24ce7-bfd8-4af7-bf28-a5cafb7fb6a2
📒 Files selected for processing (21)
helm/swo-extension-aws/charts/api/templates/configmap.yamlhelm/swo-extension-aws/charts/worker/templates/configmap.yamlhelm/swo-extension-aws/values.yamlswo_aws_extension/config.pyswo_aws_extension/flows/jobs/billing_journal/generators/agreement.pyswo_aws_extension/flows/jobs/billing_journal/generators/currency.pyswo_aws_extension/flows/jobs/billing_journal/generators/journal_line.pyswo_aws_extension/flows/jobs/billing_journal/generators/line_processors/base.pyswo_aws_extension/flows/jobs/billing_journal/generators/line_processors/credit.pyswo_aws_extension/flows/jobs/billing_journal/generators/line_processors/marketplace.pyswo_aws_extension/flows/jobs/billing_journal/generators/pls_charge_manager.pyswo_aws_extension/flows/jobs/billing_journal/models/context.pyswo_aws_extension/management/commands/generate_billing_journals.pytests/flows/jobs/billing_journal/conftest.pytests/flows/jobs/billing_journal/generators/line_processors/test_base.pytests/flows/jobs/billing_journal/generators/line_processors/test_credit.pytests/flows/jobs/billing_journal/generators/line_processors/test_marketplace.pytests/flows/jobs/billing_journal/generators/test_agreement.pytests/flows/jobs/billing_journal/generators/test_journal_line.pytests/flows/jobs/billing_journal/generators/test_pls_charge_manager.pytests/flows/jobs/billing_journal/models/test_context.py
✅ Files skipped from review due to trivial changes (6)
- helm/swo-extension-aws/values.yaml
- helm/swo-extension-aws/charts/api/templates/configmap.yaml
- swo_aws_extension/flows/jobs/billing_journal/models/context.py
- helm/swo-extension-aws/charts/worker/templates/configmap.yaml
- tests/flows/jobs/billing_journal/generators/line_processors/test_marketplace.py
- tests/flows/jobs/billing_journal/generators/test_pls_charge_manager.py
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/flows/jobs/billing_journal/generators/line_processors/test_credit.py
- tests/flows/jobs/billing_journal/generators/test_journal_line.py
- tests/flows/jobs/billing_journal/generators/line_processors/test_base.py
- swo_aws_extension/flows/jobs/billing_journal/generators/currency.py
- tests/flows/jobs/billing_journal/generators/test_agreement.py
- swo_aws_extension/flows/jobs/billing_journal/generators/line_processors/credit.py
- swo_aws_extension/flows/jobs/billing_journal/generators/journal_line.py
d3rky
approved these changes
Mar 24, 2026
d3rky
deleted the
MPT-18440-aws-billing-transfer-support-pls-additional-discount
branch
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
2 participants
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.
Closes MPT-18440
PlSChargeManagerto compute and generate PLS charge journal lines based on a configurable percentage applied to total usage amountsMarketplaceJournalLineProcessorto filter out Tax service metrics from marketplace recordsLineProcessorbase class toJournalLineProcessorand updated all subclasses (CreditJournalLineProcessor,MarketplaceJournalLineProcessor)resolve_service_amount()utility function to handle currency conversion and exchange rate calculationsAgreementJournalGeneratorto detect PLS support type and trigger PLS charge processingJournalLineGeneratorwithis_plsparameter to conditionally exclude support record types when PLS is enabledEXT_PLS_CHARGE_PERCENTAGEenvironment variable with a default value of 5.0% (configurable via Helm values)BillingJournalContextmodel to includepls_charge_percentagefield