MCP-2581 and MCP-2582 Offramp in Error Scenarios #1353
Conversation
JaCoCo Test Coverage
|
hayleymdawson
changed the title
| @@ -157,6 +157,13 @@ public void setOffRampReason(Claim claimWithOffRamp) { | |||
| Optional<ClaimSubmissionEntity> claimSubmission = | |||
| claimSubmissionRepository.findFirstByReferenceIdAndIdTypeOrderByCreatedAtDesc( | |||
| claimWithOffRamp.getCollectionId(), claimWithOffRamp.getIdType()); | |||
| if (claimSubmission.isEmpty()) { | |||
| log.info( | |||
There was a problem hiding this comment.
Before Yoom asks, yes this is info not error. There are cases we offramp and this doesn't exist yet. When i collapsed the MasIntegration routes for offramping I found that out.
| MasCompletionStatus completionStatus = MasCompletionStatus.of(origin, sufficient); | ||
| String offRampError = exchange.getProperty("offRampError", String.class); | ||
| // Update our database with offramp reason. | ||
| if (offRampError != null) { |
There was a problem hiding this comment.
OffRampError is our key here on properties to know if we're offramping or not.
| .to(ENDPOINT_ACCESS_ERR) | ||
| .setProperty("offRampError", constant("Sufficiency cannot be determined.")) | ||
| .setProperty("sourceRoute", constant("assessorError")) | ||
| .log("Assessor Error. Off-ramping claim") |
There was a problem hiding this comment.
ENDPOINT_ACCESS_ERR was very close to what I needed from ENDPOINT_OFFRAMP_ERROR and in the interest of not causing too many camel routes and making the code harder, it's been collapsed and what wasn't contained moved here.
| .setProperty("offRampError", constant(pdfFailError)) | ||
| .setProperty("sourceRoute", constant(rfdRouteId)) | ||
| .to(ENDPOINT_OFFRAMP_ERROR) | ||
| .stop() |
There was a problem hiding this comment.
stop is now explicit to ensure that the last masComplete after the catch doesn't run.
There was a problem hiding this comment.
This should also allow future ENDPOINT_MAS_COMPLETE error handling to stand independently of this try catch as needed.
| // Wiretap does NOT let camel work as expected when placed directly inside doCatch() | ||
| // Thus it is broken out here, in the interest of letting normal flow/control happen. | ||
| from(ENDPOINT_OFFRAMP_ERROR) | ||
| .wireTap(ENDPOINT_NOTIFY_AUDIT) // Send error notification to slack |
There was a problem hiding this comment.
I realize I often call stop() explicitly rather than relying on this strange behavior. I think that would be easier for devs to follow in the future. And if camel ever makes that work the way that it seems it should.
There was a problem hiding this comment.
Just to clarify, the right call would be wireTap but due to the redhat issue, we should use stop() in the meantime?
There was a problem hiding this comment.
Not quite but I will sync with you in a huddle to explain. :) easier i think
| .doCatch(BipException.class) | ||
| // Mas Complete Processing code expects this to be the body of the message | ||
| .setBody(simple("${exchangeProperty.payload}")) | ||
| // Wiretap will cause no code to execute after the end of the try. Intentional here. |
There was a problem hiding this comment.
I wanted to not rely on the wiretap side-effect behavior here which stops processing, but i'd have to make an entirely new route just to contain the wiretap in a non offramp scenario and somehow pass the onprepare the right variables differently.
Due to time I commented that I am relying on the strange behavior here. (This case execution did not change in this ticket, i just made endDoCatch() end which is more correct in this case even with wiretap involved).
hayleymdawson
requested review from
au2,
yoomlam,
alan-amida,
anthonyvelazquez,
ianwolson and
wlin2019
There was a problem hiding this comment.
I personally don't see anything objectionable here, but I also don't know what's in the businesses' head like some others do. The code looks sound. I'll let others comment on design.
Also this is the thing Hayley is talking about with respect to Camel wiretap causing doCatch()s to fail:
https://access.redhat.com/solutions/6749521
You need an account to view the whole thing, but otherwise, it's there, it's known, and RedHat are the only ones so far who have an article about it. Good find, @hayleymdawson
anthonyvelazquez
requested review from
anthonyvelazquez
and removed request for
anthonyvelazquez
What was the problem?
Claims were stuck in the following two error cases
Associated tickets or Slack threads:
How does this fix it?
Claims are now offramped in the above two cases. (See MCP tickets or github issue)
How to test this PR
Run End to End Tests
I have left some review comments on this one, because there's some things that other devs probably care about.