-
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
Nj 104 taxable interest income #4924
Conversation
@@ -234,6 +223,25 @@ def is_ineligible_or_unsupported_for_property_tax | |||
StateFile::NjHomeownerEligibilityHelper.determine_eligibility(@intake) != StateFile::NjHomeownerEligibilityHelper::ADVANCE | |||
end | |||
|
|||
def calculate_line_40a |
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.
Moved to be in sequential order by line number, did not change as part of this PR
Heroku app: https://gyr-review-app-4924-2cc183bc5ab3.herokuapp.com/ |
@@ -223,31 +239,31 @@ def hash_for_pdf | |||
# line 40a | |||
if get_property_tax.present? | |||
answers.merge!(insert_digits_into_fields(get_property_tax.to_i, [ | |||
"24539a#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.
Linter auto fix, ugh. Is there a rubocop setting we should change somewhere to get our respective text editors to treat this the same?
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 to me. Left a comment wrt to decimal v. floats. I don't think it actually matters in this specific case but I'd like an explicit decision to either punt on it or to make the change and then I'll approve.
return nil | ||
end | ||
|
||
is_mfs_same_home ? (property_tax_paid / 2.0).round : property_tax_paid.round |
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.
You may consider 2.to_d
since this is money. I'm not sure that it would matter with a single calculation that's being rounded anyways, but converting to decimal is more correct because the decimal type uses (to my understanding) two integers to represent the decimal in a fraction notation until the number is ultimately intended to be used when it finally does the math that converts back to a float for display.
In this specific case it probably doesn't matter but it might be good to begin using decimals by default with taxes?
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.
This was not part of this PR, I just moved the method to put them in sequential order
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!
|
||
context 'with interest on government bonds' do | ||
let(:intake) { create(:state_file_nj_intake, :df_data_two_deps) } | ||
it 'sets line 16a to 300 (fed taxable income minus sum of bond interest)' 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.
[dust/pebble] could we add numbers to these calculation descriptions? Helps me track down where they're coming from in the test data.
ie, "sets line 16a to $300 (fed taxable income $XXX minus sum of bond interest $XXX)"
"undefined_41", | ||
"undefined_40", | ||
"undefined_39", | ||
"undefined_43"].each { |pdf_field| expect(pdf_fields[pdf_field]).to eq "" } |
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.
Love this
15fa06d
to
db9dffe
Compare
Link to pivotal/JIRA issue
https://github.com/newjersey/affordability-pm/issues/104
Is PM acceptance required? (delete one)
What was done?
Add line 16a to PDF and XML
How to test?
Use the Zeus two dependents profile, see 16a filled in pdf and xml