Moving a filter to the end of a Filter group in filter rearrange doesn't work but does allow dropping in invalid regions

This is still an actual problem (8.4.x). But there is more wrong with drag and drop in the rearrange form than the title suggests.
The problem renders the UI pretty useless, marking this as major.

Drag and drop when creating a new group is problematic, you can drag to zones that shouldn't be available:

For existing groups, the group ID is not updated after a drop in a different group:

Up !

Doesn't work on Drupal 8.5.x
:(

Is there a patch ?

Here's patch #6 rerolled against HEAD. I also think we shouldn't postpone this major bug for a phpunit-related issue that has not been commented on for 12 months.

By manual testing I can confirm the latest patch does fix the original problem with moving a filter to the end of the previous filter group. :)

However it doesn't fix the additional bug reported in comment #9 that renders the drag-and-drop interface useless for creating a new filter group.

IMHO let's get the original bug fixed first, and then move on to the second one in #3024891: Drag-and-drop builder won't let me arrange filter groups.

New version using WebDriverTestBase instead of the deprecated JavascriptTestBase, otherwise still the very same patch from #9 rerolled against HEAD. Don't know if the one test failure disappears, otherwise we need to look into that test again.

Manual testing shows the failing test correctly detects the additional bug, see enclosed test-only patch.

The test here passes with the latest patch from #2215857: Behaviors get attached to removed forms applied too

Wow. So I tried to merge the two patches (#20 and #2215857-44: Behaviors get attached to removed forms) for testing. Hope I did it well so the tests pass and are significant.

Tried to apply the merged patch in #22 against a test code base and all is well.

$ patch -p1 < 2825446-moving-a-filter-to-the-end-of-a-Filter-group-in-filter-rearrange-doesnt-work-22.patch
patching file core/misc/ajax.es6.js
patching file core/misc/ajax.js
patching file core/modules/views_ui/js/views-admin.js
patching file core/modules/views_ui/tests/src/FunctionalJavascript/FilterCriteriaTest.php

Before the functionality was completely broken / unusable both from a UX and feature POV. With the patch I can add a new group and easily move rows around in the new group. More importantly things are finally saved and working correctly. I've tested exposed filters and I can return (exposed) results correctly without any issue. Good job!

I see #2215857: Behaviors get attached to removed forms was recently updated. Should we update the merged patch accordingly for further testing?

Sure. Go forward!

Updated the merged patch.

EDIT: actually I think I've messed up the merge here. Please ignore.

New attempt at merging the 2 issues.

This should be the most up-to-date combined patch.

#2215857: Behaviors get attached to removed forms is now in core, both 8.6.x and 8.7.x. This needs reroll and cleanup now that a "combined patch" would no longer apply.

Thanks!
-Derek

Cool that the other issue got in!
This is a bug, so fix in 8.6.x-dev first.

Just the fix, no further tests.

I installed HEAD to see what the new behavior was like after #2215857: Behaviors get attached to removed forms got committed. To my surprise, everything seems to be working as expected 🤨. I'd like someone else to please confirm/deny this statement because it looks odd to me it'd work all of a sudden without the patch from this issue.

@anavarre: You may have above patch in your composer.json, do you? For me the bug still exists with #2215857-49: Behaviors get attached to removed forms applied but #31 not. Please check again.

Otherwise I would reintroduce the tests, so we can get this to RTBC.

@Pancho: no, I tried from scratch with 8.7.x locally and tried again just now with a remote test install. Still couldn't reproduce the issue.

I've switched this to 8.7.x because this is the version against which we should be fixing the bug first and leave it up to core committers to backport to 8.6.x - Could you please try with 8.7.x and report back if you see anything different?

The test-only file in #20 still fails, so that would indicate the problem still exists in HEAD

Here we go

Updated the IS and title (finally, sorry @dmsmidt!)

I understand we have a test-only patch that demonstrates the problem, but at the risk of being a PITA here, I still cannot reproduce against 8.7.x - I tried several fresh instances with different stacks, and rancomposer clear-caches before installing several times as well. I've attached a test view which takes taxonomy term names and TIDs.

• Populate the default Tags vocabulary with 3, 4 terms to see the view in action.
• Try and filter out in the (view) preview by name AND tid to see different terms show up accordingly.

I can create filter groups just fine on my end, without the patch in this issue. Perhaps I'm doing something wrong but I cannot see what exactly ATM.

Re: #38 nice work! Almost RTBC...

1. +++ b/core/modules/views_ui/js/views-admin.js
   @@ -450,7 +450,7 @@
   -        if ($(this.rowObject.element).prev('tr').is('.group-message') && !groupField.is('.views-group-select-' + groupName)) {
   +        if (!groupField.is('.views-group-select-' + groupName)) {
   

   Fix looks great!

2. +++ b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterCriteriaTest.php
   @@ -61,13 +62,42 @@ public function testFilterCriteriaDialog() {
   -    // Checks that the admin summary is not double escaped.
   -    $this->drupalGet('admin/structure/views/view/who_s_online');
   -    $page = $this->getSession()->getPage();
   -    $this->assertNotNull($page->findLink('User: Last access (>= -15 minutes)'));
   -
   

   Why are we removing this test coverage? Seems unrelated to the fix here. :/

3. +++ b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterCriteriaTest.php
   @@ -61,13 +62,42 @@ public function testFilterCriteriaDialog() {
   +    // Assert dragging a filter works (#2717629).
   

   I don't remember seeing us put issue nids into comments like this. The nid is also #2717629: Add filter group for a View fails which on a very quick skim doesn't seem related to what this assertion is testing for. Long day, please excuse me if I'm confused. ;)

Re: #41: I tested manually, and yes, it's still broken in 8.7.x HEAD. To see the bug:

1. Create a view with a lot of filters (does not matter what they are). At least 4.
2. Click on the "Rearrange" tab to bring up the filter group dialog
3. Create a 2nd filter group
4. Move one filter to the new group
5. Apply
6. Note that you have 2 filter groups
7. Click on "Rearrange" again
8. Try to move a filter from group 1 to the end of group 2.
9. At this point, if you click "Show row weights", you'll see the filter is still in the wrong group.
10. Apply
11. Notice that the filter didn't move and is still in group 1.

I'd make a series of screenshots but that'd take too long. ;)

I'll RTBC as soon as #2 is addressed. #3 is just informational / nit.

Thanks! This bug has been bothering me for ages, will be very happy to see it finally fixed in core. :)

Cheers,
-Derek

Thanks, your step-by-step helped. I wasn't reproducing exactly like that (less filters and not paying attention to the weight). Very glad to see this fixed as well.

Addressed #2 to get the ball rolling. As to #3, it seems we're adding NIDs in other core files already. E.g.

core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php
91:    // but some MTAs incorrectly replace LF with CRLF. See #234403.
core/lib/Drupal/Core/Session/SessionManager.php
214:    // as #2238561 remains open.

Seems there was a reason why those lines got removed. Re-tested #38 and it still returns back green.

Marking RTBC to solicit core committers opinion on the patch readiness.

Sorry, that's not really how RTBC works. ;) If we have known issues, we sort them out ourselves before we bump to RTBC.

Maybe @Lendude can comment on this point? What's up with #42.2?

It seems like that assertion has nothing to do with this issue. Not clear why re-adding it caused #44 to fail, especially since on a quick skim it doesn't seem like it failed in the re-added assertion.

Okay, I dug a little deeper. #44 re-added the hunk, but in totally the wrong place. ;) No wonder it's failing. I'll re-roll properly. Stay tuned.

Okay, that's better. The assertion in question is a stand-alone assertion and can't be mixed in with a new page get in the middle of all the filter group stuff the rest of this test case is doing. Moved it to the very top of testFilterCriteriaDialog(). This now passes locally. Bot should agree.

Also, at @Lendude's request in Slack, I'm removing the nids from the comments (#42.3). Confirmed that we don't (normally) do that and if folks want to find the nids, they can git blame.

Interdiffs relative to #38 and #44 for the interested reader.

Enjoy,
-Derek

🤦wrong copy paste, sorry!

Thanks for re-rolling this cleanly. Back to RTBC now that this was sorted out.

RTBC++

Mmm. Love the taste of patches that are one-line fixes and bazillions of tests. ;)

Committed and pushed to 8.8, and since it's a major bug fix, also backported to 8.7. Thanks!

This change should have been applied to the ES6 file, and then compiled. At the moment, whenever you compile JavaScript, this change gets reverted.

Applied the patch, compiled JS, no other changes then the patched es6 file, so this checks out. Sorry for missing this!

Good point! Whoops, we missed. #58 is indeed RTBC.

Thanks/sorry,
-Derek

Oopsie. :( Thanks for the fix!

Committed and pushed to 8.8.x and 8.7.x. Thanks!