-
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 #4963
base: main
Are you sure you want to change the base?
Conversation
Heroku app: https://gyr-review-app-4963-5054cb9a135f.herokuapp.com/ |
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]>
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]>
93999a0
to
16fe2ce
Compare
Co-authored-by: Mike Rotondo <[email protected]>
|
||
def valid_amounts | ||
if @intake.direct_file_data.fed_student_loan_interest.present? | ||
if (primary_student_loan_interest_ded_amount.to_f + spouse_student_loan_interest_ded_amount.to_f).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.
[q] was there a reason we chose to_i
over to_round
?
If a client enters 5.25 and 5.25 for both amounts, and the resulting sum is 10.5, is it more correct to make sure it gets compared to the federal_student_loan_interest
as 10 or 11? I'm guessing this is a really unlikely situation here that someone seeing an integer value will purposefully add decimal points here.
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 this should call @intake.direct_file_data.fed_student_loan_interest.to_f
instead then, and the prev value should not call to_i, so that the decimals are compared properly
Co-authored-by: Mike Rotondo <[email protected]>
|
||
def valid_amounts | ||
if @intake.direct_file_data.fed_student_loan_interest.present? | ||
if (primary_student_loan_interest_ded_amount.to_f + spouse_student_loan_interest_ded_amount.to_f) != @intake.direct_file_data.fed_student_loan_interest.to_f |
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.
say fed_student_loan_interest
is 100 and the person puts in 50 and 49.5 (for some reason), do we want to be strict and have them correct that data to be 50? or should we round these inputs?
Co-authored-by: Mike Rotondo <[email protected]>
<p class="spacing-below-5"><%= t(".subtitle") %></p> | ||
<p class="text--small spacing-below-5"><%= t(".total", total_deduction: number_to_currency(@total_deduction)) %></p> | ||
<div class="form-question spacing-below-25"> | ||
<%= f.vita_min_money_field(:primary_student_loan_interest_ded_amount, "Your portion of the student loan interest deduction", classes: ["form-width--long"]) %> |
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.
Does the label text have to be translated?
</div> | ||
|
||
<div class="form-question spacing-below-25"> | ||
<%= f.vita_min_money_field(:spouse_student_loan_interest_ded_amount, "Your spouse's portion of the student loan interest deduction", classes: ["form-width--long"]) %> |
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.
and translations here as well!
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.
good points, added 👍
I have a few minor translation comments, but the tests are failing, so I don't feel confident approving. I'm happy to take a look once again once they're passing! |
Co-authored-by: Mike Rotondo <[email protected]>
Co-authored-by: Mike Rotondo <[email protected]>
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.
ty for your patience over the many rounds of reviews !
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)
copy of #4945 to restar heroku builds