Nj 125 income editing #5288

mluedke2 · 2024-12-24T16:15:43Z

Link to pivotal/JIRA issue

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

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?

Adapted to the new XML schema for UI/WF/SWF to UIWFSWF and UIHCWD
An overwrite method is now in the W2 model to capture intended behavior that combines UIHCWD into UIWFSWF if UIWFSWF is empty... and downstream calculators and XML for NJ1040 and NJ2450 also needed adapting for this.
The W2 model also has validation checks for the limits for those specific types of income. The validation has to be conditional to allow for importing any info, but then turned on by the Income Review and W2 Editing controllers to not allow users to proceed without fixing.
I decided to reuse the existing income and W2 editing controllers and forms, and put in checks for the existence of info about a given state not taxing a type of income.
There weren't E2E tests yet specifically for just income review so I added NJ-specific tests there for this ticket's AC

How to test?

Use Lucky to see W2 Box 14 editing, and O'Neal Walker for seeing messaging for other income forms.

Screenshots (for visual changes)

after trying to save values above the limits

mluedke2 · 2024-12-31T20:44:36Z

@jachan @mmazanec22 this is ready for review again!

mluedke2 · 2025-01-02T17:13:53Z

[boulder] - i need to change how i am validating the limit on W2 Box 14 because as a validation on the model I am blocking even the import of invalid values, which causes a 500 on the Lucky profile since it has a too-high value... Will likely move the validation to just be on the w2 editing form step

EDIT: Addressed now!

mluedke2 · 2025-01-02T17:14:25Z

~~[boulder] - there's also a boulder on the XML export which is still exporting both UI values for box14 instead of consolidating them.~~

EDIT: Addressed now!

mluedke2 · 2025-01-03T19:01:54Z

the two boulders have been addressed!

# Conflicts: # app/services/state_file/state_information_service.rb

anisharamnani

looks good, also tested MD box 14 to see if anything changed - tahsina

mluedke2 · 2025-01-07T20:41:21Z

app/controllers/state_file/questions/w2_controller.rb

@@ -9,6 +9,7 @@ def edit

      def update
        @w2.assign_attributes(form_params)
+        @w2.check_box14_limits = true


note from review: can we turn on this flag in one place when the import is done?

@mmazanec22 I did manage to try out an after_create callback on the W2 model to toggle check_box14_limits just after creation, which does allow import... but it's a bit of a challenge to get it to stay set, I think because the controllers re-fetch them from the database when the forms load. So I think I'd either need to

1- find a way to pass the w2 model instances from one place to another instead of loading from DB
2- add a db column for this and set it
3- keep how i have it, setting in each controller each time

I think the second option is good but is not so much better that I want to add it to this PR.

That seems reasonable!

mluedke2 · 2025-01-07T20:47:20Z

app/lib/efile/nj/nj1040_calculator.rb

      end

      def line_59_spouse
        if @intake.filing_status_mfj?
-          get_personal_excess(@intake.spouse.ssn, :box14_ui_wf_swf, EXCESS_UI_WF_SWF_MAX) +
-            get_personal_excess(@intake.spouse.ssn, :box14_ui_hc_wd, EXCESS_UI_WF_SWF_MAX)
+          get_personal_excess(@intake.spouse.ssn, :box14_ui_wf_swf, EXCESS_UI_WF_SWF_MAX)


review comment: instead of passing excess type, could just pass the value itself

@mmazanec22 actually, i looked at this again and because get_excess_value() is called within two different iterators (ie needs to look at several w2s), it actually makes that function not so easy to get rid of...

Maybe we add this to the follow up ticket as a "take a look at this"? I don't fully understand why we can't pass in get_box14_ui_overwrite instead of the field name here

mluedke2 · 2025-01-07T20:50:16Z

app/models/state_file_w2.rb

+    if value.present? && value > limit
+      errors.add(field, I18n.t("validators.dollar_limit", limit: '%.2f' % limit))
+    end
+  end


review comment: check if it would be cleaner to put this in the NJ-specific W2

@mmazanec22 it looks like we don't actually have an NJ-specific W2 model yet. we could add one, but i think i will make a follow-up ticket for this

https://github.com/orgs/newjersey/projects/74/views/1?pane=issue&itemId=92886410&issue=newjersey%7Caffordability-pm%7C241

Sorry, meant this comment for the XML builder file. Follow up seems fine to me though

mluedke2 · 2025-01-07T20:58:49Z

app/views/state_file/questions/w2/edit.html.erb

+                @w2.get_box14_ui_overwrite
+              else
+                @w2.send(field_name)
+              end %>


review comment: could a method on the W2 model prevent us from having to do all these if-else to get these values?

@mmazanec22 I want to keep this one open too, but since it's only used in a few spots (and in different ways) I think I want to leave as a follow-up in the same ticket as above https://github.com/orgs/newjersey/projects/74/views/1?pane=issue&itemId=92886410&issue=newjersey%7Caffordability-pm%7C241

mmazanec22 · 2025-01-06T20:52:20Z

app/lib/efile/nj/nj2450_calculator.rb

-          sum + (w2.box14_ui_hc_wd || 0) + (w2.box14_ui_wf_swf || 0)
-        end
-        total.round
+        w2s.sum { |w2| w2.get_box14_ui_overwrite || 0 }.round


Nice, more readable

mmazanec22 · 2025-01-08T19:19:20Z

app/controllers/state_file/questions/w2_controller.rb

@@ -9,6 +9,7 @@ def edit

      def update
        @w2.assign_attributes(form_params)
+        @w2.check_box14_limits = true


That seems reasonable!

mmazanec22 · 2025-01-08T19:20:05Z

app/models/state_file_w2.rb

+    if value.present? && value > limit
+      errors.add(field, I18n.t("validators.dollar_limit", limit: '%.2f' % limit))
+    end
+  end


Sorry, meant this comment for the XML builder file. Follow up seems fine to me though

mmazanec22 · 2025-01-08T19:23:13Z

app/lib/efile/nj/nj1040_calculator.rb

      end

      def line_59_spouse
        if @intake.filing_status_mfj?
-          get_personal_excess(@intake.spouse.ssn, :box14_ui_wf_swf, EXCESS_UI_WF_SWF_MAX) +
-            get_personal_excess(@intake.spouse.ssn, :box14_ui_hc_wd, EXCESS_UI_WF_SWF_MAX)
+          get_personal_excess(@intake.spouse.ssn, :box14_ui_wf_swf, EXCESS_UI_WF_SWF_MAX)


Maybe we add this to the follow up ticket as a "take a look at this"? I don't fully understand why we can't pass in get_box14_ui_overwrite instead of the field name here

mluedke2 added 30 commits December 17, 2024 09:46

work in progress - comments on relevant lines

be6ee8c

Merge branch 'main' into nj-125-income-editing

c4b0a65

one way to hide the unemployment box in nj

b04f37a

use mailing state instead

aaaa000

hide ssa-1099 if nj as well

46f7b6d

only show income review if there is something to review

8848c80

share display condition logic between controller and erb

026f17b

Merge branch 'main' into nj-125-income-editing

2d736b0

for now just check for state is nj

89fcc1a

Merge branch 'main' into nj-125-income-editing

bf0927f

remove comment for out of scope

60e06ab

remove extra comment

505c8a8

add e2e tests for income review visual changes

37bdf9c

remove unused imports

648e1ae

update lucky w2

a74cca3

nj-specific content

c1192ca

dry and condition styling

b0ddea4

edit text for box14

850a75d

Merge branch 'main' into nj-125-income-editing

f8465ab

update xml for box 14

1b36583

Merge branch 'main' into nj-125-income-editing

71bd8d5

sync es

1c6ba7d

conform to new format without slashes

5ac1833

expectations for w2 editing

fc59a55

proper test for assigning box 14 values

496b44f

update more syntax for the no-slash format

609969e

don't use shared tests for nj since it's tested separately

d23cc09

missing key in es

7bb59ea

normalize strings

34db50e

Merge branch 'main' into nj-125-income-editing

2826eda

remove unneeded comments

b939fb1

mluedke2 marked this pull request as ready for review December 31, 2024 20:28

mluedke2 requested review from mmazanec22 and jachan December 31, 2024 20:28

Merge branch 'main' into nj-125-income-editing

5745774

mluedke2 added 6 commits January 2, 2025 11:01

Merge branch 'main' into nj-125-income-editing

bd6b566

allow imports to happen without box14 validation

3b769dd

remove non-supported box 14 values from xml

ed9ada7

test for box 14 validation in w2 editing screen

370ef24

validate box14 on income review too

106ccf0

only remove specific desc to not break other state tests

dd775ab

leave hc_wd out of 2450 and use overwrite there too

f264b7e

mluedke2 requested review from arinchoi03 and tahsinaislam January 3, 2025 21:47

mluedke2 added 4 commits January 3, 2025 13:53

tests on model for validation

071764d

Merge branch 'main' into nj-125-income-editing

8b8a794

Merge branch 'main' into nj-125-income-editing

df54a83

# Conflicts: # app/services/state_file/state_information_service.rb

simplify syntax

6f409c9

anisharamnani approved these changes Jan 7, 2025

View reviewed changes

Merge branch 'main' into nj-125-income-editing

c010c6d

mluedke2 commented Jan 7, 2025

View reviewed changes

amanifarooque mentioned this pull request Jan 8, 2025

SC 141 -- Color imbued with semantic qualities newjersey/accessibility-pm#26

Open

mmazanec22 reviewed Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nj 125 income editing #5288

Nj 125 income editing #5288

mluedke2 commented Dec 24, 2024 •

edited

Loading

mluedke2 commented Dec 31, 2024

mluedke2 commented Jan 2, 2025 •

edited

Loading

mluedke2 commented Jan 2, 2025 •

edited

Loading

mluedke2 commented Jan 3, 2025

anisharamnani left a comment

mluedke2 Jan 7, 2025

mluedke2 Jan 7, 2025

mmazanec22 Jan 8, 2025

mluedke2 Jan 7, 2025

mluedke2 Jan 7, 2025

mmazanec22 Jan 8, 2025

mluedke2 Jan 7, 2025

mluedke2 Jan 7, 2025 •

edited

Loading

mmazanec22 Jan 8, 2025

mluedke2 Jan 7, 2025

mluedke2 Jan 8, 2025 •

edited

Loading

mmazanec22 Jan 6, 2025

mmazanec22 Jan 8, 2025

mmazanec22 Jan 8, 2025

mmazanec22 Jan 8, 2025

Nj 125 income editing #5288

Are you sure you want to change the base?

Nj 125 income editing #5288

Conversation

mluedke2 commented Dec 24, 2024 • edited Loading

Link to pivotal/JIRA issue

Is PM acceptance required? (delete one)

What was done?

How to test?

Screenshots (for visual changes)

mluedke2 commented Dec 31, 2024

mluedke2 commented Jan 2, 2025 • edited Loading

mluedke2 commented Jan 2, 2025 • edited Loading

mluedke2 commented Jan 3, 2025

anisharamnani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mluedke2 Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mluedke2 Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mluedke2 commented Dec 24, 2024 •

edited

Loading

mluedke2 commented Jan 2, 2025 •

edited

Loading

mluedke2 commented Jan 2, 2025 •

edited

Loading

mluedke2 Jan 7, 2025 •

edited

Loading

mluedke2 Jan 8, 2025 •

edited

Loading