CRM-20430 - Permission 'save Report Criteria'. #12107
Conversation
There was a problem hiding this comment.
(CiviCRM Review Template WORD-1.1)
- (
r-explain) Pass - (
r-test) Pass - (
r-code) Pass - (
r-doc) Pass - (
r-maint) Pass - (
r-run) Pass: Can save or copy reports with the new permission. Don't have the option to do so without it. - (
r-user) Pass: A pre-upgrade warning to review the permissions exists. - (
r-tech) Issue: see below - (
rg-upgrades) Issue: Pre-upgrade message is displayed twice when upgrading from <4.7.32. - (
rg-perm) Undecided: Should this permission be added to the civicrm_webtest_user role in civibuild or civicrm_webtest.install so that users can test saving reports on dmaster demo?
| @@ -84,6 +84,7 @@ public function setPreUpgradeMessage(&$preUpgradeMessage, $rev, $currentVer = NU | |||
| } | |||
| if ($rev == '4.7.32') { | |||
| $preUpgradeMessage .= '<p>' . ts('A new %1 permission has been added. It is not granted by default. If you use SMS, you may wish to review your permissions.', array(1 => 'send SMS')) . '</p>'; | |||
| $preUpgradeMessage .= '<p>' . ts('A new %1 permission has been added. It is not granted by default. If your users create reports, you may wish to review your permissions.', array(1 => 'save Report Criteria')) . '</p>'; | |||
There was a problem hiding this comment.
Seems to be a leftover from the previous PR.
eileenmcnaughton
force-pushed
the
save_report
branch
2 times, most recently
from
714515b to
356401e
Compare
|
@tschuettler thanks for the review - I've fixed the 4.7.32 double up. Good point about demo site permissions - I feel like this is something that has been off our radar when adding perms - @totten how do we best handle / audit this (current test failure is not related I believe -looks like server resources) |
|
Thanks @eileenmcnaughton! (Seems like the installation did not even finish this time since the mysql server was down) |
|
Hmm - are those permissions what is not used in buildkit or are they legacy - I think this might be where I need to add them - https://github.com/civicrm/civicrm-buildkit/search?utf8=%E2%9C%93&q=access_civimail&type= |
|
test this please |
1 similar comment
|
test this please |
|
@totten can you comment on where we need to add permissions for the buildkit / demo environment? |
eileenmcnaughton
force-pushed
the
save_report
branch
from
356401e to
cd32a18
Compare
|
@eileenmcnaughton -- @tschuettler's reference to
Note: the Phrased another way:
|
|
hmm - that's intuitive - update a drupal module for generic permissions. I don't even want to ask about WP & Joomla! :-) Doing it now & will merge this once done as I feel this satisfies the review |
If you don't have the permission, the 'save report' actions aren't shown on the form of the report instance. CRM-20430 - Check 'save Report Criteria' permission before saving a report instance. Coding style issues. Added a pre-commit message about the new permission.
eileenmcnaughton
force-pushed
the
save_report
branch
from
cd32a18 to
4341efe
Compare
|
unrelated fail - merging |
|
yay! |
|
@tschuettler good idea! |
Overview
Nuance report permissions, adding a new save report criteria permission
Before
Without or with permission
After
Without permission
With permission
Technical Details
I am torn between renaming the perm to 'edit report_templates' for consistency with other entities or sticking with 'save Report Criteria' to be more consistent with other report perms - per original submitter
Comments
This is a recut of #10164 with the things that were blocking it tweaked. It does makes sense & has made sense to others - however, I won't keep this open if it is not merged by the time the 5.3 rc is recut that is it's 'last chance'