boonebgorges / buddypress-docs

GNU General Public License v3.0
106 stars 44 forks source link

Enable anyone to comment when permissions allow #637

Closed christianwach closed 6 years ago

christianwach commented 6 years ago

Unless I am misunderstanding something, at present it does not seem possible for logged-out users to comment on a Doc regardless of its access settings. This PR moves the bp_docs_post_comments capability check out of the "Special Cases" section since the ability for logged-out users to comment on a Doc is presumably desired behaviour.

christianwach commented 6 years ago

From the failed checks, it looks to me as though the tests are wrong too - shouldn't assertFalse() actually be assertTrue() in test_user_can_post_comments_anyone? @boonebgorges If so, I'll add that to the PR.

boonebgorges commented 6 years ago

What you've put in the PR is probably what we originally intended the plugin to do, but now that it does something else - ie, limits comment-posting to logged-in users - I wonder whether it's something we can safely change. I think it's probably OK, but I would like someone else to spend a second thinking about it :) @dcavins Do you have thoughts about this?

dcavins commented 6 years ago

I agree that at the moment there's an inconsistency where the permissions make it look as though non-logged-in users can post comments (by allowing the "anyone" permission for "who can post comments"). So I'd agree that we need to make a decision and match up what is shown/selected with the resulting behavior.

However, Boone specifically changed this setting in a commit: https://github.com/boonebgorges/buddypress-docs/commit/5ebb57521ae12e6f7e2f464ddb44cc959e133638

I'm guessing that this was because comments are a hard-to-control spam vector for most site owners. Because everything is complicated, I'd say that we need to make our permissions code respect what is selected on the /wp-admin/options-discussion.php screen, which offers this option: "Users must be registered and logged in to comment"

and not include the "anyone" permission for comments when that option is deselected. If we do that (so it's easy for site owners to disable this ability) then I think the Christian's change make a lot of sense.

Here are settings and permissions changes that build on Christian's change set: https://github.com/dcavins/buddypress-docs/commit/6906af1dd52fb7014d6f9bb2a41af0d2930465d8

I think this is a good idea, though. :)

christianwach commented 6 years ago

@dcavins Thank you for elaborating on this. I now see that there has already been an unresolved PR about this previously #607 and that you've already made the same commit as I just have with 8ca7528d2928d2f1c2741c4f2a878e6a40d47509. I agree that 6906af1dd52fb7014d6f9bb2a41af0d2930465d8 is also necessary and should be part of any eventual merge.

So, if this issue is dependent on an "editorial decision" on the desirability of public comments, who's going to make that call? :-) Would adding a new option to page=bp-docs-settings be a way to avoid making that decision?

dcavins commented 6 years ago

Please see https://github.com/dcavins/buddypress-docs/commits/comments-over-2.2 for a few commits that address the question here and expands to deal with some of the related problems in #607 (Thanks, Christian, for reminding me of that open PR.)

In my limited testing, things worked as expected, but I would appreciate more testing and a sanity check of the new code, including the decisions I made about what should be the right behavior. (For instance, I didn't overrule the spam approval status, even for logged-in users, so comments that Akismet or other services identify as spam would have to be manually approved.)

I also removed a reference to a deprecated BP function, bp_blogs_record_comment(), so Boone, please think about whether that makes sense to you.

Re "the desirability of public comments", my opinion is that if we give the site admin easily accessible control via the site-wide "Users must be registered and logged in to comment" setting, then the site admin can do what's best for the site, and we don't have to make a blanket determination. Spam is a serious problem, but if the site admin is disabling anonymous comments to prevent it, with the new code, BP Docs will follow suit, so I'm satisfied.

Thanks!

christianwach commented 6 years ago

That's awesome @dcavins thanks for taking on the broader task! I've been using your branch for a while on my dev site and have not come across any problems - everything seems to work as expected. The caveat, I suppose, is that I have not gone through every combination of options and behaviours to verify all possible scenarios.

christianwach commented 6 years ago

One thing I'm a little confused about is what happens when a comment needs approval and none of the people with access to the Doc have the moderate_comments capability. This would presumably mean that someone with that capability who may not even know of the existence of the Doc would have to approve the comment. Could that be a problem?

One way round that scenario (if it is indeed a problem) might be to assign the moderate_comments capability to the Doc author and filter the comments on wp-admin/edit-comments.php to show only those associated with Docs that the user is an author of. Not entirely sure how that could be done reliably whilst preserving the full list for those with a role-based moderate_comments capability.

Another option might be to filter out comments on Docs from wp-admin/edit-comments.php and add a new doc-tab (alongside "Read", "Edit" and "History") that allows comment moderation by the Doc author and/or group admins/mods.

Dang it, am I overthinking this? Hmm, I can see why enabling this functionality stalled!

christianwach commented 6 years ago

Heh, I think I may have answered my own question: if the held-for-moderation comments are from logged-out users then by definition the Doc must be public - so the discoverability of the Doc is not an issue. I do think the issue of who is responsible for comment approvals still remains: it would be a nice touch if the Doc author could be notified and moderate incoming comments, but I'd say it's not a blocker.

dcavins commented 6 years ago

Hi Christian-

When comments are held for moderation, the standard WP workflow kicks in:

It would be nice if the doc author could approve comments, but it's way down the list of "nice-to-haves" because I'm imagining it would take a lot of work to hijack the standard WP process (or build a custom interface as you've suggested) for the very few times that it would happen.

Thanks again for testing out the new code! Please let us know if you run into any kinks.

christianwach commented 6 years ago

When comments are held for moderation, the standard WP workflow kicks in:

I got myself in a muddle thinking about Docs attached to hidden groups and what would happen if the person responsible for comment moderation wasn't an admin but an editor - moderate_comments is also a cap for the Editor role - and IIRC editors don't have access to hidden groups and the Docs associated with them. The Doc would, of course, have to be public for that to happen, so it's a non-issue.

It would be nice if the doc author could approve comments, but it's way down the list of "nice-to-haves"

Agreed :-) I should probably think things through more thoroughly before posting my comments.

Thanks again for your work on this.

boonebgorges commented 6 years ago

The changes in the branch from @dcavins look well thought-out to me. Thanks for working on them, and thanks to @christianwach for helping to talk them through!

christianwach commented 6 years ago

Shall I close this in favour of #607?

dcavins commented 6 years ago

Hi Christian-

Can you add the test updates mentioned above to this PR? Then I can start by merging this PR then add the other changes on top of it.

So, no, please don't close this one.

Thanks!

-David

christianwach commented 6 years ago

@dcavins Done.

dcavins commented 6 years ago

Thanks, @christianwach!