4241 change agency type to enum #4931

Benjamin-Couey · 2025-01-17T17:41:21Z

Resolves #4241

Description

This PR modifies the attribute Partners::Profile.agency_type to be a Rails enum that stores a code-string in the database. The partners_profile.en.yml localization file was also created so I18n may be used to convert from the code-string to a full, descriptive string. These translations were added to existing views.

The existing map of code-strings to descriptive strings, Partners::AGENCY_TYPES, was left as is because it is a dependency of the migration 20241122201255_cleanup_partner_agency_types.rb and removing it would necessitate modifying this old migration, which would cause even worse problems. A warning comment was added to steer people away from using AGENCY_TYPES in the future and other parts of the code which referenced AGENCY_TYPES where updated to reference the new enum.

Lastly, a migration was created to convert existing values of agency_type to the specific values of the enum in cases where the existing value directly maps to one of the enum's values.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Existing tests which referenced agency_type were updated to refer to the enum. No new tests were added.

I manually verified the translations worked in the pages for viewing and updating a partner's profile.

…ptive strings

…PES constant

dorner · 2025-01-17T20:16:30Z

Just a note about

The existing map of code-strings to descriptive strings, Partners::AGENCY_TYPES, was left as is because it is a dependency of the migration 20241122201255_cleanup_partner_agency_types.rb

Honestly? We can just delete that migration from the codebase. It's already run on production and staging, and people's local databases shouldn't be affected.

dorner

Overall looks good! Some requests/comments.

dorner · 2025-01-17T20:17:29Z

app/models/partners/profile.rb

@@ -95,6 +95,17 @@ class Profile < Base
    accepts_nested_attributes_for :served_areas, allow_destroy: true

    has_many_attached :documents
+    enum :agency_type, other: "OTHER", career: "CAREER", abuse: "ABUSE", bnb: "BNB",


Can this be reformatted so each value is on a single line?

Made this change in the recent commits.

dorner · 2025-01-17T20:18:10Z

app/models/partner.rb

    @agency_info = {
      address: [profile.address1, profile.address2].select(&:present?).join(', '),
      city: profile.city,
      state: profile.state,
      zip_code: profile.zip_code,
      website: profile.website,
-      agency_type: (profile.agency_type == AGENCY_TYPES["OTHER"]) ? "#{AGENCY_TYPES["OTHER"]}: #{profile.other_agency_type}" : profile.agency_type
+      agency_type: (symbolic_agency_type == :other) ? "#{Partners::Profile.agency_types[:other]}: #{profile.other_agency_type}" : profile.agency_type


I'm assuming the values here are for display? Then I'd suggest

agency_type: (symbolic_agency_type == :other) ? "Other: #{profile.other_agency_type}" : profile.agency_type

Actually - would we want to do i18n.translate for these values? As far as I can tell this is only used in the CSV export, so we should have readable strings here.

I had originally left profile.agency_type because @cielf had a recollection that the type codes were used in the CSV and some of the full descriptive strings seemed quite long for a CSV column..

Below are some screenshots of the CSV under different conditions. Which looks best?

CSV with full descriptive strings

CSV with full descriptive strings (agency type column fully expanded)

CSV with untranslated type codes

CSV with untranslated type codes, including `other_agency_type` for `:other`

dorner · 2025-01-17T20:20:06Z

app/views/partners/profiles/edit/_agency_information.html.erb

+            <%= profile_form.input :agency_type,
+            # Symbolize keys, because simple_form will only do a I18n lookup for
+            # symbols
+              collection: Partners::Profile.agency_types.symbolize_keys.keys,


Probably a bit more performant to do Partners::Profile.agency_types.keys.map(&:symbolize), but I may be nitpicking here.

Made this change in the recent commits. For my own edification, why was my implementation less performant?

dorner · 2025-01-17T20:21:03Z

db/migrate/20250116180121_convert_partner_agency_types_to_enum.rb

+    profiles = Partners::Profile.all()
+    profiles.each do |profile|


Suggested change

profiles = Partners::Profile.all()

profiles.each do |profile|

Partners::Profile.all.each do |profile|

Made this change in the recent commits.

dorner · 2025-01-17T20:21:14Z

db/migrate/20250116180121_convert_partner_agency_types_to_enum.rb

+      puts profile.id
+      puts current_agency_type


Please remove these puts statements.

Made this change in the recent commits.

dorner · 2025-01-17T20:21:41Z

db/migrate/20250116180121_convert_partner_agency_types_to_enum.rb

+      current_agency_type = profile.read_attribute_before_type_cast(:agency_type)
+      puts profile.id
+      puts current_agency_type
+      if !current_agency_type.nil?


Suggested change

if !current_agency_type.nil?

if current_agency_type.present?

Made this change in the recent commits.

dorner · 2025-01-17T20:22:13Z

db/migrate/20250116180121_convert_partner_agency_types_to_enum.rb

+      if !current_agency_type.nil?
+        # If a profile has a descriptive string (ignoring case) as an agency type
+        # update it to the associated value code.
+        if( string_to_value_mapping.key?( current_agency_type.downcase ) )


Suggested change

if( string_to_value_mapping.key?( current_agency_type.downcase ) )

if string_to_value_mapping.key?(current_agency_type.downcase)

(similarly below - not sure why lint didn't pick this up)

Hmm, for some reason the linter missed this file entirely. It only made corrections when I specified this file explicitly. Regardless, ran this through the inter in the recent commits.

dorner · 2025-01-17T20:22:41Z

spec/models/partner_spec.rb

@@ -294,7 +294,7 @@
    let(:agency_state) { "ND" }
    let(:agency_zipcode) { "09980-7010" }
    let(:agency_website) { "bosco.example" }
-    let(:agency_type) { Partner::AGENCY_TYPES["OTHER"] }
+    let(:agency_type) { Partners::Profile.agency_types[:other] }


Suggested change

let(:agency_type) { Partners::Profile.agency_types[:other] }

let(:agency_type) { :other }

Made this change, and also fixed the test improperly expecting the enum value (OTHER). Might need to update this again depending on what we choose for the CSV export.

cielf · 2025-01-18T00:06:56Z

We'll need me to do some light manual testing when those are all addressed, methinks.

…ntation

…om partner model

…handled by 20241122201255_cleanup_partner_agency_types

Benjamin-Couey · 2025-01-21T16:55:45Z

Just a note about

The existing map of code-strings to descriptive strings, Partners::AGENCY_TYPES, was left as is because it is a dependency of the migration 20241122201255_cleanup_partner_agency_types.rb

Honestly? We can just delete that migration from the codebase. It's already run on production and staging, and people's local databases shouldn't be affected.

Alright, in the most recent commits I removed 20241122201255_cleanup_partner_agency_types.rb and Partners::AGENCY_TYPES. I also copied the mappings from 20241122201255_cleanup_partner_agency_types.rb to the migration added by this PR to preserve the relations and just in case someone hadn't run the now deleted migration.

Benjamin-Couey · 2025-01-21T16:57:43Z

Alright, a bunch of checks are failing post merge. Working on resolving them.

Benjamin-Couey · 2025-01-22T16:06:00Z

Alright, resolved the issues that were causing checks to fail. Now just waiting on @cielf and/or @dorner to make a decisions regarding how we want to style the agency types when exported to csv. After that, I can do another round of manual testing.

cielf · 2025-01-22T16:56:11Z

Sorry I missed that-- this should be as seamless a change as possible for the users, so keep putting out the full text, please.

Benjamin-Couey · 2025-01-23T16:13:48Z

Alright, finished the last round of manual testing. I checked the bank and partner facing view and edit pages, as well as the exported CSV and everything looked good.

Bank facing view page

Partner facing view page

Bank facing edit page

Partner facing edit page with dropdown open

Exported CSV

Benjamin-Couey and others added 11 commits January 10, 2025 06:59

Update agency_type to be an enum

2566e16

Created new I18n config file to translate agency_type codes to descri…

4e156a2

…ptive strings

Applied translation of agency_type code to descriptive strings in views

b8a4d61

Applied translation of agency_type code in partner info report

7c9d8a8

Updated existing tests to refer to enum

9521d48

Updated Partner.agency_info to refer to new enum instead of AGENCY_TY…

f840bb0

…PES constant

Removed unecessary brackets

de2d69f

Added comment warning people away from using AGENCY_TYPES

c5e260d

Created migration to update existing agency_type values to enum values

6c1625c

Changes made by linter

c62835c

Merge branch 'main' into 4241-change-agency-type-to-enum

93feb8f

cielf requested a review from dorner January 17, 2025 18:58

dorner requested changes Jan 17, 2025

View reviewed changes

Benjamin-Couey and others added 8 commits January 18, 2025 10:24

Cleaned up formatting, removed puts statements, ran through linter

ec45472

Reformatted :agency_type so each entry is on one line

21a8cd3

Fixed test using and expecting enum values

f32237d

Changed calculation of agency_type symbols to more performant impleme…

74073cb

…ntation

Changes made by linter

a516804

Removed old cleanup of partner agency types and associated mapping fr…

1f4bd43

…om partner model

Updated mapping in migration to handle agency_type values previously …

596e79c

…handled by 20241122201255_cleanup_partner_agency_types

Merge branch 'main' into 4241-change-agency-type-to-enum

2987a2a

Benjamin-Couey added 2 commits January 22, 2025 07:12

Fixed using wrong function to symbolize strings

b5606af

Updated agency_info to translate agency_types for now

2e5a0a1

Fixed views not comparing agency_type to enum keys

9fc8950

Merge branch 'main' into 4241-change-agency-type-to-enum

723d43d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

4241 change agency type to enum #4931

4241 change agency type to enum #4931

Benjamin-Couey commented Jan 17, 2025

dorner commented Jan 17, 2025

dorner left a comment

dorner Jan 17, 2025

Benjamin-Couey Jan 21, 2025

dorner Jan 17, 2025

dorner Jan 17, 2025

Benjamin-Couey Jan 21, 2025

dorner Jan 17, 2025

Benjamin-Couey Jan 21, 2025

dorner Jan 17, 2025

Benjamin-Couey Jan 21, 2025

dorner Jan 17, 2025

Benjamin-Couey Jan 21, 2025

dorner Jan 17, 2025

Benjamin-Couey Jan 21, 2025

dorner Jan 17, 2025

Benjamin-Couey Jan 21, 2025

dorner Jan 17, 2025

Benjamin-Couey Jan 21, 2025 •

edited

Loading

cielf commented Jan 18, 2025

Benjamin-Couey commented Jan 21, 2025

Benjamin-Couey commented Jan 21, 2025

Benjamin-Couey commented Jan 22, 2025

cielf commented Jan 22, 2025

Benjamin-Couey commented Jan 23, 2025

		profiles = Partners::Profile.all()
		profiles.each do \|profile\|

	profiles = Partners::Profile.all()
	profiles.each do \|profile\|
	Partners::Profile.all.each do \|profile\|

	if( string_to_value_mapping.key?( current_agency_type.downcase ) )
	if string_to_value_mapping.key?(current_agency_type.downcase)

	let(:agency_type) { Partners::Profile.agency_types[:other] }
	let(:agency_type) { :other }

4241 change agency type to enum #4931

Are you sure you want to change the base?

4241 change agency type to enum #4931

Conversation

Benjamin-Couey commented Jan 17, 2025

Description

Type of change

How Has This Been Tested?

dorner commented Jan 17, 2025

dorner 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

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

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

Benjamin-Couey Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

cielf commented Jan 18, 2025

Benjamin-Couey commented Jan 21, 2025

Benjamin-Couey commented Jan 21, 2025

Benjamin-Couey commented Jan 22, 2025

cielf commented Jan 22, 2025

Benjamin-Couey commented Jan 23, 2025

Benjamin-Couey Jan 21, 2025 •

edited

Loading