Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split CbvApplicant into three STI subclasses #451

Merged
merged 3 commits into from
Feb 14, 2025

Conversation

tdooner
Copy link
Contributor

@tdooner tdooner commented Feb 13, 2025

Ticket

Related to FFS-2456. Maybe not completely solving the ultimate purpose of that ticket, but I think this will go far enough towards solving it that we can probably close that ticket out until we hit more pain from these agency-specific configurations.

Changes

Split CbvApplicant into three Single-Table Inheritance (STI) subclasses

These subclasses contain the complexity for validating MA/NYC/Sandbox
sites. The base class contains shared functionality. The underlying
cbv_applicants table still contains all the columns for all the
subclasses :-/.

Context for reviewers

Discussed via Slack and it seems like we agree on this approach. So this is mergeable now (2/14 ❤️ ).

Acceptance testing

  • No acceptance testing needed
    • This change will not affect the user experience (bugfix, dependency updates, etc.)
  • Acceptance testing prior to merge
    • This change can be verified visually via screenshots attached below or by sending a link to a local development environment to the acceptance tester
    • Acceptance testing should be done by design for visual changes, product for behavior/logic changes, or both for changes that impact both.
  • Acceptance testing after merge
    • This change is hard to test locally, so we'll test it in the demo environment (deployed automatically after merge.)
    • Make sure to notify the team once this PR is merged so we don't inadvertently deploy the unaccepted change to production. (e.g. :alert: Deploy block! @ffs-eng I just merged PR [#123] and will be doing acceptance testing in demo - please don't deploy until I'm finished!)

These subclasses contain the complexity for validating MA/NYC/Sandbox
sites. The base class contains shared functionality. The underlying
`cbv_applicants` table still contains all the columns for all the
subclasses :-/.
@tdooner tdooner force-pushed the td/ffs-2456-use-key-value-table-named-wip-sti-approach branch from 89b41c1 to 3123eb7 Compare February 13, 2025 00:51
Also simplify the validation logic by using Rails built-ins.
We don't need the agency prefixes anymore since Rails will automatically
try to look for the superclass's validation error messages.
@tdooner tdooner marked this pull request as ready for review February 14, 2025 18:47
last_name:
blank: Enter the client's last name.
snap_application_date:
invalid_date: Enter today's date or the date you contacted the client.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this would be the error that's given for agencies that are not STI? i.e. sandbox?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it's the default used when an STI subclass doesn't override it (as cbv_applicant/ma and cbv_applicant/nyc both do.)

Copy link
Contributor

@GeorgeCodes19 GeorgeCodes19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I'm a big fan of this approach.

I can see how things can get a bit confusing with using helper methods such as a agency_translation while having multiple language file entries

 pdf:
          agency_header_name:
            ma: Massachusetts Department of Transitional Assistance
            nyc: NYC Human Resources Administration
            sandbox: CBV Test Agency

But also having the STI convention

cbv_applicant/ma:
          attributes:
            agency_id_number:
              blank: Enter a valid agency ID number.
              invalid_format: Agency ID number must be 7 digits.
            beacon_id:
              invalid_format: Your WELID must be 6 characters. It can only include letters and numbers.
            snap_application_date:
              invalid_date: Enter today's date or the date you contacted the client. This date must be today or in the past year.
        cbv_applicant/nyc:

attributes:
agency_id_number:
blank: Enter a valid agency ID number.
invalid_format: Agency ID number must be 7 digits.
beacon_id:
invalid_format: Your WELID must be 6 characters. It can only include letters and numbers.
snap_application_date:
invalid_date: Enter today's date or the date you contacted the client. This date must be today or in the past year.
cbv_applicant/nyc:
Copy link
Contributor

@allthesignals allthesignals Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my MDR sense is that this is "scary," but IDk why — it's unusual. Why does it work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Rails i18n gem is mysterious and important. In this case, it's because the validation error message lookup logic is aware of potential STI ancestors and tries multiple different keys until it finds one that matches.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhh. i see. the inheriting model serializes its name with the slash. fascinating thx

@tdooner
Copy link
Contributor Author

tdooner commented Feb 14, 2025

r.e. @GeorgeCodes19

I can see how things can get a bit confusing with using helper methods such as a agency_translation while having multiple language file entries

Yeah, this is a second pattern for have agency-specific translation overrides. However, we already had this pattern because the snap_application_date helper had different keys for different agencies. I don't love having a second pattern for this, but at least it does match the agencies' attributes pretty cleanly.

@tdooner tdooner merged commit e045d68 into main Feb 14, 2025
16 checks passed
@tdooner tdooner deleted the td/ffs-2456-use-key-value-table-named-wip-sti-approach branch February 14, 2025 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants