CRM 18651 - Add in Option Group and soft validation #8503

seamuslee001 · 2016-06-05T23:54:54Z

No description provided.

totten · 2016-06-06T00:07:33Z

During installation and upgrade, we should probably be setting the data_type for a few OptionGroups, e.g.

Installation (e.g. patch xml/templates/civicrm_data.tpl and maybe run regen.sh; if regen.sh is failing, then manually patch sql/civcrm_generated.mysql)
Upgrade (e.g. UPDATE civicrm_option_group SET data_type = 'Integer' WHERE name = 'event_type';)

yashodha · 2016-07-12T11:37:39Z

@seamuslee001 Could you please rebase your PR and also move the upgrade code to 4.7.10.mysql.tpl since that's the next release.

seamuslee001 · 2016-07-13T05:10:46Z

@yashodha have done so now

yashodha · 2016-07-13T08:27:28Z

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

+ADD `data_type` varchar(128) COLLATE utf8_unicode_ci DEFAULT NULL COMMENT 'Data Type of Option Group.';
+UPDATE civicrm_option_group SET `data_type` = 'Integer'
+WHERE name in ('activity_type', 'gender', 'payment_instrument', 'participant_role', 'event_type',
+  'activity_status',);


There is a syntax error - unwanted comma after activity_status.

jitendrapurohit · 2016-07-13T13:17:31Z

CRM/Admin/Form/OptionGroup.php

@@ -71,6 +71,8 @@ public function buildQuickForm() {
      CRM_Core_DAO::getAttribute('CRM_Core_DAO_OptionGroup', 'description')
    );

+    $this->addSelect('Data Type', CRM_Utils_Type::dataTypes(), TRUE);


This doesn't let Option Group Form to load. Form to add option group results into a fatal error.

Cannot determine default entity. CRM_Admin_Form_OptionGroup should implement getDefaultEntity().

AkA84 · 2016-07-20T07:51:34Z

Why allowing the user to create an invalid event type at all? Couldn't the error be caught at form validation? I can still create an event type with value "foo" and get the "Unable to reach the server. Please refresh this page in your browser and try again." error when creating an event of that type

AkA84 · 2016-07-20T08:00:45Z

CRM/Admin/Form/Options.php

@@ -376,6 +376,19 @@ public static function formRule($fields, $files, $self) {
      }
    }

+    $optionGroup = civicrm_api3('OptionGroup', 'get', array(


I would wrap this in a private method to make the code cleaner and more readable (kind of pseudo-code following):

/** * * */ private function getOptionGroupDataType($groupName) { $optionGroup = civicrm_api3('OptionGroup', 'get', array( 'sequential' => 1, 'name' => $groupName, )); return CRM_Core_BAO_OptionGroup::getDataType($optionGroup['id']); } // ... public static function formRule($fields, $files, $self) { $dataType = $self->getOptionGroupDataType($self->_gName); if ($dataType) { // ... } //.. }

AkA84 · 2016-07-20T08:18:23Z

Anyway this PR still has some issues unsolved and the branch has conflicts that need to be fixed if we want to merge it

seamuslee001 · 2016-07-20T23:19:38Z

@AkA84 Hi Allesandro, The idea of soft Validation came from @totten . I would be in favor of more harder failure but Tim suggested that we should do soft validation.

AkA84 · 2016-07-25T09:34:37Z

hi @seamuslee001 , is this ready to be reviewed again?

totten · 2016-07-25T21:10:57Z

+1 for soft-validation / warnings. I'm usually a fan of hard-validation / failures, but in this case soft-validation feels like it has better overall cost/benefit (although I don't have a strict formula to rationalize that).

seamuslee001 · 2016-08-31T00:13:40Z

@AkA84 @totten @jitendrapurohit I've rebased this against master and moved upgrade code to 4.7.12. I would appreciate another review of this with the hope this may get in for .12

jitendrapurohit · 2016-09-02T10:39:22Z

CRM/Admin/Form/Options.php

@@ -376,10 +376,36 @@ public static function formRule($fields, $files, $self) {
      }
    }

+    $optionGroup = $this->getOptionGroupDataType($self->_gName);


I think $this call need to be changed to self:: under static formRule() function to avoid fatal error.

jitendrapurohit · 2016-09-02T10:47:35Z

Found a CiviCRM_API3_Exception: "DB Error: no such field" when editing an event type option after applying the changes.

jitendrapurohit · 2016-09-07T13:05:33Z

CRM/Admin/Form/Options.php

+    $optionGroup = civicrm_api3('OptionGroup', 'get', array(
+      'sequential' => 1,
+      'name' => $optionGroupName,
+    ));


Can we use getFieldValue() directly here to get the group id instead of an API call ? Something like -

$optionGroupId = CRM_Core_DAO::getFieldValue('CRM_Core_DAO_OptionGroup', $optionGroupName, 'id', 'name');

jitendrapurohit · 2016-09-07T13:28:26Z

According to changes done here, we don't build a value text field for activity_type. So, I get an invalid soft validation error on saving an activity_type option value even though there's no value field present on the form.

seamuslee001 · 2016-09-08T22:12:04Z

@jitendrapurohit thanks for those pickups, I will make those fixes and try and work out a solution for activity_type. Will ping you once i have pushed changes

seamuslee001 · 2016-09-08T22:12:17Z

Jenkins test this please

seamuslee001 · 2016-09-09T07:28:15Z

thanks @jitendrapurohit I have fixed up the code now as per your comments and tested locally

… given data type

…ields

Fix up upgrade script and add a system check rerun regen after rebasing WIP on adding data type for option group Further work on soft fail when option value value field doesn't match given data type

Re run regen.sh

…type also minor code changes as suggested on PR

…rray

…rom the check when saving the option value form and re run regen.sh

seamuslee001 · 2016-09-10T22:08:40Z

@jitendrapurohit Jitendra would you be able to test this again?

jitendrapurohit · 2016-09-12T13:24:53Z

@seamuslee001 Yes, I'll test this very soon :-)

jitendrapurohit · 2016-09-13T11:09:08Z

I've tested this on installation/upgradation. Option values are soft validated and problematic values are shown on the status page correctly. From my side, this is ok to merge.

Thanks.

totten · 2016-09-13T22:22:33Z

Jenkins reports one test failure which is pre-existing/not-relevant.

civicrm-builder added the master label Jun 5, 2016

JoeMurray mentioned this pull request Jun 13, 2016

CRM-12252 Add in help_pre and help_post fields for price field options #8521

Merged

seamuslee001 force-pushed the CRM-18651 branch 4 times, most recently from 126efc8 to b9a780e Compare June 18, 2016 02:22

JoeMurray mentioned this pull request Jul 7, 2016

Invitation: CiviCRM 4.7.10 release August 2016 civicrm/release-management#4

Closed

seamuslee001 force-pushed the CRM-18651 branch from b9a780e to 77832a4 Compare July 13, 2016 05:04

yashodha reviewed Jul 13, 2016
View reviewed changes

seamuslee001 force-pushed the CRM-18651 branch from 8978c0e to a6aca73 Compare July 13, 2016 08:50

jitendrapurohit reviewed Jul 13, 2016
View reviewed changes

AkA84 reviewed Jul 20, 2016
View reviewed changes

seamuslee001 force-pushed the CRM-18651 branch 2 times, most recently from 4d189e7 to d4c966c Compare July 21, 2016 00:12

seamuslee001 force-pushed the CRM-18651 branch 3 times, most recently from 2bd9c67 to bc1924b Compare August 31, 2016 00:13

jitendrapurohit reviewed Sep 2, 2016
View reviewed changes

jitendrapurohit reviewed Sep 7, 2016
View reviewed changes

seamuslee001 force-pushed the CRM-18651 branch from c63b34d to 53b3f76 Compare September 9, 2016 07:27

seamuslee001 added 16 commits September 11, 2016 07:53

WIP on adding data type for option group

eaecfa2

Further work on soft fail when option value value field doesn't match…

d67d221

… given data type

Add in setting Data Type when option groups are created from custom f…

d333d95

…ields

Add in updates to install to set data types for some option groups

2487758

Fix up upgrade script and add a system check rerun regen after rebasing WIP on adding data type for option group Further work on soft fail when option value value field doesn't match given data type

Add in updates to install to set data types for some option groups

b8f81d2

Fix up upgrade script and add a system check

e6ff4bd

Fix up Option Group Check

9e48be7

Shift upgrade script to 4.7.10

e567b79

Re run regen.sh

Move getting dataType into own function and add test of loading data …

c62c37c

…type also minor code changes as suggested on PR

Fix up OptionGroup Form

1d4e545

Shift Upgrade code to php for more stability

d0e7e12

Shift upgrade code to 4.7.12 and re run regen.sh

7c61320

move to static call as per comment from Jitendra and re run regen.sh

57aa6ec

Fix variable issue found by Jitendra

d3b018e

Fix upgrade for multilingual and have strings as string rather than a…

a37e4e6

…rray

Improve code as per comment from Jitendra and exclude activity_type f…

89c05af

…rom the check when saving the option value form and re run regen.sh

seamuslee001 force-pushed the CRM-18651 branch from 53b3f76 to 89c05af Compare September 10, 2016 21:58

totten merged commit 121e6fd into civicrm:master Sep 13, 2016

seamuslee001 deleted the CRM-18651 branch April 29, 2019 00:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM 18651 - Add in Option Group and soft validation #8503

CRM 18651 - Add in Option Group and soft validation #8503

seamuslee001 commented Jun 5, 2016

totten commented Jun 6, 2016

yashodha commented Jul 12, 2016

seamuslee001 commented Jul 13, 2016

yashodha Jul 13, 2016 •

edited

Loading

jitendrapurohit Jul 13, 2016

AkA84 commented Jul 20, 2016

AkA84 Jul 20, 2016

AkA84 commented Jul 20, 2016

seamuslee001 commented Jul 20, 2016

AkA84 commented Jul 25, 2016

totten commented Jul 25, 2016

seamuslee001 commented Aug 31, 2016

jitendrapurohit Sep 2, 2016 •

edited

Loading

jitendrapurohit commented Sep 2, 2016

jitendrapurohit Sep 7, 2016

jitendrapurohit commented Sep 7, 2016 •

edited

Loading

seamuslee001 commented Sep 8, 2016

seamuslee001 commented Sep 8, 2016

seamuslee001 commented Sep 9, 2016

seamuslee001 commented Sep 10, 2016

jitendrapurohit commented Sep 12, 2016

jitendrapurohit commented Sep 13, 2016

totten commented Sep 13, 2016

CRM 18651 - Add in Option Group and soft validation #8503

CRM 18651 - Add in Option Group and soft validation #8503

Conversation

seamuslee001 commented Jun 5, 2016

totten commented Jun 6, 2016

yashodha commented Jul 12, 2016

seamuslee001 commented Jul 13, 2016

yashodha Jul 13, 2016 • edited Loading

Choose a reason for hiding this comment

jitendrapurohit Jul 13, 2016

Choose a reason for hiding this comment

AkA84 commented Jul 20, 2016

AkA84 Jul 20, 2016

Choose a reason for hiding this comment

AkA84 commented Jul 20, 2016

seamuslee001 commented Jul 20, 2016

AkA84 commented Jul 25, 2016

totten commented Jul 25, 2016

seamuslee001 commented Aug 31, 2016

jitendrapurohit Sep 2, 2016 • edited Loading

Choose a reason for hiding this comment

jitendrapurohit commented Sep 2, 2016

jitendrapurohit Sep 7, 2016

Choose a reason for hiding this comment

jitendrapurohit commented Sep 7, 2016 • edited Loading

seamuslee001 commented Sep 8, 2016

seamuslee001 commented Sep 8, 2016

seamuslee001 commented Sep 9, 2016

seamuslee001 commented Sep 10, 2016

jitendrapurohit commented Sep 12, 2016

jitendrapurohit commented Sep 13, 2016

totten commented Sep 13, 2016

yashodha Jul 13, 2016 •

edited

Loading

jitendrapurohit Sep 2, 2016 •

edited

Loading

jitendrapurohit commented Sep 7, 2016 •

edited

Loading