[FYST-1704] Income/ donation forms: Inaccurate date selection #5570

squanto · 2025-02-12T16:03:57Z

Link to pivotal/JIRA issue

https://codeforamerica.atlassian.net/browse/FYST-1704

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?

Update date accessible validations, so that all incoming parts of a date (day, month, and year) are validated together instead of piece by piece. This fixes cases where users enter invalid dates, like Feb 31st.

github-actions · 2025-02-12T16:08:17Z

Heroku app: https://gyr-review-app-5570-2ce2c8acb364.herokuapp.com/
View logs: heroku logs --app gyr-review-app-5570 (optionally add --tail)

arinchoi03

I think someone else @powersurge360 / @tahsinaislam who have worked on date_accessible in the past should review this but I'm trying to understand the code!

arinchoi03 · 2025-02-13T17:41:27Z

app/models/concerns/date_accessible.rb

+                  send("#{property}_day").to_i,
+              )
+            )
+          end


what happens if day/month/year attribute is missing? (fails the .present? check?) Is there validation further down the line that will give an error that the date is invalid b/c something is missing?

if one of the values is missing, then the date won't get set / updated, and it'll fail the inclusion check in it's model

arinchoi03 · 2025-02-13T17:41:45Z

app/models/az321_contribution.rb

@@ -33,6 +33,5 @@ class Az321Contribution < ApplicationRecord
  validates :date_of_contribution,
    inclusion: {
      in: TAX_YEAR.beginning_of_year..TAX_YEAR.end_of_year
-    },
-    presence: true
+    }


why is the presence check removed?

for this the inclusion validation serves as a presence check as well, as nil values fall outside of the range. It made it so I didn't have to duplicate the error message

arinchoi03 · 2025-02-13T17:42:15Z

app/models/concerns/date_accessible.rb

@@ -4,7 +4,6 @@ module DateAccessible
  TAX_YEAR = Date.new(Rails.configuration.statefile_current_tax_year)

  included do
-    private


what's the significance of making these not private methods?

The methods from the module are class methods, so they can't be private, and I believe this line has no effect

arinchoi03 · 2025-02-13T17:45:48Z

spec/models/concerns/date_accessible_spec.rb

+      subject.expiration_date_day = 21
+      subject.expiration_date_year = nil
+      subject.valid?
+      expect(subject.expiration_date).to eq(Date.new(year, 2, 1))


hmm i don't understand this example -- the year is nil but the date is still set to year -- is it b/c year nil is invalid so therefore it was never written to the expiration date?

also should maybe split into a "it" block to describe what we're trying to test here

that's right, the intent is to say that b/c ``year nil` is invalid, it won't write to the date. I'll split this out for better clarity on what it's testing

mrotondo · 2025-02-13T19:41:06Z

One blocking issue I noticed: This breaks the _year, _month, and _day attr_readers, which means that the forms no longer show the previously entered dates when rendering. To see this, go through a state_id page like /primary-state-id, enter valid issue & expiration dates, continue, then go back and note that the date selects are blank. Happy to pair on this - there's definitely some ruby wizardry at work, since I can't tell what the attr_readers are backed by but they seem to work when they've been used to set the date parts on an in-memory object, so clearly they're backed by something.

Edit: As @arinchoi03 mentioned, @powersurge360 would also be a good pair on this, as he created date_accessible

powersurge360 · 2025-02-13T19:46:29Z

app/models/concerns/date_accessible.rb

-        self.define_method("#{property}_day") do
-          send(property)&.day
-        end
+        attr_reader :"#{property}_month", :"#{property}_year", :"#{property}_day"


I think this is the spot to repair the issue Mike surfaced. I think something like

self.define_method("#{property}_month") do super || send(property)&.month end

should do the trick. Or maybe you'll have to retrieve the ivar directly.

squanto · 2025-02-20T17:27:58Z

With the update to the attr accessors, it appears we're now able to view incorrect fields when rerendering @mrotondo

powersurge360

Looks great. I left two notes but the only one that I feel is a blocker is translating a validation string in the new default validator for the date.

powersurge360 · 2025-02-20T19:53:50Z

app/models/concerns/date_accessible.rb

        self.define_method("#{property}_day") do
-          send(property)&.day
+          self.instance_variable_get("@#{property}_day") || send(property)&.day


This will sidestep any other setters, as opposed to calling super. Intended?

Yes that's the intent - for this setter to default to the instance variable, with a backup of parsing the persisted value

powersurge360 · 2025-02-20T19:56:21Z

app/models/concerns/date_accessible.rb

+          define_method("#{property}_date_valid") do
+            date = send(property)
+            if date.present? && !Date.valid_date?(date.year, date.month, date.day)
+              errors.add(property, :invalid_date, message: "is not a valid calendar date")


If we're doing validation, should be translated.

absolutely, thanks for the catch

Co-authored-by: Hugo Melo <[email protected]>

powersurge360

Issues addressed. Approved.

Co-authored-by: Hugo Melo <[email protected]>

squanto force-pushed the FYST-1704-all-states-income-donation-forms-date-field branch 2 times, most recently from 28a4708 to bc93673 Compare February 12, 2025 17:46

arinchoi03 reviewed Feb 13, 2025

View reviewed changes

powersurge360 reviewed Feb 13, 2025

View reviewed changes

squanto requested review from powersurge360, arinchoi03 and mrotondo February 20, 2025 17:28

squanto force-pushed the FYST-1704-all-states-income-donation-forms-date-field branch from 0b36b7b to 097de8d Compare February 20, 2025 17:28

powersurge360 requested changes Feb 20, 2025

View reviewed changes

squanto added 14 commits February 20, 2025 15:12

Validate when all date parts are set

eb4961e

Co-authored-by: Hugo Melo <[email protected]>

Fix failing spec

792af63

Co-authored-by: Hugo Melo <[email protected]>

normalize i18n

cb411e3

Co-authored-by: Hugo Melo <[email protected]>

Set when not set for initial creation via factory

8651a55

Co-authored-by: Hugo Melo <[email protected]>

Fix model spec

8fc6433

Co-authored-by: Hugo Melo <[email protected]>

Fix failing model spec

fe12345

Co-authored-by: Hugo Melo <[email protected]>

always update val when all 3 are present

0f84752

Co-authored-by: Hugo Melo <[email protected]>

Fix controller spec

a40dae7

Co-authored-by: Hugo Melo <[email protected]>

update specs to use real model

d94b9da

Co-authored-by: Hugo Melo <[email protected]>

test setting and fetching ivars

b05c77c

Co-authored-by: Hugo Melo <[email protected]>

Fix rebase error

fa878c0

Co-authored-by: Hugo Melo <[email protected]>

Normalize translations

f854dd2

Co-authored-by: Hugo Melo <[email protected]>

Normalize translations

cc00a54

Co-authored-by: Hugo Melo <[email protected]>

use translations for error message

fd87c4b

Co-authored-by: Hugo Melo <[email protected]>

squanto force-pushed the FYST-1704-all-states-income-donation-forms-date-field branch from 867a9a4 to fd87c4b Compare February 20, 2025 20:12

Revert unrelated translations

618546d

Co-authored-by: Hugo Melo <[email protected]>

powersurge360 approved these changes Feb 20, 2025

View reviewed changes

empty commit for deploy

6226267

Co-authored-by: Hugo Melo <[email protected]>

squanto force-pushed the FYST-1704-all-states-income-donation-forms-date-field branch from a9e4734 to 4b7f5ba Compare February 21, 2025 15:43

Consistently lint

e164c23

Co-authored-by: Hugo Melo <[email protected]>

squanto force-pushed the FYST-1704-all-states-income-donation-forms-date-field branch from 4b7f5ba to e164c23 Compare February 21, 2025 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FYST-1704] Income/ donation forms: Inaccurate date selection #5570

[FYST-1704] Income/ donation forms: Inaccurate date selection #5570

squanto commented Feb 12, 2025

github-actions bot commented Feb 12, 2025

arinchoi03 left a comment •

edited

Loading

arinchoi03 Feb 13, 2025

squanto Feb 14, 2025

arinchoi03 Feb 13, 2025

squanto Feb 13, 2025

arinchoi03 Feb 13, 2025

squanto Feb 14, 2025

arinchoi03 Feb 13, 2025 •

edited

Loading

squanto Feb 14, 2025

mrotondo commented Feb 13, 2025 •

edited

Loading

powersurge360 Feb 13, 2025 •

edited

Loading

squanto commented Feb 20, 2025

powersurge360 left a comment

powersurge360 Feb 20, 2025

squanto Feb 20, 2025

powersurge360 Feb 20, 2025

squanto Feb 20, 2025

powersurge360 left a comment

[FYST-1704] Income/ donation forms: Inaccurate date selection #5570

Are you sure you want to change the base?

[FYST-1704] Income/ donation forms: Inaccurate date selection #5570

Conversation

squanto commented Feb 12, 2025

Link to pivotal/JIRA issue

Is PM acceptance required? (delete one)

What was done?

github-actions bot commented Feb 12, 2025

arinchoi03 left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arinchoi03 Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrotondo commented Feb 13, 2025 • edited Loading

powersurge360 Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

squanto commented Feb 20, 2025

powersurge360 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

powersurge360 left a comment

Choose a reason for hiding this comment

arinchoi03 left a comment •

edited

Loading

arinchoi03 Feb 13, 2025 •

edited

Loading

mrotondo commented Feb 13, 2025 •

edited

Loading

powersurge360 Feb 13, 2025 •

edited

Loading