Page MenuHomePhabricator

Add an option to put comment context into emails.
ClosedPublic

Authored by klimek on Aug 5 2014, 1:00 PM.
Tags
None
Referenced Files
F12826624: D10146.id24747.diff
Thu, Mar 28, 9:33 AM
Unknown Object (File)
Feb 6 2024, 3:16 AM
Unknown Object (File)
Jan 6 2024, 4:38 PM
Unknown Object (File)
Jan 6 2024, 4:38 PM
Unknown Object (File)
Jan 6 2024, 4:37 PM
Unknown Object (File)
Dec 30 2023, 11:50 PM
Unknown Object (File)
Dec 27 2023, 5:50 AM
Unknown Object (File)
Dec 24 2023, 7:50 PM
Subscribers

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Commits
Restricted Diffusion Commit
rPeb3ed9bbc994: Add an option to put comment context into emails.
Summary

When enabled, this will show the full history of review comments in an
email-compatible threading-view.

Test Plan

Sending emails with the option on and off.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

klimek retitled this revision from to Add an option to put comment context into emails..
klimek updated this object.
klimek edited the test plan for this revision. (Show Details)
klimek added a reviewer: epriestley.
epriestley edited edge metadata.

This is basically fine, technically. Some inlines, but only one (draft selection) is an actual correctness issue.

I'm pretty hesitant to upstream this because I think your install is the only install using the existing option and I haven't seen more interest in this, but I've had to port the existing stuff forward once already (in connection with T2222) and am likely to need to generalize it for Audit (in connection with T4896), and make it interact correctly with planned changes to inline behavior (T1460). Making the feature more complicated is increasing the upstream cost for a feature that only your install uses.

In T1460, we also plan to introduce explicit "done" and "reply" states to inline comments, but have a different definition of "reply".

If you:

  • fix the correctness issue around drafts;
  • remove the configuration option (so this behavior is always enabled if you have inlines enabled);
  • are comfortable that we will change what "reply" means in the future when T1460 happens; and
  • do what you reasonably can about the query stuff (the hard query should get easier after T1460, so it's not hugely important that it get corrected)

...I guess we can upstream this. The config option is the biggest issue, but I think that's not contentious to remove.

src/applications/differential/config/PhabricatorDifferentialConfigOptions.php
297–311

Do we need this option? Do you think it's useful to enable one the other option but not this one?

At a minimum, this should mention that the option has no effect unless inlines are already enabled.

src/applications/differential/editor/DifferentialTransactionEditor.php
1307

Since this must be an array, prefer an array typehint.

1308

By convention, avoid foreach-by-ref, or explicitly unset the reference variable after exiting the loop.

This pattern (binding references and not unsetting them) tends to lead to subtle bugs where reuse of an iterator later in a function has surprising, nonlocal side effects.

1314

Prefer class typehint.

1316–1319

This incorrectly selects draft inlines (which have not yet been submitted, and should not be visible to other users) in addition to visible inlines.

Add a transactionPHID IS NOT NULL clause to avoid selecting them.

1316–1319

This suffers from the "N+1" query problem:

https://secure.phabricator.com/book/phabcontrib/article/n_plus_one/

Specifically, if there are 100 inlines, we'll issue 100 queries. Although this probably isn't a huge problem since it's rare to have 100 inlines, it would be better to issue a single query.

1325–1326

This is another N+1 query. Instead, authors should be loaded in a single query.

For objects where a real Query class is available (most objects, but not DifferentialTransactionComment), prefer it. The appropriate Query class in this case is PhabricatorPeopleQuery.

1328

Using getRealName() is unconventional: Phabricator identifies users with getUsername().

This revision now requires changes to proceed.Aug 12 2014, 12:39 PM
klimek edited edge metadata.

Address review comments.

Regarding your effort to maintain this, a few thoughts:

  1. Thank you so much for doing it; no matter whether you take this in the end, I really appreciate your feedback and time; I understand the trade-off between features and maintenance headaches, so I fully appreciate if we have to keep our own fork.
  2. That said, keeping our own fork often feels very very frustrating; I know that's not your problem, but I think that 99.9% of the time I've spent countless hours in syncing and resolving merge conflicts would have cost no time if the person writing the feature (-> you ;) would have changed the code with the refactoring (I love the refactorings, just not the merging ...); that would save so much of my time that you dropping me an email whenever there is a problem with this code so I can resolve this would be immediately prioritized for me, based on that I would lose so much if I have to keep our own fork
  3. <<</me opens the bag and pulls out a carrot; the carrot looks somewhat translucent, so you can't really be sure whether it exists>>>: we would very much like not having to maintain our own instance, and we'd like to become paying customers (the countless hours I throw at maintaining our own GCE instance with this stuff are beyond reasonable); not sure whether you still aim at providing such a service though...
  4. I think the feature might not be used very much until now because it's not feature complete yet; I think the comments in the diff actually make the whole email diff thingy much more useful, and if we add parsing in-line replies and adding them to comment hunks as replies, that would make it even more useful (it would mean I could just do reviews completely by email if I have enough context); but of course I would say that, so probably not a very convincing argument on its own :P
src/applications/differential/config/PhabricatorDifferentialConfigOptions.php
297–311

Done.

src/applications/differential/editor/DifferentialTransactionEditor.php
1307

Done.

1308

Yea, that doesn't make any sense. /me shudders. Sorry for having left that in ...

1314

Done.

1316–1319

Draft bug: Yikes. Done.

1316–1319

N+1. Done, yea, that was bad. /me's first real LAMP project, so thanks for teaching :)

1325–1326

And done.

1328

Hm, I'd like to honor the user-address-format setting, but I don't think people care too much (hopefully). I'll go with getUserName and we'll see whether people scream at me.

epriestley edited edge metadata.

Cool, this is in pretty good shape. One query construction safety thing and a couple of minor things.

src/applications/differential/editor/DifferentialTransactionEditor.php
1331

Slightly better as:

pht('%s wrote:', $user->getUserName())

...so it can be translated.

1369–1371

This should use qsprintf() to prevent possible SQL injection:

$table = new DifferentialTransactionComment();
$conn_r = $table->establishConnection('r');

$queries = array();
foreach ($stuff as $thing) {
  $queries[] = qsprintf(
    $conn_r,
    '(changesetID = %d AND (lineNumber IN (%Ld))',
    $id,
    $line_numbers);
}

Then, below, use:

$table->loadAllWhere(
  'transactionPHID IS NOT NULL AND (%Q)',
  implode(' OR ', $queries));
1381

Slightly simpler as mpull($all_comments, 'getAuthorPHID').

This revision now requires changes to proceed.Aug 14 2014, 4:30 PM
klimek edited edge metadata.
  • Address review comments
  • Merge to head
src/applications/differential/editor/DifferentialTransactionEditor.php
1331

Done.

1369–1371

Done, thx again for the teaching effort :)
Btw, I assume 'implode' is just more php'y than 'join'?

1381

So that would assume that 'withPHIDs' doesn't depend on the keys. I assume in php it's basically "unless otherwise stated, if you take a list of things you shouldn't depend on the keys in the array?". This seems a little fragile at a first glance, but probably just takes some getting used to... So, done :)

epriestley edited edge metadata.

Cool. Thanks!

This revision is now accepted and ready to land.Aug 15 2014, 5:13 PM
epriestley updated this revision to Diff 24747.

Closed by commit rPeb3ed9bbc994 (authored by @klimek, committed by @epriestley).

On join/implode, they're totally equivalent, we just try to pick one and stick with it in this codebase. We should write a lint rule but it doesn't come up too often.