Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fearlsgroove’s picture

Component: Rules Core » Rules Engine
Status: Active » Needs review
FileSize
2.19 KB

.. and the patch

Status: Needs review » Needs work

The last submitted patch, rules-list-has-count.patch, failed testing.

fearlsgroove’s picture

Ooo look the test bot is working. Patch against git -dev attached ...

fearlsgroove’s picture

Status: Needs work » Needs review

status

Status: Needs review » Needs work

The last submitted patch, rules-list-has-count-condition-1284266-3.patch, failed testing.

fearlsgroove’s picture

Status: Needs work » Needs review

Hrm busted tests

fago’s picture

Status: Needs review » Needs work

Makes sense to me. Anyone else who could need this and wants to give it a test?

+++ b/modules/data.rules.inc
@@ -475,6 +475,32 @@ function rules_data_condition_info() {
+          'description' => t('A multi value data element to have its count compared, specified by using a data selector, e.g. "node:author:name".'),

This is a bad example, as it is not multi-valued.

+++ b/modules/data.rules.inc
@@ -475,6 +475,32 @@ function rules_data_condition_info() {
+          'description' => t('The count to compare the data count with.'),
...
+        'op' => array(
+          'type' => 'text',
+          'label' => t('Operator'),

Usually we have $op being the second parameter.

fearlsgroove’s picture

Updated patch addressing Fago's feedback. Also attached a simple example rule.

fearlsgroove’s picture

Status: Needs work » Needs review

status...

mitchell’s picture

Title: List has count condition » Condition: "list has count"
Version: 7.x-2.0-rc2 » 7.x-2.x-dev

bump

mitchell’s picture

Assigned: Unassigned » fago
Status: Needs review » Reviewed & tested by the community

This seems to relate to #1525582: Action: generate a list from an integer.

Makes sense to me. Anyone else who could need this and wants to give it a test?

@fago: Do you want to mark this as 'postponed: needs info' and wait for more reviews? I changed the status based on the bot's review and "Updated patch addressing Fago's feedback", but haven't looked into it further.

mitchell’s picture

Assigned: fago » Unassigned
Status: Reviewed & tested by the community » Needs review

I applied the patch to 2.1, and it succeeded with offsets, but I couldn't get the import to work because it said the condition didn't exist (even though I cleared my cache). The condition also didn't appear in the conditions list. I'm guessing it needs a reroll, but since I didn't test it 100% correctly, I'm just setting it back to needs review.

fago:

Anyone else who could need this and wants to give it a test?

I wish this solicitation had brought more help, but I think there's too much noise right now in the issue queue (as usual, but particularly now). Perhaps we'll be able to pick up more reviewers with some outreach in the near future. This gives me a couple of ideas on how to help people help the Rules maintainers more effectively.

I removed the assign because you obviously knew about the issue already, and so you know that the ones I do put will have the requisite need to call your attention.

PatchRanger’s picture

@mitchell I have rerolled your patch against latest dev. It is applied nicely for me.
I changed the name of operator to more appropriate : list_has_count -> list_count_is. It makes this patch consistent with other Rules condition (for example, Data Comparison condition is called rules_condition_data_is).

@mitchell offtop

I wish this solicitation had brought more help, but I think there's too much noise right now in the issue queue (as usual, but particularly now). Perhaps we'll be able to pick up more reviewers with some outreach in the near future. This gives me a couple of ideas on how to help people help the Rules maintainers more effectively

I am currently working on a project that could help you. It is on development stage now but I will move it to production soon. Its goal is to make patch crowd funding platform for drupalers. Please see here . There you could find a manifesto of the project and the roadmap to make it work.

mitchell’s picture

Assigned: Unassigned » klausi
Status: Needs review » Reviewed & tested by the community

@klausi: Applied and tested.
"Makes sense to me." -fago #7
---

Just fyi, #1600272: Calculating with lists: sum, mean, count, min, max is one issue that's directly relevant, but there are a lot of issues with math ops that still need work. Won't go into that here.

@Staratel: Here was my reaction... "WHOAAAH!!" :) I have a number of thoughts about this.. Would you please make a project on d.o for some issue tracking?

PatchRanger’s picture

@mitchell Thank you for your amazing feedback, Mitchell. :)
It has an issue queue already : http://drupal.org/project/issues/1717054 .
In order not to annoy everyone who is following this thread we should continue our discussion there.

MrPeanut’s picture

Component: Rules Engine » Rules Core
Status: Reviewed & tested by the community » Needs work

Hope this is okay changing back to needs work.

I applied the patch from #13 cleanly but when attempting to clear my cache, I got this error:

ReflectionException: Function rules_action_data_convert() does not exist in ReflectionFunction->__construct() (line 1691 of C:\Program Files\wamp\www\drupal\sites\all\modules\rules\includes\rules.core.inc).

Using Drupal 7.15 and Rules 7.x-2.2+3-dev.

mitchell’s picture

Status: Needs work » Needs review
FileSize
640 bytes
1.93 KB

Please test #8.

It applies cleanly and works well. I tested it again with the attached component. (It requires Conditional Rules.)

#16: That error doesn't look related to this issue. Please checkout the latest code from git, instead of using a snapshot release.

#13: I like the naming in #8 better. Attached interdiff will help if others want to review both patches.

mitchell’s picture

Assigned: klausi » Unassigned
Priority: Normal » Major
Status: Needs review » Needs work

#17 doesn't apply, and I couldn't get #8 to recognize an added list variable or a list created by Views Rules' "Collect views results".

Setting back to needs work and looking forward to someone else's updated patch.

Setting to major since this is a powerful feature addition.

See also #474840: Condition: User has # of content type for a possible use case.

David Stosik’s picture

Priority: Major » Normal
Status: Needs work » Needs review

My bad.

David Stosik’s picture

Works for me, can't reproduce error on #17.
Mitchell's resulting patch is attached (applies on 2.x).

Regards,
David
This patch is part of the #1day1patch initiative.

GuyPaddock’s picture

Is this supposed to work when checking how many values (i.e. deltas) a multi-valued field has?

It accepts the top-level field as a list value, but the count is always 1. I'm guessing it's because the count of things inside the field is always 1 unless the site is multi-lingual (i.e. the field is an array, keyed by language, where the values of that array are the actual field values).

For example:

$myentity->myfield[LANGUAGE_NONE] = array(each value for the field);

Ronino’s picture

#20 works great for me, thank you!

geek-merlin’s picture

Issue summary: View changes

Sorry for joining late.
While this might be useful imho it will be superfluous code.

We should implement #2211461: Add count property to list wrapper instead (which makes this *and* other use cases possible) and mark this WONTFIX.

cvharris’s picture

This issue was definitely resolved for me with #20. It really improves the usability for Rules by offering a pretty basic PHP function for your workflow. I was surprised this wasn't already a part of core.

In my instance I needed to add items to two different Entity Reference fields automatically given certain conditions, but since both fields' possible values was Unlimited there was no way to loop through each list of items and stop more than a certain amount from being added. Throwing this simple condition in to check the number of items already in the reference field helped me avoid worse purely-Rules solutions like creating a new VBO with contextual filters that would limit my results or something else. Push this to core, it is a stable and worthy function.

Similarly this should be included since it adds a simple calculation in case the count needs to be used as a variable in the rule.

mitchell’s picture

Status: Needs review » Reviewed & tested by the community

#20 is rtbc, based on #22 & #24.

@cvharris: Cheers to stepping up and providing valuable info! Will you provide patches too?

fago’s picture

Status: Reviewed & tested by the community » Fixed

Yep, this looks good and is a sensible addition. Committed, thanks!

  • Commit 0a56a35 on 7.x-2.x by fago:
    Issue #1284266 by fearlsgroove, mitchell, David Stosik, PatchRanger:...

Status: Fixed » Closed (fixed)

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