-
-
Notifications
You must be signed in to change notification settings - Fork 504
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
Comments
I'd like to pick this issue up. |
Go for it! |
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, |
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. |
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. |
We're just waiting on a data cleanup (#4261) before we move forward on this one. |
Automatically unassigned after 7 days of inactivity. |
NDBN sent me back some stuff, but I need to review it -- probably a Monday job at this point. |
I had provided the requested mapping on #4261 but it hasn't been finished. That would be the next step on this. |
May I work on this one? |
Please do. |
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? |
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? |
I'll do some digging to get a better sense of how we're currently using the agency type codes and full description strings. |
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 The 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. |
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. |
@dorner - can you chime in on this? |
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 |
I've found two ways to handle translating the 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
And in the views the translation is queried like this
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
And the views themselves look like
Do people have preferences between these two methods? From my perspective, I am leaning towards option two because it avoids:
|
I would lean that way, too, but it's @dorner's call. |
Option 2 is definitely a lot cleaner. |
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 thePartner
class to thePartnerProfile
class, as that's where it's actually used.Criteria for Completion
profile.other?
instead of checking against any specific string value.The text was updated successfully, but these errors were encountered: