If you have lots of actions or conditions, then drag-n-drop re-ordering of actions doesn't work. The current delta for the weight widget is capped out at 20. This patch checks the number of items in the rule and increases the delta accordingly.

Issue fork rules-2065291

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn’s picture

Status: Active » Needs review
Issue tags: +Novice, +Needs manual testing
FileSize
1.21 KB

I see this must have affected someone else at some point since when I came to apply my patch against the dev branch, the delta was already increased to 50. While that helps, this should more fully fix the issue because I need more than 25 actions (half of 50) in some of my rules.

TR’s picture

Issue summary: View changes

Patch still applies. Anyone want to review this?

sano’s picture

FileSize
324.61 KB

Not sure I understand the purpose of this patch, but even after it is applied, I am unable to reorder actions in one of my rules. Once I start dragging an action with many "children," the handle jumps to the left margin of the "viewport" and can't be made a child of another indented action. See the attached pic for illustration

Chris Matthews’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

The patch in #1 does not apply to the latest 7.x-2.x-dev and needs a reroll.

Checking patch ui/ui.core.inc...
error: while searching for:
    // Recurse over all element childrens or use the provided iterator.
    $iterator = isset($iterator) ? $iterator : $this->element->elements();
    $root_depth = $this->element->depth();
    foreach ($iterator as $key => $child) {
      $id = $child->elementId();

error: patch failed: ui/ui.core.inc:985
error: ui/ui.core.inc: patch does not apply
TR’s picture

Status: Needs work » Needs review

Patch does still apply, with fuzz 1. Why don't you provide a re-roll to eliminate the fuzz? It's just one letter that needs to be removed ...

What this issue needs is a REVIEW, so perhaps you can apply the patch and test it out and report on your experience with it?

delacosta456’s picture

hi
i can confirm that @sano is correct .. i am facing the exact issue of what he said and show in his attached image ... The applied successfully for me with phpstorm and PHP7 on the current version but this issue is still arround.
Can anyone help us ?
Thanks

TR’s picture

Category: Bug report » Feature request
Priority: Normal » Minor

@sano:

Not sure I understand the purpose of this patch

I'm not sure why it's unclear - actions have weights, which are integers used to determine the order of the actions. Currently in Rules, weights can range from -50 to 50. These weights are normally hidden in the UI and managed automatically by the core Drupal JavaScript, but if you click on "Show row weights" you can manually change them to manually reorder the actions without drag and drop. If you do this you will see that the possible values are between -50 and 50.

The problem is this range can only accommodate 50 different values, so if you have more actions than that you don't have enough available values to uniquely identify the order of the actions, and so changing the order would fail.

This was a bigger problem when the range was -20 to 20, because you would encounter a problem when you had "only" 20 actions. That was raised to 50 back in 2012 by #1573014: Elements in table are not in correct order after saving rule with more than 10 elements.

The patch proposes to make this range a variable value, ranging from -(# of actions + 10) to +(# of actions + 10). That way no matter how many actions you have, you will always have an extra 10 free values for weight, and you will always be able to rearrange things.

Because this currently works properly as long as you don't have more than 50 actions, I'm going to mark this as a feature request. IMO if you need more than 50 actions you really shouldn't be implementing your task in Rules, you should be writing the code directly. I don't think the current situation is really inconveniencing anybody. The patch will effectively remove this limit if it is an inconvenience, but on one but the original poster has expressed a concern about this limit in the past 6+ years since the limit was changed to 50, so I think it's justified to call this a "Minor" issue.

To quote @fago from #1573014-19: Elements in table are not in correct order after saving rule with more than 10 elements:

50 should be fine. Everything else is insanity anyway

@sano:

but even after it is applied, I am unable to reorder actions in one of my rules

If you are having problems both before and after the patch, then it's not a problem with the patch. The patch was never intended to modify the dragging behavior. And what you describe seems to be totally unrelated to this patch. First, you're using Conditional Rules, so if you are having problems with dragging then that is the first thing to look at and eliminate as a cause - try to reproduce the problem without Conditional Rules. Second, dragging is a function of the core tabledrag JavaScript, so if that's not behaving properly it's still not a Rules issue - try settings the weights manually to see if this is a problem with the JavaScript. Third, you didn't export a simplified version your Rule (using only core Rules conditions and actions, using only core Drupal entities and data types...) so we can't easily try what you're doing - we would have to re-create this from scratch with Conditional Rules and a dozen different actions. That's not very convenient - if you want help you should really try to make this easy to reproduce / confirm. I can't even tell how many actions you have, so I can't see if you're even close to the current 50 limit.

Regardless, I don't see how this patch could change anything about the dragging behavior.

@delacosta456:

this issue is still arround.

What issue? The 50 actions limit? That's what this thread is about. If you're having a dragging issue then provide the necessary information to reproduce your problem and post it in a new thread.

I'm inclined to commit this patch, but there hasn't been a single real review since it was posted. A real review would be to reproduce the error (problems reordering actions when you have more than 50 actions, but no problems with fewer than that) then apply the patch and test if the error went away.

delacosta456’s picture

FileSize
41.13 KB

hi @TR

Thanks for the long explanations .. please look at attached file to see my exported rule.

Inside it i can move some action like fetch entity or execute efq_rules but actions like Unpublish , when touch it it automatically moved to left

sano’s picture

FileSize
8.51 KB

@TR thank you for the detailed comment. What I meant to write was that I was not sure whether the problem I encountered was related to (fixable by) the patch attached to this issue.

Anyways, in an attempt to help resolve this issue I created a test rule containing 51 actions (attached) and run it against unpatched version 7.x-2.11. I do not see any problem when shuffling actions around, so I did not test the patch. I did not use the conditional actions even though they are available to me, in an attempt to make the test focused. However, @heddn mentions that his actions contained conditions, so maybe it is necessary to have them in the rule to reproduce the issue in the first place. Maybe this issue should be set to "can't reproduce" here and refer it instead to the Conditional Rules module.

TR’s picture

FileSize
8.9 KB

The range limit (-50 to 50) applies to each level of indentation separately. Thus, you need to have 51 or more actions at the same level of indentation for this to be a problem. You example Rule (very helpful, thank you) only has 45. I modified it to contain 6 more actions, and this does indeed show problems with drag and drop, exactly as described in #1573014: Elements in table are not in correct order after saving rule with more than 10 elements.

However, after I apply the patch, I can see that the range of allowed weights has changed for this Rule (Click "Show row weights" to see these) and I stop having problems with drag and drop. I would appreciate it if you could confirm this on your site.

So I think this is still a valid issue to keep open, but still a minor one because if you encounter this bug you have way too many actions, IMO.

sano’s picture

I imported the extended rule 51-actions.txt and moved around some actions. I do not see any unexpected repositioning of actions after saving new configurations, but when I have the weights shown, I see the last item has value -50 before the patch is applied.

After applying the patch the last action's weight changes to 51 and reshuffled actions weights show correct values.

When it comes to the opinion that the number of actions in any given rule should not be "too excessive" (say exceed 20-ish actions), it is probably sound, even from the point of view that such rules become hard to scrutinize. I am afraid though that novices might get carried away and use the Rules mechanism for implementing business logic of their web apps, given the lack of clear guidance how to do this in a more intelligent way. But that is a topic for a different forum/thread I guess.

EDIT: I should mention that my test was performed against Rules 7.x-2.11 and the patch applied with fuzz:
Hunk #1 succeeded at 948 (offset -37 lines).
Hunk #2 succeeded at 962 (offset -37 lines).

TR’s picture

I do not see any unexpected repositioning of actions after saving new configurations, but when I have the weights shown, I see the last item has value -50 before the patch is applied

Yes, that's the same behavior as discussed in #1573014: Elements in table are not in correct order after saving rule with more than 10 elements - the reordering will take place when you press "Save". In my example rule from #10, the weight of -50 will move the last action to the top, and the rule will fail because the last action requires an added variable from a previous action.

reenaraghavan’s picture

Assigned: Unassigned » reenaraghavan
reenaraghavan’s picture

Assigned: reenaraghavan » Unassigned