Skip to content

MPT-20694: include certificate in order factory payload#324

Merged
robcsegal merged 1 commit intomainfrom
MPT-20694-fix-flaky-commerce-order-e-2-e-tests-400-cannot-update-agreemment-on-setup
Apr 29, 2026
Merged

MPT-20694: include certificate in order factory payload#324
robcsegal merged 1 commit intomainfrom
MPT-20694-fix-flaky-commerce-order-e-2-e-tests-400-cannot-update-agreemment-on-setup

Conversation

@robcsegal
Copy link
Copy Markdown
Contributor

@robcsegal robcsegal commented Apr 29, 2026

🤖 AI-generated PR — Please review carefully.

Summary

Fixes the flaky commerce order e2e tests that were failing during setup with 400 Cannot update agreement on setup. The upstream API now requires a certificates reference on the agreement when an order is created, so the order factory was producing an invalid payload.

Changes

  • tests/e2e/commerce/order/conftest.py — add certificate_id to the order_factory signature and include "certificates": [{"id": certificate_id}] in the order payload.
  • tests/e2e/conftest.py — promote the certificate_id fixture to the shared e2e conftest so it is available to fixtures outside the program.certificate scope.
  • tests/e2e/program/certificate/conftest.py — remove the now-redundant local certificate_id fixture.

Testing

  • e2e commerce order suite (sync + async) to confirm the 400 on setup is gone.
  • Existing program certificate e2e tests still pass (fixture moved, not removed).

Jira

MPT-20694

Closes MPT-20694

Changes

  • Updated order_factory fixture to accept a certificate_id parameter and include a certificates field in the constructed order payload
  • Promoted certificate_id fixture from tests/e2e/program/certificate/conftest.py to tests/e2e/conftest.py to make it available across all e2e test scopes
  • Removed the local certificate_id fixture from tests/e2e/program/certificate/conftest.py after moving it to the shared e2e configuration

Impact

Resolves flaky commerce order e2e test setup failures by ensuring orders include the required certificate reference in the payload as mandated by the upstream API.

AI Generated.
Add a certificate reference to the commerce order factory so that order
creation in e2e tests no longer fails with a 400 when the upstream API
requires a certificate during agreement setup. Promote the certificate_id
fixture from tests/e2e/program/certificate/conftest.py to the shared
tests/e2e/conftest.py so the order factory can consume it outside the
program.certificate scope.
@robcsegal robcsegal requested a review from a team as a code owner April 29, 2026 15:05
@robcsegal robcsegal requested review from alephsur and jentyk April 29, 2026 15:05
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 29, 2026

📝 Walkthrough

Walkthrough

The pull request relocates the certificate_id fixture from a program-certificate-specific conftest to the root E2E conftest for broader test accessibility. The order_factory fixture is enhanced to accept a certificate_id parameter and embed it in the constructed order payload.

Changes

Cohort / File(s) Summary
Fixture Relocation
tests/e2e/conftest.py, tests/e2e/program/certificate/conftest.py
Moves certificate_id fixture from program-certificate-specific conftest to root E2E conftest, making it available across all E2E tests.
Factory Enhancement
tests/e2e/commerce/order/conftest.py
Updates order_factory to accept a certificate_id parameter and includes it in the constructed order JSON under certificates: [{id: ...}].

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 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.
Jira Issue Key In Title ✅ Passed The PR title contains exactly one Jira issue key MPT-20694 in the correct MPT-XXXX format at the beginning.
Test Coverage Required ✅ Passed The PR includes changes exclusively within the tests/ folder (tests/e2e/commerce/order/conftest.py, tests/e2e/conftest.py, and tests/e2e/program/certificate/conftest.py). All three files are test fixtures.
Single Commit Required ✅ Passed The PR contains exactly one commit (f097b7e fix: MPT-20694 include certificate in order factory payload), satisfying the single commit requirement.

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


Review rate limit: 4/5 reviews remaining, refill in 12 minutes.

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

@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/e2e/conftest.py (1)

184-186: Align fixture scope with other config-backed IDs.

certificate_id (Line 184) is derived from session-level e2e_config, like the other *_id fixtures in this file. Consider making it session-scoped for consistent lifecycle and slightly less fixture churn.

Suggested diff
-@pytest.fixture
+@pytest.fixture(scope="session")
 def certificate_id(e2e_config):
     return e2e_config["program.certificate.id"]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/conftest.py` around lines 184 - 186, The certificate_id fixture is
created from the session-level e2e_config but currently has default (function)
scope; change the pytest fixture declaration for certificate_id to use session
scope so its lifecycle matches other config-backed *_id fixtures—update the
decorator on certificate_id (the pytest.fixture for certificate_id that returns
e2e_config["program.certificate.id"]) to include scope="session".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/e2e/conftest.py`:
- Around line 184-186: The certificate_id fixture is created from the
session-level e2e_config but currently has default (function) scope; change the
pytest fixture declaration for certificate_id to use session scope so its
lifecycle matches other config-backed *_id fixtures—update the decorator on
certificate_id (the pytest.fixture for certificate_id that returns
e2e_config["program.certificate.id"]) to include scope="session".

ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro

Run ID: e4466201-8f0e-439a-9025-b6be718783f8

📥 Commits

Reviewing files that changed from the base of the PR and between 486461f and f097b7e.

📒 Files selected for processing (3)
  • tests/e2e/commerce/order/conftest.py
  • tests/e2e/conftest.py
  • tests/e2e/program/certificate/conftest.py
💤 Files with no reviewable changes (1)
  • tests/e2e/program/certificate/conftest.py

@robcsegal robcsegal merged commit 8389441 into main Apr 29, 2026
4 checks passed
@robcsegal robcsegal deleted the MPT-20694-fix-flaky-commerce-order-e-2-e-tests-400-cannot-update-agreemment-on-setup branch April 29, 2026 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants