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

Replicate prod bug: Case Movement user cannot reassign task #16206

Merged
merged 7 commits into from May 6, 2021

Conversation

yoomlam
Copy link
Contributor

@yoomlam yoomlam commented May 5, 2021

In response to an issue from Mike Sanford: Cannot Assign Task to Any Attorney #187.

Description

Add test for replicating a bug in production where a Case Movement user cannot reassign task during Quality Review.
This will help with fixing #16205.

Acceptance Criteria

  • Code compiles correctly
  • No PII exist in spec/records/scm-cant-reassign.json

Testing Plan

Add a binding.pry and run the test in spec/fixes/investigate_scm_cant_reassign_spec.rb

@yoomlam yoomlam self-assigned this May 5, 2021
@yoomlam yoomlam added Stakeholder: BVA Functionality associated with the Board of Veterans' Appeals workflows/feature requests Team: Echo 🐬 Type: Bug labels May 5, 2021
@va-bot
Copy link
Collaborator

va-bot commented May 5, 2021

1 Warning
⚠️ This is a Big PR. Try to break this down if possible. Stacked pull requests encourage more detailed and thorough code reviews

Generated by 🚫 Danger

@codeclimate
Copy link

codeclimate bot commented May 5, 2021

Code Climate has analyzed commit 95fddf8 and detected 0 issues on this pull request.

View more on Code Climate.

Comment on lines +53 to +66
JudgeCaseReview => {
sanitize_fields: %w[comment],
retrieval: lambda do |records|
judge_task_ids = Task.where(type: JudgeTask.descendants.map(&:name), appeal: records[Appeal]).ids
JudgeCaseReview.where(task_id: judge_task_ids).order(:id)
end
},
AttorneyCaseReview => {
sanitize_fields: %w[comment],
retrieval: lambda do |records|
atty_task_ids = Task.where(type: AttorneyTask.descendants.map(&:name), appeal: records[Appeal]).ids
AttorneyCaseReview.where(task_id: atty_task_ids).order(:id)
end
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reordering so that these CaseReviews are exported after Tasks because they rely on tasks.

@@ -276,7 +276,7 @@ def id_offset
# Start with important types that other records will reassociate with
def first_types_to_import
# HearingDay is needed by Hearing
@first_types_to_import ||= [Appeal, Organization, User, HearingDay]
@first_types_to_import ||= [Appeal, Organization, User, HearingDay, Task]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensure Tasks are one of the types imported before other records, i.e. CaseReviews, because CaseReviews rely on Tasks existing in the DB.

Comment on lines +151 to +152
elsif obj_hash[field_name].is_a?(String)
obj_hash[field_name] = (obj_hash[field_name].to_i + @id_offset).to_s
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CaseReview records use strings for task_id to refer to legacy appeals, so we need to handle strings. This importer currently only supports AMA appeals so these strings can be converted to integers without error.

@eileen-nava
Copy link
Contributor

eileen-nava commented May 6, 2021

Hi Yoom, I tried to test this PR but I think I am missing a step. Could you please clarify?

First, I ran my app locally and logged in as a user with Special Case Movement roles. Then, I navigated to an appeal that was at the quality review phase and reassigned the case to an attorney. I didn't see an error banner or an error message in dev console when I took these steps. Below, I included a screen recording of the process.

TestYoomSCMPRNotInSpec.mov

If it's helpful, here is the task tree of the appeal that I worked with locally.

TestYoomSCMPRTaskTree

Then, I had a facepalm moment when I realized that uncommenting the binding.pry breakpoint wouldn't do anything unless I actually ran the test. I ran the test and assumed that it would open the app, after which I could replicate the error. However, the app didn't open when I ran the spec. At this point I realized that I had wrongly assumed the test you wrote was a feature test. Below, I included a screen recording of this process.

TestYoomSCMPRInTest.mov

Could you please clarify how I should test this PR and reproduce the "Error assigning tasks" error? Thanks!

Copy link
Contributor

@eileen-nava eileen-nava left a comment

Choose a reason for hiding this comment

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

I ran the test and received the desired error message. This looks good to me! 👍 I realize the build is currently failing on lint, but I am still approving it because I know the build will pass after you merge in the upgrade to Rails 5.2.4.6.

@yoomlam yoomlam 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 May 6, 2021
@va-bot va-bot merged commit d2f30a0 into master May 6, 2021
@va-bot va-bot deleted the yoom/replicate-scm-cant-reassign branch May 6, 2021 19:00
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 Stakeholder: BVA Functionality associated with the Board of Veterans' Appeals workflows/feature requests Team: Echo 🐬 Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants