fix: Set 'sender.login' instead 'pullRequest.user.login'

[bbaarriiss]

Fix PR-closed-template.
Use sender.login where pullRequest.user.login is used.

[claude]

Review — fix: Set 'sender.login' instead 'pullRequest.user.login'

The fix is correct and I'd merge it. A few optional notes below, mostly about the file you're already touching.

Correctness ✅

On GitHub's pull_request.closed webhook, pull_request.user is the PR author while sender is the actor who performed the action. So "Pull Request Merged/Closed by X" was previously naming the wrong person whenever someone other than the author closed or merged the PR. Swapping to sender is the right semantics.

Safety-wise this is fine too: sender is a non-nullable field in GitHubResponse (src/main/kotlin/com/wire/github/response/model/GitHubResponse.kt:14), so there's no risk of the template silently rendering an empty name — a payload without sender already fails deserialization today. It also makes this template consistent with push.template, issues.closed.template, and issue_comment.created.template, which all use {{sender.login}}.

pull_request.opened.template keeping pullRequest.user.login is fine — for opened, sender and author are the same person.

Suggestions (non-blocking)

1. Style nit: inner spaces in the mustache tags

🚀 **Pull Request Merged!** by **{{ sender.login }}**

Every other tag in the repo (including lines 8–10 of this same file) is written without inner padding: {{sender.login}}. Mustache trims either way, so this is purely cosmetic, but dropping the spaces keeps the templates uniform.

2. merged_by would be more precise for the merged branch

For a plain "click merge" the sender is the merger, so this works. But with auto-merge or a merge queue, sender can be the user who enabled auto-merge (or the queue actor) rather than whoever actually merged; GitHub exposes pull_request.merged_by for exactly this. If you want it airtight, add @SerialName("merged_by") val mergedBy: User? = null to PullRequest and use a fallback section:

{{#pullRequest.merged}}
🚀 **Pull Request Merged!** by **{{#pullRequest.mergedBy}}{{login}}{{/pullRequest.mergedBy}}{{^pullRequest.mergedBy}}{{sender.login}}{{/pullRequest.mergedBy}}**
{{/pullRequest.merged}}

Worth it only if auto-merge is actually used in your repos — otherwise sender is a fine approximation and simpler.

3. The author's name is now gone from the merge notification

That's the intended trade-off of this PR, but for merge notifications readers often want both names. If you agree, by **{{sender.login}}** (authored by **{{pullRequest.user.login}}**) gives you the actor without losing attribution. Your call — the current one-name form is also perfectly readable.

4. Pre-existing in this file: {{pullRequest.title}} is HTML-escaped

DefaultMustacheFactory HTML-escapes {{ }} by default, so a PR titled fix: A & B "quoted" renders in Wire as fix: A & B "quoted". The issue templates already work around this with triple-stache ({{{issue.title}}}). Since you are in this file anyway, line 9 could become {{{pullRequest.title}}} for the same treatment. (sender.login needs no such change — GitHub logins are alphanumeric/hyphen only.)

Test coverage ⚠️

This is the one real gap. TemplateHandler has no rendering tests — ApplicationTest mocks it out entirely (every { templateHandler.handleEvent(...) } returns DUMMY_TEMPLATE), so nothing in CI would have caught the original bug, and nothing verifies this fix either. A small unit test would lock in the behaviour, e.g.:

@Test
fun `given merged PR closed by another user, when rendering, then sender is named`() {
    val output = TemplateHandler().handleEvent(
        event = "pull_request",
        response = gitHubResponse(
            action = "closed",
            sender = User(avatarUrl = "url", login = "merger"),
            pullRequest = pullRequest(user = User(avatarUrl = "url", login = "author"), merged = true)
        )
    )

    assertTrue(output!!.contains("Merged!** by **merger**"))
    assertFalse(output.contains("author"))
}

…plus the merged = false counterpart. The asymmetric fixture (sender ≠ author) is the important part — with identical logins the test would pass under both the old and the new template. This would also give you cheap regression coverage for the other nine templates and for the MustacheNotFoundException path.

---

Nice catch on the bug — no blocking concerns; items 1–4 and the test are follow-ups you can take or leave.