Convert admin/content/comment to a View

[klonos]

We didn't have an issue for that from #151. Here it is ;)

---

PR by @klonos (single admin page for both published/unapproved comments): backdrop/backdrop#2341 (consensus to try combine into single admin page in a new follow-up issue)
Alternative PR by @herbdool (separate pages for published/unapproved comments): backdrop/backdrop#2313

[klonos]

...I don't know about the milestone here, but I'm guessing not 1.0.0 :p

[klonos]

Just as a note here so that we don't repeat any mistakes from the conversion of /admin/content/ to a view (#548), we need to make sure that:

• the page has a proper title like "Manage comments"
• The page title is reflected in the admin menu
• The available operations (currently titled "Update options") in the drop-down menu are either listed in alphabetical order or in groups that make sense.

also, I really don't like the way that the comments are split into "published" and "unapproved" via tabs. I propose having a single list for all comments and making the published/unapproved a filter instead.

[klonos]

I think I'll take a stab at this...

[jenlampton]

@klonos creating the view should be easy. We may need to create actions for the bulk-operations checkbox to match what we have now. I just did that for the files view so LMK if you need help with that part.

(we are 15 days from feature freeze - but this issue is tagged as task so may be able to get in after Jan 1)

[klonos]

Thanx @jenlampton. Health is not favoring me lately (even missed my daughter's birthday party) and I have a huge backlog of issues I need to catch up with. I'll try to get this done to go along with the rest of the issues about our admin views. I'll take a look at how you've done the bulk operations for the manage files view. If I can't figure it out, I'll surely turn to you for help. Thanx 😉 👍

[klonos]

Compared to the non-view listing, the manage comments view additionally gets us a "delete" and an "approve" operation in the dropbutton. Question is this though: in the non-view admin page, we had two separate local task tabs for published and unapproved comments. Do we want that recreated with the view listing, or do we prefer having the published/unapproved state of comments as a filter exposed to users. I think the second. Reason being that with the non-view mode splitting the comments to two lists does not allow a specific action (delete for example) to be applied to both published and unpublished comments. If these are in a single list, we can offer that option to the users and still allow them to filter by published/ unapproved (through a filter action instead of selecting between two local tasks tabs).

[klonos]

The bulk operations for publish/unpublish comments work just fine, but the "approve" link in the dropbutton throws an "access denied". Need to add a hook_permission()...

[klonos]

Alright, I've just put a PR up for review/feedback. Far from complete, but here are the things that need to be done or that I need feedback before proceeding:

1. The previous, non-view "Comments" page has not been removed yet. I left that in for now so that reviewers can compare the two, but will remove it once I had enough feedback on the rest of the items.

2. Previous non-view page was under /admin/content/comment and also had a secondary tab (local task) for unapproved comments under /admin/content/comment/approve. The new, views-based "Manage comments" page accommodates for both pages in one, as it provides an "Approved" exposed filter which can be used to filter things accordingly. The view is under /admin/content/comments (plural for comments) which I think is more appropriate (since with Add an interface to view and manage files #2375 we are also going to have plural for files under /admin/content/files). Perhaps a URL redirect from the old /admin/content/comment path to the new /admin/content/comments is in order too(?).

   Decision to make here is whether this (a single page with exposed filter instead of two separate local tasks tabs) is a functionality regression or an improvement. We could ditch the exposed filter and have two separate displays of the same view instead; one that shows published comments and another that shows unapproved ones. Then we can have these displays be two local tasks, replacing the two non-views pages. Do we want that? I thought that alternatively we could have the single view and pass the comment status as a contextual filter instead. That could provide us a menu item under /admin/content/comment/unapproved (with the last bit being the contextual filter), but I could not find a contextual filter available with the comment status. Worth the trouble adding a contextual filter for comment (and perhaps node) published status in core just for that? If not, then /admin/content/comments?status=1 and /admin/content/comments?status=0 work equally well.

3. Similarly as above, we only have a single menu item in the admin bar under Content -> Manage comments. We previously had two separate menu items for managing comments in the admin bar: one under Content -> Comments -> Published comments and a separate under Content -> Comments -> Unapproved comments. The second menu item also featured a count of unpublished comments that the views-provided menu item does not have. Again, acceptable change or regression?

   Makes me wonder why we are special-casing comments only(?). I mean, why aren't there also two sub menu items for published/unpublished content under Content -> Find content?

4. The respective search field for content has a "Title contains" label, but with comments we do not expose a title field (the "Allow comment title" setting is turned off by default when creating new content types and also for the post content type that we ship with - besides, comments get a trimmed version of the body text as their title). So, I thought that the best way to go about it here is to expose a combined search field with a generic "Comment contains" label. It searches both the comment body and the comment title. Acceptable?

5. I have added an exposed search field for searching by a specific author (use case: admin wants to mass-publish/unpublish comments by a specific user in case of spam investigation), but there are certain issues with it: ...I don't think that it makes sense to be fuzzy searching author names because for instance it might return comments by multiple users and that in turn might end up in sticky situations especially with the use case I have in mind, like accidentally unpublishing or deleting comments from multiple users when the intention was to do so from a specific one. So the exposed field operator is set to "is equal to" in order to limit results to entries by a single user each time. That though demands a precise username to be entered, which is not great UX unless Views: Autocomplete exposed filters #2423 happens. So, bottom line is: keep or ditch?

6. There is no "delete" bulk operation yet.

7. The "edit" and "delete" dropbutton operations seem to be working fine, but the "approve" link (only available for unapproved comments) is not. There is an "Access denied - You are not authorized to access this page." message. If I do not exclude the "Comment: Approve link" field from display and use that one instead, it works fine. The issue occurs when the link is being rendered as a dropbutton action (using the special "Global: Dropbutton" field). Core bug??

8. Need to add an "unpublish" dropbutton operation for published comments.

That's all I can think of right now. Could use opinions/pointers/thoughts on the above before proceeding.

[klonos]

...

9. Investigate if the generic "Publish comment was applied to 1 item." message can be customized for the view in order to say something more like "1 comment has been published.". Not a biggie, since the message in the non-view page is equally bad or worse (more generic): "The update has been performed."

[klonos]

Slightly updated the PR so that the default view config includes "module": "comment". This makes it so that the view is marked as module-provided and thus can only be reverted to default (disallows deleting it). Also added a description to it. Still need decisions on the points above so to know how to proceed.

[Dinornis]

Here is my initial quick look over this. Will do some more testing with more comments later:

RE 2.

Decision to make here is whether this (a single page with exposed filter instead of two separate local tasks tabs) is a functionality regression or an improvement. We could ditch the exposed filter and have two separate displays of the same view instead; one that shows published comments and another that shows unapproved ones. Then we can have these displays be two local tasks, replacing the two non-views pages. Do we want that? I thought that alternatively we could have the single view and pass the comment status as a contextual filter instead. That could provide us a menu item under /admin/content/comment/unapproved (with the last bit being the contextual filter), but I could not find a contextual filter available with the comment status. Worth the trouble adding a contextual filter for comment (and perhaps node) published status in core just for that? If not, then /admin/content/comments?status=1 and /admin/content/comments?status=0 work equally well.

I think a single page with exposed filter would be better, as long as the menu includes a link labeled Unapproved Comments loading the filter with status=0. I would see this as an improvement because as an admin I would have quick access to unapproved comments, yet in addition have the options to search for particular comment(s) on the same page.

RE 3.

Similarly as above, we only have a single menu item in the admin bar under Content -> Manage comments. We previously had two separate menu items for managing comments in the admin bar: one under Content -> Comments -> Published comments and a separate under Content -> Comments -> Unapproved comments.

The two separate menu items is a better option because having only single menu item labeled Manage Comment linked to the single views-generated page defaulting to status=0 may confuse some users if they landed on the page with no comments showing.

The second menu item also featured a count of unpublished comments that the views-provided menu item does not have. Again, acceptable change or regression?

Having the count in the menu is quite useful and I would consider this as regression if it can't be retained.

RE 4.

I thought that the best way to go about it here is to expose a combined search field with a generic "Comment contains" label. It searches both the comment body and the comment title. Acceptable?

To me this seems acceptable.

RE 5.

I don't think that it makes sense to be fuzzy searching author names because for instance it might return comments by multiple users and that in turn might end up in sticky situations especially with the use case I have in mind, like accidentally unpublishing or deleting comments from multiple users when the intention was to do so from a specific one. So the exposed field operator is set to "is equal to" in order to limit results to entries by a single user each time.

Yes, set to "is equal to" will the way to go.

That though demands a precise username to be entered, which is not great UX unless #2423 happens. So, bottom line is: keep or ditch?

Definitely bad UX without the autocomplete. I think this is a regression from Views in D6. I am pretty sure the autocomplete is part of Views for D6 without extra module. Odd call to have this removed from core.

[jenlampton]

#2 The view is under /admin/content/comments (plural for comments) which I think is more appropriate. Perhaps a URL redirect from the old /admin/content/comment path to the new /admin/content/comments is in order too(?).

Not a redirect, no, but a menu router item that does a backdrop_goto_deprecated(). See https://github.com/backdrop/backdrop/pull/1673/files for examples of changing system paths mid-cycle.

#2 Decision to make here is whether this (a single page with exposed filter instead of two separate local tasks tabs) is a functionality regression or an improvement.

It's in improvement IMO.

#3 The second menu item also featured a count of unpublished comments that the views-provided menu item does not have. Again, acceptable change or regression?

I don't like the idea of losing it mid-cycle. There could be people who are using it, and taking it away would make them grumpy. Maybe it would be acceptable in backdrop 2. Maybe we can add a hook_menu_alter to change the title of the menu link?

#3 Makes me wonder why we are special-casing comments only(?). I mean, why aren't there also two sub menu items for published/unpublished content under Content -> Find content?

Because comments are something that need review frequently. Content is almost always published by default, so moderation is an edge case. The special case here matches the use-cases.

#4 So, I thought that the best way to go about it here is to expose a combined search field with a generic "Comment contains" label. It searches both the comment body and the comment title. Acceptable?

In Backdrop the 'comment title' is actually generated from the 'comment body' by default, so a single "Contains" field on the body here is sufficient. If people choose to use a separate title field (or other fields!) they can adjust the view to their needs. Let's keep it simple :)

#5 So, bottom line is: keep or ditch?

Let's keep it. :)

#7 If I do not exclude the "Comment: Approve link" field from display, it works fine. The issue occurs when the link is being rendered as a dropbutton action (using the special "Global: Dropbutton" field). Core bug??

That is very strange, indeed! Perhaps a core bug.

[quicksketch]

With the release tomorrow, I'm moving this to the next point version.

[jenlampton]

This issue is now unblocked, since #2433 went in. Does anyone want to take a stab at building the recent comments list as a view?

[klonos]

Thanks @herbdool good job 👍 ...I have tested the PR sandbox and have not found anything to not be working as expected. Only a couple of minor nits to pick. Also, would be better if we used human language:

Unpublish comment was applied to 1 item.

vs:

Successfully unpublished 1 comment.

(same for the "publish" action). Also:

Are you sure you want to delete these comments and all their children?

vs:

...and all the replies to them?

I have made a pass through the code as well, but my level of experience is usually enough to catch small things, so would need a second pass by a pair of more experienced eyes.

PS: I am still ambivalent about having 2 separate tabs for something that could easily be implemented as an exposed filter, but if there's consensus on that, then OK.

[klonos]

...although this is a task (which means that it could go in with a bug release), I think that the change is big enough to qualify for a minor release. I have added the milestone candidate label accordingly, but feel free to change to the bug milestone if you think it's OK.

[jenlampton]

Are you sure you want to delete these comments and all their children?

How about...

Are you sure you want to delete these comments and all their replies?

edit: I also agree with the minor-release milestone. Once we get this to RTBC we can add it :) (as per our new rules about managing issues in milestones:
https://backdropcms.org/user-guide/adding-milestones-issues

[klonos]

...yep, that works too 🙂 👍

[herbdool]

Thanks @jenlampton @klonos. I've updated with your suggestions.

[klonos]

Thanks @herbdool 👍 ...the confirmation and success messages for deleting comments have been updated, but I still get:

Unpublish comment was applied to 50 items.

and:

Publish comment was applied to 43 items.

...whereas instead I'd expect:

Unpublished 50 comments.

and:

Published 43 comments.

I have also looked a bit more into the details of each view, and I have the following suggestions:

1. The URL parameters when searching for published/unapproved comments, are as bellow:

   /admin/content/comment/new?combine=something&name=someone
/admin/content/comment/approval?combine=something&name=someone

   I would expect new to be published, and combine to be contains. name is OK, but author would be better I think.

2. Change the machine names of the 2 page displays to published_comments and unpublished_comments instead of the current page and page_1.

[herbdool]

This is like Zeno's paradox of Achilles and the tortoise! Everytime I think it's done the goalpost has moved a little bit. 😋

"Publish comment was applied to 43 items": this might be outside the scope of this issue since it's language coming from vbo.

Regarding URLs I'll have to see. We might want to preserve the existing URL but parameters can be changed.

[jenlampton]

Yes on preserving url. Also yes on improving params for views filter names.

…

On Sun, May 26, 2019, 8:00 PM Herb ***@***.***> wrote: This is like Zeno's paradox of Achilles and the tortoise! Everytime I think it's done the goalpost has moved a little bit. 😋 "Publish comment was applied to 43 items": this might be outside the scope of this issue since it's language coming from vbo. Regarding URLs I'll have to see. We might want to preserve the existing URL but parameters can be changed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#549?email_source=notifications&email_token=AADBER2A5BCHOBZAXKWGHNTPXMXEJA5CNFSM4AZ3ZSR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWIRESI#issuecomment-496046665>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADBER2JBNEYCW4WD5D3KBTPXMXEJANCNFSM4AZ3ZSRQ> .

[klonos]

We might want to preserve the existing URL...

Why? I have checked, and the previous non-view functionality does not provide any way to search through any of the published/unapproved comments lists. There is nothing in /admin/content/comment/new currently (just redirects to /admin/content/comment); and I did not recommend changing /admin/content/comment/approval (which exists in the non-view version of the "Unapproved comments" admin page). Is there anything I'm missing?

[klonos]

"Publish comment was applied to 43 items": this might be outside the scope of this issue since it's language coming from vbo.

Sure 👍 ...can be a follow-up.

[jenlampton]

The path part of the URL and the query string part are different. You are referring to the query string part. I think k we all agree about that, it's the path part that needs to he preserved.

…

On Sun, May 26, 2019, 9:01 PM Gregory Netsas ***@***.***> wrote: We might want to preserve the existing URL... Why? I have checked, and the previous non-view functionality does not provide any way to search through any of the published/unapproved comments lists. There is nothing in /admin/content/comment/new currently (just redirects to /admin/content/comment); and I did not recommend changing /admin/content/comment/approval. Is there anything I'm missing? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#549?email_source=notifications&email_token=AADBER7LQ4VBRTBBFAXZSPTPXM6G5A5CNFSM4AZ3ZSR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWIS25A#issuecomment-496053620>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADBER6JIYRAVCZBU6FODG3PXM6G5ANCNFSM4AZ3ZSRQ> .

[herbdool]

Ready for review.

[herbdool]

@quicksketch @docwilmot or @serundeputy are you able to review this? It's been ready for a few months.

[ghost]

@herbdool The PR needs rebasing to fix some conflicts.

[ghost]

I fixed the conflicts which also pulled in the latest 1.x changes. So should be ready for testing/review again.

[argiepiano]

Many test failures for PHP 8.1