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 1 #13604

Merged
merged 6 commits into from
Mar 12, 2020

Conversation

yoomlam
Copy link
Contributor

@yoomlam yoomlam commented Mar 4, 2020

Connects #13130

Description

First, let's just refactor to make setting of a task's appeal consistent.

Acceptance Criteria

  • All tests pass

@yoomlam yoomlam self-assigned this Mar 4, 2020
@codeclimate
Copy link

codeclimate bot commented Mar 4, 2020

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

View more on Code Climate.

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

factory :higher_level_review_task, class: DecisionReviewTask do
type { DecisionReviewTask.name }
appeal { create(:higher_level_review, benefit_type: "education") }
assigned_by { nil }
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.

Moved these up here for now since they create appeal differently.

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 even know we had these! Looks like they're used for intake tests

@hschallhorn hschallhorn self-requested a review March 4, 2020 17:11
@@ -67,7 +79,7 @@

factory :distribution_task, class: DistributionTask do
type { DistributionTask.name }
appeal { create(:appeal) }
appeal
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 to all of these

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.

Great change!

@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
* 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
@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 Mar 12, 2020
@va-bot
Copy link
Collaborator

va-bot commented Mar 12, 2020

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

Generated by 🚫 Danger

@yoomlam yoomlam merged commit 2dc2d25 into master Mar 12, 2020
@yoomlam yoomlam deleted the yoom/13130.1-factoryBot_dislocated_parent_tasks branch March 12, 2020 21:34
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. Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master Team: Echo 🐬 Type: Tech-Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants