Problem

Filter groups adding and removing does not work at all.

The involved files are:
- core/modules/views_ui/js/views-admin.js
- core/modules/views_ui/lib/Drupal/views_ui/Form/Ajax/RearrangeFilter.php

Steps to reproduce

  • Edit a View
  • Add at least one Filter
  • Click on "And/Or Rearrange" in the Filter options dropdown
  • Click on "Create new filter group"
  • Nothing happens

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because adding and removing filter groups is broken
Issue priority Major because it is a UX issue of a feature which doesn't work.
Prioritized changes The main goal of this issue is usability
Files: 
CommentFileSizeAuthor
#89 not-hidden.png87.58 KBmarthinal
#89 hidden.png80.48 KBmarthinal
#85 interdiff-2168893-83-85.txt2.51 KBmarthinal
#85 2168893-85.patch5.32 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,079 pass(es). View
#83 2168893-82.patch6.32 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,406 pass(es). View
#83 interdiff-2168893-81-82.txt1.04 KBmarthinal
#8 views_filter_group-2168893-8394165.patch4.07 KBeuphoric_mv
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch views_filter_group-2168893-8394165.patch. Unable to apply patch. See the log in the details link for more information. View
#11 core-js-views-add-group-2168893-11.patch1.4 KBnod_
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-js-views-add-group-2168893-11.patch. Unable to apply patch. See the log in the details link for more information. View
#13 2168893-13.patch1.22 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,728 pass(es). View
#14 2168893-14.patch2.76 KBeuphoric_mv
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,725 pass(es). View
#17 2168893-17.patch2.91 KBeuphoric_mv
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,728 pass(es). View
#17 interdiff-2168893-17.txt818 byteseuphoric_mv
#22 2168893-18.patch3.01 KBeuphoric_mv
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,849 pass(es). View
#22 interdiff-2168893-18.txt933 byteseuphoric_mv
#31 views_filters_groups-2168893-31.patch2.44 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,856 pass(es). View
#36 interdiff-2168893-31-36.txt797 bytesmarthinal
#36 views_filters_groups-2168893-36.patch2.43 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,406 pass(es). View
#39 interdiff-2168893-36-39.txt1.18 KBmarthinal
#39 views_filters_groups-2168893-39.patch1.92 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,412 pass(es). View
#39 Screen Shot 2015-04-13 at 22.45.09.png66.79 KBmarthinal
#45 interdiff-2168893-39-45.txt820 bytesmarthinal
#45 views_filters_groups-2168893-45.patch2.09 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,434 pass(es). View
#47 2168893-47.patch4.35 KBmarthinal
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2168893-47.patch. Unable to apply patch. See the log in the details link for more information. View
#47 interdiff-2168893-45-47.txt2.53 KBmarthinal
#59 node_admin_page_is-2473989-10.patch2.58 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,604 pass(es), 12 fail(s), and 0 exception(s). View
#59 interdiff.txt659 byteslauriii
#62 views_filters_groups-2168893-62.patch4.31 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,601 pass(es). View
#68 views_filters_groups-2168893-68-only-test.patch3.83 KBmarthinal
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,713 pass(es), 7 fail(s), and 0 exception(s). View
#68 views_filters_groups-2168893-68.patch8.14 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,716 pass(es). View
#71 interdiff-2168893-68-71.txt4.33 KBmarthinal
#71 views_filters_groups-2168893-71-only-test.patch3.82 KBmarthinal
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,766 pass(es), 7 fail(s), and 0 exception(s). View
#71 views_filters_groups-2168893-71.patch8.11 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,775 pass(es). View
#75 interdiff-2168893-71-75.txt3.34 KBmarthinal
#75 2168893-75.patch8.88 KBmarthinal
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2168893-75.patch. Unable to apply patch. See the log in the details link for more information. View
#81 2168893-81-only-test.patch1.31 KBmarthinal
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,393 pass(es), 7 fail(s), and 0 exception(s). View
#81 2168893-81.patch6.38 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,385 pass(es). View

Comments

Cottser’s picture

Confirmed.

euphoric_mv’s picture

Assigned: Unassigned » euphoric_mv
dawehner’s picture

Priority: Normal » Major
Issue tags: +VDC

Thank you for reporting the issue. I consider this as a major bug, at least.
Also updating some tags.

nod_’s picture

Issue tags: +JavaScript
dawehner’s picture

Priority: Major » Critical

There are several bugs:

* The actual add functionality does not work (even if you show the button itself)
* The button is hidden from the no-JS variant

Given that the functionality is pretty useless so this is a huge regression compared to d7.

euphoric_mv’s picture

There are bugs in the javascript and php files too.

I found it in javascript and i will post a patch file today, but in php file, concrete in core/modules/views_ui/lib/Drupal/views_ui/Form/Ajax/RearrangeFilter.php, we can't use logic like in Drupal 7.

For example, there is no $form_state['clicked_button'] element and there are more bugs.

catch’s picture

Priority: Critical » Major

It's useless but it's a self-contained bug in the views UI, so major.

euphoric_mv’s picture

FileSize
4.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch views_filter_group-2168893-8394165.patch. Unable to apply patch. See the log in the details link for more information. View

In this patch I fixed javascript bug for click on "Create new filter group" link.
It work now but there is a problem because it doesn't trigger ajax. It opens new page with JSON output and when I go back and click again on rearrange the filters, new filter group is shown. I can't figure out why this happens.

Please test it and if someone has idea how to solve the problem, that would be great.

euphoric_mv’s picture

Assigned: euphoric_mv » Unassigned
nod_’s picture

The JS error is because event is not a defined variable in that scope. function definition needs to be changed to clickAddGroupButton: function (event) { no need for return false;.

I don't know why the ajax stuff doesn't trigger though.

nod_’s picture

Status: Active » Needs work
FileSize
1.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-js-views-add-group-2168893-11.patch. Unable to apply patch. See the log in the details link for more information. View

The input clicked is not ajax enabled so it wouldn't do anything.

Still doesn't work but at least no JS error.

euphoric_mv’s picture

this.addGroupButton.trigger('mousedown').trigger('submit'); doesn't work. I putted this.addGroupButton.click(); and then event.preventDefault(); and then it fire button.

Also, by adding class 'use-ajax', button for add group get class 'ajax-processed' but on submit it still open page with JSON ouput.

I noticed that other buttons on view has class 'drupal-ajax-processed'.

marthinal’s picture

FileSize
1.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,728 pass(es). View

About the json output we have the same behavior for d7 using .click(). If we add the #ajax attribute with a 'path' for example, we obtain the class 'drupal-ajax-processed' added to the input. Also the form submit is executed as expected but for the moment this is where I am. Maybe this info is helpful .

euphoric_mv’s picture

FileSize
2.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,725 pass(es). View

@marthinal Your post was very helpful. Thank you.

I needed to change some other lines in PHP too, like $form_state['view']->addFormToStack('rearrange-filter', $form_state['display_id']); and it wirks for me now.

Same problem appears with removing filter groups.

dawehner’s picture

  1. +++ b/core/modules/views_ui/js/views-admin.js
    @@ -519,7 +519,7 @@
    -    clickAddGroupButton: function () {
    +    clickAddGroupButton: function (event) {
           // Due to conflicts between Drupal core's AJAX system and the Views AJAX
           // system, the only way to get this to work seems to be to trigger both the
           // mousedown and submit events.
    

    WOW, we need javascript tests!

  2. +++ b/core/modules/views_ui/lib/Drupal/views_ui/Form/Ajax/RearrangeFilter.php
    @@ -199,12 +206,18 @@ public function buildForm(array $form, array &$form_state) {
    +    // Get form path
    +    $form_path = empty($form_state['path']) ? current_path() : $form_state['path'];
    +
    

    It is certainly wrong to still rely on current_path(), isn't there something on the request object which does the same?

euphoric_mv’s picture

Assigned: Unassigned » euphoric_mv
euphoric_mv’s picture

FileSize
2.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,728 pass(es). View
818 bytes

I tried to find path on the request object but no results.

euphoric_mv’s picture

Status: Needs work » Needs review

The last submitted patch, 8: views_filter_group-2168893-8394165.patch, failed testing.

The last submitted patch, 11: core-js-views-add-group-2168893-11.patch, failed testing.

euphoric_mv’s picture

So, we need to replace current_path in whole views system, right?

euphoric_mv’s picture

FileSize
3.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,849 pass(es). View
933 bytes
// Get requestUri from request object.
    $requestUri = $this->getRequest()->getRequestUri();

I don't know is this right solution but i am getting path that I need. Please advice.

blueminds’s picture

FileSize
2.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,804 pass(es). View

Rerolled and reviewed. JS file is fixed in HEAD now. Looks fine, only not sure why we need to remove "filter" from

-      $form_state['view']->addFormToStack('rearrange-filter', $form_state['display_id'], 'filter');
+      $form_state['view']->addFormToStack('rearrange-filter', $form_state['display_id']);

Other than that I think RTBC.

Elijah Lynn’s picture

@#23

The 'filter' appears to be for a mandatory parameter of $type.

public ViewUI::addFormToStack($key, $display_id, $type, $id = NULL, $top = FALSE, $rebuild_keys = FALSE)
https://api.drupal.org/api/drupal/core%21modules%21views_ui%21src%21View...

Elijah Lynn’s picture

Issue summary: View changes
FileSize
151.8 KB

@#23

I am seeing these console errors when clicking the "Create new filter group" and nothing happens.

POST http://sandbox/d8/d8/admin/structure/views/ajax/rearrange-filter/345/default 404 (Not Found) jquery.js?v=2.1.0:8556
jQuery.ajaxTransport.send jquery.js?v=2.1.0:8556
jQuery.extend.ajax jquery.js?v=2.1.0:8084
$.fn.ajaxSubmit jquery.form.js?v=3.5:257
Drupal.ajax.eventResponse ajax.js?v=8.0-dev:342
(anonymous function) ajax.js?v=8.0-dev:263
jQuery.event.dispatch jquery.js?v=2.1.0:4371
jQuery.event.add.elemData.handle jquery.js?v=2.1.0:4057
jQuery.event.trigger jquery.js?v=2.1.0:4286
(anonymous function) jquery.js?v=2.1.0:4828
jQuery.extend.each jquery.js?v=2.1.0:381
jQuery.fn.jQuery.each jquery.js?v=2.1.0:137
jQuery.fn.extend.trigger jquery.js?v=2.1.0:4827
$.extend.clickAddGroupButton views-admin.js?v=8.0-dev:531
jQuery.extend.proxy.proxy jquery.js?v=2.1.0:516
jQuery.event.dispatch jquery.js?v=2.1.0:4371
jQuery.event.add.elemData.handle
Uncaught object ajax.js?v=8.0-dev:552
Drupal.ajax.error ajax.js?v=8.0-dev:552
Drupal.ajax.ajax.options.complete ajax.js?v=8.0-dev:247
$.fn.ajaxSubmit.options.complete jquery.form.js?v=3.5:219
jQuery.Callbacks.fire jquery.js?v=2.1.0:3047
jQuery.Callbacks.self.fireWith jquery.js?v=2.1.0:3159
done jquery.js?v=2.1.0:8198
jQuery.ajaxTransport.send.callback

Elijah Lynn’s picture

Status: Needs review » Needs work
Elijah Lynn’s picture

My actual path is http://sandbox/d8/admin and not http://sandbox/d8/d8/admin like it is searching for, and there are multiple paths of http://sandbox/d8/d8 in the network tab. I don't have time to debug this just yet but I think this is the main issue in my case of having Drupal installed in a subdirectory.

Elijah Lynn’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,164 pass(es). View
1.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch interdiff-2168893-23-28.patch. Unable to apply patch. See the log in the details link for more information. View

Here is a patch that uses getPathInfo() instead of getRequestUri and removes the subdirectory from the URL. It also adds 'filter' back.

Now on first click it does add a new filter group but on subsequent clicks it throws an error.

Status: Needs review » Needs work

The last submitted patch, 28: interdiff-2168893-23-28.patch, failed testing.

Elijah Lynn’s picture

Okay, some work that still needs to be done.

  1. The cancel button doesn't work
  2. Subsequent "create new filter groups" throw the console error and don't do anything of course. POST http://sandbox/d8/admin/structure/views/ajax/rearrange-filter/345/default/filter 404 (Not Found)
lauriii’s picture

Status: Needs work » Needs review
FileSize
2.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,856 pass(es). View

Rerolled the patch

lauriii’s picture

Assigned: euphoric_mv » Unassigned
joelpittet’s picture

Assigned: Unassigned » nod_

Assigning to @nod_ because maybe he can clear up why the JS isn't doing the ajax requests correctly and closing the window once called or maybe point us in the right direction here.

Selfishly this is holding up our twig conversions but also it's quite a big bug for beta as well.
#1963978: Convert theme_views_ui_build_group_filter_form() to Twig

lauriii’s picture

Status: Needs review » Needs work
nod_’s picture

Assigned: nod_ » Unassigned

Looked into it it's out of my hands. The no-js version of adding a group doesn't work at all.

From what I understand the group configuration form doesn't get added to "the stack". It also means that when using JS there will be a CloseDialogCommand added to the ajax commands to close everything and start fresh. The JS bug is a side effect of a PHP problem I don't know how to fix.

marthinal’s picture

Status: Needs work » Needs review
Issue tags: +drupaldevdays
FileSize
797 bytes
2.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,406 pass(es). View
75.79 KB

I was reviewing this patch with @dawhener and found the problem.

tim.plunkett’s picture

  1. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
    @@ -202,12 +209,23 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +    $request_uri = $this->getRequest()->getPathInfo();
    

    We don't use this anywhere

  2. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
    @@ -202,12 +209,23 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +    $form_path = $form_state->get('path') ? current_path() : $form_state->get('path');
    

    This line doesn't make sense to me. First, why call it twice? Second, why use the result only if it casts to FALSE? Thirdly, current_path() isn't even a function any more.

jhedstrom’s picture

Issue summary: View changes
FileSize
431.86 KB

This still doesn't seem to work quite right in manual testing. After adding a filter group, I can drag items in, but the group is not saved.

marthinal’s picture

FileSize
1.18 KB
1.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,412 pass(es). View
66.79 KB

@tim.plunkett Done.

@jhedstrom I think you need to drag more to the bottom. I was testing and it works... weird.

thanks!

jhedstrom’s picture

Issue summary: View changes
FileSize
119.46 KB

Confirmed that dragging more than one filter all the way to the bottom will allow the group to be saved, but surely there are use-cases for only adding one filter to a new group (eg, a single filter belonging to a group joined by OR).

Also, after the group is created, there's an escaping issue on the main form:

Status: Needs review » Needs work

The last submitted patch, 39: views_filters_groups-2168893-39.patch, failed testing.

jhedstrom’s picture

Actually, adding a single filter works if it is dragged all the way to the bottom. I interpreted #39 as meaning more than one :)

tim.plunkett’s picture

+++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
@@ -224,7 +216,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+        'path' => \Drupal::request()->getRequestUri(),

$this->getRequest()->getRequestUri(), I guess

marthinal’s picture

Assigned: Unassigned » marthinal
Status: Needs work » Needs review
Related issues: +#2469563: Double-escape on Views filters
FileSize
820 bytes
2.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,434 pass(es). View

@tim.plunkett Yes thanks

Added related bug about double-escaped filters.

b0unty’s picture

Status: Needs review » Needs work

The remove group button still doesnt work, altough the group is removed if its left empty but its bit confusing to have a button that does nothing.

So should we remove the button, or make the button work?

marthinal’s picture

Status: Needs work » Needs review
FileSize
4.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2168893-47.patch. Unable to apply patch. See the log in the details link for more information. View
2.53 KB

Removed duplicated code, fixed Create new group and remove group + avoid duplicate select for the operator.

Thanks to nod_ for the help with the js for the select for the operator :)

nod_’s picture

This works for me, I'll let tim or someone else confirm it.

Status: Needs review » Needs work

The last submitted patch, 47: 2168893-47.patch, failed testing.

marthinal queued 47: 2168893-47.patch for re-testing.

The last submitted patch, 47: 2168893-47.patch, failed testing.

marthinal queued 47: 2168893-47.patch for re-testing.

marthinal’s picture

Status: Needs work » Needs review
dawehner’s picture

I'm a bit confused as the inline template changes are missing?

marthinal’s picture

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Both nod_ and marthinal have been testing this manually in Views. I also just ran it on simplytest.me, and it fixes the bug described in the issue summary: you can now add/remove groups in the Filter rearrange dialog.

I took a look at the code. The changes are straightforward and the code looks clean to me.

No tests because it's a JS bug. Sigh.

Anyway, let's get this bug fix in!

Quick beta eval: This fixes a Major UI Bug, and there are no API changes or disruption.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: 2168893-47.patch, failed testing.

nod_’s picture

lauriii’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,604 pass(es), 12 fail(s), and 0 exception(s). View
659 bytes
JeroenT’s picture

Status: Needs review » Needs work

@lauriii,

I think this is the wrong patch.

The last submitted patch, 59: node_admin_page_is-2473989-10.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
4.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,601 pass(es). View

This should be the right one :) Sorry for the previous one!

joelpittet’s picture

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

Back to RTBC

dawehner’s picture

I'm curious whether we can write a test for at least the second half of the problem (the part in PHP).

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Good question.

joelpittet’s picture

@marthinal and @nod_, thoughts on possible tests for mostly #47?

marthinal’s picture

Assigned: Unassigned » marthinal

We can create a group filter and then remove it. We don't need js enabled for it, so maybe this is enough to cover this part...

I can work on it.

marthinal’s picture

FileSize
3.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,713 pass(es), 7 fail(s), and 0 exception(s). View
8.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,716 pass(es). View

I'm using the test I have created at #2469563: Double-escape on Views filters.

The last submitted patch, 68: views_filters_groups-2168893-68-only-test.patch, failed testing.

joelpittet’s picture

Status: Needs review » Needs work

@marthinal that is awesome, thanks for separating out the test and pass patches.

There are some coding standards nitpicks and one not so nit pick outside the test, would you mind tackling them? I'd RTBC this again after these are tackled.

Also thanks for moving that todo move one too!

  1. +++ b/core/modules/views_ui/js/views-admin.js
    @@ -551,10 +551,7 @@
    -      this.table.find('#' + event.data.buttonId).trigger('submit');
    +      this.table.find('input#' + event.data.buttonId).trigger('mousedown').trigger('submit');
    

    Can we avoid this overly specific selector? I had this as a pet peeve in D7 because I couldn't covert theme_button to a button element because these things in panels and views.

  2. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
    @@ -128,6 +135,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +          '#ajax' => array(
    +            'path' => $this->getRequest()->getRequestUri(),
    +          )
    

    Nit: Short array syntax :s/array()/[]/ and can be one line.

  3. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
    @@ -203,11 +213,15 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      '#ajax' => array(
    +        'path' => $this->getRequest()->getRequestUri(),
    +      ),
    
    +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
    @@ -0,0 +1,89 @@
    +
    +
    

    Ditto

  4. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
    @@ -0,0 +1,89 @@
    +class FilterUITest extends ViewTestBase {
    +
    +
    

    Nit: Extra line break.

  5. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
    @@ -0,0 +1,89 @@
    +  public static $testViews = array('test_filter_in_operator_ui', 'test_filter_groups');
    

    Nit: Short array syntax s/array()/[]/

  6. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
    @@ -0,0 +1,89 @@
    +  public static $modules = array('views_ui', 'node');
    ...
    +    $this->drupalCreateContentType(array('type' => 'page'));
    

    Nit: here too [].

  7. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
    @@ -0,0 +1,89 @@
    +   * Tests that "Limit list to selected items" option is saved as expected when editing
    +   * the Content type filter from the UI.
    

    Nit: Wrap on 80 chars.

  8. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
    @@ -0,0 +1,89 @@
    +    $admin_user = $this->drupalCreateUser(array('administer views', 'administer site configuration'));
    

    Nit: Short array syntax s/array()/[]/

  9. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
    @@ -0,0 +1,89 @@
    +    $edit = array(
    +      'options[expose][reduce]' => TRUE,
    +    );
    

    Nit: Short array syntax s/array()/[]/ and can be one line.

  10. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
    @@ -0,0 +1,89 @@
    +    $admin_user = $this->drupalCreateUser(array('administer views', 'administer site configuration'));
    ...
    +    $this->drupalPostForm(NULL, array(), t('Create new filter group'));
    +    $this->drupalPostForm(NULL, array(), t('Create new filter group'));
    ...
    +    $this->drupalPostForm(NULL, array(), t('Remove group 3'));
    ...
    +    $this->drupalPostForm(NULL, array(), t('Remove group 3'));
    

    Nit: Short array syntax s/array()/[]/

  11. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
    @@ -0,0 +1,89 @@
    +    $this->assertNoRaw('<span>Group 3</span>', 'Group 3 has not been added yet.');
    +
    +  }
    

    Nit: Extra line break.

Any changed/new diff hunks can get the short array syntax(don't go out of the hunks or it could cause needless rerolls on other patches and make the committers and reviews cross:P)

marthinal’s picture

Status: Needs work » Needs review
FileSize
4.33 KB
3.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,766 pass(es), 7 fail(s), and 0 exception(s). View
8.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,775 pass(es). View

@joelpittet sure! and thanks for reviewing.:)

1. trigger('mousedown') is needed otherwise it does not work.

Here the same situation:

    clickAddGroupButton: function (event) {
      // Due to conflicts between Drupal core's AJAX system and the Views AJAX
      // system, the only way to get this to work seems to be to trigger both the
      // mousedown and submit events.
      this.addGroupButton
        .trigger('mousedown')
        .trigger('submit');
      event.preventDefault();
    },

The rest are fixed as well.

The last submitted patch, 71: views_filters_groups-2168893-71-only-test.patch, failed testing.

joelpittet’s picture

Oh I wasn't complaining about the mousedown. I'm sure that was needed however weird that looks. I was just mentioning the need for 'input' in the CSS selector.

It makes it hard to theme buttons by hardcoding what element that class is on.

Also missing a few ending commas, I'll identify them when I get into the office on dreditor.

joelpittet’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/views_ui/js/views-admin.js
    @@ -551,10 +551,7 @@
    +      this.table.find('input#' + event.data.buttonId).trigger('mousedown').trigger('submit');
    

    So %s/input#'/#'/

  2. +++ b/core/modules/views_ui/js/views-admin.js
    @@ -567,7 +564,11 @@
    +      var titleRows = $('tr.views-group-title').once('duplicateGroupsOperator');
    

    Would love the same thing here but I don't know where views-group-title is used so we can leave the tr. on here.

  3. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
    @@ -128,6 +135,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +          '#ajax' => [
    +            'path' => $this->getRequest()->getRequestUri(),
    +          ]
    

    Needs a comma at the end, and can make that one line.

  4. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
    @@ -203,11 +213,15 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      '#ajax' => [
    +        'path' => $this->getRequest()->getRequestUri(),
    +      ]
    

    Same here, one line and comma at the end.

  5. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
    @@ -0,0 +1,90 @@
    +    $edit = [
    +      'options[expose][reduce]' => TRUE
    +    ];
    

    One line, OR at least if not needs a comma after TRUE.

marthinal’s picture

Status: Needs work » Needs review
FileSize
3.34 KB
8.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2168893-75.patch. Unable to apply patch. See the log in the details link for more information. View

1. Done. ( Sorry added while working on it :) )

2. Without removing tr for the moment.

3. Done.

4. Done.

5. Done

Thanks!

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Awesome thank you @marthinal

olli’s picture

I'm still seeing #38 with this patch. Do we have an issue to fix it?

+++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
@@ -128,7 +135,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+          '#ajax' => ['path' => $this->getRequest()->getRequestUri()],

@@ -197,18 +205,20 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      '#ajax' => ['path' => $this->getRequest()->getRequestUri()],

We use 'url' now so these lines are same as '#ajax' => ['url' => NULL],, right? The 'use-ajax-submit' class with trigger('click') could also work here.

joelpittet’s picture

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

I'm not sure the answer to that @olli, so I'm needs reviewing this to see if someone knows?

marthinal queued 75: 2168893-75.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 75: 2168893-75.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
6.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,385 pass(es). View
1.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,393 pass(es), 7 fail(s), and 0 exception(s). View

For the moment rerolled... Not sure if we should use url instead of path. So needs review and this is a quick change anyways...

The last submitted patch, 81: 2168893-81-only-test.patch, failed testing.

marthinal’s picture

FileSize
1.04 KB
6.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,406 pass(es). View

I can see info about it and I think that using url we avoid to call getRequest() method.

      // Assign default settings. When 'url' is set to NULL, ajax.js submits the
      // Ajax request to the same URL as the form or link destination is for
      // someone with JavaScript disabled. 
YesCT’s picture

Status: Needs review » Needs work

Thanks for #83. Seems to make sense about url.

in #70 @joelpittet said

Any changed/new diff hunks can get the short array syntax(don't go out of the hunks or it could cause needless rerolls on other patches and make the committers and reviews cross:P)

  1. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
    @@ -120,7 +127,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -        $form['remove_groups'][$id] = array(
    +        $form['remove_groups'][$id] = [
    

    we do not need to change this for the patch to work.

  2. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
    @@ -197,18 +205,20 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -        '#attributes' => array('class' => array('views-remove-checkbox')),
    +        '#attributes' => ['class' => ['views-remove-checkbox']],
    

    same.

  3. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
    @@ -197,18 +205,20 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -    $form['actions']['add_group'] = array(
    +
    +    $form['actions']['add_group'] = [
    

    same.

  4. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
    @@ -73,6 +73,26 @@ public function testFiltersUI() {
    +        // Create 2 new groups.
    

    nit, comment indent is off.

marthinal’s picture

Status: Needs work » Needs review
FileSize
5.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,079 pass(es). View
2.51 KB

Done!

Thanks! :)

joelpittet’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

@marthinal thank you for the remaining cleanup!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

There's no confirmation that #38 is solved or is a followup.

joelpittet’s picture

Issue tags: +Novice

@alexpott, you're right it does work but there just seems to be a bug left with two drop areas, not sure if that is a template level thing or just JS duplicating dom elements... so yes this does still needs work.

Will tag this as novice for someone with javascript skills can look at why this is the case.

When you create a new group there are two drop cells and the second one is where the filter needs to be dropped into that second group.

marthinal’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
80.48 KB
87.58 KB

@joelpittet I think this is because the description is hidden. And not sure if it should be hidden...

Hidden:

Not hidden:

joelpittet’s picture

Interesting @marthinal could you post a patch with it unhidden? We can test it further. I don't know if it's supposed to be hidden and why that would effect the drop areas. Maybe a class name collision or something.

marthinal’s picture

Issue tags: +DrupalCampSpain2015

@joelpittet Before adding it to the patch I prefer to know if we want to hide it.

views-admin.js

      // Get rid of the explanatory text around the operator; its placement is
      // explanatory enough.
      this.operator.find('label').add('div.description').addClass('visually-hidden');

I was discussing with @lauriii about it and maybe we should open a new issue about this problem and fix this one.

marthinal’s picture

@joelpittet Before adding it to the patch I prefer to know if we want to hide it.

views-admin.js

      // Get rid of the explanatory text around the operator; its placement is
      // explanatory enough.
      this.operator.find('label').add('div.description').addClass('visually-hidden');

I was discussing with @lauriii about it and maybe we should open a new issue about this problem and fix this one.

lauriii queued 85: 2168893-85.patch for re-testing.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

I created a follow up for broken dragging thing. I don't see how it would fit she scope of this issue. #2493945: Views filters dragging in the grouping view is broken

joelpittet’s picture

@marthinal Since you mentioned that unhiding it fixed the drag drop groups I was hoping you'd post a patch so we could review your findings. But yes this patch as it is now makes things kinda work more than it does so moving it to a follow-up seems reasonable considering the amount of time it took to get this far.

lauriii’s picture

There is already solution suggested on the other issue and it relates to problem in the tabledrag - so no need to worry about that here :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Okay lets fix the tabledrag in the other issue.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed d982616 and pushed to 8.0.x. Thanks!

  • alexpott committed d982616 on 8.0.x
    Issue #2168893 by marthinal, euphoric_mv, lauriii, Elijah Lynn, nod_,...

Status: Fixed » Closed (fixed)

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

sb56637’s picture

Version: 8.0.x-dev » 7.43

This is also broken in the current release of Drupal 7, as described in the original bug report.

sb56637’s picture

Status: Closed (fixed) » Active
joelpittet’s picture

Issue tags: +needs backport to D7
sb56637’s picture

Looks like this is actually a conflict with the hide_submit module:
https://www.drupal.org/node/1939426

sb56637’s picture

David_Rothstein’s picture

Version: 7.43 » 8.0.x-dev
Status: Active » Closed (fixed)
Issue tags: -needs backport to D7

The original bug report was always about Drupal 8 (the early comments in this issue refer to it as a regression compared to Drupal 7).

Also, Views is not in Drupal 7 core.

I think you're probably right that the problem you're experiencing is covered by the issue you linked to. (In my experience, filter groups do work fine in Views for Drupal 7 in the general case.)

certifiedGeek’s picture

I'm creating a site in Drupal 8.1.8 and the filter grouping is not working for me. I can create the filter group and add items to it but it does not save. Is this broken again, or are there other modules that cause this to break?

joelpittet’s picture

@certifiedgeek you may have a new issue. First search open issues and if not found, open a new one and post a link here to that issue

nod_’s picture

thtas’s picture

@certifiedgeek
Same here with 8.1.10, but i have a lot of custom code and contrib modules (in various un-stable states) which could be causing it.
No errors in the JS console or watchdog to indicate a problem.

egouleau’s picture

same here :( totally broken even after update to 8.2 ...

tomsaw’s picture

Same for me running Drupal 8.2.3

anou’s picture

Same problem has comment #106 on drupal 8.2.3:
I can create new group, add field to it but after applying my changes, nothing is saved. Coming back to it, I find only one group...

typotraum’s picture

Same for me -> filter groups are gone

handkerchief’s picture

Same for me --> #112
But with drupal 8.2.4 :(

handkerchief’s picture

Does anyone reopen this issue or do we have to create a new issue? I'm not waiting as long as my comment predecessors :)