Skip to content
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-1868] NC: only ask about 1099rs with taxable amt #5680

Merged
merged 10 commits into from
Mar 6, 2025

Conversation

arinchoi03
Copy link
Contributor

@arinchoi03 arinchoi03 commented Mar 4, 2025

Link to pivotal/JIRA issue

Is PM acceptance required? (delete one)

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

Reminder: merge main into this branch and get green tests before merging to main

What was done?

  • Consolidated logic for eligible_1099rs into StateFileBaseIntake since all states now (except NJ) requires taxable_amount to show a RetirementIncomeSubtractionController descendant page. NC previously had no assumption about what was an eligible 1099r, so it utilized the base intake method for eligible_1099rs which returned state_file1099_rs (basically all 1099rs from df)
  • Updated self.show? logic on RetirementIncomeSubtractionController to be consistent across all states as well

How to test?

  • Describe the testing approach taken to verify the changes, including:
    • unit tests written
    • Test data used: Elphaba, should only see 1 1099R subtractions question
    • Since this was refactored & impacts all states, double check that the show logic works for all states (AZ/ID/NC/MD) as currently expected -- only eligible (>0 taxable_amount) 1099rs should be shown
      • for ID, the disability logic for 62-64 will factor into the show logic

Copy link

github-actions bot commented Mar 4, 2025

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

@arinchoi03 arinchoi03 marked this pull request as ready for review March 4, 2025 19:57
@@ -313,4 +313,15 @@
end
end

describe "#eligible_1099rs" do
%w[az md nc nj].each do |state_code|
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nj is not using this method, but wanted to document that this method does exist for them

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm guessing idaho already has a separate test for this because of its additional logic with mfs

Copy link
Contributor

@tahsinaislam tahsinaislam left a comment

Choose a reason for hiding this comment

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

lgtm!

@arinchoi03
Copy link
Contributor Author

arinchoi03 commented Mar 6, 2025

you can look at the individual commits to make sense of what's happening:

  1. 93ad0ec fixing the bug by only going through 1099rs that are eligible -- the indices were not matching btwn 1099Rs shown on retirement subtraction controllers & nc review)
  2. e7e20db making NC use the partial like AZ/ID (MD to come in a later PR, didn't want to add even more to this PR)
  3. 80e9476 spec tweaked to guard against showing ineligible 1099Rs on this page, although that was guarded by the follow up check
  4. 9b4a030 use <section> instead of <div> since the implementation was inconsistent & probably better practice to use section for accessibility.

Copy link
Contributor

@tahsinaislam tahsinaislam left a comment

Choose a reason for hiding this comment

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

lgtm!

@arinchoi03 arinchoi03 merged commit 1b90224 into main Mar 6, 2025
7 checks passed
@arinchoi03 arinchoi03 deleted the FYST-1868-nc-only-ask-about-1099rs-with-taxable-amt branch March 6, 2025 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants