MPT-19910: add e2e tests for currency endpoints

[albertsola]

Summary

Adds end-to-end tests for the CurrenciesService and AsyncCurrenciesService covering all operations exposed by the service.

Changes

• tests/e2e/exchange/__init__.py — new module init
• tests/e2e/exchange/currencies/__init__.py — new module init
• tests/e2e/exchange/currencies/conftest.py — fixtures for service, currency ID, currency data, and created currency (with icon upload + teardown)
• tests/e2e/exchange/currencies/test_sync_currencies.py — sync e2e tests
• tests/e2e/exchange/currencies/test_async_currencies.py — async e2e tests
• e2e_config.test.json — added exchange.currency.id seeded value

Tests covered

Test	Sync	Async
create	✅	✅
get	✅	✅
get not found (404)	✅	✅
update	✅	✅
delete	✅	✅
filter + iterate	✅	✅
download icon	✅	✅

Closes MPT-19910

• Add comprehensive end-to-end tests for CurrenciesService and AsyncCurrenciesService covering create, retrieve, update, delete, filter/iterate, and icon download operations
• Add pytest fixtures in tests/e2e/exchange/currencies/conftest.py providing sync/async service instances, deterministic currency test data, logo file fixture, and lifecycle management (create + teardown with delete; teardown ignores MPTAPIError)
• Add synchronous e2e tests in tests/e2e/exchange/currencies/test_sync_currencies.py validating CRUD, 404 handling, filtering with iteration, and icon download (asserting FileModel)
• Add asynchronous e2e tests in tests/e2e/exchange/currencies/test_async_currencies.py mirroring sync tests with async/await patterns; module-level flaky mark applied
• Update e2e_config.test.json to add exchange.currency.id (CUR-6831), add helpdesk channel/chat IDs, and reorder related config entries for logical grouping

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro

Run ID: db365f58-5a4c-4100-bbe0-6c0038f19e50

📥 Commits

Reviewing files that changed from the base of the PR and between 45d37c8 and 6fdb67a.

📒 Files selected for processing (6)

• e2e_config.test.json
• tests/e2e/exchange/__init__.py
• tests/e2e/exchange/currencies/__init__.py
• tests/e2e/exchange/currencies/conftest.py
• tests/e2e/exchange/currencies/test_async_currencies.py
• tests/e2e/exchange/currencies/test_sync_currencies.py

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

• e2e_config.test.json
• tests/e2e/exchange/currencies/test_sync_currencies.py
• tests/e2e/exchange/currencies/test_async_currencies.py

---

📝 Walkthrough

Walkthrough

Added exchange.currency.id to test config and reordered some ID entries; introduced E2E fixtures and resource-management teardown for exchange currencies; added synchronous and asynchronous E2E test suites covering CRUD, filtering, error cases, and icon download.

Changes

Cohort / File(s)	Summary
Configuration e2e_config.test.json	Inserted exchange.currency.id and added helpdesk.channel.id, helpdesk.chat.id; reordered several existing ID entries (commerce, notifications, commerce.user). No ID values changed otherwise.
Test Fixtures tests/e2e/exchange/currencies/conftest.py	New pytest fixtures: currencies_service, async_currencies_service, session currency_id (reads exchange.currency.id), deterministic currency_data(short_uuid), and lifecycle fixtures created_currency / async_created_currency with teardown that catches MPTAPIError.
Test Suites tests/e2e/exchange/currencies/test_sync_currencies.py, tests/e2e/exchange/currencies/test_async_currencies.py	New sync and async E2E tests (file-level flaky mark) for create/get/update/delete, 404 handling, filter iteration assertions, and icon download; assertions include field values and FileModel type checks.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• MPT-13307: implement exchange currencies resource #274 — Adds Exchange/Currencies service surface (sync and async) used by these new E2E tests.
• MPT-14896 E2E for catalog/price_lists #138 — Provides test helper fixtures/functions (e.g., filter/iterate assertions) referenced by the new currency tests.

🚥 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-19910) in the correct format, properly positioned at the beginning.
Test Coverage Required	✅ Passed	PR modifies only test and configuration files; no source code in mpt_api_client/ directory was changed. PR summary confirms no exported/public code entities were altered.
Single Commit Required	✅ Passed	The pull request contains exactly one commit (6fdb67a) with all changes for adding E2E tests for currency endpoints, including e2e_config.test.json updates and new test fixtures in tests/e2e/exchange/currencies/.

_{✏️ 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: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/e2e/exchange/currencies/conftest.py`:
- Around line 42-45: The teardown currently catches all MPTAPIError when calling
currencies_service.delete(currency.id) which hides real failures; change the
handler to only suppress expected not-found errors by checking the error's
status/code (e.g., error.status == 404 or error.code == "not_found") and
silently ignore those, but for any other MPTAPIError re-raise or log and raise
so real teardown failures surface; apply the same change to the other similar
block that calls currencies_service.delete.

In `@tests/e2e/exchange/currencies/test_async_currencies.py`:
- Around line 29-36: The test_update_currency may send an empty file because
logo_fd was already consumed by the initial create call; before calling
async_currencies_service.update in test_update_currency, reset the file-like
object's cursor (e.g., call logo_fd.seek(0)) so the uploaded file is read from
the start when async_currencies_service.update(...) is invoked; update
references: test_update_currency, async_currencies_service.update, and
async_created_currency.create.

In `@tests/e2e/exchange/currencies/test_sync_currencies.py`:
- Around line 29-33: The test test_update_currency uses logo_fd previously
consumed during creation, so reset the file pointer before reuse: ensure logo_fd
is rewound (e.g., call logo_fd.seek(0) or equivalent) right before invoking
currencies_service.update(created_currency.id, update_data, file=logo_fd) so the
upload reads the file from the beginning; check logo_fd supports seek to avoid
flaky EOF uploads.

🪄 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: 461d270a-eb76-4c9f-982b-6e0345f9febf

📥 Commits

Reviewing files that changed from the base of the PR and between 5340a75 and d89775a.

📒 Files selected for processing (6)

• e2e_config.test.json
• tests/e2e/exchange/__init__.py
• tests/e2e/exchange/currencies/__init__.py
• tests/e2e/exchange/currencies/conftest.py
• tests/e2e/exchange/currencies/test_async_currencies.py
• tests/e2e/exchange/currencies/test_sync_currencies.py

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

tests/e2e/exchange/currencies/conftest.py (2)

40-43: ⚠️ Potential issue | 🟠 Major

Teardown still swallows all API errors (duplicate of previous review).

The teardown continues to catch all MPTAPIError exceptions without discriminating. Only 404 (resource already deleted) should be suppressed; other failures (permissions, network issues, etc.) should surface to prevent resource leaks and mask real problems.

🛡️ Proposed fix to check error status

     try:
         currencies_service.delete(currency.id)
     except MPTAPIError as error:
-        print(f"TEARDOWN - Unable to delete currency {currency.id}: {error.title}")  # noqa: WPS421
+        if error.status != 404:
+            raise

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/exchange/currencies/conftest.py` around lines 40 - 43, The teardown
currently swallows all MPTAPIError exceptions in the call to
currencies_service.delete(currency.id); modify the except block for MPTAPIError
to inspect the HTTP status on the exception (e.g., error.status or
error.status_code) and only suppress/log and continue when it equals 404
(resource already gone), otherwise re-raise the exception so
permission/network/errors surface and don't get masked.

---

52-55: ⚠️ Potential issue | 🟠 Major

Async teardown has the same error suppression issue (duplicate).

Same as the sync version: all MPTAPIError exceptions are caught without checking the status. Only 404 should be ignored; other errors must surface.

🛡️ Proposed fix to check error status

     try:
         await async_currencies_service.delete(currency.id)
     except MPTAPIError as error:
-        print(f"TEARDOWN - Unable to delete currency {currency.id}: {error.title}")  # noqa: WPS421
+        if error.status != 404:
+            raise

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/exchange/currencies/conftest.py` around lines 52 - 55, The async
teardown is suppressing all MPTAPIError exceptions—update the exception handler
around async_currencies_service.delete(currency.id) so it only ignores the error
when the MPTAPIError indicates a 404 (e.g., check error.status == 404 or
equivalent property on MPTAPIError) and re-raise or let other errors surface;
keep the existing log for the 404 case but do not swallow non-404 errors.

🧹 Nitpick comments (1)

tests/e2e/exchange/currencies/conftest.py (1)

23-31: Currency code generation is functional but has minor collision risk.

The digit-to-alpha translation for generating unique currency codes works correctly. Since short_uuid is hex (0-9a-f), the first three characters could theoretically collide (e.g., "abc" → "ABC", "afc" → "AFC" differ by only one char). For sequential E2E tests this is likely acceptable, but be aware of potential collisions if tests run concurrently.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/exchange/currencies/conftest.py` around lines 23 - 31, The currency
code generation in fixture currency_data uses only the first three chars of
short_uuid and a digit->alpha map (digit_to_alpha) which can lead to collisions
in concurrent runs; update currency_data to produce a more unique code by
expanding the input (e.g., use the first 4–6 characters of short_uuid or append
a short random/timestamp suffix) and keep the same translation logic
(digit_to_alpha) and .upper() call so existing downstream expectations remain
valid; ensure the resulting code still meets any length/format constraints
expected by the system.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/e2e/exchange/currencies/conftest.py`:
- Line 43: Remove the inline "# noqa: WPS421" from the print statement in
tests/e2e/exchange/currencies/conftest.py (the print(f"TEARDOWN - Unable to
delete currency {currency.id}: {error.title}") line) and add a per-file-ignores
entry in pyproject.toml under [tool.flake8] to suppress WPS421 for this file (or
for the broader tests/e2e/**/conftest.py pattern if preferred); update the
per-file-ignores list to include the path and WPS421 so the inline suppression
can be deleted.

---

Duplicate comments:
In `@tests/e2e/exchange/currencies/conftest.py`:
- Around line 40-43: The teardown currently swallows all MPTAPIError exceptions
in the call to currencies_service.delete(currency.id); modify the except block
for MPTAPIError to inspect the HTTP status on the exception (e.g., error.status
or error.status_code) and only suppress/log and continue when it equals 404
(resource already gone), otherwise re-raise the exception so
permission/network/errors surface and don't get masked.
- Around line 52-55: The async teardown is suppressing all MPTAPIError
exceptions—update the exception handler around
async_currencies_service.delete(currency.id) so it only ignores the error when
the MPTAPIError indicates a 404 (e.g., check error.status == 404 or equivalent
property on MPTAPIError) and re-raise or let other errors surface; keep the
existing log for the 404 case but do not swallow non-404 errors.

---

Nitpick comments:
In `@tests/e2e/exchange/currencies/conftest.py`:
- Around line 23-31: The currency code generation in fixture currency_data uses
only the first three chars of short_uuid and a digit->alpha map (digit_to_alpha)
which can lead to collisions in concurrent runs; update currency_data to produce
a more unique code by expanding the input (e.g., use the first 4–6 characters of
short_uuid or append a short random/timestamp suffix) and keep the same
translation logic (digit_to_alpha) and .upper() call so existing downstream
expectations remain valid; ensure the resulting code still meets any
length/format constraints expected by the system.

🪄 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: e744d9b9-f101-4864-b678-bf5a0a182f4b

📥 Commits

Reviewing files that changed from the base of the PR and between d89775a and 45d37c8.

📒 Files selected for processing (1)

• tests/e2e/exchange/currencies/conftest.py

[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