Gerrit notification email contains "null"
Closed, ResolvedPublicBUG REPORT
Actions

Assigned To

Authored By

	Umherirrender
	Aug 5 2021, 11:27 PM

Description

What happens?:
I have a email from gerrit with unexpected "null" between the "comment count" and the "Patchset" text:
[<!-- this line looks unexpected added for visibility]

Attention is currently required from: Umherirrender.
DannyS712 has posted comments on this change by Umherirrender. ( https://gerrit.wikimedia.org/r/c/mediawiki/core/+/710084 )

Change subject: build: Suppress ObjectTypeHintReturn for upcoming sniff change
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

null             <!-- this line looks unexpected
Patchset:

PS2:
Fix typo in commit message ("newly" -> "new")

There is no null in the gerrit UI - https://gerrit.wikimedia.org/r/c/mediawiki/core/+/710084/2#message-f79c7956faef859fa07144f50dd1188158f558d1

What should have happened instead?:
No "null" in the notification.

Software version (if not a Wikimedia wiki), browser information, screenshots, other information, etc:
Gerrit 3.3.5

Details

	Subject	Repo	Branch	Lines +/-
	gerrit: prevent 'null' entry in email	operations/puppet	production	+2 -5

Customize query in gerrit

Related Objects

Mentioned Here: T43608: List change author in gerrit email templates
rOPUP84c651bfc6e8: Gerrit: Reveal the author in the title of the email

Event Timeline

Umherirrender created this task.Aug 5 2021, 11:27 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 5 2021, 11:27 PM

Umherirrender unsubscribed.Nov 25 2021, 6:30 PM

The message payload can be retrieved from the change metadata git fetch origin refs/changes/84/710084/meta

commit f79c7956faef859fa07144f50dd1188158f558d1
Author: Gerrit User 6729 <6729@e9e9afe9-4712-486d-8885-f54b72dd1951>
Date:   Thu Aug 5 23:02:11 2021 +0000

    Update patch set 2
    
    Patch Set 2: Code-Review+2
    
    (1 comment)
    
    Patch-set: 2
    Reviewer: Gerrit User 6729 <6729@e9e9afe9-4712-486d-8885-f54b72dd1951>
    Label: Code-Review=+2
    Attention: {"person_ident":"Gerrit User 161 \u003c161@e9e9afe9-4712-486d-8885-f54b72dd1951\u003e","operation":"ADD","reason":"DannyS712 replied on the change"}

diff --git a/baf4d01efa04359553f0b150a1a7b63b80b4bff1 b/baf4d01efa04359553f0b150a1a7b63b80b4bff1
new file mode 100644
index 00000000000..d1b6df84c9b
--- /dev/null
+++ b/baf4d01efa04359553f0b150a1a7b63b80b4bff1
@@ -0,0 +1,21 @@
+{
+  "comments": [
+    {
+      "unresolved": false,
+      "key": {
+        "uuid": "aa2802a5_5f0e0b54",
+        "filename": "/PATCHSET_LEVEL",
+        "patchSetId": 2
+      },
+      "lineNbr": 0,
+      "author": {
+        "id": 6729
+      },
+      "writtenOn": "2021-08-05T23:02:11Z",
+      "side": 1,
+      "message": "Fix typo in commit message (\"newly\" -\u003e \"new\")",
+      "revId": "baf4d01efa04359553f0b150a1a7b63b80b4bff1",
+      "serverId": "e9e9afe9-4712-486d-8885-f54b72dd1951"
+    }
+  ]
+}
\ No newline at end of file

I have looked at a recent email which comes with both plain text and HTML. The plain text payload indeed shows the null text but the HTML ones does not.

HTML

Plain text

The email comes from templates and I found out that we forked the upstream ones for some reason, the sources are in operations/puppet:

$ git ls-files modules/gerrit/ |grep soy
modules/gerrit/files/homedir/review_site/etc/mail/ChangeSubject.soy
modules/gerrit/files/homedir/review_site/etc/mail/Comment.soy
modules/gerrit/templates/its/PatchSetAbandoned.soy.erb
modules/gerrit/templates/its/PatchSetCreated.soy.erb
modules/gerrit/templates/its/PatchSetMerged.soy.erb
modules/gerrit/templates/its/PatchSetRestored.soy.erb

https://gerrit.wikimedia.org/g/operations/puppet/+/refs/heads/production/modules/gerrit/files/homedir/review_site/etc/mail/ChangeSubject.soy
https://gerrit.wikimedia.org/g/operations/puppet/+/refs/heads/production/modules/gerrit/files/homedir/review_site/etc/mail/Comment.soy

Our Comment.soy has:

{for $group in $commentFiles}
  // Insert a space before the newline so that Gmail does not mistakenly link
  // the following line with the file link. See issue 9201.
  {$group.link}{sp}{\n}
  {$group.title}:{\n}

Which look related to https://bugs.chromium.org/p/gerrit/issues/detail?id=12997 and is an aftermath of the Gerrit 3.3 upgrade. There are two changes upstream to the Comment.soy file:

https://gerrit-review.googlesource.com/c/gerrit/+/274475 "Fix patchset level comment link"
https://gerrit-review.googlesource.com/c/gerrit/+/279252 "Update url in emails to use new comment route"

The group.link variable is removed and I think that is the one resulting in a showing up a null.

Looks like the template got introduced in 2017 by 84c651bfc6e8db67933e6322f37efa082946d259 for T43608. I think I will drop them in favor of using the upstream built in templates.

Change 768005 had a related patch set uploaded (by Hashar; author: Hashar):

[operations/puppet@production] gerrit: prevent 'null' entry in email

https://gerrit.wikimedia.org/r/768005

gerritbot added a project: Patch-For-Review.Mar 4 2022, 8:17 AM

Change 768005 merged by Alexandros Kosiaris:

[operations/puppet@production] gerrit: prevent 'null' entry in email

https://gerrit.wikimedia.org/r/768005

Mentioned in SAL (#wikimedia-releng) [2022-03-09T17:08:08Z] <hashar> Updating Gerrit Comment.soy to get rid of a literal null string being inserted in notification emails | https://gerrit.wikimedia.org/r/c/operations/puppet/+/768005 | https://phabricator.wikimedia.org/T288312

Maintenance_bot removed a project: Patch-For-Review.Mar 9 2022, 5:10 PM

This should be solved now ™

	F34974720: gerrit_html.png
	Mar 4 2022, 6:51 AM

	F34974722: gerrit_text.png
	Mar 4 2022, 6:51 AM

Gerrit notification email contains "null"Closed, ResolvedPublicBUG REPORTActions