-
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-1717] Add exemption amounts for Dependent Taxpayers #5481
[FYST-1717] Add exemption amounts for Dependent Taxpayers #5481
Conversation
Heroku app: https://gyr-review-app-5481-9939f3a5ea01.herokuapp.com/ |
@@ -244,7 +244,7 @@ | |||
|
|||
context "exemptions stuff" 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.
can we rename this to context "exemptions" 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.
you got it
xml.Count calculated_fields.fetch(:MD502_LINE_D_COUNT_TOTAL) | ||
xml.Amount calculated_fields.fetch(:MD502_LINE_D_AMOUNT_TOTAL) | ||
end | ||
if has_dependent_exemption? |
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 you add a test for this?
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.
xml.Count calculated_fields.fetch(:MD502_LINE_A_COUNT) | ||
xml.Amount calculated_fields.fetch(:MD502_LINE_A_AMOUNT) | ||
end | ||
if has_exemptions? |
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 you also add a test for this?
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.
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.
I have a few clarifying questions and requests for tests.
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.
thanks so much for walking me through this!
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?