Fix activity view bug where an activity type id in the url overrides … #27496

eileenmcnaughton · 2023-09-18T21:37:04Z

Overview

Fix activity view bug where an activity type id in the url overrides the actual activity type id

When we view an activity there is code in the template that renders differently depending on the activity type id. However, if the activity ID (atype) is wrong in the url it gives the url precedence

This is a bit obscure - I hit it when I changed the activity type id in the database & the view did not update

Before

Create an activity with details

-ALTERNATIVE ITEM 0-
Hi,

Wassup!?!?

Let's check if the output when viewing the form has legible line breaks in the output.

Thanks!

-ALTERNATIVE ITEM 1-

<div dir="ltr">Hi,<br></div>
<div dir="ltr"><br></div>
<div dir="ltr">Wassup!?!?<br></div>
<div dir="ltr"><br></div>
<div dir="ltr">Let&#39;s check if the output when viewing the form has legible line breaks in the output.<br></div>
<div dir="ltr"><br></div>
<div dir="ltr">Thanks!<br></div>
-ALTERNATIVE END-

(might need to save directly to DB UPDATE - no through the UI works)

View the activity - it looks like

Alter the activity view url - put the activity type ID for pdf letter (default install = 22 ) into the url as atype - you get the unadulterated version

After

The activity type ID comes from the activity not the url

Technical Details

A bit of minor clean up too

Comments

civibot · 2023-09-18T21:37:06Z

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

➡️ PR Demo site (can be rebuilt with the command test this please).
📖 Review standards
🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

eileenmcnaughton · 2023-09-18T21:50:17Z

CRM/Activity/Form/Activity.php

@@ -65,7 +69,6 @@ class CRM_Activity_Form_Activity extends CRM_Contact_Form_Task {
   */
  protected $_sourceContactId;
  protected $_targetContactId;
-  protected $_asigneeContactId;


eileenmcnaughton · 2023-09-18T21:51:03Z

CRM/Activity/Form/Activity.php

@@ -265,33 +268,20 @@ public function preProcess() {

    $this->assign('context', $this->_context);

-    $this->_action = CRM_Utils_Request::retrieve('action', 'String', $this);
+    $this->setAction(CRM_Utils_Request::retrieve('action', 'String', $this));


Once #27495 is merged it will cast to an int & we can be confident it is numeric

demeritcowboy · 2023-09-18T22:21:02Z

I can review this but there's a lot going on here and if I remember right this area was a bit fraught with danger. And I think case activity calls this too.

eileenmcnaughton · 2023-09-18T22:55:59Z

@demeritcowboy yeah - your tests found the danger - I'm pulling it back ...

eileenmcnaughton · 2023-09-18T23:06:28Z

templates/CRM/Activity/Form/ActivityView.tpl

@@ -9,7 +9,7 @@
 *}
 <div class="crm-block crm-content-block crm-activity-view-block">
      {if $activityTypeDescription}
-        <div class="help">{$activityTypeDescription}</div>
+        <div class="help">{$activityTypeDescription|escape}</div>


no direct reason - but the temptation to change how this is loaded is there so this is hardening in case that happens

Going to try to look at this this weekend but just noting that this field is html so purify seems more better.

eileenmcnaughton · 2023-09-23T06:27:59Z

test this please

templates/CRM/Case/Form/Activity.tpl

demeritcowboy · 2023-09-25T17:09:21Z

tests/phpunit/CRM/Activity/Form/ActivityTest.php


-    // cleanup
-    $this->callAPISuccess('option_value', 'delete', ['id' => $result['id']]);
+    $form = $this->getTestForm('CRM_Activity_Form_Activity', [], ['atype' => 800, 'cid' => $this->source]);


I get a fail here because action is null, so I think it just needs to add 'action' => 'add' to the params. But also while it's a nice addition to check the template vars, what this test was about was checking that somebody doesn't mess up $form->_activityTypeName (line 267).

@demeritcowboy I've pushe dup the fix - but I am trying to avoid checking properties & check what the form outputs instead - the property is 'public' but ... not really. It's an internal form property - but what the form outputs is the variable....

Since removing the existing coverage isn't necessary for fixing the bug and the discussion around public vars gets into scope creep, I'd suggest a compromise to add your test as a new test, which adds some good coverage, and then this would be good to merge (with the other test fail about the action url param addressed).

@demeritcowboy actually the way I make it work to test the form is by switching to the form test wrapper - which gives a better level of testing - but it explicitly doesn't allow testing properties - other than where they have publicly supported getter methods

demeritcowboy · 2023-09-25T17:10:37Z

templates/CRM/Case/Form/Activity.tpl

@@ -25,7 +25,7 @@
  <table class="form-layout">
    {if $activityTypeDescription }


This line is where it needs a rebase.

demeritcowboy · 2023-09-25T17:10:55Z

templates/CRM/Case/Form/Case.tpl

@@ -27,7 +27,7 @@
 <table class="form-layout">
    {if $activityTypeDescription }


And also a rebase here.

eileenmcnaughton · 2023-11-27T21:16:07Z

@demeritcowboy this kinda fell off the radar but I just rebased it since that seemed to be the issue....

demeritcowboy · 2023-11-27T21:46:39Z

Right. The other issue was that it changes what the existing test was testing which I'd like to keep, and that change isn't necessary to fix the bug. If it's added as a new test then this is an easy merge.

eileenmcnaughton · 2023-11-27T22:24:42Z

@demeritcowboy are you wanting to keep the test on that property because you are accessing the property from an extension? The form-test methodology is deliberately biased against testing properties that are not supported for external use as part of trying to test the contract rather than things that are really internal to the form. We have been adding supported functions like getActivityID() & getActivityValue() (we have getParticipantID() & getParticipantValue() on a few forms.

demeritcowboy · 2023-11-27T22:31:57Z

Ok what I'll do is take the test and move it to an extension and run it elsewhere.

So there's just the action param problem - I guess it's expecting CRM_Core_Action::ADD instead although that feels wrong.

eileenmcnaughton · 2023-11-27T22:39:33Z

@demeritcowboy I can add in the getActivityValue() function if you want & add a test on that?

I'm not totally clear on the action one - what do we need to do

demeritcowboy · 2023-11-27T23:06:30Z

Oh it's a different test failing than before. CRM_Case_BAO_CaseTest::testGetRelatedCases now needs to set $_REQUEST['action'] = 'add'.

For this test, the name and docblock should be changed since that's not what it's testing anymore. It's now testing the smarty template variable.

demeritcowboy · 2023-12-03T04:20:30Z

jenkins retest this please

This PR seems to keep growing...

eileenmcnaughton · 2023-12-03T05:17:16Z

I moved getActivityValue() to a trait & added some handling for the inconsistency around action

demeritcowboy · 2023-12-03T17:03:59Z

CRM/Activity/Form/Activity.php

-
-    if ($this->_action & CRM_Core_Action::DELETE) {
+    $action = CRM_Utils_Request::retrieve('action', 'String', $this);
+    if ($action === 'add') {


This isn't needed. Request::retrieve already does this hack.

As a general comment, I think this is unfair. This should be a 3 line PR, and now I have to r-run it again, and I've already spent a few hours reviewing it several times. All that had to happen on the last revision to get a merge was to add one line to one of the tests.

Oh gawd you are right - my head was just spinning on how it was working & I couldn't see it - I'll take this out again

…the actual activity type id When we view an activity there is code in the template that renders differently depending on the activity type id. However, if the activity ID (atype) is wrong in the url it gives the url precedence This is a bit obscure - I hit it when I changed the activity type id in the database & the view did not update

eileenmcnaughton · 2023-12-04T19:25:35Z

Thanks @demeritcowboy - sorry this was such a drag

civibot bot added the master label Sep 18, 2023

eileenmcnaughton commented Sep 18, 2023

View reviewed changes

eileenmcnaughton force-pushed the activity_view branch 2 times, most recently from 43ea66f to 8781048 Compare September 18, 2023 22:55

eileenmcnaughton force-pushed the activity_view branch from 8781048 to 4377104 Compare September 18, 2023 22:58

eileenmcnaughton commented Sep 18, 2023

View reviewed changes

eileenmcnaughton force-pushed the activity_view branch from 4377104 to ff6107d Compare September 22, 2023 22:56

demeritcowboy reviewed Sep 25, 2023

View reviewed changes

templates/CRM/Case/Form/Activity.tpl Outdated Show resolved Hide resolved

demeritcowboy reviewed Sep 25, 2023

View reviewed changes

eileenmcnaughton force-pushed the activity_view branch from ff6107d to bd0a9b7 Compare September 30, 2023 05:49

eileenmcnaughton force-pushed the activity_view branch from bd0a9b7 to 355f76b Compare November 27, 2023 21:08

eileenmcnaughton force-pushed the activity_view branch 2 times, most recently from ffe3408 to 09f7a34 Compare December 3, 2023 02:52

demeritcowboy reviewed Dec 3, 2023

View reviewed changes

eileenmcnaughton force-pushed the activity_view branch from 09f7a34 to cedc609 Compare December 3, 2023 21:17

eileenmcnaughton force-pushed the activity_view branch from cedc609 to c69cd9b Compare December 3, 2023 21:18

demeritcowboy merged commit 6ef0fa1 into civicrm:master Dec 4, 2023

eileenmcnaughton deleted the activity_view branch December 4, 2023 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix activity view bug where an activity type id in the url overrides … #27496

Fix activity view bug where an activity type id in the url overrides … #27496

eileenmcnaughton commented Sep 18, 2023 •

edited

Loading

civibot bot commented Sep 18, 2023

eileenmcnaughton Sep 18, 2023

eileenmcnaughton Sep 18, 2023

demeritcowboy commented Sep 18, 2023

eileenmcnaughton commented Sep 18, 2023

eileenmcnaughton Sep 18, 2023

demeritcowboy Sep 22, 2023

eileenmcnaughton Sep 22, 2023

eileenmcnaughton commented Sep 23, 2023

demeritcowboy Sep 25, 2023 •

edited

Loading

eileenmcnaughton Sep 30, 2023

demeritcowboy Sep 30, 2023

eileenmcnaughton Sep 30, 2023

demeritcowboy Sep 25, 2023

demeritcowboy Sep 25, 2023

eileenmcnaughton commented Nov 27, 2023

demeritcowboy commented Nov 27, 2023

eileenmcnaughton commented Nov 27, 2023

demeritcowboy commented Nov 27, 2023

eileenmcnaughton commented Nov 27, 2023

demeritcowboy commented Nov 27, 2023

demeritcowboy commented Dec 3, 2023

eileenmcnaughton commented Dec 3, 2023

demeritcowboy Dec 3, 2023

eileenmcnaughton Dec 3, 2023

eileenmcnaughton commented Dec 4, 2023

		@@ -25,7 +25,7 @@
		<table class="form-layout">
		{if $activityTypeDescription }

		@@ -27,7 +27,7 @@
		<table class="form-layout">
		{if $activityTypeDescription }

Fix activity view bug where an activity type id in the url overrides … #27496

Fix activity view bug where an activity type id in the url overrides … #27496

Conversation

eileenmcnaughton commented Sep 18, 2023 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demeritcowboy commented Sep 18, 2023

eileenmcnaughton commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Sep 23, 2023

demeritcowboy Sep 25, 2023 • edited Loading

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

eileenmcnaughton commented Nov 27, 2023

demeritcowboy commented Nov 27, 2023

eileenmcnaughton commented Nov 27, 2023

demeritcowboy commented Nov 27, 2023

eileenmcnaughton commented Nov 27, 2023

demeritcowboy commented Nov 27, 2023

demeritcowboy commented Dec 3, 2023

eileenmcnaughton commented Dec 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Dec 4, 2023

eileenmcnaughton commented Sep 18, 2023 •

edited

Loading

demeritcowboy Sep 25, 2023 •

edited

Loading