Implement action/condition reordering with drag & drop

I've made a start on this here: https://github.com/fago/rules/pull/452

Think I've got this working now :) Need review, and also needs tests.

Could do with thinking of some test scenarios. The best I can think of at the moment is two actions that create nodes and then compare the node ids to check the order...

Also I have not done nesting, just flat ordering. I was thinking we could do nesting in a follow up issue?

@yanniboi Can you add your changes in the patch file so that other can review your changes.

The patch is here:
https://patch-diff.githubusercontent.com/raw/fago/rules/pull/452.patch

I have found some issue is with button, I dont think so it should be in li tag.

Manjit.Singh thanks for the input. Correct me if I'm wrong, but I htink the li tag is not related to this patch and should therefore be discussed in a separate ticket?

@dasjo right !! This has not been caused by this patch. So we can create a new issue for this.
setting this issues it need review.

@dasjo #2754867: Bullet should not be there in action button. I have created an issue for that. Please review.

Thanks, this looks already pretty good. I've added some comments to the PR. As noted we should add some basic test coverage for that also - fortunately core got some javascript enabled test case meanwhile. So let's leverage that.

Setting to needs work for the comments and the tests.

Thanks for this patch, but i find that if i have more than 2 conditions, the sort is not happening properly. When i debugged, I saw a drupal error which was not shown on the browser. The line that caused the error is line 43 on rules/src/Plugin/RulesExpression/RulesAnd.php

uasort() expects parameter 2 to be a valid callback, first array member is not a valid class name or object.

Thanks
Sukanya

Ok heres the solution, changed the line @uasort($conditions, [$this->conditions, 'expressionSortHelper']) to @uasort($conditions, [$this, 'expressionSortHelper']);

This change needs to be done wherever the method is called!

Thanks,
Sukanya

Thanks for finding - could you provide an updated pull request?

I don't want to be rude, I am just asking, is it already fix it?
May I have access to last version by cloning the repository on Github (https://github.com/fago/rules) or should I download the patch and apply it directly to my local instead?

I'm splitting this into two parts (explained below).

The first part is to modify Rules core so that expressions have weights, and so that evaluation of expressions occurs in weight order. I am posting a patch for that here, in this issue. The patch is very similar to what @yanniboi posted, but instead of explicitly sorting each time the expression is used (which puts the burden on the using code and requires a lot of code duplication) I put the sorting into the ExpressionContainerBase so that the using code is automatically returned a sorted array with no extra work required.

The second part is to modify the UI to make use of weights. This means implementing tabledrag on the Reaction Rule and Rules Component edit forms, as well as saving the weights when those forms are saved. I am providing this part as a patch to D8 Rules Essentials because that module has a much-improved UI for Rules that I will be moving into Rules proper over the next few months. It would be a lot more work for me to implement this part in both the current Rules UI and in the new UI, and I don't think the extra effort is worth it.

So to fully test this feature you will need to apply the patch here in this comment plus you will need to install D8 Rules Essentials and apply the patch from #3100154-2: Implement action/condition reordering with drag & drop.

Regarding #11, I don't see any comments from @fago in the PR, so I can't address those. I also haven't written tests yet, but I think this issue should NOT be blocked on tests because it is such an important feature. Yes, we need tests, but let's get the functionality in first because not many people will ever try this patch so having live people use the functionality on real sites will be the best test of the feature.

Testbot is currently broken. It should be fixed later today by #3099984: Composer require failure since Drupal 8.8.0 ...

This patch fixes the test failures in #16 and removes the 1 coding standards violation.

The tests that failed were Unit tests, and they failed because they didn't know to expect calls to the new getWeight() method. Although "Segmentation fault" is not really the most helpful message that the testbot could have shown ...

Whoops, I committed another patch while the test was queued, which affected one of the same files as this patch. So now I need a slight re-roll to correct that.

I think it will be easy to add some Unit tests to test the execution order of conditions and actions - these should be added as extra test function in tests/src/Unit/*Expression*.php. We just have to mock the weights and set up multiple conditions and actions that depend on order so we can test the end result. For example if we have an always FALSE condition followed by an always TRUE condition in a rules_and group then the TRUE condition should never be evaluated because the FALSE condition will cause the AND evaluation to stop. Etc.

But the UI tests are more difficult but they can be handled in #3100154: Implement action/condition reordering with drag & drop after this patch gets committed.

I'm inclined to commit this patch pretty soon, because it should have no effect on existing sites - it just adds the concept of weight but since all weights are initialized to 0 there should be no difference unless you're manipulating rules programmatically. The difference for most users comes only when you add the UI part where you can set the weights ... but that's independent of what's in this patch.

I'm inclined to commit this patch pretty soon, because it should have no effect on existing sites

To help this, I have read through the patch, and followed it, and yes it would be good to get this in soon.

I just have one question, which might have an obvious answer (so apologies if I have missed something easy). But what is the reason for adding the new submitForm() function in /src/Form/Expression/RuleExpressionForm.php when none existed before?

+  /**
+   * {@inheritdoc}
+   */
+  public function submitForm(array &$form, FormStateInterface $form_state) {
+    $this->rule->getConditions()->getFormHandler()->submitForm($form, $form_state);
+    $this->rule->getActions()->getFormHandler()->submitForm($form, $form_state);
+  }

Just wondering how we were getting away without it before?

Just wondering how we were getting away without it before?

The Reaction Rule edit page is pretty complicated, and is composed of many nested forms. The RuleExpressionForm displays the "Conditions" and "Actions" block, but nothing else on that page. The "Conditions" block is in turn provided by ConditionContainerForm and the "Actions" block is provided by ActionContainerForm. Before this patch, neither ConditionContainerForm nor ActionContainerForm did anything but display information - they needed only a form() method to return a static form array that could not be modified by the user. So neither of those container forms needed a submit, and likewise the RuleExpressionForm that called them didn't need a submit because none of its subforms could change anything.

When we add tabledrag to the ConditionContainerForm and ActionContainerForm, we now have user input - changing condition and action weights via drag or via direct input to the "weight" textfield. So now we need submitForm functions on both those container forms. But since these forms are nested, it's not the Form API that calls the build/validate/submit functions, it's the parent form that has to do that explicitly. That's the reason we need the submit function on RuleExpressionForm - it is needed to call the submit functions on the ConditionContainerForm and ActionContainerForm that are nested within it.

Your question got me to thinking, however, that I had only tested the submit functionality on the action container and condition container - I didn't test to make sure we weren't breaking the "Settings" information that is also part of what gets submitted when the "Save" button is pressed. Alas, this patch does break that functionality so now you can't change the Rule name, tags, or description. So there's something missing in the patch which I will now look into ...

I'm also going to have to add a test for the "Settings" information, plus Unit tests for the order of evaluations of conditions and actions.

I applied the patch from #20 on a clean site WITHOUT the UI changes from #3100154-2: Implement action/condition reordering with drag & drop and I am still able to change the rule name/tags/description. That means that the problem I mentioned in #23 is not due to the patch in this issue, but is a problem with the UI patch.

So I don't see anything negative about committing this patch as-is, once I write the Unit tests for the execution order.

Thank you for the detailed answers, yes that all makes sense. Sorry that my question has caused another area for you to work on, but better to discover that now rather than later. I will review the tests when you post them.

Here's a new patch. I added two tests: one checks the execution order of conditions and one checks the execution order of actions. If this passes on the testbot I will be committing it.

NOTE: I did determine the cause of the problem I mentioned in #23 and #24, and it WAS in the UI portion of the code, and there is now a new patch in #3100154: Implement action/condition reordering with drag & drop that fixed the issue.

Committed. This feature will now work if you have the latest -dev version of D8 Rules Essentials installed.

I am leaving this issue open until that code from D8 Rules Essentials is moved into Rules proper, and I am tagging this issue D8RE so I remember to do that.

Adding related issue as the commit in #27 above is the one which caused the test failures on PHP5

The UI part of the code has now been moved into Rules via #3102627: Move changes to ReactionRuleEditForm/RulesComponentEditForm from D8RE into Rules

This feature should now be fully functional in Rules.