Page MenuHomePhabricator

Make Differential email subject more configurable.
AbandonedPublic

Authored by talshiri on May 31 2014, 2:41 AM.

Details

Summary

We wanted a bit more information in our differential email subjects, so we've made it parameterized.
Moving forward, it may make sense to merge subject-prefix and subject-vary into this, but I was hesitant of introducing that large
of a change before discussing this further.

Test Plan

tested locally

Diff Detail

Repository
rP Phabricator
Branch
configureable_differential_mail_subject
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 2189
Build 2193: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

talshiri updated this revision to Diff 22251.May 31 2014, 2:41 AM
talshiri retitled this revision from to Make Differential email subject more configurable..
talshiri updated this object.
talshiri edited the test plan for this revision. (Show Details)
talshiri added a reviewer: epriestley.
epriestley edited edge metadata.Jun 2 2014, 11:49 PM

Offhand, here are some issues with this:

  • Repository may be empty (e.g., copy/paste an arbitrary diff).
  • Branch may be empty (e.g., arc diff --raw) or meaningless (SVN, detached HEAD).
  • Branch is the branch the diff is on, not the upstream branch it was branched from, so in normal/suggested usage it's meaningless (e.g., the branch name will be something like headers, which is redundant with the subject).
  • This will break threading in some clients by altering "Thread-Topic" during the thread.
  • This will break threading in some clients by altering the subject during the thread.
  • This will break threading in some clients by altering sections of the subject not contained inside square brackets during the thread, under some configurations.
  • This does not respect the "Vary Subjects" mail configuration setting, and will thus break some clients which would otherwise work properly.

Overall, I'm really hesitant about this. We also do "multiplexing" in the Editor right now (which is really "demultiplexing") and I'd like to move that to the daemon queue, but this makes it more difficult.

We also haven't had other requests for this.

I think your best bet might be to patch this locally. I think you only need to adjust $subject ('Thread-Topic' is not normally visible to users):

$branch = nonempty($object->getActiveDiff()->getBranch(), '-');
$repository = $object->getRepository() ? $object->getRepository()->getName() : '-';
$subject = "[{$repository}/{$branch}] {$subject}";

Not ideal, but probably not really a whole ton of work to keep in sync with the upstream.

src/applications/differential/config/PhabricatorDifferentialConfigOptions.php
255

This uses {$...}, but the actual parser uses ${...}.

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

Particularly, this may fatal.

talshiri updated this revision to Diff 22273.Jun 3 2014, 12:47 AM
talshiri edited edge metadata.

Don't modify Topic
Don't die if no repo/branch are defined

talshiri updated this revision to Diff 22274.Jun 3 2014, 1:32 AM

Moved things to getMailSubjectPrefix, which is slightly more useful and doesn't add an extra variable.

talshiri updated this revision to Diff 22275.Jun 3 2014, 1:49 AM

minor oops

talshiri updated this revision to Diff 22425.Jun 5 2014, 10:10 PM
  • if repo name doesn't exist, use the basename of the source path
epriestley requested changes to this revision.Nov 23 2015, 3:39 PM
epriestley edited edge metadata.

Just pushing this out of my queue, see T5244. I have no plans to bring these changes upstream.

This revision now requires changes to proceed.Nov 23 2015, 3:39 PM
talshiri abandoned this revision.Nov 23 2015, 6:31 PM
kaya added a subscriber: kaya.May 5 2016, 4:59 PM
kaya awarded a token.May 18 2016, 6:09 PM