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-1842] Do not allow clients to update W2 money amount with a nil value #5629

Conversation

arinchoi03
Copy link
Contributor

@arinchoi03 arinchoi03 commented Feb 22, 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?

  • Added validations if user enters nil into these fields
  • they were not folded into existing validates lines (using allow_blank: false) b/c it got too messy (i.e. need to evaluate local boxes & box14 conditional, easier to see in one place, i think)
  • instead, they are only run from the state_file_edit context (w2 editing screen; not from the income review page), since we believe this page is the source of the error that we're seeing.
  • Furthermore, we should ensure any calculations we do guard against potential nil values -- this was communicated to Tiffany & I'm flagging places where I'm seeing these errors pop up https://codeforamerica.atlassian.net/browse/FYST-1842
  • Ideally, we would allow users to proceed without the client having to put in the 0 -- however, this is not how our money fields work currently across the board (user always has to enter a value to proceed), so we're going to stay ✨ consistent until we determine a better approach for handling. Discussed this with Jey & Erica

How to test?

  • Describe the testing approach taken to verify the changes, including:
    • manual tests
    • Test data used:
      • MD zeus w2s
      • NC Bert 1099r (no w2 edit; checking nothing broke)
      • NJ Zeus box 14
      • AZ Tycho single 1099R
  • Specify any relevant testing environments used: dev
  • Risk Assessment
    • should be completely isolated to the w2 edit page

Screenshots (for visual changes)

  • Before
  • After
Screenshot 2025-02-24 at 9 50 21 AM

Copy link

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

@arinchoi03 arinchoi03 force-pushed the FYST-1842-editing-state-withholding-on-w-2-edit-screen-for-az-and-id-causes-error-on-final-review-page branch from c1a5d1f to d33051b Compare February 22, 2025 00:17
@@ -26,7 +26,7 @@
<%= f.vita_min_money_field(
field_name.to_sym,
t(".box14_#{code['name'].downcase}_html"),
options: { value: value || 0 },
options: { value: value },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed b/c we were pre-filling with 0 and then showing them an error when they saved to "put in 0"

box14_stpickup: 0,
box14_ui_hc_wd: 0,
box14_ui_wf_swf: 0,
w2_index: 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a valid state_file_w2 needs the values at set to non-nil value

@arinchoi03 arinchoi03 changed the title FYST-1842 Do not allow clients to update W2 money amount with a nil value [FYST-1842] Do not allow clients to update W2 money amount with a nil value Feb 22, 2025
@arinchoi03 arinchoi03 marked this pull request as ready for review February 24, 2025 17:40
…thholding-on-w-2-edit-screen-for-az-and-id-causes-error-on-final-review-page
@arinchoi03 arinchoi03 merged commit 096be0b into main Feb 25, 2025
7 checks passed
@arinchoi03 arinchoi03 deleted the FYST-1842-editing-state-withholding-on-w-2-edit-screen-for-az-and-id-causes-error-on-final-review-page branch February 25, 2025 18:24
@@ -70,12 +71,38 @@ class StateFileW2 < ApplicationRecord
validate :validate_tax_amts
validate :state_specific_validation
end

validate :validate_nil_tax_amounts, on: :state_file_edit
Copy link
Contributor Author

@arinchoi03 arinchoi03 Feb 25, 2025

Choose a reason for hiding this comment

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

re: conversation with @powersurge360 we could do a presence check here instead; maybe I could've added another block for state_file_edit form instead of creating a method:

with_options on :state_file_edit do
    validates :state_wages_amount, :presence => {message: ->(_object, _data) { I18n.t("state_file.questions.w2.edit.no_money_amount") }},
    validates :state_income_tax_amount,  :presence => {message: ->(_object, _data) { I18n.t("state_file.questions.w2.edit.no_money_amount") }},
    validates :local_wages_and_tips_amount,  :presence => {message: ->(_object, _data) { I18n.t("state_file.questions.w2.edit.no_money_amount") }}, if: -> StateFile::StateInformationService.w2_include_local_income_boxes(state_file_intake.state_code) }
    validates :local_income_tax_amount,  :presence => {message: ->(_object, _data) { I18n.t("state_file.questions.w2.edit.no_money_amount") }}, if: -> StateFile::StateInformationService.w2_include_local_income_boxes(state_file_intake.state_code) }
    validates :box14_fli,  :presence => {message: ->(_object, _data) { I18n.t("state_file.questions.w2.edit.no_money_amount") }}, if: -> { supported_box14_codes.includes("box14_fli")}
...etc

I think the pros of the pulled out logic is that I know exactly what were doing in the state_file_edit form only and it's easy to see when the validations run (the if blocks get gnarly toward the end of the lines)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also chatted about using https://api.rubyonrails.org/v7.1/classes/ActiveRecord/Normalization/ClassMethods.html
if we decided to default nil values to 0.

Something like this?

  normalizes :state_wages_amount, with: -> state_wages_amount { state_wages_amount.to_d }

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