Intake | Bug | Use saved withdrawn issue IDs when processing asynchronously #14116
Conversation
|
Code Climate has analyzed commit c6e9fad and detected 0 issues on this pull request. View more on Code Climate. |
…ent-of-veterans-affairs/caseflow into sally/13483-async-withdrawal-bug
| context "when create_contentions raises VBMS service error" do | ||
| let(:request_issues_data) { request_issues_data_with_new_issue } | ||
|
|
||
| it "saves error message and logs error" do | ||
| capture_raven_log | ||
| raise_error_on_remove_contention | ||
| raise_error_on_create_contentions |
There was a problem hiding this comment.
I think this is GitHub weirdness - this create_contentions test already existed, and there was a similar one for remove_contention that I moved to the withdrawn issues context.
leikkisa
changed the title
| @user = user | ||
| @review = review | ||
| @request_issues_update = request_issues_update | ||
| @request_issues_data = request_issues_data |
There was a problem hiding this comment.
I'm satisfied that this PR fixes the bug of RIUs misbehaving when performed asynchronously, but would like to see this code hardened a bit more and made less bug-prone for future engineers who might venture here. Hopefully I can accurately describe my concerns here, although I'm not sure how to address them yet.
The main problem is that request_issues_data is being passed here from the RIU, but RIU's @request_issues_data is an ephemeral attribute set by controllers during HTTP requests, and presumably nil during asynchronous jobs.
I see that delegation to RIU's withdrawn_request_issue_ids is now being used to determine when not to rely on calculate_withdrawn_issues ➡️ withdrawn_issue_data ➡️ request_issues_data, but passing in an unreliable request_issues_data in the first place feels like the root problem. Consequently, this whole group of methods will be unreliable at job time, yet nothing in the code makes that obvious.
Assuming my understanding is correct (lemme know if not!) I'll spend a little more effort thinking about a way to address this.
There was a problem hiding this comment.
And now, I'm actually wondering if this PR actually handles all edge cases of the bug. If you look at the call method, which is the point of entry when actually performing the withdrawal, it refers to not only withdrawn_issues but also withdrawn_issue_data to get a withdrawal date.
There was a problem hiding this comment.
Hey @nanotone , the withdraw! part isn't failing because it's happening in the Caseflow code. It's kind of hard to see in this PR but the part that's getting retried is in RequestIssuesUpdate.establish! where removed_or_withdrawn_issues are having their contentions removed in order to consider the RequestIssuesUpdate processed.
The RequestIssuesUpdate delegates the withdrawn issues to RequestIssueWithdrawal, and when it's attempting that asynchronously later, it's not fetching the withdrawn request issues.
There was a problem hiding this comment.
@nanotone I started working on the refactor that we discussed, but ran into something that would makes it bigger than I had realized. The refactor would also impact RequestIssueCorrections which is a more complex workflow. So to proceed, I'd need to refactor that one too.
How would you feel if I added a separate refactor ticket instead? I think it would benefit from more eyes on the overall architecture and how the controller is being used.
In the short term... I could update RequestIssueWithdrawal to be closer to RequestIssueCorrection (pass in the withdrawn issue IDs instead of the request issues update).
One potential option for the longer term - we could consider using workflows for all of them, but having those set up to operate strictly on the params, and move things that depend on the issue IDs back over to RequestIssuesUpdate.
There was a problem hiding this comment.
Making a separate ticket sounds good! Don't let the perfect be the enemy of the good, etc etc.
I didn't realize that RIW and RIC were intended to be parallel workflows, with very similar methods. It does seem like this refactor will need a bit more thought.
leikkisa
added
the
Ready-to-Merge
connects #13483
Description
This adds the ability for request issue withdrawals to fetch withdrawn issues from their IDs saved in the database on the request issues update. This is needed if they are being accessed asynchronously outside of the original update that was submitted which may happen if the VBMS
remove_contentioncall fails on the first attempt.There is a related manual data fix GitHub issue to look for more instances of this and re-attempt processing for them.