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

Intake | Bug | Use saved withdrawn issue IDs when processing asynchronously #14116

Merged
merged 6 commits into from
May 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/models/request_issues_update.rb
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ def process_withdrawn_issues!
def withdrawal
@withdrawal ||= RequestIssueWithdrawal.new(
user: user,
review: review,
request_issues_update: self,
request_issues_data: @request_issues_data
)
end
Expand Down
13 changes: 9 additions & 4 deletions app/workflows/request_issue_withdrawal.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
# frozen_string_literal: true

class RequestIssueWithdrawal
def initialize(user:, review:, request_issues_data:)
def initialize(user:, request_issues_update:, request_issues_data:)
@user = user
@review = review
@request_issues_update = request_issues_update
@request_issues_data = request_issues_data
Copy link
Contributor

@nanotone nanotone May 7, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@leikkisa leikkisa May 12, 2020

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

end

Expand All @@ -15,12 +15,17 @@ def call
end

def withdrawn_issues
@withdrawn_issues ||= calculate_withdrawn_issues
@withdrawn_issues ||= withdrawn_request_issue_ids ? fetch_withdrawn_issues : calculate_withdrawn_issues
end

private

attr_reader :user, :review, :request_issues_data
attr_reader :user, :request_issues_update, :request_issues_data
delegate :review, :withdrawn_request_issue_ids, to: :request_issues_update

def fetch_withdrawn_issues
RequestIssue.where(id: withdrawn_request_issue_ids)
end

def calculate_withdrawn_issues
withdrawn_issue_data.map do |issue_data|
Expand Down
47 changes: 34 additions & 13 deletions spec/models/request_issues_update_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ def allow_update_contention
expect(rating_end_product_establishment.reload.synced_status).to eq(nil)
end

context "when an issue is withdrawn" do
context "when an issue is withdrawn and there are active tasks" do
let(:request_issues_data) do
[{ request_issue_id: existing_legacy_opt_in_request_issue.id, withdrawal_date: Time.zone.now }]
end
Expand Down Expand Up @@ -595,28 +595,45 @@ def allow_update_contention
end
end
end
end

context "when create_contentions raises VBMS service error" do
let(:request_issues_data) { request_issues_data_with_new_issue }
context "when remove_contention raises VBMS service error and is re-tried" do
let(:request_issues_data) do
[{ request_issue_id: existing_legacy_opt_in_request_issue.id, withdrawal_date: Time.zone.now }]
end

it "saves error message and logs error" do
capture_raven_log
raise_error_on_create_contentions
it "saves error message, logs error and removes contention on re-attempt" do
capture_raven_log
raise_error_on_remove_contention

subject
subject

expect(request_issues_update.error).to eq(vbms_error.inspect)
expect(@raven_called).to eq(true)
expect(request_issues_update.error).to eq(vbms_error.inspect)
expect(@raven_called).to eq(true)

withdrawn_issue = request_issues_update.withdrawn_issues.first

expect(withdrawn_issue).to_not be_nil
expect(withdrawn_issue).to have_attributes(
closed_status: "withdrawn",
closed_at: Time.zone.now,
contention_removed_at: nil
)

allow_remove_contention
DecisionReviewProcessJob.perform_now(request_issues_update)

expect(request_issues_update.processed_at).to eq Time.zone.now
expect(withdrawn_issue.reload.contention_removed_at).to eq Time.zone.now
end
end
end

context "when remove_contention raises VBMS service error" do
let(:request_issues_data) { [{ request_issue_id: existing_request_issue.id }] }
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
Comment on lines +631 to +636
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


subject

Expand Down Expand Up @@ -681,6 +698,10 @@ def allow_create_contentions
def raise_error_on_remove_contention
allow(Fakes::VBMSService).to receive(:remove_contention!).and_raise(vbms_error)
end

def allow_remove_contention
allow(Fakes::VBMSService).to receive(:remove_contention!).and_call_original
end
end
end

Expand Down