dev/core#3692: use buildOptions for activityType, activityStatus where easy. Also a relationshipType #23880

herbdool · 2022-06-24T19:27:51Z

Fixes https://lab.civicrm.org/dev/core/-/issues/3692

This might be hard to test. I focused on the ones that were more straightforward and left the ones that might need more decisions.

civibot · 2022-06-24T19:27:53Z

(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).

CRM/Activity/BAO/Activity.php

herbdool · 2022-06-27T15:56:20Z

I made another pass. I changed some of the "get" to "search" for fetching statuses, where the original code was not including inactive items. And I fixed a few others. In all cases I tried to ensure the original filtering was preserved. Where fetching an array of activity types or statuses was only used to find the label I either left it to fetch all, or switched it to use CRM_Core_Pseudoconstant::getLabel().

There were a couple tricky spots where it either needed to only include case activities or exclude. For that I used CRM_Core_OptionGroup::values() which I saw was being used in a report already. Though it is quite low level I didn't see any warnings about using it or not. And it appears to be cached.

For reference the original CRM_Core_PseudoConstant::activityType() uses this to include/exclude:

$all, defaults to TRUE
$includeCaseActivities, defaults to FALSE
$reset, defaults to FALSE
$returnColumn, defaults to 'label'
$includeCampaignActivities, defaults to FALSE
$onlyComponentActivities, defaults to FALSE

In each case I went over the flags and ensured they would still match. The replacement buildOptions doesn't care about including case/campaign/component activities - they're included by default with 'get' and with 'search' are excluded only if they are disabled or if the component is disabled.

demeritcowboy · 2022-06-27T19:14:09Z

CRM/Report/Form/Activity.php

@@ -564,7 +564,8 @@ public function where($recordType = NULL) {
                empty($this->_params['activity_type_id_value'])
              ) {
                if (empty($this->_params['include_case_activities_value'])) {
-                  $this->activityTypes = CRM_Core_PseudoConstant::activityType(TRUE, FALSE, FALSE, 'label', TRUE);
+                  $componentId = CRM_Core_Component::getComponentID('CiviCase');
+                  $this->activityTypes = CRM_Core_OptionGroup::values('activity_type', FALSE, FALSE, FALSE, " AND v.component_id !={$componentId} OR v.component_id IS NULL");


All of the test fails seem to be here. Because $componentId is null if civicase isn't enabled.

CRM_Report_Form_ActivityTest.testLongCustomFieldNames
CRM_Report_Form_ActivityTest.testTargetAddressFields
api_v3_ReportTemplateTest.testActivityDetails
api_v3_ReportTemplateTest.testActivityDetailsNullFilters
api_v3_ReportTemplateTest.testActivityDetailsContactFilter
api_v3_ReportTemplateTest.testReportTemplateSelectWhere with data set #2
api_v3_ReportTemplateTest.testReportTemplateGetRowsAllReports with data set #2
api_v3_ReportTemplateTest.testReportTemplateGetRowsAllReportsACL with data set #2
api_v3_ReportTemplateTest.testReportTemplateGetStatisticsAllReports with data set #2

Thanks. I'll see if my update fixes that.

demeritcowboy · 2022-07-04T12:06:39Z

CRM/Activity/BAO/Activity.php

@@ -2511,7 +2509,7 @@ public static function getContactActivitySelector(&$params) {
        }

        $activity['activity_date_time'] = CRM_Utils_Date::customFormat($values['activity_date_time']);
-        $activity['status_id'] = $activityStatus[$values['status_id']];
+        $activity['status_id'] = CRM_Core_Pseudoconstant::getLabel('CRM_Activity_BAO_Activity', 'status_id', $values['status_id']);


These aren't the same - it now includes disabled. I personally like that but some people won't.

demeritcowboy · 2022-07-04T12:11:07Z

CRM/Activity/Form/Activity.php

@@ -143,7 +143,7 @@ public function getDefaultEntity() {
  public function setFields() {
    // Remove print document activity type
    $unwanted = CRM_Core_OptionGroup::values('activity_type', FALSE, FALSE, FALSE, "AND v.name = 'Print PDF Letter'");
-    $activityTypes = array_diff_key(CRM_Core_PseudoConstant::ActivityType(FALSE), $unwanted);
+    $activityTypes = array_diff_key(CRM_Activity_BAO_Activity::buildOptions('activity_type_id', 'search'), $unwanted);


These aren't the same. Before it didn't include case activity types and now it does and also all the ones with filter=1.

demeritcowboy · 2022-07-04T12:11:52Z

tests/phpunit/Civi/CCase/SequenceListenerTest.php

@@ -25,7 +25,7 @@ public function setUp(): void {
  }

  public function testSequence() {
-    $actStatuses = array_flip(\CRM_Core_PseudoConstant::activityStatus('name'));
+    $actStatuses = array_flip(\CRM_Activity_BAO_Activity::buildOptions('status_id', 'validate'));


This isn't the same, but for it's use here in the test is ok.

demeritcowboy · 2022-07-04T12:13:16Z

CRM/Contribute/BAO/Contribution.php

@@ -1234,7 +1235,7 @@ public static function deleteContribution($id) {
    $params = [
      'source_record_id' => $id,
      // activity type id for contribution
-      'activity_type_id' => 6,
+      'activity_type_id' => CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', 'Contribution'),


I like the number 6, but ok. (grin)

CRM/Activity/Tokens.php

eileenmcnaughton · 2022-07-10T21:48:30Z

Unfortunately this has gone stale & needs a rebase - I pulled out a handful of small changes into separate commits so this will lose a bit of bulk when it is rebased

…asy. Also a relationshipType

herbdool · 2022-08-10T18:46:15Z

I've rebased this. Though would need to check a few more comments before it's ready.

civicrm-builder · 2023-01-15T07:28:30Z

Can one of the admins verify this patch?

eileenmcnaughton · 2023-11-30T20:23:11Z

CRM/Campaign/Form/Task/Reserve.php

@@ -70,7 +70,7 @@ public function preProcess() {
    CRM_Campaign_BAO_Survey::retrieve($params, $this->_surveyDetails);

    //get the survey activities.
-    $activityStatus = CRM_Core_PseudoConstant::activityStatus('name');


@demeritcowboy these swaps are safe aren't they? This PR is stale & I think turned out to be way trickier than initial appearance but I'm thinking maybe I can salvage some of the bits that are straight foward

Some of them might be.

civicrm#23880 is thoroughly stale so I'm closing it - but just thought I'd rescue a couple of changes These are in the barely maintained legacycustomsearches so of marginal risk & marginal benefit but since I looked maybe no-one else has to ever again...

eileenmcnaughton · 2023-11-30T20:37:55Z

I'm going to close this out because it's stale - it probably stalled on some of the challenges picked up in the review. I did save the last couple of changes in #28384 (just because I randomly started from the bottom)

civicrm#23880 is thoroughly stale so I'm closing it - but just thought I'd rescue a couple of changes These are in the barely maintained legacycustomsearches so of marginal risk & marginal benefit but since I looked maybe no-one else has to ever again...

civibot bot added the master label Jun 24, 2022

demeritcowboy reviewed Jun 24, 2022

View reviewed changes

CRM/Activity/BAO/Activity.php Outdated Show resolved Hide resolved

demeritcowboy changed the title ~~Issue 3692: use buildOptions for activityType, activityStatus where easy. Also a relationshipType~~ dev/core#3692: use buildOptions for activityType, activityStatus where easy. Also a relationshipType Jun 24, 2022

demeritcowboy reviewed Jun 27, 2022

View reviewed changes

demeritcowboy reviewed Jul 4, 2022

View reviewed changes

CRM/Activity/Tokens.php Outdated Show resolved Hide resolved

eileenmcnaughton mentioned this pull request Jul 10, 2022

dev/core#3692 Comment fixes #23979

Merged

herbdool added 3 commits August 10, 2022 14:41

Issue 3692: use buildOptions for activityType, activityStatus where e…

913c24b

…asy. Also a relationshipType

fix some of the status gets to searches and fix some more activity types

513b070

don't check components if CiviCase not enabled

bd9c6f3

demeritcowboy added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Oct 4, 2022

eileenmcnaughton reviewed Nov 30, 2023

View reviewed changes

eileenmcnaughton mentioned this pull request Nov 30, 2023

Remove some calls to deprecated function #28384

Merged

eileenmcnaughton closed this Nov 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#3692: use buildOptions for activityType, activityStatus where easy. Also a relationshipType #23880

dev/core#3692: use buildOptions for activityType, activityStatus where easy. Also a relationshipType #23880

herbdool commented Jun 24, 2022

civibot bot commented Jun 24, 2022

herbdool commented Jun 27, 2022

demeritcowboy Jun 27, 2022 •

edited

Loading

herbdool Jun 28, 2022

demeritcowboy Jul 4, 2022

demeritcowboy Jul 4, 2022

demeritcowboy Jul 4, 2022

demeritcowboy Jul 4, 2022

eileenmcnaughton commented Jul 10, 2022

herbdool commented Aug 10, 2022

civicrm-builder commented Jan 15, 2023

eileenmcnaughton Nov 30, 2023

demeritcowboy Nov 30, 2023

eileenmcnaughton commented Nov 30, 2023

dev/core#3692: use buildOptions for activityType, activityStatus where easy. Also a relationshipType #23880

dev/core#3692: use buildOptions for activityType, activityStatus where easy. Also a relationshipType #23880

Conversation

herbdool commented Jun 24, 2022

civibot bot commented Jun 24, 2022

herbdool commented Jun 27, 2022

demeritcowboy Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

herbdool Jun 28, 2022

Choose a reason for hiding this comment

demeritcowboy Jul 4, 2022

Choose a reason for hiding this comment

demeritcowboy Jul 4, 2022

Choose a reason for hiding this comment

demeritcowboy Jul 4, 2022

Choose a reason for hiding this comment

demeritcowboy Jul 4, 2022

Choose a reason for hiding this comment

eileenmcnaughton commented Jul 10, 2022

herbdool commented Aug 10, 2022

civicrm-builder commented Jan 15, 2023

eileenmcnaughton Nov 30, 2023

Choose a reason for hiding this comment

demeritcowboy Nov 30, 2023

Choose a reason for hiding this comment

eileenmcnaughton commented Nov 30, 2023

demeritcowboy Jun 27, 2022 •

edited

Loading