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

Fix dislocated parent tasks in FactoryBot - Part 2 #13605

Conversation

yoomlam
Copy link
Contributor

@yoomlam yoomlam commented Mar 4, 2020

Connects #13130
Part 1 #13604

Description

Second, collect and group all AMA tasks together under a common parent factory, in preparation for next part.

Acceptance Criteria

  • All tests pass

@yoomlam yoomlam self-assigned this Mar 4, 2020
@yoomlam yoomlam changed the base branch from master to yoom/13130.1-factoryBot_dislocated_parent_tasks March 4, 2020 13:43
@codeclimate
Copy link

codeclimate bot commented Mar 4, 2020

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

View more on Code Climate.

factory :change_hearing_disposition_task, class: ChangeHearingDispositionTask do
type { ChangeHearingDispositionTask.name }
assigned_to { HearingAdmin.singleton }
factory :ama_task_base_factory do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New parent factory to collect all AMA tasks.

@va-bot
Copy link
Collaborator

va-bot commented Mar 4, 2020

1 Warning
⚠️ This is a Big PR. Try to break this down if possible.

Generated by 🚫 Danger

Comment on lines 239 to 248
factory :appeal_task, class: DecisionReviewTask do
type { DecisionReviewTask.name }
appeal { create(:appeal, benefit_type: "education") }
assigned_by { nil }
end

factory :assign_hearing_disposition_task, class: AssignHearingDispositionTask do
type { AssignHearingDispositionTask.name }
assigned_to { Bva.singleton }
appeal
parent { create(:hearing_task) }
factory :higher_level_review_task, class: DecisionReviewTask do
type { DecisionReviewTask.name }
appeal { create(:higher_level_review, benefit_type: "education") }
assigned_by { nil }
Copy link
Contributor Author

@yoomlam yoomlam Mar 4, 2020

Choose a reason for hiding this comment

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

Deal with these later (in a subsequent stacked PR) since they create appeals differently.

appeal
end
factory :ama_colocated_task, traits: [ColocatedTask.actions_assigned_to_colocated.sample.to_sym],
parent: :colocated_task do
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't know we could call a different parent. 👌

end
factory :ama_task, class: Task do
type { Task.name }
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use this task as the parent factory instead as it already serves as a base "generic task on an ama appeal"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! My only concern is class: Task, i.e., if child factories will override it properly. I created a parent ama_task_base_factory in Part 3 so I'll execute your suggestion there.

Copy link
Contributor

@hschallhorn hschallhorn left a comment

Choose a reason for hiding this comment

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

🚢

@yoomlam yoomlam added Type: Tech-Improvement Team: Echo 🐬 Priority: Low Reported issue, not a blocker. "Last in" priority for new work. Eng: Backend Work Group backend engineering task labels Mar 6, 2020
* DRY it up by setting task.type based on declared `class`

* set attributes before saving task record

* use ama_task as the parent factory

* Fix dislocated parent tasks in FactoryBot - Part 4 (#13607)

* create parent tasks to be in the same appeal

* check parent task consistency

* create the task in the parent's appeal

* temporarily disable the check

* fix tests

* address deprecation warning

* add warnings

* move task that create their own appeal under ama_task

* remove warnings

* add code comment for diagnosis if tests fail

* Fix dislocated parent tasks in FactoryBot - Part 5 (#13620)

* trial to see how many tests fail

* test colocated_task having no parent

* fix some tests with Kat

* check if any tests would produce this warning

* don't leave colocated_task type selection to chance

* always pass appeal when creating parent task

* fix extra appeal created due to overriding appeal

* pass the task as the parent, not the id

* pass the task as the parent, not the id

* don't pass appeal if parent task is provided

* pass parent task so the created task is in same appeal

* pass actual parent task, not id

* increase counts to account for colocated's parent

* pass appeal when creating root_task

* add comments; adjust failure message

* reload task to see it's new children
@yoomlam yoomlam merged commit a5f39da into yoom/13130.1-factoryBot_dislocated_parent_tasks Mar 12, 2020
@yoomlam yoomlam deleted the yoom/13130.2-factoryBot_dislocated_parent_tasks branch March 12, 2020 19:28
yoomlam added a commit that referenced this pull request Mar 12, 2020
* set task's appeal consistently

* move outlier away from those that fit a pattern

* Fix dislocated parent tasks in FactoryBot - Part 2 (#13605)

* collect ama tasks together

* group all ama tasks under a common parent factory

* make it DRY

* Fix dislocated parent tasks in FactoryBot - Part 3 (#13606)

* DRY it up by setting task.type based on declared `class`

* set attributes before saving task record

* use ama_task as the parent factory

* Fix dislocated parent tasks in FactoryBot - Part 4 (#13607)

* create parent tasks to be in the same appeal

* check parent task consistency

* create the task in the parent's appeal

* temporarily disable the check

* fix tests

* address deprecation warning

* add warnings

* move task that create their own appeal under ama_task

* remove warnings

* add code comment for diagnosis if tests fail

* Fix dislocated parent tasks in FactoryBot - Part 5 (#13620)

* trial to see how many tests fail

* test colocated_task having no parent

* fix some tests with Kat

* check if any tests would produce this warning

* don't leave colocated_task type selection to chance

* always pass appeal when creating parent task

* fix extra appeal created due to overriding appeal

* pass the task as the parent, not the id

* pass the task as the parent, not the id

* don't pass appeal if parent task is provided

* pass parent task so the created task is in same appeal

* pass actual parent task, not id

* increase counts to account for colocated's parent

* pass appeal when creating root_task

* add comments; adjust failure message

* reload task to see it's new children

* address CodeClimate complaints
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Eng: Backend Work Group backend engineering task Priority: Low Reported issue, not a blocker. "Last in" priority for new work. Team: Echo 🐬 Type: Tech-Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants