[MPT-14913] Added e2e tests for journal charges

[robcsegal]

Added e2e tests for journal charges.
Had to reseed and recreate journal information after platform fix.

https://softwareone.atlassian.net/browse/MPT-14913

Closes MPT-14913

• Added end-to-end test fixtures for journal charges (valid and invalid journal charge IDs)
• Added async e2e tests for journal charges: get by ID, 404 handling for invalid ID, list with pagination, and RQL-based filtering
• Added sync e2e tests for journal charges with equivalent coverage
• Updated e2e config and test data to reseeded journal identifiers:
  • billing.journal.id -> BJO-2589-1434
  • billing.journal.attachment.id -> JOA-2849-3620
  • billing.journal.charge.id -> CHG-2589-1434-0000-0000-0200
  • test data external/reference IDs adjusted to match reseeded data
• Tests marked as flaky and validate MPT API error handling and query filtering features

[coderabbitai]

📝 Walkthrough

Walkthrough

Updated billing journal identifiers and test data; added fixtures and two new end-to-end test modules (async and sync) for billing journal charge operations.

Changes

Cohort / File(s)	Summary
Configuration and Test Data Updates e2e_config.test.json, tests/data/test_billing_journal.jsonl	Updated billing journal identifiers: billing.journal.id -> BJO-2589-1434, billing.journal.attachment.id -> JOA-2849-3620; added billing.journal.charge.id -> CHG-2589-1434-0000-0000-0200; updated external/search references in test JSONL record.
Test Fixtures tests/e2e/billing/journal/charge/conftest.py	Added pytest fixtures: journal_charge_id(e2e_config) returning the charge ID from config and invalid_journal_charge_id() returning an invalid charge ID string.
Async Journal Charge Tests tests/e2e/billing/journal/charge/test_async_journal_charge.py	New async E2E tests: get by ID (success), get by invalid ID (expect 404/MPTAPIError), list with pagination, and filter using RQLQuery; includes journal_charges fixture binding to async client.
Sync Journal Charge Tests tests/e2e/billing/journal/charge/test_sync_journal_charge.py	New sync E2E tests mirroring async cases: retrieval, not-found handling, listing, and RQL-based filtering; marked flaky.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Jira Issue Key In Title	✅ Passed	The PR title contains exactly one Jira issue key MPT-14913 in the correct MPT-XXXX format at the beginning.
Test Coverage Required	✅ Passed	All modified files are test-related files in the tests/ folder. No source code files were modified, satisfying the requirement.
Single Commit Required	✅ Passed	The PR contains exactly one commit, which adheres to the single commit requirement.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/e2e/billing/journal/charge/test_sync_journal_charge.py (1)

25-30: Consider verifying pagination limit is respected.

The test validates that results are returned but doesn't verify the pagination limit. While the current assertion is sufficient for e2e testing, you could optionally add a check that len(result) <= limit to ensure pagination works correctly.

♻️ Optional enhancement

 def test_list_journal_charges(journal_charges):
     limit = 10
 
     result = journal_charges.fetch_page(limit=limit)
 
     assert len(result) > 0
+    assert len(result) <= limit

📜 Review details

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

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 92f8042 and 4d70046.

⛔ Files ignored due to path filters (1)

• tests/data/test_billing_journal.xlsx is excluded by !**/*.xlsx

📒 Files selected for processing (5)

• e2e_config.test.json
• tests/data/test_billing_journal.jsonl
• tests/e2e/billing/journal/charge/conftest.py
• tests/e2e/billing/journal/charge/test_async_journal_charge.py
• tests/e2e/billing/journal/charge/test_sync_journal_charge.py

🚧 Files skipped from review as they are similar to previous changes (3)

• tests/data/test_billing_journal.jsonl
• tests/e2e/billing/journal/charge/conftest.py
• tests/e2e/billing/journal/charge/test_async_journal_charge.py

🧰 Additional context used

📓 Path-based instructions (2)

**/*

⚙️ CodeRabbit configuration file

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

Files:

• tests/e2e/billing/journal/charge/test_sync_journal_charge.py
• e2e_config.test.json

**/*.py

⚙️ CodeRabbit configuration file

**/*.py: Follow the linting rules defined in pyproject.toml under [tool.ruff] and [tool.flake8] sections.
For formatting, use Ruff instead of Black. Do not suggest Black formatting changes.

Files:

• tests/e2e/billing/journal/charge/test_sync_journal_charge.py

🧠 Learnings (3)

📚 Learning: 2026-01-08T08:34:05.465Z

Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution. Reserve isolated fixtures (e.g., created_price_list) for destructive tests that require per-test creation and cleanup. Ensure tests document when a fixture creates/destroys data and clearly indicate which operations are destructive, so tests stay fast and properly isolated.

Applied to files:

• tests/e2e/billing/journal/charge/test_sync_journal_charge.py

📚 Learning: 2025-12-12T15:02:20.732Z

Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.

Applied to files:

• tests/e2e/billing/journal/charge/test_sync_journal_charge.py

📚 Learning: 2026-01-08T23:38:19.565Z

Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 186
File: tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py:33-39
Timestamp: 2026-01-08T23:38:19.565Z
Learning: In Python e2e tests under tests/e2e, hardcoded external IDs (e.g., INV12345) are intentional because they come from seeded test data. Ensure the test data seeds consistently include these IDs; if seeds change, update tests accordingly. Prefer using a named constant for such IDs (e.g., INV_EXTERNAL_ID) and document the dependency on seed data to avoid brittle tests.

Applied to files:

• tests/e2e/billing/journal/charge/test_sync_journal_charge.py

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: build

🔇 Additional comments (6)

tests/e2e/billing/journal/charge/test_sync_journal_charge.py (5)

1-6: LGTM!

Imports and test markers are properly configured for end-to-end testing.

---

9-11: LGTM!

The fixture correctly constructs the journal charges endpoint from the billing journal ID, following the established pattern for e2e test fixtures.

---

14-18: LGTM!

Basic retrieval test is correctly implemented.

---

20-22: LGTM!

Error handling test correctly validates 404 Not Found behavior.

---

33-43: No changes needed. The hardcoded "INV12345" is intentional and comes from the seeded test data (verified in test_billing_journal.jsonl). This pattern is consistent across all similar e2e tests (both journal and ledger, sync and async variants) and aligns with the project's approach to e2e testing where hardcoded values from import files are guaranteed to exist.

e2e_config.test.json (1)

19-22: Verify that journal and ledger charge IDs are intentionally identical.

Lines 19 and 22 both reference the same charge ID (CHG-2589-1434-0000-0000-0200) for different entities:

• billing.journal.charge.id (line 19)
• billing.ledger.charge.id (line 22)

This is inconsistent with the pattern elsewhere in the config: billing.custom_ledger.charge.id (line 16) uses a distinct ID (CHG-2665-3524-0000-0000-0020). Additionally, the parent entities themselves have different IDs (billing.journal.id vs billing.ledger.id), and they are tested in separate modules (tests/e2e/billing/journal/charge/ and tests/e2e/billing/ledger/charge/). If these represent distinct journal and ledger charges, using the same ID may prevent tests from properly validating entity-specific behavior.

⛔ Skipped due to learnings

Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 186
File: tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py:33-39
Timestamp: 2026-01-08T23:38:26.691Z
Learning: In the mpt-api-python-client e2e tests, hardcoded values like "INV12345" for invoice external IDs are intentional and guaranteed to exist because they come from import files used to seed the test data.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud