Independent cities #3603
Independent cities #3603
Conversation
| "Other US County, other US region", other US region |
There was a problem hiding this comment.
Actually, this probably shouldn't be a County in the database. We should have separate logic to ensure this always comes last. Otherwise it'll be in a weird middle-of-the-list. Technically we can just move this line to the end of this CSV, but my hunch is there'll be some ordering tomfoolery which will make it not always come last.
There was a problem hiding this comment.
Hmm. I see what you're saying -- though with a list this long most (I'm not going to be putting a number on it) people are going to be accessing their counties by typing ahead, so I'm not sure it's that big an issue.
Honestly, I had not noticed that I wasn't ordering alpha before displaying the list -- probably because the original list was in strict alpha. I'm going to fix that.
I also thought about whether we could just remove "Other US county" if we have total US coverage. But we might have a case where someone has 2% spread over several counties, and would want to just use it for that.
There was a problem hiding this comment.
And moving it to the end wouldn't work -- it's already in the db, after all, so if it's ordered by creation, it would be weirdly at the end of the counties but before the cities, and if it's alpha, it will be in the middle.
There was a problem hiding this comment.
We could add an additional ordering priority, set everything else to something low, and Other to 9999, and then order by alpha within the ordering value . I think it's a mite kludgey, but if keeping Other at the end is important, that might be the easiest way.
There was a problem hiding this comment.
@dorner -- What do you think of that? It would also have the advantage of allowing us to put all the US counties first if we wanted to if/when we add other countries (and we do have a Toronto bank that has signed up)
There was a problem hiding this comment.
I don't think that makes a lot of sense... maybe add a type/metadata/category etc. column which we can use to group results. Then you can do a sort by category + name. And if Other has its own category then we can ensure it shows up the right way.
There was a problem hiding this comment.
Ok. Allows the same results-ish but with more sophistication. Assuming support doesn't blow up I'll try to get that in tomorrow.
There was a problem hiding this comment.
Also noticed I had a leading space on the regions. Fixed that.
| @@ -0,0 +1,5 @@ | |||
| class AddCategoryToCounties < ActiveRecord::Migration[7.0] | |||
| def change | |||
| add_column :counties, :category, :string | |||
There was a problem hiding this comment.
This is really an enum rather than a string - can we do it this way? https://blog.saeloun.com/2022/01/12/rails-7-adds-custom-enum-support-in-postgresql/#after
There was a problem hiding this comment.
It seems that we can.
Co-authored-by: Daniel Orner <daniel.orner@flipp.com>
lib/tasks/load_us_counties.rake
Outdated
| counties<< {name: row[0], region: row[1]} | ||
| CSV.foreach("lib//assets//us_county_and_equivalent_list.csv") do |row| | ||
| category = row[2] ? row[2].strip.presence : "US_County" | ||
| counties << { name: row[0], region: row[1].strip, category: category } |
There was a problem hiding this comment.
Spacing is off here? :) Otherwise looks good!
There was a problem hiding this comment.
That darn rubocop -- just when you start relying on it!
|
Done and done! |
Resolves #3584
Description
The initial counties implementation didn't include the independent cities of Baltimore, St. Louis, Carson City, and a slew in Virginia. This addresses that.
Type of change
How Has This Been Tested?
manual verification that Baltimore is in the county lists
On copy of production, checked one partner that has served areas. Same before and after.