Skip to content

Commit 69d2e55

Browse files
Thomas StrombergThomas Stromberg
Thomas Stromberg
authored and
Thomas Stromberg
committed
Improve WorkflowState handling
1 parent 677f723 commit 69d2e55

File tree

4 files changed

+267
-54
lines changed

4 files changed

+267
-54
lines changed

‎pkg/bot/integration_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ func TestTurnclientEmojiIntegration(t *testing.T) {
107107
}{
108108
{
109109
name: "newly published takes precedence",
110-
workflowState: "newly_published",
110+
workflowState: string(turn.StateNewlyPublished),
111111
nextActions: map[string]turn.Action{
112112
"author": {Kind: turn.ActionFixTests},
113113
},
@@ -196,7 +196,7 @@ func TestTurnclientEmojiIntegration(t *testing.T) {
196196

197197
// Verify consistency: if there's a primary action, its emoji should match
198198
// (except for newly_published and merged edge cases)
199-
if tt.workflowState != "newly_published" && tt.workflowState != "merged" && tt.expectedAction != "" {
199+
if tt.workflowState != string(turn.StateNewlyPublished) && tt.workflowState != "merged" && tt.expectedAction != "" {
200200
actionEmoji := notify.PrefixForAction(tt.expectedAction)
201201
if emoji != actionEmoji {
202202
t.Errorf("%s: emoji mismatch - PrefixForAnalysis=%q but PrefixForAction(%q)=%q",

‎pkg/notify/format_test.go‎

Lines changed: 137 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -382,7 +382,7 @@ func TestFormatChannelMessageBase(t *testing.T) {
382382
CheckResult: &turn.CheckResponse{
383383
PullRequest: prx.PullRequest{},
384384
Analysis: turn.Analysis{
385-
WorkflowState: "newly_published",
385+
WorkflowState: string(turn.StateNewlyPublished),
386386
},
387387
},
388388
Owner: "test-owner",
@@ -395,24 +395,154 @@ func TestFormatChannelMessageBase(t *testing.T) {
395395
contains: []string{":new:", "New PR", "test-repo#44", "testuser", "?st=newly_published"},
396396
},
397397
{
398-
name: "with next actions",
398+
name: "IN_DRAFT",
399399
params: MessageParams{
400400
CheckResult: &turn.CheckResponse{
401401
PullRequest: prx.PullRequest{},
402402
Analysis: turn.Analysis{
403-
NextAction: map[string]turn.Action{
404-
"user1": {Kind: turn.ActionReview},
405-
},
403+
WorkflowState: string(turn.StateInDraft),
406404
},
407405
},
408406
Owner: "test-owner",
409407
Repo: "test-repo",
410408
PRNumber: 45,
411-
Title: "PR needs review",
409+
Title: "Draft PR",
412410
Author: "testuser",
413411
HTMLURL: "https://github.com/test-owner/test-repo/pull/45",
414412
},
415-
contains: []string{":hourglass:", "PR needs review", "test-repo#45", "testuser"},
413+
contains: []string{":construction:", "Draft PR", "test-repo#45", "testuser", "?st=draft"},
414+
},
415+
{
416+
name: "PUBLISHED_WAITING_FOR_TESTS with broken tests",
417+
params: MessageParams{
418+
CheckResult: &turn.CheckResponse{
419+
PullRequest: prx.PullRequest{},
420+
Analysis: turn.Analysis{
421+
WorkflowState: string(turn.StatePublishedWaitingForTests),
422+
NextAction: map[string]turn.Action{
423+
"author": {Kind: turn.ActionFixTests},
424+
},
425+
},
426+
},
427+
Owner: "test-owner",
428+
Repo: "test-repo",
429+
PRNumber: 46,
430+
Title: "PR with broken tests",
431+
Author: "testuser",
432+
HTMLURL: "https://github.com/test-owner/test-repo/pull/46",
433+
},
434+
contains: []string{":cockroach:", "PR with broken tests", "test-repo#46", "testuser", "?st=tests_broken"},
435+
},
436+
{
437+
name: "PUBLISHED_WAITING_FOR_TESTS with pending tests",
438+
params: MessageParams{
439+
CheckResult: &turn.CheckResponse{
440+
PullRequest: prx.PullRequest{},
441+
Analysis: turn.Analysis{
442+
WorkflowState: string(turn.StatePublishedWaitingForTests),
443+
NextAction: map[string]turn.Action{
444+
"author": {Kind: turn.ActionTestsPending},
445+
},
446+
},
447+
},
448+
Owner: "test-owner",
449+
Repo: "test-repo",
450+
PRNumber: 47,
451+
Title: "PR with tests running",
452+
Author: "testuser",
453+
HTMLURL: "https://github.com/test-owner/test-repo/pull/47",
454+
},
455+
contains: []string{":test_tube:", "PR with tests running", "test-repo#47", "testuser", "?st=tests_running"},
456+
},
457+
{
458+
name: "TESTED_WAITING_FOR_ASSIGNMENT",
459+
params: MessageParams{
460+
CheckResult: &turn.CheckResponse{
461+
PullRequest: prx.PullRequest{},
462+
Analysis: turn.Analysis{
463+
WorkflowState: string(turn.StateTestedWaitingForAssignment),
464+
},
465+
},
466+
Owner: "test-owner",
467+
Repo: "test-repo",
468+
PRNumber: 48,
469+
Title: "PR needs reviewer assignment",
470+
Author: "testuser",
471+
HTMLURL: "https://github.com/test-owner/test-repo/pull/48",
472+
},
473+
contains: []string{":shrug:", "PR needs reviewer assignment", "test-repo#48", "testuser", "?st=awaiting_assignment"},
474+
},
475+
{
476+
name: "ASSIGNED_WAITING_FOR_REVIEW",
477+
params: MessageParams{
478+
CheckResult: &turn.CheckResponse{
479+
PullRequest: prx.PullRequest{},
480+
Analysis: turn.Analysis{
481+
WorkflowState: string(turn.StateAssignedWaitingForReview),
482+
},
483+
},
484+
Owner: "test-owner",
485+
Repo: "test-repo",
486+
PRNumber: 49,
487+
Title: "PR awaiting review",
488+
Author: "testuser",
489+
HTMLURL: "https://github.com/test-owner/test-repo/pull/49",
490+
},
491+
contains: []string{":hourglass:", "PR awaiting review", "test-repo#49", "testuser", "?st=awaiting_review"},
492+
},
493+
{
494+
name: "REVIEWED_NEEDS_REFINEMENT",
495+
params: MessageParams{
496+
CheckResult: &turn.CheckResponse{
497+
PullRequest: prx.PullRequest{},
498+
Analysis: turn.Analysis{
499+
WorkflowState: string(turn.StateReviewedNeedsRefinement),
500+
},
501+
},
502+
Owner: "test-owner",
503+
Repo: "test-repo",
504+
PRNumber: 50,
505+
Title: "PR needs changes",
506+
Author: "testuser",
507+
HTMLURL: "https://github.com/test-owner/test-repo/pull/50",
508+
},
509+
contains: []string{":carpentry_saw:", "PR needs changes", "test-repo#50", "testuser", "?st=changes_requested"},
510+
},
511+
{
512+
name: "REFINED_WAITING_FOR_APPROVAL",
513+
params: MessageParams{
514+
CheckResult: &turn.CheckResponse{
515+
PullRequest: prx.PullRequest{},
516+
Analysis: turn.Analysis{
517+
WorkflowState: string(turn.StateRefinedWaitingForApproval),
518+
},
519+
},
520+
Owner: "test-owner",
521+
Repo: "test-repo",
522+
PRNumber: 51,
523+
Title: "PR awaiting approval",
524+
Author: "testuser",
525+
HTMLURL: "https://github.com/test-owner/test-repo/pull/51",
526+
},
527+
contains: []string{":hourglass:", "PR awaiting approval", "test-repo#51", "testuser", "?st=awaiting_approval"},
528+
},
529+
{
530+
name: "APPROVED_WAITING_FOR_MERGE",
531+
params: MessageParams{
532+
CheckResult: &turn.CheckResponse{
533+
PullRequest: prx.PullRequest{},
534+
Analysis: turn.Analysis{
535+
WorkflowState: string(turn.StateApprovedWaitingForMerge),
536+
},
537+
},
538+
Owner: "test-owner",
539+
Repo: "test-repo",
540+
PRNumber: 52,
541+
Title: "PR approved and ready",
542+
Author: "testuser",
543+
HTMLURL: "https://github.com/test-owner/test-repo/pull/52",
544+
},
545+
contains: []string{":white_check_mark:", "PR approved and ready", "test-repo#52", "testuser", "?st=approved"},
416546
},
417547
}
418548

‎pkg/notify/notify.go‎

Lines changed: 76 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -76,15 +76,18 @@ func FormatChannelMessageBase(ctx context.Context, params MessageParams) string
7676
} else if pr.State == "closed" {
7777
emoji, state = ":x:", "?st=closed"
7878
slog.Info("using :x: emoji - PR is closed but not merged", "pr", prID)
79-
} else if a.WorkflowState == "newly_published" {
80-
emoji, state = ":new:", "?st=newly_published"
81-
slog.Info("using :new: emoji - newly published PR", "pr", prID, "workflow_state", a.WorkflowState)
79+
} else if a.WorkflowState != "" {
80+
// Use WorkflowState as primary signal (most reliable source of truth)
81+
emoji, state = emojiFromWorkflowState(a.WorkflowState, a.NextAction)
82+
slog.Info("using emoji from workflow_state", "pr", prID, "workflow_state", a.WorkflowState, "emoji", emoji, "state_param", state)
8283
} else if len(a.NextAction) > 0 {
84+
// Fallback to NextAction if no WorkflowState (shouldn't normally happen)
8385
action := PrimaryAction(a.NextAction)
8486
emoji = PrefixForAction(action)
8587
state = stateParam(params.CheckResult)
86-
slog.Info("using emoji from primary next_action", "pr", prID, "primary_action", action, "emoji", emoji, "state_param", state)
88+
slog.Info("using emoji from primary next_action (no workflow_state)", "pr", prID, "primary_action", action, "emoji", emoji, "state_param", state)
8789
} else {
90+
// Final fallback based on PR properties
8891
emoji, state = fallbackEmoji(params.CheckResult)
8992
//nolint:revive // line length acceptable for structured logging
9093
slog.Info("using fallback emoji - no workflow_state or next_actions", "pr", prID, "emoji", emoji, "state_param", state, "fallback_reason", "empty_workflow_state_and_next_actions")
@@ -114,6 +117,62 @@ func FormatNextActionsSuffix(ctx context.Context, params MessageParams) string {
114117
return ""
115118
}
116119

120+
// emojiFromWorkflowState determines emoji based on WorkflowState as the primary signal.
121+
// Uses NextAction for additional granularity in specific states (e.g., test failures).
122+
func emojiFromWorkflowState(workflowState string, nextActions map[string]turn.Action) (emoji, state string) {
123+
switch workflowState {
124+
case string(turn.StateNewlyPublished):
125+
return ":new:", "?st=newly_published"
126+
127+
case string(turn.StateInDraft):
128+
return ":construction:", "?st=draft"
129+
130+
case string(turn.StatePublishedWaitingForTests):
131+
// Use NextAction to distinguish between broken tests vs pending tests
132+
if len(nextActions) > 0 {
133+
action := PrimaryAction(nextActions)
134+
if action == string(turn.ActionFixTests) {
135+
return ":cockroach:", "?st=tests_broken"
136+
}
137+
if action == string(turn.ActionTestsPending) {
138+
return ":test_tube:", "?st=tests_running"
139+
}
140+
}
141+
// Default to tests running
142+
return ":test_tube:", "?st=tests_running"
143+
144+
case string(turn.StateTestedWaitingForAssignment):
145+
// Waiting for reviewers to be assigned
146+
return ":shrug:", "?st=awaiting_assignment"
147+
148+
case string(turn.StateAssignedWaitingForReview):
149+
// Waiting for assigned reviewers to review
150+
return ":hourglass:", "?st=awaiting_review"
151+
152+
case string(turn.StateReviewedNeedsRefinement):
153+
// Author needs to address feedback/comments
154+
return ":carpentry_saw:", "?st=changes_requested"
155+
156+
case string(turn.StateRefinedWaitingForApproval):
157+
// Waiting for final approval after refinements
158+
return ":hourglass:", "?st=awaiting_approval"
159+
160+
case string(turn.StateApprovedWaitingForMerge):
161+
// Approved and ready to merge
162+
return ":white_check_mark:", "?st=approved"
163+
164+
default:
165+
// Unknown workflow state - fall back to NextAction
166+
slog.Warn("unknown workflow state, falling back to NextAction",
167+
"workflow_state", workflowState)
168+
if len(nextActions) > 0 {
169+
action := PrimaryAction(nextActions)
170+
return PrefixForAction(action), "?st=unknown"
171+
}
172+
return ":postal_horn:", "?st=unknown"
173+
}
174+
}
175+
117176
// stateParam returns the URL state parameter based on PR analysis.
118177
func stateParam(r *turn.CheckResponse) string {
119178
pr := r.PullRequest
@@ -376,8 +435,8 @@ func PrimaryAction(nextActions map[string]turn.Action) string {
376435

377436
// PrefixForAnalysis returns the emoji prefix based on workflow state and next actions.
378437
// This is the primary function for determining PR emoji - it handles the logic:
379-
// 1. If workflow_state == "newly_published" → ":new:".
380-
// 2. Otherwise → emoji based on primary next_action.
438+
// 1. Use WorkflowState as primary signal (most reliable)
439+
// 2. Fall back to NextAction if no WorkflowState
381440
func PrefixForAnalysis(workflowState string, nextActions map[string]turn.Action) string {
382441
// Log input for debugging emoji selection
383442
actionKinds := make([]string, 0, len(nextActions))
@@ -389,25 +448,27 @@ func PrefixForAnalysis(workflowState string, nextActions map[string]turn.Action)
389448
"next_actions_count", len(nextActions),
390449
"next_actions", actionKinds)
391450

392-
// Special case: newly published PRs always show :new:
393-
if workflowState == "newly_published" {
394-
slog.Debug("using :new: emoji for newly published PR")
395-
return ":new:"
451+
// Use WorkflowState as primary signal
452+
if workflowState != "" {
453+
emoji, _ := emojiFromWorkflowState(workflowState, nextActions)
454+
slog.Debug("determined emoji from workflow_state",
455+
"workflow_state", workflowState,
456+
"emoji", emoji)
457+
return emoji
396458
}
397459

398-
// Determine primary action and return corresponding emoji
460+
// Fallback to NextAction if no WorkflowState
399461
primaryAction := PrimaryAction(nextActions)
400462
if primaryAction != "" {
401463
emoji := PrefixForAction(primaryAction)
402-
slog.Debug("determined emoji from primary action",
464+
slog.Debug("determined emoji from primary action (no workflow_state)",
403465
"primary_action", primaryAction,
404466
"emoji", emoji)
405467
return emoji
406468
}
407469

408-
// Fallback if no actions
409-
slog.Info("no primary action found - using fallback emoji",
410-
"workflow_state", workflowState,
470+
// Final fallback if no workflow state or actions
471+
slog.Info("no workflow_state or primary action - using fallback emoji",
411472
"next_actions_count", len(nextActions),
412473
"fallback_emoji", ":postal_horn:")
413474
return ":postal_horn:"

0 commit comments

Comments
 (0)