Prevent duplicate contention migration (fix #2054) #2145
Conversation
raudabaugh
force-pushed
the
iss2054-migrate-logic
branch
from
9c39ebb to
a48b1f2
Compare
raudabaugh
force-pushed
the
iss2054-migrate-logic
branch
from
7299308 to
47311fb
Compare
...e-ep-merge-app/tests/responses/get_ep400_claim_contentions_increase_multicontention_200.json
Outdated
Show resolved
Hide resolved
| @@ -78,6 +79,8 @@ def on_get_ep400_contentions(self, pending_contentions=None): | |||
|
|
|||
| @running_set_temp_station_of_jurisdiction.enter | |||
| def on_set_temp_station_of_jurisdiction(self, pending_contentions=None, ep400_contentions=None): | |||
There was a problem hiding this comment.
So on line 89 pending_contentions and ep400_contentions are both added to the self.process(...) function as kwargs. So they are actually available for you to use in the function signature of is_duplicate as kwargs: is_duplicate(self, pending_contentions, ep400_contentions). So you dont actually need to add them to the MergeJob class. 🪄 🧙
There was a problem hiding this comment.
I was thinking those might be useful to have for restarts/recovery when we get to converting the job store to RDS or some persistent backend. But we could definitely leave those out until we get to that point - thoughts?
There was a problem hiding this comment.
Noted! I'd say for now, lets just leave them out! We can have another conversation when it comes to the data we should/can/can't store in RDS!
| @@ -23,6 +25,8 @@ class MergeJob(BaseModel): | |||
| state: JobState = JobState.PENDING | |||
| error_state: JobState | None = None | |||
| messages: list[Any] | None = None | |||
| pending_contentions: list[ContentionSummary] | None = None | |||
There was a problem hiding this comment.
These additions are unnecessary. See comment on ep_merge_machine.py 👀
domain-ee/ee-ep-merge-app/src/python_src/service/ep_merge_machine.py
Outdated
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| try: | ||
| return [*pending_contentions.contentions, *ContentionsUtil.new_contentions(pending_contentions.contentions, ep400_contentions.contentions)] | ||
| except CompareException: | ||
| raise |
There was a problem hiding this comment.
Is there any reason to catch this CompareException and re-raise the original? It seems easier to just leave out the try, except, and raise lines.
raudabaugh
force-pushed
the
iss2054-migrate-logic
branch
from
afe407b to
e48c638
Compare
raudabaugh
force-pushed
the
iss2054-migrate-logic
branch
from
e48c638 to
33b8913
Compare
raudabaugh
merged commit ad0c62d
into
department-of-veterans-affairs:develop
What was the problem?
The merge app did not check if an EP400 contention already existed in the pending claim before migrating it.
Associated tickets or Slack threads:
How does this fix it?1
The merge job skips migrating a contention if there is already one in the pending claim with the same
claimantTextandcontentionTypeCode.How to test this PR
abd-vrodirectory, run the unit tests:./gradlew :domain-ee:testFootnotes
Pull-Requests guidelines. If PR is significant, update Current Software State wiki page. ↩