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

Change agency_type to a Rails enum #4241

Open
dorner opened this issue Mar 31, 2024 · 21 comments · May be fixed by #4254 or #4931
Open

Change agency_type to a Rails enum #4241

dorner opened this issue Mar 31, 2024 · 21 comments · May be fixed by #4254 or #4931

Comments

@dorner
Copy link
Collaborator

dorner commented Mar 31, 2024

Summary

There are a number of tests that check PartnerProfile#agency_type against a string "Other". Some other tests check the value in an array instead:

Partner::AGENCY_TYPES['OTHER']

This seems to be hiding a hidden issue, which is that this field really should be an enum. We don't need to go all the way to defining a Postgres enum; we can just turn the field into a Rails enum as documented here.

Things to consider

A bonus to this would be moving the AGENCY_TYPES map from the Partner class to the PartnerProfile class, as that's where it's actually used.

Criteria for Completion

  • [] Agency type is a Rails enum.
  • Specs are updated to query e.g. profile.other? instead of checking against any specific string value.
  • All other functionality still works as expected.
@veezus
Copy link
Contributor

veezus commented Apr 1, 2024

I'd like to pick this issue up.

@dorner
Copy link
Collaborator Author

dorner commented Apr 1, 2024

Go for it!

@veezus
Copy link
Contributor

veezus commented Apr 1, 2024

It looks like we're currently storing the agency type description string in the database (not the code). Do we want, as part of this PR, a migration to convert agency types from, for example, Government Agency/Affiliate to GOVT?

@dorner
Copy link
Collaborator Author

dorner commented Apr 2, 2024

Hmm... that's a good question. I think it's definitely a better approach than storing the full string. We'd have to be very careful about the rollout, though.

I think we'd have to do something like allowing both long and short values in the enum hash, then doing the migration, then removing the long values from the hash.

Copy link
Contributor

github-actions bot commented May 3, 2024

This issue is marked as stale due to no activity within 30 days. If no further activity is detected within 7 days, it will be unassigned.

@github-actions github-actions bot added the stale label May 3, 2024
@veezus
Copy link
Contributor

veezus commented May 3, 2024

We're just waiting on a data cleanup (#4261) before we move forward on this one.

Copy link
Contributor

Automatically unassigned after 7 days of inactivity.

@cielf
Copy link
Collaborator

cielf commented May 11, 2024

NDBN sent me back some stuff, but I need to review it -- probably a Monday job at this point.

@cielf cielf added Help Wanted Groomed + open to all! and removed stale labels Oct 11, 2024
@cielf
Copy link
Collaborator

cielf commented Oct 11, 2024

I had provided the requested mapping on #4261 but it hasn't been finished. That would be the next step on this.

@Benjamin-Couey
Copy link
Contributor

May I work on this one?

@cielf
Copy link
Collaborator

cielf commented Jan 7, 2025

Please do.

@github-actions github-actions bot removed the Help Wanted Groomed + open to all! label Jan 7, 2025
@Benjamin-Couey
Copy link
Contributor

Looking at the migration made in #4801, it seems like things are already set up to store the agency type codes (GOVT, CAREER, OTHER, etc.) in the db. Do we also want to have a map from these enumerated codes to the existing full description strings?

@cielf
Copy link
Collaborator

cielf commented Jan 7, 2025

As input into that, I can say that we show the full description strings to the users for selection -- I'm not sure off the top of my head of we ever show the agency type codes to the users -- maybe in the export?

@Benjamin-Couey
Copy link
Contributor

I'll do some digging to get a better sense of how we're currently using the agency type codes and full description strings.

@Benjamin-Couey
Copy link
Contributor

Benjamin-Couey commented Jan 8, 2025

Alright, here's what I found. As far as I can tell, it seems like the type codes aren't used, but it's muddy.

There are a number of places that use the agency_type attribute directly from the database. The various views for CRUD operations on a partner's profile, the PartnerInfoReportService which populates the annual survey report, and the attributes for exporting a partner to a CSV.

The Partner::AGENCY_TYPES mapping is only used in the Partner class itself to compare the full strings to the agency_type attribute, and in the forms to get a list of the available full string options. It isn't ever used to convert a code from the db to a full string.

If we assume the code expects the db to contain the full descriptive names, then all this would be consistent with your recollection that we use the full names in the partner edit form and point to type codes not really being used. However, this would be inconsistent with the migration made in #4801 which seemed to want to move the values in the db from full strings to the type codes.

@cielf
Copy link
Collaborator

cielf commented Jan 8, 2025

The db currently has the full string, but we want to move away from that. #4801 was a cleanup necessary to make that possible. It's a bit of a weird situation because the field used to be free-form.

@cielf
Copy link
Collaborator

cielf commented Jan 8, 2025

@dorner - can you chime in on this?

@dorner
Copy link
Collaborator Author

dorner commented Jan 10, 2025

Yeah - let's create a mapping in code for this (so e.g. "GOVT" => "Government Agency/Affiliate"). Maybe a nice way to do this would be to use i18n for this (in en.yml). I definitely don't want the full strings in the database.

@Benjamin-Couey
Copy link
Contributor

I've found two ways to handle translating the agency_type values into the full descriptive strings within views.

Option one is to rely on Rail's lazy lookup which will look up translation keys based on the file path to the file in which the translation is occurring. In this case, the translation file looks like this

en:
  profiles:
    show:
      *agency_type_map

  partners:
    profiles:
      show:
        agency_information:
          *agency_type_map

And in the views the translation is queried like this

Agency Type: <%= t ".#{partner_profile.agency_type}" %>

Option two is to specify a specific key(s) for the translation in the view, which lets us organize the keys in the translation files however we want. In this case, the translation file looks like this

en:
  partners_profile:
    *agency_type_map

And the views themselves look like

Agency Type: <%= t partner_profile.agency_type, scope: [:partners_profile] %>

Do people have preferences between these two methods? From my perspective, I am leaning towards option two because it avoids:

  • Repeating translations for each view in which agency_type is used
  • Having to refactor the translation keys if the views' name or file structure is refactored

@cielf
Copy link
Collaborator

cielf commented Jan 14, 2025

I would lean that way, too, but it's @dorner's call.

@dorner
Copy link
Collaborator Author

dorner commented Jan 16, 2025

Option 2 is definitely a lot cleaner.

@Benjamin-Couey Benjamin-Couey linked a pull request Jan 17, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment