Page MenuHomePhabricator

Allow users to remove their own comments, and administrators to remove any comment
ClosedPublic

Authored by epriestley on May 2 2014, 3:04 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 26, 4:42 PM
Unknown Object (File)
Fri, Dec 13, 9:41 AM
Unknown Object (File)
Thu, Dec 12, 5:09 PM
Unknown Object (File)
Tue, Dec 10, 12:55 PM
Unknown Object (File)
Sun, Dec 8, 5:49 PM
Unknown Object (File)
Wed, Dec 4, 7:43 PM
Unknown Object (File)
Sat, Nov 30, 12:09 PM
Unknown Object (File)
Nov 28 2024, 12:55 AM
Tokens
"Love" token, awarded by qgil.

Details

Summary

Fixes T4909. Adds a "remove" link next to the edit link, which permanently hides a comment. Addresses two use cases:

  • Allowing administrators to clean up spam.
  • Allowing users to try to put the genie back in the bottle if they post passwords or sensitive links, etc.

The user who removed the comment is named in the removal text to enforce some level of administrative accountability.

No data is deleted, but there's currently no method to restore these comments. We'll see if we need one.

This is cheating a little bit by storing "removed" as "2" in the isDeleted field. This doesn't seem tooooo bad for now.

Test Plan
  • Removed some of my comments.
  • As an administrator, removed other users' comments.
  • Failed to view history of a removed comment.
  • Failed to edit a removed comment.
  • Failed to remove a removed comment.
  • Verified feed doesn't show the old comment after comment removal.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

epriestley retitled this revision from to Allow users to remove their own comments, and administrators to remove any comment.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.

Particularly, in addition to the WMF use case there was a user on Twitter recently who accidentally posted something sensitive or embarrassing and was frustrated they could not remove it. This can't fully put the genie back in the bottle (emails will already have gone out, for example) but can limit the damage in some cases.

I support this conclusion overall, thanks for thinking through it.

Yeah, I think this does a reasonably good job of balancing all the concerns here.

I think the biggest downside is that the upper-right corner is starting to get a bit cluttered. We could put "Edit" and "Remove" behind a single "Removedit" link, but the dialog would be a bit odd (e.g., have three buttons?) and it would be less obvious that you could remove the comment. Separate links doesn't feel too bad to me, at least on web: for non-admins, they only show up on your own comments, so it's not like there's a wall of text or anything.

Oh, yeah I would just have three buttons, or two with a "delete" checkbox floated left.

Also, the button says "Edit Comment" as the action, which sounds weird after you've edited. Maybe Save Changes or Save Comment?

qgil added a subscriber: qgil.
  • Provide a better submit button for edits.
  • Provide some explanatory text for removal.

The multi-button thing is a bit involved because administrators (who can not edit) and authors (who can) need to have different options.

Do you have thoughts on how to do the quote/reply button in T4119, from a pure "what does the button look like" point of view? Maybe that could help inform this -- my naive approach would be to put another link up here.

Maybe a little row of remarkup looking buttons (pencil, x, quote)

Let me take a quick shot at that and see how it looks...

Normal case:

Screen_Shot_2014-05-02_at_7.02.00_AM.png (398×980 px, 64 KB)

Worst case:

Screen_Shot_2014-05-02_at_7.02.51_AM.png (393×986 px, 64 KB)

We could also make the content source an icon (small phone, mail, etc.) with the caveat that it used to be and users flipped out. However, that was ages ago on a different design.

A single "actions" gear/dropdown would be better future proofed and usable on mobile.

comments.png (94×785 px, 21 KB)

(Better match style of PropertyList Actions List)

comments2.png (94×785 px, 20 KB)

I dig the gear version, though the vertical line in v1 just before it made me clean my monitor then squint. The v2 version is better. Does it look terrible with no vertical line at all?

I like that a lot better visually, and like that we don't have to do anything special for mobile, and that we can label the actions. Some possible issues:

  1. The right edge will be ragged since not all transactions have actions (or, many transactions will have an empty/disabled menu?).
  2. There will no longer be any top-level visual indicator that a comment has been edited.
  3. Our dropdowns aren't accessible and I don't currently have the expertise to fix the ARIA stuff, or a copy of JAWS to test it.
  4. "Quote", which is primarily a convenience, will end up buried in a way that's probably less convenient.

Not sure how bad these are. For (1), I'd guess that a ragged edge will look bad but disabling the menu would make it OK again. Once we have Quote, this menu will almost always be available (only disabled for logged out users or some kind of future locked object we might build, I guess?).

For (2), we could maybe use a different menu to indicate an edited comment? Feels kind of overloadey. I don't know that this is really all that important, but it feels bad to bury "edited" even more heavily than we already are. We could also put a separate "edited" icon in grey, since these are very rare.

For (3), I don't think this is a blocker and dropdowns need to get fixed in a general way eventually.

For (4), it might hurt the feature a lot. We could have two buttons (quote, all other stuff), but then (1) gets bad again.

I think we can still list "edited" copy. Quote will be the only action for most comments and it's just there for convenience.

Okay, how about we do this:

  • Land this as-is, if there are no technical issues.
  • I'll build quote as a followup and we can land that as a clutter-link for now too.
  • Then I'll fix this into a dropdown and move all the clutter into it at once.
btrahan edited edge metadata.

Plan sounds good. Step 1 unlocked as the code looks good.

Regarding the "Edited" comment thing, check out T4944 which is suggesting putting it for description. (I am opposed and put some prose as to why but I encourage more opinions or closing it out if you all agree. =D )

src/applications/transactions/controller/PhabricatorApplicationTransactionCommentRemoveController.php
69–71

Feed, notifications clear up too?
Email is sent though. :/

(actually just checking, not sure if doc needs updates)

src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php
63

what's up with 2 (as opposed to 1) ?

This revision is now accepted and ready to land.May 2 2014, 3:59 PM

On T4944, I agree with your comment in T4944#4.

I'm not, like, wildly opposed to rendering "edited" textually as a footer in comments, but that seems pretty noisy/loud. I think the loudness of the marker is about right right now: it's there, but it's not too in your face.

src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php
63

The states are:

0 = normal
1 = deleted (user has deleted all the text, but history is still visible)
2 = removed (user has removed the comment, history is not visible)

The reason for "1" is mostly that we don't normally allow empty comments, and no-op them in transactions. The explicit "1" says "this is an empty comment on purpose", and various things check for it and react appropriately.

A cleaner approach would be to add a separate isRemoved column, but we'd have to migrate every comment table to support it and I don't want to put installs through a ton of migrations over this. e.g., Facebook would need to migrate many millions of rows.

No need for migrations. Explanation is good enough for me and if you want to get fancy about it to help the next guy class constants should do the trick.

epriestley updated this revision to Diff 21315.

Closed by commit rP58f66fea804e (authored by @epriestley).