dev/core#1093 add a bulkCreate action for many customFields in one go #14694

eileenmcnaughton · 2019-07-02T00:15:30Z

Overview

Add bulkCreate function for CustomField with a view towards this being a new protocol for how bulk create actions would look from a code POV which we could expose via apiv4

Before

No function for efficiently creating multiple fields

After

Function exists. There is a view to expose this via apiv4 but at this stage that has been left off to allow 'maturing' - unit test added

Technical Details

Preliminary cleanup changes are split off into smaller commits

#14705
#14697
#14693 (merged)
#14689 (merged)

Comments

I haven't switched Utils_Migrate to use the bulk action but that is the next logical step since it is kinda creating complexity to do 'bulk-action-lite'

https://lab.civicrm.org/dev/core/issues/1093

civibot · 2019-07-02T00:15:32Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

pradpnayak · 2019-07-04T15:00:00Z

This needs rebasing

eileenmcnaughton · 2019-07-04T22:04:47Z

rebased

colemanw · 2019-07-04T23:17:34Z

Api4 signature could look like this: civicrm/org.civicrm.api4#180

colemanw · 2019-07-05T02:33:45Z

@eileenmcnaughton looks like a brittle test fell over on a whitespace change made in this PR:

CRM_Core_BAO_SchemaHandlerTest::testBuildFieldChangeSql
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'ALTER TABLE big_table\n
+'ALTER TABLE big_table \n

eileenmcnaughton · 2019-07-05T02:38:27Z

@colemanw good spotting - that was also what the sub-pr got stuck on - #14727

eileenmcnaughton · 2019-07-05T04:10:26Z

@colemanw now all the preliminary refactoring is done this is pretty simple - I have renamed it to bulkSave per your recommendation

eileenmcnaughton · 2019-07-05T07:11:39Z

test this please

eileenmcnaughton · 2019-07-06T05:31:23Z

@colemanw we are down to one unrelated fail here!

colemanw · 2019-07-06T19:57:01Z

CRM/Core/BAO/CustomField.php

+    $params = [
+      'table_name' => CRM_Core_DAO::getFieldValue('CRM_Core_DAO_CustomGroup', $field->custom_group_id, 'table_name'),
+    ];
+    $sql = str_repeat(' ', 8);


????????

<?php echo str_repeat('?', 8);

yeah weird but retained from 'create'

colemanw · 2019-07-06T19:58:20Z

CRM/Core/BAO/CustomField.php

+      $params = array_merge($defaults, $fieldParams);
+      $operation = empty($params['id']) ? 'add' : 'modify';
+      $customField = self::createCustomFieldRecord($params);
+      // Only doing 'add' for now - so hard coded & no indexExists.


What would it take to get this working with update? Then it would be ready for api4.

oh - I think I mostly altered it but not in this spot - will fix

The updates the way we add bulk fields to custom tables. It uses work I did upstream civicrm/civicrm-core#14694 and ported here https://gerrit.wikimedia.org/r/#/c/wikimedia/fundraising/crm/civicrm/+/520665/ to consolidate DB changes into a single sql call This took about 25 mins on staging to add 'all the fields' but note that this change does not add those fields as yet Update field add Bug: T170972 Change-Id: Ifabe56b4d9956d5a42a03948db20385ab51832f4

This function is only used by one place and feels like the wrong separation of concerns - ie we have a single create action & a multiple create action. They both need to share the generation of the sql to run but can handle the batching of that & rebuilding of triggers etc separately

Add bulkCreate function for CustomField with a view towards this being a new protocol for how bulk create actions would look from a code POV which we could expose via apiv4

eileenmcnaughton · 2019-07-11T01:26:12Z

@colemanw it passed! (& test cover was good - I couldn't sneak anything past it)

colemanw · 2019-07-11T20:05:04Z

CRM/Core/BAO/SchemaHandler.php

@@ -295,7 +295,7 @@ public static function buildForeignKeySQL($params, $separator, $prefix, $tableNa
   * @return bool
   */
  public static function alterFieldSQL($params, $indexExist = FALSE, $triggerRebuild = TRUE) {
-
+    CRM_Core_Error::deprecatedFunctionWarning('function no longer in use / supported');


Probably should add a @deprecated annotation too.

Ok - I'll add that to the last PR (rather than invalidate tests on this)

colemanw · 2019-07-11T20:07:31Z

Code & test coverage looks good. Merging.

…y (ish) I have worked to extract functions upstream to support 'create' and 'bulkCreate' with a view to api support. civicrm#14694 This backports the additional functions to support bulk create (all changes to make this not a duplication of code are upstream but not in this port since it would make this a crazy complex & risky backport) Change-Id: Id437f335d122c993ca6b94d04bd7048c89f991e7

civibot bot added the master label Jul 2, 2019

eileenmcnaughton changed the title ~~[REF] refactors towards supporting a bulkCreate action~~ dev/core#1093 [REF] refactors towards supporting a bulkCreate action Jul 2, 2019

eileenmcnaughton changed the title ~~dev/core#1093 [REF] refactors towards supporting a bulkCreate action~~ [wip] dev/core#1093 [REF] refactors towards supporting a bulkCreate action Jul 2, 2019

eileenmcnaughton force-pushed the cust_field_bulk branch 2 times, most recently from aa7555a to 7084881 Compare July 2, 2019 06:14

eileenmcnaughton changed the title ~~[wip] dev/core#1093 [REF] refactors towards supporting a bulkCreate action~~ dev/core#1093 add a bulkCreate action for many customFields in one go Jul 2, 2019

eileenmcnaughton force-pushed the cust_field_bulk branch from 7084881 to a6abf77 Compare July 2, 2019 09:22

eileenmcnaughton mentioned this pull request Jul 2, 2019

[REF] further simplification of custom field create action #14705

Closed

eileenmcnaughton force-pushed the cust_field_bulk branch 2 times, most recently from 76df6f6 to 861d07d Compare July 3, 2019 22:38

This was referenced Jul 3, 2019

[REF] clarify variable (very minor change with good test cover) #14724

Merged

[REF] extract portion that creates the custom field record #14725

Merged

eileenmcnaughton force-pushed the cust_field_bulk branch 4 times, most recently from 4238cc4 to 0c62271 Compare July 4, 2019 01:11

eileenmcnaughton force-pushed the cust_field_bulk branch from 0c62271 to 1cc807f Compare July 4, 2019 22:04

eileenmcnaughton force-pushed the cust_field_bulk branch from 1cc807f to 9cbea12 Compare July 5, 2019 02:36

eileenmcnaughton force-pushed the cust_field_bulk branch 3 times, most recently from 6b874d1 to b78a8a1 Compare July 5, 2019 04:09

eileenmcnaughton force-pushed the cust_field_bulk branch from b78a8a1 to 473f059 Compare July 5, 2019 04:11

eileenmcnaughton force-pushed the cust_field_bulk branch from 473f059 to 94472dd Compare July 5, 2019 09:50

colemanw reviewed Jul 6, 2019

View reviewed changes

eileenmcnaughton force-pushed the cust_field_bulk branch 3 times, most recently from c2b9d75 to 0638164 Compare July 8, 2019 21:40

eileenmcnaughton force-pushed the cust_field_bulk branch from 0638164 to f928673 Compare July 9, 2019 08:14

eileenmcnaughton force-pushed the cust_field_bulk branch 2 times, most recently from b303735 to d49c025 Compare July 10, 2019 23:15

dev/core#1093 add a bulkSave action for many customFields in one go

e5ce15c

Add bulkCreate function for CustomField with a view towards this being a new protocol for how bulk create actions would look from a code POV which we could expose via apiv4

eileenmcnaughton force-pushed the cust_field_bulk branch from d49c025 to e5ce15c Compare July 11, 2019 00:23

colemanw reviewed Jul 11, 2019

View reviewed changes

eileenmcnaughton added the has-test label Jul 11, 2019

colemanw merged commit 2752a7b into civicrm:master Jul 11, 2019

colemanw deleted the cust_field_bulk branch July 11, 2019 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#1093 add a bulkCreate action for many customFields in one go #14694

dev/core#1093 add a bulkCreate action for many customFields in one go #14694

eileenmcnaughton commented Jul 2, 2019 •

edited

Loading

civibot bot commented Jul 2, 2019

pradpnayak commented Jul 4, 2019

eileenmcnaughton commented Jul 4, 2019

colemanw commented Jul 4, 2019

colemanw commented Jul 5, 2019

eileenmcnaughton commented Jul 5, 2019

eileenmcnaughton commented Jul 5, 2019

eileenmcnaughton commented Jul 5, 2019

eileenmcnaughton commented Jul 6, 2019

colemanw Jul 6, 2019 •

edited

Loading

eileenmcnaughton Jul 6, 2019

colemanw Jul 6, 2019

eileenmcnaughton Jul 6, 2019

eileenmcnaughton commented Jul 11, 2019

colemanw Jul 11, 2019

eileenmcnaughton Jul 11, 2019

colemanw commented Jul 11, 2019

dev/core#1093 add a bulkCreate action for many customFields in one go #14694

dev/core#1093 add a bulkCreate action for many customFields in one go #14694

Conversation

eileenmcnaughton commented Jul 2, 2019 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Jul 2, 2019

pradpnayak commented Jul 4, 2019

eileenmcnaughton commented Jul 4, 2019

colemanw commented Jul 4, 2019

colemanw commented Jul 5, 2019

eileenmcnaughton commented Jul 5, 2019

eileenmcnaughton commented Jul 5, 2019

eileenmcnaughton commented Jul 5, 2019

eileenmcnaughton commented Jul 6, 2019

colemanw Jul 6, 2019 • edited Loading

Choose a reason for hiding this comment

eileenmcnaughton Jul 6, 2019

Choose a reason for hiding this comment

colemanw Jul 6, 2019

Choose a reason for hiding this comment

eileenmcnaughton Jul 6, 2019

Choose a reason for hiding this comment

eileenmcnaughton commented Jul 11, 2019

colemanw Jul 11, 2019

Choose a reason for hiding this comment

eileenmcnaughton Jul 11, 2019

Choose a reason for hiding this comment

colemanw commented Jul 11, 2019

eileenmcnaughton commented Jul 2, 2019 •

edited

Loading

colemanw Jul 6, 2019 •

edited

Loading