If when editing a view, you rearrange a list of fields and then apply the changes, all the items in the list will be removed. This happens to both the list of fields and the items in the sort criteria.

Just to add some extra data for troubleshooting:
You do not have to actually rearrange any of the items. Just clicking the Apply button will remove the items.
Canceling the dialog will not remove the items.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pcambra’s picture

Issue tags: +VDC

Tagging

Anonymous’s picture

Assigned: Unassigned »
oadaeh’s picture

I forgot to add earlier that the same functionality in the filters list is not broken (at least not in this way).

Anonymous’s picture

Priority: Normal » Major
Status: Active » Needs review
FileSize
824 bytes

The rearrange handler for foreach'ing over $form_state['values'] rather than $form_state['values']['fields'].

Psikik’s picture

I experienced this issue also and the patch applies cleanly and fixes the bug.

oadaeh’s picture

Status: Needs review » Reviewed & tested by the community

The patch in #4 fixes the problem for me, too. It also seems to be correctly formed.

I'm marking it as RTBC, but we may want to wait for the testbot (just in case).

Status: Reviewed & tested by the community » Needs work
Issue tags: -VDC

The last submitted patch, 2002506-Cannot-Rearrange-Views-Fields-004.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
Issue tags: +VDC
Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

Resetting the issue to RTBC since that is what it was before the testbot went mental.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Can we have a test for this please? Thanks!

longwave’s picture

This was apparently mistakenly committed in http://drupalcode.org/project/drupal.git/commitdiff/db2df2d

Dries’s picture

Rerolled this patch.

Anonymous’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.83 KB

Added tests.

damiankloip’s picture

Status: Needs review » Needs work

Nice start!

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/RearrangeTest.phpundefined
@@ -0,0 +1,92 @@
+ * Tests the reordering of fields via AJAX.

Can we add an '@see \Drupal\views_ui\Form\Ajax\Rearrange' here too.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/RearrangeTest.phpundefined
@@ -0,0 +1,92 @@
+  private static $addedFields = array('node.nid', 'node.type');
+  private static $totalFields = array('title', 'nid', 'type');
...
+  private function createRandomView() {
...
+  private function assertFieldOrder($view, $fields) {

We don't use private, these should all be protected. Also, the properties need docblocks.

We generally don't use other modules in tests unless we really need to. We try to use views_test_data for as much of this as possible. So I think we should convert to use that maybe.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/RearrangeTest.phpundefined
@@ -0,0 +1,92 @@
+      'name' => 'Rearrange',

Rearrange fields? Maybe we should also test the reordering of sorts too.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/RearrangeTest.phpundefined
@@ -0,0 +1,92 @@
+   * A helper method which creates a random view.

This could just be 'Creates a random view' but I think it should also say via the wizard/ui too. Also, pretty much the same thing is used in DisplayTest, maybe we should look at moving a form of this method into UiTestBase? Also, we could just use a new test view instead, then there would be no need to create in the test, as we aren't testing that here.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/RearrangeTest.phpundefined
@@ -0,0 +1,92 @@
+   * Test that the order on the rearrange form matches the creation order if unchanged.
...
+   * Test that the new field order is respected.
...
+   * Test that a field is not deleted if a value is not passed back.

'Tests'

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/RearrangeTest.phpundefined
@@ -0,0 +1,92 @@
+    $this->assertFieldOrder($view, RearrangeTest::$totalFields);
...
+    $reversedFields = array_reverse(RearrangeTest::$totalFields);
...
+    $this->assertFieldOrder($view, RearrangeTest::$totalFields);

These don't really need to be static properties? If they do, we should use the static keyword and not the actual class name. Seems like a regular property would be fine though.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/RearrangeTest.phpundefined
@@ -0,0 +1,92 @@
+}

Needs newline at EOF.

pcambra’s picture

Here's a rework of the field test, I've removed the new view to rely on the test_view one, I think the only thing that we have left here is decide if we want the sort test or not... (shall we need to add then testing reorder on filters, area handlers, arguments...?)

pcambra’s picture

It's better if I give the testbot something to test :P, renaming the class file.

dawehner’s picture

This is basically RTBC. Thank you!

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/RearrangeFieldsTest.phpundefined
@@ -0,0 +1,95 @@
+ * Contains \Drupal\views_ui\tests\RearrangeTest.

Should also be Tests instead of tests

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/RearrangeFieldsTest.phpundefined
@@ -0,0 +1,95 @@
+ * @see \Drupal\views_ui\Form\Ajax\Rearrange

Nice!

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/RearrangeFieldsTest.phpundefined
@@ -0,0 +1,95 @@
+   * Gets the fields from the View.
+   */
+  protected function getViewFields($view_name = 'test_view', $display_id = 'default') {
...
+  /**
+   * Check if the fields are in the correct order.
+   */
+  protected function assertFieldOrder($view_name, $fields) {

Let's document the two parameters.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/RearrangeFieldsTest.phpundefined
@@ -0,0 +1,95 @@
+    $this->drupalPost('/admin/structure/views/nojs/rearrange/' . $view_name . '/default/field', $fields, t('Apply'));
...
+    $this->drupalPost('/admin/structure/views/nojs/rearrange/' . $view_name . '/default/field', $fields, t('Apply'));

Just remove the starting '/' as you never know whether drupal will be installed in a folder.

pcambra’s picture

Fixed comments in #17

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/RearrangeFieldsTest.phpundefined
@@ -0,0 +1,100 @@
+  /**
+   * Tests that the order on the rearrange form matches the creation order if
+   * unchanged.
+   */
+  public function testRearrangeFormInitialOrder() {
+    $view_name = 'test_view';
+
+    $this->assertFieldOrder($view_name, $this->getViewFields($view_name));
+  }
+
+  /**
+   * Tests that the new field order is respected.
+   */
+  public function testRearrangeValidValues() {
+    $view_name = 'test_view';
+    $reversedFields = array_reverse($this->getViewFields($view_name));
+
+    $fields = array();
+    foreach ($reversedFields as $delta => $field) {
+      $fields['fields[' . $field . '][weight]'] = $delta;
+    }
+
+    $this->drupalPost('admin/structure/views/nojs/rearrange/' . $view_name . '/default/field', $fields, t('Apply'));
+
+    $this->assertFieldOrder($view_name, $reversedFields);
+  }
+
+  /**
+   * Tests that a field is not deleted if a value is not passed back.
+   */
+  public function testRearrangeEmptyValues() {
+    $view_name = 'test_view';
+    $fields = array();
+
+    $this->drupalPost('admin/structure/views/nojs/rearrange/' . $view_name . '/default/field', $fields, t('Apply'));
+
+    $this->assertFieldOrder($view_name, $this->getViewFields($view_name));
+  }

Can we combine these into one test method... each test necessitates a full re-install of drupal and this seems overkill.

pcambra’s picture

Status: Needs work » Needs review
FileSize
2.18 KB
3.55 KB

Here we go :)

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

The interdiff definitely fixes #20 and it was RTBC before, so let's do this.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f7ded17 and pushed to 8.x. Thanks!

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