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.

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

CRv2: Process postal_code attribute #35538

Merged
merged 4 commits into from Jun 30, 2020
Merged

Conversation

hacodeorg
Copy link
Contributor

@hacodeorg hacodeorg commented Jun 27, 2020

PLC-918: Use contact's postal_code to set Pardot prospect's db_Postal_Code.

  • Contact's postal_code value is calculated from dashboard.users#postal_code,pegasus.form_geos#postal_code and dashboard.schools#zip.
  • Prospect's db_Postal_Code is a text attribute and should show a single value.

Links

Speadsheet with all attributes to process.

Test story

See #35508.

@hacodeorg hacodeorg marked this pull request as ready for review June 27, 2020 00:55
@hacodeorg hacodeorg requested review from bencodeorg and a team June 27, 2020 00:55

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.

Base automatically changed from ha/cr-process-state to staging June 30, 2020 16:46
@hacodeorg hacodeorg merged commit 37201c9 into staging Jun 30, 2020
@hacodeorg hacodeorg deleted the ha/cr-process-postalcode branch June 30, 2020 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants