Page MenuHomePhabricator
Create Task
Maniphest T13620

Improve access to commit messages from email and timeline contexts
Open, LowPublic
Actions

Description

See PHI2007. An install suggests a specific workflow related to reverting commits could be improved by surfacing more of the commit message earlier in the pipeline. In particular, if revision A is reverted by commit B, users associated with A who aren't familiar with B need to click through two pages (from the email notification to the revision page, then from the revision page to the commit page) to read more than the first few words of the commit message.

This is likely somewhat niche, but various aspects of this are reasonable to improve:

  • Generic edge stories currently don't support hovercards, but should. Other types of similar stories, like subscriber stories, do support hovercards. I'm fairly sure this is just a legacy inconsistency, not an explicit design decision.
  • Commit hovercards could reasonably show more of the message. They currently show only the "summary", but there's enough room to show significantly more content:

Screen Shot 2021-02-25 at 10.16.43 AM.png (240×617 px, 40 KB)

  • Inlining more details about the object into the email may be reasonable.

Revisions and Commits

rP Phabricator
D21574rPe9804bb7e5f1 Provide hovercards for generic edge stories, and include more message…

Related Objects

Event Timeline

epriestley triaged this task as Low priority.Feb 25 2021, 6:18 PM
epriestley created this task.
epriestley added a revision: D21574: Provide hovercards for generic edge stories, and include more message information in commit hovercards.Feb 25 2021, 6:28 PM
epriestley added a commit: rPe9804bb7e5f1: Provide hovercards for generic edge stories, and include more message….Feb 25 2021, 6:30 PM
epriestley added a comment.Feb 25 2021, 7:01 PM

D21574 implements the hovercard and summary length changes.

The email change is a bit more complex. This story is currently a generic edge story, rendered through PhabricatorApplicationTransaction, with limited ability to emit custom email content.

I think the way I'd like to render these is to inline blocks in the email timeline. For example:

alice added a reverting change: rXYZabcd Lorem ipsum dolor sit amet, consectetur adipiscing elit...

     COMMIT DETAILS: rXYZabcd
     Lorem ipsum dolor sit amet, consectetur adipiscing elit. Vivamus posuere a
     nisl id mattis. Integer non risus quis enim posuere hendrerit. Donec
     vestibulum mauris nec tristique eleifend. Vestibulum ut justo mi. Lorem
     ipsum dolor sit amet...
     <https://install.com/rXYZabcd>

alice added a subscriber: bailey.
alice added a comment.

In production, we ran into an issue with the deployed version of some library...

The major alternative is likely to send everything to the bottom in the body section. This is somewhat easier today. This would probably be okay, but if this information is genuinely useful it seems most useful in context.

In practice, today, these specific events are never normally emitted with other types of transactions, so the decision is largely moot: the email will look about the same in practice regardless of the behavior it is designed for when it has a lot of other simultaneous transactions.

The entire API for emitting email content is kind of a mess (PhabricatorModularTransactionType->getBodyForMailWithRenderingTarget() is kind of terrible, but it's the "good" API!) and the feed/pre-Modular transaction cruft doesn't help.

epriestley added a comment.Feb 25 2021, 7:11 PM
  • T12921 is adjacent to mail rendering.
  • T13439 mentioned the hovercard summary text.
epriestley added a comment.Feb 25 2021, 7:32 PM

This is a brute force approach to support "context blocks" without taking any steps backwards (i.e., it supports older transactions and newer, modular transactions), but it feels like this is a lot of steps sideways and this change would be far better if it took a nuanced approach through T12921.

diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index 437a7a1437..7964349871 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -3535,6 +3535,16 @@ abstract class PhabricatorApplicationTransactionEditor
         if ($header_html !== null) {
           $headers_html[] = $header_html;
         }
+
+        $context_text = $this->getContextBlocksForTextMail($xaction);
+        if ($context_text) {
+          $headers[] = $context_text;
+        }
+
+        $context_html = $this->getContextBlocksForHTMLMail($xaction);
+        if ($context_html) {
+          $headers_html[] = $context_html;
+        }
       }
 
       if ($xaction->hasChangeDetailsForMail()) {
@@ -5292,6 +5302,39 @@ abstract class PhabricatorApplicationTransactionEditor
     return $xaction->getTitleForHTMLMail();
   }
 
+  private function getContextBlocksForTextMail(
+    PhabricatorApplicationTransaction $xaction) {
+    $type = $xaction->getTransactionType();
+
+    $xtype = $this->getModularTransactionType($type);
+    if ($xtype) {
+      $xtype = clone $xtype;
+      $xtype->setStorage($xaction);
+      $comment = $xtype->getContextBlocksForTextMail();
+      if ($comment !== false) {
+        return $comment;
+      }
+    }
+
+    return $xaction->getContextBlocksForTextMail();
+  }
+
+  private function getContextBlocksForHTMLMail(
+    PhabricatorApplicationTransaction $xaction) {
+    $type = $xaction->getTransactionType();
+
+    $xtype = $this->getModularTransactionType($type);
+    if ($xtype) {
+      $xtype = clone $xtype;
+      $xtype->setStorage($xaction);
+      $comment = $xtype->getContextBlocksForHTMLMail();
+      if ($comment !== false) {
+        return $comment;
+      }
+    }
+
+    return $xaction->getContextBlocksForHTMLMail();
+  }
 
   private function getBodyForTextMail(
     PhabricatorApplicationTransaction $xaction) {
diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
index 43878a0bec..347f705334 100644
--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@@ -1730,6 +1730,22 @@ abstract class PhabricatorApplicationTransaction
       ->addInt(-$this->getActionStrength());
   }
 
+  final public function getContextBlocksForTextMail() {
+    return $this->newContextBlocksForTextMail();
+  }
+
+  protected function newContextBlocksForTextMail() {
+    return array();
+  }
+
+  final public function getContextBlocksForHTMLMail() {
+    return $this->newContextBlocksForHTMLMail();
+  }
+
+  protected function newContextBlocksForHTMLMail() {
+    return array();
+  }
+
 
 /* -(  PhabricatorPolicyInterface Implementation  )-------------------------- */
 
diff --git a/src/applications/transactions/storage/PhabricatorModularTransactionType.php b/src/applications/transactions/storage/PhabricatorModularTransactionType.php
index 7d5e3c533e..4821eae5b1 100644
--- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php
+++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php
@@ -435,51 +435,67 @@ abstract class PhabricatorModularTransactionType
   // actually return Remarkup, not text or HTML.
 
   final public function getTitleForTextMail() {
-    return $this->getTitleForMailWithRenderingTarget(
+    return $this->getContentForMailWithRenderingTarget(
+      'title',
       PhabricatorApplicationTransaction::TARGET_TEXT);
   }
 
   final public function getTitleForHTMLMail() {
-    return $this->getTitleForMailWithRenderingTarget(
+    return $this->getContentForMailWithRenderingTarget(
+      'title',
       PhabricatorApplicationTransaction::TARGET_TEXT);
   }
 
   final public function getBodyForTextMail() {
-    return $this->getBodyForMailWithRenderingTarget(
+    return $this->getContentForMailWithRenderingTarget(
+      'body',
       PhabricatorApplicationTransaction::TARGET_TEXT);
   }
 
   final public function getBodyForHTMLMail() {
-    return $this->getBodyForMailWithRenderingTarget(
+    return $this->getContentForMailWithRenderingTarget(
+      'body',
       PhabricatorApplicationTransaction::TARGET_TEXT);
   }
 
-  private function getTitleForMailWithRenderingTarget($target) {
-    $storage = $this->getStorage();
-
-    $old_target = $storage->getRenderingTarget();
-    try {
-      $storage->setRenderingTarget($target);
-      $result = $this->getTitleForMail();
-    } catch (Exception $ex) {
-      $storage->setRenderingTarget($old_target);
-      throw $ex;
-    }
-    $storage->setRenderingTarget($old_target);
+  final public function getContextBlocksForTextMail() {
+    return $this->getContentForMailWithRenderingTarget(
+      'context',
+      PhabricatorApplicationTransaction::TARGET_TEXT);
+  }
 
-    return $result;
+  final public function getContextBlocksForHTMLMail() {
+    return $this->getContentForMailWithRenderingTarget(
+      'context',
+      PhabricatorApplicationTransaction::TARGET_TEXT);
   }
 
-  private function getBodyForMailWithRenderingTarget($target) {
+  private function getContentForMailWithRenderingTarget($type, $target) {
     $storage = $this->getStorage();
 
     $old_target = $storage->getRenderingTarget();
     try {
       $storage->setRenderingTarget($target);
-      $result = $this->getBodyForMail();
+      switch ($type) {
+        case 'title':
+          $result = $this->getTitleForMail();
+          break;
+        case 'body':
+          $result = $this->getBodyForMail();
+          break;
+        case 'context':
+          $result = $this->getContextBlocksForMail();
+          break;
+        default:
+          throw new Exception(
+            pht('Unknown content type "%s".', $type));
+      }
     } catch (Exception $ex) {
       $storage->setRenderingTarget($old_target);
       throw $ex;
+    } catch (Throwable $ex) {
+      $storage->setRenderingTarget($old_target);
+      throw $ex;
     }
     $storage->setRenderingTarget($old_target);
 
@@ -494,4 +510,8 @@ abstract class PhabricatorModularTransactionType
     return false;
   }
 
+  protected function getContextBlocksForMail() {
+    return array();
+  }
+
 }
epriestley mentioned this in 2021 Week 9 (Very Late February).Feb 26 2021, 11:09 PM
Phabricator · Built by Phacility