Skip to content
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.

Sign up for GitHub

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

Jump to bottom

CRv2: Process postal_code attribute #35538

Merged
merged 4 commits into from
Jun 30, 2020
Merged

CRv2: Process postal_code attribute #35538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3fd5db1
Add extract_postal_code
hacodeorg Jun 27, 2020
ed09f5b
Add tests for extract_postal_code
hacodeorg Jun 27, 2020
e93809c
Test converting postal_code to db_Postal_Code
hacodeorg Jun 27, 2020
c371d98
[skip ui] Merge staging
hacodeorg Jun 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions dashboard/app/models/contact_rollups_processed.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ def self.import_from_raw_table(batch_size = DEFAULT_BATCH_SIZE)
processed_contact_data.merge! extract_roles(contact_data)
processed_contact_data.merge! extract_state(contact_data)
processed_contact_data.merge! extract_city(contact_data)
processed_contact_data.merge! extract_postal_code(contact_data)
processed_contact_data.merge! extract_country(contact_data)
processed_contact_data.merge! extract_updated_at(contact_data)
valid_contacts += 1
Expand Down Expand Up @@ -341,6 +342,18 @@ def self.extract_country(contact_data)
form_geo_country.nil? ? {} : {country: form_geo_country}
end

def self.extract_postal_code(contact_data)
# Priority: school zip > user postal code > form geo postal code
school_zip = extract_field_latest_value contact_data, 'dashboard.schools', 'zip'
return {postal_code: school_zip} if school_zip

user_postal_code = extract_field_latest_value contact_data, 'dashboard.users', 'postal_code'
return {postal_code: user_postal_code} if user_postal_code

form_geo_postal_code = extract_field_latest_value contact_data, 'pegasus.form_geos', 'postal_code'
form_geo_postal_code.nil? ? {} : {postal_code: form_geo_postal_code}
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing this priority pattern for city, state, and postal code makes me wonder if it's time to extract a method for this. But when I try to imagine calling such a method, it's not much cleaner:

def self.extract_postal_code(contact_data)
  postal_code = extract_field_latest_value_with_priority(
    contact_data,
    [
      ['dashboard.schools', 'zip'],
      ['dashboard.users', 'postal_code'],
      ['pegasus.form_geos', 'postal_code']
    ]
  )
  postal_code.nil? ? {} : {postal_code: postal_code}
end

Do you think there's another way to DRY up this code, or is it better to leave it as-is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought is to keep it as-is. My primary concern with this method is performance because it will be called for 4 million rows. Early return may not be the best pattern but it is slightly faster, easier to read and leaves room for custom processing (such as converting state abbreviation to full name if state info comes from dashboard.schools).

Btw, I messed up my PRs by merging them on top of each. The changes in extract_state should not be in this PR at all.


# Extract the latest value of a field in a source table from contact data.
# @param contact_data [Hash] output of the +parse_contact_data+ method
# @param table [String]
Expand Down
2 changes: 2 additions & 0 deletions dashboard/test/models/contact_rollups_pardot_memory_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ class ContactRollupsPardotMemoryTest < ActiveSupport::TestCase
'roles' => 'Form Submitter',
'state' => 'Washington',
'city' => 'Seattle',
'postal_code' => '98101',
'country' => 'United States',
}
refute ContactRollupsPardotMemory.find_by_email(contact.email)
Expand All @@ -147,6 +148,7 @@ class ContactRollupsPardotMemoryTest < ActiveSupport::TestCase
'db_Roles_0' => 'Form Submitter',
'db_State' => 'Washington',
'db_City' => 'Seattle',
'db_Postal_Code' => '98101',
'db_Country' => 'United States',
}
assert_equal expected_data_synced, record[:data_synced]
Expand Down
64 changes: 64 additions & 0 deletions dashboard/test/models/contact_rollups_processed_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ class ContactRollupsProcessedTest < ActiveSupport::TestCase
once.returns({})
ContactRollupsProcessed.expects(:extract_city).
once.returns({})
ContactRollupsProcessed.expects(:extract_postal_code).
once.returns({})
ContactRollupsProcessed.expects(:extract_country).
once.returns({})
ContactRollupsProcessed.expects(:extract_updated_at).
Expand Down Expand Up @@ -420,6 +422,68 @@ class ContactRollupsProcessedTest < ActiveSupport::TestCase
end
end

test 'extract_postal_code' do
base_time = Time.now.utc
form_geos_input = {
'pegasus.form_geos' => {
'postal_code' => [
{'value' => '62702', 'data_updated_at' => base_time - 1.day},
{'value' => '90249', 'data_updated_at' => base_time},
]
}
}
users_input = {
'dashboard.users' => {
'postal_code' => [
{'value' => '10118', 'data_updated_at' => base_time - 1.day},
{'value' => 'EC4N', 'data_updated_at' => base_time},
]
}
}
schools_input = {
'dashboard.schools' => {
'zip' => [
{'value' => '00-493', 'data_updated_at' => base_time},
{'value' => 'EC2P', 'data_updated_at' => base_time - 1.day},
]
}
}

tests = [
{
input: {}, expected_output: {}
},
# data come from the same table, the most recent value wins
{
input: form_geos_input,
expected_output: {postal_code: '90249'}
},
{
input: users_input,
expected_output: {postal_code: 'EC4N'}
},
{
input: schools_input,
expected_output: {postal_code: '00-493'}
},
# users data has higher priority than form_geos data
{
input: form_geos_input.merge(users_input),
expected_output: {postal_code: 'EC4N'}
},
# schools data has higher priority than users data
{
input: form_geos_input.merge(users_input).merge(schools_input),
expected_output: {postal_code: '00-493'}
}
]

tests.each_with_index do |test, index|
output = ContactRollupsProcessed.extract_postal_code test[:input]
assert_equal test[:expected_output], output, "Test index #{index} failed"
end
end

test 'extract_city' do
base_time = Time.now.utc
form_geos_input = {
Expand Down
2 changes: 2 additions & 0 deletions lib/test/cdo/contact_rollups/test_pardot_v2.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ def test_convert_to_pardot_prospect_single_value_attributes
form_roles: 'engineer,teacher',
state: 'Washington',
city: 'Seattle',
postal_code: '98101',
country: 'United States',
},
expected_output: {
Expand All @@ -216,6 +217,7 @@ def test_convert_to_pardot_prospect_single_value_attributes
db_Form_Roles: 'engineer,teacher',
db_State: 'Washington',
db_City: 'Seattle',
db_Postal_Code: '98101',
db_Country: 'United States',
}
},
Expand Down