Follow-up to #2898344: Add filters to the comments administration pages

Problem/Motivation

Provide semantic machine names (instead of mystery IDs) and correct titles (using the correct case and matching the menu tabel labels) of the for the displays comment administration view (newly added in 8.4.x).

Proposed resolution

TBD

Remaining tasks

TBD

User interface changes

Only in the Views UI:

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

annajl created an issue. See original summary.

xjm’s picture

Title: Give semantic display names to all admin views displays » Give semantic display names to comment admin views displays
Status: Needs review » Active

I think we should first change the comment display names since that view was just added in this release and it has two page displays that are semantically different. It's less of an issue for admin/content and admin/people since they have a single page display.

xjm’s picture

Issue tags: +Novice

A novice contributor could probably create this patch also!

xjm’s picture

Status: Active » Needs review
Issue tags: -Novice
FileSize
838 bytes

Okay view display machine names are a pet peeve of mine and it seems especially silly to ship with mystery IDs for a view we just added only to change it later (and not be able to have 8.5.x or whatever match the view on 8.4.x sites), so I rolled the patch myself.

xjm’s picture

Title: Give semantic display names to comment admin views displays » Provide correct display name and title for comment admin view displays
FileSize
1002 bytes
591 bytes

Noticed a related bug while I was in there (the display titles do not match the case of the tabs, which we fixed already in the original patch) and then made the display machine name correlate directly with the user-facing labels.

xjm’s picture

With a screenshot to show what it's fixing in the Views UI.

The files admin view has a similar issue with the machine name, but that's less compelling to fix for 8.4.0 by tomorrow since it's been in core since 8.0.0. We wouldn't provide an upgrade path for the file admin view since it could seriously screw with people's custom code, plus we shouldn't be modifying sites' config anyway for such changes (sites own their config, not modules).

Edit: Fixed a fantastically run-on sentence.

xjm’s picture

Issue summary: View changes

The last submitted patch, 4: comment-2901778-4.patch, failed testing. View results

The last submitted patch, 5: comment-2901778-5.patch, failed testing. View results

jibran’s picture

It just needs display name updates in \Drupal\Tests\comment\Kernel\Views\CommentAdminViewTest::testFilters()

jibran’s picture

+++ b/core/modules/comment/tests/src/Kernel/Views/CommentAdminViewTest.php
@@ -0,0 +1,267 @@
+    $this->doTestFilters('page_1');
...
+    $this->doTestFilters('page_2');

I meant these.

Status: Needs review » Needs work

The last submitted patch, 6: comment-2901778-5.patch, failed testing. View results

xjm’s picture

Status: Needs work » Needs review
FileSize
1.81 KB
856 bytes

Thanks @jibran.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Perfect!

  • catch committed 9a38944 on 8.5.x
    Issue #2901778 by xjm, jibran, annajl: Provide correct display name and...

  • catch committed 424300e on 8.4.x
    Issue #2901778 by xjm, jibran, annajl: Provide correct display name and...

catch credited catch.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Status: Fixed » Closed (fixed)

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