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

CRM-21674 allow lat & long to be pre-calculated for proximity query / search #11542

Merged
merged 1 commit into from
Feb 7, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jan 18, 2018

Overview

Allow calling function to calculate the lat & long before calling proximity query and allow that to be entered in the proximity custom search

Before

civicrm_api3('Contact', 'get', ['prox_distance' => 4]);
required address fields like 'prox_city' to calculate the centre point

Proximity search:
screenshot 2018-02-07 15 15 46

After

'prox_geo_code_1' & 'prox_geo_code_2' can be passed in instead of address fields

If people know the geoco-ordinates they can self enter them.

screenshot 2018-02-07 15 16 32

Technical Details

The BAO (& by extension api) supports parameters in contact.get for proximity searching. prox_distance is a required param for this & if submitted the following parameters are used to calculate the centre point

'prox_street_address'
'prox_city'
'prox_postal_code'
'prox_state_province_id'
'prox_country_id'
'prox_state_province' ,
'prox_country'
'prox_distance_unit' ,

I wish to make it such that those parameters are optional & not used if 'prox_geo_code_1' & 'prox_geo_code_2' are supplied.

Comments

The immediate use case is to get a unit test added & allow people to search if their geocoder won't accurately find their co-ordinates

Later I also want this toimprove the code in the custom proximity search so that it can use the main group filter (currently smart groups don't work)

b) add the UI option of specifying lat & long rather than an address where preferred (not all coordinates have an address!)



if (!is_numeric(CRM_Utils_Array::value('geo_code_1', $this->_formValues)) ||
!is_numeric(CRM_Utils_Array::value('geo_code_2', $this->_formValues)) ||
!isset($this->_formValues['distance'])
) {
CRM_Core_Error::fatal(ts('Could not geocode input'));
throw new CRM_Core_Exception(ts('Could not geocode input'));
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton should we be translating Exception text? ping @mlutfy

Copy link
Contributor

Choose a reason for hiding this comment

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

No, we shouldn't. I grepped to spot-check, and none of the several examples I reviewed translated the text.

$class::format($params);
// skip_geocode is an optional parameter through the api.
// manual_geo_code is on the contact edit form. They do the same thing....
if (empty($params['skip_geocode']) && empty($params['manual_geo_code'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton, is it expanding scope too much to remove the form-layer logic from this class? Seems like the form should use the skip_geocode parameter rather than invent its own.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you about the form - could just rename the field to 'skip_geocode'. I could possibly expand to it since we are both quite engaged I'm a bit less worried about increasing scope than I normally would be

@eileenmcnaughton eileenmcnaughton changed the title CRM-21674 allow lat & long to be pre-calculated for proximity query CRM-21674 allow lat & long to be pre-calculated for proximity query / search Feb 7, 2018
@eileenmcnaughton
Copy link
Contributor Author

@monishdeb I've rebased this too now - I added a little to it on the form side

@monishdeb
Copy link
Member

Cool, will now review it 👍

@eileenmcnaughton
Copy link
Contributor Author

test this please

@monishdeb
Copy link
Member

Tested on my local works fine. Nice improvement. Liked the idea to use dummy geoprovider CRM_Core_Config::singleton()->geocodeMethod = 'CRM_Utils_MockGeocoder'; for UT 😀

Merging it now.

@monishdeb monishdeb merged commit 361d008 into civicrm:master Feb 7, 2018
@monishdeb monishdeb deleted the prox branch February 7, 2018 13:19
@mlutfy mlutfy added this to the 4.7.31 milestone Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants