Follow-up to #1986606: Convert the comments administration screen to a view

Problem/Motivation

There is an utter lack of filters on the comment admin screen.

Proposed resolution

Add some exposed filters because it is a view now.

Remaining tasks

  • Agree on the filters.
  • Add filters.
  • Update path. Not needed see #13
  • Update path tests. Not needed see #13
  • Review.
  • Commit.
  • Rejoice.

User interface changes

Comment admin screens have filters now.

API changes

None.

Data model changes

None

CommentFileSizeAuthor
#39 add_filters_to_the-2898344-39.patch23.83 KBjibran
#39 interdiff-c3da8b.txt1.51 KBjibran
#38 add_filters_to_the-2898344-36.patch23.85 KBjibran
#38 interdiff-443dc8.txt6.13 KBjibran
#31 comment-admin-view-filters.png63.4 KBLendude
#30 add_filters_to_the-2898344-30.patch21.24 KBjibran
#30 interdiff-5ab16c.txt3.74 KBjibran
#27 add_filters_to_the-2898344-27.patch21.21 KBjibran
#27 interdiff-293974.txt5.92 KBjibran
#20 add_filters_to_the-2898344-19.patch24.16 KBjibran
#20 interdiff-6b2684.txt1.43 KBjibran
#18 Comments___Site-Install.png25.02 KBxjm
#18 confusing_language_options.png59.15 KBxjm
#15 add_filters_to_the-2898344-15.patch23.58 KBjibran
#15 interdiff-0d117c.txt2.2 KBjibran
#12 add_filters_to_the-2898344-12.patch25.77 KBjibran
#12 interdiff-58be9c.txt7.67 KBjibran
#5 add_filters_to_the-2898344-5.patch18.7 KBjibran
#5 interdiff-9616b0.txt3.61 KBjibran
#3 add_filters_to_the-2898344-3.patch16.21 KBjibran
#3 interdiff-b6a49f.txt1.05 KBjibran
#2 screenshot-d8.dev-2017-07-30-01-38-13.png45.09 KBjibran
#2 screenshot-d8.dev-2017-07-30-01-35-58.png16.52 KBjibran
#2 add_filters_to_the-2898344-2.patch15.16 KBjibran
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

jibran created an issue. See original summary.

jibran’s picture

Title: Add filters to the comments administration screen » Add filters to the comments administration pages
Issue summary: View changes
Status: Fixed » Needs review
FileSize
15.16 KB
16.52 KB
45.09 KB

Added a comment subject filters just like node title on node admin page.
Added author and username combine filter to by comment author name.
Added comment type filter just like node type filter.
Added language filter just like on node admin page.

Query:

jibran’s picture

andypost’s picture

+++ b/core/modules/comment/comment.post_update.php
@@ -36,3 +36,25 @@ function comment_post_update_enable_comment_admin_view() {
+  $entity_type_manager = \Drupal::entityTypeManager();
...
+  $entity_type_manager

there's no reason for ETM variable cos it used only once

jibran’s picture

The last submitted patch, 3: add_filters_to_the-2898344-3.patch, failed testing. View results

dawehner’s picture

Congratulations on getting the conversion in! I'm wondering whether we should do some simple testing of the exposed filters?

+++ b/core/modules/comment/comment.post_update.php
@@ -36,3 +36,31 @@ function comment_post_update_enable_comment_admin_view() {
+  if ($view = $view_storage->load('comment')) {
+    // If we have an existing view then update that.
+    $new_view
+      ->set('uuid', $view->uuid())
+      ->enforceIsNew(FALSE);
+  }
+  $new_view->save();

Do we consider it as fine to override a view someone might have adapted? I guess its fine, given we have never promised it?

jibran’s picture

Congratulations on getting the conversion in! I'm wondering whether we should do some simple testing of the exposed filters?

Sure

Do we consider it as fine to override a view someone might have adapted? I guess its fine, given we have never promised it?

I'm open to suggestions.

Lendude’s picture

Do we have any way to handle/detect if people made modifications to the View before we load the new version? Cause this way we just discard any updates that people have done.

I think a lot more people will benefit from adding the filters but this could theoretically lead to loss of data. And we can't really mark UI exposed config as @internal.

Edit: cross post with @dawehner, guess we thought the same thing :)

dawehner’s picture

If we would stay in 8.4.x, we could just override it, we wouldn't even need to provide an update path, given that we don't need to support one.

Let's assume this won't make it to 8.4.x, but rather just in 8.5. IN that case we could use the config hash stored in _core to determine whether things got changed.

jibran’s picture

Just for the reference core default_config_hash is:

_core:
  default_config_hash: CTRys5yrLceyvD5kd4rr-XLUceQHt-9x9C3Hnn8K5Rg

#2560049: Incorrect capitalisation of translatable strings, #2600576: MIME, not Mime / mime and #2712647: Update Symfony components to ~3.2 all made changes to the file admin default view without updating the existing view in storage. We can totally drop upgrade path for it if it won't make it to 8.4.x.

jibran’s picture

Added a test for expose filters. This is ready for review.

dawehner’s picture

Status: Needs review » Needs work

We can totally drop upgrade path for it if it won't make it to 8.4.x.

Let's a good idea as well. Given that we don't want an update path in 8.4.x, but then also want to drop one afterwards, let's just no worry about it anymore 🎆

dawehner’s picture

Nice work with the test!

andypost’s picture

Looks good to me!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice test coverage!

xjm’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup
FileSize
59.15 KB
25.02 KB

This looks great! Nice to see the test coverage as well.

  1. I was going to say that the language field should only be displayed on a multilingual site "like for nodes"... but then discovered that admin/content has a language filter even on a monolingual site in HEAD, with all the confusing/overwhelming options that nodes do. So I guess that's not in scope here, but I'd want to see a followup or two to (a) only display the language filter when content might exist in more than one language and (b) not show confusing, irrelevant options. Issues for these might already exist.

  2. The headers should be sentence case rather than title case. ("Author name" rather than "Author Name" and "Comment type" rather than "Comment Type". (Compare to admin/content.)

  3. I'm also not sure we should ship with "Comment type" as a default filter, since (unlike content types) multiple comment types are not the 80% usecase, and Standard itself only ships with one comment type.
  4. +++ b/core/modules/comment/comment.post_update.php
    @@ -36,3 +36,31 @@ function comment_post_update_enable_comment_admin_view() {
    +/**
    + * Update the comment admin view.
    + */
    +function comment_post_update_update_comment_admin_view() {
    +  $module_handler = \Drupal::moduleHandler();
    

    I don't think we should have this in the patch; the view has only been added in 8.4.x which has not shipped a release yet. Unless I've missed something?

  5. I noticed another maybe-out-of-scope thing, which is the unhelpful label "Update Options" on the bulk update field. (I think this is in the view in HEAD too.) admin/content has "Action" as the label, which is more informative.
xjm’s picture

Okay, out-of-scope point 1 is a whole can of worms. Relating some of the related issues.

I wonder if we could improve the situation by only displaying these exposed filters if more than one kind of the thing exists on the site? That would also fix point 3. That's magic and an extra query, so probably should be a followup, and we go with just doing the same meh thing that nodes do at the moment. So let's make that a followup issue (for both this view and admin/content maybe), and just go with my suggestion of removing the comment type filter for now.

jibran’s picture

Thanks for the review.

  1. I was concerned about it as well. Let's see what multi-lang experts are saying. I'll wait on them to create the follow-up meanwhile the issue can move ahead.
  2. Nice catch, fixed.
  3. Adding it later would be tricky so I think having this benefits not having this.
  4. It was removed in #15 after @dawehner's suggestion but thanks for clearing that out. We all agreed we don't need this so no you are not missing anything.
  5. Changed.
jibran’s picture

That's magic and an extra query, so probably should be a followup

Agreed, let's do it in the follow-up issue.

and just go with my suggestion of removing the comment type filter for now

I still think having the filter out weights not having it. This view is not shipped with the standard profile it is shipped with the comment module so I don't strongly buy into 80% use-case argument.

andypost’s picture

IMO comment type is useless
Right now it used just to point to which entity type the comment field is bound to.
But 80% case is nodes
If someone will decide to add comments to products or taxonomy able to edit default view and customize...
To have a documentation page with few sentences about that makes sense otoh better to add it to change record

xjm’s picture

Issue tags: +Needs usability review

I still think we should remove the comment type filter. (A content type filter from the node relationship would me more useful than a comment type filter, even. Edit: Not saying we should add that since it would make the view node-specific; just offering it as an example.) If someone wants to have multiple comment types on their site, it's very easy for them to add a comment type filter to the view (that's the point of it being a view, after all). :)

I don't think it's tricky to add later either if we decide it is worthwhile?

I guess a usability maintainer could weigh in on whether we should include that filter.

Bojhan’s picture

I agree with @xjm, it seems like we would be optimizing for a use case thats just not the 80%, and people who do that likely know how to adjust a view.

If possible we can do some special logic to hide it if it's only one?

jibran’s picture

> If possible we can do some special logic to hide it if it's only one?

I have some ideas around that but that's way out of the scope for this issue.

Bojhan’s picture

Then we should not show it, lets aim for the 80% usecase.

jibran’s picture

andypost’s picture

IMO good to go

Lendude’s picture

Awesome test coverage! Just some really tiny nits:

  1. +++ b/core/modules/comment/config/optional/views.view.comment.yml
    @@ -600,6 +665,128 @@ display:
    +            identifier: combine
    
    +++ b/core/modules/comment/config/optional/views.view.comment.yml
    @@ -733,6 +938,128 @@ display:
    +            identifier: combine
    

    Maybe use 'author_name' here? Much clearer in the URL when searching (would also need an update in the test then afterwards)

  2. +++ b/core/modules/comment/tests/src/Kernel/Views/CommentAdminViewTest.php
    @@ -0,0 +1,204 @@
    +    // Unpublish the comment to test the Unapproved comments tab.
    

    comment => comments

  3. +++ b/core/modules/comment/tests/src/Kernel/Views/CommentAdminViewTest.php
    @@ -0,0 +1,204 @@
    +    // Test the combine filter using  username.
    

    one space too much between using and username.

jibran’s picture

Thanks, @Lendude for the awesome review. Here is an updated patch fixes everything form #29.

Lendude’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
63.4 KB

Nice. Updated the screenshot to the most recent version of the patch.

xjm’s picture

  1. +++ b/core/modules/comment/config/optional/views.view.comment.yml
    @@ -641,10 +828,27 @@ display:
    +      relationships:
    +        uid:
    +          id: uid
    

    So this does add a relationship to the existing view, which we'd typically want to profile per https://www.drupal.org/core/gates#performance. However, I looked over #1986606: Convert the comments administration screen to a view and I don't see any documentation of profiling there either. :P I'll check with catch.

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

    Does this mean that we didn't give semantic display names to the tabs of the comment view? We should fix that if so. (Out of scope here but we'd want to backport that if possible before beta.)

  3. +++ b/core/modules/comment/tests/src/Kernel/Views/CommentAdminViewTest.php
    @@ -0,0 +1,204 @@
    +   * Runs tests comment admin view display.
    

    Nit: This is not really a sentence.

  4. +++ b/core/modules/comment/tests/src/Kernel/Views/CommentAdminViewTest.php
    @@ -0,0 +1,204 @@
    +    // Assert the expose filters on the admin page.
    

    Nit: exposed filters.

  5. +++ b/core/modules/comment/tests/src/Kernel/Views/CommentAdminViewTest.php
    @@ -0,0 +1,204 @@
    +    $this->assertEquals(1, count($elements), 'Only anonymous comment is visible.');
    +    $executable->destroy();
    ...
    +    $this->assertEquals(1, count($elements), 'Only admin comment is visible.');
    +    $executable->destroy();
    ...
    +    $this->assertEquals(1, count($elements), 'Only anonymous comment is visible.');
    ...
    +    $this->assertEquals(1, count($elements), 'Only admin comment is visible.');
    

    This isn't actually asserting that the anonymous comment is visible; it's asserting that one comment of some sort is visible. We should add assertText()s and assertNoText()s for the comment we expect to have displayed.

  6. +++ b/core/modules/comment/tests/src/Kernel/Views/CommentAdminViewTest.php
    @@ -0,0 +1,204 @@
    +    // Test the language filter.
    +    $executable->setExposedInput(['langcode' => '***LANGUAGE_site_default***']);
    +    $build = $executable->preview($display_id);
    +    $this->setRawContent($renderer->renderRoot($build));
    +    $this->verbose($this->getRawContent());
    +
    +    $elements = $this->cssSelect('input[type="checkbox"]');
    +    $this->assertEquals(2, count($elements), 'Both comments are visible.');
    +    $executable->destroy();
    

    Should we test with another language also?

xjm’s picture

Status: Reviewed & tested by the community » Needs work
xjm’s picture

Issue tags: +Needs followup

For #32.2.

xjm’s picture

Issue tags: +MWDS2017
mradcliffe’s picture

I will work on analyzing the query changes using EXPLAIN to see performance impact. First will need to generate many users and comments since the join is on the user table.

annajl’s picture

Fixed #32.3 and #32.4

Tim Plunkett said I don't need an interdiff. Leaving it at needs work because the other issues still need to be fixed.

jibran’s picture

Thanks, for the review.
Re #32:

  1. Yes, we do discuss before and after query when view is replacing an existing page and we forgot to do that. I'm happy to tackle that in follow-up.
  2. I don't like those name as well but while working on #1986606: Convert the comments administration screen to a view I observed that views.view.content page display is also called page_1 so I followed that. I agree we should discuss it in a follow-up and rename all admin views displays.
  3. Fixed.
  4. Fixed.
  5. Fixed.
jibran’s picture

Fixes minor doc issues.

annajl’s picture

mradcliffe’s picture

Status: Needs review » Needs work

I was having some development environment issues and wasn't able to get to finish performance testing today.

jibran’s picture

Status: Needs work » Needs review

So?

mradcliffe’s picture

Sorry. I didn't mean to change the status.

jibran’s picture

All good @mradcliffe. Now we have to conversations going one in #1986606-373: Convert the comments administration screen to a view and one here.
Edit: And I'm sorry about my abrupt reply I didn't intend it like that.

mradcliffe’s picture

The 4 possible queries running here are

  1. No filters used.
    • No index scan used on left join to field_users_data. Note that MySQL EXPLAIN in the above related issue also does this, and this is not necessarily a bad thing.
  2. Filter on author name using the combine plugin (concatenating two columns and doing a like).
    • No index scan used on left join to field_users_data.
  3. Filter on subject
    • Index scan (uid, default langcode, langcode) used on merge right join to field_users_data.
  4. Filter on author name and subject.
    • Index scan (uid, default langcode, langcode) used on merge right join to field_users_data.
  5. In PostgreSQL 9.6 the query planner did well for everything except #1 and #2 and used the index on the users_field_data table. However for some reason it did not choose the index scan when only the author name filter was done. Not using the index increased the "cost" of the query significantly. If I only filtered on the comment author name, then the index WAS used. It's possible the query planner made the correct decision because as the number of rows increase it gets more expensive to run the index. This probably happened on my VM with 4gb memory using 1 vcpu with 10000 users and 10000 nodes with max of 50 comments where the name I searched for returned about 1-50 rows (after limit). This was also done after auto vacuum as I got the data in there yesterday and have done subsequent restarts of the VM since then.

    MySQL should also have similar behavior with choosing indexes, and it looks like EXPLAIN in #1986606: Convert the comments administration screen to a view.

    I did not see any significant issues with the query, and I think views is doing a good job here.

    As @andypost noted in #1986606: Convert the comments administration screen to a view there may be render performance issues, but that's probably more to do with views in general rather than adding in filters? If anything filters would improve render performance by reducing the number of comment entities to load individually.

xjm’s picture

Thanks @mradcliffe, nice detailed summary.

larowlan’s picture

Thanks @mradcliffe, appreciate the profiling and analysis

catch’s picture

larowlan did a blackfire comparison and it looks like there's a small performance improvement in rendering (which has he pointed out, makes sense given Views has had a lot of render caching attention compared to comment admin controller by now). Thanks for the testing!

xjm’s picture

Status: Needs review » Reviewed & tested by the community

The above addresses all my feedback. RTBCing (which means I can't commit it), but if someone else can review/confirm @jibran's changes then I could commit it also.

larowlan’s picture

The last round of changes (last two interdiffs) look good to me.

  • xjm committed 620cc4c on 8.5.x
    Issue #2898344 by jibran, xjm, Lendude, dawehner, andypost, mradcliffe,...

  • xjm committed ca60214 on 8.4.x
    Issue #2898344 by jibran, xjm, Lendude, dawehner, andypost, mradcliffe,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.4.0 release notes

Alright, committed and pushed to 8.5.x and backported to 8.4.x since this view is new in 8.4.x anyway. Thanks everyone!

jibran’s picture

Thank you all. I don't think we are missing any follow-ups from #18 or #32.