Nj 75 medical #4869

mmazanec22 · 2024-10-17T15:38:00Z

Link to pivotal/JIRA issue

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

Is PM acceptance required? (delete one)

Yes - don't merge until JIRA issue is accepted!

What was done?

Add medical expenses column
Add calculated medical expenses deduction
Include medical expenses deduction in line 38

There are a few issues that need to be resolved in this PR or a subsequent one:

~~The help text and label overlapping (we could do a quick fix with css obviously, but I would rather figure out why this is happening and fix that)~~ Saw that this is also corrected in honeycrisp compact, so I corrected it for the whole application
~~The placeholder text returning to 0.00 even though I gave an empty placeholder to the currency field~~ should be fixed
We need to update honeycrisp so we can use the accessible reveal component here and elsewhere

How to test?

Select any directfile import and proceed to the medical expenses screen. Enter a value that is greater than 2% of gross income. When you get to the review screen, you should see medical expenses and be able to edit them.

A note for screen reader testing: one change I made here is on the review screen, where I included sr-only text to distinguish the medical expenses edit link from the others. We can use this pattern going forward and change it when we do the sundry a11y bugs ticket. The review screen should also use headers instead of styled text to indicate sections.

Screenshots (for visual changes)

https://github.com/user-attachments/assets/51d9a3d5-b81d-43c4-99e9-543ff8b06566

github-actions · 2024-10-17T15:42:33Z

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

aloverso

Looks good! Left a couple comments, and looks like a couple failing tests and line_data needs to be updated.

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

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

config/locales/en.yml

db/migrate/20241007210254_add_medical_expenses_to_nj_intake.rb

spec/forms/state_file/nj_medical_expenses_form_spec.rb

aloverso · 2024-10-17T16:12:51Z

app/forms/state_file/nj_medical_expenses_form.rb

+                       :medical_expenses
+
+    validates_numericality_of :medical_expenses, only_integer: true, message: :round_to_whole_number, if: -> { medical_expenses.present? }
+    validates :medical_expenses, presence: true, allow_blank: false, numericality: { greater_than_or_equal_to: 0 }, if: -> { medical_expenses.present? }


[dust] I'm a little confused by the validating presence-true if present. It seems like nil is a valid value here, ie, they could move forward with an empty field. Is that correct?

If so, might be nice to have an explicit test to document that nil is a valid value

The way I understand it - if medical_expenses.present? THEN presence: true, allow_blank: false, greater_than_or_equal_to: 0. Melanie can you confirm?

I tested something similar here: https://github.com/codeforamerica/vita-min/pull/4856/files#diff-62e5f3eca04b36aa7e3dd487d951844767312fbae9e56015100adc09a80f5a57, and Kevin from CFA recommended shoulda-matchers as a good way to test these.

Currently this defaults to 0. This could be a good idea because of the requirement to show medical expenses on the review screen regardless of value and let the user go back and edit.

However, we can change it to allow blank, default to nil, and just show a 0 on the review screen if there is no value.

Maybe we should make some blanket decisions about defaults vs nil and add them to our internal eng practices doc?

[dust] I am also confused why you have validates :medical_expenses, presence: true ... if: -> { medical_expenses.present? } - is this just "validate true if true"?

I think we have also been wondering about when to allow blank vs requiring a 0 and it's more challenging than I expected to make a blanket decision so we often handle it on a case by case basis :/

spec/lib/efile/nj/nj1040_calculator_spec.rb

spec/lib/pdf_filler/nj1040_pdf_spec.rb

jachan

A few code style things that should be fixed before merging.

jachan · 2024-10-17T17:31:28Z

app/forms/state_file/nj_medical_expenses_form.rb

+                       :medical_expenses
+
+    validates_numericality_of :medical_expenses, only_integer: true, message: :round_to_whole_number, if: -> { medical_expenses.present? }
+    validates :medical_expenses, presence: true, allow_blank: false, numericality: { greater_than_or_equal_to: 0 }, if: -> { medical_expenses.present? }


The way I understand it - if medical_expenses.present? THEN presence: true, allow_blank: false, greater_than_or_equal_to: 0. Melanie can you confirm?

I tested something similar here: https://github.com/codeforamerica/vita-min/pull/4856/files#diff-62e5f3eca04b36aa7e3dd487d951844767312fbae9e56015100adc09a80f5a57, and Kevin from CFA recommended shoulda-matchers as a good way to test these.

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

config/locales/es.yml

db/migrate/20241007210254_add_medical_expenses_to_nj_intake.rb

spec/controllers/state_file/questions/nj_medical_expenses.rb

app/controllers/state_file/questions/nj_medical_expenses_controller.rb

app/lib/efile/nj/nj1040_calculator.rb

mpidcock

Just the tiniest comment, otherwise looks great!

mpidcock · 2024-10-23T00:18:50Z

app/forms/state_file/nj_medical_expenses_form.rb

+                       :medical_expenses
+
+    validates_numericality_of :medical_expenses, only_integer: true, message: :round_to_whole_number, if: -> { medical_expenses.present? }
+    validates :medical_expenses, presence: true, allow_blank: false, numericality: { greater_than_or_equal_to: 0 }, if: -> { medical_expenses.present? }


[dust] I am also confused why you have validates :medical_expenses, presence: true ... if: -> { medical_expenses.present? } - is this just "validate true if true"?

I think we have also been wondering about when to allow blank vs requiring a 0 and it's more challenging than I expected to make a blanket decision so we often handle it on a case by case basis :/

mmazanec22 force-pushed the nj-75-medical branch from 4a5401a to f6e0b1e Compare October 17, 2024 15:46

mmazanec22 marked this pull request as draft October 17, 2024 15:47

aloverso reviewed Oct 17, 2024

View reviewed changes

jachan approved these changes Oct 17, 2024

View reviewed changes

mmazanec22 force-pushed the nj-75-medical branch from 73d536e to 3cae308 Compare October 17, 2024 20:50

mmazanec22 marked this pull request as ready for review October 18, 2024 15:18

mmazanec22 force-pushed the nj-75-medical branch 3 times, most recently from b003088 to 6a5d5b0 Compare October 21, 2024 13:30

mpidcock approved these changes Oct 23, 2024

View reviewed changes

aloverso mentioned this pull request Oct 23, 2024

NJ 76 - sales and use tax question & calculation #4899

Merged

mmazanec22 added 5 commits October 23, 2024 15:39

Add medical expenses to nj intake

6d3611a

Correct description formatting

a993061

Make description paragraphs rather than a list

32cfebc

Move do not claim text and adjust help text spacing

e12dfcd

Remove confusing conditional in medical expenses form

9eec5aa

mmazanec22 force-pushed the nj-75-medical branch from 211bf0e to 9eec5aa Compare October 23, 2024 19:39

mmazanec22 merged commit 2c5d650 into main Oct 23, 2024
7 checks passed

mmazanec22 deleted the nj-75-medical branch October 23, 2024 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nj 75 medical #4869

Nj 75 medical #4869

mmazanec22 commented Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024

aloverso left a comment

aloverso Oct 17, 2024

jachan Oct 17, 2024

mmazanec22 Oct 18, 2024

mpidcock Oct 23, 2024

jachan left a comment

jachan Oct 17, 2024

mpidcock left a comment

mpidcock Oct 23, 2024

Nj 75 medical #4869

Nj 75 medical #4869

Conversation

mmazanec22 commented Oct 17, 2024 • edited Loading

Link to pivotal/JIRA issue

Is PM acceptance required? (delete one)

What was done?

How to test?

Screenshots (for visual changes)

github-actions bot commented Oct 17, 2024

aloverso left a comment

Choose a reason for hiding this comment

aloverso Oct 17, 2024

Choose a reason for hiding this comment

jachan Oct 17, 2024

Choose a reason for hiding this comment

mmazanec22 Oct 18, 2024

Choose a reason for hiding this comment

mpidcock Oct 23, 2024

Choose a reason for hiding this comment

jachan left a comment

Choose a reason for hiding this comment

jachan Oct 17, 2024

Choose a reason for hiding this comment

mpidcock left a comment

Choose a reason for hiding this comment

mpidcock Oct 23, 2024

Choose a reason for hiding this comment

mmazanec22 commented Oct 17, 2024 •

edited

Loading