-
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 996 add two income sub md #4945
base: main
Are you sure you want to change the base?
Conversation
Heroku app: https://gyr-review-app-4945-f3f4db820050.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.
can we add some feature spec expectations in the spec/features/state_file/complete_intake_spec.rb
for the MD context?
happy to chat through the comments as well.
Also if you end up changing the migration, you may have to close & reopen the PR to re-create the heroku env with the migration from scratch!
@@ -31,6 +31,7 @@ class StateFileMdQuestionNavigation < Navigation::StateFileBaseQuestionNavigatio | |||
Navigation::NavigationStep.new(StateFile::Questions::MdCountyController), | |||
Navigation::NavigationStep.new(StateFile::Questions::DataTransferOffboardingController, false), | |||
Navigation::NavigationStep.new(StateFile::Questions::IncomeReviewController), | |||
Navigation::NavigationStep.new(StateFile::Questions::MdTwoIncomeSubtractionsController), |
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.
It says Updated: Page displays right before ID page
in the Jira ticket, has this been updated to move before the UnemploymentController? Maybe we can ask if this comes before or after the unemployment controller?
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.
ah yes, just double checked with the ticket author Erica and this should go after the unemployment controller, updated 👍
@@ -0,0 +1,6 @@ | |||
class AddStudentLoanInterestDedAmountToStateFileMdIntakes < ActiveRecord::Migration[7.1] | |||
def change | |||
add_column :state_file_md_intakes, :student_loan_interest_ded_amount_cents, :bigint, default: 0, null: false |
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.
can we call this primary_student_loan_interest_ded_amount
also spouse_student_loan_interest_ded_amount
(remove _cents
for both)
can we also make this a decimal column like all our other _amount
or _amt
fields?
ex:
add_column :state_file_az_intakes, :charitable_cash_amount, :decimal, precision: 12, scale: 2
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.
updated this column, and added a feature spec context for mfj to include this added two income sub form 👍
Co-authored-by: Mike Rotondo <[email protected]>
Co-authored-by: Mike Rotondo <[email protected]>
Co-authored-by: Mike Rotondo <[email protected]>
Co-authored-by: Mike Rotondo <[email protected]>
Co-authored-by: Mike Rotondo <[email protected]>
Co-authored-by: Mike Rotondo <[email protected]>
Co-authored-by: Mike Rotondo <[email protected]>
Co-authored-by: Mike Rotondo <[email protected]>
Co-authored-by: Mike Rotondo <[email protected]>
Co-authored-by: Mike Rotondo <[email protected]>
f5cca25
to
dff5726
Compare
Co-authored-by: Mike Rotondo <[email protected]>
Co-authored-by: Mike Rotondo <[email protected]>
note - seeing a few failing specs on circleci that pass locally (run)
rerunning |
8f9b876
to
5b82d6d
Compare
Co-authored-by: Mike Rotondo <[email protected]>
5b82d6d
to
aea0918
Compare
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.
a few more comments but I swear we're nearly there! up to pair on these too if you want :)
|
||
def valid_amounts | ||
if @intake.direct_file_data.fed_student_loan_interest.present? | ||
if (primary_student_loan_interest_ded_amount.to_i + spouse_student_loan_interest_ded_amount.to_i) != @intake.direct_file_data.fed_student_loan_interest.to_i |
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.
Hmm I see a potential problem here:
primary: 20.75 + spouse: 20.75 = total loan interest: 41.50
use .to_i
for each value:
20 + 20 = 41
therefore the sum will never be equal. I guess fed_student_loan_interest
is always an integer (according to the xsd) so maybe we'll never be in this situation?
I think we should sum the value without converting to_i
, then convert to an integer (preferably round?) and make sure they're equal? what do you think?
(primary_student_loan_interest_ded_amount + spouse_student_loan_interest_ded_amount).round != @intake.direct_file_data.fed_student_loan_interest
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.
also could we add a spec for this validation?
expect(page).to have_text I18n.t('state_file.questions.terms_and_conditions.edit.title') | ||
click_on I18n.t("state_file.questions.terms_and_conditions.edit.accept") | ||
|
||
step_through_df_data_transfer("Transfer Zeus two w2s") |
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.
is it possible to use Transfer Zeus two w2s
for the fixture in the existing spec?
@@ -191,6 +191,7 @@ | |||
it "saves imported_permanent_address_confirmed as true" do | |||
expect(form.valid?).to eq true | |||
form.save | |||
intake.reload |
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.
hmm why did we need to update this in this PR?
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.
oh is it b/c of this? #4945 (comment) that's so odd. Did you double check running these specs locally on main
branch?
Link to pivotal/JIRA issue
https://codeforamerica.atlassian.net/browse/FYST-996
Is PM acceptance required? (delete one)
What was done?
How to test?
Screenshots (for visual changes)