Skip to content

NJ 119 - total income tax withheld line 55 #5049

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.

Sign up for GitHub

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

Jump to bottom
Merged
aloverso merged 2 commits into from
Dec 3, 2024
Merged

NJ 119 - total income tax withheld line 55 #5049

aloverso merged 2 commits into from
Dec 3, 2024

Conversation

@aloverso
Copy link
Contributor

@aloverso aloverso commented Dec 2, 2024 •
edited
Loading

Link to pivotal/JIRA issue

https://github.com/newjersey/affordability-pm/issues/119

Is PM acceptance required? (delete one)

  • Yes - don't merge until JIRA issue is accepted!

What was done?

  • Add line 55 summing box 17 across w2s

How to test?

  • zeus 2 w2s or zeus many w2s

Screenshots (for visual changes)

image image

@github-actions
Copy link

github-actions bot commented Dec 2, 2024

Heroku app: https://gyr-review-app-5049-f9131416808b.herokuapp.com/
View logs: heroku logs --app gyr-review-app-5049 (optionally add --tail)

@aloverso
Copy link
Contributor Author

aloverso commented Dec 2, 2024

@mluedke2 the undefined_114 PDF duplicated field name issue is included in here - it will be resolved when we update to the 2024 PDF anyways, so no worries there.

mluedke2
mluedke2 approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@mluedke2 mluedke2 left a comment

Choose a reason for hiding this comment

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

all clean to me! as you noted, we'll have to revisit to make sure the non-unique field works on the new pdf

jenny-heath
jenny-heath reviewed Dec 2, 2024
View reviewed changes
jenny-heath
jenny-heath approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@jenny-heath jenny-heath left a comment

Choose a reason for hiding this comment

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

looks good aside from the one comment about test setup!

noah-marcus
noah-marcus reviewed Dec 2, 2024
View reviewed changes
Copy link

@noah-marcus noah-marcus left a comment

Choose a reason for hiding this comment

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

I am missing some context overall, specifically on the nj1040_pdf.rb code. While I don't fully understand the why things are happening, it all looks good to me functionally 🙂

Left a comment around the use of rounding. Based off the tests, this seems intentional but wanted to call it out just in case!

@aloverso aloverso force-pushed the nj-119-total-income-tax-withheld branch from 074cb16 to f796769 Compare December 3, 2024 16:42
@aloverso aloverso merged commit c3d5ef3 into main Dec 3, 2024
4 checks passed
@aloverso aloverso deleted the nj-119-total-income-tax-withheld branch December 3, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mluedke2 mluedke2 mluedke2 approved these changes

@jenny-heath jenny-heath jenny-heath approved these changes

+1 more reviewer

@noah-marcus noah-marcus noah-marcus left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@aloverso @mluedke2 @noah-marcus @jenny-heath