From dbac1198913ec4c160f3f8a80f7073b6fd5069bd Mon Sep 17 00:00:00 2001
From: Roberto Tyley <roberto.tyley@gmail.com>
Date: Thu, 25 Feb 2016 19:03:34 +0000
Subject: [PATCH] Don't add a 'From:' header to the body if the email is a
 'noreply'

The old logic included a 'From:' in-body header if the author of the
commit was different to the person who was sending the email. However,
commits generated through GitHub's interface often (always?) have an
author email of `githubusername@users.noreply.github.com`, which is
definitely _not_ preferable.

PatchBomb emails by SubmitGit are always sent 'from' the user's *primary*
email address in GitHub.

See also:

http://article.gmane.org/gmane.comp.version-control.git/286879
https://github.com/rtyley/submitgit/issues/28
---
 app/lib/model/PatchBomb.scala      | 18 ++++++++++++-----
 test/lib/model/PatchBombSpec.scala | 31 +++++++++++++++++++++++-------
 2 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/app/lib/model/PatchBomb.scala b/app/lib/model/PatchBomb.scala
index cd51177..6c468b5 100644
--- a/app/lib/model/PatchBomb.scala
+++ b/app/lib/model/PatchBomb.scala
@@ -4,7 +4,7 @@ import java.text.NumberFormat
 
 import lib.Email
 import lib.Email.Addresses
-import lib.model.PatchBomb.countTextFor
+import lib.model.PatchBomb.{fromBodyPrefixOpt, countTextFor}
 
 case class PatchBomb(
   patchCommits: Seq[PatchCommit],
@@ -13,18 +13,17 @@ case class PatchBomb(
   additionalPrefix: Option[String] = None,
   footer: String
 ) {
+
   lazy val emails: Seq[Email] = {
     for ((patchCommit, index) <- patchCommits.zipWithIndex) yield {
       val patchDescriptionAndIndex = (Seq(shortDescription) ++ countTextFor(index + 1, patchCommits.size)).mkString(" ")
       val prefixes = (additionalPrefix.toSeq :+ patchDescriptionAndIndex).map("["+_+"]")
 
-      val authorEmailString = patchCommit.commit.author.userEmailString
-      val fromBodyPrefixOpt = Some(s"From: $authorEmailString\n").filterNot(_ => authorEmailString == addresses.from)
-
       Email(
         addresses,
         subject = (prefixes :+ patchCommit.commit.subject).mkString(" "),
-        bodyText = (fromBodyPrefixOpt.toSeq :+ s"${patchCommit.patch.body}\n--\n$footer").mkString("\n")
+        bodyText =
+          (fromBodyPrefixOpt(patchCommit, addresses).toSeq :+ s"${patchCommit.patch.body}\n--\n$footer").mkString("\n")
       )
     }
   }
@@ -32,6 +31,15 @@ case class PatchBomb(
 
 object PatchBomb {
 
+  def fromBodyPrefixOpt(patchCommit: PatchCommit, addresses: Addresses): Option[String] = {
+    val author = patchCommit.commit.author
+
+    val shouldAddFromBodyPrefix =
+      author.email == addresses.from.getAddress || author.email.contains("noreply")
+
+    if (shouldAddFromBodyPrefix) None else Some(s"From: ${author.userEmailString}\n")
+  }
+
   def numberFormatFor(size: Int) = {
     val nf = NumberFormat.getIntegerInstance
     nf.setMinimumIntegerDigits(size.toString.length)
diff --git a/test/lib/model/PatchBombSpec.scala b/test/lib/model/PatchBombSpec.scala
index 752d9bf..755ca47 100644
--- a/test/lib/model/PatchBombSpec.scala
+++ b/test/lib/model/PatchBombSpec.scala
@@ -7,25 +7,42 @@ import org.eclipse.jgit.lib.ObjectId
 import org.scalatestplus.play.PlaySpec
 
 class PatchBombSpec extends PlaySpec {
-  val patchCommit = {
-    val author = UserIdent("bob", "bob@x.com")
+
+  val bob = UserIdent("bob", "bob@x.com")
+  val fred = UserIdent("fred", "fred@y.com")
+
+  def patchCommitAuthoredBy(author: UserIdent) =
     PatchCommit(Patch(ObjectId.zeroId, "PATCHBODY"), Commit(ObjectId.zeroId, author, author, "COMMITMESSAGE"))
-  }
 
-  def patchBombFrom(text: String) = PatchBomb(
+
+  def patchBombFrom(user: UserIdent, patchCommit: PatchCommit) = PatchBomb(
     Seq(patchCommit),
-    Addresses(from = new InternetAddress(text)),
+    Addresses(from = new InternetAddress(user.userEmailString)),
     footer = "FOOTER"
   )
   
   "Patch bomb" should {
     "add a in-body 'From' header when commit author differs from email sender" in {
-      patchBombFrom("fred <fred@y.com>").emails.head.bodyText must startWith(s"From: bob <bob@x.com>\n\n${patchCommit.patch.body}")
+      val patchCommit = patchCommitAuthoredBy(bob)
+      val patchBomb = patchBombFrom(fred, patchCommit)
+
+      patchBomb.emails.head.bodyText must startWith(s"From: bob <bob@x.com>\n\n${patchCommit.patch.body}")
     }
 
     "not add an in-body 'From' header when the commit author matches the email sender" in {
-      patchBombFrom("bob <bob@x.com>").emails.head.bodyText must not contain "From:"
+      val patchBomb = patchBombFrom(bob, patchCommitAuthoredBy(bob))
+
+      patchBomb.emails.head.bodyText must not include "From:"
     }
 
+    "not add an in-body 'From' header when the commit author used a 'noreply' address" in {
+      // http://article.gmane.org/gmane.comp.version-control.git/286879
+      val mrNoReply = UserIdent("Peter Dave Hello", "peterdavehello@users.noreply.github.com")
+      val patchBomb = patchBombFrom(bob, patchCommitAuthoredBy(mrNoReply))
+
+      val text = patchBomb.emails.head.bodyText
+
+      text must not include "From:"
+    }
   }
 }