NJ 193 - Add none of the above option to property tax #5295

jachan · 2024-12-26T20:15:13Z

Link to pivotal/JIRA issue

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

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?

Add None of the Above checkbox to tenant and homeowner property tax eligibility
None of the Above unchecks all other checkboxes when selected
If none of the above is checked, and user checks another checkbox, none of the above becomes unchecked
Moved NJ End to End tests to its own file, as it was getting very large (~400 lines out of a 1000 line test)

How to test?

Describe the testing approach taken to verify the changes, including:
- Local Tests
  - Select "Lauryn MFS" (this allows an additional checkbox to appear for spouse)
    - Navigate to Property Tax, select Tenant or Homeowner
    - Validate that None of the Above behavior is as expected
- Added end-to-end tests

Screenshots (for visual changes)

Before

After

NJ.203.After.mov

jachan · 2024-12-26T20:16:01Z

spec/features/state_file/eligibility_spec.rb

Removed what appeared to be an empty test file.

jachan · 2024-12-26T20:17:03Z

spec/features/state_file/nj/complete_intake_spec.rb

+    def advance_to_property_tax_page(df_persona_name)
+      advance_to_start_of_intake(df_persona_name)
+      advance_county_and_municipality
+      if has_text? I18n.t("state_file.questions.nj_disabled_exemption.edit.title")


Made these conditional, as not all personas are asked about Disability (e.g. if Federal XML claims blindness, we take this exemption for them)

jachan · 2024-12-26T20:17:30Z

spec/features/state_file/nj/complete_intake_spec.rb

+        advance_disabled_exemption
+      end
+      advance_veterans_exemption
+      if has_text? I18n.t("state_file.questions.nj_college_dependents_exemption.edit.title")


Similarly, only personas with dependents are shown this controller, so made this conditional as well

jachan · 2024-12-26T20:18:10Z

spec/features/state_file/nj/complete_intake_spec.rb

+        expect_estimated_tax_page
+      end
+
+      it "handles homeowner none of the above checkbox", required_schema: "nj" do 


New test added here for homeowner none of the above

jachan · 2024-12-26T20:18:20Z

spec/features/state_file/nj/complete_intake_spec.rb

+        expect_estimated_tax_page
+      end
+
+      it "handles tenant none of the above checkbox", required_schema: "nj" do 


New test added here for tenant none of the above

jachan · 2024-12-26T20:19:08Z

spec/features/state_file/nj/complete_intake_spec.rb

Commented on changes, as moving all NJ tests out to a separate file makes it difficult to see what was modified.

Separating this out is a big improvement!

i agree! i think over future PRs it would be useful to separate out even more, so that this file really is just the minimal steps for a "complete intake" and then we can get into the weeds on edge cases in other places... but that can be done later

github-actions · 2024-12-26T20:19:42Z

Heroku app: https://gyr-review-app-5295-9b59a84ef4f5.herokuapp.com/
View logs: heroku logs --app gyr-review-app-5295 (optionally add --tail)

jachan · 2024-12-26T20:20:31Z

spec/features/state_file/nj/complete_intake_spec.rb

+      continue
+    end
+
+    def select_homeowner_eligibility(checkboxes, continue_to_next: true)


Updated this helper function and the one below by adding continue_to_next

This pattern will be useful for a11y testing too!

mmazanec22

Looks great! Non-blocking refactor suggestion

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

mmazanec22 · 2024-12-27T16:27:48Z

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

@@ -65,4 +70,36 @@

    <%= f.continue %>
  <% end %>
-<% end %>
+  <script>
+    document.addEventListener("DOMContentLoaded", function() {


[pebble] Seeing that we're using the same logic twice, I wonder if it's worth adding the function passed to the event listener to `app/javascript. You could refactor it to work for both cases by passing in the IDs of all not-none checkboxes, for each id adding the checkbox to the checkbox array if they're not null, passing in the noneCheckbox ID, and then optional checkbox ids that should be hidden when none is checked

i like this suggestion too. we or other states might need this pattern in more places so it would be handy to reuse

mmazanec22 · 2024-12-27T16:29:46Z

spec/features/state_file/nj/complete_intake_spec.rb

Separating this out is a big improvement!

mmazanec22 · 2024-12-27T16:30:16Z

spec/features/state_file/nj/complete_intake_spec.rb

+      continue
+    end
+
+    def select_homeowner_eligibility(checkboxes, continue_to_next: true)


This pattern will be useful for a11y testing too!

mluedke2

looks good! thanks for sorting tests too

mpidcock

Looks great!

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

mpidcock · 2024-12-30T23:05:12Z

spec/features/state_file/nj/complete_intake_spec.rb

[dust] I like that you separated this out! something to be cautious of is that these journey tests that walk through an entire intake are the longest running tests we have. we tend to use the "complete intake" spec to cover only the minimum flow to ensure all pages can load and in the right sequence.

for less common/more complex pages you could consider splitting them out into a separate journey test, such as this grocery credit page in idaho.

mrotondo

IMO this looks good, and should have a followup PR to refactor the duplicated code into a separate JS file.

jachan · 2025-01-09T17:56:55Z

Rebased onto main, created separate followup ticket for refactor: https://github.com/newjersey/affordability-pm/issues/245. Planning to merge if all tests are green!

jachan commented Dec 26, 2024

View reviewed changes

mmazanec22 approved these changes Dec 27, 2024

View reviewed changes

mluedke2 approved these changes Dec 27, 2024

View reviewed changes

mpidcock approved these changes Dec 30, 2024

View reviewed changes

mrotondo approved these changes Jan 8, 2025

View reviewed changes

jachan force-pushed the nj-193-property-tax-none branch from 3021c6f to 42921f0 Compare January 9, 2025 17:55

jachan added 2 commits January 9, 2025 16:09

NJ 193 - Add none of the above option to property tax

c99dfa1

Included changes from past 2 weeks in rebase

705f918

jachan force-pushed the nj-193-property-tax-none branch from 42921f0 to 705f918 Compare January 9, 2025 21:09

jachan merged commit a751e35 into main Jan 9, 2025
7 checks passed

jachan deleted the nj-193-property-tax-none branch January 9, 2025 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NJ 193 - Add none of the above option to property tax #5295

NJ 193 - Add none of the above option to property tax #5295

jachan commented Dec 26, 2024

jachan Dec 26, 2024

jachan Dec 26, 2024

jachan Dec 26, 2024

jachan Dec 26, 2024

jachan Dec 26, 2024

jachan Dec 26, 2024

mmazanec22 Dec 27, 2024

mluedke2 Dec 27, 2024

github-actions bot commented Dec 26, 2024

jachan Dec 26, 2024

mmazanec22 Dec 27, 2024

mmazanec22 left a comment

mmazanec22 Dec 27, 2024

mluedke2 Dec 27, 2024

mmazanec22 Dec 27, 2024

mmazanec22 Dec 27, 2024

mluedke2 left a comment

mpidcock left a comment

mpidcock Dec 30, 2024

mrotondo left a comment

jachan commented Jan 9, 2025

NJ 193 - Add none of the above option to property tax #5295

NJ 193 - Add none of the above option to property tax #5295

Conversation

jachan commented Dec 26, 2024

Link to pivotal/JIRA issue

Is PM acceptance required? (delete one)

What was done?

How to test?

Screenshots (for visual changes)

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

github-actions bot commented Dec 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmazanec22 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

mluedke2 left a comment

Choose a reason for hiding this comment

mpidcock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrotondo left a comment

Choose a reason for hiding this comment

jachan commented Jan 9, 2025