So we only have the action: Fetch content flagged by user. This only grabs a list as nodes. I need to be able to get flagged content that is something other than nodes... In this case actually entityForms...

One of my rules needs to check to see if you have flagged any of the entities related to a specific entity form type.

I've noticed that there is already code allowing you to flag any flag type. Should be able to implement the same for fetching flagged entities as well.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

generalredneck’s picture

Since there is already a loop that prints out all of the entity types and corresponding actions for each one, I just piggy backed off of it.

This allows you to get more than just nodes by user which was the case before. It also maintains backward compatibility so that people who currently use the option for just nodes are not disrupted by this being updated.

joachim’s picture

Status: Needs review » Needs work

Good call!

Just a few things:

+++ b/flag.rules.inc
@@ -229,38 +206,64 @@ function flag_rules_action_info() {
+    $label = isset($entity_info['label']) ? $entity_info['label'] : $type;

I have a hunch that the entity label must always be set -- check the docs for the info hook.

+++ b/flag.rules.inc
@@ -292,6 +295,9 @@ function flag_rules_action_info() {
+  // For backward compatibility sake. This was the original name of the 'fetch node by user'.
+  $items['flag_fetch_entity_by_user'] = $items['flag_fetch_node_by_user'];
+  unset($items['flag_fetch_node_by_user']);

Nice touch! (Shame this didn't get spotted till after I released 3.0, as we could have just changed it and not bothered with backwards compatibility...)

But can we keep the 'node' item in, even if it's a duplicate, to encourage the better behaviour?

generalredneck’s picture

But can we keep the 'node' item in, even if it's a duplicate, to encourage the better behaviour?

I think to be able to do that we would have to write up great script to change all of the existing rules from entity to node. I dont see having a duplicate dropdown item as desirable. It would most likely get called out as a bug later.

I have a hunch that the entity label must always be set -- check the docs for the info hook.

Your hunch is right... but if you see what I replaced below you will understand why I did that.

'label' => isset($entity_info[$type]['label']) ? $entity_info[$type]['label'] : $type,

I figured someone had a reason for doing it before. Also turns out that having that fore the label in the old code actuslly covrred up the fact they were checking the wrong key. The array is only 1 level deep and not 2. Which meant we were showing the machine name for all types.

I'll get it fixed up.

joachim’s picture

> I dont see having a duplicate dropdown item as desirable. It would most likely get called out as a bug later.

Call the backward compatibility one something like 'Node (legacy)' so they look different in the dropdown. I do see your point about the duplicate being taken for a bug in future, but the node one being called 'entity' will be taken as a WTF in future :)

> I figured someone had a reason for doing it before.

Maybe. Or maybe whoever it was didn't read the docs ;) Let's drop it.

generalredneck’s picture

I was going through and cleaning up the lump of open issues that I've started and not finished and I came across this one...

I did try to make an upgrade so that we could go ahead and get rid of the "Legacy"... but it was really difficult. I've spent all morning on it. I had a hard time exporting and then importing the action which was the only way I could figure out how to do that...

So instead I did almost exactly what you asked for. Enjoy!

shabana.navas’s picture

Status: Needs review » Reviewed & tested by the community

Thanks generalredneck. Tested this patch and it works as designed. As much as I hate seeing duplicate behaviors (Fetch node flagged by user & Fetch node flagged by user (Legacy), I guess it is something that we will have to stick in there for compatibility purposes. Hoping to commit this to the dev version asap. @Joachim, hope you confer.

joachim’s picture

> As much as I hate seeing duplicate behaviors (Fetch node flagged by user & Fetch node flagged by user (Legacy), I guess it is something that we will have to stick in there for compatibility purposes

Indeed! But once this is in, let's file a follow-up to remove the legacy one for D8.

Patch looks good to me. Go ahead and commit :)

generalredneck’s picture

Just a quick thought... something I didn't think of doing. You may ask in the rules IRC channel about if they would have a good idea on how to do an upgrade on rules that people import and currently have in affect to update from the legacy rule to the new one. This may make this a bit less painful to commit :P.

Oh and if you don't mind, could you please put the commit in as attributed to me? I would be much appreciative.

joachim’s picture

> do an upgrade on rules

Digging around in rules.install may reveal some existing upgrade procedures for this sort of thing.

Yup, I always try to attribute commits, but good idea to remind our new maintainer :)

@Shabana: I've probably already told you, but I suggest you use Dreditor, which makes making commit messages very easy, including attributing them to the primary author of the patch.

shabana.navas’s picture

@generalredneck, You will definitely get your name attributed. You did the work, you should get the kudos.

> Digging around in rules.install may reveal some existing upgrade procedures for this sort of thing.

Yeah, I will do some digging around and hopefully, we can find an easier solution for the D8 upgrade for the 'Legacy' issue.

@joachim, Yup, I have been using Dreditor since you gave me the heads-up on it. It really makes life a lot easier when it comes to checking patches and creating commit messages, which were always a pain for me. Thanks for that tip.

shabana.navas’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x-3.x branch.

joachim’s picture

Congrats!

It looks like your forgot to give the git attribution though -- using dreditor, you need to click on one of the names it pops up beneath the proposed commit message, and then use the entire git command on the command line to make that user the author of the commit (as well as being credited in the commit message).

shabana.navas’s picture

Doh! Is it too late or can I still change it?

joachim’s picture

We can't re-do commits once they're pushed to d.org git unfortunately!

shabana.navas’s picture

Yikes! @generalredneck, really sorry. Definitely won't happen again. Will make sure you get your proper dues in the next commit for the issue: https://drupal.org/node/2054927.

generalredneck’s picture

Don't worry too much about it... **It happens.

Status: Fixed » Closed (fixed)

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

joachim’s picture

I don't know if it was this patch or another of the changes to our Rules support, but something has broken -- any chance someone could take a look at #2077017: Most Recent Update (7.x-3.1) Broke Rules Configurations?