CRM-19464 Make 'Supplemental Address 3' usable #9857

aydun · 2017-02-17T23:38:23Z

Many files are changed by this, but each change is simple. Wherever supplemental_address_2 was used, there is an addition for supplemental_address_3.

CRM-19464: supplemental_address_3 only exists in API

eileenmcnaughton

I've read through the code but not tested / done UI analysis & here are my thoughts

I agree that since the field exists it makes sense to treat it like the other supplemental_addresss fields
I agree the bulk of these changes are clear cut & safe & could be merged based on reading the code, as I have done
There is an addition of these fields to the search that would require UI testing/ consideration, which I have not done
the PR is stale :-(

If it were not for 3 + 4 I would merge this. I would suggest you might put 3 in a separate PR since once 4 is fixed the rest of this could be on the fast-track

eileenmcnaughton · 2017-03-08T11:48:09Z

CRM/Contact/Form/Search/Criteria.php

@@ -297,6 +297,9 @@ public static function location(&$form) {

    $elements = array(
      'street_address' => array(ts('Street Address'), $attributes['street_address'], NULL, NULL),
+      'supplemental_address_1' => array(ts('Supplemental Address 1'), $attributes['supplemental_address_1'], NULL, NULL),


This appears to be a separate change.

Hi @eileenmcnaughton thanks for looking at this. The section you highlighted is a separate change and @yashodha has just merged PR-9850. That change just added supplemental 1&2 to the search. This change builds on that to add supplemental 3 to search, alongside all the other supplemental 3 changes

aydun · 2017-03-08T13:02:14Z

Hmm, I replied to the search criteria issue above and then used the 'Resolve conflicts' button just below which seems to have attempted to merge master into this PR. That does not seem right but could be my limited git fu.

eileenmcnaughton · 2017-03-08T13:38:50Z

git rebase -i is your friend

try

git pull --rebase origin master
git fetch -f origin master
git rebase -i origin/master
git push -f aydun CRM-19464

There is some belt & braces there - the git fetch -f may be not required & I always rebase twice - once 'clean' & the second edited, but it should mostly work.

Other git tips from the CIA are here https://wikileaks.org/ciav7p1/cms/page_1179773.html

aydun · 2017-03-08T14:30:25Z

Many thanks - that worked! Re the changes to xml/templates/civicrm_data.tpl https://github.com/civicrm/civicrm-core/pull/9857/files#diff-cf219e03577e796a89f712d33a769168 - new installs get those values but there is nothing here to update the table for current installs. Should that be part of this PR or is that handled differently?

colemanw · 2017-03-08T16:00:12Z

I agree the code changes look good/safe.

@aydun regarding the option value update, you would need to add an INSERT query to CRM/Upgrade/Incremental/sql/4.7.17.mysql.tpl

aydun · 2017-03-08T21:03:50Z

@colemanw assuming this should be added to 4.7.18.mysql.tpl not 4.7.17.mysql.tpl? Is that update/insert approach ok or is delete/insert better?

eileenmcnaughton · 2017-03-08T21:05:35Z

I'm going to merge this & leave you to add the upgrade fix as a follow up. I think there is a php ensureOptionValue exists function to cope with people who have self-added in advance

colemanw · 2017-03-09T02:54:17Z

@aydun looks like the test caught a sql error. In multilingual the label column doesn't exist it's label_en_US etc.

colemanw · 2017-03-09T15:09:50Z

CRM/Upgrade/Incremental/sql/4.7.18.mysql.tpl

+INSERT INTO
+ `civicrm_option_value` (`option_group_id`, {localize field='label'}label{/localize}, `value`, `name`, `grouping`, `filter`, `is_default`, `weight`, `description`, `is_optgroup`, `is_reserved`, `is_active`, `component_id`, `visibility_id`, `icon`)
+VALUES
+  (@option_group_id_adOpt, {localize}'{ts escape="sql"}Supplemental Address 3{/ts}'{/localize}, 4, 'supplemental_address_3', NULL, 0, NULL, 4, NULL, 0, 0, 1, NULL, NULL, NULL);


There is a merge conflict here (due to some other upgrade code that's been merged into this file.
But in general I agree with @eileenmcnaughton we should use the ensureOptionValueExists function instead of raw sql.
Also I'm not sure that it's necessary to renumber the option values. Is it a problem to have them numbered as-is?

I would suggest this should be tacked onto the end of the sequence. The option value in an upgraded db != brand new db

I've eventually got round to digging into this a bit further. The id of the new value doesn't matter, but the weight does so that it displays in the right order. ensureOptionValueExists is a wrapper around civicrm_api3('OptionValue', 'create') If no weight is specified a default weight at the end of the group is assigned, otherwise the specified weight value is used even if it results in duplicates.

If OptionValues are created/reordered with the GUI, the weights of all OptionValues are adjusted so that there are no duplicates, but doing this via the API can result in duplicate weights. There is CRM_Core_OptionValue::addOptionValue() which has more logic around weights which maybe should be called instead from the API or from ensureOptionValueExists but that is straying a long way from the focus of this PR.

So we can use ensureOptionValueExists but it will result in a duplicate weight whereas the SQL I had increments whatever other existing weights there are to make space for the new one.

More generally, with incremental upgrades, how closely are we trying to make an upgraded system match a freshly installed system? So in this case, it makes sense to put supplemental_3 after supplemental_2 in xml/templates/civicrm_data.tpl for new systems. For an upgrade, we can try to match that (the approach I was following), or we can add it to the end of the table as ensureOptionValueExists would do.

I'll change the logic to look for supplemental_2 and set the new weight based on that, but given that the weight is important, how do we proceed?

@aydun So i think the solution here should be is find out the weight of supplemental_2 in systems we are upgrading, Then run a piece of SQL like the following

UPDATE civicrm_option_value SET weight = weight + 1 WHERE option_group_id = <relevant option group> AND weight > <supplemental_2 weight>

Then in your insert get the supplemental_2 weight + 1 as weight

Thanks @seamuslee001 . @eileenmcnaughton, @colemanw do you agree that SQL is the better option here? That's where I started and is in the currently committed code, and then was directed towards ensureOptionValueExists but that has issues with specifying weights.

If you can get it to work without mutlilingual issues go for it. Otherwise you'll need to add some weight fixes in to php.

colemanw · 2017-03-10T22:18:27Z

Sorry but this is stale again.

eileenmcnaughton · 2017-03-11T00:08:34Z

I this stale-making process is too annoying it might help to get all the other changes in this PR merged first. There are 2 highly-stale-prone areas, changes to the DAO & changes to the upgrade script. It might be easier to break it up?

eileenmcnaughton · 2017-04-05T12:22:35Z

@aydun I think it might be worth pulling out the non-upgrade fixes into a separate PR & getting them merged. There are a lot of upgrade & DAO changes happening at the moment so some chunking might help with the staleness issue

In some situations, the address format is too limited. The 'Supplemental Address 3' already exists in the schema but was not exposed to the GUI. This touches many files but the changes are fairly simple. Wherever 'Supplemental Address 2' existed, there is now a 'Supplemental Address 3' as well.

aydun · 2017-04-14T21:00:07Z

I've moved the upgrade steps to PR-10166.

eileenmcnaughton · 2017-04-15T21:46:14Z

ok - got this part merged then!

aydun · 2017-04-15T23:31:36Z

Thanks!

civicrm-builder added the master label Feb 17, 2017

eileenmcnaughton requested changes Mar 8, 2017

View reviewed changes

aydun force-pushed the CRM-19464 branch from 5fbfc94 to 9c87c3a Compare March 8, 2017 14:08

eileenmcnaughton approved these changes Mar 8, 2017

View reviewed changes

aydun force-pushed the CRM-19464 branch from 90149d8 to 99815da Compare March 8, 2017 20:58

eileenmcnaughton added the merge on pass label Mar 8, 2017

colemanw reviewed Mar 9, 2017

View reviewed changes

aydun force-pushed the CRM-19464 branch from 14845ca to 207f62c Compare April 14, 2017 20:12

eileenmcnaughton merged commit 659c02b into civicrm:master Apr 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-19464 Make 'Supplemental Address 3' usable #9857

CRM-19464 Make 'Supplemental Address 3' usable #9857

aydun commented Feb 17, 2017 •

edited by civicrm-builder

Loading

eileenmcnaughton left a comment

eileenmcnaughton Mar 8, 2017

aydun Mar 8, 2017

aydun commented Mar 8, 2017 •

edited

Loading

eileenmcnaughton commented Mar 8, 2017

aydun commented Mar 8, 2017

colemanw commented Mar 8, 2017

aydun commented Mar 8, 2017

eileenmcnaughton commented Mar 8, 2017

colemanw commented Mar 9, 2017

colemanw Mar 9, 2017

seamuslee001 Mar 9, 2017

aydun Apr 1, 2017

seamuslee001 Apr 1, 2017

aydun Apr 5, 2017

eileenmcnaughton Apr 5, 2017

colemanw commented Mar 10, 2017

eileenmcnaughton commented Mar 11, 2017

eileenmcnaughton commented Apr 5, 2017

aydun commented Apr 14, 2017

eileenmcnaughton commented Apr 15, 2017

aydun commented Apr 15, 2017

CRM-19464 Make 'Supplemental Address 3' usable #9857

CRM-19464 Make 'Supplemental Address 3' usable #9857

Conversation

aydun commented Feb 17, 2017 • edited by civicrm-builder Loading

eileenmcnaughton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aydun commented Mar 8, 2017 • edited Loading

eileenmcnaughton commented Mar 8, 2017

aydun commented Mar 8, 2017

colemanw commented Mar 8, 2017

aydun commented Mar 8, 2017

eileenmcnaughton commented Mar 8, 2017

colemanw commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colemanw commented Mar 10, 2017

eileenmcnaughton commented Mar 11, 2017

eileenmcnaughton commented Apr 5, 2017

aydun commented Apr 14, 2017

eileenmcnaughton commented Apr 15, 2017

aydun commented Apr 15, 2017

aydun commented Feb 17, 2017 •

edited by civicrm-builder

Loading

aydun commented Mar 8, 2017 •

edited

Loading