Here is a patch that Does a couple of things;

  1. Adds a new view display to the Administration: Comments view for the admin/content/comment/approval. This is the same as the original comments view except;
  2. Removed exposed filter for comment status
  3. The original view is now filtered by Comment: Approved - yes, and the new view is filtered by Comment: Approved - no
CommentFileSizeAuthor
#12 admin_views.comment-approval.12.patch10.16 KBsun
#8 admin_views.comment-approval.8.patch13.29 KBsun
#5 1616324-5.patch7.39 KBdamiankloip
admin-views-comment-approval-display.patch4.81 KBdamiankloip

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Hm. I'm not sure whether this makes sense.

With admin_views, the separate "Approval" page makes little sense in general. Even less, if you take into account that the entire "approval" state for comments is nothing else than published and unpublished in reality. Unpublishing a comment unexpectedly means to "disapprove" it, and makes it re-appear in the list of comments to approve (moderate). That's completely nuts and I've no idea why core chose to use a terminology that implies (moderation) business logic that doesn't exist.

I definitely want to retain the exposed filter for the publishing status on the main comments view. (should be consistent with the node and user views.)

That's not to say we cannot also replace the approval page with a view. Despite being generally needless, it might be unexpected for some to not see a view there.

damiankloip’s picture

damiankloip commented

Yep, sure. That is more the point I was getting at; The comment/approval page should also be a view. So on the main comments page we can keep the exposed filter for state, and then have a view that only shows unpublished comments? I agree, core should either not have this, or have something a little more extensible for Approvals, rather than just a published/unpublished state.

damiankloip’s picture

damiankloip commented

So essentially we keep the current admin view for comments but just add another display for the comments/approval page?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

The funny tidbit is that in an ideal world, we'd just make that second tab execute the page handler for the first tab, and merely append the query string ?status=0 before doing so... but sadly, that would be a hack.

If you think there should be a separate view on the approval page, then sure, we can do that.

For simplicity of maintenance, I'd perhaps leave the exposed filter for status on it? I'm a bit concerned about proper maintenance + updates for the (overridden) filters and exposed filters in the future. (It's a bit sad that Views immediately overrides the entire stack of filters, even if only a single filter override is desired.)

damiankloip’s picture

damiankloip commented
StatusFileSize
new 1616324-5.patch7.39 KB

Yes, it would be nice to be able to set the default filter without having to hack a query string to the path.

Here is a new patch, both views are the same except for the default filter values.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

ok, I didn't actually test, but this looks good to go.

Speaking of tests, do we want to add some simple automated, so we don't have to check manually that these two pages are working and are showing what they are supposed to show? :)

damiankloip’s picture

damiankloip commented

That sounds sensible :) Shall we do this in this issue as one patch? or another? Also, I'm not sure of the best way to test this. Any pointers would be appreciated.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new admin_views.comment-approval.8.patch13.29 KB

I'd say something simple like this?

Didn't do Node. But left some @todos on exposed filter names...

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work

oops. of course, that locale default admin view file shouldn't have been in there ;)

damiankloip’s picture

damiankloip commented

ha, sure. I will remove that file and commit to 7.x, as we are happy with the view and we have some passing tests for it. I can then add some more tests for node in a follow up, as it probably feels a bit left out now :)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Can we also fix those exposed filter names quickly before commit? (since we're touching the filter definitions anyway?)

Hm. But hold on... why are the filters duplicated for the first system display?

Also, do we really have to default the published status to 1 on the main comments view?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new admin_views.comment-approval.12.patch10.16 KB

So how about this? :)

damiankloip’s picture

damiankloip commented
Status: Needs review » Needs work

@sun, do you mean change the names of the filter ids? And yes, I will change the main comments view default to all.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review

eh? Did you see #12 ? ;)

damiankloip’s picture

damiankloip commented

Damn you cross posting! :)

damiankloip’s picture

damiankloip commented

@sun, unless you have any objections, I think this is ok to be committed now?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Yup, let's go ahead with this :)

damiankloip’s picture

damiankloip commented
Status: Reviewed & tested by the community » Fixed

ok, committed to 7.x.

Status: Fixed » Closed (fixed)

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