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
Add special interface for Colombian Schools #35843
Conversation
Colombian schools identifiers don't align nicely to "city" and "name" concepts, and we want to be able to define the schools very precisely, so we add some special logic for dealing with Colombian schools. Specifically, we make the following changes: 1. **Use dropdowns instead of text inputs.** We have data on all schools in Colombia, so we can use it to generate more standardized data. 2. **Add extra fields for filtering.** Because we're using dropdowns and because there are quite a few schools out there, we want to be able to filter the dropdowns to provide a good user experience. We therefore add two extra fields to make the numbers work better. 3. **Add custom labels.** Because the values we're using for "name" and "city" don't quite align to those concepts, and because we've introduced a couple of new fields, we want to be able to use some Colombian-specific labels. Note that we're still adding these in English and running them through our i18n pipeline even though we expect these to be only used in Colombia.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a comment on how we load the data. I don't have much context on this feature, though. would it be possible to include some before and after screenshots which capture the visible changes?
|
||
class Pd::InternationalOptIn < ApplicationRecord | ||
include Pd::Form | ||
include InternationalOptInPeople | ||
|
||
COLOMBIAN_SCHOOL_DATA = JSON.parse( | ||
File.read( | ||
File.join(Rails.root, 'config', 'colombianSchoolData.json') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use a shorter dataset if CDO.stub_school_data
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could but I'm not convinced we should. My understanding is that option is used to change how we seed our data on US schools specifically, it's not clear to me that it has relevance here. Do you have a specific reason why you'd like to see that change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes - the point is to avoid slowing down the application startup time in environments (currently development, adhoc and test, including drone) which do not need the full file. IIRC dashboard/config/schools.tsv
takes about 60 seconds to seed and is only 3-4x larger than this file, so I think it's worth at least checking that loading and parsing this file doesn't add more than a few seconds to application startup time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that makes sense!
I put together this test:
#!/usr/bin/env ruby
require 'json'
start = Time.now
COLOMBIAN_SCHOOL_DATA = JSON.parse(
File.read(
File.join("dashboard", "config", "colombianSchoolData.json")
)
).freeze
finish = Time.now
puts "JSON loaded in #{finish-start} seconds"
the result on my local machine:
JSON loaded in 0.052831755 seconds
I'd conclude that the time added by this is negligible, and we should be fine to deploy without a flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOL, that seems within the time budget :) thanks for checking! maybe all the DB operations make seeding schools take forever.
Apologies, I see the screen capture now. looking... |
@@ -68,7 +75,7 @@ def validate_required_fields | |||
def self.options | |||
entry_keys = { | |||
gender: %w(male female non_binary not_listed none), | |||
schoolCountry: %w(canada chile israel malaysia mexico thailand), | |||
schoolCountry: %w(canada chile colombia israel malaysia mexico thailand), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the significance of this list of countries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no idea 😄
I was just told we want to add Colombia to the list of countries in the dropdown, and add these custom options for school data for when it's selected.
Colombian schools identifiers don't align nicely to "city" and "name" concepts, and we want to be able to define the schools very precisely, so we add some special logic for dealing with Colombian schools.
Specifically, we make the following changes:
School data was obtained from our school data spreadsheet using the following ruby script:
Links
Testing story
Manually tested:
Also added a unit test
Reviewer Checklist: