CRM-21189 - Add permission for Close and reopen Batch

[pradpnayak]

Overview

This PR adds 4 new permission for Batches as mentioned in issue CRM-21189

NOTE for release notes - may require a permission tweak for batch users

[eileenmcnaughton]

So on this there are 3 aspects

1. code appears ok
2. this adds additional permissions - there has been some pushback about growing our list of permissions. From a code point of view permissions are cheap & while it does grow the list on the screen that is not a frequently used screen. So I feel like the bigger issue is that it is confusing what the permissions mean. In this case the answer seems to be to ensure it is clearly documented in our docs
3. it appears to me that there is a behaviour change here that might require someone to re-configure the permissions on their install. This doesn't seem to be spelt out in JIRA or on the PR & probably needs an upgrade message

[seamuslee001]

In regards to 3 especially, i would recommend setting a preUpgradeMessage for this along the lines of what we did for the changes to sql imports. For reference see here https://github.com/civicrm/civicrm-core/blob/master/CRM/Upgrade/Incremental/php/FourSeven.php#L63

[monishdeb]

@seamuslee001 @eileenmcnaughton thanks a lot for your feedback :)

For 3, I have added the preUpgrade message looks for these four permissions:
Does this message makes sense?

For 2, I am having trouble in finding the right place to document these changes in https://github.com/civicrm/civicrm-dev-docs , is there any section to enlist permissions?

[eileenmcnaughton]

@monishdeb when in doubt about docs ping @seanmadsen ... :-)

[eileenmcnaughton]

Note it seems to me from the code that there is an actual change required by the site owner?

[seamuslee001]

My thinking would be is that it might actually be in the User docs or Sys Admin docs perse

[seancolsen]

Thanks for the ping. Here's the relevant area of our docs:

https://wiki.civicrm.org/confluence/display/CRMDOC/Default+Permissions+and+Roles

I'm currently working on migrating these docs into the Sysadmin Guide, but please feel free to make edits in the mean time. My migration process is script-based and relatively atomic. Soon that wiki page will redirect you to the new Sysadmin Guide! Until then, edit away, and any changes you make will be scooped up by my script.

[seancolsen]

On second thought, that wiki page doesn't really offer much. I haven't looked at this PR closely, but as far as documentation, I think the post important thing is that new permissions should have a description which displays to the user in the UI like these do:

[monishdeb]

@seanmadsen yes those 4 new permissions have their description:

On otherhand, these four permissions that are responsible to control the batch actions against batch listing are added along with three other batch permissions under 'Notes on Special Permissions' section https://wiki.civicrm.org/confluence/display/CRMDOC/Default+Permissions+and+Roles .
Not sure if this is the right place to do it.

[seancolsen]

Thanks @monishdeb Regarding documentation only, I think this this PR is in good shape ✅

[monishdeb]

Cool :)

@eileenmcnaughton @seamuslee001 is there anything else I need to address or is it good enough to merge?

[seamuslee001]

Final Changes look fine to me. If @seanmadsen is happy on the documentation front I am happy also

[monishdeb]

@eileenmcnaughton can you merge this PR?

[eileenmcnaughton]

@monishdeb @seamuslee001 my concern is just that I think we are not being quite explicit enough about what action a site admin would need to take. I feel like they might need to grant an additional permission or some users might lose the ability to do something they were previously able to do?

[monishdeb]

@eileenmcnaughton in my opinion after the 4.7.26 upgrade/install, these new permissions doesn't affect admin user. However, it might affect other users with authenticated role and for that purpose, we are providing an indication via preUpgrade message about this change. Further we have a documentation https://wiki.civicrm.org/confluence/display/CRMDOC/Default+Permissions+and+Roles

[eileenmcnaughton]

@monishdeb Yep - I'm happy with the concept of putting the note in the preUpgradeMessage - but I just feel it needs to be a bit clearer in terms of something like "If you use batches you may need to assign permission y to ensure users can still do x - with a link to the documentation

[monishdeb]

@eileenmcnaughton I have changed the preUpgrade message. Here how it looks now

[eileenmcnaughton]

I think that does it.

[monishdeb]

Yayee. Thanks a lot, @eileenmcnaughton @seanmadsen @seamuslee001 for your feedback :)

[eileenmcnaughton]

Fail is unrelated - some temporary issue