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

Expand the only_open_task_of_type validation to AttorneyTask and JudgeDecisionReviewTask #16586

Merged
merged 29 commits into from
Aug 16, 2021

Conversation

eileen-nava
Copy link
Contributor

@eileen-nava eileen-nava commented Aug 3, 2021

Resolves jira ticket Research and prevent recurrence of bug where one case has multiple JudgeAssignTasks, JudgeDecisionReviewTasks, and AttorneyTasks .
Relates to github PR Write feature test to replicate scenario with multiple Judge/Attorney tasks.

Description

A bug occurred in production where a user created multiple Judge and Attorney tasks by doing the same actions in two different tabs, as described in this comment from Yoom.
The JudgeAssignTask model already had a validation to ensure that an appeal cannot have more than one JudgeAssignTask open at the same time. I expanded this validation to the JudgeDecisionReviewTask and AttorneyTask models. I also refactored the validation to use instance variables instead of thread-local variables, as suggested in this StackOverflow post that Yoom linked in a previous thread.

Acceptance Criteria

  • Code compiles correctly
  • All automated tests pass
  • Task validations no longer use thread-local variables
  • If a user attempts to act on the same JudgeAssignTask in two different windows, they will not be able to create multiple JudgeDecisionReviewTasks and AttorneyTasks.

Testing Plan

[ ] Find an appeal with an open JudgeAssignTask

jat = JudgeAssignTask.open.last
appeal = jat.appeal

[ ] On local caseflow, switch users to the judge who has the JudgeAssignTask assigned to them
[ ] Navigate to the case details page for the relevant appeal. Open the same page in a second tab
/queue/appeals/${appeal.uuid}
[ ] In the first tab, reassign the JudgeAssignTask to another judge
[ ] In the second tab, attempt to complete the JudgeAssignTask by assigning the case to an attorney. You should not be able to complete this action

app/models/task.rb Outdated Show resolved Hide resolved
@codeclimate
Copy link

codeclimate bot commented Aug 3, 2021

Code Climate has analyzed commit 2a0d120 and detected 0 issues on this pull request.

View more on Code Climate.

@eileen-nava eileen-nava changed the title WIP: 2075/limit jdr atty tasks WIP: Expand the only_open_task_of_type validation to AttorneyTask and JudgeDecisionReviewTask Aug 3, 2021
@eileen-nava eileen-nava changed the title WIP: Expand the only_open_task_of_type validation to AttorneyTask and JudgeDecisionReviewTask Expand the only_open_task_of_type validation to AttorneyTask and JudgeDecisionReviewTask Aug 9, 2021
@eileen-nava
Copy link
Contributor Author

Via slack, Yoom had encouraged me to answer the below question on my own:

When you verbally explained this bug to me and showed its timeline in logs, you noted that one JudgeAssignTask changed status from cancelled to completed, which is an invalid flow. Do you want me to write code to ensure that this change in status is impossible? Or are the expanded validations sufficient to prevent the bug from recurring?

My understanding is that when a judge completes a JudgeAssignTask, this creates an AttorneyTask. If a judge reassigns a JudgeAssignTask in one tab and then completes the now-cancelled JudgeAssignTask in a second tab, the action in the second tab will attempt to create a second AttorneyTask. Due to the new validation on AttorneyTask, that action will now fail, and the JudgeAssignTask won’t change status from cancelled to completed.

@eileen-nava eileen-nava marked this pull request as ready for review August 9, 2021 23:23
@eileen-nava
Copy link
Contributor Author

@yoomlam PR ready for review. TY!


private

def only_open_task_of_type
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 felt odd even as I typed it, but it was necessary to maintain the docket switch flow. Maybe we should check with the board that we want to allow multiple DocketSwitchDeniedTask and DocketSwitchGrantedTask instances for one appeal?

Copy link
Contributor

Choose a reason for hiding this comment

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

@eileen-nava Yes, check with Foxtrot and @araposo-tistatech first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ann-Marie directed me to Foxtrot. Slack response from Yang:

I believe there should only ever be two open DocketSwitchAbstractAttorneyTasks on an appeal, parent assigned to COTB and child assigned to a user.

Yang also noted that @jcq created this task tree structure. I'll check with JC about this when he is back from PTO.

For now, overriding the validation seems fine.

Comment on lines +236 to +237
expect(page).to have_content("Looks like this appeal already has an open JudgeDecisionReviewTask "\
"and this action cannot be completed.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change this error message to:

Suggested change
expect(page).to have_content("Looks like this appeal already has an open JudgeDecisionReviewTask "\
"and this action cannot be completed.")
expect(page).to have_content("This appeal already has an open JudgeDecisionReviewTask "\
"and this action cannot be completed.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current error message was suggested by design in a slack conversation with @mbaenatan and @rutvigupta-design. I'd appreciate their thoughts on any changes.

spec/models/appeal_spec.rb Outdated Show resolved Hide resolved
spec/models/appeal_spec.rb Outdated Show resolved Hide resolved
spec/models/appeal_spec.rb Outdated Show resolved Hide resolved
spec/models/appeal_spec.rb Outdated Show resolved Hide resolved
spec/models/appeal_spec.rb Outdated Show resolved Hide resolved
spec/models/appeal_spec.rb Outdated Show resolved Hide resolved
@@ -919,7 +935,7 @@
end

it "should know the right assigned attorney with a cancelled task" do
task2.update(status: "cancelled")
appeal.tasks.open.of_type("AttorneyTask").last.update!(status: Constants.TASK_STATUSES.cancelled)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
appeal.tasks.open.of_type("AttorneyTask").last.update!(status: Constants.TASK_STATUSES.cancelled)
task2.cancelled!

Copy link
Contributor

@yoomlam yoomlam left a comment

Choose a reason for hiding this comment

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

Minor suggestions.
Very nice work! A side-effect is that now, our tests are more realistic (only 1 open task of certain type at a time).

@yoomlam
Copy link
Contributor

yoomlam commented Aug 10, 2021

Related feature test: PR #16582

Copy link
Contributor

@yoomlam yoomlam left a comment

Choose a reason for hiding this comment

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

🥇

@eileen-nava eileen-nava added the Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master label Aug 16, 2021
@va-bot va-bot merged commit bf38fcf into master Aug 16, 2021
@va-bot va-bot deleted the 2075/limit-jdr-atty-tasks branch August 16, 2021 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants