Rearranging fields and sorts causes existing items to be removed

Tagging

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

Can we have a test for this please? Thanks!

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

Rerolled this patch.

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.

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...?)

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

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.

Fixed comments in #17

Thank you!

+++ 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.

Here we go :)

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

Committed f7ded17 and pushed to 8.x. Thanks!