Remove stray token code

[eileenmcnaughton]

Overview

Remove stray token code

Before

Code in the Activity_Tokens class and the associated trait to support a feature that I believe was not adopted for core

After

poof

Technical Details

@mattwire @aydun @demeritcowboy @totten
The removed code is, from my recollection, intended to add support for a syntax for multiple activity contacts that we did not, in the end, agree to merge to core (leaving the question of how to support that still unsolved and out of scope for this PR).

So from what I can tell we should be able to remove it & simplify the code base at this point. Is there any reason not to?

Comments

[civibot]

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

[eileenmcnaughton]

Note tests are failing - but that doesn;t change the question - since if we don't need these lines we can remove them & fix the tests pretty easily

[colemanw]

@eileenmcnaughton +1 from me - I think it's better not to keep code lying around that isn't being used for anything. Can you update the test?

[demeritcowboy]

I think this part never made it in, and so there is no such token in the dropdown. I don't know if manually inserting such a token is being used anywhere, but since there's a test that covers it it suggests it was intended for core. What was still open was a more thorough treatment of multiple values.

Me personally I'm not using the token anywhere. @aydun ?

[mattwire]

Right, it was intended that it would go into core, just it ran out of steam and we weren't certain if the _N_ was the right approach. This is the branch containing the patch that I maintain on all my client sites: mattwire@4bbaf83

Agree that the code doesn't need to be in core unless we merge the rest - but we should also remove the test in that case. It would also be really good to decide whether we actually should have this functionality in the core tokens - it's certainly really useful for a number of sites I maintain that use activities/cases to manage patient/counsellor relationships and send lots of emails with details including assignees/targets etc.

[eileenmcnaughton]

Ok - based on the above I'm gonna remove it & the test. I note there is also an extension that is pretty good on this that @demeritcowboy pointed other people to - I don't know if that's the right approach.

[demeritcowboy]

By extension do you mean @mattwire's branch?

FYI the test that covers it linked above is not the same test that failed here. I'm not sure what the fail here is about since it's not related.

[eileenmcnaughton]

I've updated the PR to remove the code that was supporting those few lines

[aydun]

I'm not currently using those _N_ tokens but I do think the functionality is useful even though we may want a different representation now that token syntax has evolved a bit.

It was originally for someone else's client but between us we've lost track of who it was for :-(

[eileenmcnaughton]

@aydun yeah - I think there is still consensus that it would be useful to have 'something' that does that

[eileenmcnaughton]

this is passing now - we built on the original tests @aydun added so cover is solid

[demeritcowboy]

A quick r-run seems ok for me. Putting merge-ready.