Skip to content

Commit f40ebe6

Browse files
authored
fix(bitbucket): better handling of bot-initiated events (#30)
1 parent 3216d38 commit f40ebe6

11 files changed

Lines changed: 122 additions & 33 deletions

File tree

pkg/bitbucket/pull_requests.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ func InitPRData(ctx workflow.Context, event PullRequestEvent, prChannelID, slack
3030
slog.Any("error", err), slog.String("pr_url", prURL))
3131
}
3232

33-
email := users.BitbucketIDToEmail(ctx, event.Actor.AccountID)
33+
email := users.BitbucketActorToEmail(ctx, event.Actor)
3434
if email == "" {
3535
logger.From(ctx).Error("initializing Bitbucket PR data without author's email",
3636
slog.String("pr_url", prURL), slog.String("account_id", event.Actor.AccountID))
@@ -42,8 +42,8 @@ func InitPRData(ctx workflow.Context, event PullRequestEvent, prChannelID, slack
4242
data.InitTurns(ctx, prURL, email)
4343
}
4444

45-
// accountIDs extracts the IDs from a slice of [Account]s.
46-
// The output is guaranteed to be sorted, without repetitions, and not contain teams/apps.
45+
// accountIDs extracts the IDs from a slice of [Account]s. The output is guaranteed
46+
// to be sorted, without repetitions, and not contain apps/bots or teams.
4747
func accountIDs(accounts []Account) []string {
4848
if len(accounts) == 0 {
4949
return nil

pkg/bitbucket/pull_requests_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ func TestAccountIDs(t *testing.T) {
4545
accounts: []Account{
4646
{AccountID: "aaa", Type: "user"},
4747
{AccountID: "team1", Type: "team"},
48-
{AccountID: "app1", Type: "app"},
48+
{AccountID: "app1", Type: "app_user"},
4949
{AccountID: "bbb", Type: ""},
5050
},
5151
want: []string{"aaa", "bbb"},

pkg/bitbucket/workflows/comment.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,7 @@ func (c Config) CommentCreatedWorkflow(ctx workflow.Context, event bitbucket.Pul
3636
defer bitbucket.UpdateChannelBookmarks(ctx, event.PullRequest, prURL, channelID)
3737

3838
// Don't abort if this fails - it's more important to post the comment.
39-
email := users.BitbucketIDToEmail(ctx, event.Actor.AccountID)
40-
_ = data.SwitchTurn(ctx, prURL, email, false)
39+
_ = data.SwitchTurn(ctx, prURL, users.BitbucketActorToEmail(ctx, event.Actor), false)
4140

4241
// If the comment was created by RevChat, i.e. mirrored from Slack, don't repost it.
4342
// Also, don't poll Bitbucket for updates because we expect them to come from Slack.
@@ -168,7 +167,7 @@ func CommentResolvedWorkflow(ctx workflow.Context, event bitbucket.PullRequestEv
168167
return nil
169168
}
170169

171-
data.UpdateActivityTime(ctx, prURL, data.BitbucketIDToEmail(ctx, event.Actor.AccountID))
170+
data.UpdateActivityTime(ctx, prURL, users.BitbucketActorToEmail(ctx, event.Actor))
172171
defer bitbucket.UpdateChannelBookmarks(ctx, event.PullRequest, prURL, channelID)
173172

174173
url := bitbucket.HTMLURL(event.Comment.Links)
@@ -187,7 +186,7 @@ func CommentReopenedWorkflow(ctx workflow.Context, event bitbucket.PullRequestEv
187186
return nil
188187
}
189188

190-
data.UpdateActivityTime(ctx, prURL, data.BitbucketIDToEmail(ctx, event.Actor.AccountID))
189+
data.UpdateActivityTime(ctx, prURL, users.BitbucketActorToEmail(ctx, event.Actor))
191190
defer bitbucket.UpdateChannelBookmarks(ctx, event.PullRequest, prURL, channelID)
192191

193192
url := bitbucket.HTMLURL(event.Comment.Links)

pkg/bitbucket/workflows/pullrequest.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -129,13 +129,13 @@ func (c Config) PullRequestUpdatedWorkflow(ctx workflow.Context, event bitbucket
129129

130130
defer bitbucket.UpdateChannelBookmarks(ctx, pr, prURL, channelID)
131131

132-
email := data.BitbucketIDToEmail(ctx, event.Actor.AccountID)
132+
email := users.BitbucketActorToEmail(ctx, event.Actor)
133133
var errs []error
134134

135135
// Announce transitions between draft and ready-to-review modes.
136136
if !snapshot.Draft && pr.Draft {
137137
bitbucket.MentionUserInMsg(ctx, channelID, event.Actor, "%s marked this PR as a draft. :construction:")
138-
_, _, _ = data.Nudge(ctx, prURL, data.BitbucketIDToEmail(ctx, event.PullRequest.Author.AccountID))
138+
_, _, _ = data.Nudge(ctx, prURL, users.BitbucketActorToEmail(ctx, event.PullRequest.Author))
139139
} else if snapshot.Draft && !pr.Draft {
140140
bitbucket.MentionUserInMsg(ctx, channelID, event.Actor, "%s marked this PR as ready for review. :eyes:")
141141
_ = data.SwitchTurn(ctx, prURL, email, true)
@@ -240,7 +240,7 @@ func PullRequestReviewedWorkflow(ctx workflow.Context, event bitbucket.PullReque
240240

241241
defer bitbucket.UpdateChannelBookmarks(ctx, event.PullRequest, prURL, channelID)
242242

243-
email := users.BitbucketIDToEmail(ctx, event.Actor.AccountID)
243+
email := users.BitbucketActorToEmail(ctx, event.Actor)
244244
msg := "%s "
245245
var err error
246246

pkg/data/turns.go

Lines changed: 34 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -427,20 +427,20 @@ func readTurnsFile(ctx workflow.Context, url string) (*PRTurns, error) {
427427
f, err := os.Open(path) //gosec:disable G304 // URL received from signature-verified 3rd-party, suffix is hardcoded.
428428
if err != nil {
429429
if errors.Is(err, os.ErrNotExist) {
430-
return resetTurns(ctx, url)
430+
return resetTurns(ctx, url, nil)
431431
}
432432
return nil, err
433433
}
434434
defer f.Close()
435435

436436
t := new(PRTurns)
437437
if err := json.NewDecoder(f).Decode(&t); err != nil {
438-
return resetTurns(ctx, url)
438+
return resetTurns(ctx, url, nil)
439439
}
440440

441441
// Data sanity checks.
442442
if t.Author == "" {
443-
return resetTurns(ctx, url)
443+
return resetTurns(ctx, url, t)
444444
}
445445
normalizeEmailAddresses(t)
446446
if t.Reviewers == nil {
@@ -513,51 +513,67 @@ func writeTurnsFileActivity(_ context.Context, url string, t *PRTurns) error {
513513

514514
// resetTurns recreates the attention state file for a specific PR, based on the current
515515
// PR snapshot. This is a fallback for when the turn file is missing or corrupted.
516-
func resetTurns(ctx workflow.Context, url string) (*PRTurns, error) {
516+
func resetTurns(ctx workflow.Context, url string, t *PRTurns) (*PRTurns, error) {
517517
logger.From(ctx).Warn("resetting PR attention state file", slog.String("pr_url", url))
518518

519519
pr, err := LoadPRSnapshot(ctx, url)
520520
if err != nil {
521521
return nil, err
522522
}
523523

524-
author := userEmail(ctx, pr["author"])
524+
author, accountType := userEmailAndType(ctx, pr["author"])
525+
if author == "" && accountType == "app_user" {
526+
author = "bot"
527+
}
528+
529+
// If the only thing that needs fixing is the author's email, do just that.
530+
if t != nil && len(t.Reviewers)+len(t.Activity)+len(t.Approvers) > 0 {
531+
t.Author = author
532+
return t, writeTurnsFile(ctx, url, t)
533+
}
525534

535+
// Otherwise, recreate the entire struct and file.
526536
reviewers := map[string]bool{}
527537
jsonList, ok := pr["reviewers"].([]any)
528538
if !ok {
529539
jsonList = []any{}
530540
}
531541
for _, r := range jsonList {
532-
if email := userEmail(ctx, r); email != "" {
542+
if email, _ := userEmailAndType(ctx, r); email != "" {
533543
reviewers[email] = true
534544
}
535545
}
536546

537-
t := &PRTurns{Author: author, Reviewers: reviewers, Activity: map[string]time.Time{}, Approvers: map[string]time.Time{}}
547+
approvers := map[string]time.Time{}
548+
549+
t = &PRTurns{Author: author, Reviewers: reviewers, Activity: map[string]time.Time{}, Approvers: approvers}
538550
return t, writeTurnsFile(ctx, url, t)
539551
}
540552

541-
// userEmail extracts the Bitbucket account ID from user details map, and converts
542-
// it into the user's email address, based on RevChat's own user database.
543-
func userEmail(ctx workflow.Context, detailsMap any) string {
553+
// userEmailAndType extracts the Bitbucket account ID from user details map, and converts
554+
// it into the user's email address and account type, based on RevChat's own user database.
555+
func userEmailAndType(ctx workflow.Context, detailsMap any) (email, accountType string) {
544556
user, ok := detailsMap.(map[string]any)
545557
if !ok {
546-
return ""
558+
return "", ""
547559
}
548560

549561
accountID, ok := user["account_id"].(string)
550562
if !ok {
551-
return ""
563+
return "", ""
564+
}
565+
accountType, ok = user["type"].(string)
566+
if !ok {
567+
accountType = ""
552568
}
553569

554-
return BitbucketIDToEmail(ctx, accountID)
570+
return BitbucketIDToEmail(ctx, accountID, accountType), accountType
555571
}
556572

557573
// BitbucketIDToEmail converts a Bitbucket account ID into an email address. This function returns an empty
558574
// string if the account ID is not found. It uses persistent data storage, or API calls as a fallback.
559575
// This function is also wrapped in the "users" package, and reused by other packages from there.
560-
func BitbucketIDToEmail(ctx workflow.Context, accountID string) string {
576+
func BitbucketIDToEmail(ctx workflow.Context, accountID, accountType string) string {
561577
if accountID == "" {
562578
return ""
563579
}
@@ -566,6 +582,10 @@ func BitbucketIDToEmail(ctx workflow.Context, accountID string) string {
566582
return user.Email
567583
}
568584

585+
if accountType == "app_user" {
586+
return "" // Not a real user, so no point to check in Jira.
587+
}
588+
569589
// We use the Jira API as a fallback because the Bitbucket API doesn't expose email addresses.
570590
jiraUser, err := jira.UsersGet(ctx, accountID)
571591
if err != nil {

pkg/data/users.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ func SelectUserByBitbucketID(ctx workflow.Context, accountID string) User {
153153
}
154154

155155
func SelectUserByEmail(ctx workflow.Context, email string) User {
156-
if email == "" {
156+
if email == "" || email == "bot" {
157157
return User{}
158158
}
159159

pkg/slack/commands/turns.go

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,16 @@ func WhoseTurn(ctx workflow.Context, event SlashCommandEvent) error {
168168
}
169169

170170
// whoseTurnText builds a "whose turn is it" summary message, reused by multiple slash commands.
171+
// The emails slice must be deduped, but may contain invalid/bot email addresses (which are removed).
171172
func whoseTurnText(ctx workflow.Context, emails []string, user data.User, tweak string) string {
173+
// Ignore invalid/bot email addresses.
174+
if i := slices.Index(emails, ""); i > -1 {
175+
emails = slices.Delete(emails, i, i+1)
176+
}
177+
if i := slices.Index(emails, "bot"); i > -1 {
178+
emails = slices.Delete(emails, i, i+1)
179+
}
180+
172181
// If the user who ran the command is in the list, highlight that to them.
173182
var msg strings.Builder
174183
i := slices.Index(emails, user.Email)
@@ -180,11 +189,14 @@ func whoseTurnText(ctx workflow.Context, emails []string, user data.User, tweak
180189
msg.WriteString(tweak)
181190

182191
withOthers := false
183-
if i == -1 {
192+
switch {
193+
case i == -1 && len(emails) == 0:
194+
msg.WriteString(" no one's turn")
195+
case i == -1 && len(emails) > 0:
184196
msg.WriteString(" the turn of ")
185-
} else {
186-
emails = slices.Delete(emails, i, i+1)
197+
default:
187198
msg.WriteString(" *your* turn")
199+
emails = slices.Delete(emails, i, i+1)
188200
if len(emails) > 0 {
189201
msg.WriteString(" - along with ")
190202
withOthers = true

pkg/slack/commands/turns_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,12 @@ func TestWhoseTurnText(t *testing.T) {
1717
tweak string
1818
want string
1919
}{
20+
{
21+
name: "no_one",
22+
emails: []string{"", "bot"},
23+
user: data.User{Email: "author@example.com"},
24+
want: "I think it's no one's turn to review this PR.",
25+
},
2026
{
2127
name: "author_only_with_tweak",
2228
emails: []string{"author@example.com"},

pkg/slack/pr_data.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,9 @@ func LoadPRTurns(ctx workflow.Context, onlyCurrentTurn, authors, reviewers bool)
5151
}
5252

5353
for _, email := range emails {
54+
if email == "" || email == "bot" {
55+
continue // Nothing to check/remove.
56+
}
5457
if id := users.EmailToSlackID(ctx, email); id != "" {
5558
usersToPRs[id] = append(usersToPRs[id], prURL)
5659
continue

pkg/users/bitbucket.go

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,20 @@ func EmailToBitbucketID(ctx workflow.Context, email string) string {
4545
return users[0].AccountID
4646
}
4747

48+
// BitbucketActorToEmail is a trivial wrapper around [BitbucketIDToEmail]. Merely syntactic sugar.
49+
func BitbucketActorToEmail(ctx workflow.Context, actor bitbucket.User) string {
50+
return BitbucketIDToEmail(ctx, actor.AccountID, actor.Type)
51+
}
52+
4853
// BitbucketIDToEmail is a trivial wrapper around [data.BitbucketIDToEmail].
49-
func BitbucketIDToEmail(ctx workflow.Context, accountID string) string {
50-
return data.BitbucketIDToEmail(ctx, accountID)
54+
// It exists only for architectural clarity (all "XxxToYyy" functions in this package).
55+
// However, outside the "data" package, we return "bot" instead of "" for non-user accounts.
56+
func BitbucketIDToEmail(ctx workflow.Context, accountID, accountType string) string {
57+
email := data.BitbucketIDToEmail(ctx, accountID, accountType)
58+
if email == "" && accountType == "app_user" {
59+
return "bot"
60+
}
61+
return email
5162
}
5263

5364
// BitbucketIDToSlackID converts a Bitbucket account ID into a Slack user ID. This function returns an empty
@@ -56,7 +67,7 @@ func BitbucketIDToSlackID(ctx workflow.Context, accountID string, checkOptIn boo
5667
user := data.SelectUserByBitbucketID(ctx, accountID)
5768
if user.SlackID == "" {
5869
// Workaround in case only the user's Bitbucket account ID isn't stored yet, but the rest is.
59-
user = data.SelectUserByEmail(ctx, BitbucketIDToEmail(ctx, accountID))
70+
user = data.SelectUserByEmail(ctx, BitbucketIDToEmail(ctx, accountID, "user"))
6071
}
6172

6273
if checkOptIn && !user.IsOptedIn() {
@@ -72,7 +83,7 @@ func BitbucketIDToSlackRef(ctx workflow.Context, accountID, displayName string)
7283
user := data.SelectUserByBitbucketID(ctx, accountID)
7384
if user.SlackID == "" {
7485
// Workaround in case only the user's Bitbucket account ID isn't stored yet, but the rest is.
75-
user = data.SelectUserByEmail(ctx, BitbucketIDToEmail(ctx, accountID))
86+
user = data.SelectUserByEmail(ctx, BitbucketIDToEmail(ctx, accountID, "user"))
7687
}
7788

7889
if user.SlackID != "" {

0 commit comments

Comments
 (0)