Fix activity tokens #21489
Fix activity tokens #21489
Conversation
|
(Standard links)
|
eileenmcnaughton
changed the title
eileenmcnaughton
changed the title
| /** | ||
| * @inheritDoc | ||
| */ | ||
| public function getActiveTokens(TokenValueEvent $e) { |
There was a problem hiding this comment.
Just copied back from the trait - this overrides the parent to
a) add in the old-style tokens and
b) do weird stuff - which is the general issue with this class
eileenmcnaughton
force-pushed
the
act_toke
branch
2 times, most recently
from
a5fd58d to
d491ed0
Compare
I wasn't gonna touch these until the end but there
seems to be some active breakage so this does enough to get the following to
work - but leaves a full cleanup pass out of scope as it does
the reconcilliation between the trait & parent class
and any decisions about any finalised interface.
Tests cover the changes to tokens and
testActivityDateTimeMatchRepeatableSchedule covers the schedule rendering
Existing tokens still work but new-style are advertised with this
```
Subject: {activity.subject}
Date: {activity.activity_date_time}
Duration: {activity.duration}
Location: {activity.location}
Details: {activity.details}
Status ID: {activity.status_id}
(legacy) Status: {activity.status}
Status: {activity.status_id:label}
Activity Type ID: {activity.activity_type_id}
(legacy) Activity Type: {activity.activity_type}
Activity Type: {activity.activity_type_id:label}
Activity ID: {activity.activity_id}
(legacy) Activity ID: {activity.id}
(just weird) Case ID: {activity.case_id}
```
eileenmcnaughton
force-pushed
the
act_toke
branch
from
d491ed0 to
b441a50
Compare
|
I think this will work for me but need to get @demeritcowboy to check as well |
|
test this please |
| } | ||
| elseif (in_array($field, ['case_id'])) { | ||
| // An activity can be linked to multiple cases so case_id is always an array. | ||
| // We just return the first case ID for the token. | ||
| $row->tokens($entity, $field, is_array($activity['case_id']) ? reset($activity['case_id']) : $activity['case_id']); | ||
| // this weird hack might exist because apiv3 is weird & |
There was a problem hiding this comment.
I haven't reviewed this fully yet just commenting on this - it's not api. You can have a single activity on multiple cases. Link-cases is an example in core. But returning a random case id when there's more than one maybe isn't a useful token.
There was a problem hiding this comment.
@demeritcowboy do you think I can just remove it? It's pre-existing but I'd be happy to kill it.... although maybe I need to socialise that / make it a follow up?
There was a problem hiding this comment.
When there's only one case it lives in, which is most of the time, it could be a useful token.
I am curious about hearing more about the use-cases for activity print/merge in general.
There was a problem hiding this comment.
@demeritcowboy yeah that stuff was added by @aydun
I'd like to see if we can resolve this to get the existing tokens working & move that question to gitlab (since I just sorts keep it working here)
|
thanks @demeritcowboy |
|
@demeritcowboy I opened https://lab.civicrm.org/dev/core/-/issues/2848 to ask the question about the tokens.... |
Overview
Fix activity tokens
Before
In testing for another PR @demeritcowboy and I were finding not all advertised tokens were being resolved
After
Working, test cover extended, still passes action schedule token tests.
Technical Details
I wasn't gonna touch these until the end since my plan was to get comprehensive test cover in place
and migrate the ambigous tokens to the new style before figuring out the 'final look' of whatever
combo of parent class/ trait we use - but it turns out there is some active breakage.
This is not intended to be the 'final word' but just 'enough to get it working again & supported
the preferred tokens'
It leaves out of scope a final cleanup pass and
the reconcilliation between the trait & parent class
and any decisions about any finalised interface.
Existing tokens still work but new-style are advertised with this
This is the block I've been testing with - I also tried
{activity.location}Comments
Tests cover the changes to tokens and
testActivityDateTimeMatchRepeatableSchedule covers the schedule rendering
Note that if this is merged I'll action the updates to scheduled reminders, docs, and the form rules for pdf & email but I won't add any noise / break the legacy style tokens as there is higher risk this class is being used outside of core. At some point we can