Regions #1331
Regions #1331
Conversation
app/models/region.rb
Outdated
| region | ||
| end | ||
|
|
||
| def self.backfill! |
There was a problem hiding this comment.
Putting the backfill method here for easier testing.
app/models/region.rb
Outdated
| facility_group_region = create_region_from(source: facility_group, kind: facility_group_kind, parent: org_region) | ||
|
|
||
| facility_group.facilities.group_by(&:zone).each do |block_name, facilities| | ||
| block_region = create_region_from(name: block_name, kind: block_kind, parent: facility_group_region) |
There was a problem hiding this comment.
We probably don't want to create blocks until the data is cleaned up on prod, but I wanted to include it here to see how it would work.
app/models/region_kind.rb
Outdated
| @@ -0,0 +1,14 @@ | |||
| class RegionKind < ApplicationRecord | |||
There was a problem hiding this comment.
This could've been named RegionType, but using the name type tends to conflict w/ Rails STI. This is basically where we define the region hiearchy for each Simple instance.
There was a problem hiding this comment.
Do RegionKinds have to be in the database? I'm wondering if we can just initialize them using some config hash-map instead?
Feels like creating a table and db rows for this might be overkill.
There was a problem hiding this comment.
They do have to be in the DB, for a couple reasons:
- we need to know the canonical structure of Regions for building out the admin UI -- we could infer it from Region paths, but that would be error-prone and dangerous
- the names of Regions will vary between countries. For example, in India we have blocks, in Bangladesh we will have upazila.
app/models/region.rb
Outdated
| end | ||
|
|
||
| def self.backfill! | ||
| instance_kind = RegionKind.find_by!(name: "Instance") |
There was a problem hiding this comment.
Can we use something like Root or RootRegion instead of Instance for these top level RegionKinds? Instance doesn't really convey anything to me semantically.
There was a problem hiding this comment.
Yeah we went back and forth on the name for the top level thing. I'll change it to Root.
"kind" was awkward, and I think as long as we don't have a column named "type" we can avoid the Rails name conflicts with STI
Also maintain the case, and just gsub out chars that are not valid
|
ch1455 |
so we can see the summary view of things
| @@ -0,0 +1,14 @@ | |||
| class RegionType < ApplicationRecord | |||
There was a problem hiding this comment.
Definitely prefer RegionType to RegionKind, but wasn't there some concern about using Type in a model name?
There was a problem hiding this comment.
type as a field name is problematic, but model should be fine.
app/services/region_backfill.rb
Outdated
| block_type = find_region_type("Block") | ||
| facility_type = find_region_type("Facility") | ||
|
|
||
| current_country_name = CountryConfig.current[:name] |
There was a problem hiding this comment.
We may want to ensure this only can only be run in India (especially if dry_run = false), since the Region hierarchy is likely going to be different elsewhere.
Don't want to trigger this accidentally via a migration or manually in other countries.
There was a problem hiding this comment.
👍 added a fail safe check
|
Mostly looks good, may want to be a little extra defensive and ensure this can only be triggered in India envs. |
|
|
||
| def set_path | ||
| self.path = "#{parent.path}.#{name}" | ||
| end |
There was a problem hiding this comment.
Why do we need to do manual parent and path stuff in this class?
There was a problem hiding this comment.
Because these records also need to build their path when they get created. The ltree gem doesn't do that for us, it just adds all the handy finder methods.
| @@ -55,6 +55,10 @@ class CountryConfig | |||
| def self.for(abbreviation) | |||
| CONFIGS[abbreviation] | |||
| end | |||
|
|
|||
| def self.current | |||
| Rails.application.config.country | |||
There was a problem hiding this comment.
Oh... this is... nice...
| friendly_id :name, use: :slugged | ||
|
|
||
| validates :name, presence: true | ||
| validates :slug, presence: true, uniqueness: true |
There was a problem hiding this comment.
Don't think we can enforce uniqueness of the slug, can we? What happens if two blocks have the same name?
There was a problem hiding this comment.
Er, no, they need to be unique actually. FriendlyID should be able to handle name dupes and append a number to get a unique one. We need them unique so that we can use them for URLs and such.
| # Labels must be less than 256 bytes long. | ||
| def name_to_path_label | ||
| name.gsub(/\W/, "_").slice(0, MAX_LABEL_LENGTH) | ||
| end |
There was a problem hiding this comment.
Is this only a convenience method for our backfill, or is it used by ltree under the hood as well?
There was a problem hiding this comment.
Just for our backfill.
Also turn off datadog in test, its throwing errors
Story card: ch1455 https://app.clubhouse.io/simpledotorg/story/1455/create-region-schema-run-dry-run-of-region-backfill-in-prod
Because
We want a clean(er) way to represent hierarchical geographic areas in our domain model. Regions are a way to model Organization -> District (ie FacilityGroup) -> Block -> Facility, and they allow us to use the same object at each level for situations where that is what we want - for example with reporting.
This addresses
Adds a Region model and RegionType model. Adds a backfill with a dry_run mode, and this backfill will not run in production automatically via any migrations for now. We want to run it in dry run mode and then examine the log output to see what happens and what sort of data cleanup is necessary.
Future PRs will fix up the backfill to be more robust as needed, and then wire up the real backfill to run via a data migration.