Support from Acquia helps fund testing for Drupal Acquia logo

Comments

brunodbo’s picture

Status: Active » Needs review
sdboyer’s picture

Status: Needs review » Needs work

have a look at the pipelines branch. this is all gonna be SOOO much better...and no more global perms. woot.

because i want it to be done out of pipelines and not with a global perm, though, i'd want this patch rerolled against that system. it might be a bit tough for you to penetrate; if so, i'll get to it as i can.

ar-jan’s picture

Does that branch address granular settings for which types of pane roles are allowed to add in that 'Add content' dialog as well? (although that is probably not IPE-specific)

populist’s picture

Title: Add granular IPE permissions » Add IPE permissions for changing pane settings or deleting panes

Updated the title to be a bit more descriptive

setvik’s picture

Re-rolled patch with additional permission for "Adding" panes via IPE

setvik’s picture

Issue summary: View changes

Clarifying issue description

heddn’s picture

Issue summary: View changes

The permissions should be added to the ipe module, not to the panels module.

PI_Ron’s picture

This patch doesn't work against the latest dev.

PI_Ron’s picture

I've created a new patch with a few changes that should now apply to the latest dev. The main difference was the $links variable had changed to $vars['links']

This issue / patch is a great leap forward for Panels IPE, allowing us to completely remove the buttons from the IPE for certain roles. When combined with the 'Immovable' setting, it allows much more control over what certain roles can do with the IPE. In saying that, these permissions are still global and I believe there is a great need for individual pane IPE permissions, if anyone can shed some more light on how these permissions or access control rules could be added to IPE that would be great. Do you think I should create a new issue for this, or do you think it is covered here: https://www.drupal.org/node/2151943

Many thanks for your guys to your work on this so far.

heddn’s picture

Please add these permissions/changes to IPE module, not directly to panel's itself.

PI_Ron’s picture

@heddn I have now moved the permissions to the Panels IPE module. I also added a new permission to Panels IPE called 'administer panels styles in place editing'.

For the styles button to show up in IPE now the user needs either the permission 'administer panels styles in place editing' or the more global 'administer panels styles'.

PI_Ron’s picture

Added permission for changing region styles through the IPE.

mrjmd’s picture

Status: Needs work » Needs review

The last submitted patch, ipepermissions.patch, failed testing.

The last submitted patch, 5: panels-ipe-add-remove-settings-permissions-1699432-5.patch, failed testing.

mrjmd’s picture

The function panels_permissions() has a TODO of "Almost all of these need to be moved into pipelines.", and I found it confusing how some IPE were still in panels but these new ones weren't.

So I've gone ahead and moved all IPE-related permissions into the IPE module. Patch attached.

Functionally tested this too and it seems to work very well. I find the main Panels permissions to still be a bit confusing, but that's not this patch's fault :).

mglaman’s picture

+++ b/panels_ipe/panels_ipe.module
@@ -264,3 +268,44 @@ function theme_panels_ipe_toolbar($vars) {
+    'change pane settings in place editing' => array(
+      'title' => t("Change pane settings with the Panels In-Place Editor"),
+      'description' => t("Allows a user to change pane settings with the IPE."),
+    ),
...
+    'add panes in place editing' => array(
+      'title' => t("Add panes with the Panels In-Place Editor"),
+      'description' => t("Allows a user to add panes with the IPE."),
+    ),
+    'remove panes in place editing' => array(
+      'title' => t("Remove panes with the Panels In-Place Editor"),
+      'description' => t("Allows a user to remove panes with the IPE."),
+    ),

You're not writing and update hook to ensure people with the general permission have this permission.

All of the sudden my customers would lose ability to add/delete/customize their pages while still able to "customize page."

PI_Ron’s picture

So we need to write an update hook to give these permissions to roles with the 'Use In Place Editor' permission?

mglaman’s picture

Yep! There's another patch in the queue we can copy that does the same for region styling.

mrjmd’s picture

Status: Needs work » Needs review
FileSize
10.19 KB

Thanks for the review mglaman! Here's another attempt that includes the update script. I left the update in panels.install rather than creating an install file for panels_ipe.

mrjmd’s picture

Blarg, no newline on that, and wrong update number. One more try.

I guess depending on which makes it in first, this or #2329419: Style permissions not granular enough, one of these updates is going to cause a conflict.

DamienMcKenna’s picture

Status: Needs review » Needs work
  1. +++ b/panels.install
    @@ -492,3 +492,19 @@ function panels_update_7303() {
    +function panels_update_7304() {
    

    I suggest adding a drupal_set_message() here to explain to the site builder that the permissions changed.

  2. +++ b/panels_ipe/panels_ipe.module
    @@ -160,11 +164,11 @@ function template_preprocess_panels_ipe_add_pane_button(&$vars) {
    +  $vars['region_links'] = '';
    

    Are there any problems with renaming this variable?

  3. +++ b/panels_ipe/panels_ipe.module
    @@ -264,3 +268,44 @@ function theme_panels_ipe_toolbar($vars) {
    +      'title' => t("Use the Panels In-Place Editor"),
    

    The title should append "(IPE)" to help some of the other descriptions.

  4. +++ b/panels_ipe/panels_ipe.module
    @@ -264,3 +268,44 @@ function theme_panels_ipe_toolbar($vars) {
    +      'description' => t("Allows a user to utilize Panels' In-Place Editor."),
    

    Replace "Panels'" with "the Panels" to match other descriptions.

  5. +++ b/panels_ipe/panels_ipe.module
    @@ -264,3 +268,44 @@ function theme_panels_ipe_toolbar($vars) {
    +      'description' => t('Allows users with access to the In-Place editor to administer page manager pages. This permission is only needed for users without "use page manager" access.'),
    

    Should be spelled "In-Place Editor". Also, the "without "use page manager" access" bit could be slightly improve, e.g.: without the "use page manager" permission.

mrjmd’s picture

Status: Needs work » Needs review
FileSize
9.06 KB

Addressing Damien's comments:

1. I've added a comment and a link to the permissions page.
2. I agree, not sure there was ever any compelling reason to change it. Switched back.
3. Done
4. Done
5. Done

I've also removed the space from $add_button = ' ';
I don't think it was necessary.

PI_Ron’s picture

+++ b/panels_ipe/panels_ipe.module
@@ -160,11 +164,11 @@ function template_preprocess_panels_ipe_add_pane_button(&$vars) {
+ $vars['region_links'] = '';
Are there any problems with renaming this variable?

2. I agree, not sure there was ever any compelling reason to change it. Switched back.

That variable was changed from links to region_links to allow differentiating between the links to be added for a pane and the links for a region. Without the change in variable I couldn't see a way to create the 'administer region styles in place editing' permission.

DamienMcKenna’s picture

Michelle’s picture

Status: Needs review » Needs work

I tested #24 and it mostly works. The various permissions seemed to work fine, other than I couldn't see any change whether "Administer region styles with the Panels In-Place Editor" was enabled or not. I set this to "needs work" because I'm getting this error:

Notice: Undefined index: links in theme_panels_ipe_add_pane_button() (line 210 of /var/www/html/drupal-contrib/sites/all/modules/panels/panels_ipe/panels_ipe.module).

That line is $links = theme('links', array('links' => $vars['links'], 'attributes' => $attributes));

At that point, it's 'region_links' in $vars according to my debugger so I'm wondering if that just got missed in the name change?

Michelle’s picture

Status: Needs work » Needs review
FileSize
10.37 KB

I tried changing it to "region_links" and the error went away so I'm uploading a revised patch. I don't know if changing it in that spot will have any other effects, though, so this may not be the right way to do it.

DamienMcKenna’s picture

Doh! Thanks for fixing that, Michelle.

PI_Ron’s picture

Awesome stuff, these permissions are going to help me a lot. Thanks.

aleksijohansson’s picture

Status: Needs review » Reviewed & tested by the community

#26 seems to work great and in general the patch does what it says! Tested with 7.x-3.5.

Chris Charlton’s picture

I'm excited for this feature.

ron_s’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
11.43 KB

Patch #26 no longer applies cleanly due to the recent release. The new patch has two changes:

- Changes panels_update_7304() to panels_update_7307(), since there is now a 7304 in the module.
- Moves the new bypass access in place editing permission from panels_permission to panels_ipe_permission, to match the others.

Please review the attached, thanks.

ron_s’s picture

FileSize
10.88 KB

With the new release on Oct 16, 2016, the patch in #31 no longer works. New version is attached for review.

PI_Ron’s picture

Tried patch from #32 with latest dev, but got malformed patch:

$ patch -p1 < panels-n1699432-32.patch                                                        ⬡ 5.12.0
patching file panels.install
patching file panels.module
patching file panels_ipe/panels_ipe.module
patch: **** malformed patch at line 162: @@ -204,7 +206,7 @@ function theme_panels_ipe_add_pane_button($vars) {
arnested’s picture

FileSize
12.18 KB

I fixed the patch from #32.

I also added a separate permission for drag panes in place editing for dragging panes with the Panels In-Place Editor.

ron_s’s picture

FileSize
12.1 KB

Patch #34 no longer works with the latest 7.x-3.x-dev branch and 7.x-3.9 release. I've created a new version for review.

ron_s’s picture

FileSize
11.96 KB

After 3+ years and another new release, patch #35 no longer works.

Have again created a new version... I'd appreciate if this can finally get reviewed.

ron_s’s picture

FileSize
11.91 KB

Attached is a new version of this patch. In 7.x-3.11, panels_update_7309 was added, which conflicts with the update this patch needs to do.

The new patch pushes the update to 7310. All other changes are the same.

ron_s’s picture

Patch in #37 applies cleanly to the latest 7.x-3.12 version.