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

Refactor Venue::Geocoder #471

Merged
merged 6 commits into from Sep 28, 2015
Merged

Conversation

botandrose
Copy link
Contributor

@botandrose botandrose commented Sep 25, 2015

Split up complex #geocode method into a composed method.


def map_geo_to_venue
VENUE_GEO_FIELD_MAP.each do |venue_field, geo_field|
next if venue[venue_field].present?
Copy link
Member

@reidab reidab Sep 25, 2015

Choose a reason for hiding this comment

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

This deviates from the original implementation, in that we used to write lat and lng even when values were present on the venue. I think this is required for the "force geocoding" feature to work correctly.

Copy link
Contributor Author

@botandrose botandrose Sep 25, 2015

Choose a reason for hiding this comment

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

Ah, good catch. This requirement should be captured in a unit test. I'll go ahead and add one. While we're discussing requirements, why not overwrite every field?

Copy link
Member

@reidab reidab Sep 25, 2015

Choose a reason for hiding this comment

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

I think we're not overwriting all the fields because there are situations where local users will have a better idea how an address should be written than the geocoder will.

Also IIRC, not all of the geocoding backends actually return all the address fields (and this is especially true for international addresses). We probably want to ensure that the address component from geocoder result isn't blank before trying to set it.

Copy link
Contributor Author

@botandrose botandrose Sep 25, 2015

Choose a reason for hiding this comment

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

Roger that. I'll write a unit test to force the original behavior.

Copy link
Contributor Author

@botandrose botandrose Sep 26, 2015

Choose a reason for hiding this comment

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

Okay, old behavior is restored, and I added another unit test to codify the requirement of not stomping on existing values with blank ones from the geocoder.

reidab added a commit that referenced this issue Sep 28, 2015
@reidab reidab merged commit c5df0b1 into calagator:master Sep 28, 2015
2 checks passed
@reidab
Copy link
Member

@reidab reidab commented Sep 28, 2015

🌍

@botandrose botandrose deleted the refactor_venue_geocoder branch Sep 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants