Page MenuHomePhabricator

Make the !accept email command configurable such that it only works if the diff was inlined
Closed, ResolvedPublic

Description

We recently enabled inlined diffs in emails, we haven't yet turned on !accept. Our reasoning here is that we've only allowed inlined diffs to show up for diffs shorter than 250 lines, and that it's not reasonable to allow people to respond with !accept if the diff wasn't inlined. It would be nice if we could be kinda-certain that someone probably read the diff before accepting it, rather than blanket enabling the email command.

Event Timeline

You can subclass MetaMTAEmailTransactionCommand, following DifferentialActionEmailCommand (which does Differential stuff) and ManiphestPriorityEmailCommand (which is a better example of implementing an individual command), to implement !hahaha-yes-i-definitely-reviewed-this-hahaha which applies !accept only for small diffs.

I think the desired behavior is too niche and fuzzy for inclusion in the upstream. In particular: counting changed line is not an exact science; an earlier diff may have been inlined but the diff may be over the size threshold by the time a user's !hahaha is processed; someone might eventually complain that generated changes shouldn't be counted; etc. It's possible users looked at the diff if attach-patches is enabled, even if inline-patches is not, but opinions on the "right" behavior here could differ since there is probably (???) no context (maybe VPN stuff?) where you can easily look at an attachment but can not easily look at the web UI. This is also a fairly gigantic pain to test.

Mail command extensions should be flexible enough to support this already. If they aren't, I'd prefer to add whatever features might be missing.

You'd need to duplicate the "count the diff size" code from DifferentialTransactionEditor but implementation should otherwise be relatively straightforward, I believe.

$patch = id(new DifferentialRawDiffRenderer())
  ->setViewer($this->getActor())
  ->setFormat('git')
  ->setChangesets($diff->getChangesets())
  ->buildPatch();
$lines = substr_count($patch, "\n");
yelirekim claimed this task.

Thanks!

Roughly, you should be able to:

  • Subclass the thing.
  • Drop it in somewhere.
  • Reload /applications/mailcommands/PhabricatorDifferentialApplication/revision/ until your new command shows up and looks right.
  • Start piping raw mail into cat mail.txt | ./scripts/mail/mail_handler.php --process-duplicates until the command actually does what you want it to. You can usually set this up by sending a real mail, doing "View Entire Raw Mail" on the sent message in your client, saving that as "mail.txt", and then messing with the body. Also maybe useful is cat body.txt | ./bin/mail receive-test --as user --to D123 (actually, maybe that's better here since you don't really care about the headers in this case).