Mark contact rollups as teachers based on forms, and other fixes #13767
Conversation
There was a problem hiding this comment.
Backing up a step, as these queries get more and more complex, I'm increasingly worried about readability, testing, and maintainability (as @aoby has already raised). Any thoughts about this?
lib/cdo/contact_rollups.rb
Outdated
| # Values of forms.kind field we care about | ||
| FORM_KINDS = %w(BringToSchool2013 CSEdWeekEvent2013 DistrictPartnerSubmission HelpUs2013 Petition K5OnlineProfessionalDevelopmentPostSurvey).freeze | ||
| # Values of forms.kind field with form data we care about | ||
| FORM_KINDS_WITH_DATA = %w(BringToSchool2013 CSEdWeekEvent2013 DistrictPartnerSubmission HelpUs2013 Petition K5OnlineProfessionalDevelopmentPostSurvey).freeze |
There was a problem hiding this comment.
nit: Long line (over 80 chars). Reformat as
FORM_KINDS_WITH_DATA = %w(
BringToSchool2013
CSEdWeekEvent2013
DistrictPartnerSubmission
HelpUs2013
Petition
K5OnlineProfessionalDevelopmentPostSurvey
).freeze
There was a problem hiding this comment.
nit: Also, the variable name isn't so descriptive. What kind of data? All form kinds have data.
There was a problem hiding this comment.
Thx, have split the line. Will think about a better variable name (but at least it is well described in the comment!).
lib/cdo/contact_rollups.rb
Outdated
| 'PLP interest form','Teacher interest form','School interest form','HelpUs2013', | ||
| 'K5OnlineProfessionalDevelopmentPostSurvey','K5ProfessionalDevelopmentSurvey', | ||
| 'ProfessionalDevelopmentWorkshop','ProfessionalDevelopmentWorkshopSignup', | ||
| 'StudentNomination','TeacherNomination'" |
There was a problem hiding this comment.
nit: Long line (over 80 chars). Also I think this would be cleaner as an array of strings, that's gets joined for any queries.
There was a problem hiding this comment.
nit: This seems more naturally named FORM_KINDS_TEACHER.
There was a problem hiding this comment.
Thx, have split the line and taken your rename suggestion. Will consider changing to string array in the future. (Since it is only used in one place and would have to rejoin it into this form for the one place it is used, not sure the dynamic reassembly is a net win for code complexity.)
There was a problem hiding this comment.
I think Asher was suggesting something like this:
FORM_LIST_TEACHER = [
'BringToSchool2013',
'ClassSubmission',
'DistrictPartnerSubmission',
'PLP interest form',
'Teacher interest form',
'School interest form',
'HelpUs2013',
'K5OnlineProfessionalDevelopmentPostSurvey',
'K5ProfessionalDevelopmentSurvey',
'ProfessionalDevelopmentWorkshop',
'ProfessionalDevelopmentWorkshopSignup',
'StudentNomination',
'TeacherNomination'
].join(',').freeze
This generates essentially the same string in the the constant, but without whitespace, and the code is more readable and maintainable (easier to remove or insert a new form type).
| # returns left most 255 characters of string if non-nil, otherwise nil | ||
| def self.truncate_or_nil(value) | ||
| value.present? ? value[0...255] : nil | ||
| end |
There was a problem hiding this comment.
That was @aoby's suggestion :-)
This PR makes the following improvements/fixes to the contact rollups process:
Mark a contact in the Teacher role not only if they have a dashboard teacher account, but also if they have submitted one of the forms in the
FORM_LIST_TEACHERlist, or filled out a form with the roleeducator(notably the Petition form). This matches the SOLR query we have been doing to find teachers to mail to. If we didn't perform this logic in the rollup process, then every teacher query in Pardot would be super complicated and error-prone.Mark contacts coming from
pd_enrollmentstable as in the Teacher role. We had been generating a handful of contacts in no role at all. It turns out this was people inpd_enrollmentswho did not appear in any other tables (for instance, no Code Studio teacher login under that email). With this change, in a production dry run all of our ~3M contacts were marked with at least one role in the rollup.Bug fix for correct address data. When we use address data present in a form, we need to overwrite all fields, even if the form data is missing some fields. Otherwise we can get IP geo data overlaid with partial user-reported data that is very different. In production, we were getting some users with geo information in the rollup like city: Dubai, state: Dubai, country: United States, zip code: 00971. In that example, the user had IP geo data showing Dubai, United Arab Emirates. But had hand-entered zip code 00971 (Puerto Rico, US) in a form. The partial form data was getting overlaid on top of the IP geo data. We consider the hand-entered data authoritative and need to use that exclusively if any is present (and clear out city and state in this example). This had been working previously but got broken in a simplification change I made.