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-20459: Actively deprecate CRM_Core_OptionGroup::getValue[Sub PR 1] #12049

Merged
merged 1 commit into from
Apr 30, 2018

Conversation

monishdeb
Copy link
Member

@monishdeb monishdeb commented Apr 29, 2018

Overview

This function is deprecated and needs to be replaced by respective function in CRM_Core_Pseudoconstant. This PR is all about this replacement.

This is a sub PR of #12041


@@ -92,7 +92,7 @@ public static function create(&$params) {
$params['is_default'] = CRM_Utils_Array::value('is_default', $params, FALSE);
$params['is_reserved'] = CRM_Utils_Array::value('is_reserved', $params, FALSE);

$params['label_type_id'] = CRM_Core_OptionGroup::getValue('label_type', 'Event Badge', 'name');
$params['label_type_id'] = CRM_Core_PseudoConstant::getKey('CRM_Core_DAO_PrintLabel', 'label_type_id', 'Event Badge');
Copy link
Member Author

Choose a reason for hiding this comment

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

Cannot be changed to CRM_Core_DAO_PrintLabel as the class don't exist

Copy link
Contributor

Choose a reason for hiding this comment

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

OK - I have tested this line through the civicrm/admin/badgelayout?action=add&reset=1 screen

@@ -162,7 +162,8 @@ public static function buildLayout(&$params) {
$layoutParams = array('id' => $params['badge_id']);
CRM_Badge_BAO_Layout::retrieve($layoutParams, $layoutInfo);

$formatProperties = CRM_Core_OptionGroup::getValue('name_badge', $layoutInfo['label_format_name'], 'name');
$formatProperties = CRM_Core_PseudoConstant::getKey('CRM_Core_DAO_PrintLabel', 'name_badge', $layoutInfo['label_format_name']);

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested this & it actually needs to be

    $formatProperties = CRM_Core_PseudoConstant::getKey('CRM_Core_DAO_PrintLabel', 'label_format_name', $layoutInfo['label_format_name']);

Copy link
Member Author

Choose a reason for hiding this comment

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

@eileenmcnaughton oops my bad .. just updated

@@ -94,7 +94,7 @@ public static function getPetitionSummary($params = array(), $onlyCount = FALSE)

//we only have activity type as a
//difference between survey and petition.
$petitionTypeID = CRM_Core_OptionGroup::getValue('activity_type', 'petition', 'name');
$petitionTypeID = CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', 'Petition');
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok this activity type pattern is really common & familiar so I have just checked that the the value is the value in the relevant 'name' field & it is

@@ -160,7 +160,7 @@ public static function getPetitionSummary($params = array(), $onlyCount = FALSE)
public static function getPetitionCount() {
$whereClause = 'WHERE ( 1 )';
$queryParams = array();
$petitionTypeID = CRM_Core_OptionGroup::getValue('activity_type', 'petition', 'name');
$petitionTypeID = CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', 'Petition');
Copy link
Contributor

Choose a reason for hiding this comment

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

this one looks the same as above & fine

@@ -167,7 +167,7 @@ public static function getSurveySummary($params = array(), $onlyCount = FALSE) {

//we only have activity type as a
//difference between survey and petition.
$petitionTypeID = CRM_Core_OptionGroup::getValue('activity_type', 'petition', 'name');
$petitionTypeID = CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', 'Petition');
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -434,7 +434,7 @@ public function postProcess() {
'street_unit',
'survey_response',
);
if (CRM_Core_OptionGroup::getValue('activity_type', 'WalkList') ==
if (CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', 'WalkList') ==
Copy link
Contributor

Choose a reason for hiding this comment

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

ok - although it would make more sense to be if (CRM_Core_PseudoConstant::getName.... ==== )
However, as is I think I can approve without replicating as this pattern on this field is well known

@@ -456,14 +456,14 @@ public function postProcess() {
),
);
}
elseif (CRM_Core_OptionGroup::getValue('activity_type', 'PhoneBank') ==
elseif (CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', 'PhoneBank') ==
Copy link
Contributor

Choose a reason for hiding this comment

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

ok

$this->_values['activity_type_id']
) {
array_push($displayFields, 'phone');
}
elseif ((CRM_Core_OptionGroup::getValue('activity_type', 'Survey') ==
elseif ((CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', 'Survey') ==
Copy link
Contributor

Choose a reason for hiding this comment

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

ok

$this->_values['activity_type_id']) ||
(CRM_Core_OptionGroup::getValue('activity_type', 'Canvass') ==
(CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', 'Canvass') ==
$this->_values['activity_type_id'])
Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@eileenmcnaughton
Copy link
Contributor

I've checked the first handful & marked those I've checked. I was find to do ones that fitted an existing pattern (CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', ) without testing all of them but I need to figure out somewhere to test the others I think

@@ -162,7 +162,8 @@ public static function buildLayout(&$params) {
$layoutParams = array('id' => $params['badge_id']);
CRM_Badge_BAO_Layout::retrieve($layoutParams, $layoutInfo);

$formatProperties = CRM_Core_OptionGroup::getValue('name_badge', $layoutInfo['label_format_name'], 'name');
$formatProperties = CRM_Core_PseudoConstant::getKey('CRM_Core_DAO_PrintLabel', 'label_format_name', $layoutInfo['label_format_name']);
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested this via the 'Save and Preview' on the badge layout page

@eileenmcnaughton
Copy link
Contributor

@monishdeb what say I figure out how to confirm the priority_id ones & you drop the others out of this PR to get it mergeable. TBH the pcp ones I doubt I am going to want to try to replicate.

@@ -1127,10 +1127,10 @@ public static function getCaseActivity($caseID, &$params, $contactID, $context =
}

if (!empty($dao->priority)) {
if ($dao->priority == CRM_Core_OptionGroup::getValue('priority', 'Urgent', 'name')) {
if ($dao->priority == CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'priority_id', 'Urgent')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

OK - tested - need to view case tab & have some set priorities - & row colour results
screenshot 2018-04-30 19 21 42

$caseActivity['DT_RowClass'] .= " priority-urgent ";
}
elseif ($dao->priority == CRM_Core_OptionGroup::getValue('priority', 'Low', 'name')) {
elseif ($dao->priority == CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'priority_id', 'Low')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ok tested

@@ -155,7 +155,7 @@ public static function endPostProcess(&$form, &$params, $activity) {

$params['status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_status_id', 'Completed');
$activity->status_id = $params['status_id'];
$params['priority_id'] = CRM_Core_OptionGroup::getValue('priority', 'Normal', 'name');
$params['priority_id'] = CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'priority_id', 'Normal');
Copy link
Contributor

Choose a reason for hiding this comment

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

OK - Checked against checked pattern & checked option value

@@ -205,7 +205,7 @@ public static function endPostProcess(&$form, &$params, $activity) {
}
$params['status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_status_id', 'Completed');
$activity->status_id = $params['status_id'];
$params['priority_id'] = CRM_Core_OptionGroup::getValue('priority', 'Normal', 'name');
$params['priority_id'] = CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'priority_id', 'Normal');
Copy link
Contributor

Choose a reason for hiding this comment

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

OK - Checked against checked pattern & checked option value

@@ -160,7 +160,7 @@ public static function endPostProcess(&$form, &$params, $activity) {

$params['status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_status_id', 'Completed');
$activity->status_id = $params['status_id'];
$params['priority_id'] = CRM_Core_OptionGroup::getValue('priority', 'Normal', 'name');
$params['priority_id'] = CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'priority_id', 'Normal');
Copy link
Contributor

Choose a reason for hiding this comment

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

OK - Checked against checked pattern & checked option value

@@ -85,7 +85,7 @@ public static function processSoftContribution($params, $contribution) {
$softParams['pcp_display_in_roll'] = CRM_Utils_Array::value('pcp_display_in_roll', $pcp);
$softParams['pcp_roll_nickname'] = CRM_Utils_Array::value('pcp_roll_nickname', $pcp);
$softParams['pcp_personal_note'] = CRM_Utils_Array::value('pcp_personal_note', $pcp);
$softParams['soft_credit_type_id'] = CRM_Core_OptionGroup::getValue('soft_credit_type', 'pcp', 'name');
$softParams['soft_credit_type_id'] = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_ContributionSoft', 'soft_credit_type_id', 'pcp');
Copy link
Contributor

Choose a reason for hiding this comment

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

OK - checked this in the course of doing a reviewer's commit on #12044

@@ -149,7 +149,7 @@ public static function formatSoftCreditParams(&$params, &$form) {
$honorId = NULL;

// @todo fix use of deprecated function.
$contributionSoftParams['soft_credit_type_id'] = CRM_Core_OptionGroup::getValue('soft_credit_type', 'pcp', 'name');
$contributionSoftParams['soft_credit_type_id'] = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_ContributionSoft', 'soft_credit_type_id', 'pcp');
Copy link
Contributor

Choose a reason for hiding this comment

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

OK - based on this being identical to checked change above

@eileenmcnaughton
Copy link
Contributor

@monishdeb I've checked & approved all of these except the very last one (CRM/Contribute/Form/Contribution/Confirm.php) - if you can drop that out we can merge the rest

@monishdeb
Copy link
Member Author

Thanks @eileenmcnaughton I have moved the changes of Confirm.php to #12050

@eileenmcnaughton
Copy link
Contributor

OK - this is good - there is a version of the Confirm changes in the PR fixing the pcp note field which is up for review so let's close that one for now - it's a lot of work to test so better to have it tested by someone who uses pcps!

@eileenmcnaughton eileenmcnaughton merged commit cf57b96 into civicrm:master Apr 30, 2018
@monishdeb monishdeb deleted the CRM-20459-1 branch April 30, 2018 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants