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): https://github.com/backdrop/backdrop/pull/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): https://github.com/backdrop/backdrop/pull/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 #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 #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 https://github.com/backdrop/backdrop-issues/issues/2433 went in. Does anyone want to take a stab at building the recent comments list as a view?

[klonos]

Well, the issue is still assigned to me 😄 ...unless anyone else beets me to it.

[quicksketch]

There is still a working PR here, though we need to do a bit more to provide an upgrade path for existing sites.

[jenlampton]

Upgrade path, I think I might be able to tackle this one :) Okay to steal it from you @klonos ?

[quicksketch]

Things that need to be done for this issue:

• Upgrade path needs to create the view on existing sites.
• For compatibility, the view should make two menu paths (Published comments and Unapproved comments) matching the current menu structure.
• The old menu paths need to be removed.

[quicksketch]

With the release tomorrow, this is getting bumped to 1.8.0.

[jenlampton]

Since there was no activity on this since the last release, removing milestone.

[herbdool]

Need to fix https://github.com/backdrop/backdrop-issues/issues/3306 in order to get this fully working.

[klonos]

... Okay to steal it from you @klonos ?

@jenlampton sorry I never responded, but you should have taken my silence as a sign ...and my "unless anyone else beets me to it." comment as a "go for it!" 😄

@herbdool glad to see you have picked this up 👍, but please see previous comments re ditching the separate local tasks tabs in favor of a single page with a "approved/unapproved" exposed filter:

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.

...

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.

[herbdool]

@klonos, @quicksketch says above:

For compatibility, the view should make two menu paths (Published comments and Unapproved comments) matching the current menu structure.

So I made it two tabs.

[herbdool]

@klonos or am I misreading it? Keep the two menu tabs but get rid of the two tabs?

[klonos]

@herbdool the way I understand it, the consensus was to keep the menu paths/items in the admin toolbar (under Content -> Manage comments), so that they are still available as quick jump links, but remove the separate local tabs and merge this whole thing into a single view. Switching between approved/unapproved would still be possible via a "Status" exposed filter, so there would actually be no feature/functionality regression.

[herbdool]

Okay, I've got something to report after attempting to make it all one view based on your comments, @klonos . In brief, it's going to be difficult given the backwards-compatibility constraints. Difficult enough that I'm not sure it's worth me putting more effort into it. Unless anyone has some insights into the technical problems.

Technical problems:

1. So for backwards compatibility, we need the menu items in the admin toolbar. However, hook_menu does not deal with query parameters on a URL. It'll only work with the actual path. That is, "admin/content/comments/approval" works, but "admin/content/comments?status=0" does not work. There's no obvious way to get around that and I can't recall anyone else trying something like this.
2. There is no easy way to show the total unapproved comments number. The title callback on hook_menu doesn't seem to work in combo with Views. Nothing I've tried has helped. In my current PR I had decided to add the results summary to the header so the admin can see the total unapproved on the Approval tab. With no tab, we can't show a total of just unapproved. It'll always be the total based on the filters. Maybe that's not a bad thing but it means the language has to be more generic since it won't be necessarily the entire number of unapproved.

Non-technical problems:

1. I'm not sure what advantage the exposed filter has over the two tabs. The two tabs take only one click to switch between views. Whereas the exposed filter takes three clicks: first click to select the dropdown, then select the option in the dropdown, then select "Execute". With the exposed filter just being two options: Approved / Unapproved, having two tabs looks like a nice improvement rather than a problem.
2. With just one view not all the operations make sense. It doesn't make sense to have an "Approve" link for already published comments. With them all in one view there's no way to selectively show the Approve link.

-- In sum, if we can keep the PR as-is, with tests passing, I can help with ensuring it gets approved with minor tweaks if needed. But if the consensus is that people want to combine two tabs into one, then the technical challenges are big enough that I don't really feel like spending more time on this. I'm not optimistic that it would get done soon since so far this issue has been open for three years.

Does anyone else want to take this one if the latter is true?

[herbdool]

To clarify, even if the current PR is accepted doesn't mean that we can't work towards making it a single View in a new issue and figure out the technical issues that go along with that.

[jenlampton]

My vote is that we move forward with the PR as-is, and worry about combining the views later if people still feel strongly about it.

On Tue, Oct 16, 2018, 11:31 AM Herb notifications@github.com wrote:

To clarify, even if the current PR is accepted doesn't mean that we can't work towards making it a single View in a new issue and figure out the technical issues that go along with that.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/backdrop/backdrop-issues/issues/549#issuecomment-430346791, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYSRwm2-DBmZvLK9V46pEuhZIQgRZFbks5uliYIgaJpZM4DO8yj .

[klonos]

1. I'm not sure what advantage the exposed filter has over the two tabs. The two tabs take only one click to switch between views. ...

The main one is the inability to search across both approved/unapproved comments by content/author, and then being able to apply bulk operations to comments that are in either state. For example search by author, and then delete all comments made by them (both approved and unapproved). With split views/tabs, one has to search and perform the bulk operation in the approved list, then repeat the same steps in the unapproved list.

2. With just one view not all the operations make sense. It doesn't make sense to have an "Approve" link for already published comments. With them all in one view there's no way to selectively show the Approve link.

Fair enough, but the existing functionality does not include any "Approve" link (although the addition of such an operation would be a very welcomed UX+), so there would be no feature regression AFAICT. In fact, the way this works currently with content is that we have "pairs" of actions in the respective bulk operations dropdown:

Anyway, we could try achieving that feature by duplicating the approved/unapproved column, and then rewriting the results as links (which could then be themed to look like buttons). Just a thought; I have not actually tested this.

To clarify, even if the current PR is accepted doesn't mean that we can't work towards making it a single View in a new issue and figure out the technical issues that go along with that.

👍 Follow-up ticket for merging the views sounds fine if we want to get this in ASAP. But the milestone is set to 1.12, which is due for January.

[herbdool]

@klonos

1. Fair point. That would be an improvement to search across both, and hopefully at some future point can be incorporated.
2. I see. I had assumed it was there before, but I guess you had added the Approved link (in the global dropdown) in your original PR. (I have a separate PR to fix the bug with the Approved link, so depending on that we may need to remove it from this one if it doesn't get in.)
3. Yes, the milestone is January but it'll depend on anyone willing to help. I expect a flurry of activity in the last few days of the milestone.

[laryn]

I tested @herbdool's PR and it works as expected for me. The only thing that stood out to me is actually the same as it is in the current version: the language seems to shift between "unapproved" and "unpublished" comments. It might be more consistent to keep it all the same and the simplest solution would be having the tab labeled as "Unpublished comments" if people feel that's still clear enough.

[herbdool]

Thanks @laryn for testing. I'll take a look.

I'm hoping to get this PR tested first https://github.com/backdrop/backdrop-issues/issues/3306 since this one is blocked on it. Do you have time to test it too?

[laryn]

@herbdool I just tested that one and it looks good. It also made me realize the "Approved" language is mixed in to more places than just this -- the View includes an "Approve link".

[herbdool]

@laryn That language is intentionally set to match the current Manage comments screens so we can focus this issue on just transforming it to a View first. Though I agree the language could be more consistent. Is that okay with you and willing to RTBC this?

[laryn]

That makes sense @herbdool. I reviewed again and noticed what @klonos mentioned earlier:

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?

[herbdool]

@laryn I hadn't figured out how to replicate it. In this thread I explained why using a VIew made it hard to replicate (here https://github.com/backdrop/backdrop-issues/issues/549#issuecomment-430346175). Instead I added text just below tabs to display the number of unapproved comments.

I'm hoping that this is acceptable, or if not, that someone else takes the torch. I've reached a dead end on figuring that last 5%.

[laryn]

I'm okay with that but wasn't sure if I could RTBC since there were a few other comments above your explanation indicating they didn't want to lose the summary in the menu.

[jenlampton]

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?

To me this is an acceptable change. I feel the utility we get by making this a view outweighs the loss of the comment count.

[jenlampton]

Since this issue is blocked by https://github.com/backdrop/backdrop-issues/issues/3306#issuecomment-487333755, bumping to next minor release.

[jenlampton]

Removing the 1.14 milestone until this issue gets unblocked.

[herbdool]

@jenlampton I updated the PR so it's not blocked by the other issue. Just have to fix a test.

[herbdool]

Tests pass. Ready for a code review.

[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 notifications@github.com 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 https://github.com/backdrop/backdrop-issues/issues/549?email_source=notifications&email_token=AADBER2A5BCHOBZAXKWGHNTPXMXEJA5CNFSM4AZ3ZSR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWIRESI#issuecomment-496046665, or mute the thread https://github.com/notifications/unsubscribe-auth/AADBER2JBNEYCW4WD5D3KBTPXMXEJANCNFSM4AZ3ZSRQ .