-
Notifications
You must be signed in to change notification settings - Fork 13
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
FYST-545-add-maryland-address-screen-and-data-model-to-md #4927
FYST-545-add-maryland-address-screen-and-data-model-to-md #4927
Conversation
Heroku app: https://gyr-review-app-4927-987f2c261dd1.herokuapp.com/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few requested changes
app/controllers/state_file/questions/md_permanent_address_controller.rb
Outdated
Show resolved
Hide resolved
@@ -91,6 +97,7 @@ class StateFileMdIntake < StateFileBaseIntake | |||
enum eligibility_homebuyer_withdrawal: { unfilled: 0, yes: 1, no: 2 }, _prefix: :eligibility_homebuyer_withdrawal | |||
enum eligibility_homebuyer_withdrawal_mfj: { unfilled: 0, yes: 1, no: 2 }, _prefix: :eligibility_homebuyer_withdrawal_mfj | |||
enum eligibility_home_different_areas: { unfilled: 0, yes: 1, no: 2 }, _prefix: :eligibility_home_different_areas | |||
enum confirmed_permanent_address: { unfilled: 0, yes: 1, no: 2 }, _prefix: :confirmed_permanent_address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enum confirmed_permanent_address: { unfilled: 0, yes: 1, no: 2 }, _prefix: :confirmed_permanent_address | |
enum confirmed_permanent_address: { unfilled: 0, yes: 1, no: 2 }, _prefix: :confirmed_permanent_address | |
enum permanent_address_outside_md: { unfilled: 0, yes: 1, no: 2 }, _prefix: :permanent_address_outside_md |
@@ -118,6 +124,16 @@ | |||
spouse_last_name { "Lando" } | |||
end | |||
|
|||
factory :state_file_md_refund_intake do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this factory case? And if you do need i would advise adding it as a trait instead so it would build or overwrite the base intake something more like:
factory :state_file_md_refund_intake do | |
trait :with_refund do | |
after(:build) do |intake, evaluator| | |
intake.direct_file_data.fed_wages = 2_000 | |
intake.direct_file_data.fed_taxable_income = 2_000 | |
intake.direct_file_data.fed_taxable_ssb = 0 | |
intake.direct_file_data.fed_unemployment = 0 | |
end | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just that one change and I think you'll have to pull main before merging to resolve the merge conflictt
@@ -1,8 +1,7 @@ | |||
module StateFile | |||
module Questions | |||
class MdPermanentAddressController < QuestionsController | |||
include ReturnToReviewConcern | |||
include EligibilityOffboardingConcern | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I think you'll still need include ReturnToReviewConcern
though
Link to pivotal/JIRA issue
Is PM acceptance required? (delete one)
Reminder: merge main into this branch and get green tests before merging to main
What was done?
How to test?