MPT-19058 Adopt notification templates in AWS Extension#283
Merged
Conversation
📝 WalkthroughWalkthroughThis PR renames four order-status helpers to remove implicit notification side-effects, updates all call sites and tests to the new names, and adds try/except handling in the processing helper to stop the notification path on MPTError. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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. 📝 Coding Plan
Comment |
alephsur
force-pushed
the
MPT-19058-adopt-notification-templates-in-aws-extension
branch
from
4b4425a to
064f610
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
swo_aws_extension/flows/order_utils.py (1)
105-126:⚠️ Potential issue | 🟠 MajorDo not silently swallow Processing transition failures.
At Line 120–126,
MPTErroris logged and then returned, which hides the failure from callers and can leave the flow assuming the order moved to Processing when it did not.Proposed fix (explicit success/failure contract)
-def switch_order_status_to_process( +def switch_order_status_to_process( client: MPTClient, context: InitialAWSContext, template_name: str -): +) -> bool: @@ try: context.order = process_order( client, context.order_id, **kwargs, ) except MPTError as error: logger.info( "%s - Cannot switch order to 'Processing': %s", context.order_id, error, ) - return + return False + return True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swo_aws_extension/flows/order_utils.py` around lines 105 - 126, The function switch_order_status_to_process currently logs MPTError and returns None, silently swallowing failures; change it to provide an explicit success/failure contract by returning True on success and False on failure. In switch_order_status_to_process (which calls set_order_template and process_order), wrap the process_order call in the existing try/except but after logging the MPTError return False (or alternatively re-raise if you prefer exceptions); on successful completion return True. Update callers to check the boolean result so they do not assume the order moved to Processing when process_order failed.swo_aws_extension/processors/querying/aws_customer_roles.py (1)
83-88:⚠️ Potential issue | 🟠 MajorSwitch to Processing before updating phase/parameters in timeout path.
At Line 83–88, the code updates phase via
update_orderwhile still in querying, then switches status. This is the wrong order for this flow and can fail state updates.Proposed ordering fix
- context.order = set_phase(context.order, PhasesEnum.CREATE_SUBSCRIPTION) - context.order = update_order( - self.client, context.order_id, parameters=context.order["parameters"] - ) - - switch_order_status_to_process(self.client, context, get_template_name(context)) + switch_order_status_to_process(self.client, context, get_template_name(context)) + context.order = set_phase(context.order, PhasesEnum.CREATE_SUBSCRIPTION) + context.order = update_order( + self.client, context.order_id, parameters=context.order["parameters"] + )Based on learnings: “order parameters cannot be updated while the order is in "querying" state… the order must first be switched to "processing" … and then the phase can be updated via a separate update_order call.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swo_aws_extension/processors/querying/aws_customer_roles.py` around lines 83 - 88, The current sequence calls set_phase and update_order while the order is still in querying; change the flow to first call switch_order_status_to_process(self.client, context, get_template_name(context)) to transition the order to processing, then perform a separate set_phase(context.order, PhasesEnum.CREATE_SUBSCRIPTION) and follow that with update_order(self.client, context.order_id, parameters=context.order["parameters"]) so phase/parameter updates occur after the state switch; update only the calls around set_phase, update_order, and switch_order_status_to_process to reflect this new ordering.swo_aws_extension/processors/querying/aws_channel_handshake.py (1)
102-106:⚠️ Potential issue | 🟠 MajorTimeout handling should switch status before parameter update.
In
_manage_querying_timeout, Line 102–106 updates order parameters first and only then switches to Processing. This violates the required transition order for querying flows.Proposed ordering fix
- context.order = set_phase(context.order, PhasesEnum.CHECK_CUSTOMER_ROLES) - context.order = update_order( - self.client, context.order_id, parameters=context.order["parameters"] - ) - switch_order_status_to_process(self.client, context, get_template_name(context)) + switch_order_status_to_process(self.client, context, get_template_name(context)) + context.order = set_phase(context.order, PhasesEnum.CHECK_CUSTOMER_ROLES) + context.order = update_order( + self.client, context.order_id, parameters=context.order["parameters"] + )Based on learnings: “order parameters cannot be updated while the order is in "querying" state… the order must first be switched to "processing" … and then the phase can be updated via a separate update_order call.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swo_aws_extension/processors/querying/aws_channel_handshake.py` around lines 102 - 106, In _manage_querying_timeout, change the call order so the order status is switched to Processing before attempting to update order parameters: call switch_order_status_to_process(self.client, context, get_template_name(context)) first, then call update_order(self.client, context.order_id, parameters=context.order["parameters"]); keep set_phase(context.order, PhasesEnum.CHECK_CUSTOMER_ROLES) as needed on the context but ensure the network update_order call happens after switch_order_status_to_process to avoid updating parameters while the order is still in "querying".
🧹 Nitpick comments (1)
tests/flows/test_order_utils.py (1)
116-170: Strengthen these tests to lock in “no implicit notification” behavior.These cases patch
MPTNotificationManagerbut do not consistently assertassert_not_called()in success paths after switching toswitch_order_status_to_process/switch_order_status_to_complete.Also applies to: 173-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/flows/test_order_utils.py` around lines 116 - 170, The tests patch MPTNotificationManager but only assert no notification on the error path; update the success-path tests for both switch_order_status_to_process and switch_order_status_to_complete to explicitly assert the patched notification manager was not called (call notification_mock.assert_not_called()) after invoking switch_order_status_to_process and switch_order_status_to_complete respectively so the "no implicit notification" behavior is locked in; locate the tests using the notification_mock variable and the calls to switch_order_status_to_process / switch_order_status_to_complete to add these assertions.
🤖 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/order_utils.py`:
- Line 86: There is trailing whitespace after a closing parenthesis on line
containing the lone ")" in the order_utils module; remove the trailing spaces so
the line contains only the closing parenthesis, then run ruff format (or ruff
--fix) to ensure the file conforms to the project linting rules; reference the
closing parenthesis in order_utils.py to locate and fix the whitespace.
---
Outside diff comments:
In `@swo_aws_extension/flows/order_utils.py`:
- Around line 105-126: The function switch_order_status_to_process currently
logs MPTError and returns None, silently swallowing failures; change it to
provide an explicit success/failure contract by returning True on success and
False on failure. In switch_order_status_to_process (which calls
set_order_template and process_order), wrap the process_order call in the
existing try/except but after logging the MPTError return False (or
alternatively re-raise if you prefer exceptions); on successful completion
return True. Update callers to check the boolean result so they do not assume
the order moved to Processing when process_order failed.
In `@swo_aws_extension/processors/querying/aws_channel_handshake.py`:
- Around line 102-106: In _manage_querying_timeout, change the call order so the
order status is switched to Processing before attempting to update order
parameters: call switch_order_status_to_process(self.client, context,
get_template_name(context)) first, then call update_order(self.client,
context.order_id, parameters=context.order["parameters"]); keep
set_phase(context.order, PhasesEnum.CHECK_CUSTOMER_ROLES) as needed on the
context but ensure the network update_order call happens after
switch_order_status_to_process to avoid updating parameters while the order is
still in "querying".
In `@swo_aws_extension/processors/querying/aws_customer_roles.py`:
- Around line 83-88: The current sequence calls set_phase and update_order while
the order is still in querying; change the flow to first call
switch_order_status_to_process(self.client, context, get_template_name(context))
to transition the order to processing, then perform a separate
set_phase(context.order, PhasesEnum.CREATE_SUBSCRIPTION) and follow that with
update_order(self.client, context.order_id,
parameters=context.order["parameters"]) so phase/parameter updates occur after
the state switch; update only the calls around set_phase, update_order, and
switch_order_status_to_process to reflect this new ordering.
---
Nitpick comments:
In `@tests/flows/test_order_utils.py`:
- Around line 116-170: The tests patch MPTNotificationManager but only assert no
notification on the error path; update the success-path tests for both
switch_order_status_to_process and switch_order_status_to_complete to explicitly
assert the patched notification manager was not called (call
notification_mock.assert_not_called()) after invoking
switch_order_status_to_process and switch_order_status_to_complete respectively
so the "no implicit notification" behavior is locked in; locate the tests using
the notification_mock variable and the calls to switch_order_status_to_process /
switch_order_status_to_complete to add these assertions.
🪄 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: a5fba1ab-e826-4e4b-b0d7-c9c71eee087d
📒 Files selected for processing (12)
swo_aws_extension/flows/order_utils.pyswo_aws_extension/flows/steps/base.pyswo_aws_extension/flows/steps/complete_order.pyswo_aws_extension/processors/querying/aws_billing_transfer_invitation.pyswo_aws_extension/processors/querying/aws_channel_handshake.pyswo_aws_extension/processors/querying/aws_customer_roles.pytests/flows/steps/test_base_phase_step.pytests/flows/steps/test_complete_order.pytests/flows/test_order_utils.pytests/processor/querying/test_aws_billing_transfer_invitation.pytests/processor/querying/test_aws_channel_handshake.pytests/processor/querying/test_aws_customer_roles.py
alephsur
force-pushed
the
MPT-19058-adopt-notification-templates-in-aws-extension
branch
from
064f610 to
6aa51a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
swo_aws_extension/processors/querying/aws_channel_handshake.py (1)
100-104:⚠️ Potential issue | 🔴 CriticalSwitch to
Processingbefore persisting the new phase.This timeout branch still calls
update_order()while the order isQuerying. That prevents timed-out handshakes from advancing toCHECK_CUSTOMER_ROLES. Moveswitch_order_status_to_process(...)ahead ofset_phase(...)andupdate_order(...).Based on learnings: order parameters cannot be updated while the order is in "querying" state; switch to "processing" first, then persist the phase with a separate `update_order` call.Suggested fix
- context.order = set_phase(context.order, PhasesEnum.CHECK_CUSTOMER_ROLES) - context.order = update_order( - self.client, context.order_id, parameters=context.order["parameters"] - ) - switch_order_status_to_process(self.client, context, get_template_name(context)) + switch_order_status_to_process(self.client, context, get_template_name(context)) + context.order = set_phase(context.order, PhasesEnum.CHECK_CUSTOMER_ROLES) + context.order = update_order( + self.client, context.order_id, parameters=context.order["parameters"] + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swo_aws_extension/processors/querying/aws_channel_handshake.py` around lines 100 - 104, The timeout branch currently updates order parameters while the order is still in the Querying state; move the call to switch_order_status_to_process(self.client, context, get_template_name(context)) so it executes before set_phase(context.order, PhasesEnum.CHECK_CUSTOMER_ROLES) and the subsequent update_order(...) call, ensuring the order status is switched to Processing first; then persist the new phase by calling set_phase(...) and update_order(self.client, context.order_id, parameters=context.order["parameters"]) afterwards as a separate operation.swo_aws_extension/flows/order_utils.py (1)
110-122:⚠️ Potential issue | 🟠 MajorDon't swallow
process_order()failures.This logs and returns, so callers leave the branch as if the order reached
Processingeven when the MPT transition failed. That silently strands the workflow inQueryinginstead of surfacing a retryable error.Suggested fix
try: context.order = process_order( client, context.order_id, **kwargs, ) - except MPTError as error: - logger.info( - "%s - Cannot switch order to 'Processing': %s", - context.order_id, - error, - ) - return + except MPTError: + logger.exception( + "%s - Cannot switch order to 'Processing'", + context.order_id, + ) + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swo_aws_extension/flows/order_utils.py` around lines 110 - 122, The current except block around process_order(client, context.order_id, **kwargs) catches MPTError, logs it, and returns which silently leaves the workflow in the wrong state; change the handler to log the error and then re-raise (or raise a more specific retryable exception) so the caller sees the failure and can retry or handle it. Update the except for MPTError in the order_utils.py block that assigns context.order to ensure the error is propagated (do not simply return after logger.info), e.g., log context.order_id and error via logger.info or logger.error and then raise the caught MPTError (or a wrapped exception) so the workflow does not proceed as if processing succeeded.
🤖 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/flows/test_order_utils.py`:
- Line 97: The test test_switch_order_to_failed currently passes a plain string
as the error to switch_order_status_to_failed; update the test to pass a
dict/object matching real error payloads (e.g., {"message": "...", "code":
"..."} or similar fields) so the call to switch_order_status_to_failed receives
an error-like structure; apply the same change to the other assertions in the
same test block (lines around 106-113) to use a dict error payload instead of a
plain string, ensuring the test exercises the real handling of error objects.
---
Outside diff comments:
In `@swo_aws_extension/flows/order_utils.py`:
- Around line 110-122: The current except block around process_order(client,
context.order_id, **kwargs) catches MPTError, logs it, and returns which
silently leaves the workflow in the wrong state; change the handler to log the
error and then re-raise (or raise a more specific retryable exception) so the
caller sees the failure and can retry or handle it. Update the except for
MPTError in the order_utils.py block that assigns context.order to ensure the
error is propagated (do not simply return after logger.info), e.g., log
context.order_id and error via logger.info or logger.error and then raise the
caught MPTError (or a wrapped exception) so the workflow does not proceed as if
processing succeeded.
In `@swo_aws_extension/processors/querying/aws_channel_handshake.py`:
- Around line 100-104: The timeout branch currently updates order parameters
while the order is still in the Querying state; move the call to
switch_order_status_to_process(self.client, context, get_template_name(context))
so it executes before set_phase(context.order, PhasesEnum.CHECK_CUSTOMER_ROLES)
and the subsequent update_order(...) call, ensuring the order status is switched
to Processing first; then persist the new phase by calling set_phase(...) and
update_order(self.client, context.order_id,
parameters=context.order["parameters"]) afterwards as a separate operation.
🪄 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: 54bfa88f-d87f-4290-af3c-37227b167abf
📒 Files selected for processing (12)
swo_aws_extension/flows/order_utils.pyswo_aws_extension/flows/steps/base.pyswo_aws_extension/flows/steps/complete_order.pyswo_aws_extension/processors/querying/aws_billing_transfer_invitation.pyswo_aws_extension/processors/querying/aws_channel_handshake.pyswo_aws_extension/processors/querying/aws_customer_roles.pytests/flows/steps/test_base_phase_step.pytests/flows/steps/test_complete_order.pytests/flows/test_order_utils.pytests/processor/querying/test_aws_billing_transfer_invitation.pytests/processor/querying/test_aws_channel_handshake.pytests/processor/querying/test_aws_customer_roles.py
🚧 Files skipped from review as they are similar to previous changes (2)
- swo_aws_extension/processors/querying/aws_billing_transfer_invitation.py
- swo_aws_extension/processors/querying/aws_customer_roles.py
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ruben-sebrango
approved these changes
Mar 17, 2026
albertsola
requested changes
Mar 17, 2026
Contributor
albertsola
left a comment
albertsola
left a comment
There was a problem hiding this comment.
added a command to the code
d3rky
force-pushed
the
MPT-19058-adopt-notification-templates-in-aws-extension
branch
from
6aa51a5 to
66ab7d0
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
swo_aws_extension/flows/order_utils.py (1)
110-123:⚠️ Potential issue | 🟠 MajorDo not swallow
MPTErrorwhen switching to Processing.On Line 116–122, the function logs and returns on failure, so callers cannot tell that the status transition failed. This can let downstream logic continue with phase/order updates under the wrong status.
Suggested fix
try: context.order = process_order( client, context.order_id, **kwargs, ) except MPTError as error: logger.info( "%s - Cannot switch order to 'Processing': %s", context.order_id, error, ) - return + raiseBased on learnings, transitioning from querying requires a successful switch to processing before further order updates (like phase changes) are performed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swo_aws_extension/flows/order_utils.py` around lines 110 - 123, The current try/except around process_order swallows MPTError (in the block setting context.order in order_utils), which prevents callers from knowing the transition to 'Processing' failed; modify the handler so the error is propagated after logging—either remove the except entirely or re-raise the caught MPTError after calling logger.info with context.order_id and error—so callers of process_order/context.order can detect and react to the failed status transition.
♻️ Duplicate comments (1)
tests/flows/test_order_utils.py (1)
97-113:⚠️ Potential issue | 🟡 MinorUse a dict error payload in
test_switch_order_to_failed.
switch_order_status_to_failedexpects a structured error object, but Line 106 still passes a plain string. This weakens contract coverage for the failed-order path.Suggested fix
def test_switch_order_to_failed(mocker, order_factory, fulfillment_parameters_factory): @@ - switch_order_status_to_failed(client, context, "Failure reason") # act + error = {"id": "FailureReason", "message": "Failure reason"} + switch_order_status_to_failed(client, context, error) # act @@ fail_order_mock.assert_called_with( client, context.order_id, - "Failure reason", + error, parameters=context.order["parameters"], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/flows/test_order_utils.py` around lines 97 - 113, The test passes a plain string to switch_order_status_to_failed but the function expects a structured error payload; update test_switch_order_to_failed to call switch_order_status_to_failed with a dict error object (e.g., {"message": "Failure reason"} or the project-standard error shape) and adjust the expected fail_order_mock.assert_called_with to expect parameters=context.order["parameters"] and the same dict error payload for the second argument; reference the PurchaseContext.from_order_data, switch_order_status_to_failed, and the fail_order mock to locate where to change the input and assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@swo_aws_extension/flows/order_utils.py`:
- Around line 110-123: The current try/except around process_order swallows
MPTError (in the block setting context.order in order_utils), which prevents
callers from knowing the transition to 'Processing' failed; modify the handler
so the error is propagated after logging—either remove the except entirely or
re-raise the caught MPTError after calling logger.info with context.order_id and
error—so callers of process_order/context.order can detect and react to the
failed status transition.
---
Duplicate comments:
In `@tests/flows/test_order_utils.py`:
- Around line 97-113: The test passes a plain string to
switch_order_status_to_failed but the function expects a structured error
payload; update test_switch_order_to_failed to call
switch_order_status_to_failed with a dict error object (e.g., {"message":
"Failure reason"} or the project-standard error shape) and adjust the expected
fail_order_mock.assert_called_with to expect
parameters=context.order["parameters"] and the same dict error payload for the
second argument; reference the PurchaseContext.from_order_data,
switch_order_status_to_failed, and the fail_order mock to locate where to change
the input and assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b17071d9-2e06-4d6f-a134-f4e8880ddecb
📒 Files selected for processing (12)
swo_aws_extension/flows/order_utils.pyswo_aws_extension/flows/steps/base.pyswo_aws_extension/flows/steps/complete_order.pyswo_aws_extension/processors/querying/aws_billing_transfer_invitation.pyswo_aws_extension/processors/querying/aws_channel_handshake.pyswo_aws_extension/processors/querying/aws_customer_roles.pytests/flows/steps/test_base_phase_step.pytests/flows/steps/test_complete_order.pytests/flows/test_order_utils.pytests/processor/querying/test_aws_billing_transfer_invitation.pytests/processor/querying/test_aws_channel_handshake.pytests/processor/querying/test_aws_customer_roles.py
🚧 Files skipped from review as they are similar to previous changes (5)
- swo_aws_extension/processors/querying/aws_customer_roles.py
- swo_aws_extension/processors/querying/aws_billing_transfer_invitation.py
- tests/flows/steps/test_complete_order.py
- swo_aws_extension/processors/querying/aws_channel_handshake.py
- tests/processor/querying/test_aws_billing_transfer_invitation.py
alephsur
force-pushed
the
MPT-19058-adopt-notification-templates-in-aws-extension
branch
from
66ab7d0 to
af8cb9f
Compare
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
swo_aws_extension/processors/querying/aws_customer_roles.py (1)
81-87:⚠️ Potential issue | 🔴 CriticalSwitch order status before
update_orderin timeout flow.Line 82 updates parameters while the order is still querying; that can fail and block the timeout transition. Move
switch_order_status_to_process(...)beforeset_phase/update_order.Based on learnings: In the swo-aws-extension codebase, order parameters cannot be updated while the order is in "querying" state—switch to "processing" first, then update phase via `update_order`.Proposed fix
logger.info( "%s - Updating order to processing with Phase CREATE_SUBSCRIPTION.", context.order_id, ) + switch_order_status_to_process(self.client, context, get_template_name(context)) context.order = set_phase(context.order, PhasesEnum.CREATE_SUBSCRIPTION) context.order = update_order( self.client, context.order_id, parameters=context.order["parameters"] ) - - switch_order_status_to_process(self.client, context, get_template_name(context))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swo_aws_extension/processors/querying/aws_customer_roles.py` around lines 81 - 87, The order parameters are being updated while the order may still be in the "querying" state; move the call to switch_order_status_to_process(self.client, context, get_template_name(context)) so it runs before set_phase(context.order, PhasesEnum.CREATE_SUBSCRIPTION) and before update_order(self.client, context.order_id, parameters=context.order["parameters"]); this ensures the order is transitioned to processing first and avoids failures when update_order tries to modify parameters while in querying state.swo_aws_extension/processors/querying/aws_channel_handshake.py (1)
100-104:⚠️ Potential issue | 🔴 CriticalReorder timeout operations: switch status before updating parameters.
Line 101 updates order parameters while still in querying state. This should be inverted to avoid invalid updates on timed-out querying orders.
Based on learnings: In swo_aws_extension/processors/querying/aws_channel_handshake.py for CHECK_CHANNEL_HANDSHAKE_STATUS transitions, status must be switched to processing before updating phase/order parameters.Proposed fix
logger.info( "%s - Updating order to processing with Phase CHECK_CUSTOMER_ROLES.", context.order_id, ) + switch_order_status_to_process(self.client, context, get_template_name(context)) context.order = set_phase(context.order, PhasesEnum.CHECK_CUSTOMER_ROLES) context.order = update_order( self.client, context.order_id, parameters=context.order["parameters"] ) - switch_order_status_to_process(self.client, context, get_template_name(context))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swo_aws_extension/processors/querying/aws_channel_handshake.py` around lines 100 - 104, The current sequence calls update_order(self.client, context.order_id, parameters=context.order["parameters"]) while still in querying state; move the call to switch_order_status_to_process(self.client, context, get_template_name(context)) to occur before update_order so the order status is switched to processing prior to updating phase parameters; keep the set_phase(context.order, PhasesEnum.CHECK_CUSTOMER_ROLES) as-is, then call switch_order_status_to_process(...) and only after that call update_order(...) with context.order["parameters"] to avoid updating timed-out querying orders.swo_aws_extension/flows/order_utils.py (1)
110-122:⚠️ Potential issue | 🟠 MajorDon't hide a failed switch to
Processing.This turns an MPT API failure into a silent no-op. In this workflow, the template/parameter update happens in a separate step and is only valid after the order has actually moved out of
Querying, so callers should get an explicit failure signal here instead of just a log line.Minimal fix
except MPTError as error: logger.info( "%s - Cannot switch order to 'Processing': %s", context.order_id, error, ) - return + raiseBased on learnings, "order parameters cannot be updated while the order is in
queryingstate ... the order must first be switched toprocessingstate viaswitch_order_status_to_process_and_notify, and then the phase can be updated via a separateupdate_ordercall."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swo_aws_extension/flows/order_utils.py` around lines 110 - 122, The current try/except around process_order swallows MPTError and returns silently; instead surface the failure so callers know the order did not switch to Processing. In the except block for MPTError raised by process_order (and any direct use of context.order assignment), log the error as before but then re-raise the exception (or raise a more specific error) so the workflow fails fast; ensure callers expecting the order to be in Processing (e.g., code that calls switch_order_status_to_process_and_notify and then update_order) will not proceed when process_order fails.
♻️ Duplicate comments (1)
tests/flows/test_order_utils.py (1)
107-113:⚠️ Potential issue | 🟡 MinorPass a real error payload in this test.
switch_order_status_to_failedstill takeserror: dict; using a plain string keeps the test out of sync with the production contract and can hide payload-shape issues.Suggested fix
- switch_order_status_to_failed(client, context, "Failure reason") # act + error = {"id": "FailureReason", "message": "Failure reason"} + switch_order_status_to_failed(client, context, error) # act fail_order_mock.assert_called_with( client, context.order_id, - "Failure reason", + error, parameters=context.order["parameters"], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/flows/test_order_utils.py` around lines 107 - 113, The test passes a string to switch_order_status_to_failed but the production function expects an error dict; update the call to pass a real error payload (e.g. error={"message": "Failure reason", "code": "ERR_FAIL"} or whatever the production contract requires) and adjust the fail_order_mock.assert_called_with third argument to match that dict (keep client, context.order_id and parameters=context.order["parameters"] unchanged); make sure the error shape matches the function signature used in switch_order_status_to_failed so the test validates payload shape.
🧹 Nitpick comments (2)
tests/processor/querying/test_aws_customer_roles.py (1)
136-160: Assert call order in timeout path to lock the querying→processing invariant.The timeout test checks all calls but not their order. Please assert that status switch happens before phase update/update_order so regressions are caught.
Based on learnings: In the swo-aws-extension codebase, order parameters cannot be updated while the order is in "querying" state—when transitioning to "processing", switch status first and only then update phase/order.Suggested assertion pattern
from unittest.mock import call assert mock_switch.call_count == 1 assert mock_set_phase.call_count == 1 assert mock_update_order.call_count == 1 # verify ordering using timestamps/order index from mock_calls combined = mock_switch.mock_calls + mock_set_phase.mock_calls + mock_update_order.mock_calls # or attach all three to a parent mock and compare expected call sequence🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/processor/querying/test_aws_customer_roles.py` around lines 136 - 160, Add assertions that verify the call order of switch_order_status_to_process, set_phase, and update_order in the timeout test: after invoking processor.process(mock_context) assert each mock (mock_switch, mock_set_phase, mock_update_order) was called once and then verify their ordering by inspecting mock_switch.mock_calls, mock_set_phase.mock_calls, and mock_update_order.mock_calls (or by aggregating them via mock_calls on a parent/combined mock) to ensure switch_order_status_to_process was called before set_phase and update_order; update the test around processor.process(...) to fail if the sequence is not switch → set_phase → update_order.tests/processor/querying/test_aws_channel_handshake.py (1)
131-162: Harden timeout test by asserting call order.Please assert that transition to processing occurs before phase mutation/order update in the timeout path, not just that all three were called.
Based on learnings: In this codebase, order parameters cannot be updated while in "querying" state, so status must switch to "processing" before phase/update_order operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/processor/querying/test_aws_channel_handshake.py` around lines 131 - 162, The timeout test must assert call order so status is switched to processing before phase mutation/order update; keep the existing call assertions for switch_order_status_to_process, set_phase, and update_order but also add a sequential assertion that mock_switch is called before mock_set_phase and mock_update_order. Replace or augment the separate asserts with an ordering check using the mocks' call history (e.g., assert_has_calls or comparing call indices on mock_switch, mock_set_phase, mock_update_order) so the sequence is: switch_order_status_to_process(...) then set_phase(...) then update_order(...); reference the functions switch_order_status_to_process, set_phase, update_order and the processor.process(mock_context) invocation to locate the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@swo_aws_extension/flows/order_utils.py`:
- Around line 110-122: The current try/except around process_order swallows
MPTError and returns silently; instead surface the failure so callers know the
order did not switch to Processing. In the except block for MPTError raised by
process_order (and any direct use of context.order assignment), log the error as
before but then re-raise the exception (or raise a more specific error) so the
workflow fails fast; ensure callers expecting the order to be in Processing
(e.g., code that calls switch_order_status_to_process_and_notify and then
update_order) will not proceed when process_order fails.
In `@swo_aws_extension/processors/querying/aws_channel_handshake.py`:
- Around line 100-104: The current sequence calls update_order(self.client,
context.order_id, parameters=context.order["parameters"]) while still in
querying state; move the call to switch_order_status_to_process(self.client,
context, get_template_name(context)) to occur before update_order so the order
status is switched to processing prior to updating phase parameters; keep the
set_phase(context.order, PhasesEnum.CHECK_CUSTOMER_ROLES) as-is, then call
switch_order_status_to_process(...) and only after that call update_order(...)
with context.order["parameters"] to avoid updating timed-out querying orders.
In `@swo_aws_extension/processors/querying/aws_customer_roles.py`:
- Around line 81-87: The order parameters are being updated while the order may
still be in the "querying" state; move the call to
switch_order_status_to_process(self.client, context, get_template_name(context))
so it runs before set_phase(context.order, PhasesEnum.CREATE_SUBSCRIPTION) and
before update_order(self.client, context.order_id,
parameters=context.order["parameters"]); this ensures the order is transitioned
to processing first and avoids failures when update_order tries to modify
parameters while in querying state.
---
Duplicate comments:
In `@tests/flows/test_order_utils.py`:
- Around line 107-113: The test passes a string to switch_order_status_to_failed
but the production function expects an error dict; update the call to pass a
real error payload (e.g. error={"message": "Failure reason", "code": "ERR_FAIL"}
or whatever the production contract requires) and adjust the
fail_order_mock.assert_called_with third argument to match that dict (keep
client, context.order_id and parameters=context.order["parameters"] unchanged);
make sure the error shape matches the function signature used in
switch_order_status_to_failed so the test validates payload shape.
---
Nitpick comments:
In `@tests/processor/querying/test_aws_channel_handshake.py`:
- Around line 131-162: The timeout test must assert call order so status is
switched to processing before phase mutation/order update; keep the existing
call assertions for switch_order_status_to_process, set_phase, and update_order
but also add a sequential assertion that mock_switch is called before
mock_set_phase and mock_update_order. Replace or augment the separate asserts
with an ordering check using the mocks' call history (e.g., assert_has_calls or
comparing call indices on mock_switch, mock_set_phase, mock_update_order) so the
sequence is: switch_order_status_to_process(...) then set_phase(...) then
update_order(...); reference the functions switch_order_status_to_process,
set_phase, update_order and the processor.process(mock_context) invocation to
locate the test.
In `@tests/processor/querying/test_aws_customer_roles.py`:
- Around line 136-160: Add assertions that verify the call order of
switch_order_status_to_process, set_phase, and update_order in the timeout test:
after invoking processor.process(mock_context) assert each mock (mock_switch,
mock_set_phase, mock_update_order) was called once and then verify their
ordering by inspecting mock_switch.mock_calls, mock_set_phase.mock_calls, and
mock_update_order.mock_calls (or by aggregating them via mock_calls on a
parent/combined mock) to ensure switch_order_status_to_process was called before
set_phase and update_order; update the test around processor.process(...) to
fail if the sequence is not switch → set_phase → update_order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b1d7aee3-ce59-4da9-a986-edf576e28359
📒 Files selected for processing (12)
swo_aws_extension/flows/order_utils.pyswo_aws_extension/flows/steps/base.pyswo_aws_extension/flows/steps/complete_order.pyswo_aws_extension/processors/querying/aws_billing_transfer_invitation.pyswo_aws_extension/processors/querying/aws_channel_handshake.pyswo_aws_extension/processors/querying/aws_customer_roles.pytests/flows/steps/test_base_phase_step.pytests/flows/steps/test_complete_order.pytests/flows/test_order_utils.pytests/processor/querying/test_aws_billing_transfer_invitation.pytests/processor/querying/test_aws_channel_handshake.pytests/processor/querying/test_aws_customer_roles.py
🚧 Files skipped from review as they are similar to previous changes (2)
- swo_aws_extension/flows/steps/base.py
- tests/flows/steps/test_base_phase_step.py
d3rky
approved these changes
Mar 18, 2026
d3rky
deleted the
MPT-19058-adopt-notification-templates-in-aws-extension
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
4 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-19058
Release Notes
_and_notifysuffix: