Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cancellation as claim note through VRO's svc-bgs-api #2315

Merged
merged 6 commits into from
Dec 12, 2023

Conversation

dfitchett
Copy link
Contributor

What was the problem?

EP Merge needs to add a claim note to the EP400 upon canceling the claim.

Associated tickets or Slack threads:

How does this fix it?1

Adds step in state machine to add a claim note with the cancellation reason to the EP400 claim.

How to test this PR

  • pull code
  • from /domain-ee/ee-ep-merge-app run pytest
  • from /domain-ee/ee-ep-merge-app run pytest ./integration
  • run this CI action to verify successful integration test

Footnotes

  1. Pull-Requests guidelines. If PR is significant, update Current Software State wiki page.

@dfitchett dfitchett requested review from a team as code owners December 8, 2023 00:12
@@ -62,18 +62,6 @@ jobs:
# Quit after 60 seconds
retries: 30

# Temporary step added to avoid condition where bipApiExchange is not yet created.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer necessary as the integration tests declare the bipApiExchange

Base automatically changed from dfitchett/2300/bgs-updates to develop December 11, 2023 16:43
Copy link
Contributor

github-actions bot commented Dec 11, 2023

Test Results

144 tests  +144   144 ✔️ +144   46s ⏱️ +46s
  39 suites +  39       0 💤 ±    0 
  39 files   +  39       0 ±    0 

Results for commit 9789ad1. ± Comparison against base commit 49d9a71.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@nanotone nanotone left a comment

Choose a reason for hiding this comment

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

Just some tiny non-blocking nits! LGTM

f"error=\'{error}\'")
self.job.error(self.job.state, error)
f"error=\"{jsonable_encoder(error)}\"")
self.job.error(error if type(error) is list else [error])
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer isinstance(error, list) over type(error) is list to support subclasses.

self.state = JobState.COMPLETED_ERROR
if self.messages is None:
self.messages = []
self.messages.append(message)
for message in messages:
self.messages.append(message)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be self.messages.extend(messages)

@@ -52,4 +65,6 @@ class ClientName(str, Enum):
os.environ.get("CREATE_CLAIM_CONTENTIONS_RESPONSE") or "createClaimContentionsResponseQueue",
ClientName.CANCEL_CLAIM:
os.environ.get("CANCEL_CLAIM_RESPONSE") or "cancelClaimResponseQueue",
ClientName.BGS_ADD_CLAIM_NOTE:
os.environ.get("ADD_CLAIM_NOTE_RESPONSE") or "add-note-response",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it too late to switch these to camel-case? 😅

@@ -48,8 +56,10 @@ class EpMergeMachine(StateMachine):
| running_merge_contentions.to(completed_error, cond="has_error")
| running_move_contentions_to_pending_claim.to(running_cancel_ep400_claim, unless="has_error")
| running_move_contentions_to_pending_claim.to(completed_error, cond="has_error")
| running_cancel_ep400_claim.to(completed_success, unless="has_error")
| running_cancel_ep400_claim.to(running_add_claim_note_to_ep400, unless="has_error")
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the plan was to add a claim note when merging failed, not on success?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. The plan is to add a claim note as part of the happy path. See #2161. The claim note is being added after the claim is cancelled that way the cancellation reason for the claim will show in VBMS. 👍

Copy link
Contributor

JaCoCo Test Coverage

There is no coverage information present for the Files changed

Total Project Coverage 76.73%

@dfitchett dfitchett merged commit 21c7f20 into develop Dec 12, 2023
1 check passed
@dfitchett dfitchett deleted the dfitchett/2161/ep-merge-add-claim-note branch December 12, 2023 18:29
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.

5 participants