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-1900] Make MD retirement subtraction card consistent with other states' #5691

Conversation

arinchoi03
Copy link
Contributor

@arinchoi03 arinchoi03 commented Mar 6, 2025

Followup to #5680; noticed that all states now use the partial in the review_header but only MD does not

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?

  • Uses the same pattern as other states
    • only showing the information for subtractions for eligible_1099rs: previously we were showing this card if any 1099rs exist, and then only showing the relevant sub-elements if followup existed (which could only exist if the 1099r was qualified) which is a bit less straightforward.
  • Left the disability card in the md_review b/c it needs to show if 1099Rs exist at all (at this point anyway, that's the logic); there is a followup ticket to refactor this to show if it matches the include logic on MD502R; Anyways, disability needs to be shown if 502R exists at all, so eligible_1099rs logic does not factor into showing disability-related information on the final review. I can move the whole section into the partial, if desired.
  • Alternatives considered:
    • I don't like how we're putting not shared html into the shared folder, i think they should all move back into the #{stsate_code}_review folders as specific partials. However, that'll involve me moving around more copy & not sure if i want to make Aaron/September do more testing for all states 😭

How to test?

  • Describe the testing approach taken to verify the changes, including:
    • Unit/integration/manual tests
    • Test data used
  • Specify any relevant testing environments used (e.g., development, staging, demo, Heroku).
  • Risk Assessment
    • Risks or side effects associated with the changes and how they were mitigated.
    • Highlight areas that may need extra attention during code review or testing.
    • Paste SQL queries or output where relevant

Screenshots (for visual changes)

  • Before
  • After

Copy link

github-actions bot commented Mar 6, 2025

Heroku app: https://gyr-review-app-5691-6c3432eecf93.herokuapp.com/
View logs: heroku logs --app gyr-review-app-5691 (optionally add --tail)

@arinchoi03 arinchoi03 changed the title Move MD retirement subtraction followups into partial like the other … [FYST-1900] Make MD retirement subtraction card consistent with other states' Mar 6, 2025
@arinchoi03 arinchoi03 force-pushed the FYST-1900-refactor-md-retirement-subtractions-card-on-final-review-to-be-consistent-with-az-md-nc branch from 97c8c9a to 385c9a1 Compare March 6, 2025 20:18
@arinchoi03 arinchoi03 force-pushed the FYST-1900-refactor-md-retirement-subtractions-card-on-final-review-to-be-consistent-with-az-md-nc branch from 385c9a1 to a544708 Compare March 6, 2025 20:26
<div class="spacing-below-5">
<h2 class="text--body text--bold spacing-below-15"><%=t(".retirement_income_deductions") %></h2>

<% current_intake.eligible_1099rs.each_with_index do |state_file1099_r, index| %>
Copy link
Contributor Author

@arinchoi03 arinchoi03 Mar 6, 2025

Choose a reason for hiding this comment

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

note that we're changing to eligible_1099rs instead of state_file1099_rs here -- could have problems navigating back to a specific 1099R unless we have the same indices as we are using in the subtraction controllers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in fact, we do have a problem. I used frank rollover & tried to navigate to the 2nd retirement deduction shown -- it takes me to oops page (on demo; fixed on this PR)

@arinchoi03 arinchoi03 marked this pull request as ready for review March 6, 2025 21:42
Copy link
Contributor

@anisharamnani anisharamnani left a comment

Choose a reason for hiding this comment

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

⭐ Thanks Arin for taking on this work!

Base automatically changed from FYST-1868-nc-only-ask-about-1099rs-with-taxable-amt to main March 6, 2025 23:41
…rement-subtractions-card-on-final-review-to-be-consistent-with-az-md-nc
…rement-subtractions-card-on-final-review-to-be-consistent-with-az-md-nc
@arinchoi03 arinchoi03 merged commit 4d4a35e into main Mar 7, 2025
7 checks passed
@arinchoi03 arinchoi03 deleted the FYST-1900-refactor-md-retirement-subtractions-card-on-final-review-to-be-consistent-with-az-md-nc branch March 7, 2025 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants