Nj 97 income eligibility #4941

mluedke2 · 2024-11-01T15:30:14Z

Link to pivotal/JIRA issue

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

Is PM acceptance required?

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?

I moved logic that was going to be reused (wage calculation, age check, property tax eligibility) into lib/efile/
I edited both next_path() and show?() for several of the property tax-related screens to skip questions not needed due to taxpayer not being eligible for what they were asking. (I learned that next_path() sort of overrides show?(), and had to use both to properly build the flow)
I considered adding a line to the calculator (and a bunch of related tests) to make super explicit the requirement 40a, and 41 should be blank. Those lines are blank if property_tax_paid or rent_paid are nil, which is always true when the screens this PR skips are skipped. So in the end it felt over-constrained and would not happen in practice so I removed the extra code. You can see what I removed in this commit if interested.

How to test?

The current flow stays the same for everyone with income above the minimum, so that can be still experienced with Zeus many w2s (video below)
The Minimal transfer has an income below the minimum, so using that you can see that the user is still asked about being homeowner/tenant but then is moved along (video below)
The Blind primary transfer has income below minimum but qualifies for credit-only, so you can see user is asked about homeowner/tenant and the housing details eligibility, but skips the screen asking for property tax/rent paid (video below)
The new Married filing jointly 15k wages transfer has an income that is below the $20k minimum for MFJ
The new Married filing separately 15k wages transfer has an income that is above the $10k minimum for MFS

Videos

Zeus many w2s

zeus_many_w2s_720.mov

Minimal

minimal_720.mov

Blind primary

blind_primary_720.mov

MFJ $15k Wages (below $20k minimum)

mfj_15k_720.mov

MFS $15k Wages (above $10k minimum)

mfs_15k_720.mov

…gible

…ible

…e 40a

# Conflicts: # app/controllers/state_file/questions/nj_homeowner_eligibility_controller.rb # app/controllers/state_file/questions/nj_homeowner_property_tax_controller.rb # app/controllers/state_file/questions/nj_tenant_eligibility_controller.rb # app/controllers/state_file/questions/nj_tenant_rent_paid_controller.rb # app/lib/efile/nj/nj1040_calculator.rb # spec/lib/efile/nj/nj1040_calculator_spec.rb

…lity # Conflicts: # app/lib/efile/nj/nj1040_calculator.rb

# Conflicts: # app/lib/efile/nj/nj1040_calculator.rb

…de after all

# Conflicts: # app/lib/efile/nj/nj1040_calculator.rb

github-actions · 2024-11-01T15:34:39Z

Heroku app: https://gyr-review-app-4941-589890dc884a.herokuapp.com/
View logs: heroku logs --app gyr-review-app-4941 (optionally add --tail)

app/controllers/state_file/questions/nj_homeowner_eligibility_controller.rb

app/lib/efile/nj/nj_property_tax_eligibility.rb

app/lib/efile/nj/nj_state_wages.rb

spec/lib/efile/nj/nj_property_tax_eligibility_spec.rb

app/lib/efile/nj/nj_senior.rb

jachan

Agree with Melanie's comments on naming to avoid NOT_INELIGIBLE, and then have some minor comments on code style. Great work with your first (I think?) PR!

app/controllers/state_file/questions/nj_homeowner_eligibility_controller.rb

app/lib/efile/nj/nj1040_calculator.rb

app/lib/efile/nj/nj_property_tax_eligibility.rb

app/lib/efile/nj/nj_state_wages.rb

jachan · 2024-11-04T18:55:30Z

spec/controllers/state_file/questions/nj_tenant_eligibility_controller_spec.rb

+            tenant_home_subject_to_property_taxes: "no"
+          )
+        }
+        it "next path is ineligible page" do


[DUST] Great tests for next path, these are very legible.

…nimum

arinchoi03

[Suggestion] I'm a bit newer to the team so I'm still figuring this out but wondering if all the specific housing-related (NjTenantEligibilityController, NjHomeownerEligibilityController, NjUnsupportedPropertyTaxController) controllers need to be in the StateFileNjQuestionNavigation? This may help with all the logic in the show method -- if that controller is not in the navigation, only time show? will be called if you explicitly navigate to that controller.

app/controllers/state_file/questions/nj_homeowner_eligibility_controller.rb

arinchoi03 · 2024-11-06T23:26:20Z

app/controllers/state_file/questions/nj_homeowner_eligibility_controller.rb

        case StateFile::NjHomeownerEligibilityHelper.determine_eligibility(current_intake)
        when StateFile::NjHomeownerEligibilityHelper::INELIGIBLE
          NjIneligiblePropertyTaxController.to_path_helper(options)
        when StateFile::NjHomeownerEligibilityHelper::UNSUPPORTED
          NjUnsupportedPropertyTaxController.to_path_helper(options)
        else
-          NjHomeownerPropertyTaxController.to_path_helper(options)
+          if Efile::Nj::NjPropertyTaxEligibility.determine_eligibility(current_intake) == Efile::Nj::NjPropertyTaxEligibility::POSSIBLY_ELIGIBLE_FOR_CREDIT


[dust] would it be cleaner/easier to read if there a way to just call current_intake.possibly_eligible_for_credit? instead of doing comparisons like this everywhere? i.e. define these methods on NJ intake?

Ooh thanks for this suggestion. I don't feel too strongly about having the logic live in the util vs intake model, especially since this is my first PR so there may be a pattern I'm not aware of, but I lean util to keep it separate from everything else.

That being said I'll make the functions like .possibly_eligible_for_credit?(intake) in the util, so it can be

Efile::Nj::NjPropertyTaxEligibility.possibly_eligible_for_deduction_or_credit?(intake)

instead of

Efile::Nj::NjPropertyTaxEligibility.determine_eligibility(intake) == Efile::Nj::NjPropertyTaxEligibility::POSSIBLY_ELIGIBLE_FOR_DEDUCTION_OR_CREDIT

sounds fine to put it in the util

added these in the latest commit

Thanks for humoring me!

mluedke2 · 2024-11-07T18:18:10Z

@arinchoi03 that is helpful to know that the questions don't all need to be in the Navigation-- our process thus far has been to add all of them to it. I think since this logic is not too tedious I'll keep it, but I'll remember that for next time

mluedke2 and others added 30 commits October 25, 2024 14:32

work in progress - util methods for age checking, wage summing

112a781

fix ruby syntax

5549619

initial tests for property tax eligibility

dfadf6e

add more test cases

14f10fd

remaining test cases for eligibility logic

82a7bab

only allow spouse exceptions when filing jointly

fb74201

tests for state wages util

e0148bf

tests for 65yr check

2d5607d

correct place for age util

0edabff

hide homeowner or tenant questions when user is already found not eli…

0aeaa54

…gible

test for all possibilities on line 40a to make sure blank when inelig…

f71772b

…ible

NJ 140 - fix XML errors and add schema validation to all XML tests

a1f33ea

add possible eligibility enums to tests

72a6d8a

undoes the past two commits to just allow existing tests to cover lin…

192b455

…e 40a

put in the redundancy after all so that it is explicitly tested

9e867cf

rename existing function to specifically check for credit ineligibility

c97adfa

deduction and credit lines are blank when expected

0746509

reword some test names

d579b1c

fix test failures

1bcd972

review comments

c876c2f

add tests to xml layer

c6c21ec

make locality name test conditional

d179b0c

review comment

7c6f7c4

Merge branch 'main' into nj-140-xml-schema-validation

579b381

Merge branch 'nj-140-xml-schema-validation' into nj-97-income_eligibi…

9735feb

…lity # Conflicts: # app/lib/efile/nj/nj1040_calculator.rb

Merge branch 'main' into nj-97-income_eligibility

69e8242

# Conflicts: # app/lib/efile/nj/nj1040_calculator.rb

expect nil directly

99a1025

follow the pattern better by using traits not mocks

cb6faad

next_path overrides show? so start moving to that

16442b7

mluedke2 added 10 commits October 31, 2024 13:37

fully use next_path and not show?

316ad77

cleanup some tests

73da3ec

revert calculator line that does not seem needed

ce1b058

tests on pdf layer

0251a7a

put the show? logic back in-- has downstream effects so isn't dead co…

81ac466

…de after all

make sure to consider all eligibility for credit

6292055

go to whatever's next and not the ineligible question

228b289

tenant side of combining eligibility types

1a2a38d

odd whitespace

e20ade0

Merge branch 'main' into nj-97-income_eligibility

01e5d95

# Conflicts: # app/lib/efile/nj/nj1040_calculator.rb

mluedke2 requested review from jachan and mmazanec22 November 1, 2024 15:56

mmazanec22 reviewed Nov 4, 2024

View reviewed changes

jachan approved these changes Nov 4, 2024

View reviewed changes

mluedke2 added 6 commits November 6, 2024 09:30

rename enums and arrange to reduce lines in tests

87f0012

use predicate methods when possible

b1538bb

refactor senior age checks to use methods in intake

4b1e46e

add comments to calls to super in next_path to make behavior more clear

895dce2

add married filing separately with $15k wages to test property tax mi…

1ecd53a

…nimum

add married filing jointly $15k wages to test property tax minimum

0b8f9cd

mluedke2 marked this pull request as ready for review November 6, 2024 20:54

Merge branch 'main' into nj-97-income_eligibility

8e8ece4

arinchoi03 approved these changes Nov 6, 2024

View reviewed changes

mluedke2 added 2 commits November 7, 2024 08:58

Merge branch 'main' into nj-97-income_eligibility

46a253e

remove whitespace

84f213f

convenience functions for readability

65719ab

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nj 97 income eligibility #4941

Nj 97 income eligibility #4941

mluedke2 commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 1, 2024

jachan left a comment

jachan Nov 4, 2024

arinchoi03 left a comment •

edited

Loading

arinchoi03 Nov 6, 2024 •

edited

Loading

mluedke2 Nov 7, 2024

arinchoi03 Nov 7, 2024 •

edited

Loading

mluedke2 Nov 7, 2024

arinchoi03 Nov 7, 2024

mluedke2 commented Nov 7, 2024

Nj 97 income eligibility #4941

Are you sure you want to change the base?

Nj 97 income eligibility #4941

Conversation

mluedke2 commented Nov 1, 2024 • edited Loading

Link to pivotal/JIRA issue

Is PM acceptance required?

What was done?

How to test?

Videos

Zeus many w2s

Minimal

Blind primary

MFJ $15k Wages (below $20k minimum)

MFS $15k Wages (above $10k minimum)

github-actions bot commented Nov 1, 2024

jachan left a comment

Choose a reason for hiding this comment

jachan Nov 4, 2024

Choose a reason for hiding this comment

arinchoi03 left a comment • edited Loading

Choose a reason for hiding this comment

arinchoi03 Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

mluedke2 Nov 7, 2024

Choose a reason for hiding this comment

arinchoi03 Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

mluedke2 Nov 7, 2024

Choose a reason for hiding this comment

arinchoi03 Nov 7, 2024

Choose a reason for hiding this comment

mluedke2 commented Nov 7, 2024

mluedke2 commented Nov 1, 2024 •

edited

Loading

arinchoi03 left a comment •

edited

Loading

arinchoi03 Nov 6, 2024 •

edited

Loading

arinchoi03 Nov 7, 2024 •

edited

Loading