Make Send SMS permission independent of Edit Contact permission

[michaelmcandrew]

Overview

A (presumably fairly trivial) follow on from #11590.

Most of the tasks listed in CRM_Contact_Task::tasks() also require the edit contact permission but a few do not. The ones that do not are mentioned in the final else of ::permissionedTaskTitles().

We want people to be able to Send SMS without having the edit contacts permission. Therefore, we need to mention it in this final else clause.

Before

You needed an Edit contact permission AND the send SMS permission to send SMS

After

You only need the send SMS permission to send SMS.

[eileenmcnaughton]

@mattwire @JKingsnorth are either of you able to review this one?

[monishdeb]

Here's a scope for code improvement:

// lets declare a array of tasks we should be looking, to be included in final list of permitted $tasks
foreach ([
 self::MAP_CONTACTS,
 ...
 self::CREATE_MAILING,
 self::TASK_SMS,
] as $taskID) {
  if (isset(self::$_tasks[$taskID]) && !empty(self::$_tasks[$taskID]['title']) {
    $tasks[$taskID] = self::$_tasks[$taskID]['title'];
  }
}

@michaelmcandrew what do you think?

[mattwire]

So we have a number of functions in play here:

• CRM_Contact_Task::tasks(); (and then CRM_Core_Task::tasks()) returns an array of ALL available tasks for the Contact entity which should be independent of permissions (but currently many permissions are defined in that function).

• CRM_Contact_Task::permissionedTaskTitles(); (and then CRM_Core_Task::corePermissionedTaskTitles()`) should return an array of tasks which the current user has permission to access.

• CRM_Contact_Task::taskTitles(); (and then CRM_Core_Task::taskTitles()) should really be deprecated and merged into the permissionedTaskTitles() function (there was only so much refactoring I could do at the time!).

As a step forwards I think it would be better to:

1. Remove the permission check from tasks():
   $providersCount = CRM_SMS_BAO_Provider::activeProviderCount(); if ($providersCount && CRM_Core_Permission::check('send SMS')) {

2. Move the initial SMS task declaration into the main self::$_tasks = array( declaration.

3. Add that check after the final "else" in permissionedTaskTitles(), or if it may apply to multiple entities in corePermissionedTaskTitles(). So you would have:

if ($providersCount && CRM_Core_Permission::check('send SMS')) {
  $tasks[self::TASK_SMS] = self::$_tasks[self::TASK_SMS]['title'];
} else {
  unset($tasks[self::TASK_SMS]);
}

[michaelmcandrew]

Thanks for the feedback @monishdeb and @mattwire - I'll take a look ASAP.

[michaelmcandrew]

@mattwire - I've thought about this for a while, and although I get the motivation, I can't see a sensible way to do what you said. When I try, the code ends up more complex.

If I add it unconditionally in tasks() then when it comes to permissionedTaskTitles() I have to do two things

1. if the person has edit all contacts, but does not have send SMS, I have to remove the permission from $tasks

2. If the person does not have edit all contacts, but does have send SMS I have to add it (which is what I am currently doing).

So it seems simpler to keep it as it is.

I can make the change if you prefer but wanted to check first. Or if I am missing something, feel free to let me know / have a go yourself.

@Monish - if we end up keeping it this way, I will add your refactoring

[mattwire]

@michaelmcandrew Ok let's keep it simple for now and just change it as @monishdeb suggested. Is there a unit test we could add at the same time?

[michaelmcandrew]

@mattwire - ok sounds good. give me a shout if you ever want to discuss further clean up on this area of the code base. re: tests, I did add a permissions test in #11590 which this PR also passes. Might that be good enough? Else, are there any unit tests specifically for CRM/Contact/Task.php that I could copy?

@monishdeb - I've implemented the code cleanup that you suggested.

[eileenmcnaughton]

Merging - I think the change has been adequately reviewed (and I also checked the code myself).

We don't have tests on this function yet (the permissioned tiles) and it would be good to add but I also think this change could slip through without one as it is pretty trivial