Steps to reproduce:

  1. Create a few nodes
  2. Create view with table format
  3. Add an edit link
  4. Add a delete link
  5. Edit the table format settings and put the delete link into the edit link column
  6. Take a peek the table and see the delete link is missing

Found a little mistake in `template_preprocess_views_view_table`:

// ...
if (!empty($field_output) && empty($column_reference['content'])) {
  // Place the field into the column, along with an optional separator.
  if (!empty($column_reference['content'])) {
  // ...

How can the 'content' be empty and not empty? I removed the check in the outer-if.
Small patch attached in first comment.

I assume this also needs a test as well. I haven't made one yet.

CommentFileSizeAuthor
#10 2399263-10.patch5 KBspadxiii
#10 2399263-10-test-only.patch3.76 KBspadxiii
#7 2399263-7.patch3.57 KBspadxiii
#3 2399263-3.patch3 KBspadxiii
#3 2399263-3-test-only.patch2.33 KBspadxiii
#1 2399263-1.patch682 bytesspadxiii

Comments

spadxiii’s picture

spadxiii commented
Status: Active » Needs review
StatusFileSize
new 2399263-1.patch682 bytes
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work
Issue tags: +VDC, +Needs tests

Sorry, but every bug needs a test :(

spadxiii’s picture

spadxiii commented
Status: Needs work » Needs review
StatusFileSize
new 2399263-3-test-only.patch2.33 KB
new 2399263-3.patch3 KB

Here's a small test for the combined columns.
Not sure if this is the correct way to write the test, but it's what I could come up with for now.

It's the first test I've ever written for views :)

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/views/src/Tests/Plugin/StyleTableTest.php
@@ -77,4 +77,13 @@ public function testAccessibilitySettings() {
+
+    $this->assertText('Drummer, Drummer', 'Ensure the job columns are joined into a single column');

assertText() is not really a full test ... shouldn't we use some xpath select to check whether the result is really as expected?

The last submitted patch, 3: 2399263-3-test-only.patch, failed testing.

spadxiii’s picture

spadxiii commented

What kind of tests are you thinking of?

I think I noticed that the td-class is wrong as well. I'll have to double check that and extend the patch for that. If I remember correctly, the td-class is set to the field that's added to the column of the first field.

spadxiii’s picture

spadxiii commented
StatusFileSize
new 2399263-7.patch3.57 KB

I also noticed that the attributes of the second column override those from the first column.
For example, the class of the first column is gone, and only the one from the second column is there.

`$column_reference['attributes']` was being reset every time, instead of checking if it already existed. I've added the check in the attached patch.

Only thing that remains, is extend the test.

dawehner’s picture

dawehner
Primary language German
commented

SpadXIII

Did my response in IRC helped you?

spadxiii’s picture

spadxiii commented

dawehner, I got called away so I didn't get around doing anything anymore then. I'll probably have some time later to fix some tests. I'll get back on irc then, to check if I understood it correctly.

spadxiii’s picture

spadxiii commented
StatusFileSize
new 2399263-10-test-only.patch3.76 KB
new 2399263-10.patch5 KB

Extended the tests quite a bit (removed the assertText completely).

The test data now adds the job field in 2 separate columns. In the test, it combines them into one. That way, some before/after tests can be done.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/views/src/Tests/Plugin/StyleTableTest.php
@@ -77,4 +77,36 @@ public function testAccessibilitySettings() {
+    // Combine the second job-column with the first one, with ', ' as separator.
+    $view = entity_load('view', 'test_table');
+    $display = &$view->getDisplay('default');
+    $display['display_options']['style']['options']['columns']['job_1'] = 'job';
+    $display['display_options']['style']['options']['info']['job']['separator'] = ', ';
+    $view->save();

Awesome that you changed it!

RTBC if it passes

The last submitted patch, 10: 2399263-10-test-only.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: -Needs tests

We do have some test coverage now.

spadxiii’s picture

spadxiii commented

yep, and the tests fail for the correct reason! (and pass with the whole patch merged)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 4a9c76a and pushed to 8.0.x. Thanks!

  • alexpott committed 4a9c76a on 8.0.x 
    Issue #2399263 by SpadXIII: Table format combine fields into single...

Status: Fixed » Closed (fixed)

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