Beta-breaker: Make Actions usable by mere mortals

not much but we need to start somewhere :P

Just so ppl understand what is one of the problems: I am sitting at admin/build/actions/assign/node and laughing my a* off. See screenshot why.

I'd like to step in and give some background information on the way the current D6 implementation of actions works. There are currently three components to the actions system:

1. Individual discrete operations -- like publishing a node, banning a user, adding a node to a queue, sending an email -- that are exposed by modules as 'actions'.
2. Modules that use actions_do() to execute one of those actions.
3. The actions.module itself, which lets users configure what actions should be executed when specific drupal hooks are triggered. (node save, cron, comment posting, etc.)
Some have questioned the usefulness of #2 in that list, since a module could set node->status just as easily as calling actions_do(). The advantage lies in loose coupling. For example, a 'flag offensive content' module could be written with lots of configurable options for what happens after a node is flagged. With actions, however, the module's settings page could just present a list of actions. "What ones should I execute when a node is flagged as offensive?' As other modules expose new actions (sending the node's content to an anti-spam service for future flagging, perhaps), those options would automatically become available for the 'flag offensive content' module without any additional code being written. That's the power of actions, as an API.

The 'Actions Module' that ships with Drupal 6 is just one example of a module doing that. It shows a list of core hooks (nodeapi, comment, user, cron, etc.), and the ops that each of them have (insert, update, delete, etc). It lets admins wire up actions to be executed when those hook/op combinations are triggered. It's important to keep in mind here that this Actions *module* is only one of the many potential uses of actions. We could use them all over the place in core, but there wasn't time invested in a larger-scale conversion before freeze.

Actions *module* offers two points of pluggability for contrib modules. First, by implementing hook_hook_info(), a module can tell actions.module that ITS hook should be listed along with nodeapi, comment, cron, etc. and amins should be able to connect actions to it. This is probably a good thing, although for many modules it will still make more sense to integrate actions right into their config screens, as described above. If enough modules implement hook_hook_info() it could easily become a page with dozens upon dozens of tabs.

The second point of pluggability, though, is fundamentally flawed -- and this is what leads to the goofy screenshots that chx posted above, with 'save this node' appearing under nodeapi's delete op. Although it's not required by the actions *API*, the actions *module* allows any action to have a list of 'hooks' and 'ops' that it supports. This information is NOT necessary for the 'flag offensive content' module mentioned above; it can always say, 'give me a list of actions that are node-related' or 'execute this list of actions'. The 'I support these hooks' data is used exclusively by Actions Module to determine what actions should be offered to a user for association with a particular hook and operation in its UI.

The simplest solution to the problems webchick and chx have noted is to edit the action definitions for the assorted core actions, and reduce the list of ops that each action claims to support. That would make them only appear in places that 'make sense'.

That, however, runs into a new problem. What if our aforementioned 'flag offensive content' module uses hook_hook_info() to expose its hook_content_was_flagged() for use in actions.module? That hook would, in fact, appear in the list on Actions.module's UI. Unfortunately, NO ACTIONS would appear. Because they have the names of the hooks they support hard-coded in hook_actions_info(). This creates a very tight coupling between each action and each hook. They only way to get around it is to change the definition of the action to say that it supports 'all' hooks. This is what the 'send email' and 'display a message' hooks do.

This leaves us between a rock and a hard place, architecturally. We can:

1. Restrict the number of hooks and ops that each action claims to support, which makes the UI make more sense out of the box. Unfortunately, this will mean that new hooks added to the screen will be crippled; no actions will be available to them.
2. List actions as applying to 'all' hooks, which makes them available for new hooks when they are implemented. Unfortunately, this puts us back in the position of having nonsense operations like 'save the node when it's deleted' and 'ban the user when they read content' available in the UI.
3. Restrict the number of hooks and ops that each action claims to support, AND insert a call do drupal-alter('actions', $action-info) into actions.inc. This would prevent goofy options form appearing AND give modules like our hypothetical 'flag offensive content' a chance to manually say, 'no, I know that publish_node_action will work with my hook. I'm adding that to its hooks list.'

For the D6 release, I believe that option #3 is the soundest solution with the least impact. It will keep strange and nonsensical options out of the UI, while allowing contrib to fix some of the inflexible aspects of the actions.module as it currently stands.

In the long term, I believe that the best solution is to replace the 'what hooks does this action support' list with a 'context' list. IE, 'this action can do stuff to nodes, users, and comments...' Matching the context objects from a hook to the contexts supported by an action would support intelligent filtering while maintainer a looser coupling between the two pieces of the system.

Here it is: webchick's A is done! moving on...

I've gone through and cleared out quite a few of the actions that were appearing in strange places. While it's interesting to demonstrate that certain actions *exist*, there is no logical reason to associate 'ban IP address' actions with the user logout event. Similarly strange pairings have been eliminated.

The strong coupling effect of the 'hook' mechanism, as described in my earlier post, is the real source of the problem but this makes it much cleaner.

We still need a docs/help page.

As I am an user who never used actions before, I am a perfect match for an user who could be confused with actions. And I am. Even after i applied patch above.

I think that actions are great and I think that now i understand some portions of it. And the biggest question in my head is:

WHEN is the action triggered?

Let me stop by few actions available in core:

"Change an author of a post" to "Username: ". WHEN? Everytime some post is added? Or are there some conditions? Hidden where? in Actions module, in additional settings? Do I have to use PHP coding to use it?

I think that this really needs clarifying. The answer may be: "Hey MeBa, you are so dumb, it's written here: ...", but I am really interested in how actions stuff works.

P.S.: Actions vs. Actions module are very confusing. Did you think about renaming one of them to Triggers or something?

I think the effect the last patch makes is very good. It is still a bit less confusing, but it is still somewhat. I added an extra sentence to the actions settings page: "The events that trigger these actions can be enabled and configured by enabling the actions module."

Rerolled patch below.

"Change an author of a post" to "Username: ". WHEN? Everytime some post is added? Or are there some conditions? Hidden where? in Actions module, in additional settings? Do I have to use PHP coding to use it?

Unfortunately, no. Outside of the two configurable actions ('unpublish node/comment if it contains a keyword...') there is no provision for conditionals at the moment.

P.S.: Actions vs. Actions module are very confusing. Did you think about renaming one of them to Triggers or something?

Yes. I think that would be an excellent change. Quick show of hands from the powers that be -- any thoughts on renaming the MODULE portion of this system to 'Triggers'? Perhaps 'Automator' module?

I think renaming actions.module to triggers.module is a great idea. Clearly we also need a videocast to demonstrate how to get actions and triggers up and running. I will respond to other questions above shortly.

That, however, runs into a new problem. What if our aforementioned 'flag offensive content' module uses hook_hook_info() to expose its hook_content_was_flagged() for use in actions.module? That hook would, in fact, appear in the list on Actions.module's UI. Unfortunately, NO ACTIONS would appear. Because they have the names of the hooks they support hard-coded in hook_actions_info(). This creates a very tight coupling between each action and each hook.

Not so.

actions_list() in actions.inc contains a drupal_alter('action_info', $actions) call specifically to avoid this problem. Your module can expose hooks via hook_hook_info(), expose actions via hook_action_info(), and alter the existing hook_action_info() information via the drupal_alter() call.

So your module can actually support any of the existing hooks in Drupal, it can add more, it can even subtract them.

I'm a complete tool. Thank you, John -- in earlier reviews I had missed the presence of that alter() call, and even suggested its addition in one of my posts above. :)

I still think that a 'context' mechanism would be more flexible, and prevent the tight coupling, but the alter hook in particular gives us a LOT of breathing room for enhancement in contrib over the Drupal 6 life cycle.

Actually the credit for adding the drupal_alter() goes to Moshe.

Well, actions definitely got sense for me after i enabled Actions module. I am trying to contrib to Drupal core and I knew there will be actions in D6, but I didn't know that there is additional module in core (not in contrib). I think this means that:

1. Renaming Actions.module to Triggers will make a lot of sense
2. I will definitely hide Actions menuitem until Triggers module is enabled. That's very confusing. (After renaming, Actions can become a tab of Triggers?)
3. At first look after enabling Actions module, i was a little bit confused with "Comment Content Cron Taxonomy User" tabs. It took me about 5 seconds to determine they mean "Triggers when something about Comment happens", etc. I think that this will be OK after the module is renamed to Triggers, but then it appears that my second suggestion will be confusing, because "Actions" will not fit into "Triggers when ... happens". So...I don't know

My 2. meant "I would definitely hide..." :-)

I will definitely hide Actions menuitem until Triggers module is enabled. That's very confusing. (After renaming, Actions can become a tab of Triggers?)

This isn't really possible using the current system. The 'Actions' menu item is for managing the configuration of specific custom actions like the 'Send Email...' and 'Change author...' actions. It applies whether the Triggers module is enabled or not, and its functionality is needed by other modules, like Workflow and Voting Actions.

For consistency with other core module names - "Trigger" module rather than the plural, please?

Ok - so I'm game for renaming actions to trigger module. I'll do that tonight. Beyond that, we'll still need more help pages. Patch forthcoming

Phew!
couple of things

• Actions renamed to trigger
• Page split (trigger.module and trigger.admin.inc) couldn't help it
• Added some new actions
• Changed actions from admin/build/actions to admin/settings/actions
• Removed all actions that didn't make sense in the context
• Still needs better help pages, and changing from "Actions" to "Trigger" in the help pages

You could also decrease the amount of wasted screen space on the Actions Assign page by splitting the page up into two columns, like the administration section. The attached image is a proof of concept.

Could someone help me out of this cloud of confusion? My understanding is that from now on we call Trigger the module, which is responsible to fire the Actions (core). Do we touch the core actions at all? I started to work on the help page for the trigger.module as dmitrig01 asked, when I realized we use the same text for the settings/actions page. For me there is no clear separation where and what do we call triggers from here on, and what are the actions.

For example on the build/trigger page it says:
"Triggers are functions that Drupal can execute when certain events happen," But in the drop-down boxes it says "actions". (Moreover I have my doubts if calling an action "trigger" is correct in english at all (trigger is more like the mechanism rather than the action AFAIK)).

The action is the thing that Drupal does (e.g., unpublish a node, ban a user). The trigger is fired when an event happens that has an action that has been assigned to that event. Actions are assigned to events using trigger.module. An event is a Drupal hook-op combination like the 'login' operation of the 'user' hook. We use hook_hook_info() to translate "'login' op of the 'user' hook" into English so we can display a nice interface in trigger.module, the result being "When a user has logged in'.

To answer your question, Do we touch the core actions at all?, the answer is No. Think of the actions as living in a big repository which you can access via the actions administration page. There you can add actions, remove actions, and configure actions in the repository. You can do this regardless of whether trigger.module is enabled; trigger.module just gives you the ability to use the actions from your repository when an event happens. You could leave trigger.module disabled and using the voting actions module, and the voting actions module would also have access to the repository of actions in your Drupal installation.

Workflow is another module that utilizes that 'pool' of available actions. Whenever a node changes 'workflow states', specific actions can be fired to do things like notifying admins, etc.

@dmitrig01 - thanks a lot for rolling the patch.

An important question - should the actions indeed be under admin/settings or admin/build?

I think that when the Trigger module is enabled, it makes sense at /admin/build, but seems confusing without it.

jvandyk: #28: print.module - THIS is very descriptive (first paragraph). I think it can be refactored and used as help text somewhere...

thank you for the explanation

This is a definite beta breaker. Awaiting rewritten help texts and a generally better tested patch. (I have seen there was a consensus in private emails, that the actions.module -> trigger module renaming is agreed on, just recording it here for the future).

Why does this need to happen that late in the devel cycle? This is just "l'art pour l'art".

since you asked ...

- usability improvements are welcome late in the cycle and they are important
- we are not currently "late in the release cycle". in fact, we haven't even put out a single beta.

I agree that we want to rename this in core, and we will. Let's get this module ready so it can be part of beta 1. I'm less likely to make these kind of changes once beta 1 is out.

Not sure the protocol here, but I thoght I'd jump in (since Karoly asked ). Looking at the help text on the actions, might suggest the following instead:

--snip

Actions are functions that Drupal can execute when certain trigger events happen, such as when a post is added or a user logs in. Contributed modules may provide new actions and/or triggers.

This page lists all actions that are available. Simple actions that do not require any configuration are listed automatically. Some actions need be added manually as they require configuration. To add an action, select the action type from the drop-down below and click the Add button. After completing the configuration form, the action will be available for use. You can proceed to the Build Trigger menu to assign your action to a trigger.

--snip

I'd recommend renaming the Configure button to Add, so that it avoids confusion with the "Configure" links which appear to be self explanatory. Also, make the word "Trigger" a link to the configuration page for triggers. That way people don't have to hunt the previous menus.

Finally I'd recomment that the "System" category be changed to "Other" and the "Node" category be changed to "Content" in all of the action drop downs.

Don't know if that helps, but hey I'm trying.....

Let me know if you'd like my help to take a different form.

Dave

This help text is very easy to read and understand...

Great work, Angie. Although I haven't had much time to review it, I have a quick question here.... Since Actions is being renamed to Triggers, should admin/settings/actions than be moved to admin/settings/triggers?

Finally got some time to review your patch in depth and everything looks great! The help texts you added really assist with explaining everything. The menu structure makes complete sense and I am really looking forward to seeing this in Drupal 6.

What I think the Triggers page is missing now is cosmetics. When the user first visits admin/build/trigger, they see a bunch of empty fieldsets. What if we were to make those both collapsible and collapsed? The attached screenshot demonstrates how it would look.

"Actions are individual tasks that the system can do, such as unpublishing a piece of content or banning a user. Modules, such as Trigger module,"
shouldn't "Trigger module" be "the trigger module"?

Below you can assign actions to run when certain user-related operations happen. For example, you could send an e-mail to an administrator if a user account is deleted.
shouldn't that be "when" instead of "if" (on all of them)

admin/build/trigger/cron
maybe explain what cron is? (cron runs hourly...)

also, I think we need to add back in that save post stuff, and make it more obvious. After reading the node stuff *five times* I see that save post is needed. Maybe make this more obvious 'n rename it to save node?

OK, I haven't read the issue (webchick told me not too ;-)) so here's what I got:

Coming at it with totally fresh eyes and mind here are my observations.

We have "Triggers are system events" and then the next para states "when certain content-related operations." Are events and operations the same thing? If they are different I'm not sure how they are related or what the distinction is.

Also, it isn't terribly clear what "Trigger module combines these triggers with actions" means. The word "combine" is sort of mysterious. Is it mapping, creating an association, building something new for each combination?

Is this what is going on?

You have an event that "happens", this is a trigger, and you can make an action (functional task) occur or fire or run when that trigger happens. The trigger module creates this association.

Is that correct?

Usability wise it looks good to me.

Minor thing:

The first trigger says "Before saving new or edited post to the database, or deleting a post" but a later one says "After saving an updated post to the database". Might be ore clear to use the same wording for both of them.

I also encountered a bug when trying to send an email, but it's probably not part of this patch: SMTP server response: 501 Invalid mail address, must have a domain part in W:\www\drupal-cvs\includes\mail.inc on line 191.
(Tell me and I file a separate report).

One more small thing: "Action Type" should be "Action type", right?

i was asked to test this patch, without reading this thread first - so here goes blind testing:

on Content Triggers, the first dropdown menu is confusing.
"Trigger: Before saving new or updated post to the database, or deleting a post"
1. before drupal deletes a node it'll be made sticky?? to what end? why bother?
2. I can make a node sticky before saving it, or after saving it. This seems strange if you're not familiar with node api as a developer. What's the difference between before and after if you're a web admin?

"post" is user friendly word for node? are they always interchangable? node seems more accurate.

Content triggers second dropdown makes complete sense,
"Trigger: After saving a new post to the database"
and having got this far i understand the first dropdown better.

i made a new action: send email to author (%author)
and assigned to a comment creation - i did wonder if that's the comment's author or the node's author.

otherwise completely oarsum functionality. once i got the concept of "triggers", then "actions" made made sense - well chosen nouns.

With that, I can't find anything more.Expanations are crystal clear and no more "ugh what" combinations. With so many great, great reviews I think we are ready to roll, here.

One last thing :)

"A unique description for this configuration of this action. This will be used to describe this action when assigning actions."

Shouldn't the word "trigger" be there somewhere? If not, set back to RTBC.

There is no need for Recepeint and Subject to have size="20". They can be as long as the description on "send e-mail" action page.

¤%&/ . (inserting endtag, sorry for the spam).

I have an updated patch, just need to get the add-and-remove-files in CVS working.

Fixed typo in the taxonomy.module help.

Someone tried to sneak in node_save() calls into the node actions. Please note that omitting node_save() in the node actions is intentional. If you need the node saved, use the node save action -- that's why we have it as a separate action. If you have a case for changing this, do it in a separate issue.

Removed "to the database" from the trigger descriptions. We don't talk about saving stuff "to the database". It's implied that when we "save" something, it goes to the database. No need to have the user worrying about "why is it not deleted 'from the database' then?".

I find it somewhat humorous that we started out with explanations like "After a user account has been created" and then we changed them all to "When a user account has been created" and now we are changing them back to "After a user account has been created".

And a word about "it doesn't make sense to have such-and-such an action in such-and-such a context". I disagree. I think that just like Drupal itself, there are use cases that we do not forsee and are blocking out of the gate. I won't fight it in this patch, I'll just create triggerunlock.module which will do a drupal_alter() to allow all actions to be used in all contexts. E.g., maybe I really do want to allow a user to be blocked after viewing a single node on my site. You don't know that, but by whittling down actions_info() to only contexts that "make sense", we prevent these kinds of functionality. OK, I'll get off my soapbox now.

Thanks for all the work people have put into this!

Well, we cannot remove ALL contexts from an action. For example, the user actions and the comment_unpublish actions are missing the "hooks" key in their hook_action_info() entries. It needs to either be hooks => array('any' => TRUE) or some specific hook-op combo. Otherwise the action is not assignable.

And a word about "it doesn't make sense to have such-and-such an action in such-and-such a context". I disagree. I think that just like Drupal itself, there are use cases that we do not forsee and are blocking out of the gate. I won't fight it in this patch, I'll just create triggerunlock.module which will do a drupal_alter() to allow all actions to be used in all contexts. E.g., maybe I really do want to allow a user to be blocked after viewing a single node on my site. You don't know that, but by whittling down actions_info() to only contexts that "make sense", we prevent these kinds of functionality. OK, I'll get off my soapbox now.

I agree -- I can think of scenarios where these kinds of things would be useful. We've had clients ask us to do very, very crazy things at times :> The biggest risk, though, is that enabling actions like that makes it confusing for users trying to wrap their heads around 'why would I use this?' questions, AND makes it easy for someone tinkering with the site to accidentally ban their own IP, block the admin account, and so on. 'Destructive' actions like deleting nodes, banning users, and so on are are best suited for places like the node admin and user admin pages. They're also the ones that would benefit the most from conditional filters. I'd really like to get on top of adding those via the magic of hook_action_info_alter() in contrib for D6...

This was changed intentionally, because it's incredibly confusing that when you say "Change such and such flag on the node" it doesn't actually do it, unless you do a second step of saving the node. You don't need to do that second step on any other type of action. It doesn't make sense why this one is treated differently than the others. Is the underlying reason for not doing it this way performance or something? So you're not re-running through all of the hooks that fire on node_save for each flag you set on a particular trigger?

My understanding is that it is specifically to prevent repeated saves on nodes when several actions are chained together. In other words, actions like 'publish' and 'hide' and 'change the author' are designed to alter a property on the in-memory copy of the node. To prevent EACH of those actions from saving and executing additional hooks, they only change the in-memory node, and the 'save node' action can be used to persist them.

This replaced relatively complex and confusing 'batch mode' for actions that existed in the D5 version of the module. It allowed actions to execute in one of two modes, only one of which implied actually saving to the database.

It's tricky; there's definitely the potential for thrashing if someone attaches several persisting actions to an event like, say, node_view(). However, it does get rather confusing if things aren't saved by default. I would agree that the improvements in this patch are more than worthwhile on their own, and that the 'granular vs. monolithic save' debate should be put off for a separate thread if it's important enough to continue.

Brought back all actions except ban IP of current user.

Eaton is exactly right on why the node actions are broken out into discrete actions, not combo actions like "make node sticky + save node".

I think one way to go is to have a toggle, "[x] Show recommended actions [ ] Show all actions". But that's an improvement for another issue. Let's get this one wrapped up!

This RTBC was set for a previous patch. Let's get this reviewed/tested.

Let's commit #63. All the power in the world is meaningless if none can use it and a separate node save is extremely confusing.

We can debate this later but undoing something that a horde of people reviewed to be useable on the grounds of "more features" is not a good idea.

Let me give you an example. We had an immensely powerful feature in form API (yes it was even more powerful at a point than now!), namely using references for the #value but the usage was so hard that we instead went with form_set_value . Batchable actions do not justify the confusion here.

I'll wait on John's more detailed writeup on why he thinks the user should specify the node save action vs. saving everything when a change is made. One obvious thing is that you'd need to run lots of node saves if you modify lots of data. I read up the initial actions issue: http://drupal.org/node/148410 and found some reasoning about batched actions (but not much), which might be in connection with the possible performance problems, or it might not.

If we keep automated saves, then there is no reason to have the save action.

Also, John added new stuff in his latest patch, although he forgot to change the status from "ready to be committed", so either webchicks previous patch is up for discussion, or the latest one from John.

Why should the site administrator specify the node save action vs. saving everything? Because we learn lessons from writing code, and when we take a wrong approach we correct it. Saving the node automatically seems the most straightforward approach, and was the approach we took in 4.7 actions. What webchick asks for, a way for trigger.module to "be smart here" is the approach we took in 4.7 actions. Both were a mistake.

They were a mistake because (1) even though it seems simpler from an interface perspective, it was a nightmare to debug. Drupal does not currently have the internal logic that can handle recursive node_save() calls; (2) it is a performance hog to run through all the hooks and their associated functions when all you really want to do is to change a flag on a node.

As I said above, actions need to be discrete. Changing a node to sticky is one action. Changing a node to unpublished is another action. Saving a node is another action. If you want to combine them into sets of actions, then we need more code to manage 'action sets', and that is exactly what eaton is cooking up for Drupal 7.

I think everyone has missed a major point here. We added the 'presave' op to the nodeapi hook specifically to address this problem. The hook description was originally "When content is about to be saved" and was changed in this thread to "When either saving a new post or updating an existing post". Here's the solution: by default, we restrict flag-changing actions like "Make post sticky" to the nodeapid 'presave' op only. That way the flag change happens prior to Drupal's built-in node_save(), and everyone is happy and not confused. Yay!

The attached patch does that.

The last patch solved the problem for node actions assigned to nodeapi hooks. We still have a problem where you want to do something like "Promote node to front page after saving a new comment."

Worked with webchick this afternoon on a solution to that. Now node actions that modify node properties declare that they do so in node_action_info(). If one of these actions is assigned outside of nodeapi 'presave', we automatically add the Save post option and display a message to the administrator: "You have added an action that changes a the property of a post. A Save post action has been added so that the property change will be saved."

This gives us the best of both worlds.

Oh.my.god. What a nice work! Thank you so much Angie, John and all for working on this to perfection.

Next time i will check the actual patch and not RTBC on "out of issue" information -- but actually, I warned Angie that the trigger module was left out of the post.

Huh. *Thanks to all*, who participated. I am glad we have a consensus now, so committed.