Problem/Motivation

When configuring rules.module integration for flags, a boolean parameter exists to enable "Permissions check" such to allow execution of a flag/unflag action whether the user has the permission to execute the action or not. However, the effect of this checkbox is inverted -- it should be labeled "Skip permission check."

Proposed resolution

Change UI to properly reflect permission-check skip booleans, provide permission-check skip boolean to flag_trim_flag().

Remaining tasks

None, but related issue is that Trim a flag operation deserves consideration: #1689510: "Trim a flag" action should support "Skip permission check" parameter.

User interface changes

The checkboxes when configuring flag-related rules are reversed, and now (correctly) represent "Skip permission check" instead of "Permissions check."

API changes

None

Original report by refineo

I would like the system to trigger a rule that unflags non-global flags on behalf of other / all users.

The reaction rule works fine as long as a user has permission to unflag:

1. Fetch users who have flagged a node
2. Loop User From the List (1)
   2a. Unflag a node for each user User on whose behalf to flag

I would like the rule to be able to unflag on behalf of a user that has no permission to unflag manually.

I don't want the user to be able to unflag manually at all.
I would like the site administrator to trigger the rule that unflags node on behalf of another user who flagged the node.

Drupal 7.12

CommentFileSizeAuthor
#17 flag-1550540-17.patch5.78 KBc4rl
#16 flag-1550540-16.patch5.19 KBc4rl
#14 1550540-flag-rules-skip-perm.patch582 bytesklausi
#7 permission-checkbox-1550540-07.patch706 bytesRefineo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

MStrzelecki_’s picture

MStrzelecki_ CreditAttribution: MStrzelecki_ commented

Hi!
I want to know how to combine above code with views vbo? With that code all user's flags will be unflaged, but I want to have a vbo interface that I can manually uset one user flag... How to populate users that only checkboxed in vbo?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Category: feature » bug

I have the same issue (as far as I understand it correctly):

- my component fetches a list of nodes via VBO
- loops through all nodes and attempts to reset the flags
- fails for all users with improper permissions

Unflagging via Rules is only possible, if the flag owner has permission to.

The permission checkbox at the bottom of the action form isn't ticked. IMHO this means: Flag should ignore permissions and rules should be able to reset all flags.

I therefore switch this issue to bug report.

@miszcz: you're off-topic. Better file a support issue at the VBO project to get a qualified answer. My opinion is, what you want is not possible solely with Rules/VBO/Flags: VBO is either used for manual mass-operation or automated mass-operation (with rules).

Refineo’s picture

Refineo CreditAttribution: Refineo commented

@Shnapoo, yes, this is exactly what I wanted to say. Flag should ignore permissions and rules should be able to reset all flags when the permission checkbox at the bottom of the action form isn't ticked.

@miszcz, it is a bit off topic, but, IMHO, if you have a separate flag that is neither visible on node display nor on the teaser the only way to assign it would be to use the vbo /views. What do you think ?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

I got it. The "Permission Check" checkbox is name wrong. Tick it to ignore permissions.

In the code it is...

function flag_rules_action_unflag($flag, $entity, $flagging_user, $permissions_check) {
  $flag->flag('unflag', $flag->get_content_id($entity), $flagging_user, $permissions_check);
}

...and the $permission_check variable matches $skip_permission_check...
function flag($action, $content_id, $account = NULL, $skip_permission_check = FALSE)

The best way of fixing it (without breaking existing rules) would be, to just rename the field. This is a patch against 7.x-2.0-beta6:

Index: flag.rules.inc
===================================================================
--- flag.rules.inc	(revision 6)
+++ flag.rules.inc	(working copy)
@@ -185,8 +185,8 @@
     ),
     'permission_check' => array(
       'type' => 'boolean',
-      'label' => t('Permission check'),
-      'description' => t('Whether to check access permissions against the user on whose behalf to flag.'),
+      'label' => t('Skip permission check'),
+      'description' => t('Whether to ignore permissions of the user on whose behalf to flag.'),
       'restriction' => 'input',
     ),
   );
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Priority: Normal » Major
Status: Active » Needs review
Refineo’s picture

Refineo CreditAttribution: Refineo commented
Assigned: Unassigned » Refineo
Status: Needs review » Needs work

We need a patch against the latest 7.x-2.x-dev version that is now 7.x-2.0-beta6+31-dev. I will do it today.

Refineo’s picture

Refineo CreditAttribution: Refineo commented
Assigned: Refineo » Unassigned
Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
permission-checkbox-1550540-07.patch706 bytes

Here is the patch file against 7.x-2.0-beta6+31-dev.

MStrzelecki_’s picture

MStrzelecki_ CreditAttribution: MStrzelecki_ commented

@refino: my users can assign flag on nodes manually, themselfs. But I don't know how to use vbo and rules to remove flag from a particular user on specific node.

1. Fetch users who have flagged a node
2. Loop User From the List (1)
2a. Unflag a node for each user User on whose behalf to flag

remove flags from all users whos assign on specific node.

Sorry for my English ;-D

MStrzelecki_’s picture

MStrzelecki_ CreditAttribution: MStrzelecki_ commented

If you have no idea also I will post it on vbo issue board.

Refineo’s picture

Refineo CreditAttribution: Refineo commented

@miszcz , regarding #8, in short, I used a flag "spam" and the separate global flag called "remove all spam flags" and created a rule "Unflag All Spam Flags". The rule, when triggered by flagging "remove all spam flags", unflags all users' flags on the flag "spam".
Please also see this question: Flag module - add a link to unflag all node

IMHO In future please rise this kind of questions as a separate support question. Please do not hijack existing issues for asking new (even though somehow related) questions.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

@miszcz
- If it's a one-time event or shall be triggered manually: create a node view and include the VBO field. Add exposed filters if you like, or normal filters. No need to use Rules, because you can execute the bulk operation via UI directly from the view.
- If it's a frequent event to be triggered automatically: create a node view and include the VBO field. Add filters/relations to limit it to a user and a flag. Then you can loop through the nodes via Rules.

Refineo is right. Hijacking issues is no good idea. It dilutes the original issue. So please open a VBO issue in case of further questions.

MStrzelecki_’s picture

MStrzelecki_ CreditAttribution: MStrzelecki_ commented

Ok, sorry and thx for help ;)

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Reviewed & tested by the community
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
FileSize
1550540-flag-rules-skip-perm.patch582 bytes

Patch does not apply. Rerolled to work with drush make.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Reviewed & tested by the community » Needs work

I don't understand how this patch which appears to just change some UI strings can be the fix for this bug's issue title.

Could someone explain a bit more please?

c4rl’s picture

c4rl CreditAttribution: c4rl commented
Status: Needs work » Needs review
FileSize
flag-1550540-16.patch5.19 KB

@joachim Permissions bypass was possible, but the UI was simply providing a checkbox that declared the opposite intention of the actual boolean. That is, the boolean permissions check in $flag->flag() should be sent as TRUE to *skip* the access check.

Rules integration was also completely leaving out this check for trims, which could be a separate issue, but I feel is related to this one. Attached patch includes this check for flag trims.

c4rl’s picture

c4rl CreditAttribution: c4rl commented
FileSize
flag-1550540-17.patch5.78 KB

Better comments.

c4rl’s picture

c4rl CreditAttribution: c4rl commented
Title: Flag Rules integration: rule to be able to unflag on behalf of a user that has no permission to unflag manually. » Rules "Permission check" parameters are inverted; should be "Skip permission check"

Better title.

c4rl’s picture

c4rl CreditAttribution: c4rl commented
Issue summary: View changes

Updated issue summary.

joachim’s picture

joachim CreditAttribution: joachim commented

> Rules integration was also completely leaving out this check for trims, which could be a separate issue, but I feel is related to this one. Attached patch includes this check for flag trims.

Thanks, but I'm confused enough by the Rules stuff in the module enough as it is, so I'd rather have that as a separate issue please!

c4rl’s picture

c4rl CreditAttribution: c4rl commented

Do you mind being more specific to explain what you mean by "I'm confused enough by Rules stuff in the module enough as it is?" What exactly is confusing?

With all due respect, one person simply saying "I'm confused" is too vague and personal of a justification for me to change the patch yet.

In my view, this issue is related to permissions checks in rules, and fixing it for both flag/unflag operations and trim operations was the comprehensive fix since trims are essentially a bulk unflag.

joachim’s picture

joachim CreditAttribution: joachim commented

I'm the new maintainer. Hi :)

I don't use Rules much at all, so I'm not familiar with its UI, and I don't develop components for it so I'm not familiar with its API.

Hence any patches I commit for rules are going to go in based on reviewing by other users. But I'd still like to keep them simple. This patch has gone from being a simple one (albeit one I didn't understand, but I think I do now), to being one that changes the API in the rest of the flag module. Which make it suddenly way more confusing. Hence why I'd like to commit the earlier, smaller, patch, and come back to this big change in another issue. Confused maintainers tend not to commit patches :)

c4rl’s picture

c4rl CreditAttribution: c4rl commented
Assigned: Unassigned » c4rl
Status: Needs review » Needs work

I don't use Rules much at all, so I'm not familiar with its UI, and I don't develop components for it so I'm not familiar with its API. Hence any patches I commit for rules are going to go in based on reviewing by other users.

Since you are the new maintainer I encourage you to become more familiar with Rules' API since it has been part of Flag for over 3 years (http://drupal.org/node/298109#comment-1355042)

Nevertheless, I understand hesitance to change API functions; I assume you are referring to flag(), correct?

After some consideration, it should be possible to write this patch without making API changes, by simply using $flag->flag() instead of the procedural flag() since $flag->flag() already takes the permissions check as an argument. I'll re-roll shortly.

joachim’s picture

joachim CreditAttribution: joachim commented

> Nevertheless, I understand hesitance to change API functions; I assume you are referring to flag(), correct?

I'm happy to make them. But I'd like to keep them isolated in patches that don't also have effects to parts of the module I'm not yet up to speed with / have no easy means of testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

> I'm the new maintainer. Hi :)

Welcome! That's great news!

The original issue was all about a false named checkbox. The patch was ready to be committed. It shouldn't be delayed by a second bug fix.

The second fix might also require some refactoring or additional tests to be written. Who knows, some more eyes should get a chance to look over it at a separate issue.

joachim’s picture

joachim CreditAttribution: joachim commented

Exactly.

@c4rl: I do really appreciate your work on this. I'm going to need people who know their way around rules to review patches (and write them, too please!). Just that we have/had a simple patch here I was just about thinking was RTBC, so let's get that in so the next release has it, and work on the more complex bit in another issue :)

c4rl’s picture

c4rl CreditAttribution: c4rl commented
Assigned: c4rl » Unassigned
Status: Needs work » Needs review

Sounds good to me. I'll post another issue and edit this one to clarify.

@joacihm, your concerns in #15 are resolved such that the patch in #14 is sufficient for rtbc?

c4rl’s picture

c4rl CreditAttribution: c4rl commented

Issue regarding the trim action #1689510: "Trim a flag" action should support "Skip permission check" parameter.

Issue regarding possible API change for flag() #1689530: flag() should support $skip_permission_check

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Fixed

Heh, like I say, I'm counting on people who know what they're doing with Rules to tell me when to commit stuff :D

But looks like enough people agree, so in it goes!

Issue #1550540 by refineo, klausi: Fixed Rules 'Permission check' parameter label.

Does this need backporting?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

Wasn't it me investigating and solving the issue at #4? Never mind! There are worse things to fix than credits. :)

joachim’s picture

joachim CreditAttribution: joachim commented

Sorry!

Dreditor's automatic commit message doodad only looks at who posted patches!
(I'd fix the git commit if that were possible, but it's not.)

joachim’s picture

joachim CreditAttribution: joachim commented
Issue summary: View changes

Ugly grammar :)

joachim’s picture

joachim CreditAttribution: joachim commented

Doesn't seem to be anything like this on 6.x-2.x.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Updated description to reflect applied patch.