Problem/Motivation

During the fast development of views to d8 some parts of the UI didn't got converted, because there hasn't been tests for that small feature and you shouldn't drop backward compability layers without it.

Proposed resolution

This issue fixes the linking to other displays UI and adding tests for it.

Original report by [username]

// Text of original report here.
(for legacy issues whose initial post was not the issue summary)

C:\xampp\htdocs\views\lib\Drupal\views\Plugin\views\display\DisplayPluginBase.php
    1227        $link_display = empty($this->view->display[$display_id]) ? t('None') : check_plain($this->view->display[$display_id]['display_title']);
2 matches in C:\xampp\htdocs\views\lib\Drupal\views\Plugin\views\display\DisplayPluginBase.php
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
943 bytes

Patches!

dawehner’s picture

Here are tests + the fix

Status: Needs review » Needs work

The last submitted patch, views-1810816-2.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.44 KB
4.36 KB

This should fix the tests.

xjm’s picture

Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.x-dev
Component: Code » views.module
Status: Needs review » Needs work
Issue tags: +Needs reroll
fastangel’s picture

Status: Needs work » Needs review
FileSize
4.59 KB

I attached one patch with the reroll.

tim.plunkett’s picture

+++ b/core/modules/views/lib/Drupal/views/Tests/UI/DisplayTest.phpundefined
@@ -55,7 +55,7 @@ public function testRemoveDisplay() {
+    $this->assertTrue($this->xpath('//div[contains(@class, views-display-deleted-link)]'), 'Make sure the display link is marked as to be deleted.');

This is still wrong, assertTrue on $this->xpath is ALWAYS true. It's asserting a constant, not a string, and should use replacement. Something like

$element = $this->xpath("//a[contains[@class, :class)]', array(':class' => 'views-display-deleted-link'));
$this->assertTrue(!empty($element), 'Make sure...');
+++ b/core/modules/views/lib/Drupal/views/Tests/UI/DisplayTest.phpundefined
@@ -181,4 +181,26 @@ public function testDisplayPluginsAlter() {
+    // Link

Needs a real comment.

damiankloip’s picture

FileSize
4.75 KB

I think we should also assert the xpath against the actual link we deleted the display for and not just that we have that class.

Added the comment too.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/views/lib/Drupal/views/Tests/UI/DisplayTest.phpundefined
@@ -182,4 +183,26 @@ public function testDisplayPluginsAlter() {
+    // The form redirects to the master display.
+    $this->drupalGet($path);
...
+    // The form redirects to the master display.
+    $this->drupalGet($path);

We maybe should open an follow up on that, #1826358: Redirect the user to the active editing display after saving.

xjm’s picture

#8: 1810816-8.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

This needs a re-roll.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
4.78 KB

Rerolled, thanks again Tim! (That damn views UI patch) ;)

Status: Needs review » Needs work

The last submitted patch, 1810816-12.patch, failed testing.

dawehner’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.07 KB

Just a rerole.

Status: Needs review » Needs work

The last submitted patch, durpal-1810816-15.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.23 KB

Much better this time.

The good parts of views, if something fails then it fails everywhere :)

xjm’s picture

This could use something resembling a summary. :)

dawehner’s picture

Updated.

damiankloip’s picture

FileSize
4.22 KB

Rerolled.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This has been RTBC before, thanks for the rerole!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated