Rules integration

The upside I saw for defining 2 actions for each flag was the ability to use $flag->flag_short and $flag->unflag_short as the action titles. I thought this might be a little more user friendly since the admin could see the verbiage that they had defined for performing these actions.

After trying to implement this though, I realized that I only knew how to implement dynamic events but not dynamic actions for workflow-ng...and I ended up going the route you suggest. Not sure if that is a workflow-ng limitation, but I could not find any other examples of this in existing modules either.

The action's settings form will also have a chechbox, "ignore the user's permission", which will make it possible to flag a node even if the user on whose behalf we flag doesn't have permission to do so.

With the flag() function not really supporting this, and all of my needed flags being global, I am currently getting around this by doing all flagging on behalf of uid 1. Is there a way right now for me to get rid of that hack and still use flag.module functions?

Is the plan to have flag() take an additional parameter for ignoring permission checks? I ran into this same problem when trying to force workflow state changes with workflow-ng actions and the solution there was that the function that made the workflow state change was wrapped by a function that did permissions checking, so I was able to call the former from my action.

I'm really happy to see you working on this - I was already thinking about something like that :) If you have some questions don't hesitate to ask me. Also check out the new rules developer documentation (http://drupal.org/node/298486) - I'm still working on it, but apart from a complete element reference it's done.

@base property:
Indeed, this is missing in workflow-ng. However it's a really useful property and the idea of backporting it was already there. So if you need it, I can do that.

@actions style: Having one action per flag sounds fine. You can improve the label of the action by adding a label callback (http://drupal.org/node/298545).

who, cool!

But why not taking an easier way?

Provide on action per flag - each flag has a defined object to work with. So I'd go and create one action per flag -> "Set bookmark flag" or so and specify the object the flag module needs as needed argument -> then rules does the argument handling stuff for you. You could write all these actions as one by using the 'base' property -> http://drupal.org/node/301506
Furthermore you can optionally specify an user argument, when the flag isn't global. Then let the user choose flag/unflag on the configuration form.

I think that way, it would be much easier to do.

Anyway, regardless how you do it, I'm very much looking forward to the resulting solution.

@data type: You can do basically the same in workflow-ng by providing new "entity types" - however yes the rules API is more advanced here.

ah, and you should use argument handlers to load available arguments - so they are loaded only when needed. E.g. the node of a comment - look at the comment module events if you need an example.

@problems:
# As of 2008-09-03, test this patch against the "beta" version of Rules. The "dev" version seems broken somehow.
# The patch comments out our Actions support. Rules provokes some PHP warnings when it's active.

of course both shouldn't happen - if the problems persist with beta2 I'm going to look at it. Unfortunately (? ;) I'm going on vacation right !now!, so I won't be able to do it in the next two weeks :/

Combining these two great modules will be really powerful. I'm currently testing the patch, and looking forward to the finished result.
Thank you both!

I'm back.. any news on this?

subscribe

subscribe

Is it possible for someone to apply the patch for us? I don't know how to use cvs.

Ugh, I'm an idiot. I applied this patch, and then updated to the latest oct 17th dev. Can you commit this please? And does this work with the 17th version?

This is an important issue. I hope this gets committed soon.

The previous patch applied, but it gave errors for offsets.

I re-rolled the patch, and now it applies without any errors and works as expected.

I have a feature request to add to flag.rules.inc.

Could "User on whose behalf to flag:" have an added option for user who is referred in userreference inside the node.

This is a really weird use case, but I'm trying to make a private message node type and a view that outputs "inbox (#)", where # is the number of flagged messages they have. The flag/action combo will be " 'mark message as unread' for 'referenced user' ". It will be tied to a rule that automatically flags it as unread when it is created. Then there would also be a rule that unflags it when it automatically when it is read read. Whoah, that's a mouthful.

Thank you, mooffie. It worked great.

Are there any further testing requirements before this patch can be committed?

This seems like a very important patch.

Bump

@mooffie: thanks for the re-roll. I have a new use case that I would appreciate your help with as well:

I have a project node and an issue node; the issue nodereferences the project. My flag is 'subscribe.'

When a project or an issue are created, a rule automatically flags the creator 'subscribe' to the node. I can send an email to the creators of the projects and issues using a rule that sends an email to the flagging user whenever a project or issue subscribed, but here are my difficulties:

1. I would like to automatically flag a new issue for all the users who are subscribed to the project node (so the project creator and other project users will be able to receive emails of the new issues using the email rule above).
2. When a user creates a comment on an issue, I would like to send an email to everyone subscribed to the issue with the comment-body in the email (this way people can subscribe and unsubscribe to updates on issues).

I don't have any userreferences, but I think the functionality you described in #21 is similar to what I need here.

@mooffie: You made so many points I don't know where to start. Is the 'out of the box' problem with token lacking [flag-bookmarks-subscribers], or rules email action not handling multiple emails, or just making a rule set that can load the users and loop through the actions?

If we nail this use case, then d.o can upgrade to 6.x, so I'm very interested in ironing out the missing features. I'll happily file issues in those modules if you can help me figure out what is the likely route to building this. Thank you for all the help.

#329500: support for looping and data lists will extend this feature greatly, however, it doesn't seem necessary to wait on committing this great addition.

@moofie: this patch is RTBC. quicksketch does not intend to weigh in, but he's okay with the #26.

@moofie or quicksketch: please commit.

confirm #26 works fine

sunscribe

Awesome! I was just coming over to see whether integration was ongoing... unforunately I'm not a skilled coder, so there's not much I can contribute. But I CAN supply a mean amount of high-fives to all you diligent folks. Good job on all the work, and hopefully this sees the light of day eventually

Subscribing. Flags seem to be the only way to achieve a multilevel workflow working with revisions in D6, and for this the ability to flag/unflag content based on events is a must.

Just patched #26 with the latest 6.x-1.x-dev and it worked perfectly. I created a rule when a Flag is set and it fired just fine.

So I am tossing my hat into the ring to say that everything is still good and this should be pushed into the final release. It is a *hugely* important piece to the whole Drupal puzzle!

I'm fine with committing the patch. Mooffie are there any remaining issues that we should handle first? As I've said a couple of times, I don't use either WFNG or Rules, so maintenance of this feature will need to come from the community. Considering the lack of my ability to maintain WFNG, I'd be happy to add without a backport.

>Yeah, I think it's best to forget about D5. The world is moving forward.
agreed.

>You there? Hello? If you're mad about Rules, and know you can maintain this code better than us, or rewrite it in a better way, perhaps in a separate module, say so and the pleasure is all yours.

I am! And I'm still interested in that, so I'm going to give that a further test and check whether it makes sense to take the easy way (see #9) - as it would also ease maintaining the code... I do so tomorrow.

Subscribing as I am looking for precisely this functionality on one of my Drupal 6 communities.

So I've overhauled the patch quite a bit. Unfortunately it took me much more time than I had expected and I'm not yet finished :(

Status:
* Reworked event definition so arguments get only loaded when they are really used
* Added a flag data type, which uses an input form. So the form_helper function has been changed to the input form.
* The data type would allow us to easily implement actions that alter a flag or just other arbitrary conditions/actions needs a flag as argument.
* I reworked the actions so that there is one per flag type - this lets rules handle the variable magic and we the need for the JS stuff is gone.
* I introduced a new API function for rules, that makes the flag module integration a bit cleaner. So you need the latest rules dev snapshot to test this.
* I moved the new .inc files into the include folder.
* I've tested it and it seems to work well for nodes. The user actions have problems I have to look at - maybe a rules bug. Comment stuff needs testing. More on that tomorrow.

Again:
You need the latest rules dev snapshot to test this - from tomorrow or later.

ok I gave my patch further tests. So far everything is fine with it. The bugs were in rules - this integration let me discover two name-clash edge-cases I've just fixed now. So best try it with the next generated dev snapshot.

>[x] Skip the user's permission check" checkbox on the flag/unflag actions' forms. Ticking this checkbox will make it possible to flag an item even if the user on whose behalf we flag doesn't have permission to do so

I wanted to mention this too - usually there shouldn't be such a permission check for rules actions. So I also think we should add a checkbox.

>Suppose I delete a flag for which some actions/events were set up. Will Rules handle this gracefully?
Yep it should put a warning message in the log. I have to give that a test though.

>It's plaussible to assume that developers will write flag handlers (a la class flag_node, flag_comment) for objects that don't have Rules support. In this case flag_flag::rules_get_[event|element]_arguments_definition() should return an empty array. Does the patch handle this edge case?

I don't think so - I have to check this.

>(I actually liked the previous ability to switch among the flags once the action was already set up. But this "feature" was eventually a stumbling block and just had to go.)
It's still possible to switch between flags of the same type..

>I couldn't test the code, but I read it carefully, and it looks very good. We should have a celebration when this gets committed.
It's great that you like it. =)

ok, I've updated it.

>I wanted to mention this too - usually there shouldn't be such a permission check for rules actions. So I also think we should add a checkbox.
added.

>Yep it should put a warning message in the log. I have to give that a test though.
tested, works.

>>It's plaussible to assume that developers will write flag handlers (a la class flag_node, flag_comment) for objects that don't have Rules support. In this case flag_flag::rules_get_[event|element]_arguments_definition() should return an empty array. Does the patch handle this edge case?
fixed that.

Updated patch attached. Again use it together with the latest rules code as it has some bugs fixed that might occur with it.

ehm, this is the *updated* patch..

I've just released rules 1.0 beta 5 containing quite some bug fixes - best just use that one.

hm, I think this integration misses some conditions to check whether a flag is currently flagged. Then I think we should add some basic docs in the README or so. Perhaps we do a follow-up patch for that once this is in? Any reviewers? :)

>The patch touches 'flag.install' for some reason. That's probably an accident.

Oh, indeed it is. I'll reroll it afterwards. Thanks.

>It's a pity we have no way of informing the users of this(?). I wish Drupal had a better dependency mechanism.
Yep.. Me too.

>(There should probably be a comma after "if enabled". But perhaps it's better to extend it to "If the checkbox below is ticked,")
ah yeah my english is bad.. thanks.

Sry for being a bit late.. I travelled to Washington in the meanwhile :)

ok, I've fixed the language and rerolled the patch. Furthermore I've added the mentioned condition - seems to work fine :)

Updated patch attached.

>So... reviewers? wake up ;-) We sure want to have this in the upcoming "1.0".
Agreed! Having it in 1.0 would be great!

Subscribe for later review.

Hello:

I I'm going to do a few tests for this patch.

Enviorment:
Drupal 6.9 (yes, I know, sorry)
Flag 6-rc1
Rules Beta 5

RulesPatch: March 3, 2009 - 16:40

First, patch aplies fine, no errors.

In this first test, I created a global flag. This global flag assign the flagging user to a user reference field in a node.
Works excelent.

Tomorrow I'm going to test with nodes and comments using non global flags.

But for now it seems to be working fine.

Great work fago!

Ps: Maybe another user should review the strings used in this patch. I don't speak english very well but usually I understand it. Maybe the strings can be rewritten in a more simple language.

Saludos!
Mariano

There is something that I don't like here, and that's the creation of events per flag-type. I think it should be like this:

* Have only two events: e.g. node flagged and node unflagged.
* Have the flag-type as an argument, (and possible the action "load a flag by name").
* Have a condition to check the flag (e.g. "Flag has name" like "Content has type").

Why? I can think of two main reasons:
1) Remove clutter from the triggered events.
2) Breaking up things allows more flexibility - I will be able to assign nodes to two different flags in the same rule configuration.

---

Regardless, I'm attaching the same patch only with some text and links about the Rules integration (flag.module).

thanks amitaibu, I rerolled the pach again and fixed the language a bit. Usually we talk of triggering rules, not triggering rule actions, so I've fixed that.

@flag-type:
Currently we also have flags as argument, they are just using their own input form for easier user input. That's much easier then having to do a "load flag" action first and does exactly the same. Doing it so has only one disadvantage: It's not so easy to take a flag variable provided by an event and modify exactly that one. But that's no common use for flags either. In contrast the usual use-case is just flagging a flag or checking it, that's easy to do at is.

The same is for the events, it's just easier to go with that events. I don't think it's clutters the interface so much, as usually one hasn't hundreds of flags, or? :) Doing one generic event brings us not much I think, and if I'm wrong with that we could still add a generic event too. Assigning two different flags in the same rule configuration works as it is too?

So for making it as easy as possible for people to use it, I think we should stay with it as is.

I think that for simple configurations the current solution is fine - but even then it's IMO a little non-consistent with the arguments concept. Here's an example:
1) My events is triggered when node is unflagged under "bookmark"
2) I add an action to flag a node. Now I have an radio buttons to select from which flag-type I want to uncheck (flag.snap).
3) I add another flag action, again I need to select from the radio buttons.

Furthermore in I'm concerned about the fact that I can't write a single rule that will be invoked when nodes are flagged under different flag-types.

In my suggested way flow will be like this:
1) triggered when node is unflagged.
2) Condition to check what flag(s) type
3) Since the flag type is passed as argument I don't have to choose anything - I'm already working on the passed flag type. If I want to use another flag type I'll "Load a flag type" action.

Moffie - what are your thoughts about it?

The current interface makes this kind of things easy because we don't need to first load the "review" flag.

I'm all for usability, but I think we can accept from a user that if she wants to flag another flag-type, to load it first. I can write a tutorial about it, and mark it in bold :)

Here's another more complex example, and after that comes a compromise:

1) User would like to create a rule, that when a group node is created, a flag type will be created on the fly (i.e. every group will have it's own group-flag) and mark the node in the new group-flag.
2) User would like to create a rule that is invoked when a node is flagged - and he wants to create this rule before any flag-type was created -- I think this can't be solved in the current implementation.

Now the compromise (just an idea):To the 6 general events add per flag event. It adds more events but doesn't compromise on functionality Vs usability.

I know I'm a bit stubborn on this issue - mainly because it's so awesome and I know so many people will use it :)

ad use case 1)
I don't think that dynamically creating flags is a common use case - I don't see why one would need that. But yes, it's not possible right now.

ad use case 2)
Yep, but again this is only a problem if you dynamically create your flags. Even then it's no real problem, as you flag already comes with a default flag ;)

>Now the compromise (just an idea):To the 6 general events add per flag event. It adds more events but doesn't compromise on functionality Vs usability.
Yep as said, I think we could add further generic events. But if so, let's do them really generic and also pass a boolean "flag flagged" to it - so we would have only one generic event per supported entity.

Anyway the flag 1.0 release is close and I would really love to see it shipping with rules integration. So imo the current one is fine, great usable and should go in as is. We can work on extending it with further events any time.

>I know I'm a bit stubborn on this issue - mainly because it's so awesome and I know so many people will use it :)
Yep, this is really awesome and allows one to do a lot of cool stuff. So having it in flag 1.0 would be awesome.

Ok, let's this into flags, and have following patches. Very minor:
* I've added a link to singleton in wikipedia.
* 'Cut offsize' => 'Flag queue size', I think it's less confusing.
* Comments clean-up.

Fago, please review my changes and if it's ok, then I consider it RTBC.

FYI, #394152: Improvement for the Rules integration - follow up issue for after committing this patch.

Thanks for improving comments. I also agree with the "flag queue size" - that makes sense. I'm not sure whether we should mention "singleton" there, but I'm ok with that too.

>Fago, please review my changes and if it's ok, then I consider it RTBC.
I've set it to be RTBC. Thanks for your help testing and improving this patch!

@moofie: why don't you commit it?

Thanks everyone. Committed.

Awesome, thanks!

@quicksketch,
Thanks :)
I've noticed you committed it to D5 as-well, however, this is incorrect since there's no D5 version for Rules (nor the workflow-ng module work with those files). (http://drupal.org/cvs?commit=183272)

I committed the changes to flag.inc in D5 for consistency so that patches can be backported easily. I did accidentally commit a chunk to flag.module that made some help text about Rules shown but I fixed that in #404112: D5 doesn't have Rules.

Moofie thanks for all your hard work on the Rules integration. I think that these two modules will create a lot of synergy! By the way, the flag module is simply amazing!

FYI, a follow up patch - #405316: Rules: Use flag as a 'regular' argument.

I've added a tutorial, that uses the Rules integration, on how to create 'Latest viewed content' block - http://drupal.org/node/405754

1) Well, I have of course not tried to do anything harmful - only good intentions here :)
2) I actually was so concentrated in my propsed patch I overlooked the fact you can do it with the current rules implementation - I've changed the docs accordingly. sorry about it.
3) I remembered you can check if the node is viewed as a page without PHP in workflow-ng, haven't found it in Rules - I'll consult fago about it. (EDIT: #407028: Regression: Can't check if a node is page or teaser without PHP)

For the record, I thanked Fago first, then he said to thank you. Now you are suggesting that I thank him, what great bunch of guys :) A "thanks" module would probably be pretty useful.

I can't wait to dig into the rules/flag combo, it's going to make my life much easier.

And don't worry, there will be more participant prodding in the future; specifically, when the two modules need to be upgraded to Dev. 7x :)

Thank you all for the great work you've done! It is a very useful addition to the Flag module.