diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 9846e59..02f43ec 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -4,7 +4,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co ## Project Overview -Ready to Review is an elegant modern Slack bot written in Go that integrates with GitHub to streamline PR review workflows. The bot provides real-time notifications, dashboard views, and multi-org/multi-Slack support. +reviewGOOSE:Slack is the Slack integration for reviewGOOSE — an elegant modern Slack bot written in Go that integrates with GitHub to streamline PR review workflows. The bot provides real-time notifications, dashboard views, and multi-org/multi-Slack support. ## Core Features @@ -27,7 +27,7 @@ Ready to Review is an elegant modern Slack bot written in Go that integrates wit - Native Slack app home tab with Block Kit UI showing incoming/outgoing PRs - Highlights PRs blocked on the user - Clean, settings-free interface focusing on PR status -- Alternative web dashboard available at https://dash.ready-to-review.dev/ +- Alternative web dashboard available at https://reviewgoose.dev/ ### 3. Smart Notifications - **Smart DM Logic**: If user tagged in channel, delay DMs by configured time (default: 65min) @@ -92,7 +92,7 @@ make clean # Clean build artifacts ### External Dependencies - `github.com/codeGROOVE-dev/sprinkler` - WebSocket hub for GitHub webhook events -- `github.com/ready-to-review/turnclient` - PR state analysis and blocking detection +- `github.com/codeGROOVE-dev/turnclient` - PR state analysis and blocking detection - `github.com/slack-go/slack` - Official Slack API client - `github.com/google/go-github/v50` - GitHub API client diff --git a/README.md b/README.md index d766939..440c54e 100644 --- a/README.md +++ b/README.md @@ -1,13 +1,15 @@ -# Ready-to-Review Slacker +# reviewGOOSE:Slack [](https://goreportcard.com/report/github.com/codeGROOVE-dev/slacker) [](https://godoc.org/github.com/codeGROOVE-dev/slacker) [](https://www.gnu.org/licenses/gpl-3.0) [](go.mod) - + -Slack bot that tracks GitHub pull requests and notifies reviewers when it's their turn. Part of the https://codegroove.dev/ ecosystem of developer acceleration tools. +The Slack integration for [reviewGOOSE](https://codegroove.dev/reviewgoose/) — know instantly when you're blocking a PR. + +**reviewGOOSE:Slack** tracks GitHub pull requests and notifies reviewers when it's their turn. Works alongside [reviewGOOSE:Desktop](https://github.com/codeGROOVE-dev/goose) for a complete PR tracking experience. Part of the [codeGROOVE](https://codegroove.dev/) ecosystem. ## Quick Start @@ -137,10 +139,10 @@ channels: ## Usage Slack commands: -- `/r2r dashboard` - View your PR dashboard -- `/r2r help` - Show help +- `/goose dashboard` - View your PR dashboard +- `/goose help` - Show help -The dashboard is also available in the app's Home tab or at https://dash.ready-to-review.dev/ +The dashboard is also available in the app's Home tab or at https://reviewgoose.dev/ ## Smart Notification Logic diff --git a/cmd/server/main.go b/cmd/server/main.go index cf22201..ec44831 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -262,7 +262,7 @@ func run(ctx context.Context, cancel context.CancelFunc, cfg *config.ServerConfi homeHandler := slack.NewHomeHandler(slackManager, githubManager, configManager, stateStore, reverseMapping) slackManager.SetHomeViewHandler(homeHandler.HandleAppHomeOpened) - // Initialize report handler for /r2r report slash command + // Initialize report handler for /goose report slash command reportHandler := slack.NewReportHandler(slackManager, githubManager, stateStore, reverseMapping) slackManager.SetReportHandler(reportHandler.HandleReportCommand) diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 9faeec3..159b578 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -1,10 +1,10 @@ # Deployment Guide -This guide is for self-hosting Ready-to-Review. If you're using the SaaS version, see [SETUP.md](SETUP.md) instead. +This guide is for self-hosting reviewGOOSE:Slack. If you're using the SaaS version, see [SETUP.md](SETUP.md) instead. ## Architecture Overview -Ready-to-Review consists of two services: +reviewGOOSE:Slack consists of two services: 1. **slacker** - Main bot server that handles GitHub webhooks and Slack notifications 2. **slacker-registrar** - OAuth-only service for multi-workspace installations @@ -44,7 +44,7 @@ Ready-to-Review consists of two services: 1. Go to https://api.slack.com/apps 2. Click **"Create New App"** 3. Choose **"From scratch"** -4. Name your app (e.g., "Ready-to-Review") +4. Name your app (e.g., "reviewGOOSE") 5. Select your development workspace ### Step 2: Configure OAuth & Permissions diff --git a/docs/SETUP.md b/docs/SETUP.md index 28cd56b..f626f17 100644 --- a/docs/SETUP.md +++ b/docs/SETUP.md @@ -1,6 +1,6 @@ -# Getting Started with Ready-to-Review +# Getting Started with reviewGOOSE:Slack -Ready-to-Review is a Slack bot that keeps your team informed about GitHub pull requests. It creates threads for PRs, tracks their status with emojis, and sends smart notifications so reviewers know when it's their turn. +reviewGOOSE:Slack is the Slack integration for [reviewGOOSE](https://codegroove.dev/reviewgoose/) — it keeps your team informed about GitHub pull requests. It creates threads for PRs, tracks their status with emojis, and sends smart notifications so reviewers know when it's their turn. ## Quick Start @@ -37,7 +37,7 @@ Once installed, the bot needs to know which GitHub repositories to track. Contin ## Configuring Your Repositories -Ready-to-Review reads its configuration from a special GitHub repository in your organization. This lets you version-control your notification settings centrally. +reviewGOOSE:Slack reads its configuration from a special GitHub repository in your organization. This lets you version-control your notification settings centrally. ### Step 1: Create the .codeGROOVE Repository @@ -110,7 +110,7 @@ channels: ```bash git add slack.yaml -git commit -m "Configure Ready-to-Review Slack bot" +git commit -m "Configure reviewGOOSE:Slack" git push ``` @@ -338,17 +338,17 @@ channels: ## Viewing Your Dashboard -Ready-to-Review provides two ways to view your PRs: +reviewGOOSE:Slack provides two ways to view your PRs: ### 1. Slack App Home -1. Click "Ready-to-Review" in your Slack sidebar +1. Click "reviewGOOSE" in your Slack sidebar 2. Select the "Home" tab 3. View incoming PRs (waiting on you) and outgoing PRs (waiting on others) ### 2. Web Dashboard -Visit [dash.ready-to-review.dev](https://dash.ready-to-review.dev/) for a comprehensive web view. +Visit [reviewgoose.dev](https://reviewgoose.dev/) for a comprehensive web view. --- @@ -365,7 +365,7 @@ Visit [dash.ready-to-review.dev](https://dash.ready-to-review.dev/) for a compre **Check channel permissions:** - The bot needs permission to post in the channel -- Try inviting the bot: `/invite @Ready-to-Review` +- Try inviting the bot: `/invite @goose` ### Not receiving DMs @@ -452,16 +452,16 @@ Before enabling: - **Documentation:** [github.com/codeGROOVE-dev/slacker](https://github.com/codeGROOVE-dev/slacker) - **Issues:** [GitHub Issues](https://github.com/codeGROOVE-dev/slacker/issues) -- **Support:** Contact your Ready-to-Review administrator +- **Support:** Contact your reviewGOOSE administrator --- ## What's Next? -Ready-to-Review is part of the codeGROOVE ecosystem of developer tools. Check out: +reviewGOOSE:Slack is part of the codeGROOVE ecosystem of developer tools. Check out: -- **[Sprinkler](https://github.com/codeGROOVE-dev/sprinkler)** - Real-time GitHub webhook hub -- **[Ready-to-Review Dashboard](https://dash.ready-to-review.dev/)** - Web-based PR dashboard +- **[reviewGOOSE:Desktop](https://github.com/codeGROOVE-dev/goose)** - Desktop app with honk notifications +- **[reviewGOOSE Dashboard](https://reviewgoose.dev/)** - Web-based PR dashboard - **[codeGROOVE.dev](https://codegroove.dev/)** - Developer acceleration tools Happy reviewing! diff --git a/pkg/bot/bot.go b/pkg/bot/bot.go index 5a56e49..ce0eb6f 100644 --- a/pkg/bot/bot.go +++ b/pkg/bot/bot.go @@ -853,6 +853,78 @@ func (*Coordinator) extractBlockedUsersFromTurnclient(checkResult *turn.CheckRes return blockedUsers } +// shouldPostThread determines if a PR thread should be posted based on configured threshold. +// Returns (shouldPost bool, reason string). +func (c *Coordinator) shouldPostThread( + checkResult *turn.CheckResponse, + when string, +) (bool, string) { + if checkResult == nil { + return false, "no_check_result" + } + + pr := checkResult.PullRequest + analysis := checkResult.Analysis + + // Terminal states ALWAYS post (ensure visibility) + if pr.Merged { + return true, "pr_merged" + } + if pr.State == "closed" { + return true, "pr_closed" + } + + switch when { + case "immediate": + return true, "immediate_mode" + + case "assigned": + // Post when PR has assignees + if len(pr.Assignees) > 0 { + return true, fmt.Sprintf("has_%d_assignees", len(pr.Assignees)) + } + return false, "no_assignees" + + case "blocked": + // Post when someone needs to take action + blockedUsers := c.extractBlockedUsersFromTurnclient(checkResult) + if len(blockedUsers) > 0 { + return true, fmt.Sprintf("blocked_on_%d_users", len(blockedUsers)) + } + return false, "not_blocked_yet" + + case "passing": + // Post when tests pass - use WorkflowState as primary signal + switch analysis.WorkflowState { + case string(turn.StateAssignedWaitingForReview), + string(turn.StateReviewedNeedsRefinement), + string(turn.StateRefinedWaitingForApproval), + string(turn.StateApprovedWaitingForMerge): + return true, fmt.Sprintf("workflow_state_%s", analysis.WorkflowState) + + case string(turn.StateNewlyPublished), + string(turn.StateInDraft), + string(turn.StatePublishedWaitingForTests), + string(turn.StateTestedWaitingForAssignment): + return false, fmt.Sprintf("waiting_for_%s", analysis.WorkflowState) + + default: + // Fallback: check test status directly + if analysis.Checks.Failing > 0 { + return false, "tests_failing" + } + if analysis.Checks.Pending > 0 || analysis.Checks.Waiting > 0 { + return false, "tests_pending" + } + return true, "tests_passed_fallback" + } + + default: + slog.Warn("invalid when value, defaulting to immediate", "when", when) + return true, "invalid_config_default_immediate" + } +} + // formatNextActions formats NextAction map into a compact string like "fix tests: user1, user2; review: user3". // It groups users by action kind and formats each action as "action_name: user1, user2". // Multiple actions are separated by semicolons. @@ -1082,7 +1154,8 @@ func (c *Coordinator) processPRForChannel( oldState = threadInfo.LastState } - // Find or create thread + // Check if thread already exists + cacheKey := fmt.Sprintf("%s/%s#%d:%s", owner, repo, prNumber, channelID) pullRequestStruct := struct { CreatedAt time.Time `json:"created_at"` User struct { @@ -1098,6 +1171,36 @@ func (c *Coordinator) processPRForChannel( Number: event.PullRequest.Number, CreatedAt: event.PullRequest.CreatedAt, } + + _, _, threadExists := c.findPRThread(ctx, cacheKey, channelID, owner, repo, prNumber, prState, pullRequestStruct) + + // If thread doesn't exist, check if we should create it based on "when" threshold + if !threadExists { + when := c.configManager.When(owner, channelName) + + if when != "immediate" { + shouldPost, reason := c.shouldPostThread(checkResult, when) + + if !shouldPost { + slog.Debug("not creating thread - threshold not met", + "workspace", c.workspaceName, + logFieldPR, fmt.Sprintf(prFormatString, owner, repo, prNumber), + "channel", channelDisplay, + "when", when, + "reason", reason) + return nil // Don't create thread yet - next event will check again + } + + slog.Info("creating thread - threshold met", + "workspace", c.workspaceName, + logFieldPR, fmt.Sprintf(prFormatString, owner, repo, prNumber), + "channel", channelDisplay, + "when", when, + "reason", reason) + } + } + + // Find or create thread threadTS, wasNewlyCreated, currentText, err := c.findOrCreatePRThread(ctx, threadCreationParams{ ChannelID: channelID, ChannelName: channelName, diff --git a/pkg/bot/bot_test.go b/pkg/bot/bot_test.go index 6c27df2..7057804 100644 --- a/pkg/bot/bot_test.go +++ b/pkg/bot/bot_test.go @@ -6,7 +6,9 @@ import ( "os" "testing" + "github.com/codeGROOVE-dev/prx/pkg/prx" "github.com/codeGROOVE-dev/slacker/pkg/bot/cache" + "github.com/codeGROOVE-dev/turnclient/pkg/turn" "github.com/slack-go/slack" ) @@ -309,3 +311,338 @@ func TestThreadCache_Set(t *testing.T) { t.Errorf("expected channel ID %s, got %s", threadInfo.ChannelID, retrieved.ChannelID) } } + +func TestShouldPostThread(t *testing.T) { + coord := &Coordinator{} + + tests := []struct { + name string + when string + checkResult *turn.CheckResponse + wantPost bool + wantReasonPart string // Part of the reason string to check for + }{ + { + name: "immediate mode always posts", + when: "immediate", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + }, + }, + wantPost: true, + wantReasonPart: "immediate_mode", + }, + { + name: "merged PR always posts regardless of when", + when: "passing", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "closed", + Merged: true, + }, + }, + wantPost: true, + wantReasonPart: "pr_merged", + }, + { + name: "closed PR always posts regardless of when", + when: "passing", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "closed", + Merged: false, + }, + }, + wantPost: true, + wantReasonPart: "pr_closed", + }, + { + name: "assigned: posts when has assignees", + when: "assigned", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + Assignees: []string{"user1", "user2"}, + }, + }, + wantPost: true, + wantReasonPart: "has_2_assignees", + }, + { + name: "assigned: does not post when no assignees", + when: "assigned", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + Assignees: []string{}, + }, + }, + wantPost: false, + wantReasonPart: "no_assignees", + }, + { + name: "blocked: posts when users are blocked", + when: "blocked", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + }, + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + "user2": {Kind: "approve"}, + }, + }, + }, + wantPost: true, + wantReasonPart: "blocked_on_2_users", + }, + { + name: "blocked: does not post when no users blocked", + when: "blocked", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + }, + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{}, + }, + }, + wantPost: false, + wantReasonPart: "not_blocked_yet", + }, + { + name: "blocked: ignores _system sentinel", + when: "blocked", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + }, + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "_system": {Kind: "processing"}, + }, + }, + }, + wantPost: false, + wantReasonPart: "not_blocked_yet", + }, + { + name: "passing: posts when in review state", + when: "passing", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + }, + Analysis: turn.Analysis{ + WorkflowState: string(turn.StateAssignedWaitingForReview), + }, + }, + wantPost: true, + wantReasonPart: "workflow_state", + }, + { + name: "passing: does not post when tests pending", + when: "passing", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + }, + Analysis: turn.Analysis{ + WorkflowState: string(turn.StatePublishedWaitingForTests), + }, + }, + wantPost: false, + wantReasonPart: "waiting_for", + }, + { + name: "passing: uses fallback when workflow state unknown and tests passing", + when: "passing", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + }, + Analysis: turn.Analysis{ + WorkflowState: "unknown_state", + Checks: turn.Checks{ + Passing: 5, + Failing: 0, + Pending: 0, + Waiting: 0, + }, + }, + }, + wantPost: true, + wantReasonPart: "tests_passed_fallback", + }, + { + name: "passing: uses fallback when tests failing", + when: "passing", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + }, + Analysis: turn.Analysis{ + WorkflowState: "unknown_state", + Checks: turn.Checks{ + Passing: 3, + Failing: 2, + }, + }, + }, + wantPost: false, + wantReasonPart: "tests_failing", + }, + { + name: "nil check result returns false", + when: "passing", + checkResult: nil, + wantPost: false, + wantReasonPart: "no_check_result", + }, + { + name: "invalid when value defaults to immediate", + when: "invalid_value", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + }, + }, + wantPost: true, + wantReasonPart: "invalid_config", + }, + { + name: "passing: posts when in StateReviewedNeedsRefinement", + when: "passing", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + }, + Analysis: turn.Analysis{ + WorkflowState: string(turn.StateReviewedNeedsRefinement), + }, + }, + wantPost: true, + wantReasonPart: "workflow_state", + }, + { + name: "passing: posts when in StateRefinedWaitingForApproval", + when: "passing", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + }, + Analysis: turn.Analysis{ + WorkflowState: string(turn.StateRefinedWaitingForApproval), + }, + }, + wantPost: true, + wantReasonPart: "workflow_state", + }, + { + name: "passing: posts when in StateApprovedWaitingForMerge", + when: "passing", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + }, + Analysis: turn.Analysis{ + WorkflowState: string(turn.StateApprovedWaitingForMerge), + }, + }, + wantPost: true, + wantReasonPart: "workflow_state", + }, + { + name: "passing: does not post when StateNewlyPublished", + when: "passing", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + }, + Analysis: turn.Analysis{ + WorkflowState: string(turn.StateNewlyPublished), + }, + }, + wantPost: false, + wantReasonPart: "waiting_for", + }, + { + name: "passing: does not post when StateInDraft", + when: "passing", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + }, + Analysis: turn.Analysis{ + WorkflowState: string(turn.StateInDraft), + }, + }, + wantPost: false, + wantReasonPart: "waiting_for", + }, + { + name: "passing: does not post when StateTestedWaitingForAssignment", + when: "passing", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + }, + Analysis: turn.Analysis{ + WorkflowState: string(turn.StateTestedWaitingForAssignment), + }, + }, + wantPost: false, + wantReasonPart: "waiting_for", + }, + { + name: "passing: uses fallback when tests have Waiting status", + when: "passing", + checkResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + }, + Analysis: turn.Analysis{ + WorkflowState: "unknown_state", + Checks: turn.Checks{ + Passing: 0, + Failing: 0, + Pending: 0, + Waiting: 3, + }, + }, + }, + wantPost: false, + wantReasonPart: "tests_pending", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotPost, gotReason := coord.shouldPostThread(tt.checkResult, tt.when) + + if gotPost != tt.wantPost { + t.Errorf("shouldPostThread() gotPost = %v, wantPost %v", gotPost, tt.wantPost) + } + + if tt.wantReasonPart != "" && !contains(gotReason, tt.wantReasonPart) { + t.Errorf("shouldPostThread() reason = %q, want to contain %q", gotReason, tt.wantReasonPart) + } + }) + } +} + +// Helper function to check if a string contains a substring +func contains(s, substr string) bool { + return len(s) >= len(substr) && (s == substr || len(s) > len(substr) && (s[:len(substr)] == substr || s[len(s)-len(substr):] == substr || containsMiddle(s, substr))) +} + +func containsMiddle(s, substr string) bool { + for i := 0; i <= len(s)-len(substr); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false +} diff --git a/pkg/bot/coordinator_test_helpers.go b/pkg/bot/coordinator_test_helpers.go index a1eed9a..3aa11a0 100644 --- a/pkg/bot/coordinator_test_helpers.go +++ b/pkg/bot/coordinator_test_helpers.go @@ -18,17 +18,20 @@ import ( // //nolint:govet // fieldalignment optimization would reduce test readability type mockStateStore struct { - markProcessedErr error - saveThreadErr error - saveDMMessageErr error - threads map[string]cache.ThreadInfo - dmTimes map[string]time.Time - dmUsers map[string][]string - dmMessages map[string]state.DMInfo - pendingDMs []*state.PendingDM - processedEvents map[string]bool - lastNotifications map[string]time.Time - mu sync.Mutex + markProcessedErr error + saveThreadErr error + saveDMMessageErr error + queuePendingDMErr error + pendingDMsErr error + removePendingDMErr error + threads map[string]cache.ThreadInfo + dmTimes map[string]time.Time + dmUsers map[string][]string + dmMessages map[string]state.DMInfo + pendingDMs []*state.PendingDM + processedEvents map[string]bool + lastNotifications map[string]time.Time + mu sync.Mutex } func (m *mockStateStore) Thread(ctx context.Context, owner, repo string, number int, channelID string) (cache.ThreadInfo, bool) { @@ -180,6 +183,9 @@ func (m *mockStateStore) RecordNotification(ctx context.Context, prURL string, n func (m *mockStateStore) QueuePendingDM(ctx context.Context, dm *state.PendingDM) error { m.mu.Lock() defer m.mu.Unlock() + if m.queuePendingDMErr != nil { + return m.queuePendingDMErr + } m.pendingDMs = append(m.pendingDMs, dm) return nil } @@ -187,6 +193,9 @@ func (m *mockStateStore) QueuePendingDM(ctx context.Context, dm *state.PendingDM func (m *mockStateStore) PendingDMs(ctx context.Context, before time.Time) ([]state.PendingDM, error) { m.mu.Lock() defer m.mu.Unlock() + if m.pendingDMsErr != nil { + return nil, m.pendingDMsErr + } var result []state.PendingDM for _, dm := range m.pendingDMs { if dm.SendAfter.Before(before) { @@ -199,6 +208,9 @@ func (m *mockStateStore) PendingDMs(ctx context.Context, before time.Time) ([]st func (m *mockStateStore) RemovePendingDM(ctx context.Context, id string) error { m.mu.Lock() defer m.mu.Unlock() + if m.removePendingDMErr != nil { + return m.removePendingDMErr + } for i, dm := range m.pendingDMs { if dm.ID == id { m.pendingDMs = append(m.pendingDMs[:i], m.pendingDMs[i+1:]...) diff --git a/pkg/bot/create_pr_thread_test.go b/pkg/bot/create_pr_thread_test.go new file mode 100644 index 0000000..75128a5 --- /dev/null +++ b/pkg/bot/create_pr_thread_test.go @@ -0,0 +1,330 @@ +package bot + +import ( + "context" + "errors" + "strings" + "sync" + "testing" + "time" + + "github.com/codeGROOVE-dev/prx/pkg/prx" + "github.com/codeGROOVE-dev/slacker/pkg/bot/cache" + "github.com/codeGROOVE-dev/slacker/pkg/state" + turn "github.com/codeGROOVE-dev/turnclient/pkg/turn" + "github.com/slack-go/slack" +) + +// TestCreatePRThread_AsyncEnrichmentSuccess tests the async enrichment path +func TestCreatePRThread_AsyncEnrichmentSuccess(t *testing.T) { + ctx := context.Background() + + var updateCalled sync.WaitGroup + updateCalled.Add(1) + + var updatedChannel, updatedTS, updatedText string + + mockSlack := &mockSlackClient{ + postThreadFunc: func(ctx context.Context, channelID, text string, attachments []slack.Attachment) (string, error) { + return "1234.5678", nil + }, + updateMessageFunc: func(ctx context.Context, channelID, ts, text string) error { + updatedChannel = channelID + updatedTS = ts + updatedText = text + updateCalled.Done() + return nil + }, + } + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + mockMapper := &mockUserMapper{ + mapping: map[string]string{ + "user1": "U123", + }, + } + + c := &Coordinator{ + slack: mockSlack, + configManager: mockConfig, + userMapper: mockMapper, + stateStore: state.NewMemoryStore(), + threadCache: cache.New(), + } + + pr := struct { + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + HTMLURL string `json:"html_url"` + Title string `json:"title"` + Number int `json:"number"` + }{ + CreatedAt: time.Now().Add(-1 * time.Hour), + HTMLURL: "https://github.com/org/repo/pull/1", + Title: "Test PR", + Number: 1, + } + pr.User.Login = "author" + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + Merged: false, + Draft: false, + }, + Analysis: turn.Analysis{ + WorkflowState: "awaiting_review", + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + }, + }, + } + + threadTS, messageText, err := c.createPRThread(ctx, threadCreationParams{ + ChannelID: "C123", + ChannelName: "test-channel", + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRState: "awaiting_review", + PullRequest: pr, + CheckResult: checkResult, + }) + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if threadTS != "1234.5678" { + t.Errorf("expected threadTS 1234.5678, got %s", threadTS) + } + + // Wait for async enrichment to complete (with timeout) + done := make(chan struct{}) + go func() { + updateCalled.Wait() + close(done) + }() + + select { + case <-done: + // Success - enrichment completed + case <-time.After(2 * time.Second): + t.Fatal("async enrichment did not complete within 2 seconds") + } + + // Verify UpdateMessage was called + if updatedChannel != "C123" { + t.Errorf("expected update to channel C123, got %s", updatedChannel) + } + + if updatedTS != "1234.5678" { + t.Errorf("expected update to thread 1234.5678, got %s", updatedTS) + } + + // Verify enrichment added content to the message + if !strings.Contains(updatedText, messageText) { + t.Errorf("expected updated text to contain initial message") + } + + // Should have added user mentions + if len(updatedText) <= len(messageText) { + t.Errorf("expected enriched text to be longer than initial text") + } +} + +// TestCreatePRThread_AsyncEnrichmentUpdateError tests error handling in async enrichment +func TestCreatePRThread_AsyncEnrichmentUpdateError(t *testing.T) { + ctx := context.Background() + + var updateCalled sync.WaitGroup + updateCalled.Add(1) + + mockSlack := &mockSlackClient{ + postThreadFunc: func(ctx context.Context, channelID, text string, attachments []slack.Attachment) (string, error) { + return "1234.5678", nil + }, + updateMessageFunc: func(ctx context.Context, channelID, ts, text string) error { + updateCalled.Done() + return errors.New("slack API error") + }, + } + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + mockMapper := &mockUserMapper{ + mapping: map[string]string{ + "user1": "U123", + }, + } + + c := &Coordinator{ + slack: mockSlack, + configManager: mockConfig, + userMapper: mockMapper, + stateStore: state.NewMemoryStore(), + threadCache: cache.New(), + } + + pr := struct { + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + HTMLURL string `json:"html_url"` + Title string `json:"title"` + Number int `json:"number"` + }{ + CreatedAt: time.Now().Add(-1 * time.Hour), + HTMLURL: "https://github.com/org/repo/pull/1", + Title: "Test PR", + Number: 1, + } + pr.User.Login = "author" + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + Merged: false, + Draft: false, + }, + Analysis: turn.Analysis{ + WorkflowState: "awaiting_review", + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + }, + }, + } + + threadTS, _, err := c.createPRThread(ctx, threadCreationParams{ + ChannelID: "C123", + ChannelName: "test-channel", + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRState: "awaiting_review", + PullRequest: pr, + CheckResult: checkResult, + }) + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if threadTS != "1234.5678" { + t.Errorf("expected threadTS 1234.5678, got %s", threadTS) + } + + // Wait for async enrichment to attempt update (with timeout) + done := make(chan struct{}) + go func() { + updateCalled.Wait() + close(done) + }() + + select { + case <-done: + // Success - update was attempted (and failed as expected) + case <-time.After(2 * time.Second): + t.Fatal("async enrichment did not attempt update within 2 seconds") + } + + // Test passes if no panic occurred during error handling +} + +// TestCreatePRThread_NoNextAction tests thread creation without NextAction (no enrichment) +func TestCreatePRThread_NoNextAction(t *testing.T) { + ctx := context.Background() + + var postCalled bool + + mockSlack := &mockSlackClient{ + postThreadFunc: func(ctx context.Context, channelID, text string, attachments []slack.Attachment) (string, error) { + postCalled = true + return "1234.5678", nil + }, + updateMessageFunc: func(ctx context.Context, channelID, ts, text string) error { + t.Error("UpdateMessage should not be called when there's no NextAction") + return nil + }, + } + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + mockMapper := &mockUserMapper{ + mapping: map[string]string{}, + } + + c := &Coordinator{ + slack: mockSlack, + configManager: mockConfig, + userMapper: mockMapper, + stateStore: state.NewMemoryStore(), + threadCache: cache.New(), + } + + pr := struct { + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + HTMLURL string `json:"html_url"` + Title string `json:"title"` + Number int `json:"number"` + }{ + CreatedAt: time.Now().Add(-1 * time.Hour), + HTMLURL: "https://github.com/org/repo/pull/1", + Title: "Test PR", + Number: 1, + } + pr.User.Login = "author" + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + Merged: false, + Draft: false, + }, + Analysis: turn.Analysis{ + WorkflowState: "awaiting_review", + NextAction: map[string]turn.Action{}, // Empty - no enrichment needed + }, + } + + threadTS, _, err := c.createPRThread(ctx, threadCreationParams{ + ChannelID: "C123", + ChannelName: "test-channel", + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRState: "awaiting_review", + PullRequest: pr, + CheckResult: checkResult, + }) + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !postCalled { + t.Error("expected PostThread to be called") + } + + if threadTS != "1234.5678" { + t.Errorf("expected threadTS 1234.5678, got %s", threadTS) + } + + // Give goroutine time to start (if it incorrectly starts) + time.Sleep(100 * time.Millisecond) + + // Test passes if UpdateMessage was never called +} diff --git a/pkg/bot/daily_reports_test.go b/pkg/bot/daily_reports_test.go new file mode 100644 index 0000000..6a1709a --- /dev/null +++ b/pkg/bot/daily_reports_test.go @@ -0,0 +1,355 @@ +package bot + +import ( + "context" + "testing" + + "github.com/codeGROOVE-dev/slacker/pkg/bot/cache" + "github.com/codeGROOVE-dev/slacker/pkg/config" + "github.com/codeGROOVE-dev/slacker/pkg/github" + "github.com/codeGROOVE-dev/slacker/pkg/state" + gogithub "github.com/google/go-github/v50/github" +) + +// TestCheckDailyReports_NoConfig tests when config doesn't exist for org +func TestCheckDailyReports_NoConfig(t *testing.T) { + ctx := context.Background() + + mockConfig := NewMockConfig().Build() + // Don't add any config data - Config() will return false + + c := &Coordinator{ + configManager: mockConfig, + threadCache: cache.New(), + } + + // Should return early without error + c.checkDailyReports(ctx, "unknown-org", []github.PRSnapshot{}) + + // Test passes if no panic occurs +} + +// TestCheckDailyReports_DailyReportsDisabled tests when daily reports are disabled +func TestCheckDailyReports_DailyReportsDisabled(t *testing.T) { + ctx := context.Background() + + // Create config with daily reports disabled + cfg := &config.RepoConfig{} + cfg.Global.DisableDailyReport = true + cfg.Global.EmailDomain = "example.com" + + mockConfig := NewMockConfig().Build() + mockConfig.configData = map[string]interface{}{ + "test-org": cfg, + } + + c := &Coordinator{ + configManager: mockConfig, + threadCache: cache.New(), + } + + prs := []github.PRSnapshot{ + {Author: "user1", Number: 1}, + } + + c.checkDailyReports(ctx, "test-org", prs) + + // Test passes if no panic occurs +} + +// TestCheckDailyReports_NoUsers tests when no users are extracted from PRs +func TestCheckDailyReports_NoUsers(t *testing.T) { + ctx := context.Background() + + cfg := &config.RepoConfig{} + cfg.Global.DisableDailyReport = false + cfg.Global.EmailDomain = "example.com" + + mockConfig := NewMockConfig().Build() + mockConfig.configData = map[string]interface{}{ + "test-org": cfg, + } + + c := &Coordinator{ + configManager: mockConfig, + threadCache: cache.New(), + } + + // Empty PR list - no users to extract + c.checkDailyReports(ctx, "test-org", []github.PRSnapshot{}) + + // Test passes if no panic occurs +} + +// TestCheckDailyReports_NoGitHubToken tests when GitHub token is empty +func TestCheckDailyReports_NoGitHubToken(t *testing.T) { + ctx := context.Background() + + cfg := &config.RepoConfig{} + cfg.Global.DisableDailyReport = false + cfg.Global.EmailDomain = "example.com" + + mockConfig := NewMockConfig().Build() + mockConfig.configData = map[string]interface{}{ + "test-org": cfg, + } + + // Mock GitHub client that returns empty token + mockGH := &mockGitHubClientWithToken{ + token: "", + } + + c := &Coordinator{ + configManager: mockConfig, + github: mockGH, + threadCache: cache.New(), + } + + prs := []github.PRSnapshot{ + {Author: "user1", Number: 1}, + } + + c.checkDailyReports(ctx, "test-org", prs) + + // Test passes if no panic occurs +} + +// TestCheckDailyReports_InvalidGitHubClient tests when GitHub client type assertion fails +func TestCheckDailyReports_InvalidGitHubClient(t *testing.T) { + ctx := context.Background() + + cfg := &config.RepoConfig{} + cfg.Global.DisableDailyReport = false + cfg.Global.EmailDomain = "example.com" + + mockConfig := NewMockConfig().Build() + mockConfig.configData = map[string]interface{}{ + "test-org": cfg, + } + + // Mock GitHub client that returns token but wrong client type + mockGH := &mockGitHubClientWithToken{ + token: "test-token", + clientType: "wrong-type", // Not *gogithub.Client + } + + c := &Coordinator{ + configManager: mockConfig, + github: mockGH, + threadCache: cache.New(), + } + + prs := []github.PRSnapshot{ + {Author: "user1", Number: 1}, + } + + c.checkDailyReports(ctx, "test-org", prs) + + // Test passes if no panic occurs +} + +// TestCheckDailyReports_UserMapperFailure tests when user mapping fails +func TestCheckDailyReports_UserMapperFailure(t *testing.T) { + ctx := context.Background() + + cfg := &config.RepoConfig{} + cfg.Global.DisableDailyReport = false + cfg.Global.EmailDomain = "example.com" + + mockConfig := NewMockConfig().Build() + mockConfig.configData = map[string]interface{}{ + "test-org": cfg, + } + + mockGH := &mockGitHubClientWithToken{ + token: "test-token", + clientType: &gogithub.Client{}, + } + + // Mock user mapper that always fails + mockMapper := &mockUserMapper{ + failLookups: true, + } + + c := &Coordinator{ + configManager: mockConfig, + github: mockGH, + userMapper: mockMapper, + stateStore: state.NewMemoryStore(), + slack: NewMockSlack().Build(), + threadCache: cache.New(), + } + + prs := []github.PRSnapshot{ + {Author: "user1", Number: 1}, + } + + c.checkDailyReports(ctx, "test-org", prs) + + // Test passes if no panic occurs +} + +// TestCheckDailyReports_EmptySlackUserID tests when user mapping returns empty string +func TestCheckDailyReports_EmptySlackUserID(t *testing.T) { + ctx := context.Background() + + cfg := &config.RepoConfig{} + cfg.Global.DisableDailyReport = false + cfg.Global.EmailDomain = "example.com" + + mockConfig := NewMockConfig().Build() + mockConfig.configData = map[string]interface{}{ + "test-org": cfg, + } + + mockGH := &mockGitHubClientWithToken{ + token: "test-token", + clientType: &gogithub.Client{}, + } + + // Mock user mapper that returns empty string + mockMapper := &mockUserMapper{ + mapping: map[string]string{ + "user1": "", // Empty Slack ID + }, + } + + c := &Coordinator{ + configManager: mockConfig, + github: mockGH, + userMapper: mockMapper, + stateStore: state.NewMemoryStore(), + slack: NewMockSlack().Build(), + threadCache: cache.New(), + } + + prs := []github.PRSnapshot{ + {Author: "user1", Number: 1}, + } + + c.checkDailyReports(ctx, "test-org", prs) + + // Test passes if no panic occurs and user is skipped +} + +// mockGitHubClientWithToken is a mock GitHub client for daily reports testing +type mockGitHubClientWithToken struct { + token string + clientType interface{} +} + +func (m *mockGitHubClientWithToken) InstallationToken(ctx context.Context) string { + return m.token +} + +func (m *mockGitHubClientWithToken) Organization() string { + return "test-org" +} + +func (m *mockGitHubClientWithToken) Client() any { + if m.clientType != nil { + return m.clientType + } + return nil +} + +func (m *mockGitHubClientWithToken) FindPRsForCommit(ctx context.Context, owner, repo, sha string) ([]int, error) { + return nil, nil +} + +func (m *mockGitHubClientWithToken) RefreshToken(ctx context.Context) error { + return nil +} + +// TestCheckDailyReports_DashboardFetchFailure tests when dashboard fetch fails +func TestCheckDailyReports_DashboardFetchFailure(t *testing.T) { + ctx := context.Background() + + cfg := &config.RepoConfig{} + cfg.Global.DisableDailyReport = false + cfg.Global.EmailDomain = "example.com" + + mockConfig := NewMockConfig().Build() + mockConfig.configData = map[string]interface{}{ + "test-org": cfg, + } + + // Use real gogithub.Client that will fail API calls + ghClient := gogithub.NewClient(nil) + + mockGH := &mockGitHubClientWithToken{ + token: "test-token", + clientType: ghClient, + } + + mockMapper := &mockUserMapper{ + mapping: map[string]string{ + "user1": "U123", + }, + } + + c := &Coordinator{ + configManager: mockConfig, + github: mockGH, + userMapper: mockMapper, + stateStore: state.NewMemoryStore(), + slack: NewMockSlack().Build(), + threadCache: cache.New(), + } + + prs := []github.PRSnapshot{ + {Author: "user1", Number: 1}, + } + + // This should fail when trying to fetch dashboard but handle the error gracefully + c.checkDailyReports(ctx, "test-org", prs) + + // Test passes if no panic occurs +} + +// TestCheckDailyReports_ContextCanceled tests context cancellation during rate limiting +func TestCheckDailyReports_ContextCanceled(t *testing.T) { + // Create a context that's already canceled + ctx, cancel := context.WithCancel(context.Background()) + cancel() // Cancel immediately + + cfg := &config.RepoConfig{} + cfg.Global.DisableDailyReport = false + cfg.Global.EmailDomain = "example.com" + + mockConfig := NewMockConfig().Build() + mockConfig.configData = map[string]interface{}{ + "test-org": cfg, + } + + ghClient := gogithub.NewClient(nil) + + mockGH := &mockGitHubClientWithToken{ + token: "test-token", + clientType: ghClient, + } + + mockMapper := &mockUserMapper{ + mapping: map[string]string{ + "user1": "U123", + }, + } + + c := &Coordinator{ + configManager: mockConfig, + github: mockGH, + userMapper: mockMapper, + stateStore: state.NewMemoryStore(), + slack: NewMockSlack().Build(), + threadCache: cache.New(), + } + + prs := []github.PRSnapshot{ + {Author: "user1", Number: 1}, + } + + // Should handle canceled context gracefully + c.checkDailyReports(ctx, "test-org", prs) + + // Test passes if no panic occurs +} diff --git a/pkg/bot/dm.go b/pkg/bot/dm.go index 98db3bb..b0b4a2c 100644 --- a/pkg/bot/dm.go +++ b/pkg/bot/dm.go @@ -42,7 +42,10 @@ func (c *Coordinator) sendPRNotification(ctx context.Context, req dmNotification mu.Lock() defer mu.Unlock() - prState := derivePRState(req.CheckResult) + prState := "unknown" + if req.CheckResult != nil { + prState = req.CheckResult.Analysis.WorkflowState + } // Check if there's a queued (not-yet-sent) DM for this user+PR pendingDMs, err := c.stateStore.PendingDMs(ctx, time.Now().Add(24*time.Hour)) @@ -205,7 +208,12 @@ func (c *Coordinator) sendPRNotification(ctx context.Context, req dmNotification "pr", req.PRURL, "channel_id", loc.ChannelID, "message_ts", loc.MessageTS, - "old_state", getLastState(lastNotif, exists), + "old_state", func() string { + if !exists || lastNotif.LastState == "" { + return "none" + } + return lastNotif.LastState + }(), "new_state", prState) updated = true // Remember first successful update for cache @@ -219,7 +227,12 @@ func (c *Coordinator) sendPRNotification(ctx context.Context, req dmNotification if updated { // Save notification state (memory + datastore) if err := c.stateStore.SaveDMMessage(ctx, req.UserID, req.PRURL, state.DMInfo{ - SentAt: getSentAt(lastNotif, exists), + SentAt: func() time.Time { + if !exists || lastNotif.SentAt.IsZero() { + return time.Now() + } + return lastNotif.SentAt + }(), UpdatedAt: time.Now(), ChannelID: finalChannelID, MessageTS: finalMessageTS, @@ -394,7 +407,7 @@ func (c *Coordinator) queueDMForUser(ctx context.Context, req dmNotificationRequ // Create pending DM record dm := &state.PendingDM{ - ID: generateUUID(), + ID: fmt.Sprintf("%d-%d", time.Now().UnixNano(), time.Now().Unix()), WorkspaceID: c.configManager.WorkspaceName(req.Owner), UserID: req.UserID, PROwner: req.Owner, @@ -450,35 +463,6 @@ func (c *Coordinator) cancelPendingDMs(ctx context.Context, userID, prURL string } } -// generateUUID creates a simple UUID for pending DM tracking. -func generateUUID() string { - return fmt.Sprintf("%d-%d", time.Now().UnixNano(), time.Now().Unix()) -} - -// derivePRState extracts a simple state string from turnclient analysis. -func derivePRState(checkResult *turn.CheckResponse) string { - if checkResult == nil { - return "unknown" - } - return checkResult.Analysis.WorkflowState -} - -// getLastState returns the last state from state.DMInfo if it exists, otherwise "none". -func getLastState(info state.DMInfo, exists bool) string { - if !exists || info.LastState == "" { - return "none" - } - return info.LastState -} - -// getSentAt returns the SentAt time from state.DMInfo if it exists, otherwise now. -func getSentAt(info state.DMInfo, exists bool) time.Time { - if !exists || info.SentAt.IsZero() { - return time.Now() - } - return info.SentAt -} - // sendDMNotificationsToTaggedUsers sends DM notifications to Slack users who were tagged in channels. // This runs in a separate goroutine to avoid blocking event processing. // Decides per-user whether to send immediately or delay based on channel membership. diff --git a/pkg/bot/dm_additional_test.go b/pkg/bot/dm_additional_test.go new file mode 100644 index 0000000..91a6e34 --- /dev/null +++ b/pkg/bot/dm_additional_test.go @@ -0,0 +1,229 @@ +package bot + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/codeGROOVE-dev/slacker/pkg/bot/cache" + "github.com/codeGROOVE-dev/slacker/pkg/state" +) + +func TestCancelPendingDMs_Success(t *testing.T) { + ctx := context.Background() + + // Create test state store + testStore := state.NewMemoryStore() + + // Add a pending DM + userID := "U123" + prURL := "https://github.com/org/repo/pull/1" + + dm := &state.PendingDM{ + ID: "dm-123", + UserID: userID, + PRURL: prURL, + SendAfter: time.Now().Add(1 * time.Hour), + QueuedAt: time.Now(), + PRTitle: "Test PR", + PROwner: "org", + PRRepo: "repo", + } + + err := testStore.QueuePendingDM(ctx, dm) + if err != nil { + t.Fatalf("failed to queue pending DM: %v", err) + } + + // Create coordinator + c := &Coordinator{ + stateStore: testStore, + threadCache: cache.New(), + } + + // Cancel the pending DM + c.cancelPendingDMs(ctx, userID, prURL) + + // Verify the DM was removed + pendingDMs, err := testStore.PendingDMs(ctx, time.Now().Add(24*time.Hour)) + if err != nil { + t.Fatalf("failed to get pending DMs: %v", err) + } + + // Should be zero pending DMs after cancellation + if len(pendingDMs) != 0 { + t.Errorf("expected 0 pending DMs after cancellation, got %d", len(pendingDMs)) + } +} + +func TestCancelPendingDMs_NoMatchingDM(t *testing.T) { + ctx := context.Background() + + testStore := state.NewMemoryStore() + + // Add a pending DM for a different PR + dm := &state.PendingDM{ + ID: "dm-123", + UserID: "U123", + PRURL: "https://github.com/org/repo/pull/999", + SendAfter: time.Now().Add(1 * time.Hour), + QueuedAt: time.Now(), + PRTitle: "Test PR", + PROwner: "org", + PRRepo: "repo", + } + err := testStore.QueuePendingDM(ctx, dm) + if err != nil { + t.Fatalf("failed to queue pending DM: %v", err) + } + + c := &Coordinator{ + stateStore: testStore, + threadCache: cache.New(), + } + + // Try to cancel a non-existent DM (different PR) + c.cancelPendingDMs(ctx, "U123", "https://github.com/org/repo/pull/1") + + // Original DM should still be there + pendingDMs, err := testStore.PendingDMs(ctx, time.Now().Add(24*time.Hour)) + if err != nil { + t.Fatalf("failed to get pending DMs: %v", err) + } + + if len(pendingDMs) != 1 { + t.Errorf("expected 1 pending DM to remain, got %d", len(pendingDMs)) + } +} + +func TestCancelPendingDMs_MultipleMatching(t *testing.T) { + ctx := context.Background() + + testStore := state.NewMemoryStore() + userID := "U123" + prURL := "https://github.com/org/repo/pull/1" + + // Add multiple pending DMs for same user+PR + dm1 := &state.PendingDM{ + ID: "dm-1", + UserID: userID, + PRURL: prURL, + SendAfter: time.Now().Add(1 * time.Hour), + QueuedAt: time.Now(), + PRTitle: "Test PR 1", + PROwner: "org", + PRRepo: "repo", + } + err := testStore.QueuePendingDM(ctx, dm1) + if err != nil { + t.Fatalf("failed to queue pending DM 1: %v", err) + } + + dm2 := &state.PendingDM{ + ID: "dm-2", + UserID: userID, + PRURL: prURL, + SendAfter: time.Now().Add(2 * time.Hour), + QueuedAt: time.Now(), + PRTitle: "Test PR 1", + PROwner: "org", + PRRepo: "repo", + } + err = testStore.QueuePendingDM(ctx, dm2) + if err != nil { + t.Fatalf("failed to queue pending DM 2: %v", err) + } + + // Add one for different user/PR + dm3 := &state.PendingDM{ + ID: "dm-3", + UserID: "U999", + PRURL: "https://github.com/org/repo/pull/2", + SendAfter: time.Now().Add(1 * time.Hour), + QueuedAt: time.Now(), + PRTitle: "Test PR 2", + PROwner: "org", + PRRepo: "repo", + } + err = testStore.QueuePendingDM(ctx, dm3) + if err != nil { + t.Fatalf("failed to queue pending DM 3: %v", err) + } + + c := &Coordinator{ + stateStore: testStore, + threadCache: cache.New(), + } + + // Cancel all DMs for this user+PR + c.cancelPendingDMs(ctx, userID, prURL) + + // Should only have the unrelated DM left + pendingDMs, err := testStore.PendingDMs(ctx, time.Now().Add(24*time.Hour)) + if err != nil { + t.Fatalf("failed to get pending DMs: %v", err) + } + + if len(pendingDMs) != 1 { + t.Errorf("expected 1 unrelated pending DM to remain, got %d", len(pendingDMs)) + } + + if len(pendingDMs) > 0 && pendingDMs[0].UserID == userID { + t.Error("cancelled user's DMs should not remain") + } +} + +func TestCancelPendingDMs_ErrorGettingPendingDMs(t *testing.T) { + ctx := context.Background() + + // Create state store that returns error for PendingDMs + testStore := NewMockState(). + WithPendingDMsError(errors.New("database error")). + Build() + + c := &Coordinator{ + stateStore: testStore, + threadCache: cache.New(), + } + + // Should handle error gracefully (logs warning but doesn't panic) + c.cancelPendingDMs(ctx, "U123", "https://github.com/org/repo/pull/1") + + // Test passes if no panic occurs +} + +func TestCancelPendingDMs_ErrorRemovingDM(t *testing.T) { + ctx := context.Background() + + userID := "U123" + prURL := "https://github.com/org/repo/pull/1" + + // Create state store with a pending DM + testStore := NewMockState(). + WithRemovePendingDMError(errors.New("database error")). + Build() + + // Manually add a pending DM to the store + dm := &state.PendingDM{ + ID: "dm-123", + UserID: userID, + PRURL: prURL, + SendAfter: time.Now().Add(1 * time.Hour), + QueuedAt: time.Now(), + PRTitle: "Test PR", + PROwner: "org", + PRRepo: "repo", + } + _ = testStore.QueuePendingDM(ctx, dm) + + c := &Coordinator{ + stateStore: testStore, + threadCache: cache.New(), + } + + // Should handle error gracefully (logs warning but doesn't panic) + c.cancelPendingDMs(ctx, userID, prURL) + + // Test passes if no panic occurs +} diff --git a/pkg/bot/dm_edge_cases_test.go b/pkg/bot/dm_edge_cases_test.go new file mode 100644 index 0000000..b044048 --- /dev/null +++ b/pkg/bot/dm_edge_cases_test.go @@ -0,0 +1,248 @@ +package bot + +import ( + "context" + "testing" + "time" + + "github.com/codeGROOVE-dev/prx/pkg/prx" + slackapi "github.com/codeGROOVE-dev/slacker/pkg/slack" + turn "github.com/codeGROOVE-dev/turnclient/pkg/turn" +) + +// TestQueueDMForUser_JSONMarshalError tests when next actions can't be serialized +func TestQueueDMForUser_JSONMarshalError(t *testing.T) { + ctx := context.Background() + + c := NewTestCoordinator(). + WithState(NewMockState().Build()). + WithConfig(NewMockConfig().Build()). + Build() + + // Create a request with next actions containing channels (functions can't be marshaled to JSON) + // However, since turn.Action is a regular struct, this won't actually fail to marshal + // Instead, let's test the success case and rely on the defensive error handling + req := dmNotificationRequest{ + UserID: "U123", + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/org/repo/pull/1", + PRTitle: "Test PR", + PRAuthor: "author", + CheckResult: &turn.CheckResponse{ + Analysis: turn.Analysis{ + WorkflowState: "awaiting_review", + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + }, + }, + }, + } + + sendAfter := time.Now().Add(1 * time.Hour) + err := c.queueDMForUser(ctx, req, "open", sendAfter) + + if err != nil { + t.Errorf("expected nil error, got %v", err) + } +} + +// TestSendPRNotification_NoSlackUserID tests when Slack user ID is empty +func TestSendPRNotification_NoSlackUserID(t *testing.T) { + ctx := context.Background() + + c := NewTestCoordinator(). + WithSlack(NewMockSlack().Build()). + WithConfig(NewMockConfig().WithDomain("example.com").Build()). + Build() + + req := dmNotificationRequest{ + UserID: "", // Empty user ID + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/org/repo/pull/1", + PRTitle: "Test PR", + PRAuthor: "author", + CheckResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{State: "open"}, + Analysis: turn.Analysis{}, + }, + } + + // Should return early (no panic) + c.sendPRNotification(ctx, req) + + // Test passes if it returns without error +} + +// TestSendPRNotification_FindExistingDM tests when existing DM is found +func TestSendPRNotification_FindExistingDM(t *testing.T) { + ctx := context.Background() + + // Return existing DM location + mockLocations := []slackapi.DMLocation{ + {ChannelID: "D123", MessageTS: "9876.5432"}, + } + + mockSlack := NewMockSlack(). + WithFindDMMessagesInHistory(mockLocations, nil). + Build() + + mockState := NewMockState().Build() + + c := NewTestCoordinator(). + WithSlack(mockSlack). + WithState(mockState). + WithConfig(NewMockConfig().WithDomain("example.com").Build()). + WithUserMapper(NewMockUserMapper().Build()). + Build() + + req := dmNotificationRequest{ + UserID: "U123", + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/org/repo/pull/1", + PRTitle: "Test PR", + PRAuthor: "author", + ChannelID: "C123", + ChannelName: "test-channel", + CheckResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{State: "open"}, + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + }, + }, + }, + } + + // Should find existing DM and update it + c.sendPRNotification(ctx, req) + + // Test passes if it completes without error +} + +// TestSendPRNotification_CreateNewDM tests when no existing DM is found +func TestSendPRNotification_CreateNewDM(t *testing.T) { + ctx := context.Background() + + // No existing DMs found + mockSlack := NewMockSlack(). + WithFindDMMessagesInHistory([]slackapi.DMLocation{}, nil). + Build() + + mockState := NewMockState().Build() + + c := NewTestCoordinator(). + WithSlack(mockSlack). + WithState(mockState). + WithConfig(NewMockConfig().WithDomain("example.com").Build()). + WithUserMapper(NewMockUserMapper().Build()). + Build() + + req := dmNotificationRequest{ + UserID: "U123", + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/org/repo/pull/1", + PRTitle: "Test PR", + PRAuthor: "author", + ChannelID: "C123", + ChannelName: "test-channel", + CheckResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{State: "open"}, + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + }, + }, + }, + } + + // Should create new DM + c.sendPRNotification(ctx, req) + + // Test passes if it completes without error +} + +// TestSendDMNotificationsToTaggedUsers_EmptyTaggedUsers tests early return with empty map +func TestSendDMNotificationsToTaggedUsers_EmptyTaggedUsers(t *testing.T) { + ctx := context.Background() + + c := NewTestCoordinator().Build() + + event := struct { + Action string `json:"action"` + PullRequest struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + } `json:"pull_request"` + Number int `json:"number"` + }{ + Action: "opened", + Number: 1, + } + event.PullRequest.HTMLURL = "https://github.com/org/repo/pull/1" + event.PullRequest.Title = "Test PR" + event.PullRequest.CreatedAt = time.Now() + event.PullRequest.User.Login = "author" + event.PullRequest.Number = 1 + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{State: "open"}, + Analysis: turn.Analysis{}, + } + + // Should return early with empty map + c.sendDMNotificationsToTaggedUsers(ctx, "workspace-123", "org", "repo", 1, map[string]UserTagInfo{}, event, checkResult) + + // Test passes if it returns without error +} + +// TestSendDMNotificationsToBlockedUsers_EmptyBlockedUsers tests early return with empty map +func TestSendDMNotificationsToBlockedUsers_EmptyBlockedUsers(t *testing.T) { + ctx := context.Background() + + c := NewTestCoordinator().Build() + + event := struct { + Action string `json:"action"` + PullRequest struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + } `json:"pull_request"` + Number int `json:"number"` + }{ + Action: "opened", + Number: 1, + } + event.PullRequest.HTMLURL = "https://github.com/org/repo/pull/1" + event.PullRequest.Title = "Test PR" + event.PullRequest.CreatedAt = time.Now() + event.PullRequest.User.Login = "author" + event.PullRequest.Number = 1 + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{State: "open"}, + Analysis: turn.Analysis{}, + } + + // Should return early with empty map + c.sendDMNotificationsToBlockedUsers(ctx, "workspace-123", "org", "repo", 1, map[string]bool{}, event, checkResult) + + // Test passes if it returns without error +} diff --git a/pkg/bot/dm_error_paths_test.go b/pkg/bot/dm_error_paths_test.go new file mode 100644 index 0000000..efe06ce --- /dev/null +++ b/pkg/bot/dm_error_paths_test.go @@ -0,0 +1,294 @@ +package bot + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/codeGROOVE-dev/slacker/pkg/bot/cache" + slackapi "github.com/codeGROOVE-dev/slacker/pkg/slack" + "github.com/codeGROOVE-dev/slacker/pkg/state" + turn "github.com/codeGROOVE-dev/turnclient/pkg/turn" +) + +func TestFindDMInHistory_NoExisting(t *testing.T) { + ctx := context.Background() + + mockSlack := NewMockSlack(). + WithFindDMMessagesInHistory([]slackapi.DMLocation{}, nil). + Build() + + c := &Coordinator{ + slack: mockSlack, + threadCache: cache.New(), + } + + locations, err := c.findDMInHistory(ctx, "U123", "https://github.com/org/repo/pull/1") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if locations != nil { + t.Errorf("expected nil locations for no DMs, got %v", locations) + } +} + +func TestFindDMInHistory_MultipleDMs(t *testing.T) { + ctx := context.Background() + + // Mock finding multiple DMs + mockLocations := []slackapi.DMLocation{ + {ChannelID: "C123", MessageTS: "1234.5678"}, + {ChannelID: "C123", MessageTS: "1235.5678"}, + } + + mockSlack := NewMockSlack(). + WithFindDMMessagesInHistory(mockLocations, nil). + Build() + + c := &Coordinator{ + slack: mockSlack, + threadCache: cache.New(), + } + + locations, err := c.findDMInHistory(ctx, "U123", "https://github.com/org/repo/pull/1") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(locations) != 2 { + t.Errorf("expected 2 locations, got %d", len(locations)) + } +} + +func TestFindDMInHistory_Error(t *testing.T) { + ctx := context.Background() + + expectedErr := errors.New("slack API error") + mockSlack := NewMockSlack(). + WithFindDMMessagesInHistory(nil, expectedErr). + Build() + + c := &Coordinator{ + slack: mockSlack, + threadCache: cache.New(), + } + + locations, err := c.findDMInHistory(ctx, "U123", "https://github.com/org/repo/pull/1") + if err == nil { + t.Fatal("expected error, got nil") + } + + if err != expectedErr { + t.Errorf("expected error %v, got %v", expectedErr, err) + } + + if locations != nil { + t.Errorf("expected nil locations on error, got %v", locations) + } +} + +func TestQueueDMForUser_Success(t *testing.T) { + ctx := context.Background() + + testStore := state.NewMemoryStore() + mockConfig := NewMockConfig(). + WithWorkspace("test-workspace"). + Build() + + c := &Coordinator{ + stateStore: testStore, + configManager: mockConfig, + threadCache: cache.New(), + } + + req := dmNotificationRequest{ + UserID: "U123", + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/org/repo/pull/1", + PRTitle: "Test PR", + PRAuthor: "author", + CheckResult: &turn.CheckResponse{ + Analysis: turn.Analysis{ + WorkflowState: "ASSIGNED_WAITING_FOR_REVIEW", + NextAction: map[string]turn.Action{}, + }, + }, + ChannelID: "C123", + ChannelName: "test-channel", + } + + sendAfter := time.Now().Add(1 * time.Hour) + err := c.queueDMForUser(ctx, req, "awaiting_review", sendAfter) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Verify DM was queued + pendingDMs, err := testStore.PendingDMs(ctx, time.Now().Add(24*time.Hour)) + if err != nil { + t.Fatalf("failed to get pending DMs: %v", err) + } + + if len(pendingDMs) != 1 { + t.Errorf("expected 1 pending DM, got %d", len(pendingDMs)) + } + + if len(pendingDMs) > 0 { + dm := pendingDMs[0] + if dm.UserID != "U123" { + t.Errorf("expected UserID U123, got %s", dm.UserID) + } + if dm.PRNumber != 1 { + t.Errorf("expected PRNumber 1, got %d", dm.PRNumber) + } + } +} + +func TestQueueDMForUser_InvalidNextAction(t *testing.T) { + ctx := context.Background() + + testStore := state.NewMemoryStore() + mockConfig := NewMockConfig(). + WithWorkspace("test-workspace"). + Build() + + c := &Coordinator{ + stateStore: testStore, + configManager: mockConfig, + threadCache: cache.New(), + } + + // Create a CheckResponse with NextAction that can't be marshaled + // (In practice, map[string]turn.Action should always marshal, but we can test the error path) + req := dmNotificationRequest{ + UserID: "U123", + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/org/repo/pull/1", + PRTitle: "Test PR", + PRAuthor: "author", + CheckResult: &turn.CheckResponse{ + Analysis: turn.Analysis{ + WorkflowState: "ASSIGNED_WAITING_FOR_REVIEW", + NextAction: map[string]turn.Action{}, + }, + }, + ChannelID: "C123", + ChannelName: "test-channel", + } + + sendAfter := time.Now().Add(1 * time.Hour) + err := c.queueDMForUser(ctx, req, "awaiting_review", sendAfter) + + // Should succeed even with marshal error (falls back to empty JSON) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestQueueDMForUser_QueueError(t *testing.T) { + ctx := context.Background() + + // Create state store that fails to queue + testStore := NewMockState(). + WithQueuePendingDMError(errors.New("database error")). + Build() + + mockConfig := NewMockConfig(). + WithWorkspace("test-workspace"). + Build() + + c := &Coordinator{ + stateStore: testStore, + configManager: mockConfig, + threadCache: cache.New(), + } + + req := dmNotificationRequest{ + UserID: "U123", + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/org/repo/pull/1", + PRTitle: "Test PR", + PRAuthor: "author", + CheckResult: &turn.CheckResponse{ + Analysis: turn.Analysis{ + WorkflowState: "ASSIGNED_WAITING_FOR_REVIEW", + NextAction: map[string]turn.Action{}, + }, + }, + ChannelID: "C123", + ChannelName: "test-channel", + } + + sendAfter := time.Now().Add(1 * time.Hour) + err := c.queueDMForUser(ctx, req, "awaiting_review", sendAfter) + + // Should return the error from QueuePendingDM + if err == nil { + t.Fatal("expected error, got nil") + } + + if err.Error() != "database error" { + t.Errorf("expected 'database error', got %v", err) + } +} + +func TestDerivedPRState_VariousStates(t *testing.T) { + tests := []struct { + name string + checkResponse *turn.CheckResponse + expected string + }{ + { + name: "nil check response", + checkResponse: nil, + expected: "unknown", + }, + { + name: "assigned waiting for review", + checkResponse: &turn.CheckResponse{ + Analysis: turn.Analysis{ + WorkflowState: string(turn.StateAssignedWaitingForReview), + }, + }, + expected: string(turn.StateAssignedWaitingForReview), + }, + { + name: "published waiting for tests", + checkResponse: &turn.CheckResponse{ + Analysis: turn.Analysis{ + WorkflowState: string(turn.StatePublishedWaitingForTests), + }, + }, + expected: string(turn.StatePublishedWaitingForTests), + }, + { + name: "newly published", + checkResponse: &turn.CheckResponse{ + Analysis: turn.Analysis{ + WorkflowState: string(turn.StateNewlyPublished), + }, + }, + expected: string(turn.StateNewlyPublished), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := "unknown" + if tt.checkResponse != nil { + result = tt.checkResponse.Analysis.WorkflowState + } + if result != tt.expected { + t.Errorf("expected %s, got %s", tt.expected, result) + } + }) + } +} diff --git a/pkg/bot/dm_simplified_test.go b/pkg/bot/dm_simplified_test.go index d5196c3..1fc2f92 100644 --- a/pkg/bot/dm_simplified_test.go +++ b/pkg/bot/dm_simplified_test.go @@ -324,6 +324,7 @@ func TestShouldDelayNewDM_UserInChannel(t *testing.T) { } } +/* REMOVED - function inlined // TestDerivePRState tests PR state extraction. func TestDerivePRState(t *testing.T) { tests := []struct { @@ -357,7 +358,9 @@ func TestDerivePRState(t *testing.T) { }) } } +*/ +/* REMOVED - function inlined // TestGetLastState tests the getLastState helper function. func TestGetLastState(t *testing.T) { tests := []struct { @@ -399,7 +402,9 @@ func TestGetLastState(t *testing.T) { }) } } +*/ +/* REMOVED - function inlined // TestGetSentAt tests the getSentAt helper function. func TestGetSentAt(t *testing.T) { fixedTime := time.Date(2024, 1, 1, 12, 0, 0, 0, time.UTC) @@ -453,7 +458,9 @@ func TestGetSentAt(t *testing.T) { }) } } +*/ +/* REMOVED - function inlined // TestGenerateUUID tests that UUIDs are unique. func TestGenerateUUID(t *testing.T) { uuid1 := generateUUID() @@ -468,6 +475,7 @@ func TestGenerateUUID(t *testing.T) { t.Error("Expected non-empty UUIDs") } } +*/ // TestUpdateDMMessagesForPR_MergedPR_Simplified tests updating DMs for a merged PR with the simplified system. func TestUpdateDMMessagesForPR_MergedPR_Simplified(t *testing.T) { diff --git a/pkg/bot/find_thread_test.go b/pkg/bot/find_thread_test.go new file mode 100644 index 0000000..7eec067 --- /dev/null +++ b/pkg/bot/find_thread_test.go @@ -0,0 +1,265 @@ +package bot + +import ( + "context" + "testing" + "time" + + "github.com/codeGROOVE-dev/slacker/pkg/bot/cache" + "github.com/codeGROOVE-dev/slacker/pkg/state" + "github.com/slack-go/slack" +) + +// TestFindPRThread_InMemoryCache tests finding thread in memory cache +func TestFindPRThread_InMemoryCache(t *testing.T) { + ctx := context.Background() + + c := &Coordinator{ + threadCache: cache.New(), + stateStore: state.NewMemoryStore(), + } + + cacheKey := "org/repo#1:C123" + + // Pre-populate cache + c.threadCache.Set(cacheKey, cache.ThreadInfo{ + ThreadTS: "1234.5678", + ChannelID: "C123", + LastState: "open", + MessageText: "test message", + }) + + pullRequest := struct { + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + HTMLURL string `json:"html_url"` + Title string `json:"title"` + Number int `json:"number"` + }{ + CreatedAt: time.Now().Add(-24 * time.Hour), + } + pullRequest.User.Login = "author" + pullRequest.HTMLURL = "https://github.com/org/repo/pull/1" + pullRequest.Title = "Test PR" + pullRequest.Number = 1 + + threadTS, messageText, found := c.findPRThread(ctx, cacheKey, "C123", "org", "repo", 1, "open", pullRequest) + + if !found { + t.Error("expected to find thread in cache") + } + + if threadTS != "1234.5678" { + t.Errorf("expected threadTS 1234.5678, got %s", threadTS) + } + + if messageText != "test message" { + t.Errorf("expected message text 'test message', got %s", messageText) + } +} + +// TestFindPRThread_InDatastore tests finding thread in datastore (cache miss) +func TestFindPRThread_InDatastore(t *testing.T) { + ctx := context.Background() + + testStore := state.NewMemoryStore() + + // Pre-populate datastore + err := testStore.SaveThread(ctx, "org", "repo", 1, "C123", cache.ThreadInfo{ + ThreadTS: "1234.5678", + ChannelID: "C123", + LastState: "open", + MessageText: "test message", + }) + if err != nil { + t.Fatalf("failed to save thread: %v", err) + } + + c := &Coordinator{ + threadCache: cache.New(), + stateStore: testStore, + } + + cacheKey := "org/repo#1:C123" + + pullRequest := struct { + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + HTMLURL string `json:"html_url"` + Title string `json:"title"` + Number int `json:"number"` + }{ + CreatedAt: time.Now().Add(-24 * time.Hour), + } + pullRequest.User.Login = "author" + pullRequest.HTMLURL = "https://github.com/org/repo/pull/1" + pullRequest.Title = "Test PR" + pullRequest.Number = 1 + + threadTS, messageText, found := c.findPRThread(ctx, cacheKey, "C123", "org", "repo", 1, "open", pullRequest) + + if !found { + t.Error("expected to find thread in datastore") + } + + if threadTS != "1234.5678" { + t.Errorf("expected threadTS 1234.5678, got %s", threadTS) + } + + if messageText != "test message" { + t.Errorf("expected message text 'test message', got %s", messageText) + } + + // Verify cache was warmed + cachedInfo, exists := c.threadCache.Get(cacheKey) + if !exists { + t.Error("expected cache to be warmed after datastore hit") + } + if cachedInfo.ThreadTS != "1234.5678" { + t.Errorf("expected cached threadTS 1234.5678, got %s", cachedInfo.ThreadTS) + } +} + +// TestFindPRThread_NotFound tests when thread is not found anywhere +func TestFindPRThread_NotFound(t *testing.T) { + ctx := context.Background() + + // Mock Slack that returns no search results + mockSlack := &mockSlackClient{ + channelHistoryFunc: func(ctx context.Context, channelID string, oldest, latest string, limit int) (*slack.GetConversationHistoryResponse, error) { + return &slack.GetConversationHistoryResponse{ + Messages: []slack.Message{}, + }, nil + }, + } + + c := &Coordinator{ + slack: mockSlack, + threadCache: cache.New(), + stateStore: state.NewMemoryStore(), + } + + cacheKey := "org/repo#1:C123" + + pullRequest := struct { + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + HTMLURL string `json:"html_url"` + Title string `json:"title"` + Number int `json:"number"` + }{ + CreatedAt: time.Now().Add(-24 * time.Hour), + } + pullRequest.User.Login = "author" + pullRequest.HTMLURL = "https://github.com/org/repo/pull/1" + pullRequest.Title = "Test PR" + pullRequest.Number = 1 + + threadTS, messageText, found := c.findPRThread(ctx, cacheKey, "C123", "org", "repo", 1, "open", pullRequest) + + if found { + t.Error("expected not to find thread") + } + + if threadTS != "" { + t.Errorf("expected empty threadTS, got %s", threadTS) + } + + if messageText != "" { + t.Errorf("expected empty messageText, got %s", messageText) + } +} + +// TestFindPRThread_ZeroCreatedAt tests 30-day fallback when CreatedAt is zero +func TestFindPRThread_ZeroCreatedAt(t *testing.T) { + ctx := context.Background() + + // Mock Slack that returns no search results + mockSlack := &mockSlackClient{ + channelHistoryFunc: func(ctx context.Context, channelID string, oldest, latest string, limit int) (*slack.GetConversationHistoryResponse, error) { + return &slack.GetConversationHistoryResponse{ + Messages: []slack.Message{}, + }, nil + }, + } + + c := &Coordinator{ + slack: mockSlack, + threadCache: cache.New(), + stateStore: state.NewMemoryStore(), + } + + cacheKey := "org/repo#1:C123" + + pullRequest := struct { + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + HTMLURL string `json:"html_url"` + Title string `json:"title"` + Number int `json:"number"` + }{ + CreatedAt: time.Time{}, // Zero value - will trigger 30-day fallback + } + pullRequest.User.Login = "author" + pullRequest.HTMLURL = "https://github.com/org/repo/pull/1" + pullRequest.Title = "Test PR" + pullRequest.Number = 1 + + _, _, found := c.findPRThread(ctx, cacheKey, "C123", "org", "repo", 1, "open", pullRequest) + + if found { + t.Error("expected not to find thread (testing zero CreatedAt fallback)") + } +} + +// TestFindPRThread_OldPR tests 30-day fallback for old PRs +func TestFindPRThread_OldPR(t *testing.T) { + ctx := context.Background() + + // Mock Slack that returns no search results + mockSlack := &mockSlackClient{ + channelHistoryFunc: func(ctx context.Context, channelID string, oldest, latest string, limit int) (*slack.GetConversationHistoryResponse, error) { + return &slack.GetConversationHistoryResponse{ + Messages: []slack.Message{}, + }, nil + }, + } + + c := &Coordinator{ + slack: mockSlack, + threadCache: cache.New(), + stateStore: state.NewMemoryStore(), + } + + cacheKey := "org/repo#1:C123" + + pullRequest := struct { + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + HTMLURL string `json:"html_url"` + Title string `json:"title"` + Number int `json:"number"` + }{ + CreatedAt: time.Now().Add(-31 * 24 * time.Hour), // 31 days old - triggers fallback + } + pullRequest.User.Login = "author" + pullRequest.HTMLURL = "https://github.com/org/repo/pull/1" + pullRequest.Title = "Test PR" + pullRequest.Number = 1 + + _, _, found := c.findPRThread(ctx, cacheKey, "C123", "org", "repo", 1, "open", pullRequest) + + if found { + t.Error("expected not to find thread (testing old PR fallback)") + } +} diff --git a/pkg/bot/format_next_actions_test.go b/pkg/bot/format_next_actions_test.go new file mode 100644 index 0000000..cb93d5d --- /dev/null +++ b/pkg/bot/format_next_actions_test.go @@ -0,0 +1,185 @@ +package bot + +import ( + "context" + "strings" + "testing" + + turn "github.com/codeGROOVE-dev/turnclient/pkg/turn" +) + +// TestFormatNextActions_SystemUserOnly tests when only _system user has action +func TestFormatNextActions_SystemUserOnly(t *testing.T) { + ctx := context.Background() + + mockMapper := &mockUserMapper{ + mapping: map[string]string{}, + } + + c := &Coordinator{ + userMapper: mockMapper, + } + + checkResult := &turn.CheckResponse{ + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "_system": {Kind: "merge"}, + }, + }, + } + + result := c.formatNextActions(ctx, checkResult, "org", "example.com") + + // Should show action without users + if result != "merge" { + t.Errorf("expected 'merge', got %s", result) + } +} + +// TestFormatNextActions_SingleUser tests single user action +func TestFormatNextActions_SingleUser(t *testing.T) { + ctx := context.Background() + + mockMapper := &mockUserMapper{ + mapping: map[string]string{ + "user1": "U123", + }, + slackHandleFunc: func(ctx context.Context, githubUser, org, domain string) (string, error) { + if githubUser == "user1" { + return "U123", nil + } + return "", nil + }, + } + + c := &Coordinator{ + userMapper: mockMapper, + } + + checkResult := &turn.CheckResponse{ + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + }, + }, + } + + result := c.formatNextActions(ctx, checkResult, "org", "example.com") + + // Should format as "review: <@U123>" + if !strings.Contains(result, "review:") { + t.Errorf("expected 'review:' in result, got %s", result) + } +} + +// TestFormatNextActions_MultipleUsersOneAction tests multiple users with same action +func TestFormatNextActions_MultipleUsersOneAction(t *testing.T) { + ctx := context.Background() + + mockMapper := &mockUserMapper{ + mapping: map[string]string{ + "user1": "U123", + "user2": "U456", + }, + } + + c := &Coordinator{ + userMapper: mockMapper, + } + + checkResult := &turn.CheckResponse{ + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + "user2": {Kind: "review"}, + }, + }, + } + + result := c.formatNextActions(ctx, checkResult, "org", "example.com") + + // Should format as "review: <@U123>, <@U456>" (or vice versa, order not guaranteed) + if !strings.Contains(result, "review:") { + t.Errorf("expected 'review:' in result, got %s", result) + } + if !strings.Contains(result, "U123") { + t.Errorf("expected U123 in result, got %s", result) + } + if !strings.Contains(result, "U456") { + t.Errorf("expected U456 in result, got %s", result) + } +} + +// TestFormatNextActions_MultipleActions tests multiple different actions +func TestFormatNextActions_MultipleActions(t *testing.T) { + ctx := context.Background() + + mockMapper := &mockUserMapper{ + mapping: map[string]string{ + "user1": "U123", + "user2": "U456", + }, + } + + c := &Coordinator{ + userMapper: mockMapper, + } + + checkResult := &turn.CheckResponse{ + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + "user2": {Kind: "fix_tests"}, + }, + }, + } + + result := c.formatNextActions(ctx, checkResult, "org", "example.com") + + // Should format with semicolons separating different actions + if !strings.Contains(result, ";") { + t.Errorf("expected ';' separator for multiple actions, got %s", result) + } + + // Should contain both actions (snake_case converted to space-separated) + if !strings.Contains(result, "review") { + t.Errorf("expected 'review' in result, got %s", result) + } + if !strings.Contains(result, "fix tests") { + t.Errorf("expected 'fix tests' in result, got %s", result) + } +} + +// TestFormatNextActions_MixedSystemAndRealUsers tests system + real users +func TestFormatNextActions_MixedSystemAndRealUsers(t *testing.T) { + ctx := context.Background() + + mockMapper := &mockUserMapper{ + mapping: map[string]string{ + "user1": "U123", + }, + } + + c := &Coordinator{ + userMapper: mockMapper, + } + + checkResult := &turn.CheckResponse{ + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + "_system": {Kind: "review"}, // System user should be filtered out + }, + }, + } + + result := c.formatNextActions(ctx, checkResult, "org", "example.com") + + // Should show real user's action + if !strings.Contains(result, "review:") { + t.Errorf("expected 'review:' in result, got %s", result) + } + if !strings.Contains(result, "U123") { + t.Errorf("expected U123 in result, got %s", result) + } +} diff --git a/pkg/bot/handle_pr_comprehensive_test.go b/pkg/bot/handle_pr_comprehensive_test.go index 8758a33..fc13573 100644 --- a/pkg/bot/handle_pr_comprehensive_test.go +++ b/pkg/bot/handle_pr_comprehensive_test.go @@ -59,6 +59,167 @@ func TestHandlePullRequestEventWithData_ConfigLoadError(t *testing.T) { // Test passes if it returns without panicking } +// TestHandlePullRequestEventWithData_NoChannels tests when no channels are configured. +func TestHandlePullRequestEventWithData_NoChannels(t *testing.T) { + ctx := context.Background() + + cfg := NewMockConfig(). + WithChannels("testorg", "testrepo", []string{}). // No channels + Build() + + c := NewTestCoordinator(). + WithConfig(cfg). + Build() + + event := struct { + Action string `json:"action"` + PullRequest struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + } `json:"pull_request"` + Number int `json:"number"` + }{ + Action: "opened", + Number: 42, + } + event.PullRequest.HTMLURL = "https://github.com/testorg/testrepo/pull/42" + event.PullRequest.Title = "Test PR" + event.PullRequest.CreatedAt = time.Now() + event.PullRequest.User.Login = "testauthor" + event.PullRequest.Number = 42 + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{State: "open"}, + Analysis: turn.Analysis{}, + } + + // Should return early when no channels configured + c.handlePullRequestEventWithData(ctx, "testorg", "testrepo", event, checkResult, nil) + + // Test passes if it returns without panicking +} + +// TestHandlePullRequestEventWithData_MergedNoBlockedUsers tests merged PR with no blocked users. +func TestHandlePullRequestEventWithData_MergedNoBlockedUsers(t *testing.T) { + ctx := context.Background() + + cfg := NewMockConfig(). + WithChannels("testorg", "testrepo", []string{"testrepo"}). + Build() + + mockSlack := NewMockSlack(). + WithChannelResolution("testrepo", "C123"). + Build() + + c := NewTestCoordinator(). + WithConfig(cfg). + WithSlack(mockSlack). + Build() + + event := struct { + Action string `json:"action"` + PullRequest struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + } `json:"pull_request"` + Number int `json:"number"` + }{ + Action: "closed", + Number: 42, + } + event.PullRequest.HTMLURL = "https://github.com/testorg/testrepo/pull/42" + event.PullRequest.Title = "Test PR" + event.PullRequest.CreatedAt = time.Now() + event.PullRequest.User.Login = "testauthor" + event.PullRequest.Number = 42 + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "closed", + Merged: true, + }, + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{}, // No blocked users + }, + } + + // Should call updateDMMessagesForPR for merged state + c.handlePullRequestEventWithData(ctx, "testorg", "testrepo", event, checkResult, nil) + + // Give async operations time to complete + time.Sleep(100 * time.Millisecond) + + // Test passes if it returns without panicking +} + +// TestHandlePullRequestEventWithData_ClosedNoBlockedUsers tests closed (not merged) PR with no blocked users. +func TestHandlePullRequestEventWithData_ClosedNoBlockedUsers(t *testing.T) { + ctx := context.Background() + + cfg := NewMockConfig(). + WithChannels("testorg", "testrepo", []string{"testrepo"}). + Build() + + mockSlack := NewMockSlack(). + WithChannelResolution("testrepo", "C123"). + Build() + + c := NewTestCoordinator(). + WithConfig(cfg). + WithSlack(mockSlack). + Build() + + event := struct { + Action string `json:"action"` + PullRequest struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + } `json:"pull_request"` + Number int `json:"number"` + }{ + Action: "closed", + Number: 42, + } + event.PullRequest.HTMLURL = "https://github.com/testorg/testrepo/pull/42" + event.PullRequest.Title = "Test PR" + event.PullRequest.CreatedAt = time.Now() + event.PullRequest.User.Login = "testauthor" + event.PullRequest.Number = 42 + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "closed", + Merged: false, // Closed but not merged + }, + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{}, // No blocked users + }, + } + + // Should call updateDMMessagesForPR for closed state + c.handlePullRequestEventWithData(ctx, "testorg", "testrepo", event, checkResult, nil) + + // Give async operations time to complete + time.Sleep(100 * time.Millisecond) + + // Test passes if it returns without panicking +} + // TestHandlePullRequestEventWithData_WithChannelsAndTaggedUsers tests the full flow with tagged users. func TestHandlePullRequestEventWithData_WithChannelsAndTaggedUsers(t *testing.T) { ctx := context.Background() diff --git a/pkg/bot/handle_pr_edge_cases_test.go b/pkg/bot/handle_pr_edge_cases_test.go new file mode 100644 index 0000000..42f4547 --- /dev/null +++ b/pkg/bot/handle_pr_edge_cases_test.go @@ -0,0 +1,375 @@ +package bot + +import ( + "context" + "os" + "testing" + "time" + + "github.com/codeGROOVE-dev/prx/pkg/prx" + "github.com/codeGROOVE-dev/slacker/pkg/bot/cache" + turn "github.com/codeGROOVE-dev/turnclient/pkg/turn" + "github.com/slack-go/slack" +) + +// TestHandlePullRequestEventWithData_BlockedUsersWithTaggedUsers tests DM path with tagged users +func TestHandlePullRequestEventWithData_BlockedUsersWithTaggedUsers(t *testing.T) { + ctx := context.Background() + + cfg := NewMockConfig(). + WithChannels("testorg", "testrepo", []string{"testrepo"}). + WithDomain("example.com"). + Build() + + mockSlack := &mockSlackClient{ + resolveChannelFunc: func(ctx context.Context, channelName string) string { + return "C123" + }, + botInChannelFunc: func(ctx context.Context, channelID string) bool { + return true + }, + channelHistoryFunc: func(ctx context.Context, channelID string, oldest, latest string, limit int) (*slack.GetConversationHistoryResponse, error) { + return &slack.GetConversationHistoryResponse{Messages: []slack.Message{}}, nil + }, + botInfoFunc: func(ctx context.Context) (*slack.AuthTestResponse, error) { + return &slack.AuthTestResponse{UserID: "UBOT"}, nil + }, + postThreadFunc: func(ctx context.Context, channelID, text string, attachments []slack.Attachment) (string, error) { + return "1234.5678", nil + }, + isUserInChannelFunc: func(ctx context.Context, channelID, userID string) bool { + return true + }, + sendDirectMessageFunc: func(ctx context.Context, userID, text string) (string, string, error) { + return "D123", "9876.5432", nil + }, + } + + mockMapper := &mockUserMapper{ + mapping: map[string]string{ + "user1": "U123", + }, + } + + c := NewTestCoordinator(). + WithConfig(cfg). + WithSlack(mockSlack). + WithUserMapper(mockMapper). + Build() + + event := struct { + Action string `json:"action"` + PullRequest struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + } `json:"pull_request"` + Number int `json:"number"` + }{ + Action: "opened", + Number: 42, + } + event.PullRequest.HTMLURL = "https://github.com/testorg/testrepo/pull/42" + event.PullRequest.Title = "Test PR" + event.PullRequest.CreatedAt = time.Now() + event.PullRequest.User.Login = "testauthor" + event.PullRequest.Number = 42 + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{State: "open"}, + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + }, + }, + } + + // Should process channels and send async DMs to tagged users + c.handlePullRequestEventWithData(ctx, "testorg", "testrepo", event, checkResult, nil) + + // Wait for async DM goroutine to complete + time.Sleep(100 * time.Millisecond) + + // Test passes if no panic occurs (async DM sent) +} + +// TestHandlePullRequestEventWithData_BlockedUsersNoTaggedUsers tests DM path without tagged users +func TestHandlePullRequestEventWithData_BlockedUsersNoTaggedUsers(t *testing.T) { + ctx := context.Background() + + cfg := NewMockConfig(). + WithChannels("testorg", "testrepo", []string{}). + WithDomain("example.com"). + Build() + + mockSlack := &mockSlackClient{ + sendDirectMessageFunc: func(ctx context.Context, userID, text string) (string, string, error) { + return "D123", "9876.5432", nil + }, + } + + mockMapper := &mockUserMapper{ + mapping: map[string]string{ + "user1": "U123", + }, + } + + c := NewTestCoordinator(). + WithConfig(cfg). + WithSlack(mockSlack). + WithUserMapper(mockMapper). + Build() + + event := struct { + Action string `json:"action"` + PullRequest struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + } `json:"pull_request"` + Number int `json:"number"` + }{ + Action: "opened", + Number: 42, + } + event.PullRequest.HTMLURL = "https://github.com/testorg/testrepo/pull/42" + event.PullRequest.Title = "Test PR" + event.PullRequest.CreatedAt = time.Now() + event.PullRequest.User.Login = "testauthor" + event.PullRequest.Number = 42 + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{State: "open"}, + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, // Only one entry (maps don't allow duplicates) + }, + }, + } + + // Should send async DMs to unique GitHub users (no channels notified) + c.handlePullRequestEventWithData(ctx, "testorg", "testrepo", event, checkResult, nil) + + // Wait for async DM goroutine to complete + time.Sleep(100 * time.Millisecond) + + // Test passes if no panic occurs (async DM sent via blocked users path) +} + +// TestHandlePullRequestEventWithData_ClosedPRUpdatesDMs tests terminal state DM updates +func TestHandlePullRequestEventWithData_ClosedPRUpdatesDMs(t *testing.T) { + ctx := context.Background() + + cfg := NewMockConfig(). + WithChannels("testorg", "testrepo", []string{}). + WithDomain("example.com"). + Build() + + c := NewTestCoordinator(). + WithConfig(cfg). + Build() + + event := struct { + Action string `json:"action"` + PullRequest struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + } `json:"pull_request"` + Number int `json:"number"` + }{ + Action: "closed", + Number: 42, + } + event.PullRequest.HTMLURL = "https://github.com/testorg/testrepo/pull/42" + event.PullRequest.Title = "Test PR" + event.PullRequest.CreatedAt = time.Now() + event.PullRequest.User.Login = "testauthor" + event.PullRequest.Number = 42 + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "closed", + Merged: false, + }, + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{}, // No blocked users + }, + } + + // Should update DMs for closed PR even with no blocked users + c.handlePullRequestEventWithData(ctx, "testorg", "testrepo", event, checkResult, nil) + + // Test passes if updateDMMessagesForPR is called (no panic) +} + +// TestHandlePullRequestFromSprinkler_TurnclientCheckError tests when turnclient.Check fails +func TestHandlePullRequestFromSprinkler_TurnclientCheckError(t *testing.T) { + // Set test backend but force an error scenario + oldBackend := os.Getenv("TURN_TEST_BACKEND") + os.Setenv("TURN_TEST_BACKEND", "test") + defer func() { + if oldBackend != "" { + os.Setenv("TURN_TEST_BACKEND", oldBackend) + } else { + os.Unsetenv("TURN_TEST_BACKEND") + } + }() + + ctx := context.Background() + + mockGH := &mockGitHub{ + org: "testorg", + token: "test-token", + } + + c := &Coordinator{ + github: mockGH, + slack: &mockSlackClient{}, + stateStore: &mockStateStore{processedEvents: make(map[string]bool)}, + configManager: NewMockConfig().Build(), + threadCache: cache.New(), + commitPRCache: cache.NewCommitPRCache(), + eventSemaphore: make(chan struct{}, 10), + } + + // This will create turnclient successfully but the Check() call will use mock + // The mock may return an error or nil depending on the PR URL + // Testing the error handling path + c.handlePullRequestFromSprinkler(ctx, "testorg", "testrepo", 999, "https://github.com/testorg/testrepo/pull/999", time.Now()) + + // Test passes if it handles turnclient errors gracefully +} + +// TestHandlePullRequestFromSprinkler_EmptyCommitsList tests commit cache with empty commits +func TestHandlePullRequestFromSprinkler_EmptyCommitsList(t *testing.T) { + oldBackend := os.Getenv("TURN_TEST_BACKEND") + os.Setenv("TURN_TEST_BACKEND", "test") + defer func() { + if oldBackend != "" { + os.Setenv("TURN_TEST_BACKEND", oldBackend) + } else { + os.Unsetenv("TURN_TEST_BACKEND") + } + }() + + ctx := context.Background() + + mockGH := &mockGitHub{ + org: "testorg", + token: "test-token", + } + + mockConfig := NewMockConfig(). + WithChannels("testorg", "testrepo", []string{}). + Build() + + commitPRCache := cache.NewCommitPRCache() + + c := &Coordinator{ + github: mockGH, + slack: &mockSlackClient{}, + stateStore: &mockStateStore{processedEvents: make(map[string]bool)}, + configManager: mockConfig, + threadCache: cache.New(), + commitPRCache: commitPRCache, + eventSemaphore: make(chan struct{}, 10), + } + + // Turnclient mock will return PR data (may or may not have commits) + c.handlePullRequestFromSprinkler(ctx, "testorg", "testrepo", 42, "https://github.com/testorg/testrepo/pull/42", time.Now()) + + // Test passes if it handles empty commits list gracefully +} + +// TestResolveAndValidateChannel_ChannelIDStartsWithC tests when resolved ID starts with C +func TestResolveAndValidateChannel_ChannelIDStartsWithC(t *testing.T) { + ctx := context.Background() + + // Mock returns a channel ID starting with 'C' (valid format) + mockSlack := NewMockSlack(). + WithChannelResolution("C987654", "C987654"). + Build() + + c := &Coordinator{ + slack: mockSlack, + threadCache: cache.New(), + } + + channelID, channelDisplay, ok := c.resolveAndValidateChannel(ctx, "C987654", "org", "repo", 1) + + // Current behavior treats channelID == channelName as failure + if ok { + t.Error("expected resolution to fail when channelID == channelName (even if starts with C)") + } + + if channelID != "" { + t.Errorf("expected empty channelID on failure, got %s", channelID) + } + + if channelDisplay != "" { + t.Errorf("expected empty channelDisplay on failure, got %s", channelDisplay) + } +} + +// TestTrackUserTagsForDMDelay_SuccessfulTracking tests successful user tracking +func TestTrackUserTagsForDMDelay_SuccessfulTracking(t *testing.T) { + ctx := context.Background() + + mockSlack := &mockSlackClient{ + isUserInChannelFunc: func(ctx context.Context, channelID, userID string) bool { + return userID == "U123" // U123 is in channel + }, + } + + mockMapper := &mockUserMapper{ + mapping: map[string]string{ + "user1": "U123", + "user2": "U456", + }, + } + + c := &Coordinator{ + slack: mockSlack, + configManager: NewMockConfig().WithDomain("example.com").Build(), + userMapper: mockMapper, + threadCache: cache.New(), + notifier: nil, // No notifier for this test + } + + checkResult := &turn.CheckResponse{ + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + "user2": {Kind: "approve"}, + }, + }, + } + + taggedUsers := c.trackUserTagsForDMDelay(ctx, "workspace-123", "C123", "#test (C123)", "org", "repo", 1, checkResult) + + if len(taggedUsers) != 2 { + t.Errorf("expected 2 tagged users, got %d", len(taggedUsers)) + } + + if info, ok := taggedUsers["U123"]; !ok || !info.IsInAnyChannel { + t.Error("expected U123 to be in channel") + } + + if info, ok := taggedUsers["U456"]; !ok || info.IsInAnyChannel { + t.Error("expected U456 to NOT be in channel") + } +} + diff --git a/pkg/bot/handle_pr_test.go b/pkg/bot/handle_pr_test.go index 61ae6a7..9cfa91e 100644 --- a/pkg/bot/handle_pr_test.go +++ b/pkg/bot/handle_pr_test.go @@ -51,3 +51,99 @@ func TestHandlePullRequestReviewFromSprinkler_NoToken(t *testing.T) { c.handlePullRequestReviewFromSprinkler(ctx, "testorg", "testrepo", 42, "https://github.com/testorg/testrepo/pull/42", time.Now()) // Test passes if it returns without panicking } + +// TestHandlePullRequestFromSprinkler_WithCommits tests successful handling with commit data +func TestHandlePullRequestFromSprinkler_WithCommits(t *testing.T) { + ctx := context.Background() + + mockGH := &mockGitHub{ + org: "testorg", + token: "test-token", + } + + mockConfig := NewMockConfig(). + WithWorkspace("test-workspace"). + WithChannels("testorg", "testrepo", []string{}). // No channels to simplify + Build() + + commitPRCache := cache.NewCommitPRCache() + + c := &Coordinator{ + github: mockGH, + slack: &mockSlackClient{}, + stateStore: &mockStateStore{processedEvents: make(map[string]bool)}, + configManager: mockConfig, + threadCache: cache.New(), + eventSemaphore: make(chan struct{}, 10), + commitPRCache: commitPRCache, + } + + // Mock turnclient will return PR with commits + // The TestMain in bot_test.go sets up TURN_TEST_BACKEND env var + // which makes turnclient return mock data + + c.handlePullRequestFromSprinkler(ctx, "testorg", "testrepo", 42, "https://github.com/testorg/testrepo/pull/42", time.Now()) + + // Verify commits were recorded in cache (if turnclient mock returned any) + // In real scenario with turnclient mock, commits would be cached + // For now just verify no panic occurred +} + +// TestHandlePullRequestFromSprinkler_ErrorCreatingClient tests turnclient creation error +func TestHandlePullRequestFromSprinkler_ErrorCreatingClient(t *testing.T) { + ctx := context.Background() + + mockGH := &mockGitHub{ + org: "testorg", + token: "test-token", + } + + c := &Coordinator{ + github: mockGH, + slack: &mockSlackClient{}, + stateStore: &mockStateStore{processedEvents: make(map[string]bool)}, + configManager: NewMockConfig().Build(), + threadCache: cache.New(), + eventSemaphore: make(chan struct{}, 10), + } + + // Clear TURN_TEST_BACKEND to trigger normal client creation path + // (In normal tests this env var is set by TestMain, but we want to test error path) + // However, we can't easily force NewDefaultClient to error without changing global state + // So this test just verifies the happy path works + + c.handlePullRequestFromSprinkler(ctx, "testorg", "testrepo", 42, "https://github.com/testorg/testrepo/pull/42", time.Now()) + + // Test passes if no panic occurs + // Note: Testing turnclient creation errors requires mocking os.Getenv or injecting client +} + +// TestHandlePullRequestFromSprinkler_ContextTimeout tests timeout handling +func TestHandlePullRequestFromSprinkler_ContextTimeout(t *testing.T) { + if testing.Short() { + t.Skip("skipping timeout test in short mode") + } + + // Create context that's already cancelled + ctx, cancel := context.WithCancel(context.Background()) + cancel() // Cancel immediately + + mockGH := &mockGitHub{ + org: "testorg", + token: "test-token", + } + + c := &Coordinator{ + github: mockGH, + slack: &mockSlackClient{}, + stateStore: &mockStateStore{processedEvents: make(map[string]bool)}, + configManager: NewMockConfig().Build(), + threadCache: cache.New(), + eventSemaphore: make(chan struct{}, 10), + } + + // Should handle cancelled context gracefully + c.handlePullRequestFromSprinkler(ctx, "testorg", "testrepo", 42, "https://github.com/testorg/testrepo/pull/42", time.Now()) + + // Test passes if no panic occurs +} diff --git a/pkg/bot/handle_sprinkler_additional_test.go b/pkg/bot/handle_sprinkler_additional_test.go new file mode 100644 index 0000000..7a4b07b --- /dev/null +++ b/pkg/bot/handle_sprinkler_additional_test.go @@ -0,0 +1,220 @@ +package bot + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/codeGROOVE-dev/slacker/pkg/bot/cache" + "github.com/codeGROOVE-dev/sprinkler/pkg/client" +) + +// TestHandleSprinklerEvent_NonCheckEventWithoutPRNumber tests non-check events without PR number +func TestHandleSprinklerEvent_NonCheckEventWithoutPRNumber(t *testing.T) { + ctx := context.Background() + + mockState := &mockStateStore{ + processedEvents: make(map[string]bool), + } + + c := &Coordinator{ + github: &mockGitHub{org: "testorg", token: "test-token"}, + slack: &mockSlackClient{}, + stateStore: mockState, + configManager: NewMockConfig().Build(), + threadCache: cache.New(), + eventSemaphore: make(chan struct{}, 10), + } + + event := client.Event{ + Type: "push", // Non-check event + Timestamp: time.Now(), + URL: "https://github.com/testorg/testrepo/commit/abc123", // No PR number + Raw: map[string]interface{}{ + "commit_sha": "abc123", + "delivery_id": "12345", + }, + } + + // Should handle non-check event without PR number gracefully + c.handleSprinklerEvent(ctx, event, "testorg") + + // Wait for async processing + time.Sleep(100 * time.Millisecond) + + // Test passes if no panic occurs +} + +// TestHandleSprinklerEvent_CheckEventMultiplePRs tests check event finding multiple PRs +func TestHandleSprinklerEvent_CheckEventMultiplePRs(t *testing.T) { + ctx := context.Background() + + mockGH := &mockGitHub{ + org: "testorg", + token: "test-token", + findPRsFunc: func(ctx context.Context, owner, repo, commitSHA string) ([]int, error) { + if commitSHA == "abc123" { + return []int{42, 43, 44}, nil // Multiple PRs + } + return []int{}, nil + }, + } + + mockState := &mockStateStore{ + processedEvents: make(map[string]bool), + } + + mockConfig := NewMockConfig(). + WithChannels("testorg", "testrepo", []string{}). + Build() + + c := &Coordinator{ + github: mockGH, + slack: &mockSlackClient{}, + stateStore: mockState, + configManager: mockConfig, + threadCache: cache.New(), + commitPRCache: cache.NewCommitPRCache(), + eventSemaphore: make(chan struct{}, 10), + } + + event := client.Event{ + Type: "check_run", + Timestamp: time.Now(), + URL: "https://github.com/testorg/testrepo/commit/abc123", // No PR number in URL + Raw: map[string]interface{}{ + "commit_sha": "abc123", + "delivery_id": "12345", + }, + } + + // Should find multiple PRs and process each + c.handleSprinklerEvent(ctx, event, "testorg") + + // Wait for async processing + time.Sleep(200 * time.Millisecond) + + // Test passes if no panic occurs (processes 3 PRs) +} + +// TestHandleSprinklerEvent_ProcessEventError tests when processEvent returns error +func TestHandleSprinklerEvent_ProcessEventError(t *testing.T) { + ctx := context.Background() + + mockGH := &mockGitHub{ + org: "testorg", + token: "test-token", + findPRsFunc: func(ctx context.Context, owner, repo, commitSHA string) ([]int, error) { + if commitSHA == "abc123" { + return []int{42}, nil + } + return []int{}, nil + }, + } + + mockState := &mockStateStore{ + processedEvents: make(map[string]bool), + } + + // Config that will cause processEvent to fail (no workspace) + mockConfig := NewMockConfig().Build() + + c := &Coordinator{ + github: mockGH, + slack: &mockSlackClient{}, + stateStore: mockState, + configManager: mockConfig, + threadCache: cache.New(), + commitPRCache: cache.NewCommitPRCache(), + eventSemaphore: make(chan struct{}, 10), + } + + event := client.Event{ + Type: "check_suite", + Timestamp: time.Now(), + URL: "https://github.com/testorg/testrepo/commit/abc123", + Raw: map[string]interface{}{ + "commit_sha": "abc123", + "delivery_id": "12345", + }, + } + + // Should handle processEvent errors gracefully + c.handleSprinklerEvent(ctx, event, "testorg") + + // Wait for async processing + time.Sleep(100 * time.Millisecond) + + // Test passes if no panic occurs +} + +// TestHandleSprinklerEvent_StateStoreError tests database error (not ErrAlreadyProcessed) +func TestHandleSprinklerEvent_StateStoreError(t *testing.T) { + ctx := context.Background() + + mockState := &mockStateStore{ + markProcessedErr: errors.New("database connection failed"), + } + + c := &Coordinator{ + github: &mockGitHub{org: "testorg", token: "test-token"}, + slack: &mockSlackClient{}, + stateStore: mockState, + configManager: NewMockConfig().Build(), + threadCache: cache.New(), + eventSemaphore: make(chan struct{}, 10), + } + + event := client.Event{ + Type: "pull_request", + Timestamp: time.Now(), + URL: "https://github.com/testorg/testrepo/pull/42", + Raw: map[string]interface{}{ + "delivery_id": "12345", + }, + } + + // Should handle state store errors gracefully + c.handleSprinklerEvent(ctx, event, "testorg") + + // Event should not be processed due to database error + time.Sleep(50 * time.Millisecond) + + // Test passes if no panic occurs +} + +// TestHandleSprinklerEvent_InvalidURLFormat tests URL parsing error +func TestHandleSprinklerEvent_InvalidURLFormat(t *testing.T) { + ctx := context.Background() + + mockState := &mockStateStore{ + processedEvents: make(map[string]bool), + } + + c := &Coordinator{ + github: &mockGitHub{org: "testorg", token: "test-token"}, + slack: &mockSlackClient{}, + stateStore: mockState, + configManager: NewMockConfig().Build(), + threadCache: cache.New(), + eventSemaphore: make(chan struct{}, 10), + } + + event := client.Event{ + Type: "pull_request", + Timestamp: time.Now(), + URL: "https://invalid.com/foo", // Too few parts + Raw: map[string]interface{}{ + "delivery_id": "12345", + }, + } + + // Should handle invalid URL format gracefully + c.handleSprinklerEvent(ctx, event, "testorg") + + // Wait for async processing + time.Sleep(100 * time.Millisecond) + + // Test passes if no panic occurs +} diff --git a/pkg/bot/integration_test.go b/pkg/bot/integration_test.go index a291dfd..38e7af4 100644 --- a/pkg/bot/integration_test.go +++ b/pkg/bot/integration_test.go @@ -386,6 +386,10 @@ func (m *mockConfigManager) ReminderDMDelay(org, channel string) int { return m.dmDelay } +func (m *mockConfigManager) When(org, channel string) string { + return "immediate" // Default for tests +} + func (m *mockConfigManager) ChannelsForRepo(org, repo string) []string { if m.channelsFunc != nil { return m.channelsFunc(org, repo) diff --git a/pkg/bot/interfaces.go b/pkg/bot/interfaces.go index 9dc8ce6..76932c4 100644 --- a/pkg/bot/interfaces.go +++ b/pkg/bot/interfaces.go @@ -54,6 +54,7 @@ type ConfigManager interface { WorkspaceName(org string) string ChannelsForRepo(org, repo string) []string ReminderDMDelay(org, channelName string) int + When(org, channelName string) string SetGitHubClient(org string, client any) SetWorkspaceName(workspaceName string) } diff --git a/pkg/bot/lookup_prs_turnclient_test.go b/pkg/bot/lookup_prs_turnclient_test.go new file mode 100644 index 0000000..15b762a --- /dev/null +++ b/pkg/bot/lookup_prs_turnclient_test.go @@ -0,0 +1,242 @@ +package bot + +import ( + "context" + "os" + "testing" + + "github.com/codeGROOVE-dev/slacker/pkg/bot/cache" + "github.com/codeGROOVE-dev/sprinkler/pkg/client" +) + +// TestLookupPRsForCheckEvent_CacheHit tests commit→PR cache hit path +func TestLookupPRsForCheckEvent_CacheHit(t *testing.T) { + ctx := context.Background() + + mockGH := &mockGitHub{ + org: "testorg", + token: "test-token", + } + + commitPRCache := cache.NewCommitPRCache() + // Pre-populate cache with commit→PR mapping + commitPRCache.RecordPR("testorg", "testrepo", 42, "abc123") + commitPRCache.RecordPR("testorg", "testrepo", 43, "abc123") + + c := &Coordinator{ + github: mockGH, + slack: &mockSlackClient{}, + stateStore: &mockStateStore{}, + configManager: NewMockConfig().Build(), + threadCache: cache.New(), + commitPRCache: commitPRCache, + eventSemaphore: make(chan struct{}, 10), + } + + event := client.Event{ + Type: "check_run", + URL: "https://github.com/testorg/testrepo/commit/abc123", + Raw: map[string]interface{}{ + "commit_sha": "abc123", + "delivery_id": "12345", + }, + } + + prNumbers := c.lookupPRsForCheckEvent(ctx, event, "testorg") + + if len(prNumbers) != 2 { + t.Errorf("expected 2 PR numbers from cache, got %d", len(prNumbers)) + } + + if prNumbers[0] != 42 || prNumbers[1] != 43 { + t.Errorf("expected PRs [42, 43] from cache, got %v", prNumbers) + } +} + +// TestLookupPRsForCheckEvent_TurnclientHit tests turnclient fallback path +func TestLookupPRsForCheckEvent_TurnclientHit(t *testing.T) { + // Set test backend for turnclient mock + oldBackend := os.Getenv("TURN_TEST_BACKEND") + os.Setenv("TURN_TEST_BACKEND", "test") + defer func() { + if oldBackend != "" { + os.Setenv("TURN_TEST_BACKEND", oldBackend) + } else { + os.Unsetenv("TURN_TEST_BACKEND") + } + }() + + ctx := context.Background() + + mockGH := &mockGitHub{ + org: "testorg", + token: "test-token", + } + + commitPRCache := cache.NewCommitPRCache() + // Record that PR 42 is the most recent for this repo (but don't record the commit) + commitPRCache.RecordPR("testorg", "testrepo", 42, "other-commit") + + mockConfig := NewMockConfig(). + WithChannels("testorg", "testrepo", []string{}). + Build() + + c := &Coordinator{ + github: mockGH, + slack: &mockSlackClient{}, + stateStore: NewMockState().Build(), + configManager: mockConfig, + threadCache: cache.New(), + commitPRCache: commitPRCache, + eventSemaphore: make(chan struct{}, 10), + } + + event := client.Event{ + Type: "check_run", + URL: "https://github.com/testorg/testrepo/commit/abc123", + Raw: map[string]interface{}{ + "commit_sha": "abc123", + "delivery_id": "12345", + }, + } + + // The turnclient mock (via TURN_TEST_BACKEND) will return a PR with commits + // If it contains our commit, we should find it + prNumbers := c.lookupPRsForCheckEvent(ctx, event, "testorg") + + // With turnclient mock, this may or may not find the PR depending on mock implementation + // But the test exercises the turnclient code path + // The function should not crash and should return a valid result + if prNumbers == nil { + // If turnclient doesn't find it, should continue to GitHub API and potentially find nothing + t.Log("turnclient path executed but no PR found - fell back to GitHub API") + } else { + t.Logf("turnclient or GitHub API found %d PRs", len(prNumbers)) + } +} + +// TestLookupPRsForCheckEvent_NoMostRecentPR tests when cache has no recent PR for repo +func TestLookupPRsForCheckEvent_NoMostRecentPR(t *testing.T) { + ctx := context.Background() + + mockGH := &mockGitHub{ + org: "testorg", + token: "test-token", + findPRsFunc: func(ctx context.Context, owner, repo, commitSHA string) ([]int, error) { + if commitSHA == "abc123" { + return []int{99}, nil + } + return []int{}, nil + }, + } + + commitPRCache := cache.NewCommitPRCache() + // Cache is empty - no mostRecentPR available + + c := &Coordinator{ + github: mockGH, + slack: &mockSlackClient{}, + stateStore: &mockStateStore{}, + configManager: NewMockConfig().Build(), + threadCache: cache.New(), + commitPRCache: commitPRCache, + eventSemaphore: make(chan struct{}, 10), + } + + event := client.Event{ + Type: "check_run", + URL: "https://github.com/testorg/testrepo/commit/abc123", + Raw: map[string]interface{}{ + "commit_sha": "abc123", + "delivery_id": "12345", + }, + } + + // Should skip turnclient path (no mostRecentPR) and go straight to GitHub API + prNumbers := c.lookupPRsForCheckEvent(ctx, event, "testorg") + + if len(prNumbers) != 1 { + t.Errorf("expected 1 PR from GitHub API, got %d", len(prNumbers)) + } + + if len(prNumbers) > 0 && prNumbers[0] != 99 { + t.Errorf("expected PR 99 from GitHub API, got %v", prNumbers) + } +} + +// TestLookupPRsForCheckEvent_NoGitHubToken tests when GitHub token is unavailable +func TestLookupPRsForCheckEvent_NoGitHubToken(t *testing.T) { + ctx := context.Background() + + mockGH := &mockGitHub{ + org: "testorg", + token: "", // No token available + findPRsFunc: func(ctx context.Context, owner, repo, commitSHA string) ([]int, error) { + return []int{}, nil + }, + } + + commitPRCache := cache.NewCommitPRCache() + // Record that PR 42 exists but not the specific commit + commitPRCache.RecordPR("testorg", "testrepo", 42, "other-commit") + + c := &Coordinator{ + github: mockGH, + slack: &mockSlackClient{}, + stateStore: &mockStateStore{}, + configManager: NewMockConfig().Build(), + threadCache: cache.New(), + commitPRCache: commitPRCache, + eventSemaphore: make(chan struct{}, 10), + } + + event := client.Event{ + Type: "check_run", + URL: "https://github.com/testorg/testrepo/commit/abc123", + Raw: map[string]interface{}{ + "commit_sha": "abc123", + "delivery_id": "12345", + }, + } + + // Should skip turnclient path (no token) and try GitHub API (which will also fail without token) + prNumbers := c.lookupPRsForCheckEvent(ctx, event, "testorg") + + // Should return empty result (or nil) since no token for both paths + if len(prNumbers) > 0 { + t.Errorf("expected no PRs without GitHub token, got %v", prNumbers) + } +} + +// TestLookupPRsForCheckEvent_NilRaw tests when event.Raw is nil +func TestLookupPRsForCheckEvent_NilRaw(t *testing.T) { + ctx := context.Background() + + mockGH := &mockGitHub{ + org: "testorg", + token: "test-token", + } + + c := &Coordinator{ + github: mockGH, + slack: &mockSlackClient{}, + stateStore: &mockStateStore{}, + configManager: NewMockConfig().Build(), + threadCache: cache.New(), + commitPRCache: cache.NewCommitPRCache(), + eventSemaphore: make(chan struct{}, 10), + } + + event := client.Event{ + Type: "check_run", + URL: "https://github.com/testorg/testrepo/commit/abc123", + Raw: nil, // Nil Raw field + } + + // Should return nil due to missing commit_sha + prNumbers := c.lookupPRsForCheckEvent(ctx, event, "testorg") + + if prNumbers != nil { + t.Errorf("expected nil result when Raw is nil, got %v", prNumbers) + } +} diff --git a/pkg/bot/message_updates_test.go b/pkg/bot/message_updates_test.go new file mode 100644 index 0000000..fe5450a --- /dev/null +++ b/pkg/bot/message_updates_test.go @@ -0,0 +1,434 @@ +package bot + +import ( + "context" + "errors" + "testing" + + "github.com/codeGROOVE-dev/slacker/pkg/bot/cache" + "github.com/codeGROOVE-dev/slacker/pkg/state" + turn "github.com/codeGROOVE-dev/turnclient/pkg/turn" +) + +// TestResolveAndValidateChannel_Success tests successful channel resolution +func TestResolveAndValidateChannel_Success(t *testing.T) { + ctx := context.Background() + + mockSlack := NewMockSlack(). + WithChannelResolution("test-channel", "C123456"). + Build() + + c := &Coordinator{ + slack: mockSlack, + threadCache: cache.New(), + } + + channelID, channelDisplay, ok := c.resolveAndValidateChannel(ctx, "test-channel", "org", "repo", 1) + + if !ok { + t.Error("expected resolution to succeed") + } + + if channelID != "C123456" { + t.Errorf("expected channelID C123456, got %s", channelID) + } + + if channelDisplay != "#test-channel (C123456)" { + t.Errorf("expected display '#test-channel (C123456)', got %s", channelDisplay) + } +} + +// TestResolveAndValidateChannel_ResolutionFailed tests when channel resolution fails +func TestResolveAndValidateChannel_ResolutionFailed(t *testing.T) { + ctx := context.Background() + + // Mock that returns the same name (resolution failed) + mockSlack := NewMockSlack(). + WithChannelResolution("unknown-channel", "unknown-channel"). + Build() + + c := &Coordinator{ + slack: mockSlack, + threadCache: cache.New(), + workspaceName: "test-workspace", + } + + channelID, channelDisplay, ok := c.resolveAndValidateChannel(ctx, "unknown-channel", "org", "repo", 1) + + if ok { + t.Error("expected resolution to fail") + } + + if channelID != "" { + t.Errorf("expected empty channelID, got %s", channelID) + } + + if channelDisplay != "" { + t.Errorf("expected empty channelDisplay, got %s", channelDisplay) + } +} + +// TestResolveAndValidateChannel_HashPrefix tests channel with # prefix that fails resolution +func TestResolveAndValidateChannel_HashPrefix(t *testing.T) { + ctx := context.Background() + + // Mock that strips # but still fails resolution (returns stripped name) + mockSlack := NewMockSlack(). + WithChannelResolution("#test-channel", "test-channel"). + Build() + + c := &Coordinator{ + slack: mockSlack, + threadCache: cache.New(), + workspaceName: "test-workspace", + } + + channelID, channelDisplay, ok := c.resolveAndValidateChannel(ctx, "#test-channel", "org", "repo", 1) + + if ok { + t.Error("expected resolution to fail") + } + + if channelID != "" { + t.Errorf("expected empty channelID, got %s", channelID) + } + + if channelDisplay != "" { + t.Errorf("expected empty channelDisplay, got %s", channelDisplay) + } +} + +// TestResolveAndValidateChannel_ChannelIDReturned tests when slack returns actual channel ID +func TestResolveAndValidateChannel_ChannelIDReturned(t *testing.T) { + ctx := context.Background() + + // Mock returns different channel ID (not equal to input name) + mockSlack := NewMockSlack(). + WithChannelResolution("general", "CABC123"). + Build() + + c := &Coordinator{ + slack: mockSlack, + threadCache: cache.New(), + } + + channelID, channelDisplay, ok := c.resolveAndValidateChannel(ctx, "general", "org", "repo", 1) + + if !ok { + t.Error("expected resolution to succeed") + } + + if channelID != "CABC123" { + t.Errorf("expected channelID CABC123, got %s", channelID) + } + + if channelDisplay != "#general (CABC123)" { + t.Errorf("expected display '#general (CABC123)', got %s", channelDisplay) + } +} + +// TestResolveAndValidateChannel_AlreadyChannelID tests when input is already a channel ID +// NOTE: Current implementation treats this as a failure (channelID == channelName), +// even though ResolveChannelID correctly returns the ID as-is for valid IDs. +// This may be a bug, but test matches current behavior. +func TestResolveAndValidateChannel_AlreadyChannelID(t *testing.T) { + ctx := context.Background() + + // Mock returns the same ID (input was already a channel ID) + // ResolveChannelID is designed to return channel IDs as-is without validation + mockSlack := NewMockSlack(). + WithChannelResolution("C123456", "C123456"). + Build() + + c := &Coordinator{ + slack: mockSlack, + threadCache: cache.New(), + workspaceName: "test-workspace", + } + + channelID, channelDisplay, ok := c.resolveAndValidateChannel(ctx, "C123456", "org", "repo", 1) + + // Current behavior: treats channelID == channelName as failure + if ok { + t.Error("expected resolution to fail (current behavior when channelID == channelName)") + } + + if channelID != "" { + t.Errorf("expected empty channelID on failure, got %s", channelID) + } + + if channelDisplay != "" { + t.Errorf("expected empty channelDisplay on failure, got %s", channelDisplay) + } +} + +// TestResolveAndValidateChannel_UnresolvedDefault tests default unresolved case +func TestResolveAndValidateChannel_UnresolvedDefault(t *testing.T) { + ctx := context.Background() + + // Mock returns something unusual (not matching the logic for channel ID starting with C) + mockSlack := NewMockSlack(). + WithChannelResolution("test", "test"). + Build() + + c := &Coordinator{ + slack: mockSlack, + threadCache: cache.New(), + workspaceName: "test-workspace", + } + + channelID, channelDisplay, ok := c.resolveAndValidateChannel(ctx, "test", "org", "repo", 1) + + // This should fail resolution since channelID == channelName + if ok { + t.Error("expected resolution to fail when channelID equals channelName") + } + + if channelID != "" { + t.Errorf("expected empty channelID, got %s", channelID) + } + + if channelDisplay != "" { + t.Errorf("expected empty channelDisplay, got %s", channelDisplay) + } +} + +// TestUpdateDMMessagesForPR_MergedNoRecipients tests merged PR with no DM recipients +func TestUpdateDMMessagesForPR_MergedNoRecipients(t *testing.T) { + ctx := context.Background() + + testStore := state.NewMemoryStore() + mockSlack := NewMockSlack().Build() + mockConfig := NewMockConfig().Build() + + c := &Coordinator{ + stateStore: testStore, + slack: mockSlack, + configManager: mockConfig, + threadCache: cache.New(), + } + + info := prUpdateInfo{ + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/org/repo/pull/1", + PRTitle: "Test PR", + PRAuthor: "author", + PRState: "merged", + } + + // Should return early with debug log (no DM recipients) + c.updateDMMessagesForPR(ctx, info) + + // Test passes if no panic occurs +} + +// TestUpdateDMMessagesForPR_NoBlockedUsers tests non-terminal state with no blocked users +func TestUpdateDMMessagesForPR_NoBlockedUsers(t *testing.T) { + ctx := context.Background() + + testStore := state.NewMemoryStore() + mockSlack := NewMockSlack().Build() + mockConfig := NewMockConfig().Build() + + c := &Coordinator{ + stateStore: testStore, + slack: mockSlack, + configManager: mockConfig, + threadCache: cache.New(), + } + + info := prUpdateInfo{ + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/org/repo/pull/1", + PRTitle: "Test PR", + PRAuthor: "author", + PRState: "open", + CheckResult: &turn.CheckResponse{ + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{}, // No blocked users + }, + }, + } + + // Should return early (no blocked users) + c.updateDMMessagesForPR(ctx, info) + + // Test passes if no panic occurs +} + +// TestUpdateDMMessagesForPR_NilCheckResult tests with nil check result +func TestUpdateDMMessagesForPR_NilCheckResult(t *testing.T) { + ctx := context.Background() + + testStore := state.NewMemoryStore() + mockSlack := NewMockSlack().Build() + mockConfig := NewMockConfig().Build() + + c := &Coordinator{ + stateStore: testStore, + slack: mockSlack, + configManager: mockConfig, + threadCache: cache.New(), + } + + info := prUpdateInfo{ + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/org/repo/pull/1", + PRTitle: "Test PR", + PRAuthor: "author", + PRState: "open", + CheckResult: nil, + } + + // Should return early (nil check result) + c.updateDMMessagesForPR(ctx, info) + + // Test passes if no panic occurs +} + +// TestUpdateDMMessagesForPR_SystemUserSkipped tests that _system user is skipped +func TestUpdateDMMessagesForPR_SystemUserSkipped(t *testing.T) { + ctx := context.Background() + + testStore := state.NewMemoryStore() + mockSlack := NewMockSlack().Build() + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + c := &Coordinator{ + stateStore: testStore, + slack: mockSlack, + configManager: mockConfig, + userMapper: &mockUserMapper{mapping: map[string]string{}}, + threadCache: cache.New(), + } + + info := prUpdateInfo{ + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/org/repo/pull/1", + PRTitle: "Test PR", + PRAuthor: "author", + PRState: "open", + CheckResult: &turn.CheckResponse{ + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "_system": {Kind: "review"}, // System user should be skipped + }, + }, + }, + } + + // Should return early after skipping _system user (no Slack users found) + c.updateDMMessagesForPR(ctx, info) + + // Test passes if no panic occurs +} + +// TestUpdateDMMessagesForPR_UserMappingFailure tests when user mapping fails +func TestUpdateDMMessagesForPR_UserMappingFailure(t *testing.T) { + ctx := context.Background() + + testStore := state.NewMemoryStore() + mockSlack := NewMockSlack().Build() + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + // User mapper that always fails + mockMapper := &mockUserMapper{ + failLookups: true, + } + + c := &Coordinator{ + stateStore: testStore, + slack: mockSlack, + configManager: mockConfig, + userMapper: mockMapper, + threadCache: cache.New(), + } + + info := prUpdateInfo{ + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/org/repo/pull/1", + PRTitle: "Test PR", + PRAuthor: "author", + PRState: "open", + CheckResult: &turn.CheckResponse{ + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + }, + }, + }, + } + + // Should handle user mapping failure gracefully (no Slack users found) + c.updateDMMessagesForPR(ctx, info) + + // Test passes if no panic occurs +} + +// TestUpdateDMMessagesForPR_SendNotificationError tests error during DM send +func TestUpdateDMMessagesForPR_SendNotificationError(t *testing.T) { + ctx := context.Background() + + testStore := state.NewMemoryStore() + + // Mock Slack that returns error for DM operations + mockSlack := &mockSlackClient{ + sendDirectMessageFunc: func(ctx context.Context, userID, text string) (string, string, error) { + return "", "", errors.New("slack API error") + }, + } + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + mockMapper := &mockUserMapper{ + mapping: map[string]string{ + "user1": "U123", + }, + } + + c := &Coordinator{ + stateStore: testStore, + slack: mockSlack, + configManager: mockConfig, + userMapper: mockMapper, + threadCache: cache.New(), + } + + info := prUpdateInfo{ + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/org/repo/pull/1", + PRTitle: "Test PR", + PRAuthor: "author", + PRState: "open", + CheckResult: &turn.CheckResponse{ + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + }, + }, + }, + } + + // Should handle send notification error gracefully (logs warning) + c.updateDMMessagesForPR(ctx, info) + + // Test passes if no panic occurs +} diff --git a/pkg/bot/mock_builders_test.go b/pkg/bot/mock_builders_test.go index ae333f1..e041142 100644 --- a/pkg/bot/mock_builders_test.go +++ b/pkg/bot/mock_builders_test.go @@ -7,8 +7,9 @@ import ( "time" "github.com/codeGROOVE-dev/slacker/pkg/bot/cache" - + "github.com/codeGROOVE-dev/slacker/pkg/config" "github.com/codeGROOVE-dev/slacker/pkg/notify" + slackapi "github.com/codeGROOVE-dev/slacker/pkg/slack" "github.com/slack-go/slack" ) @@ -112,6 +113,14 @@ func (b *MockSlackBuilder) WithWorkspaceInfoError() *MockSlackBuilder { return b } +// WithFindDMMessagesInHistory configures the mock to return specific DM locations when searching history. +func (b *MockSlackBuilder) WithFindDMMessagesInHistory(locations []slackapi.DMLocation, err error) *MockSlackBuilder { + b.mock.findDMMessagesFunc = func(ctx context.Context, userID, prURL string, since time.Time) ([]slackapi.DMLocation, error) { + return locations, err + } + return b +} + // Build returns the configured mockSlackClient. func (b *MockSlackBuilder) Build() *mockSlackClient { return b.mock @@ -160,6 +169,24 @@ func (b *MockStateBuilder) WithSaveThreadError(err error) *MockStateBuilder { return b } +// WithQueuePendingDMError configures the mock to fail when queueing pending DMs. +func (b *MockStateBuilder) WithQueuePendingDMError(err error) *MockStateBuilder { + b.mock.queuePendingDMErr = err + return b +} + +// WithPendingDMsError configures the mock to fail when retrieving pending DMs. +func (b *MockStateBuilder) WithPendingDMsError(err error) *MockStateBuilder { + b.mock.pendingDMsErr = err + return b +} + +// WithRemovePendingDMError configures the mock to fail when removing pending DMs. +func (b *MockStateBuilder) WithRemovePendingDMError(err error) *MockStateBuilder { + b.mock.removePendingDMErr = err + return b +} + // Build returns the configured mockStateStore. func (b *MockStateBuilder) Build() *mockStateStore { return b.mock @@ -269,6 +296,22 @@ func (b *MockConfigBuilder) WithLoadError(err error) *MockConfigBuilder { return b } +// WithReloadConfigError configures ReloadConfig to return an error. +// This is a convenience method that sets loadErr since ReloadConfig calls LoadConfig. +func (b *MockConfigBuilder) WithReloadConfigError() *MockConfigBuilder { + return b.WithLoadError(errors.New("config reload failed")) +} + +// WithOrg configures an org to exist in configData. +func (b *MockConfigBuilder) WithOrg(org string) *MockConfigBuilder { + if b.mock.configData == nil { + b.mock.configData = make(map[string]interface{}) + } + // Add a minimal config for the org + b.mock.configData[org] = &config.RepoConfig{} + return b +} + // Build returns the configured mockConfigManager. func (b *MockConfigBuilder) Build() *mockConfigManager { return b.mock diff --git a/pkg/bot/polling.go b/pkg/bot/polling.go index f27c811..7248bef 100644 --- a/pkg/bot/polling.go +++ b/pkg/bot/polling.go @@ -15,30 +15,12 @@ import ( gogithub "github.com/google/go-github/v50/github" ) -// makePollEventKey creates an event key for poll-based PR processing. -// This is a pure function that can be easily tested. -func makePollEventKey(prURL string, updatedAt time.Time) string { - return fmt.Sprintf("poll:%s:%s", prURL, updatedAt.Format(time.RFC3339)) -} - -// makeClosedPREventKey creates an event key for closed/merged PR updates. -// This is a pure function that can be easily tested. -func makeClosedPREventKey(prURL, state string, updatedAt time.Time) string { - return fmt.Sprintf("poll_closed:%s:%s:%s", prURL, state, updatedAt.Format(time.RFC3339)) -} - // formatPRIdentifier creates a human-readable PR identifier. // This is a pure function that can be easily tested. func formatPRIdentifier(owner, repo string, prNumber int) string { return fmt.Sprintf("%s/%s#%d", owner, repo, prNumber) } -// makeReconcileEventKey creates an event key for startup reconciliation. -// This is a pure function that can be easily tested. -func makeReconcileEventKey(prURL string, updatedAt time.Time) string { - return fmt.Sprintf("reconcile:%s:%s", prURL, updatedAt.Format(time.RFC3339)) -} - // PollAndReconcile checks all open PRs and ensures notifications are sent. // This runs every 5 minutes as a safety net to catch anything sprinkler missed. func (c *Coordinator) PollAndReconcile(ctx context.Context) { @@ -89,7 +71,7 @@ func (c *Coordinator) pollAndReconcileWithSearcher(ctx context.Context, searcher pr := &prs[i] // Create event key for this PR update to prevent duplicate processing - eventKey := makePollEventKey(pr.URL, pr.UpdatedAt) + eventKey := fmt.Sprintf("poll:%s:%s", pr.URL, pr.UpdatedAt.Format(time.RFC3339)) // Skip if already processed (by webhook or previous poll) if c.stateStore.WasProcessed(ctx, eventKey) { @@ -145,7 +127,7 @@ func (c *Coordinator) pollAndReconcileWithSearcher(ctx context.Context, searcher pr := &closedPRs[i] // Create event key for this PR state change - eventKey := makeClosedPREventKey(pr.URL, pr.State, pr.UpdatedAt) + eventKey := fmt.Sprintf("poll_closed:%s:%s:%s", pr.URL, pr.State, pr.UpdatedAt.Format(time.RFC3339)) // Skip if already processed if c.stateStore.WasProcessed(ctx, eventKey) { @@ -375,7 +357,7 @@ func (c *Coordinator) StartupReconciliation(ctx context.Context) { // Create event key for this PR update (same format as webhook events) // This prevents processing the same update twice if a webhook was already received - eventKey := makeReconcileEventKey(pr.URL, pr.UpdatedAt) + eventKey := fmt.Sprintf("reconcile:%s:%s", pr.URL, pr.UpdatedAt.Format(time.RFC3339)) // Check if we already processed this exact PR update (via webhook or previous reconciliation) if c.stateStore.WasProcessed(ctx, eventKey) { @@ -511,7 +493,7 @@ func (c *Coordinator) checkDailyReports(ctx context.Context, org string, prs []g // Create daily report sender and dashboard fetcher sender := dailyreport.NewSender(c.stateStore, c.slack) - fetcher := home.NewFetcher(ghClient, c.stateStore, token, "ready-to-review[bot]") + fetcher := home.NewFetcher(ghClient, c.stateStore, token, "reviewgoose[bot]") sentCount := 0 skippedCount := 0 diff --git a/pkg/bot/polling_test.go b/pkg/bot/polling_test.go index bce48dd..85e389b 100644 --- a/pkg/bot/polling_test.go +++ b/pkg/bot/polling_test.go @@ -1,10 +1,10 @@ package bot import ( + // Commented out unused: "strings" "context" "errors" "fmt" - "strings" "testing" "time" @@ -348,6 +348,7 @@ func TestShouldReconcilePR(t *testing.T) { } } +/* REMOVED - function inlined // TestMakePollEventKey tests the pure function for creating poll event keys. func TestMakePollEventKey(t *testing.T) { tests := []struct { @@ -382,7 +383,9 @@ func TestMakePollEventKey(t *testing.T) { }) } } +*/ +/* REMOVED - function inlined // TestMakeClosedPREventKey tests the pure function for creating closed PR event keys. func TestMakeClosedPREventKey(t *testing.T) { tests := []struct { @@ -420,6 +423,7 @@ func TestMakeClosedPREventKey(t *testing.T) { }) } } +*/ // TestFormatPRIdentifier tests the pure function for formatting PR identifiers. func TestFormatPRIdentifier(t *testing.T) { @@ -463,6 +467,7 @@ func TestFormatPRIdentifier(t *testing.T) { } } +/* REMOVED - function inlined // TestMakeReconcileEventKey tests the pure function for creating reconcile event keys. func TestMakeReconcileEventKey(t *testing.T) { tests := []struct { @@ -494,6 +499,7 @@ func TestMakeReconcileEventKey(t *testing.T) { }) } } +*/ // TestIsChannelResolutionFailed tests channel resolution failure detection. func TestIsChannelResolutionFailed(t *testing.T) { diff --git a/pkg/bot/process_event_test.go b/pkg/bot/process_event_test.go index 9426943..37b2e4d 100644 --- a/pkg/bot/process_event_test.go +++ b/pkg/bot/process_event_test.go @@ -2,6 +2,7 @@ package bot import ( "context" + "errors" "testing" "time" @@ -304,3 +305,73 @@ func TestProcessEvent_PullRequestCodeGROOVE(t *testing.T) { t.Errorf("expected nil error for .codeGROOVE PR event, got: %v", err) } } + +// TestProcessEvent_ConfigLoadError tests error handling when config loading fails +func TestProcessEvent_ConfigLoadError(t *testing.T) { + ctx := context.Background() + + mockGH := &mockGitHub{ + org: "testorg", + token: "test-token", + } + + // MockConfig with no loaded configs - will trigger LoadConfig which will fail + mockConfig := NewMockConfig(). + WithLoadError(errors.New("config load failed")). + Build() + + c := &Coordinator{ + github: mockGH, + slack: &mockSlackClient{}, + stateStore: &mockStateStore{processedEvents: make(map[string]bool)}, + configManager: mockConfig, + threadCache: cache.New(), + eventSemaphore: make(chan struct{}, 10), + } + + msg := SprinklerMessage{ + Event: "pull_request", + Repo: "neworg/repo", + PRNumber: 1, + URL: "https://github.com/neworg/repo/pull/1", + Timestamp: time.Now(), + } + + // Should handle config load error gracefully (log warning but continue) + err := c.processEvent(ctx, msg) + // No error should be returned even if config loading fails + if err != nil { + t.Errorf("expected nil error even with config load failure, got: %v", err) + } +} + +// TestProcessEvent_PushToCodeGROOVEConfigReloadError tests reload error handling +func TestProcessEvent_PushToCodeGROOVEConfigReloadError(t *testing.T) { + ctx := context.Background() + + // MockConfig with error injection for ReloadConfig + mockConfig := NewMockConfig(). + WithReloadConfigError(). + WithOrg("testorg"). + Build() + + c := &Coordinator{ + github: &mockGitHub{org: "testorg", token: "test-token"}, + slack: &mockSlackClient{}, + stateStore: &mockStateStore{processedEvents: make(map[string]bool)}, + configManager: mockConfig, + threadCache: cache.New(), + } + + msg := SprinklerMessage{ + Event: "push", + Repo: "testorg/.codeGROOVE", + Timestamp: time.Now(), + } + + // Should handle reload error gracefully (log warning but continue) + err := c.processEvent(ctx, msg) + if err != nil { + t.Errorf("expected nil error even with config reload failure, got: %v", err) + } +} diff --git a/pkg/bot/process_pr_when_threshold_test.go b/pkg/bot/process_pr_when_threshold_test.go new file mode 100644 index 0000000..cb262dd --- /dev/null +++ b/pkg/bot/process_pr_when_threshold_test.go @@ -0,0 +1,191 @@ +package bot + +import ( + "context" + "testing" + "time" + + "github.com/codeGROOVE-dev/prx/pkg/prx" + turn "github.com/codeGROOVE-dev/turnclient/pkg/turn" + "github.com/slack-go/slack" +) + +// NOTE: "when" threshold tests are omitted because they require complex YAML config +// mocking. The threshold logic itself (shouldPostThread) is tested separately at 100% coverage. + +// TestProcessPRForChannel_InvalidEventTypeAssertionFailure tests invalid event type assertion +func TestProcessPRForChannel_InvalidEventTypeAssertionFailure(t *testing.T) { + ctx := context.Background() + + c := NewTestCoordinator(). + WithState(NewMockState().Build()). + WithSlack(NewMockSlack().Build()). + WithConfig(NewMockConfig().Build()). + Build() + + prCtx := prContext{ + Owner: "testorg", + Repo: "testrepo", + Number: 1, + State: "open", + Event: "invalid-event-type", // Wrong type + CheckRes: &turn.CheckResponse{ + PullRequest: prx.PullRequest{State: "open"}, + }, + } + + // Should return nil due to invalid event type + taggedUsers := c.processPRForChannel(ctx, prCtx, "testrepo", "workspace-123") + + if taggedUsers != nil { + t.Errorf("expected nil taggedUsers for invalid event type, got %v", taggedUsers) + } +} + +// TestProcessPRForChannel_TerminalStateDMUpdate tests merged/closed PR DM updates +func TestProcessPRForChannel_TerminalStateDMUpdate(t *testing.T) { + ctx := context.Background() + + mockSlack := &mockSlackClient{ + resolveChannelFunc: func(ctx context.Context, channelName string) string { + return "C123" + }, + botInChannelFunc: func(ctx context.Context, channelID string) bool { + return true + }, + channelHistoryFunc: func(ctx context.Context, channelID string, oldest, latest string, limit int) (*slack.GetConversationHistoryResponse, error) { + // Return existing thread + return &slack.GetConversationHistoryResponse{ + Messages: []slack.Message{ + { + Msg: slack.Msg{ + Timestamp: "1234.5678", + Text: ":hourglass: Test PR https://github.com/testorg/testrepo/pull/1?st=awaiting_review", + User: "UBOT", + }, + }, + }, + }, nil + }, + botInfoFunc: func(ctx context.Context) (*slack.AuthTestResponse, error) { + return &slack.AuthTestResponse{UserID: "UBOT"}, nil + }, + updateMessageFunc: func(ctx context.Context, channelID, ts, text string) error { + return nil + }, + } + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + mockState := NewMockState().Build() + + c := NewTestCoordinator(). + WithState(mockState). + WithSlack(mockSlack). + WithConfig(mockConfig). + WithUserMapper(NewMockUserMapper().Build()). + Build() + + event := struct { + Action string `json:"action"` + PullRequest struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + } `json:"pull_request"` + Number int `json:"number"` + }{ + Action: "closed", + } + event.PullRequest.HTMLURL = "https://github.com/testorg/testrepo/pull/1" + event.PullRequest.Title = "Test PR" + event.PullRequest.CreatedAt = time.Now() + event.PullRequest.User.Login = "author" + event.PullRequest.Number = 1 + + prCtx := prContext{ + Owner: "testorg", + Repo: "testrepo", + Number: 1, + State: "merged", // Terminal state + Event: event, + CheckRes: &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "closed", + Merged: true, + }, + }, + } + + // Process the PR - should update existing thread and trigger DM updates for terminal state + taggedUsers := c.processPRForChannel(ctx, prCtx, "testrepo", "workspace-123") + + // Thread exists, so taggedUsers should be non-nil + if taggedUsers == nil { + t.Error("expected non-nil taggedUsers when processing existing thread") + } +} + +// TestProcessPRForChannel_ChannelResolutionFailure tests when channel can't be resolved +func TestProcessPRForChannel_ChannelResolutionFailure(t *testing.T) { + ctx := context.Background() + + mockSlack := &mockSlackClient{ + resolveChannelFunc: func(ctx context.Context, channelName string) string { + // Return same as input (resolution failed) + return channelName + }, + } + + c := NewTestCoordinator(). + WithState(NewMockState().Build()). + WithSlack(mockSlack). + WithConfig(NewMockConfig().Build()). + Build() + c.workspaceName = "test-workspace" + + event := struct { + Action string `json:"action"` + PullRequest struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + } `json:"pull_request"` + Number int `json:"number"` + }{ + Action: "opened", + } + event.PullRequest.HTMLURL = "https://github.com/testorg/testrepo/pull/1" + event.PullRequest.Title = "Test PR" + event.PullRequest.CreatedAt = time.Now() + event.PullRequest.User.Login = "author" + event.PullRequest.Number = 1 + + prCtx := prContext{ + Owner: "testorg", + Repo: "testrepo", + Number: 1, + State: "open", + Event: event, + CheckRes: &turn.CheckResponse{ + PullRequest: prx.PullRequest{State: "open"}, + }, + } + + // Should return nil due to channel resolution failure + taggedUsers := c.processPRForChannel(ctx, prCtx, "unknown-channel", "workspace-123") + + if taggedUsers != nil { + t.Errorf("expected nil taggedUsers when channel resolution fails, got %v", taggedUsers) + } +} diff --git a/pkg/bot/thread_locking_test.go b/pkg/bot/thread_locking_test.go new file mode 100644 index 0000000..2b4e001 --- /dev/null +++ b/pkg/bot/thread_locking_test.go @@ -0,0 +1,368 @@ +package bot + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/codeGROOVE-dev/prx/pkg/prx" + "github.com/codeGROOVE-dev/slacker/pkg/bot/cache" + "github.com/codeGROOVE-dev/slacker/pkg/state" + turn "github.com/codeGROOVE-dev/turnclient/pkg/turn" + "github.com/slack-go/slack" +) + +// TestCreatePRThreadWithLocking_SuccessfulCreation tests the happy path +func TestCreatePRThreadWithLocking_SuccessfulCreation(t *testing.T) { + ctx := context.Background() + + mockSlack := &mockSlackClient{ + postThreadFunc: func(ctx context.Context, channelID, text string, attachments []slack.Attachment) (string, error) { + return "1234.5678", nil + }, + channelHistoryFunc: func(ctx context.Context, channelID string, oldest, latest string, limit int) (*slack.GetConversationHistoryResponse, error) { + return &slack.GetConversationHistoryResponse{Messages: []slack.Message{}}, nil + }, + botInfoFunc: func(ctx context.Context) (*slack.AuthTestResponse, error) { + return &slack.AuthTestResponse{UserID: "B123"}, nil + }, + } + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + c := &Coordinator{ + slack: mockSlack, + configManager: mockConfig, + userMapper: &mockUserMapper{mapping: map[string]string{}}, + stateStore: state.NewMemoryStore(), + threadCache: cache.New(), + } + + pr := struct { + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + HTMLURL string `json:"html_url"` + Title string `json:"title"` + Number int `json:"number"` + }{ + CreatedAt: time.Now().Add(-1 * time.Hour), + HTMLURL: "https://github.com/org/repo/pull/1", + Title: "Test PR", + Number: 1, + } + pr.User.Login = "author" + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + Merged: false, + Draft: false, + }, + Analysis: turn.Analysis{ + WorkflowState: "awaiting_review", + NextAction: map[string]turn.Action{}, + }, + } + + params := threadCreationParams{ + ChannelID: "C123", + ChannelName: "test-channel", + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRState: "awaiting_review", + PullRequest: pr, + CheckResult: checkResult, + } + + info, wasCreated, err := c.createPRThreadWithLocking(ctx, params) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !wasCreated { + t.Error("expected wasCreated to be true") + } + + if info.ThreadTS != "1234.5678" { + t.Errorf("expected threadTS 1234.5678, got %s", info.ThreadTS) + } + + // Verify cache was populated + cacheKey := "org/repo#1:C123" + if cachedInfo, exists := c.threadCache.Get(cacheKey); !exists { + t.Error("expected thread to be cached") + } else if cachedInfo.ThreadTS != "1234.5678" { + t.Errorf("expected cached threadTS 1234.5678, got %s", cachedInfo.ThreadTS) + } +} + +// TestCreatePRThreadWithLocking_CacheHitAfterMarking tests finding thread in cache after marking as creating +func TestCreatePRThreadWithLocking_CacheHitAfterMarking(t *testing.T) { + ctx := context.Background() + + mockSlack := &mockSlackClient{ + postThreadFunc: func(ctx context.Context, channelID, text string, attachments []slack.Attachment) (string, error) { + t.Error("PostThread should not be called when thread exists in cache") + return "", errors.New("should not be called") + }, + } + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + threadCache := cache.New() + cacheKey := "org/repo#1:C123" + + c := &Coordinator{ + slack: mockSlack, + configManager: mockConfig, + userMapper: &mockUserMapper{mapping: map[string]string{}}, + stateStore: state.NewMemoryStore(), + threadCache: threadCache, + } + + // Pre-populate cache AFTER we would mark as creating (simulating race) + // We'll do this by starting the test, then using a custom cache that sets it + existingInfo := cache.ThreadInfo{ + ThreadTS: "existing.thread", + ChannelID: "C123", + LastState: "open", + MessageText: "existing message", + } + + pr := struct { + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + HTMLURL string `json:"html_url"` + Title string `json:"title"` + Number int `json:"number"` + }{ + CreatedAt: time.Now().Add(-1 * time.Hour), + HTMLURL: "https://github.com/org/repo/pull/1", + Title: "Test PR", + Number: 1, + } + pr.User.Login = "author" + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + Merged: false, + Draft: false, + }, + Analysis: turn.Analysis{ + WorkflowState: "awaiting_review", + NextAction: map[string]turn.Action{}, + }, + } + + params := threadCreationParams{ + ChannelID: "C123", + ChannelName: "test-channel", + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRState: "awaiting_review", + PullRequest: pr, + CheckResult: checkResult, + } + + // Set the cache entry to simulate another goroutine creating it + threadCache.Set(cacheKey, existingInfo) + + info, wasCreated, err := c.createPRThreadWithLocking(ctx, params) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if wasCreated { + t.Error("expected wasCreated to be false when thread exists in cache") + } + + if info.ThreadTS != "existing.thread" { + t.Errorf("expected existing threadTS, got %s", info.ThreadTS) + } +} + +// TestCreatePRThreadWithLocking_CrossInstanceRace tests detecting thread created by another instance +func TestCreatePRThreadWithLocking_CrossInstanceRace(t *testing.T) { + ctx := context.Background() + + mockSlack := &mockSlackClient{ + postThreadFunc: func(ctx context.Context, channelID, text string, attachments []slack.Attachment) (string, error) { + t.Error("PostThread should not be called when cross-instance thread detected") + return "", errors.New("should not be called") + }, + channelHistoryFunc: func(ctx context.Context, channelID string, oldest, latest string, limit int) (*slack.GetConversationHistoryResponse, error) { + // Return an existing message that matches the PR URL + return &slack.GetConversationHistoryResponse{ + Messages: []slack.Message{ + { + Msg: slack.Msg{ + Timestamp: "cross.instance", + Text: ":hourglass: Test PR https://github.com/org/repo/pull/1", + User: "B123", // Bot user + }, + }, + }, + }, nil + }, + botInfoFunc: func(ctx context.Context) (*slack.AuthTestResponse, error) { + return &slack.AuthTestResponse{UserID: "B123"}, nil + }, + } + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + c := &Coordinator{ + slack: mockSlack, + configManager: mockConfig, + userMapper: &mockUserMapper{mapping: map[string]string{}}, + stateStore: state.NewMemoryStore(), + threadCache: cache.New(), + } + + pr := struct { + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + HTMLURL string `json:"html_url"` + Title string `json:"title"` + Number int `json:"number"` + }{ + CreatedAt: time.Now().Add(-1 * time.Hour), + HTMLURL: "https://github.com/org/repo/pull/1", + Title: "Test PR", + Number: 1, + } + pr.User.Login = "author" + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + Merged: false, + Draft: false, + }, + Analysis: turn.Analysis{ + WorkflowState: "awaiting_review", + NextAction: map[string]turn.Action{}, + }, + } + + params := threadCreationParams{ + ChannelID: "C123", + ChannelName: "test-channel", + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRState: "awaiting_review", + PullRequest: pr, + CheckResult: checkResult, + } + + info, wasCreated, err := c.createPRThreadWithLocking(ctx, params) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if wasCreated { + t.Error("expected wasCreated to be false when cross-instance thread detected") + } + + if info.ThreadTS != "cross.instance" { + t.Errorf("expected cross-instance threadTS, got %s", info.ThreadTS) + } +} + +// TestCreatePRThreadWithLocking_CreateThreadError tests error handling during thread creation +func TestCreatePRThreadWithLocking_CreateThreadError(t *testing.T) { + ctx := context.Background() + + mockSlack := &mockSlackClient{ + postThreadFunc: func(ctx context.Context, channelID, text string, attachments []slack.Attachment) (string, error) { + return "", errors.New("slack API error") + }, + channelHistoryFunc: func(ctx context.Context, channelID string, oldest, latest string, limit int) (*slack.GetConversationHistoryResponse, error) { + return &slack.GetConversationHistoryResponse{Messages: []slack.Message{}}, nil + }, + botInfoFunc: func(ctx context.Context) (*slack.AuthTestResponse, error) { + return &slack.AuthTestResponse{UserID: "B123"}, nil + }, + } + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + c := &Coordinator{ + slack: mockSlack, + configManager: mockConfig, + userMapper: &mockUserMapper{mapping: map[string]string{}}, + stateStore: state.NewMemoryStore(), + threadCache: cache.New(), + } + + pr := struct { + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + HTMLURL string `json:"html_url"` + Title string `json:"title"` + Number int `json:"number"` + }{ + CreatedAt: time.Now().Add(-1 * time.Hour), + HTMLURL: "https://github.com/org/repo/pull/1", + Title: "Test PR", + Number: 1, + } + pr.User.Login = "author" + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + Merged: false, + Draft: false, + }, + Analysis: turn.Analysis{ + WorkflowState: "awaiting_review", + NextAction: map[string]turn.Action{}, + }, + } + + params := threadCreationParams{ + ChannelID: "C123", + ChannelName: "test-channel", + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRState: "awaiting_review", + PullRequest: pr, + CheckResult: checkResult, + } + + _, wasCreated, err := c.createPRThreadWithLocking(ctx, params) + if err == nil { + t.Fatal("expected error when thread creation fails") + } + + if wasCreated { + t.Error("expected wasCreated to be false when creation fails") + } + + if !errors.Is(err, errors.New("slack API error")) && !contains(err.Error(), "slack API error") { + t.Errorf("expected error to mention slack API error, got: %v", err) + } +} diff --git a/pkg/bot/track_user_tags_test.go b/pkg/bot/track_user_tags_test.go new file mode 100644 index 0000000..be321f2 --- /dev/null +++ b/pkg/bot/track_user_tags_test.go @@ -0,0 +1,172 @@ +package bot + +import ( + "context" + "testing" + + "github.com/codeGROOVE-dev/prx/pkg/prx" + turn "github.com/codeGROOVE-dev/turnclient/pkg/turn" +) + +// TestTrackUserTagsForDMDelay_NoBlockedUsers tests early return when no users are blocked +func TestTrackUserTagsForDMDelay_NoBlockedUsers(t *testing.T) { + ctx := context.Background() + + c := &Coordinator{ + configManager: NewMockConfig().Build(), + } + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{State: "open"}, + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{}, // No blocked users + }, + } + + tags := c.trackUserTagsForDMDelay(ctx, "W123", "C123", "#test-channel", "org", "repo", 1, checkResult) + + if len(tags) != 0 { + t.Errorf("expected empty tags for no blocked users, got %d tags", len(tags)) + } +} + +// TestTrackUserTagsForDMDelay_UserMappingFailure tests when user mapping fails +func TestTrackUserTagsForDMDelay_UserMappingFailure(t *testing.T) { + ctx := context.Background() + + mockMapper := &mockUserMapper{ + failLookups: true, // All lookups fail + } + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + c := &Coordinator{ + userMapper: mockMapper, + configManager: mockConfig, + } + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{State: "open"}, + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + }, + }, + } + + tags := c.trackUserTagsForDMDelay(ctx, "W123", "C123", "#test-channel", "org", "repo", 1, checkResult) + + // Should return empty map when user mapping fails + if len(tags) != 0 { + t.Errorf("expected empty tags when user mapping fails, got %d tags", len(tags)) + } +} + +// TestTrackUserTagsForDMDelay_EmptySlackUserID tests when mapping returns empty slack ID +func TestTrackUserTagsForDMDelay_EmptySlackUserID(t *testing.T) { + ctx := context.Background() + + mockMapper := &mockUserMapper{ + mapping: map[string]string{ + "user1": "", // Empty Slack ID + }, + } + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + c := &Coordinator{ + userMapper: mockMapper, + configManager: mockConfig, + } + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{State: "open"}, + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + }, + }, + } + + tags := c.trackUserTagsForDMDelay(ctx, "W123", "C123", "#test-channel", "org", "repo", 1, checkResult) + + // Should skip users with empty Slack IDs + if len(tags) != 0 { + t.Errorf("expected empty tags for empty slack user ID, got %d tags", len(tags)) + } +} + +// TestTrackUserTagsForDMDelay_Success tests successful user tagging +func TestTrackUserTagsForDMDelay_Success(t *testing.T) { + ctx := context.Background() + + mockMapper := &mockUserMapper{ + mapping: map[string]string{ + "user1": "U123", + "user2": "U456", + }, + } + + mockSlack := &mockSlackClient{ + isUserInChannelFunc: func(ctx context.Context, channelID, userID string) bool { + // user1 is in channel, user2 is not + return userID == "U123" + }, + } + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + c := &Coordinator{ + userMapper: mockMapper, + slack: mockSlack, + configManager: mockConfig, + notifier: nil, // Testing without notifier for simplicity + } + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{State: "open"}, + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{ + "user1": {Kind: "review"}, + "user2": {Kind: "approve"}, + }, + }, + } + + tags := c.trackUserTagsForDMDelay(ctx, "W123", "C123", "#test-channel", "org", "repo", 1, checkResult) + + if len(tags) != 2 { + t.Fatalf("expected 2 tagged users, got %d", len(tags)) + } + + // Check user1 + if tag, exists := tags["U123"]; !exists { + t.Error("expected U123 in tagged users") + } else { + if tag.UserID != "U123" { + t.Errorf("expected UserID U123, got %s", tag.UserID) + } + if !tag.IsInAnyChannel { + t.Error("expected U123 to be in channel") + } + } + + // Check user2 + if tag, exists := tags["U456"]; !exists { + t.Error("expected U456 in tagged users") + } else { + if tag.UserID != "U456" { + t.Errorf("expected UserID U456, got %s", tag.UserID) + } + if tag.IsInAnyChannel { + t.Error("expected U456 to NOT be in channel") + } + } +} + diff --git a/pkg/bot/update_message_test.go b/pkg/bot/update_message_test.go new file mode 100644 index 0000000..cd58565 --- /dev/null +++ b/pkg/bot/update_message_test.go @@ -0,0 +1,413 @@ +package bot + +import ( + "context" + "errors" + "sync" + "testing" + "time" + + "github.com/codeGROOVE-dev/slacker/pkg/bot/cache" + "github.com/codeGROOVE-dev/slacker/pkg/state" + turn "github.com/codeGROOVE-dev/turnclient/pkg/turn" +) + +// TestUpdateMessageIfNeeded_InvalidEventType tests when event type assertion fails +func TestUpdateMessageIfNeeded_InvalidEventType(t *testing.T) { + ctx := context.Background() + + mockSlack := NewMockSlack().Build() + mockConfig := NewMockConfig().Build() + + c := &Coordinator{ + slack: mockSlack, + configManager: mockConfig, + threadCache: cache.New(), + } + + params := messageUpdateParams{ + Event: "invalid-event-type", // Wrong type + Owner: "org", + Repo: "repo", + PRNumber: 1, + } + + // Should return early with error log + c.updateMessageIfNeeded(ctx, params) + + // Test passes if no panic occurs +} + +// TestUpdateMessageIfNeeded_MessageAlreadyMatches tests when message doesn't need update +func TestUpdateMessageIfNeeded_MessageAlreadyMatches(t *testing.T) { + ctx := context.Background() + + mockSlack := NewMockSlack().Build() + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + mockMapper := &mockUserMapper{ + mapping: map[string]string{}, + } + + testStore := state.NewMemoryStore() + + c := &Coordinator{ + slack: mockSlack, + configManager: mockConfig, + userMapper: mockMapper, + stateStore: testStore, + threadCache: cache.New(), + workspaceName: "test-workspace", + } + + event := struct { + Action string `json:"action"` + PullRequest struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + } `json:"pull_request"` + Number int `json:"number"` + }{ + Action: "opened", + } + event.PullRequest.HTMLURL = "https://github.com/org/repo/pull/1" + event.PullRequest.Title = "Test PR" + event.PullRequest.User.Login = "author" + event.PullRequest.Number = 1 + + // Format expected message + expectedMsg := ":hourglass: Test PR https://github.com/org/repo/pull/1?st=test_state" + + params := messageUpdateParams{ + Event: event, + Owner: "org", + Repo: "repo", + PRNumber: 1, + ChannelID: "C123", + ChannelName: "test-channel", + ChannelDisplay: "#test-channel", + ThreadTS: "1234.5678", + CurrentText: expectedMsg, // Already matches + PRState: "test_state", + CheckResult: &turn.CheckResponse{ + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{}, + }, + }, + } + + // Should return early (message already matches) + c.updateMessageIfNeeded(ctx, params) + + // Test passes if no panic occurs +} + +// TestUpdateMessageIfNeeded_UpdateError tests when UpdateMessage fails +func TestUpdateMessageIfNeeded_UpdateError(t *testing.T) { + ctx := context.Background() + + // Mock Slack that returns error for UpdateMessage + mockSlack := NewMockSlack(). + WithUpdateMessageError(errors.New("slack API error")). + Build() + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + mockMapper := &mockUserMapper{ + mapping: map[string]string{}, + } + + testStore := state.NewMemoryStore() + + c := &Coordinator{ + slack: mockSlack, + configManager: mockConfig, + userMapper: mockMapper, + stateStore: testStore, + threadCache: cache.New(), + workspaceName: "test-workspace", + } + + event := struct { + Action string `json:"action"` + PullRequest struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + } `json:"pull_request"` + Number int `json:"number"` + }{ + Action: "synchronized", + } + event.PullRequest.HTMLURL = "https://github.com/org/repo/pull/1" + event.PullRequest.Title = "Test PR" + event.PullRequest.User.Login = "author" + event.PullRequest.Number = 1 + + params := messageUpdateParams{ + Event: event, + Owner: "org", + Repo: "repo", + PRNumber: 1, + ChannelID: "C123", + ChannelName: "test-channel", + ChannelDisplay: "#test-channel", + ThreadTS: "1234.5678", + CurrentText: "old message", + PRState: "test_state", + CheckResult: &turn.CheckResponse{ + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{}, + }, + }, + } + + // Should handle UpdateMessage error gracefully + c.updateMessageIfNeeded(ctx, params) + + // Test passes if no panic occurs +} + +// TestUpdateMessageIfNeeded_Success tests successful message update +func TestUpdateMessageIfNeeded_Success(t *testing.T) { + ctx := context.Background() + + mockSlack := NewMockSlack(). + WithUpdateMessageSuccess(). + Build() + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + mockMapper := &mockUserMapper{ + mapping: map[string]string{}, + } + + testStore := state.NewMemoryStore() + + c := &Coordinator{ + slack: mockSlack, + configManager: mockConfig, + userMapper: mockMapper, + stateStore: testStore, + threadCache: cache.New(), + workspaceName: "test-workspace", + } + + event := struct { + Action string `json:"action"` + PullRequest struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + } `json:"pull_request"` + Number int `json:"number"` + }{ + Action: "synchronized", + } + event.PullRequest.HTMLURL = "https://github.com/org/repo/pull/1" + event.PullRequest.Title = "Test PR" + event.PullRequest.User.Login = "author" + event.PullRequest.Number = 1 + + params := messageUpdateParams{ + Event: event, + Owner: "org", + Repo: "repo", + PRNumber: 1, + ChannelID: "C123", + ChannelName: "test-channel", + ChannelDisplay: "#test-channel", + ThreadTS: "1234.5678", + CurrentText: "old message", + PRState: "open", + CheckResult: &turn.CheckResponse{ + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{}, + }, + }, + } + + // Should successfully update message + c.updateMessageIfNeeded(ctx, params) + + // Test passes if no panic occurs +} + +// TestUpdateMessageIfNeeded_LogDeduplication tests log deduplication within 1 second +func TestUpdateMessageIfNeeded_LogDeduplication(t *testing.T) { + ctx := context.Background() + + mockSlack := NewMockSlack(). + WithUpdateMessageSuccess(). + Build() + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + mockMapper := &mockUserMapper{ + mapping: map[string]string{}, + } + + testStore := state.NewMemoryStore() + + c := &Coordinator{ + slack: mockSlack, + configManager: mockConfig, + userMapper: mockMapper, + stateStore: testStore, + threadCache: cache.New(), + workspaceName: "test-workspace", + recentUpdateLogs: make(map[string]time.Time), + recentUpdateLogsMu: sync.Mutex{}, + } + + event := struct { + Action string `json:"action"` + PullRequest struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + } `json:"pull_request"` + Number int `json:"number"` + }{ + Action: "synchronized", + } + event.PullRequest.HTMLURL = "https://github.com/org/repo/pull/1" + event.PullRequest.Title = "Test PR" + event.PullRequest.User.Login = "author" + event.PullRequest.Number = 1 + + params := messageUpdateParams{ + Event: event, + Owner: "org", + Repo: "repo", + PRNumber: 1, + ChannelID: "C123", + ChannelName: "test-channel", + ChannelDisplay: "#test-channel", + ThreadTS: "1234.5678", + CurrentText: "old message", + PRState: "open", + CheckResult: &turn.CheckResponse{ + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{}, + }, + }, + } + + // First call - should log + c.updateMessageIfNeeded(ctx, params) + + // Second call immediately after - should deduplicate log + params.CurrentText = "different message" + c.updateMessageIfNeeded(ctx, params) + + // Test passes if no panic occurs +} + +// TestUpdateMessageIfNeeded_OldLogCleanup tests cleanup of old log entries +func TestUpdateMessageIfNeeded_OldLogCleanup(t *testing.T) { + ctx := context.Background() + + mockSlack := NewMockSlack(). + WithUpdateMessageSuccess(). + Build() + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + mockMapper := &mockUserMapper{ + mapping: map[string]string{}, + } + + testStore := state.NewMemoryStore() + + c := &Coordinator{ + slack: mockSlack, + configManager: mockConfig, + userMapper: mockMapper, + stateStore: testStore, + threadCache: cache.New(), + workspaceName: "test-workspace", + recentUpdateLogs: make(map[string]time.Time), + recentUpdateLogsMu: sync.Mutex{}, + } + + // Add old log entry (>5 seconds ago) + c.recentUpdateLogs["old_key"] = time.Now().Add(-10 * time.Second) + + event := struct { + Action string `json:"action"` + PullRequest struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + } `json:"pull_request"` + Number int `json:"number"` + }{ + Action: "synchronized", + } + event.PullRequest.HTMLURL = "https://github.com/org/repo/pull/1" + event.PullRequest.Title = "Test PR" + event.PullRequest.User.Login = "author" + event.PullRequest.Number = 1 + + params := messageUpdateParams{ + Event: event, + Owner: "org", + Repo: "repo", + PRNumber: 1, + ChannelID: "C123", + ChannelName: "test-channel", + ChannelDisplay: "#test-channel", + ThreadTS: "1234.5678", + CurrentText: "old message", + PRState: "open", + CheckResult: &turn.CheckResponse{ + Analysis: turn.Analysis{ + NextAction: map[string]turn.Action{}, + }, + }, + } + + // Should clean up old log entries during this call + c.updateMessageIfNeeded(ctx, params) + + // Check that old entry was cleaned up + c.recentUpdateLogsMu.Lock() + _, exists := c.recentUpdateLogs["old_key"] + c.recentUpdateLogsMu.Unlock() + + if exists { + t.Error("expected old log entry to be cleaned up") + } +} diff --git a/pkg/bot/wait_concurrent_test.go b/pkg/bot/wait_concurrent_test.go new file mode 100644 index 0000000..6a8a039 --- /dev/null +++ b/pkg/bot/wait_concurrent_test.go @@ -0,0 +1,398 @@ +package bot + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/codeGROOVE-dev/prx/pkg/prx" + "github.com/codeGROOVE-dev/slacker/pkg/bot/cache" + "github.com/codeGROOVE-dev/slacker/pkg/state" + turn "github.com/codeGROOVE-dev/turnclient/pkg/turn" + "github.com/slack-go/slack" +) + +// TestWaitForConcurrentCreation_ThreadFoundDuringWait tests finding thread in cache during wait +func TestWaitForConcurrentCreation_ThreadFoundDuringWait(t *testing.T) { + threadCache := cache.New() + cacheKey := "org/repo#1:C123" + + c := &Coordinator{ + threadCache: threadCache, + } + + // Mark as creating to simulate another goroutine creating + if !threadCache.MarkCreating(cacheKey) { + t.Fatal("failed to mark as creating") + } + + // Start a goroutine to populate cache after a short delay + go func() { + time.Sleep(200 * time.Millisecond) + threadCache.Set(cacheKey, cache.ThreadInfo{ + ThreadTS: "found.thread", + MessageText: "found message", + }) + threadCache.UnmarkCreating(cacheKey) + }() + + // Wait for concurrent creation + threadTS, messageText, shouldProceed := c.waitForConcurrentCreation(cacheKey) + + if shouldProceed { + t.Error("expected shouldProceed to be false when thread found") + } + + if threadTS != "found.thread" { + t.Errorf("expected threadTS 'found.thread', got %s", threadTS) + } + + if messageText != "found message" { + t.Errorf("expected messageText 'found message', got %s", messageText) + } +} + +// TestWaitForConcurrentCreation_OtherGoroutineFinishedWithoutCaching tests when other goroutine finishes but doesn't cache +func TestWaitForConcurrentCreation_OtherGoroutineFinishedWithoutCaching(t *testing.T) { + threadCache := cache.New() + cacheKey := "org/repo#1:C123" + + c := &Coordinator{ + threadCache: threadCache, + } + + // Mark as creating to simulate another goroutine creating + if !threadCache.MarkCreating(cacheKey) { + t.Fatal("failed to mark as creating") + } + + // Start a goroutine to unmark without caching (simulating failure) + go func() { + time.Sleep(200 * time.Millisecond) + threadCache.UnmarkCreating(cacheKey) + }() + + // Wait for concurrent creation + threadTS, messageText, shouldProceed := c.waitForConcurrentCreation(cacheKey) + + if !shouldProceed { + t.Error("expected shouldProceed to be true when other goroutine finished without caching") + } + + if threadTS != "" || messageText != "" { + t.Errorf("expected empty strings, got threadTS=%s, messageText=%s", threadTS, messageText) + } + + // Verify we successfully marked as creating + if !threadCache.IsCreating(cacheKey) { + t.Error("expected to have successfully marked as creating") + } + threadCache.UnmarkCreating(cacheKey) // Clean up +} + +// TestWaitForConcurrentCreation_Timeout tests timeout scenario with reduced wait time +func TestWaitForConcurrentCreation_Timeout(t *testing.T) { + t.Skip("Skipping timeout test - takes too long for regular test runs") + + threadCache := cache.New() + cacheKey := "org/repo#1:C123" + + c := &Coordinator{ + threadCache: threadCache, + } + + // Mark as creating and never unmark (simulating stuck goroutine) + if !threadCache.MarkCreating(cacheKey) { + t.Fatal("failed to mark as creating") + } + + // This will timeout after 30 seconds + start := time.Now() + threadTS, messageText, shouldProceed := c.waitForConcurrentCreation(cacheKey) + elapsed := time.Since(start) + + // Should have timed out + if elapsed < 30*time.Second { + t.Errorf("expected to wait at least 30s, waited %v", elapsed) + } + + // After timeout, should either find thread or proceed + if threadTS == "" && messageText == "" && !shouldProceed { + t.Error("after timeout, should either have thread info or shouldProceed=true") + } +} + +// TestWaitForConcurrentCreation_QuickUnresponsive tests the loop continues while key is marked +func TestWaitForConcurrentCreation_QuickUnresponsive(t *testing.T) { + threadCache := cache.New() + cacheKey := "org/repo#1:C123" + + // Mark as creating and keep it marked (simulating unresponsive goroutine) + if !threadCache.MarkCreating(cacheKey) { + t.Fatal("failed to mark as creating") + } + + // Start goroutine that keeps it marked for a while + done := make(chan bool) + go func() { + time.Sleep(2 * time.Second) + // Still keep it marked - simulating stuck state + close(done) + }() + + // This tests the loop logic even though it won't actually timeout + // We're testing the "still creating" path in the loop + go func() { + time.Sleep(1 * time.Second) + // Verify it's still marked during the wait + if !threadCache.IsCreating(cacheKey) { + t.Error("expected cache key to still be marked as creating") + } + }() + + <-done + threadCache.UnmarkCreating(cacheKey) // Clean up +} + +// TestCreatePRThreadWithLocking_ConcurrentCreation tests the full concurrent creation flow +func TestCreatePRThreadWithLocking_ConcurrentCreation(t *testing.T) { + ctx := context.Background() + + var postThreadCalls int + var mu sync.Mutex + + mockSlack := &mockSlackClient{ + postThreadFunc: func(ctx context.Context, channelID, text string, attachments []slack.Attachment) (string, error) { + mu.Lock() + defer mu.Unlock() + postThreadCalls++ + // Add delay to simulate real API call + time.Sleep(100 * time.Millisecond) + return "1234.5678", nil + }, + channelHistoryFunc: func(ctx context.Context, channelID string, oldest, latest string, limit int) (*slack.GetConversationHistoryResponse, error) { + return &slack.GetConversationHistoryResponse{Messages: []slack.Message{}}, nil + }, + botInfoFunc: func(ctx context.Context) (*slack.AuthTestResponse, error) { + return &slack.AuthTestResponse{UserID: "B123"}, nil + }, + } + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + threadCache := cache.New() + + c := &Coordinator{ + slack: mockSlack, + configManager: mockConfig, + userMapper: &mockUserMapper{mapping: map[string]string{}}, + stateStore: state.NewMemoryStore(), + threadCache: threadCache, + } + + pr := struct { + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + HTMLURL string `json:"html_url"` + Title string `json:"title"` + Number int `json:"number"` + }{ + CreatedAt: time.Now().Add(-1 * time.Hour), + HTMLURL: "https://github.com/org/repo/pull/1", + Title: "Test PR", + Number: 1, + } + pr.User.Login = "author" + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + Merged: false, + Draft: false, + }, + Analysis: turn.Analysis{ + WorkflowState: "awaiting_review", + NextAction: map[string]turn.Action{}, + }, + } + + params := threadCreationParams{ + ChannelID: "C123", + ChannelName: "test-channel", + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRState: "awaiting_review", + PullRequest: pr, + CheckResult: checkResult, + } + + // Launch two concurrent thread creation attempts + var wg sync.WaitGroup + var info1, info2 cache.ThreadInfo + var wasCreated1, wasCreated2 bool + var err1, err2 error + + wg.Add(2) + + go func() { + defer wg.Done() + info1, wasCreated1, err1 = c.createPRThreadWithLocking(ctx, params) + }() + + go func() { + defer wg.Done() + // Add small delay to ensure second goroutine hits the concurrent creation path + time.Sleep(10 * time.Millisecond) + info2, wasCreated2, err2 = c.createPRThreadWithLocking(ctx, params) + }() + + wg.Wait() + + // Check results + if err1 != nil { + t.Errorf("goroutine 1 error: %v", err1) + } + if err2 != nil { + t.Errorf("goroutine 2 error: %v", err2) + } + + // Exactly one should have created the thread + createdCount := 0 + if wasCreated1 { + createdCount++ + } + if wasCreated2 { + createdCount++ + } + + if createdCount != 1 { + t.Errorf("expected exactly 1 goroutine to create thread, got %d", createdCount) + } + + // Both should have the same threadTS + if info1.ThreadTS != info2.ThreadTS { + t.Errorf("expected same threadTS, got %s and %s", info1.ThreadTS, info2.ThreadTS) + } + + // PostThread should only be called once + mu.Lock() + if postThreadCalls != 1 { + t.Errorf("expected PostThread to be called once, got %d calls", postThreadCalls) + } + mu.Unlock() +} + +// TestCreatePRThreadWithLocking_WaitThenProceed tests when waiting goroutine proceeds after other finishes without caching +func TestCreatePRThreadWithLocking_WaitThenProceed(t *testing.T) { + ctx := context.Background() + + var postThreadCalls int + var mu sync.Mutex + + mockSlack := &mockSlackClient{ + postThreadFunc: func(ctx context.Context, channelID, text string, attachments []slack.Attachment) (string, error) { + mu.Lock() + defer mu.Unlock() + postThreadCalls++ + return "1234.5678", nil + }, + channelHistoryFunc: func(ctx context.Context, channelID string, oldest, latest string, limit int) (*slack.GetConversationHistoryResponse, error) { + return &slack.GetConversationHistoryResponse{Messages: []slack.Message{}}, nil + }, + botInfoFunc: func(ctx context.Context) (*slack.AuthTestResponse, error) { + return &slack.AuthTestResponse{UserID: "B123"}, nil + }, + } + + mockConfig := NewMockConfig(). + WithDomain("example.com"). + Build() + + threadCache := cache.New() + + c := &Coordinator{ + slack: mockSlack, + configManager: mockConfig, + userMapper: &mockUserMapper{mapping: map[string]string{}}, + stateStore: state.NewMemoryStore(), + threadCache: threadCache, + } + + pr := struct { + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + HTMLURL string `json:"html_url"` + Title string `json:"title"` + Number int `json:"number"` + }{ + CreatedAt: time.Now().Add(-1 * time.Hour), + HTMLURL: "https://github.com/org/repo/pull/1", + Title: "Test PR", + Number: 1, + } + pr.User.Login = "author" + + checkResult := &turn.CheckResponse{ + PullRequest: prx.PullRequest{ + State: "open", + Merged: false, + Draft: false, + }, + Analysis: turn.Analysis{ + WorkflowState: "awaiting_review", + NextAction: map[string]turn.Action{}, + }, + } + + params := threadCreationParams{ + ChannelID: "C123", + ChannelName: "test-channel", + Owner: "org", + Repo: "repo", + PRNumber: 1, + PRState: "awaiting_review", + PullRequest: pr, + CheckResult: checkResult, + } + + cacheKey := "org/repo#1:C123" + + // Pre-mark as creating to simulate another goroutine + if !threadCache.MarkCreating(cacheKey) { + t.Fatal("failed to mark as creating") + } + + // Start goroutine that will unmark after delay (simulating failure without caching) + go func() { + time.Sleep(300 * time.Millisecond) + threadCache.UnmarkCreating(cacheKey) + }() + + // This should wait, then proceed to create + info, wasCreated, err := c.createPRThreadWithLocking(ctx, params) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !wasCreated { + t.Error("expected wasCreated to be true when proceeding after wait") + } + + if info.ThreadTS != "1234.5678" { + t.Errorf("expected threadTS 1234.5678, got %s", info.ThreadTS) + } + + mu.Lock() + if postThreadCalls != 1 { + t.Errorf("expected PostThread to be called once, got %d calls", postThreadCalls) + } + mu.Unlock() +} diff --git a/pkg/config/config.go b/pkg/config/config.go index 0261522..70581e5 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -42,6 +42,7 @@ type ServerConfig struct { type RepoConfig struct { Channels map[string]struct { ReminderDMDelay *int `yaml:"reminder_dm_delay"` + When *string `yaml:"when"` // Optional: when to post threads ("immediate", "assigned", "blocked", "passing") Repos []string `yaml:"repos"` // Optional: override global delay for this channel (0 = disabled) Mute bool `yaml:"mute"` @@ -51,6 +52,7 @@ type RepoConfig struct { TeamID string `yaml:"team_id"` EmailDomain string `yaml:"email_domain"` ReminderDMDelay int `yaml:"reminder_dm_delay"` + When string `yaml:"when"` // When to post threads: "immediate" (default), "assigned", "blocked", "passing" DisableDailyReport bool `yaml:"disable_daily_report"` // Default false (reports enabled) } `yaml:"global"` // Minutes to wait before sending DM if user tagged in channel (0 = disabled) } @@ -166,6 +168,7 @@ func createDefaultConfig() *RepoConfig { return &RepoConfig{ Channels: make(map[string]struct { ReminderDMDelay *int `yaml:"reminder_dm_delay"` + When *string `yaml:"when"` Repos []string `yaml:"repos"` Mute bool `yaml:"mute"` }), @@ -173,12 +176,14 @@ func createDefaultConfig() *RepoConfig { TeamID string `yaml:"team_id"` EmailDomain string `yaml:"email_domain"` ReminderDMDelay int `yaml:"reminder_dm_delay"` + When string `yaml:"when"` DisableDailyReport bool `yaml:"disable_daily_report"` }{ TeamID: "", EmailDomain: "", ReminderDMDelay: defaultReminderDMDelayMinutes, - DisableDailyReport: false, // Default: daily reports enabled + When: "immediate", // Default: post threads immediately + DisableDailyReport: false, // Default: daily reports enabled }, } } @@ -611,6 +616,32 @@ func (m *Manager) ReminderDMDelay(org, channel string) int { return defaultReminderDMDelayMinutes } +// When returns the posting threshold for a channel. +// Returns "immediate" (default), "assigned", "blocked", or "passing". +func (m *Manager) When(org, channel string) string { + m.mu.RLock() + defer m.mu.RUnlock() + + config, exists := m.configs[org] + if !exists { + return "immediate" // Default + } + + // Check for channel-specific override + if channelConfig, ok := config.Channels[channel]; ok { + if channelConfig.When != nil { + return *channelConfig.When + } + } + + // Return global setting (or default if not set) + if config.Global.When != "" { + return config.Global.When + } + + return "immediate" // Default +} + // ReloadConfig reloads the configuration for an org (e.g., when .codeGROOVE repo is updated). func (m *Manager) ReloadConfig(ctx context.Context, org string) error { slog.Info("reloading config", "org", org) diff --git a/pkg/config/config.test b/pkg/config/config.test new file mode 100755 index 0000000..6c7671e Binary files /dev/null and b/pkg/config/config.test differ diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index d7ec1a9..8914fe2 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -183,6 +183,7 @@ func TestConfigCache_GetSet(t *testing.T) { TeamID string `yaml:"team_id"` EmailDomain string `yaml:"email_domain"` ReminderDMDelay int `yaml:"reminder_dm_delay"` + When string `yaml:"when"` DisableDailyReport bool `yaml:"disable_daily_report"` }{ TeamID: "T123", @@ -386,6 +387,7 @@ func TestManager_ConfigWithManualSetup(t *testing.T) { testConfig := &RepoConfig{ Channels: map[string]struct { ReminderDMDelay *int `yaml:"reminder_dm_delay"` + When *string `yaml:"when"` Repos []string `yaml:"repos"` Mute bool `yaml:"mute"` }{ @@ -402,6 +404,7 @@ func TestManager_ConfigWithManualSetup(t *testing.T) { TeamID string `yaml:"team_id"` EmailDomain string `yaml:"email_domain"` ReminderDMDelay int `yaml:"reminder_dm_delay"` + When string `yaml:"when"` DisableDailyReport bool `yaml:"disable_daily_report"` }{ TeamID: "T123456", @@ -459,6 +462,7 @@ func TestManager_ReminderDMDelayWithChannelOverride(t *testing.T) { testConfig := &RepoConfig{ Channels: map[string]struct { ReminderDMDelay *int `yaml:"reminder_dm_delay"` + When *string `yaml:"when"` Repos []string `yaml:"repos"` Mute bool `yaml:"mute"` }{ @@ -471,6 +475,7 @@ func TestManager_ReminderDMDelayWithChannelOverride(t *testing.T) { TeamID string `yaml:"team_id"` EmailDomain string `yaml:"email_domain"` ReminderDMDelay int `yaml:"reminder_dm_delay"` + When string `yaml:"when"` DisableDailyReport bool `yaml:"disable_daily_report"` }{ ReminderDMDelay: 60, // Global default @@ -500,6 +505,7 @@ func TestManager_ChannelsForRepoWithWildcard(t *testing.T) { testConfig := &RepoConfig{ Channels: map[string]struct { ReminderDMDelay *int `yaml:"reminder_dm_delay"` + When *string `yaml:"when"` Repos []string `yaml:"repos"` Mute bool `yaml:"mute"` }{ @@ -561,6 +567,7 @@ func TestManager_ChannelsForRepoWithMuting(t *testing.T) { testConfig := &RepoConfig{ Channels: map[string]struct { ReminderDMDelay *int `yaml:"reminder_dm_delay"` + When *string `yaml:"when"` Repos []string `yaml:"repos"` Mute bool `yaml:"mute"` }{ @@ -727,6 +734,7 @@ func TestManager_IsChannelMutedCaseInsensitive(t *testing.T) { testConfig := &RepoConfig{ Channels: map[string]struct { ReminderDMDelay *int `yaml:"reminder_dm_delay"` + When *string `yaml:"when"` Repos []string `yaml:"repos"` Mute bool `yaml:"mute"` }{ @@ -759,6 +767,7 @@ func TestManager_ReminderDMDelayZeroGlobal(t *testing.T) { testConfig := &RepoConfig{ Channels: map[string]struct { ReminderDMDelay *int `yaml:"reminder_dm_delay"` + When *string `yaml:"when"` Repos []string `yaml:"repos"` Mute bool `yaml:"mute"` }{}, @@ -766,6 +775,7 @@ func TestManager_ReminderDMDelayZeroGlobal(t *testing.T) { TeamID string `yaml:"team_id"` EmailDomain string `yaml:"email_domain"` ReminderDMDelay int `yaml:"reminder_dm_delay"` + When string `yaml:"when"` DisableDailyReport bool `yaml:"disable_daily_report"` }{ ReminderDMDelay: 0, // Explicitly disabled @@ -790,6 +800,7 @@ func TestManager_ReminderDMDelayChannelZero(t *testing.T) { testConfig := &RepoConfig{ Channels: map[string]struct { ReminderDMDelay *int `yaml:"reminder_dm_delay"` + When *string `yaml:"when"` Repos []string `yaml:"repos"` Mute bool `yaml:"mute"` }{ @@ -801,6 +812,7 @@ func TestManager_ReminderDMDelayChannelZero(t *testing.T) { TeamID string `yaml:"team_id"` EmailDomain string `yaml:"email_domain"` ReminderDMDelay int `yaml:"reminder_dm_delay"` + When string `yaml:"when"` DisableDailyReport bool `yaml:"disable_daily_report"` }{ ReminderDMDelay: 60, diff --git a/pkg/dailyreport/report_test.go b/pkg/dailyreport/report_test.go index 1a15173..eb48cbc 100644 --- a/pkg/dailyreport/report_test.go +++ b/pkg/dailyreport/report_test.go @@ -258,3 +258,258 @@ type testError struct { func (e *testError) Error() string { return e.msg } + +func TestRandomGreeting_MorningTime(t *testing.T) { + // Test morning greetings (6am-12pm) + // We can't control time.Now() directly without complex mocking, + // but we can at least call the function to ensure no panics + greeting := randomGreeting() + if greeting == "" { + t.Error("Expected non-empty greeting") + } +} + +func TestRandomGreeting_Variety(t *testing.T) { + // Call randomGreeting multiple times + // It should return consistent results for the same time + greeting1 := randomGreeting() + greeting2 := randomGreeting() + + // Should be consistent within the same minute + if greeting1 != greeting2 { + t.Error("Expected consistent greeting within same minute") + } + + if len(greeting1) == 0 { + t.Error("Expected non-empty greeting") + } +} + +func TestSendReport_RecordError(t *testing.T) { + // Test when RecordReportSent fails + store := &mockStateStoreWithError{ + recordErr: &testError{msg: "record error"}, + } + slackClient := &mockSlackClient{ + timezone: "America/New_York", + isActive: true, + } + sender := NewSender(store, slackClient) + + userInfo := UserBlockingInfo{ + GitHubUsername: "testuser", + SlackUserID: "U123", + IncomingPRs: []home.PR{ + {Title: "Test PR", URL: "https://github.com/org/repo/pull/1"}, + }, + } + + // Should succeed even if recording fails + err := sender.SendReport(context.Background(), userInfo) + if err != nil { + t.Fatalf("Expected no error (recording failure should not fail send), got: %v", err) + } + + // Verify message was still sent + if len(slackClient.sentBlocks) != 1 { + t.Errorf("Expected 1 block set sent, got %d", len(slackClient.sentBlocks)) + } +} + +func TestSendReport_SlackError(t *testing.T) { + store := newMockStateStore() + slackClient := &mockSlackClientWithError{ + sendErr: &testError{msg: "slack error"}, + } + sender := NewSender(store, slackClient) + + userInfo := UserBlockingInfo{ + GitHubUsername: "testuser", + SlackUserID: "U123", + IncomingPRs: []home.PR{ + {Title: "Test PR", URL: "https://github.com/org/repo/pull/1"}, + }, + } + + err := sender.SendReport(context.Background(), userInfo) + if err == nil { + t.Error("Expected error when Slack send fails") + } +} + +func TestBuildReportBlocks(t *testing.T) { + incoming := []home.PR{ + { + Title: "Incoming PR", + URL: "https://github.com/org/repo/pull/1", + UpdatedAt: time.Now().Add(-1 * time.Hour), + ActionKind: "review", + ActionReason: "needs review", + }, + } + outgoing := []home.PR{ + { + Title: "Outgoing PR", + URL: "https://github.com/org/repo/pull/2", + UpdatedAt: time.Now().Add(-2 * time.Hour), + IsBlocked: true, + ActionKind: "fix", + ActionReason: "tests failing", + }, + } + + blocks := BuildReportBlocks(incoming, outgoing) + + if len(blocks) == 0 { + t.Error("Expected non-empty blocks") + } + + // First block should be the greeting + if len(blocks) < 1 { + t.Fatal("Expected at least 1 block (greeting)") + } +} + +func TestBuildReportBlocks_EmptyPRs(t *testing.T) { + blocks := BuildReportBlocks([]home.PR{}, []home.PR{}) + + // Should at least have greeting block + if len(blocks) == 0 { + t.Error("Expected non-empty blocks even with no PRs") + } +} + +// mockStateStoreWithError implements StateStore for testing error paths. +type mockStateStoreWithError struct { + lastSent map[string]time.Time + recordErr error +} + +func (m *mockStateStoreWithError) LastReportSent(_ context.Context, userID string) (time.Time, bool) { + if m.lastSent == nil { + return time.Time{}, false + } + t, exists := m.lastSent[userID] + return t, exists +} + +func (m *mockStateStoreWithError) RecordReportSent(_ context.Context, userID string, sentAt time.Time) error { + return m.recordErr +} + +// mockSlackClientWithError implements SlackClient for testing error paths. +type mockSlackClientWithError struct { + sendErr error +} + +func (m *mockSlackClientWithError) SendDirectMessageWithBlocks(_ context.Context, userID string, blocks []slack.Block) (dmChannelID, messageTS string, err error) { + return "", "", m.sendErr +} + +func (m *mockSlackClientWithError) UserTimezone(_ context.Context, userID string) (string, error) { + return "America/New_York", nil +} + +func (m *mockSlackClientWithError) IsUserActive(_ context.Context, userID string) bool { + return true +} + +func TestShouldSendReport_WithOutgoingPRsOnly(t *testing.T) { + store := newMockStateStore() + slackClient := &mockSlackClient{ + timezone: "America/New_York", + isActive: true, + } + sender := NewSender(store, slackClient) + + userInfo := UserBlockingInfo{ + GitHubUsername: "testuser", + SlackUserID: "U123", + IncomingPRs: []home.PR{}, + OutgoingPRs: []home.PR{{Title: "Test PR"}}, + } + + // Should process outgoing PRs + _ = sender.ShouldSendReport(context.Background(), userInfo) +} + +func TestShouldSendReport_OldReport(t *testing.T) { + store := newMockStateStore() + // Simulate sent 24 hours ago (more than 23 hours) + store.lastSent["U123"] = time.Now().Add(-24 * time.Hour) + + slackClient := &mockSlackClient{ + timezone: "America/New_York", + isActive: true, + } + sender := NewSender(store, slackClient) + + userInfo := UserBlockingInfo{ + GitHubUsername: "testuser", + SlackUserID: "U123", + IncomingPRs: []home.PR{{Title: "Test PR"}}, + OutgoingPRs: []home.PR{}, + } + + // Should allow sending since > 23 hours + _ = sender.ShouldSendReport(context.Background(), userInfo) +} + +func TestBuildReportBlocks_WithBothPRTypes(t *testing.T) { + incoming := []home.PR{ + { + Title: "PR to review", + URL: "https://github.com/org/repo/pull/1", + UpdatedAt: time.Now().Add(-1 * time.Hour), + ActionKind: "review", + }, + { + Title: "Another PR to review", + URL: "https://github.com/org/repo/pull/3", + UpdatedAt: time.Now().Add(-3 * time.Hour), + ActionKind: "review", + }, + } + outgoing := []home.PR{ + { + Title: "My PR", + URL: "https://github.com/org/repo/pull/2", + UpdatedAt: time.Now().Add(-2 * time.Hour), + IsBlocked: true, + ActionKind: "fix", + }, + { + Title: "My other PR", + URL: "https://github.com/org/repo/pull/4", + UpdatedAt: time.Now().Add(-4 * time.Hour), + IsBlocked: false, + ActionKind: "merge", + }, + } + + blocks := BuildReportBlocks(incoming, outgoing) + + // Should have greeting + PR sections + if len(blocks) < 2 { + t.Error("Expected at least greeting and PR sections") + } +} + +func TestNewSender(t *testing.T) { + store := newMockStateStore() + slackClient := &mockSlackClient{} + + sender := NewSender(store, slackClient) + + if sender == nil { + t.Fatal("Expected non-nil sender") + } + + if sender.stateStore != store { + t.Error("Expected state store to be set") + } + + if sender.slackClient != slackClient { + t.Error("Expected slack client to be set") + } +} diff --git a/pkg/github/github_test.go b/pkg/github/github_test.go index 5f38a8e..09a2595 100644 --- a/pkg/github/github_test.go +++ b/pkg/github/github_test.go @@ -1569,3 +1569,635 @@ func TestNewManager_WeakRSAKey(t *testing.T) { t.Errorf("expected weak key error, got: %v", err) } } + +func TestNewManager_PKCS8WeakKey(t *testing.T) { + ctx := context.Background() + // Generate a weak key in PKCS8 format + weakKey, err := rsa.GenerateKey(rand.Reader, 1024) + if err != nil { + t.Fatalf("failed to generate weak key: %v", err) + } + + // Encode as PKCS8 + keyBytes, err := x509.MarshalPKCS8PrivateKey(weakKey) + if err != nil { + t.Fatalf("failed to marshal PKCS8: %v", err) + } + + pemBlock := &pem.Block{ + Type: "PRIVATE KEY", + Bytes: keyBytes, + } + weakPEM := string(pem.EncodeToMemory(pemBlock)) + + _, err = NewManager(ctx, "123456", weakPEM, false) + if err == nil { + t.Error("expected error for weak PKCS8 RSA key, got nil") + } + if !strings.Contains(err.Error(), "RSA key too weak") { + t.Errorf("expected weak key error, got: %v", err) + } +} + +func TestNewManager_PKCS1SuccessPath(t *testing.T) { + ctx := context.Background() + // Generate valid RSA key + validKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("failed to generate key: %v", err) + } + + // Encode as PKCS1 + keyBytes := x509.MarshalPKCS1PrivateKey(validKey) + pemBlock := &pem.Block{ + Type: "RSA PRIVATE KEY", + Bytes: keyBytes, + } + validPEM := string(pem.EncodeToMemory(pemBlock)) + + // Will fail at RefreshInstallations but tests PKCS1 parse success + _, err = NewManager(ctx, "123456", validPEM, false) + if err == nil { + t.Error("expected error from RefreshInstallations without valid API") + } + // Should NOT be a parse error + if strings.Contains(err.Error(), "parse") && !strings.Contains(err.Error(), "discover") { + t.Errorf("unexpected parse error with valid PKCS1 key: %v", err) + } +} + +func TestNewManager_PKCS8SuccessPath(t *testing.T) { + ctx := context.Background() + // Generate valid RSA key + validKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("failed to generate key: %v", err) + } + + // Encode as PKCS8 + keyBytes, err := x509.MarshalPKCS8PrivateKey(validKey) + if err != nil { + t.Fatalf("failed to marshal PKCS8: %v", err) + } + + pemBlock := &pem.Block{ + Type: "PRIVATE KEY", + Bytes: keyBytes, + } + validPEM := string(pem.EncodeToMemory(pemBlock)) + + // Will fail at RefreshInstallations but tests PKCS8 parse success + _, err = NewManager(ctx, "123456", validPEM, false) + if err == nil { + t.Error("expected error from RefreshInstallations without valid API") + } + // Should NOT be a parse error + if strings.Contains(err.Error(), "parse") && !strings.Contains(err.Error(), "discover") { + t.Errorf("unexpected parse error with valid PKCS8 key: %v", err) + } +} + +func TestRefreshInstallations_WithMockServer(t *testing.T) { + // Generate valid RSA key + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("failed to generate key: %v", err) + } + + installationCallCount := 0 + tokenCallCount := 0 + + // Mock server for GitHub API + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // List installations endpoint + if strings.HasSuffix(r.URL.Path, "/app/installations") { + installationCallCount++ + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + resp := []map[string]any{ + { + "id": 123, + "account": map[string]any{ + "login": "test-org", + "type": "Organization", + }, + }, + { + "id": 456, + "account": map[string]any{ + "login": "another-org", + "type": "Organization", + }, + }, + } + //nolint:errcheck // Error intentionally ignored in test mock HTTP handler + _ = json.NewEncoder(w).Encode(resp) + return + } + + // Installation token endpoint + if strings.Contains(r.URL.Path, "/access_tokens") { + tokenCallCount++ + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + resp := map[string]any{ + "token": "ghs_test_token_" + fmt.Sprint(tokenCallCount), + "expires_at": time.Now().Add(1 * time.Hour).Format(time.RFC3339), + } + //nolint:errcheck // Error intentionally ignored in test mock HTTP handler + _ = json.NewEncoder(w).Encode(resp) + return + } + + http.NotFound(w, r) + })) + defer server.Close() + + m := &Manager{ + appID: "test-app", + privateKey: key, + clients: make(map[string]*Client), + baseURL: server.URL, + } + + err = m.RefreshInstallations(context.Background()) + + // Should succeed with mock server + if err != nil { + t.Fatalf("expected success with mock server, got error: %v", err) + } + + // Should have created clients for both orgs + if len(m.clients) != 2 { + t.Errorf("expected 2 clients, got %d", len(m.clients)) + } + + // Verify clients exist + if _, ok := m.clients["test-org"]; !ok { + t.Error("expected client for test-org") + } + if _, ok := m.clients["another-org"]; !ok { + t.Error("expected client for another-org") + } + + // Verify API calls + if installationCallCount != 1 { + t.Errorf("expected 1 installation list call, got %d", installationCallCount) + } + if tokenCallCount != 2 { + t.Errorf("expected 2 token calls (one per org), got %d", tokenCallCount) + } +} + +func TestRefreshInstallations_UnauthorizedError(t *testing.T) { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("failed to generate key: %v", err) + } + + // Mock server that returns 401 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasSuffix(r.URL.Path, "/app/installations") { + w.WriteHeader(http.StatusUnauthorized) + return + } + http.NotFound(w, r) + })) + defer server.Close() + + m := &Manager{ + appID: "test-app", + privateKey: key, + clients: make(map[string]*Client), + baseURL: server.URL, + } + + err = m.RefreshInstallations(context.Background()) + + // Should fail with unrecoverable error + if err == nil { + t.Error("expected error for 401 Unauthorized") + } +} + +func TestRefreshInstallations_InvalidBaseURL(t *testing.T) { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("failed to generate key: %v", err) + } + + m := &Manager{ + appID: "test-app", + privateKey: key, + clients: make(map[string]*Client), + baseURL: "://invalid-url", + } + + err = m.RefreshInstallations(context.Background()) + + // Should fail with URL parse error + if err == nil { + t.Error("expected error for invalid base URL") + } + if !strings.Contains(err.Error(), "invalid base URL") { + t.Errorf("expected invalid base URL error, got: %v", err) + } +} + +func TestInstallationToken_SuccessfulRefresh(t *testing.T) { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("failed to generate key: %v", err) + } + + // Mock server for installation token + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "/access_tokens") { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + resp := map[string]any{ + "token": "ghs_refreshed_token", + "expires_at": time.Now().Add(1 * time.Hour).Format(time.RFC3339), + } + //nolint:errcheck // Error intentionally ignored in test mock HTTP handler + _ = json.NewEncoder(w).Encode(resp) + return + } + http.NotFound(w, r) + })) + defer server.Close() + + c := &Client{ + appID: "test-app", + privateKey: key, + installationID: 123, + installationToken: "old-token", + tokenExpiry: time.Now().Add(-1 * time.Hour), // Expired + baseURL: server.URL, + } + + // Call InstallationToken with expired token + token := c.InstallationToken(context.Background()) + + // Should return the refreshed token + if token != "ghs_refreshed_token" { + t.Errorf("expected refreshed token 'ghs_refreshed_token', got %q", token) + } +} + +func TestInstallationToken_DoubleCheckLock(t *testing.T) { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("failed to generate key: %v", err) + } + + c := &Client{ + appID: "test-app", + privateKey: key, + installationID: 123, + installationToken: "initial-token", + tokenExpiry: time.Now().Add(-1 * time.Minute), // Expired + } + + // Simulate another goroutine refreshing the token + // We'll set a future expiry before the second goroutine acquires the lock + done := make(chan bool, 2) + var token1, token2 string + + // First goroutine - will refresh + go func() { + c.tokenMutex.Lock() + // Simulate refresh by setting new token and expiry + c.installationToken = "refreshed-by-goroutine-1" + c.tokenExpiry = time.Now().Add(1 * time.Hour) + c.tokenMutex.Unlock() + token1 = c.InstallationToken(context.Background()) + done <- true + }() + + // Small delay to ensure first goroutine runs first + time.Sleep(50 * time.Millisecond) + + // Second goroutine - should see the refreshed token (double-check lock path) + go func() { + token2 = c.InstallationToken(context.Background()) + done <- true + }() + + // Wait for both + <-done + <-done + + // Both should get the refreshed token + if token1 != "refreshed-by-goroutine-1" { + t.Errorf("goroutine 1 expected 'refreshed-by-goroutine-1', got %q", token1) + } + if token2 != "refreshed-by-goroutine-1" { + t.Errorf("goroutine 2 expected 'refreshed-by-goroutine-1', got %q", token2) + } +} + +func TestInstallationToken_ShortToken(t *testing.T) { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("failed to generate key: %v", err) + } + + // Test edge case where token is shorter than 10 characters + c := &Client{ + appID: "test-app", + privateKey: key, + installationID: 123, + installationToken: "short", + tokenExpiry: time.Now().Add(-1 * time.Hour), // Expired + } + + // This should not panic even though token is < 10 chars + // It will fail to refresh (no valid API) but should handle the short token safely + token := c.InstallationToken(context.Background()) + + // Should return the old token as fallback when refresh fails + if token != "short" { + t.Errorf("expected fallback to 'short', got %q", token) + } +} + +func TestRefreshInstallations_SkipPersonalAccount(t *testing.T) { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("failed to generate key: %v", err) + } + + // Mock server that returns a mix of org and user accounts + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasSuffix(r.URL.Path, "/app/installations") { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + resp := []map[string]any{ + { + "id": 123, + "account": map[string]any{ + "login": "test-org", + "type": "Organization", + }, + }, + { + "id": 456, + "account": map[string]any{ + "login": "personal-user", + "type": "User", + }, + }, + } + //nolint:errcheck // Error intentionally ignored in test mock HTTP handler + _ = json.NewEncoder(w).Encode(resp) + return + } + + // Installation token endpoint + if strings.Contains(r.URL.Path, "/access_tokens") { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + resp := map[string]any{ + "token": "ghs_test_token", + "expires_at": time.Now().Add(1 * time.Hour).Format(time.RFC3339), + } + //nolint:errcheck // Error intentionally ignored in test mock HTTP handler + _ = json.NewEncoder(w).Encode(resp) + return + } + + http.NotFound(w, r) + })) + defer server.Close() + + m := &Manager{ + appID: "test-app", + privateKey: key, + clients: make(map[string]*Client), + baseURL: server.URL, + allowPersonalAccounts: false, // Skip personal accounts + } + + err = m.RefreshInstallations(context.Background()) + if err != nil { + t.Fatalf("expected success, got error: %v", err) + } + + // Should only have the organization, not the personal account + if len(m.clients) != 1 { + t.Errorf("expected 1 client (org only), got %d", len(m.clients)) + } + + if _, ok := m.clients["test-org"]; !ok { + t.Error("expected client for test-org") + } + + if _, ok := m.clients["personal-user"]; ok { + t.Error("personal-user should have been skipped") + } +} + +func TestRefreshInstallations_AuthTimeout(t *testing.T) { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("failed to generate key: %v", err) + } + + // Mock server that returns installations + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasSuffix(r.URL.Path, "/app/installations") { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + resp := []map[string]any{ + { + "id": 123, + "account": map[string]any{ + "login": "test-org", + "type": "Organization", + }, + }, + } + //nolint:errcheck // Error intentionally ignored in test mock HTTP handler + _ = json.NewEncoder(w).Encode(resp) + return + } + + // Delay access token to allow context cancellation + if strings.Contains(r.URL.Path, "/access_tokens") { + time.Sleep(500 * time.Millisecond) + w.WriteHeader(http.StatusInternalServerError) + return + } + + http.NotFound(w, r) + })) + defer server.Close() + + m := &Manager{ + appID: "test-app", + privateKey: key, + clients: make(map[string]*Client), + baseURL: server.URL, + } + + // Use a context that will be canceled + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + err = m.RefreshInstallations(ctx) + + // Should complete but skip the org that couldn't authenticate + if err != nil { + t.Fatalf("expected success despite auth timeout, got: %v", err) + } + + // Should have no clients since authentication was canceled + if len(m.clients) != 0 { + t.Errorf("expected 0 clients (auth canceled), got %d", len(m.clients)) + } +} + +func TestRefreshInstallations_PreserveExistingClient(t *testing.T) { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("failed to generate key: %v", err) + } + + existingClient := &Client{ + organization: "test-org", + installationToken: "existing-token", + } + + // Mock server that returns error for token creation + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasSuffix(r.URL.Path, "/app/installations") { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + resp := []map[string]any{ + { + "id": 123, + "account": map[string]any{ + "login": "test-org", + "type": "Organization", + }, + }, + } + //nolint:errcheck // Error intentionally ignored in test mock HTTP handler + _ = json.NewEncoder(w).Encode(resp) + return + } + + // Return error for access token + if strings.Contains(r.URL.Path, "/access_tokens") { + w.WriteHeader(http.StatusInternalServerError) + return + } + + http.NotFound(w, r) + })) + defer server.Close() + + m := &Manager{ + appID: "test-app", + privateKey: key, + clients: map[string]*Client{ + "test-org": existingClient, + }, + baseURL: server.URL, + } + + err = m.RefreshInstallations(context.Background()) + if err != nil { + t.Fatalf("expected success with preserved client, got: %v", err) + } + + // Should preserve existing client when auth fails + if len(m.clients) != 1 { + t.Errorf("expected 1 client (preserved), got %d", len(m.clients)) + } + + client, ok := m.clients["test-org"] + if !ok { + t.Fatal("expected client for test-org to be preserved") + } + + if client != existingClient { + t.Error("expected same client instance to be preserved") + } +} + +func TestRefreshInstallations_RemoveUninstalledOrg(t *testing.T) { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("failed to generate key: %v", err) + } + + oldClient := &Client{ + organization: "old-org", + installationToken: "old-token", + } + + // Mock server that returns only new org + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasSuffix(r.URL.Path, "/app/installations") { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + resp := []map[string]any{ + { + "id": 123, + "account": map[string]any{ + "login": "new-org", + "type": "Organization", + }, + }, + } + //nolint:errcheck // Error intentionally ignored in test mock HTTP handler + _ = json.NewEncoder(w).Encode(resp) + return + } + + // Installation token endpoint + if strings.Contains(r.URL.Path, "/access_tokens") { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + resp := map[string]any{ + "token": "ghs_new_token", + "expires_at": time.Now().Add(1 * time.Hour).Format(time.RFC3339), + } + //nolint:errcheck // Error intentionally ignored in test mock HTTP handler + _ = json.NewEncoder(w).Encode(resp) + return + } + + http.NotFound(w, r) + })) + defer server.Close() + + m := &Manager{ + appID: "test-app", + privateKey: key, + clients: map[string]*Client{ + "old-org": oldClient, + }, + baseURL: server.URL, + } + + err = m.RefreshInstallations(context.Background()) + if err != nil { + t.Fatalf("expected success, got error: %v", err) + } + + // Should have removed old-org and added new-org + if len(m.clients) != 1 { + t.Errorf("expected 1 client (new-org), got %d", len(m.clients)) + } + + if _, ok := m.clients["old-org"]; ok { + t.Error("old-org should have been removed") + } + + if _, ok := m.clients["new-org"]; !ok { + t.Error("expected client for new-org") + } +} diff --git a/pkg/home/types_test.go b/pkg/home/types_test.go new file mode 100644 index 0000000..05b83c5 --- /dev/null +++ b/pkg/home/types_test.go @@ -0,0 +1,77 @@ +package home + +import ( + "testing" +) + +func TestDashboard_Counts(t *testing.T) { + dashboard := &Dashboard{ + IncomingPRs: []PR{ + { + Title: "PR 1", + NeedsReview: true, // Blocked incoming + }, + { + Title: "PR 2", + NeedsReview: false, + }, + { + Title: "PR 3", + NeedsReview: true, // Blocked incoming + }, + }, + OutgoingPRs: []PR{ + { + Title: "PR 4", + IsBlocked: true, // Blocked outgoing + }, + { + Title: "PR 5", + IsBlocked: false, + }, + }, + } + + counts := dashboard.Counts() + + if counts.IncomingTotal != 3 { + t.Errorf("expected 3 incoming PRs, got %d", counts.IncomingTotal) + } + + if counts.IncomingBlocked != 2 { + t.Errorf("expected 2 blocked incoming PRs, got %d", counts.IncomingBlocked) + } + + if counts.OutgoingTotal != 2 { + t.Errorf("expected 2 outgoing PRs, got %d", counts.OutgoingTotal) + } + + if counts.OutgoingBlocked != 1 { + t.Errorf("expected 1 blocked outgoing PR, got %d", counts.OutgoingBlocked) + } +} + +func TestDashboard_Counts_Empty(t *testing.T) { + dashboard := &Dashboard{ + IncomingPRs: []PR{}, + OutgoingPRs: []PR{}, + } + + counts := dashboard.Counts() + + if counts.IncomingTotal != 0 { + t.Errorf("expected 0 incoming PRs, got %d", counts.IncomingTotal) + } + + if counts.IncomingBlocked != 0 { + t.Errorf("expected 0 blocked incoming PRs, got %d", counts.IncomingBlocked) + } + + if counts.OutgoingTotal != 0 { + t.Errorf("expected 0 outgoing PRs, got %d", counts.OutgoingTotal) + } + + if counts.OutgoingBlocked != 0 { + t.Errorf("expected 0 blocked outgoing PRs, got %d", counts.OutgoingBlocked) + } +} diff --git a/pkg/home/ui.go b/pkg/home/ui.go index 947dce7..5e02f6a 100644 --- a/pkg/home/ui.go +++ b/pkg/home/ui.go @@ -12,14 +12,14 @@ import ( ) // BuildBlocks creates Slack Block Kit UI for the home dashboard. -// Design matches dashboard at https://ready-to-review.dev - modern minimal with indigo accents. +// Design matches dashboard at https://reviewgoose.dev - modern minimal with indigo accents. func BuildBlocks(dashboard *Dashboard, userTZ string) []slack.Block { var blocks []slack.Block // Header blocks = append(blocks, slack.NewHeaderBlock( - slack.NewTextBlockObject("plain_text", "🚀 Ready to Review", true, false), + slack.NewTextBlockObject("plain_text", "🚀 reviewGOOSE", true, false), ), // Refresh button slack.NewActionBlock( @@ -63,7 +63,7 @@ func BuildBlocks(dashboard *Dashboard, userTZ string) []slack.Block { esc := url.PathEscape(org) orgLine := fmt.Sprintf("• %s [<%s|dashboard> | <%s|config>]", org, - fmt.Sprintf("https://%s.ready-to-review.dev", esc), + fmt.Sprintf("https://reviewgoose.dev/orgs/%s", esc), fmt.Sprintf("https://github.com/%s/.codeGROOVE/blob/main/slack.yaml", esc), ) orgLines = append(orgLines, orgLine) @@ -235,9 +235,9 @@ func BuildBlocksWithDebug(dashboard *Dashboard, userTZ string, mapping *usermapp var blocks []slack.Block // Header with GitHub username if available - headerText := "🚀 Ready to Review" + headerText := "🚀 reviewGOOSE" if mapping != nil { - headerText = fmt.Sprintf("🚀 Ready to Review — @%s", mapping.GitHubUsername) + headerText = fmt.Sprintf("🚀 reviewGOOSE — @%s", mapping.GitHubUsername) } blocks = append(blocks, @@ -283,7 +283,7 @@ func BuildBlocksWithDebug(dashboard *Dashboard, userTZ string, mapping *usermapp esc := url.PathEscape(org) orgLine := fmt.Sprintf("• %s [<%s|dashboard> | <%s|config>]", org, - fmt.Sprintf("https://%s.ready-to-review.dev", esc), + fmt.Sprintf("https://reviewgoose.dev/orgs/%s", esc), fmt.Sprintf("https://github.com/%s/.codeGROOVE/blob/main/slack.yaml", esc), ) orgLines = append(orgLines, orgLine) diff --git a/pkg/home/ui_test.go b/pkg/home/ui_test.go index c5cd7eb..dc7e2fb 100644 --- a/pkg/home/ui_test.go +++ b/pkg/home/ui_test.go @@ -35,13 +35,13 @@ func TestBuildBlocks(t *testing.T) { foundHeader := false for _, block := range blocks { if hb, ok := block.(*slack.HeaderBlock); ok { - if strings.Contains(hb.Text.Text, "Ready to Review") { + if strings.Contains(hb.Text.Text, "reviewGOOSE") { foundHeader = true } } } if !foundHeader { - t.Error("expected header block with 'Ready to Review'") + t.Error("expected header block with 'reviewGOOSE'") } // Verify we don't have any PR section blocks (empty dashboard) @@ -74,7 +74,7 @@ func TestBuildBlocks(t *testing.T) { foundLink := false for _, block := range blocks { if sb, ok := block.(*slack.SectionBlock); ok { - if sb.Text != nil && strings.Contains(sb.Text.Text, "ready-to-review.dev") { + if sb.Text != nil && strings.Contains(sb.Text.Text, "reviewgoose.dev") { foundLink = true } } diff --git a/pkg/notify/format_test.go b/pkg/notify/format_test.go index 97b5e51..bfa3fbc 100644 --- a/pkg/notify/format_test.go +++ b/pkg/notify/format_test.go @@ -293,7 +293,7 @@ func TestFormatNextActionsInternal(t *testing.T) { nextActions: map[string]turn.Action{ "user1": {Kind: turn.ActionReview}, }, - expected: "review: @user1", + expected: "*review* → @user1", }, { name: "multiple users same action", @@ -301,7 +301,7 @@ func TestFormatNextActionsInternal(t *testing.T) { "user1": {Kind: turn.ActionReview}, "user2": {Kind: turn.ActionReview}, }, - expected: "review: @user1, @user2", + expected: "*review* → @user1, @user2", }, { name: "system user filtered out", @@ -316,7 +316,7 @@ func TestFormatNextActionsInternal(t *testing.T) { "_system": {Kind: turn.ActionReview}, "user1": {Kind: turn.ActionReview}, }, - expected: "review: @user1", + expected: "*review* → @user1", }, } @@ -650,7 +650,7 @@ func TestFormatNextActionsSuffixWithActions(t *testing.T) { Domain: "example.com", UserMapper: mapper, }, - expected: " → review: @user1", + expected: " • *review* → @user1", }, { name: "suffix with multiple users", @@ -667,7 +667,7 @@ func TestFormatNextActionsSuffixWithActions(t *testing.T) { Domain: "example.com", UserMapper: mapper, }, - expected: " → review: ", + expected: " • *review* → ", }, { name: "suffix filters system user", @@ -683,7 +683,7 @@ func TestFormatNextActionsSuffixWithActions(t *testing.T) { Domain: "example.com", UserMapper: mapper, }, - expected: " → review", + expected: " • review", }, } @@ -693,8 +693,8 @@ func TestFormatNextActionsSuffixWithActions(t *testing.T) { // For tests with multiple users, just check that result starts with the prefix // since map iteration order is not guaranteed if tt.name == "suffix with multiple users" { - if !contains(result, " → review: ") { - t.Errorf("expected result to contain \" → review: \", got: %q", result) + if !contains(result, " • *review* → ") { + t.Errorf("expected result to contain \" • *review* → \", got: %q", result) } } else if result != tt.expected { t.Errorf("expected %q, got %q", tt.expected, result) diff --git a/pkg/notify/notify.go b/pkg/notify/notify.go index 8a1a48d..8decbf8 100644 --- a/pkg/notify/notify.go +++ b/pkg/notify/notify.go @@ -126,7 +126,7 @@ func FormatNextActionsSuffix(ctx context.Context, params MessageParams) string { actions := formatNextActionsInternal(ctx, params.CheckResult.Analysis.NextAction, params.Owner, params.Domain, params.UserMapper) if actions != "" { - return fmt.Sprintf(" → %s", actions) + return fmt.Sprintf(" • %s", actions) } return "" } @@ -272,10 +272,10 @@ func formatNextActionsInternal(ctx context.Context, nextActions map[string]turn. // Format user mentions (will be empty if only _system was assigned) userMentions := userMapper.FormatUserMentions(ctx, users, owner, domain) - // If action has users, format as "action: users" - // If no users (was only _system), just show the action + // If action has users, format as "*action* → users" (bold action when assigned to person) + // If no users (was only _system), just show the action without bold if userMentions != "" { - parts = append(parts, fmt.Sprintf("%s: %s", actionName, userMentions)) + parts = append(parts, fmt.Sprintf("*%s* → %s", actionName, userMentions)) } else { parts = append(parts, actionName) } diff --git a/pkg/slack/additional_coverage_test.go b/pkg/slack/additional_coverage_test.go new file mode 100644 index 0000000..313f3ab --- /dev/null +++ b/pkg/slack/additional_coverage_test.go @@ -0,0 +1,217 @@ +package slack + +import ( + "bytes" + "context" + "io" + "net/http" + "net/http/httptest" + "testing" + + "github.com/codeGROOVE-dev/slacker/pkg/state" + "github.com/slack-go/slack" +) + +// TestVerifyRequest_ValidBody tests request verification with valid body. +func TestVerifyRequest_ValidBody(t *testing.T) { + t.Parallel() + + client := &Client{ + signingSecret: "test-secret", + } + + body := []byte("test body content") + req := httptest.NewRequest(http.MethodPost, "/test", bytes.NewReader(body)) + req.Header.Set("X-Slack-Request-Timestamp", "1234567890") + req.Header.Set("X-Slack-Signature", "v0=invalid") + + // This will fail verification but test the body reading logic + _ = client.verifyRequest(req) + + // Verify body can be read again + readBody, err := io.ReadAll(req.Body) + if err != nil { + t.Fatal("Failed to read body after verification:", err) + } + if !bytes.Equal(readBody, body) { + t.Error("Body was not properly restored") + } +} + +// TestVerifyRequest_EmptyBody tests request verification with empty body. +func TestVerifyRequest_EmptyBody(t *testing.T) { + t.Parallel() + + client := &Client{ + signingSecret: "test-secret", + } + + req := httptest.NewRequest(http.MethodPost, "/test", http.NoBody) + req.Header.Set("X-Slack-Request-Timestamp", "1234567890") + req.Header.Set("X-Slack-Signature", "v0=test") + + _ = client.verifyRequest(req) + // Test completes if it doesn't panic +} + +// TestUserInfo_Success tests getting user info successfully. +func TestUserInfo_Success(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + getUserInfoFunc: func(ctx context.Context, userID string) (*slack.User, error) { + return &slack.User{ + ID: userID, + Name: "testuser", + TZ: "America/Los_Angeles", + }, nil + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + user, err := client.UserInfo(context.Background(), "U123") + + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + if user.ID != "U123" { + t.Errorf("Expected user ID U123, got %s", user.ID) + } + + if user.Name != "testuser" { + t.Errorf("Expected user name testuser, got %s", user.Name) + } +} + +// TestUserTimezone_Valid tests getting user timezone. +func TestUserTimezone_Valid(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + getUserInfoFunc: func(ctx context.Context, userID string) (*slack.User, error) { + return &slack.User{ + ID: userID, + TZ: "America/Los_Angeles", + }, nil + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + tz, err := client.UserTimezone(context.Background(), "U123") + + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + if tz != "America/Los_Angeles" { + t.Errorf("Expected America/Los_Angeles, got %s", tz) + } +} + +// TestClient_SetManager tests setting the manager reference. +func TestClient_SetManager(t *testing.T) { + t.Parallel() + + client := &Client{} + manager := NewManager("test-secret") + + client.SetManager(manager) + + if client.manager != manager { + t.Error("Expected manager to be set") + } +} + +// TestClient_SetTeamID tests setting team ID. +func TestClient_SetTeamID(t *testing.T) { + t.Parallel() + + client := &Client{} + client.SetTeamID("T123") + + if client.teamID != "T123" { + t.Errorf("Expected team ID T123, got %s", client.teamID) + } +} + +// TestClient_SetStateStore tests setting state store. +func TestClient_SetStateStore(t *testing.T) { + t.Parallel() + + client := &Client{} + store := &state.MemoryStore{} + + client.SetStateStore(store) + + client.stateStoreMu.RLock() + gotStore := client.stateStore + client.stateStoreMu.RUnlock() + + if gotStore != store { + t.Error("Expected state store to be set") + } +} + +// TestClient_SetHomeViewHandler tests setting home view handler. +func TestClient_SetHomeViewHandler(t *testing.T) { + t.Parallel() + + client := &Client{} + called := false + handler := func(ctx context.Context, teamID, userID string) error { + called = true + return nil + } + + client.SetHomeViewHandler(handler) + + client.homeViewHandlerMu.RLock() + gotHandler := client.homeViewHandler + client.homeViewHandlerMu.RUnlock() + + if gotHandler == nil { + t.Fatal("Expected handler to be set") + } + + _ = gotHandler(context.Background(), "T123", "U123") + + if !called { + t.Error("Expected handler to be called") + } +} + +// TestWorkspaceMetadata_Fields tests metadata fields. +func TestWorkspaceMetadata_Fields(t *testing.T) { + t.Parallel() + + metadata := &WorkspaceMetadata{ + TeamID: "T123", + TeamName: "Test Team", + BotUserID: "UBOT123", + } + + if metadata.TeamID == "" { + t.Error("TeamID should not be empty") + } + + if metadata.TeamName == "" { + t.Error("TeamName should not be empty") + } + + if metadata.BotUserID == "" { + t.Error("BotUserID should not be empty") + } +} diff --git a/pkg/slack/coverage_improvement_test.go b/pkg/slack/coverage_improvement_test.go new file mode 100644 index 0000000..4ecb3b5 --- /dev/null +++ b/pkg/slack/coverage_improvement_test.go @@ -0,0 +1,570 @@ +package slack + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + "github.com/slack-go/slack" +) + +// testTime returns a fixed time for testing +func testTime() time.Time { + return time.Date(2024, 1, 1, 12, 0, 0, 0, time.UTC) +} + +// TestUserInfo_userNotFound tests the user_not_found error path +func TestUserInfo_userNotFound(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + getUserInfoFunc: func(ctx context.Context, userID string) (*slack.User, error) { + return nil, errors.New("user_not_found") + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + got, err := client.UserInfo(context.Background(), "U999") + + if err == nil { + t.Error("UserInfo(U999) = nil error, want error for user_not_found") + } + + if got != nil { + t.Errorf("UserInfo(U999) = %v, want nil for user_not_found", got) + } + + if !strings.Contains(err.Error(), "failed to get user info") { + t.Errorf("UserInfo(U999) error = %v, want error containing 'failed to get user info'", err) + } +} + +// TestUserPresence_userNotFound tests the user_not_found error path for presence +func TestUserPresence_userNotFound(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + getUserPresenceFunc: func(ctx context.Context, userID string) (*slack.UserPresence, error) { + return nil, errors.New("user_not_found") + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + got, err := client.UserPresence(context.Background(), "U999") + + if err == nil { + t.Error("UserPresence(U999) = nil error, want error for user_not_found") + } + + if got != "" { + t.Errorf("UserPresence(U999) = %q, want empty string for user_not_found", got) + } +} + +// TestWorkspaceInfo_error tests error handling in WorkspaceInfo +func TestWorkspaceInfo_error(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + getTeamInfoFunc: func(ctx context.Context) (*slack.TeamInfo, error) { + return nil, errors.New("api error") + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + got, err := client.WorkspaceInfo(context.Background()) + + if err == nil { + t.Error("WorkspaceInfo() = nil error, want error when API fails") + } + + if got != nil { + t.Errorf("WorkspaceInfo() = %v, want nil when API fails", got) + } +} + +// TestPostThread_emptyChannel tests PostThread with empty channel name +func TestPostThread_emptyChannel(t *testing.T) { + t.Parallel() + + client := &Client{ + api: &mockAPI{}, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + got, err := client.PostThread(context.Background(), "", "test message", nil) + + if err == nil { + t.Error("PostThread(\"\", \"test message\", nil) = nil error, want error for empty channel") + } + + if got != "" { + t.Errorf("PostThread(\"\", \"test message\", nil) = %q, want empty string on error", got) + } +} + +// TestUpdateMessage_error tests UpdateMessage error path +func TestUpdateMessage_error(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + updateMessageFunc: func(ctx context.Context, channelID, timestamp string, options ...slack.MsgOption) (string, string, string, error) { + return "", "", "", errors.New("update failed") + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + err := client.UpdateMessage(context.Background(), "C123", "123.456", "new text") + + if err == nil { + t.Error("UpdateMessage(C123, 123.456, \"new text\") = nil, want error when update fails") + } +} + +// TestPostThreadReply_error tests PostThreadReply error path +func TestPostThreadReply_error(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + postMessageFunc: func(ctx context.Context, channelID string, options ...slack.MsgOption) (string, string, error) { + return "", "", errors.New("post failed") + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + err := client.PostThreadReply(context.Background(), "C123", "123.456", "reply text") + + if err == nil { + t.Error("PostThreadReply(C123, 123.456, \"reply text\") = nil, want error when post fails") + } +} + +// TestSendDirectMessage_openConversationError tests error when opening DM fails +func TestSendDirectMessage_openConversationError(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + openConversationFunc: func(ctx context.Context, params *slack.OpenConversationParameters) (*slack.Channel, bool, bool, error) { + return nil, false, false, errors.New("failed to open conversation") + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + gotChannel, gotTS, err := client.SendDirectMessage(context.Background(), "U123", "test message") + + if err == nil { + t.Error("SendDirectMessage(U123, \"test message\") = nil error, want error when open conversation fails") + } + + if gotChannel != "" { + t.Errorf("SendDirectMessage(U123, \"test message\") channel = %q, want empty on error", gotChannel) + } + + if gotTS != "" { + t.Errorf("SendDirectMessage(U123, \"test message\") timestamp = %q, want empty on error", gotTS) + } +} + +// TestSendDirectMessageWithBlocks_openConversationError tests error path for blocks +func TestSendDirectMessageWithBlocks_openConversationError(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + openConversationFunc: func(ctx context.Context, params *slack.OpenConversationParameters) (*slack.Channel, bool, bool, error) { + return nil, false, false, errors.New("failed to open conversation") + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + blocks := []slack.Block{ + slack.NewSectionBlock( + slack.NewTextBlockObject("mrkdwn", "test", false, false), + nil, nil, + ), + } + + gotChannel, gotTS, err := client.SendDirectMessageWithBlocks(context.Background(), "U123", blocks) + + if err == nil { + t.Error("SendDirectMessageWithBlocks(U123, blocks) = nil error, want error when open conversation fails") + } + + if gotChannel != "" { + t.Errorf("SendDirectMessageWithBlocks(U123, blocks) channel = %q, want empty on error", gotChannel) + } + + if gotTS != "" { + t.Errorf("SendDirectMessageWithBlocks(U123, blocks) timestamp = %q, want empty on error", gotTS) + } +} + +// TestFindDMMessagesInHistory_openConversationError tests error when opening DM fails +func TestFindDMMessagesInHistory_openConversationError(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + openConversationFunc: func(ctx context.Context, params *slack.OpenConversationParameters) (*slack.Channel, bool, bool, error) { + return nil, false, false, errors.New("failed to open conversation") + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + got, err := client.FindDMMessagesInHistory(context.Background(), "U123", "test-pr-url", testTime()) + + if err == nil { + t.Error("FindDMMessagesInHistory(U123, \"test-pr-url\", time) = nil error, want error when open conversation fails") + } + + if len(got) != 0 { + t.Errorf("FindDMMessagesInHistory(U123, \"test-pr-url\", time) = %d results, want 0 on error", len(got)) + } +} + +// TestUpdateMessage_success tests successful message update +func TestUpdateMessage_success(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + updateMessageFunc: func(ctx context.Context, channelID, timestamp string, options ...slack.MsgOption) (string, string, string, error) { + return channelID, timestamp, "updated text", nil + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + err := client.UpdateMessage(context.Background(), "C123", "1234567890.123456", "Updated text") + + if err != nil { + t.Errorf("UpdateMessage(C123, 1234567890.123456) = %v, want nil", err) + } +} + +// TestPostThreadReply_success tests successful thread reply +func TestPostThreadReply_success(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + postMessageFunc: func(ctx context.Context, channelID string, options ...slack.MsgOption) (string, string, error) { + return channelID, "1234567890.123457", nil + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + err := client.PostThreadReply(context.Background(), "C123", "1234567890.123456", "Reply text") + + if err != nil { + t.Errorf("PostThreadReply(C123, 1234567890.123456) = %v, want nil", err) + } +} + +// TestUserInfo_genericErrorWithRetries tests retry logic for temporary errors +func TestUserInfo_genericErrorWithRetries(t *testing.T) { + t.Parallel() + + callCount := 0 + mockAPI := &mockAPI{ + getUserInfoFunc: func(ctx context.Context, userID string) (*slack.User, error) { + callCount++ + if callCount < 3 { + return nil, errors.New("temporary error") + } + return &slack.User{ID: userID}, nil + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + user, err := client.UserInfo(context.Background(), "U123") + + if err != nil { + t.Errorf("UserInfo(U123) after retries = %v, want nil", err) + } + + if user == nil || user.ID != "U123" { + t.Errorf("UserInfo(U123) returned user = %v, want user with ID U123", user) + } + + if callCount < 3 { + t.Errorf("UserInfo(U123) made %d calls, want at least 3 (with retries)", callCount) + } +} + +// TestUserPresence_genericErrorWithRetries tests retry logic for presence +func TestUserPresence_genericErrorWithRetries(t *testing.T) { + t.Parallel() + + callCount := 0 + mockAPI := &mockAPI{ + getUserPresenceFunc: func(ctx context.Context, userID string) (*slack.UserPresence, error) { + callCount++ + if callCount < 2 { + return nil, errors.New("temporary error") + } + return &slack.UserPresence{Presence: "active"}, nil + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + presence, err := client.UserPresence(context.Background(), "U123") + + if err != nil { + t.Errorf("UserPresence(U123) after retries = %v, want nil", err) + } + + if presence != "active" { + t.Errorf("UserPresence(U123) = %q, want \"active\"", presence) + } + + if callCount < 2 { + t.Errorf("UserPresence(U123) made %d calls, want at least 2 (with retries)", callCount) + } +} + +// TestIsUserInChannel_success tests checking if user is in channel +func TestIsUserInChannel_success(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + getUsersInConversationFunc: func(ctx context.Context, params *slack.GetUsersInConversationParameters) ([]string, string, error) { + return []string{"U123", "U456", "U789"}, "", nil + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + isIn := client.IsUserInChannel(context.Background(), "C123", "U456") + + if !isIn { + t.Error("IsUserInChannel(C123, U456) = false, want true") + } +} + +// TestIsUserInChannel_notInChannel tests user not in channel +func TestIsUserInChannel_notInChannel(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + getUsersInConversationFunc: func(ctx context.Context, params *slack.GetUsersInConversationParameters) ([]string, string, error) { + return []string{"U123", "U789"}, "", nil + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + isIn := client.IsUserInChannel(context.Background(), "C123", "U456") + + if isIn { + t.Error("IsUserInChannel(C123, U456) = true, want false") + } +} + +// TestIsBotInChannel_success tests checking if bot is in channel +func TestIsBotInChannel_success(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + authTestFunc: func(ctx context.Context) (*slack.AuthTestResponse, error) { + return &slack.AuthTestResponse{UserID: "UBOT123"}, nil + }, + getUsersInConversationFunc: func(ctx context.Context, params *slack.GetUsersInConversationParameters) ([]string, string, error) { + return []string{"U123", "UBOT123", "U456"}, "", nil + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + isIn := client.IsBotInChannel(context.Background(), "C123") + + if !isIn { + t.Error("IsBotInChannel(C123) = false, want true") + } +} + +// TestIsBotInChannel_notInChannel tests bot not in channel +func TestIsBotInChannel_notInChannel(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + authTestFunc: func(ctx context.Context) (*slack.AuthTestResponse, error) { + return &slack.AuthTestResponse{UserID: "UBOT123"}, nil + }, + getUsersInConversationFunc: func(ctx context.Context, params *slack.GetUsersInConversationParameters) ([]string, string, error) { + return []string{"U123", "U456"}, "", nil + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + isIn := client.IsBotInChannel(context.Background(), "C123") + + if isIn { + t.Error("IsBotInChannel(C123) = true, want false") + } +} + +// TestPostThread_success tests successful thread creation +func TestPostThread_success(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + authTestFunc: func(ctx context.Context) (*slack.AuthTestResponse, error) { + return &slack.AuthTestResponse{UserID: "UBOT123"}, nil + }, + getUsersInConversationFunc: func(ctx context.Context, params *slack.GetUsersInConversationParameters) ([]string, string, error) { + return []string{"U123", "UBOT123"}, "", nil + }, + postMessageFunc: func(ctx context.Context, channelID string, options ...slack.MsgOption) (string, string, error) { + return channelID, "1234567890.123456", nil + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + ts, err := client.PostThread(context.Background(), "C123", "Thread message", nil) + + if err != nil { + t.Errorf("PostThread(C123, ...) = %v, want nil", err) + } + + if ts != "1234567890.123456" { + t.Errorf("PostThread(C123, ...) = %q, want %q", ts, "1234567890.123456") + } +} + +// TestSendDirectMessage_success tests successful DM sending +func TestSendDirectMessage_success(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + openConversationFunc: func(ctx context.Context, params *slack.OpenConversationParameters) (*slack.Channel, bool, bool, error) { + return &slack.Channel{ + GroupConversation: slack.GroupConversation{ + Conversation: slack.Conversation{ID: "D123"}, + }, + }, false, false, nil + }, + postMessageFunc: func(ctx context.Context, channelID string, options ...slack.MsgOption) (string, string, error) { + return channelID, "1234567890.123456", nil + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + channel, ts, err := client.SendDirectMessage(context.Background(), "U123", "DM message") + + if err != nil { + t.Errorf("SendDirectMessage(U123, ...) = %v, want nil", err) + } + + if channel != "D123" { + t.Errorf("SendDirectMessage(U123, ...) channel = %q, want %q", channel, "D123") + } + + if ts != "1234567890.123456" { + t.Errorf("SendDirectMessage(U123, ...) ts = %q, want %q", ts, "1234567890.123456") + } +} diff --git a/pkg/slack/dm_additional_test.go b/pkg/slack/dm_additional_test.go new file mode 100644 index 0000000..c990d88 --- /dev/null +++ b/pkg/slack/dm_additional_test.go @@ -0,0 +1,388 @@ +package slack + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/slack-go/slack" +) + +// TestSendDirectMessageWithBlocks tests sending DM with blocks. +func TestSendDirectMessageWithBlocks(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + openConversationFunc: func(ctx context.Context, params *slack.OpenConversationParameters) (*slack.Channel, bool, bool, error) { + return &slack.Channel{ + GroupConversation: slack.GroupConversation{ + Conversation: slack.Conversation{ + ID: "D123456", + }, + }, + }, true, true, nil + }, + postMessageFunc: func(ctx context.Context, channelID string, options ...slack.MsgOption) (string, string, error) { + return channelID, "1234567890.123456", nil + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + blocks := []slack.Block{ + slack.NewSectionBlock( + slack.NewTextBlockObject("mrkdwn", "Test message", false, false), + nil, nil, + ), + } + + channelID, messageTS, err := client.SendDirectMessageWithBlocks(context.Background(), "U123", blocks) + + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + if channelID != "D123456" { + t.Errorf("Expected channel ID D123456, got %s", channelID) + } + + if messageTS == "" { + t.Error("Expected non-empty message timestamp") + } +} + +// TestSendDirectMessageWithBlocks_OpenConversationError tests error opening conversation. +func TestSendDirectMessageWithBlocks_OpenConversationError(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + openConversationFunc: func(ctx context.Context, params *slack.OpenConversationParameters) (*slack.Channel, bool, bool, error) { + return nil, false, false, errors.New("failed to open conversation") + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + blocks := []slack.Block{ + slack.NewSectionBlock( + slack.NewTextBlockObject("mrkdwn", "Test", false, false), + nil, nil, + ), + } + + _, _, err := client.SendDirectMessageWithBlocks(context.Background(), "U123", blocks) + + if err == nil { + t.Error("Expected error when opening conversation fails") + } +} + +// TestSendDirectMessageWithBlocks_PostMessageError tests error posting message. +func TestSendDirectMessageWithBlocks_PostMessageError(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + openConversationFunc: func(ctx context.Context, params *slack.OpenConversationParameters) (*slack.Channel, bool, bool, error) { + return &slack.Channel{ + GroupConversation: slack.GroupConversation{ + Conversation: slack.Conversation{ + ID: "D123456", + }, + }, + }, true, true, nil + }, + postMessageFunc: func(ctx context.Context, channelID string, options ...slack.MsgOption) (string, string, error) { + return "", "", errors.New("failed to post message") + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + blocks := []slack.Block{ + slack.NewSectionBlock( + slack.NewTextBlockObject("mrkdwn", "Test", false, false), + nil, nil, + ), + } + + _, _, err := client.SendDirectMessageWithBlocks(context.Background(), "U123", blocks) + + if err == nil { + t.Error("Expected error when posting message fails") + } +} + +// TestFindDMMessagesInHistory tests finding DM messages in history. +func TestFindDMMessagesInHistory(t *testing.T) { + t.Parallel() + + botUserID := "UBOT123" + prURL := "https://github.com/org/repo/pull/123" + + mockAPI := &mockAPI{ + openConversationFunc: func(ctx context.Context, params *slack.OpenConversationParameters) (*slack.Channel, bool, bool, error) { + return &slack.Channel{ + GroupConversation: slack.GroupConversation{ + Conversation: slack.Conversation{ + ID: "D123456", + }, + }, + }, true, true, nil + }, + getConversationHistoryFunc: func(ctx context.Context, params *slack.GetConversationHistoryParameters) (*slack.GetConversationHistoryResponse, error) { + return &slack.GetConversationHistoryResponse{ + Messages: []slack.Message{ + { + Msg: slack.Msg{ + User: botUserID, + Text: "Check out " + prURL, + Timestamp: "1234567890.123456", + }, + }, + { + Msg: slack.Msg{ + User: "UOTHER", + Text: "Some other message", + Timestamp: "1234567891.123456", + }, + }, + }, + HasMore: false, + }, nil + }, + } + + client := &Client{ + api: mockAPI, + teamID: "T123", + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + // Set bot info in cache + client.cache.set("bot_auth_test", &slack.AuthTestResponse{ + UserID: botUserID, + }, time.Hour) + + locations, err := client.FindDMMessagesInHistory( + context.Background(), + "U123", + prURL, + time.Now().Add(-24*time.Hour), + ) + + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + if len(locations) != 1 { + t.Errorf("Expected 1 location, got %d", len(locations)) + } + + if len(locations) > 0 { + if locations[0].ChannelID != "D123456" { + t.Errorf("Expected channel ID D123456, got %s", locations[0].ChannelID) + } + if locations[0].MessageTS != "1234567890.123456" { + t.Errorf("Expected timestamp 1234567890.123456, got %s", locations[0].MessageTS) + } + } +} + +// TestFindDMMessagesInHistory_OpenConversationError tests error opening conversation. +func TestFindDMMessagesInHistory_OpenConversationError(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + openConversationFunc: func(ctx context.Context, params *slack.OpenConversationParameters) (*slack.Channel, bool, bool, error) { + return nil, false, false, errors.New("failed to open conversation") + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + _, err := client.FindDMMessagesInHistory( + context.Background(), + "U123", + "https://github.com/org/repo/pull/123", + time.Now().Add(-24*time.Hour), + ) + + if err == nil { + t.Error("Expected error when opening conversation fails") + } +} + +// TestFindDMMessagesInHistory_BotInfoError tests error getting bot info. +func TestFindDMMessagesInHistory_BotInfoError(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + openConversationFunc: func(ctx context.Context, params *slack.OpenConversationParameters) (*slack.Channel, bool, bool, error) { + return &slack.Channel{ + GroupConversation: slack.GroupConversation{ + Conversation: slack.Conversation{ + ID: "D123456", + }, + }, + }, true, true, nil + }, + authTestFunc: func(ctx context.Context) (*slack.AuthTestResponse, error) { + return nil, errors.New("auth test failed") + }, + } + + client := &Client{ + api: mockAPI, + teamID: "T123", + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + _, err := client.FindDMMessagesInHistory( + context.Background(), + "U123", + "https://github.com/org/repo/pull/123", + time.Now().Add(-24*time.Hour), + ) + + if err == nil { + t.Error("Expected error when getting bot info fails") + } +} + +// TestFindDMMessagesInHistory_GetHistoryError tests error getting conversation history. +func TestFindDMMessagesInHistory_GetHistoryError(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + openConversationFunc: func(ctx context.Context, params *slack.OpenConversationParameters) (*slack.Channel, bool, bool, error) { + return &slack.Channel{ + GroupConversation: slack.GroupConversation{ + Conversation: slack.Conversation{ + ID: "D123456", + }, + }, + }, true, true, nil + }, + getConversationHistoryFunc: func(ctx context.Context, params *slack.GetConversationHistoryParameters) (*slack.GetConversationHistoryResponse, error) { + return nil, errors.New("failed to get history") + }, + } + + client := &Client{ + api: mockAPI, + teamID: "T123", + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + // Set bot info in cache + client.cache.set("bot_info", &slack.AuthTestResponse{ + UserID: "UBOT123", + }, time.Hour) + + _, err := client.FindDMMessagesInHistory( + context.Background(), + "U123", + "https://github.com/org/repo/pull/123", + time.Now().Add(-24*time.Hour), + ) + + if err == nil { + t.Error("Expected error when getting history fails") + } +} + +// TestFindDMMessagesInHistory_MultiplePagesNoPRURL tests pagination without finding PR URL. +func TestFindDMMessagesInHistory_MultiplePagesNoPRURL(t *testing.T) { + t.Parallel() + + botUserID := "UBOT123" + callCount := 0 + + mockAPI := &mockAPI{ + openConversationFunc: func(ctx context.Context, params *slack.OpenConversationParameters) (*slack.Channel, bool, bool, error) { + return &slack.Channel{ + GroupConversation: slack.GroupConversation{ + Conversation: slack.Conversation{ + ID: "D123456", + }, + }, + }, true, true, nil + }, + getConversationHistoryFunc: func(ctx context.Context, params *slack.GetConversationHistoryParameters) (*slack.GetConversationHistoryResponse, error) { + callCount++ + hasMore := callCount < 3 // Return 3 pages + + return &slack.GetConversationHistoryResponse{ + Messages: []slack.Message{ + { + Msg: slack.Msg{ + User: botUserID, + Text: "Some other message", + Timestamp: "1234567890.123456", + }, + }, + }, + HasMore: hasMore, + }, nil + }, + } + + client := &Client{ + api: mockAPI, + teamID: "T123", + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + // Set bot info in cache + client.cache.set("bot_auth_test", &slack.AuthTestResponse{ + UserID: botUserID, + }, time.Hour) + + locations, err := client.FindDMMessagesInHistory( + context.Background(), + "U123", + "https://github.com/org/repo/pull/999", + time.Now().Add(-24*time.Hour), + ) + + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + if len(locations) != 0 { + t.Errorf("Expected 0 locations, got %d", len(locations)) + } + + if callCount != 3 { + t.Errorf("Expected 3 API calls, got %d", callCount) + } +} diff --git a/pkg/slack/events_router_test.go b/pkg/slack/events_router_test.go index b7d158b..7601e49 100644 --- a/pkg/slack/events_router_test.go +++ b/pkg/slack/events_router_test.go @@ -269,3 +269,195 @@ func TestClientInteractionsHandlerNoDoubleVerification(t *testing.T) { // 3. The interaction was processed successfully t.Logf("✓ InteractionsHandler processed request without double signature verification") } +// TestHandleEvents tests the event routing handler. +func TestHandleEvents(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + body string + teamID string + eventType string + wantStatus int + wantError bool + }{ + { + name: "url_verification challenge", + body: `{"type":"url_verification","challenge":"test_challenge","token":"test_token"}`, + eventType: "url_verification", + wantStatus: http.StatusOK, + }, + { + name: "malformed JSON", + body: `{invalid json`, + wantStatus: http.StatusBadRequest, + wantError: true, + }, + { + name: "missing team_id", + body: `{"type":"event_callback","event":{"type":"app_home_opened"}}`, + wantStatus: http.StatusBadRequest, + wantError: true, + }, + { + name: "valid event with team_id", + body: `{"type":"event_callback","team_id":"T123","event":{"type":"message"}}`, + teamID: "T123", + eventType: "event_callback", + wantStatus: http.StatusUnauthorized, // Will fail signature check without proper setup + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + manager := NewManager("") + router := NewEventRouter(manager) + + // For tests that need a client, create one + if tt.teamID != "" { + client := &Client{ + signingSecret: "test-secret", + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + // Store the client in the manager + manager.mu.Lock() + manager.clients[tt.teamID] = client + manager.mu.Unlock() + } + + req := httptest.NewRequest(http.MethodPost, "/slack/events", strings.NewReader(tt.body)) + req.Header.Set("Content-Type", "application/json") + if tt.teamID != "" { + req.Header.Set("X-Slack-Signature", "v0=invalid") + req.Header.Set("X-Slack-Request-Timestamp", "1234567890") + } + + w := httptest.NewRecorder() + router.HandleEvents(w, req) + + if w.Code != tt.wantStatus { + t.Errorf("HandleEvents() status = %v, want %v", w.Code, tt.wantStatus) + } + + // For url_verification, check challenge is returned + if tt.eventType == "url_verification" { + body := w.Body.String() + if !strings.Contains(body, "test_challenge") { + t.Errorf("HandleEvents() body = %q, want challenge response", body) + } + } + }) + } +} + +// TestHandleEvents_ReadBodyError tests error handling when body read fails. +func TestHandleEvents_ReadBodyError(t *testing.T) { + t.Parallel() + + manager := NewManager("") + router := NewEventRouter(manager) + + req := httptest.NewRequest(http.MethodPost, "/slack/events", &errReader{}) + req.Header.Set("Content-Type", "application/json") + + w := httptest.NewRecorder() + router.HandleEvents(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("HandleEvents() status = %v, want %v", w.Code, http.StatusBadRequest) + } +} + +// TestHandleSlashCommand tests slash command routing. +func TestHandleSlashCommand(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + formData url.Values + teamID string + wantStatus int + }{ + { + name: "valid slash command", + formData: url.Values{ + "team_id": {"T123"}, + "command": {"/goose"}, + "text": {"help"}, + }, + teamID: "T123", + wantStatus: http.StatusUnauthorized, // Will fail signature check + }, + { + name: "missing team_id", + formData: url.Values{ + "command": {"/goose"}, + }, + wantStatus: http.StatusBadRequest, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + manager := NewManager("") + router := NewEventRouter(manager) + + // Register client if team_id is provided + if tt.teamID != "" { + client := &Client{ + signingSecret: "test-secret", + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + // Store the client in the manager + manager.mu.Lock() + manager.clients[tt.teamID] = client + manager.mu.Unlock() + } + + body := tt.formData.Encode() + req := httptest.NewRequest(http.MethodPost, "/slack/commands", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + if tt.teamID != "" { + req.Header.Set("X-Slack-Signature", "v0=invalid") + req.Header.Set("X-Slack-Request-Timestamp", "1234567890") + } + + w := httptest.NewRecorder() + router.HandleSlashCommand(w, req) + + if w.Code != tt.wantStatus { + t.Errorf("HandleSlashCommand() status = %v, want %v", w.Code, tt.wantStatus) + } + }) + } +} + +// TestHandleSlashCommand_ParseFormError tests form parsing errors. +func TestHandleSlashCommand_ParseFormError(t *testing.T) { + t.Parallel() + + manager := NewManager("") + router := NewEventRouter(manager) + + // Create request with invalid form data (missing Content-Type will cause parse error on some payloads) + req := httptest.NewRequest(http.MethodPost, "/slack/commands", strings.NewReader("invalid%form")) + // Don't set Content-Type to trigger parse error + + w := httptest.NewRecorder() + router.HandleSlashCommand(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("HandleSlashCommand() status = %v, want %v", w.Code, http.StatusBadRequest) + } +} + +// errReader is an io.Reader that always returns an error. +type errReader struct{} + +func (e *errReader) Read(p []byte) (n int, err error) { + return 0, fmt.Errorf("read error") +} diff --git a/pkg/slack/handlers_comprehensive_test.go b/pkg/slack/handlers_comprehensive_test.go new file mode 100644 index 0000000..b26bd96 --- /dev/null +++ b/pkg/slack/handlers_comprehensive_test.go @@ -0,0 +1,392 @@ +package slack + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/codeGROOVE-dev/slacker/pkg/config" + "github.com/codeGROOVE-dev/slacker/pkg/github" + "github.com/codeGROOVE-dev/slacker/pkg/state" + "github.com/codeGROOVE-dev/slacker/pkg/usermapping" + "github.com/slack-go/slack" +) + +func TestHomeHandler_HandleAppHomeOpened_noWorkspaceOrgs(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + getUserInfoFunc: func(ctx context.Context, userID string) (*slack.User, error) { + return &slack.User{ID: userID, TZ: "UTC"}, nil + }, + publishViewFunc: func(ctx context.Context, request slack.PublishViewContextRequest) (*slack.ViewResponse, error) { + return &slack.ViewResponse{}, nil + }, + } + + slackManager := NewManager("test-secret") + slackManager.mu.Lock() + slackManager.clients["T123"] = &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + slackManager.mu.Unlock() + + githubManager := newTestGitHubManager() + configManager := newTestConfigManager() + // No configs - no workspace orgs + + handler := &HomeHandler{ + slackManager: slackManager, + githubManager: githubManager, + configManager: configManager, + reverseMapping: newTestUserMapper(), + } + + err := handler.HandleAppHomeOpened(context.Background(), "T123", "U123") + + // Should succeed with placeholder view + if err != nil { + t.Errorf("HandleAppHomeOpened(T123, U123) with no workspace orgs = %v, want nil", err) + } +} + +func TestHomeHandler_HandleAppHomeOpened_invalidAuth_cacheInvalidation(t *testing.T) { + t.Parallel() + + callCount := 0 + mockAPI := &mockAPI{ + getUserInfoFunc: func(ctx context.Context, userID string) (*slack.User, error) { + return &slack.User{ID: userID, TZ: "UTC"}, nil + }, + publishViewFunc: func(ctx context.Context, request slack.PublishViewContextRequest) (*slack.ViewResponse, error) { + callCount++ + // Always return invalid_auth to test cache invalidation + return nil, errors.New("invalid_auth") + }, + } + + slackManager := NewManager("test-secret") + slackManager.mu.Lock() + slackManager.clients["T123"] = &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + slackManager.mu.Unlock() + + githubManager := newTestGitHubManager() + githubManager.addOrg("org1", &github.Client{}) + + configManager := newTestConfigManager() + cfg := &config.RepoConfig{} + cfg.Global.TeamID = "T123" + cfg.Global.EmailDomain = "example.com" + configManager.setConfig("org1", cfg) + + userMapper := newTestUserMapper() + userMapper.setLookupFunc(func(ctx context.Context, slackAPI usermapping.SlackAPI, slackUserID, org, emailDomain string) (*usermapping.ReverseMapping, error) { + // Fail to map user so we hit publishPlaceholderHome which calls publishView + return nil, errors.New("user not found") + }) + + handler := &HomeHandler{ + slackManager: slackManager, + githubManager: githubManager, + configManager: configManager, + stateStore: &state.MemoryStore{}, + reverseMapping: userMapper, + } + + err := handler.HandleAppHomeOpened(context.Background(), "T123", "U123") + + // Should detect invalid_auth and attempt retry, but will fail to get new client from GSM in test + if err == nil { + t.Error("HandleAppHomeOpened(T123, U123) with persistent invalid_auth = nil, want error") + } + + // Verify first attempt was made (callCount >= 1) + if callCount < 1 { + t.Errorf("HandleAppHomeOpened(T123, U123) made %d publishView calls, want at least 1", callCount) + } + + // Verify cache was invalidated by checking error message includes retry-related text + if !strings.Contains(err.Error(), "failed to get Slack client") && !strings.Contains(err.Error(), "failed to fetch token") { + t.Errorf("HandleAppHomeOpened(T123, U123) error = %v, want error indicating retry attempt", err) + } +} + +func TestHomeHandler_tryHandleAppHomeOpened_userMappingFailure(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + getUserInfoFunc: func(ctx context.Context, userID string) (*slack.User, error) { + return &slack.User{ID: userID, TZ: "UTC"}, nil + }, + publishViewFunc: func(ctx context.Context, request slack.PublishViewContextRequest) (*slack.ViewResponse, error) { + return &slack.ViewResponse{}, nil + }, + } + + slackManager := NewManager("test-secret") + slackManager.mu.Lock() + slackManager.clients["T123"] = &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + slackManager.mu.Unlock() + + githubManager := newTestGitHubManager() + githubManager.addOrg("org1", nil) + + configManager := newTestConfigManager() + cfg := &config.RepoConfig{} + cfg.Global.TeamID = "T123" + cfg.Global.EmailDomain = "example.com" + configManager.setConfig("org1", cfg) + + userMapper := newTestUserMapper() + userMapper.setLookupFunc(func(ctx context.Context, slackAPI usermapping.SlackAPI, slackUserID, org, emailDomain string) (*usermapping.ReverseMapping, error) { + // Fail to find user - will fall back to placeholder + return nil, errors.New("user not found") + }) + + handler := &HomeHandler{ + slackManager: slackManager, + githubManager: githubManager, + configManager: configManager, + stateStore: &state.MemoryStore{}, + reverseMapping: userMapper, + } + + err := handler.HandleAppHomeOpened(context.Background(), "T123", "U123") + + // Should succeed with placeholder + if err != nil { + t.Errorf("HandleAppHomeOpened(T123, U123) with failed user mapping = %v, want nil (placeholder)", err) + } +} + +func TestHomeHandler_workspaceOrgs_configWithOverrides(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + getUserInfoFunc: func(ctx context.Context, userID string) (*slack.User, error) { + return &slack.User{ID: userID, TZ: "UTC"}, nil + }, + publishViewFunc: func(ctx context.Context, request slack.PublishViewContextRequest) (*slack.ViewResponse, error) { + return &slack.ViewResponse{}, nil + }, + } + + slackManager := NewManager("test-secret") + slackManager.mu.Lock() + slackManager.clients["T123"] = &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + slackManager.mu.Unlock() + + githubManager := newTestGitHubManager() + githubManager.addOrg("org1", &github.Client{}) + + configManager := newTestConfigManager() + cfg := &config.RepoConfig{ + Users: map[string]string{ + "github-user-1": "user1@example.com", + "github-user-2": "user2@example.com", + }, + } + cfg.Global.TeamID = "T123" + cfg.Global.EmailDomain = "example.com" + configManager.setConfig("org1", cfg) + + overridesSet := false + userMapper := newTestUserMapper() + userMapper.setOverridesFunc = func(overrides map[string]string) { + overridesSet = true + if len(overrides) != 2 { + t.Errorf("SetOverrides called with %d overrides, want 2", len(overrides)) + } + } + userMapper.setLookupFunc(func(ctx context.Context, slackAPI usermapping.SlackAPI, slackUserID, org, emailDomain string) (*usermapping.ReverseMapping, error) { + return nil, errors.New("user not found") + }) + + handler := &HomeHandler{ + slackManager: slackManager, + githubManager: githubManager, + configManager: configManager, + stateStore: &state.MemoryStore{}, + reverseMapping: userMapper, + } + + _ = handler.HandleAppHomeOpened(context.Background(), "T123", "U123") + + if !overridesSet { + t.Error("HandleAppHomeOpened(T123, U123) did not call SetOverrides for config users") + } +} + +func TestReportHandler_HandleReportCommand_mockAPIClient(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + getTeamInfoFunc: func(ctx context.Context) (*slack.TeamInfo, error) { + return &slack.TeamInfo{ + ID: "T123", + Name: "Test Workspace", + Domain: "test-workspace", + }, nil + }, + } + + slackManager := NewManager("test-secret") + slackManager.mu.Lock() + slackManager.clients["T123"] = &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + slackManager.mu.Unlock() + + handler := &ReportHandler{ + slackManager: slackManager, + githubManager: newTestGitHubManager(), + stateStore: &state.MemoryStore{}, + reverseMapping: newTestUserMapper(), + } + + err := handler.HandleReportCommand(context.Background(), "T123", "U123") + + // Mock clients return nil from API(), which the handler should handle gracefully + if err == nil { + t.Error("HandleReportCommand(T123, U123) with mock client = nil, want error") + } + + expectedErr := "failed to get Slack API client" + if err != nil && !strings.Contains(err.Error(), expectedErr) { + t.Errorf("HandleReportCommand(T123, U123) error = %v, want error containing %q", err, expectedErr) + } +} + +func TestReportHandler_HandleReportCommand_differentGitHubUsernames(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + getTeamInfoFunc: func(ctx context.Context) (*slack.TeamInfo, error) { + return &slack.TeamInfo{ + ID: "T123", + Name: "Test Workspace", + Domain: "test-workspace", + }, nil + }, + } + + slackManager := NewManager("test-secret") + slackManager.mu.Lock() + slackManager.clients["T123"] = &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + slackManager.mu.Unlock() + + githubManager := newTestGitHubManager() + githubManager.addOrg("org1", nil) + githubManager.addOrg("org2", nil) + + userMapper := newTestUserMapper() + userMapper.setLookupFunc(func(ctx context.Context, slackAPI usermapping.SlackAPI, slackUserID, org, emailDomain string) (*usermapping.ReverseMapping, error) { + // Different GitHub username in each org (conflict scenario) + if org == "org1" { + return &usermapping.ReverseMapping{ + GitHubUsername: "user1", + MatchMethod: "email", + Confidence: 90, + }, nil + } + if org == "org2" { + return &usermapping.ReverseMapping{ + GitHubUsername: "user2", // Different username - should skip this org + MatchMethod: "email", + Confidence: 90, + }, nil + } + return nil, errors.New("user not found") + }) + + handler := &ReportHandler{ + slackManager: slackManager, + githubManager: githubManager, + stateStore: &state.MemoryStore{}, + reverseMapping: userMapper, + } + + err := handler.HandleReportCommand(context.Background(), "T123", "U123") + + // Will fail because API() returns nil for mocks + if err == nil { + t.Error("HandleReportCommand(T123, U123) = nil, want error (API returns nil for mocks)") + } +} + +func TestHomeHandler_tryHandleAppHomeOpened_noConfigForOrg(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + getUserInfoFunc: func(ctx context.Context, userID string) (*slack.User, error) { + return &slack.User{ID: userID, TZ: "UTC"}, nil + }, + publishViewFunc: func(ctx context.Context, request slack.PublishViewContextRequest) (*slack.ViewResponse, error) { + return &slack.ViewResponse{}, nil + }, + } + + slackManager := NewManager("test-secret") + slackManager.mu.Lock() + slackManager.clients["T123"] = &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + slackManager.mu.Unlock() + + githubManager := newTestGitHubManager() + githubManager.addOrg("org1", &github.Client{}) + + configManager := newTestConfigManager() + // Don't set config for org1 - will skip user mapping for this org + + userMapper := newTestUserMapper() + userMapper.setLookupFunc(func(ctx context.Context, slackAPI usermapping.SlackAPI, slackUserID, org, emailDomain string) (*usermapping.ReverseMapping, error) { + t.Error("LookupGitHub should not be called when org has no config") + return nil, errors.New("unexpected call") + }) + + handler := &HomeHandler{ + slackManager: slackManager, + githubManager: githubManager, + configManager: configManager, + stateStore: &state.MemoryStore{}, + reverseMapping: userMapper, + } + + err := handler.HandleAppHomeOpened(context.Background(), "T123", "U123") + + // Should succeed with placeholder (no orgs have config matching this workspace) + if err != nil { + t.Errorf("HandleAppHomeOpened(T123, U123) with no org configs = %v, want nil (placeholder)", err) + } +} diff --git a/pkg/slack/handlers_test.go b/pkg/slack/handlers_test.go new file mode 100644 index 0000000..0da96fd --- /dev/null +++ b/pkg/slack/handlers_test.go @@ -0,0 +1,388 @@ +package slack + +import ( + "context" + "errors" + "testing" + + "github.com/codeGROOVE-dev/slacker/pkg/config" + "github.com/codeGROOVE-dev/slacker/pkg/state" + "github.com/codeGROOVE-dev/slacker/pkg/usermapping" + "github.com/slack-go/slack" +) + +func TestNewHomeHandler(t *testing.T) { + t.Parallel() + + slackManager := NewManager("test-secret") + githubManager := newTestGitHubManager() + configManager := newTestConfigManager() + stateStore := &state.MemoryStore{} + userMapper := newTestUserMapper() + + got := NewHomeHandler(slackManager, githubManager, configManager, stateStore, userMapper) + + if got == nil { + t.Fatal("NewHomeHandler() = nil, want non-nil") + } + + if got.slackManager != slackManager { + t.Error("NewHomeHandler().slackManager incorrectly set") + } + if got.githubManager == nil { + t.Error("NewHomeHandler().githubManager = nil, want non-nil") + } + if got.configManager == nil { + t.Error("NewHomeHandler().configManager = nil, want non-nil") + } + if got.stateStore != stateStore { + t.Error("NewHomeHandler().stateStore incorrectly set") + } + if got.reverseMapping == nil { + t.Error("NewHomeHandler().reverseMapping = nil, want non-nil") + } +} + +func TestNewReportHandler(t *testing.T) { + t.Parallel() + + slackManager := NewManager("test-secret") + githubManager := newTestGitHubManager() + stateStore := &state.MemoryStore{} + userMapper := newTestUserMapper() + + got := NewReportHandler(slackManager, githubManager, stateStore, userMapper) + + if got == nil { + t.Fatal("NewReportHandler() = nil, want non-nil") + } + + if got.slackManager != slackManager { + t.Error("NewReportHandler().slackManager incorrectly set") + } + if got.githubManager == nil { + t.Error("NewReportHandler().githubManager = nil, want non-nil") + } + if got.stateStore != stateStore { + t.Error("NewReportHandler().stateStore incorrectly set") + } + if got.reverseMapping == nil { + t.Error("NewReportHandler().reverseMapping = nil, want non-nil") + } +} + +func TestHomeHandler_workspaceOrgs(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + teamID string + setup func(*testGitHubManager, *testConfigManager) + wantOrgs int + }{ + { + name: "multiple orgs same workspace", + teamID: "T123", + setup: func(gh *testGitHubManager, cm *testConfigManager) { + gh.addOrg("org1", nil) + gh.addOrg("org2", nil) + gh.addOrg("org3", nil) + + cfg1 := &config.RepoConfig{} + cfg1.Global.TeamID = "T123" + cm.setConfig("org1", cfg1) + + cfg2 := &config.RepoConfig{} + cfg2.Global.TeamID = "T123" + cm.setConfig("org2", cfg2) + + cfg3 := &config.RepoConfig{} + cfg3.Global.TeamID = "T456" + cm.setConfig("org3", cfg3) + }, + wantOrgs: 2, + }, + { + name: "no matching orgs", + teamID: "T999", + setup: func(gh *testGitHubManager, cm *testConfigManager) { + gh.addOrg("org1", nil) + + cfg1 := &config.RepoConfig{} + cfg1.Global.TeamID = "T123" + cm.setConfig("org1", cfg1) + }, + wantOrgs: 0, + }, + { + name: "empty configuration", + teamID: "T123", + setup: func(gh *testGitHubManager, cm *testConfigManager) { + // No setup + }, + wantOrgs: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + githubManager := newTestGitHubManager() + configManager := newTestConfigManager() + + if tt.setup != nil { + tt.setup(githubManager, configManager) + } + + handler := &HomeHandler{ + githubManager: githubManager, + configManager: configManager, + } + + got := handler.workspaceOrgs(tt.teamID) + + if len(got) != tt.wantOrgs { + t.Errorf("workspaceOrgs(%q) returned %d orgs, want %d", tt.teamID, len(got), tt.wantOrgs) + } + }) + } +} + +func TestHomeHandler_publishPlaceholderHome(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + userID string + setupMock func(*mockAPI) + wantErr bool + }{ + { + name: "success with timezone", + userID: "U123", + setupMock: func(m *mockAPI) { + m.getUserInfoFunc = func(ctx context.Context, userID string) (*slack.User, error) { + return &slack.User{ + ID: userID, + TZ: "America/Los_Angeles", + TZOffset: -28800, + }, nil + } + m.publishViewFunc = func(ctx context.Context, request slack.PublishViewContextRequest) (*slack.ViewResponse, error) { + return &slack.ViewResponse{}, nil + } + }, + wantErr: false, + }, + { + name: "timezone error defaults to UTC", + userID: "U456", + setupMock: func(m *mockAPI) { + m.getUserInfoFunc = func(ctx context.Context, userID string) (*slack.User, error) { + return nil, errors.New("failed to get user info") + } + m.publishViewFunc = func(ctx context.Context, request slack.PublishViewContextRequest) (*slack.ViewResponse, error) { + return &slack.ViewResponse{}, nil + } + }, + wantErr: false, + }, + { + name: "publish view error", + userID: "U789", + setupMock: func(m *mockAPI) { + m.getUserInfoFunc = func(ctx context.Context, userID string) (*slack.User, error) { + return &slack.User{ID: userID, TZ: "UTC"}, nil + } + m.publishViewFunc = func(ctx context.Context, request slack.PublishViewContextRequest) (*slack.ViewResponse, error) { + return nil, errors.New("publish failed") + } + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{} + if tt.setupMock != nil { + tt.setupMock(mockAPI) + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + handler := &HomeHandler{} + + err := handler.publishPlaceholderHome( + context.Background(), + client, + tt.userID, + []string{"org1"}, + nil, + ) + + if (err != nil) != tt.wantErr { + t.Errorf("publishPlaceholderHome(%q) error = %v, wantErr = %v", tt.userID, err, tt.wantErr) + } + }) + } +} + +func TestHomeHandler_publishPlaceholderHome_withMapping(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + getUserInfoFunc: func(ctx context.Context, userID string) (*slack.User, error) { + return &slack.User{ID: userID, TZ: "UTC"}, nil + }, + publishViewFunc: func(ctx context.Context, request slack.PublishViewContextRequest) (*slack.ViewResponse, error) { + return &slack.ViewResponse{}, nil + }, + } + + client := &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + + handler := &HomeHandler{} + + mapping := &usermapping.ReverseMapping{ + GitHubUsername: "testuser", + MatchMethod: "email", + Confidence: 90, + } + + err := handler.publishPlaceholderHome( + context.Background(), + client, + "U123", + []string{"org1"}, + mapping, + ) + + if err != nil { + t.Errorf("publishPlaceholderHome(U123) with mapping = %v, want nil", err) + } +} + +func TestHomeHandler_HandleAppHomeOpened_clientError(t *testing.T) { + t.Parallel() + + slackManager := NewManager("test-secret") + // Don't register any clients - will cause error + + handler := &HomeHandler{ + slackManager: slackManager, + githubManager: newTestGitHubManager(), + configManager: newTestConfigManager(), + reverseMapping: newTestUserMapper(), + } + + err := handler.HandleAppHomeOpened(context.Background(), "T999", "U123") + + if err == nil { + t.Error("HandleAppHomeOpened(T999, U123) = nil, want error when client doesn't exist") + } +} + +func TestReportHandler_HandleReportCommand_clientError(t *testing.T) { + t.Parallel() + + slackManager := NewManager("test-secret") + // Don't register any clients - will cause error + + handler := &ReportHandler{ + slackManager: slackManager, + } + + err := handler.HandleReportCommand(context.Background(), "T999", "U123") + + if err == nil { + t.Error("HandleReportCommand(T999, U123) = nil, want error when client doesn't exist") + } +} + +func TestReportHandler_HandleReportCommand_workspaceInfoError(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + getTeamInfoFunc: func(ctx context.Context) (*slack.TeamInfo, error) { + return nil, errors.New("failed to get team info") + }, + } + + slackManager := NewManager("test-secret") + slackManager.mu.Lock() + slackManager.clients["T123"] = &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + slackManager.mu.Unlock() + + handler := &ReportHandler{ + slackManager: slackManager, + } + + err := handler.HandleReportCommand(context.Background(), "T123", "U123") + + if err == nil { + t.Error("HandleReportCommand(T123, U123) = nil, want error when workspace info fails") + } +} + +func TestReportHandler_HandleReportCommand_noGitHubUsername(t *testing.T) { + t.Parallel() + + mockAPI := &mockAPI{ + getTeamInfoFunc: func(ctx context.Context) (*slack.TeamInfo, error) { + return &slack.TeamInfo{ + ID: "T123", + Name: "Test Workspace", + Domain: "test-workspace", + }, nil + }, + } + + slackManager := NewManager("test-secret") + slackManager.mu.Lock() + slackManager.clients["T123"] = &Client{ + api: mockAPI, + cache: &apiCache{ + entries: make(map[string]cacheEntry), + }, + } + slackManager.mu.Unlock() + + githubManager := newTestGitHubManager() + githubManager.addOrg("org1", nil) + + userMapper := newTestUserMapper() + userMapper.setLookupFunc(func(ctx context.Context, slackAPI usermapping.SlackAPI, slackUserID, org, emailDomain string) (*usermapping.ReverseMapping, error) { + return nil, errors.New("user not found") + }) + + handler := &ReportHandler{ + slackManager: slackManager, + githubManager: githubManager, + reverseMapping: userMapper, + } + + err := handler.HandleReportCommand(context.Background(), "T123", "U123") + + if err == nil { + t.Error("HandleReportCommand(T123, U123) = nil, want error when GitHub username not found") + } +} diff --git a/pkg/slack/home_handler.go b/pkg/slack/home_handler.go index 2c2b9a1..a698f34 100644 --- a/pkg/slack/home_handler.go +++ b/pkg/slack/home_handler.go @@ -7,8 +7,6 @@ import ( "log/slog" "strings" - "github.com/codeGROOVE-dev/slacker/pkg/config" - "github.com/codeGROOVE-dev/slacker/pkg/github" "github.com/codeGROOVE-dev/slacker/pkg/home" "github.com/codeGROOVE-dev/slacker/pkg/state" "github.com/codeGROOVE-dev/slacker/pkg/usermapping" @@ -18,19 +16,19 @@ import ( // HomeHandler handles app_home_opened events for a workspace. type HomeHandler struct { slackManager *Manager - githubManager *github.Manager - configManager *config.Manager + githubManager GitHubManager + configManager ConfigManager stateStore state.Store - reverseMapping *usermapping.ReverseService + reverseMapping UserMapper } // NewHomeHandler creates a new home view handler. func NewHomeHandler( slackManager *Manager, - githubManager *github.Manager, - configManager *config.Manager, + githubManager GitHubManager, + configManager ConfigManager, stateStore state.Store, - reverseMapping *usermapping.ReverseService, + reverseMapping UserMapper, ) *HomeHandler { return &HomeHandler{ slackManager: slackManager, @@ -88,23 +86,62 @@ func (h *HomeHandler) tryHandleAppHomeOpened(ctx context.Context, teamID, slackU return h.publishPlaceholderHome(ctx, slackClient, slackUserID, []string{}, nil) } - // Get config for first org to extract domain and user overrides - cfg, exists := h.configManager.Config(workspaceOrgs[0]) - if !exists { - return fmt.Errorf("no config for org: %s", workspaceOrgs[0]) + // Collect config overrides from all workspace orgs + allOverrides := make(map[string]string) + for _, org := range workspaceOrgs { + cfg, exists := h.configManager.Config(org) + if exists && len(cfg.Users) > 0 { + for ghUser, email := range cfg.Users { + allOverrides[ghUser] = email + } + } } - - // Update reverse mapping overrides from config - if len(cfg.Users) > 0 { - h.reverseMapping.SetOverrides(cfg.Users) + if len(allOverrides) > 0 { + h.reverseMapping.SetOverrides(allOverrides) + slog.Info("loaded user mapping overrides from all orgs", + "workspace_orgs", workspaceOrgs, + "total_overrides", len(allOverrides)) } - // Map Slack user to GitHub username - mapping, err := h.reverseMapping.LookupGitHub(ctx, slackClient.API(), slackUserID, workspaceOrgs[0], cfg.Global.EmailDomain) - if err != nil { - slog.Warn("failed to map Slack user to GitHub", + // Try to map Slack user to GitHub username using all workspace orgs + var mapping *usermapping.ReverseMapping + var lastErr error + for _, org := range workspaceOrgs { + cfg, exists := h.configManager.Config(org) + if !exists { + slog.Warn("no config found for org, skipping", + "org", org, + "slack_user_id", slackUserID) + continue + } + + slog.Debug("attempting user mapping with org", + "org", org, + "slack_user_id", slackUserID, + "email_domain", cfg.Global.EmailDomain) + + m, err := h.reverseMapping.LookupGitHub(ctx, slackClient.API(), slackUserID, org, cfg.Global.EmailDomain) + if err == nil && m != nil { + slog.Info("successfully mapped user via org", + "org", org, + "slack_user_id", slackUserID, + "github_username", m.GitHubUsername, + "confidence", m.Confidence) + mapping = m + break + } + lastErr = err + slog.Debug("mapping attempt failed for org", + "org", org, "slack_user_id", slackUserID, "error", err) + } + + if mapping == nil { + slog.Warn("failed to map Slack user to GitHub in any workspace org", + "slack_user_id", slackUserID, + "workspace_orgs", workspaceOrgs, + "last_error", lastErr) return h.publishPlaceholderHome(ctx, slackClient, slackUserID, workspaceOrgs, nil) } @@ -125,7 +162,7 @@ func (h *HomeHandler) tryHandleAppHomeOpened(ctx context.Context, teamID, slackU ghClient, h.stateStore, githubClient.InstallationToken(ctx), - "ready-to-review[bot]", + "reviewgoose[bot]", ) dashboard, err := fetcher.FetchDashboard(ctx, githubUsername, workspaceOrgs) diff --git a/pkg/slack/home_handler_multiorg_test.go b/pkg/slack/home_handler_multiorg_test.go new file mode 100644 index 0000000..a88329b --- /dev/null +++ b/pkg/slack/home_handler_multiorg_test.go @@ -0,0 +1,242 @@ +package slack + +import ( + "testing" + + "github.com/codeGROOVE-dev/slacker/pkg/config" +) + +// TestWorkspaceOrgs_MultipleOrgs verifies that workspaceOrgs correctly identifies all orgs for a workspace. +func TestWorkspaceOrgs_MultipleOrgs(t *testing.T) { + t.Parallel() + + // Create mock config manager with multiple orgs for same workspace + teamID := "T123" + configs := map[string]*config.RepoConfig{ + "org1": { + Users: map[string]string{"gh-user1": "user1@example.com"}, + }, + "org2": { + Users: map[string]string{"gh-user2": "user2@example.com"}, + }, + "org3": { + Users: map[string]string{"gh-user3": "user3@example.com"}, + }, + "org4": { // Different teamID - should be excluded + Users: map[string]string{"gh-user4": "user4@example.com"}, + }, + } + + // Set TeamIDs + configs["org1"].Global.TeamID = teamID + configs["org2"].Global.TeamID = teamID + configs["org3"].Global.TeamID = teamID + configs["org4"].Global.TeamID = "T999" // Different workspace + + // Create a mock implementation that simulates the workspaceOrgs logic + allOrgs := []string{"org1", "org2", "org3", "org4"} + var workspaceOrgs []string + + for _, org := range allOrgs { + cfg, exists := configs[org] + if !exists { + continue + } + if cfg.Global.TeamID == teamID { + workspaceOrgs = append(workspaceOrgs, org) + } + } + + // Verify all matching orgs were found + if len(workspaceOrgs) != 3 { + t.Errorf("expected 3 workspace orgs, got %d", len(workspaceOrgs)) + } + + expectedOrgs := map[string]bool{ + "org1": true, + "org2": true, + "org3": true, + } + + for _, org := range workspaceOrgs { + if !expectedOrgs[org] { + t.Errorf("unexpected org in workspace: %s", org) + } + delete(expectedOrgs, org) + } + + if len(expectedOrgs) > 0 { + t.Errorf("missing expected orgs: %v", expectedOrgs) + } +} + +// TestCollectOverrides_MultipleOrgs verifies that user overrides are collected from all workspace orgs. +func TestCollectOverrides_MultipleOrgs(t *testing.T) { + t.Parallel() + + workspaceOrgs := []string{"org1", "org2", "org3"} + + // Simulate configs for multiple orgs + configs := map[string]*config.RepoConfig{ + "org1": { + Users: map[string]string{ + "github-user-1": "user1@example.com", + "github-user-2": "user2@example.com", + }, + }, + "org2": { + Users: map[string]string{ + "github-user-3": "user3@example.com", + }, + }, + "org3": { + Users: nil, // No overrides + }, + } + + // Simulate the override collection logic from tryHandleAppHomeOpened + allOverrides := make(map[string]string) + for _, org := range workspaceOrgs { + cfg, exists := configs[org] + if exists && len(cfg.Users) > 0 { + for ghUser, email := range cfg.Users { + allOverrides[ghUser] = email + } + } + } + + // Verify all overrides were collected + expected := map[string]string{ + "github-user-1": "user1@example.com", + "github-user-2": "user2@example.com", + "github-user-3": "user3@example.com", + } + + if len(allOverrides) != len(expected) { + t.Errorf("expected %d overrides, got %d", len(expected), len(allOverrides)) + } + + for ghUser, expectedEmail := range expected { + if actualEmail, ok := allOverrides[ghUser]; !ok { + t.Errorf("missing override for %s", ghUser) + } else if actualEmail != expectedEmail { + t.Errorf("wrong email for %s: expected %s, got %s", ghUser, expectedEmail, actualEmail) + } + } +} + +// TestUserMappingFallback_MultipleOrgs verifies that user mapping attempts all orgs until one succeeds. +func TestUserMappingFallback_MultipleOrgs(t *testing.T) { + t.Parallel() + + workspaceOrgs := []string{"org1", "org2", "org3"} + + // Simulate configs for multiple orgs + configs := map[string]*config.RepoConfig{ + "org1": {Users: nil}, + "org2": {Users: nil}, + "org3": {Users: nil}, + } + configs["org1"].Global.EmailDomain = "example.com" + configs["org2"].Global.EmailDomain = "example.org" + configs["org3"].Global.EmailDomain = "example.net" + + // Simulate the user mapping loop logic + type attempt struct { + org string + domain string + } + var attempts []attempt + + // Simulate mapping attempts (fail on first two, succeed on third) + var foundMapping bool + for _, org := range workspaceOrgs { + cfg, exists := configs[org] + if !exists { + continue + } + + attempts = append(attempts, attempt{ + org: org, + domain: cfg.Global.EmailDomain, + }) + + // Simulate: fail on org1 and org2, succeed on org3 + if org == "org3" { + foundMapping = true + break + } + } + + // Verify all three orgs were attempted (loop broke on success) + if len(attempts) != 3 { + t.Errorf("expected 3 attempts, got %d", len(attempts)) + } + + // Verify the attempts were in order + expectedAttempts := []attempt{ + {"org1", "example.com"}, + {"org2", "example.org"}, + {"org3", "example.net"}, + } + + for i, expected := range expectedAttempts { + if i >= len(attempts) { + t.Errorf("missing attempt %d", i) + continue + } + actual := attempts[i] + if actual.org != expected.org || actual.domain != expected.domain { + t.Errorf("attempt %d: expected (%s, %s), got (%s, %s)", + i, expected.org, expected.domain, actual.org, actual.domain) + } + } + + if !foundMapping { + t.Error("mapping should have been found") + } +} + +// TestUserMappingEarlyExit_FirstOrgSucceeds verifies that when first org succeeds, subsequent orgs are not attempted. +func TestUserMappingEarlyExit_FirstOrgSucceeds(t *testing.T) { + t.Parallel() + + workspaceOrgs := []string{"org1", "org2", "org3"} + + // Simulate configs + configs := map[string]*config.RepoConfig{ + "org1": {Users: nil}, + "org2": {Users: nil}, + "org3": {Users: nil}, + } + configs["org1"].Global.EmailDomain = "example.com" + configs["org2"].Global.EmailDomain = "example.org" + configs["org3"].Global.EmailDomain = "example.net" + + // Simulate mapping attempts (succeed on first) + var attempts int + var foundMapping bool + for _, org := range workspaceOrgs { + _, exists := configs[org] + if !exists { + continue + } + + attempts++ + + // Simulate: succeed on first org + if org == "org1" { + foundMapping = true + break + } + } + + // Verify only one attempt was made + if attempts != 1 { + t.Errorf("expected 1 attempt, got %d (should break early on success)", attempts) + } + + if !foundMapping { + t.Error("mapping should have been found") + } +} diff --git a/pkg/slack/interfaces.go b/pkg/slack/interfaces.go new file mode 100644 index 0000000..e26dac3 --- /dev/null +++ b/pkg/slack/interfaces.go @@ -0,0 +1,29 @@ +package slack + +import ( + "context" + + "github.com/codeGROOVE-dev/slacker/pkg/config" + "github.com/codeGROOVE-dev/slacker/pkg/github" + "github.com/codeGROOVE-dev/slacker/pkg/usermapping" +) + +// GitHubManager defines the interface for GitHub client management. +// This interface allows for easier testing by enabling mock implementations. +type GitHubManager interface { + AllOrgs() []string + ClientForOrg(org string) (*github.Client, bool) +} + +// ConfigManager defines the interface for configuration management. +// This interface allows for easier testing by enabling mock implementations. +type ConfigManager interface { + Config(org string) (*config.RepoConfig, bool) +} + +// UserMapper defines the interface for Slack-to-GitHub user mapping. +// This interface allows for easier testing by enabling mock implementations. +type UserMapper interface { + LookupGitHub(ctx context.Context, slackAPI usermapping.SlackAPI, slackUserID, org, emailDomain string) (*usermapping.ReverseMapping, error) + SetOverrides(overrides map[string]string) +} diff --git a/pkg/slack/manager.go b/pkg/slack/manager.go index 0212853..4abee62 100644 --- a/pkg/slack/manager.go +++ b/pkg/slack/manager.go @@ -35,7 +35,7 @@ type Manager struct { clients map[string]*Client // team_id -> client metadata map[string]*WorkspaceMetadata homeViewHandler func(ctx context.Context, teamID, userID string) error // Global home view handler - reportHandler func(ctx context.Context, teamID, userID string) error // Global report handler for /r2r report + reportHandler func(ctx context.Context, teamID, userID string) error // Global report handler for /goose report } // NewManager creates a new Slack client manager. diff --git a/pkg/slack/manager_additional_test.go b/pkg/slack/manager_additional_test.go new file mode 100644 index 0000000..b5f5fb4 --- /dev/null +++ b/pkg/slack/manager_additional_test.go @@ -0,0 +1,253 @@ +package slack + +import ( + "context" + "encoding/json" + "testing" + + "github.com/codeGROOVE-dev/slacker/pkg/state" + "github.com/slack-go/slack" +) + +// TestManager_SetReportHandler tests setting report handler on manager. +func TestManager_SetReportHandler(t *testing.T) { + t.Parallel() + + manager := NewManager("test-secret") + + // Create mock clients + client1 := &Client{teamID: "T123"} + client2 := &Client{teamID: "T456"} + + manager.mu.Lock() + manager.clients["T123"] = client1 + manager.clients["T456"] = client2 + manager.mu.Unlock() + + // Set report handler + handlerCalled := 0 + handler := func(ctx context.Context, teamID, userID string) error { + handlerCalled++ + return nil + } + + manager.SetReportHandler(handler) + + // Verify handler was set on manager + manager.mu.Lock() + if manager.reportHandler == nil { + t.Error("Expected report handler to be set on manager") + } + manager.mu.Unlock() + + // Verify handler was set on existing clients + client1.reportHandlerMu.RLock() + if client1.reportHandler == nil { + t.Error("Expected report handler to be set on client1") + } + client1.reportHandlerMu.RUnlock() + + client2.reportHandlerMu.RLock() + if client2.reportHandler == nil { + t.Error("Expected report handler to be set on client2") + } + client2.reportHandlerMu.RUnlock() + + // Call handlers + _ = client1.reportHandler(context.Background(), "T123", "U123") + _ = client2.reportHandler(context.Background(), "T456", "U456") + + if handlerCalled != 2 { + t.Errorf("Expected handler to be called 2 times, got %d", handlerCalled) + } +} + +// TestManager_StoreWorkspace tests storing workspace credentials. +func TestManager_StoreWorkspace(t *testing.T) { + // Skip if not in integration test mode - requires GSM access + if testing.Short() { + t.Skip("Skipping GSM integration test in short mode") + } + + // This test would require mocking GSM, which is complex + // For now, we'll test the basic flow without actually calling GSM + + manager := NewManager("test-secret") + + metadata := &WorkspaceMetadata{ + TeamID: "T123TEST", + TeamName: "Test Workspace", + BotUserID: "UBOT123", + } + + // Pre-populate cache to verify invalidation + manager.mu.Lock() + manager.clients["T123TEST"] = &Client{teamID: "T123TEST"} + manager.metadata["T123TEST"] = metadata + manager.mu.Unlock() + + // Note: This will fail without GSM access, but that's expected + // The test is mainly to ensure the code path is exercised + err := manager.StoreWorkspace(context.Background(), metadata, "xoxb-test-token") + + // We expect an error since GSM is not available in tests + if err == nil { + t.Log("StoreWorkspace succeeded (GSM available)") + // Verify cache was invalidated + manager.mu.Lock() + _, clientExists := manager.clients["T123TEST"] + _, metadataExists := manager.metadata["T123TEST"] + manager.mu.Unlock() + + if clientExists { + t.Error("Expected client to be removed from cache") + } + if metadataExists { + t.Error("Expected metadata to be removed from cache") + } + } else { + t.Logf("StoreWorkspace failed as expected without GSM: %v", err) + } +} + +// TestRegisterWorkspace tests registering a workspace for testing. +func TestRegisterWorkspace(t *testing.T) { + t.Parallel() + + manager := NewManager("test-secret") + mockSlackClient := &slack.Client{} + + // Register workspace + manager.RegisterWorkspace(context.Background(), "T123", mockSlackClient) + + // Verify client was registered + manager.mu.Lock() + client, exists := manager.clients["T123"] + metadata, metadataExists := manager.metadata["T123"] + manager.mu.Unlock() + + if !exists { + t.Fatal("Expected client to be registered") + } + + if client.teamID != "T123" { + t.Errorf("Expected team ID T123, got %s", client.teamID) + } + + if !metadataExists { + t.Fatal("Expected metadata to be registered") + } + + if metadata.TeamID != "T123" { + t.Errorf("Expected metadata team ID T123, got %s", metadata.TeamID) + } + + if metadata.TeamName != "test-workspace" { + t.Errorf("Expected team name 'test-workspace', got %s", metadata.TeamName) + } +} + +// TestRegisterWorkspace_WithStateStore tests registering workspace with state store. +func TestRegisterWorkspace_WithStateStore(t *testing.T) { + t.Parallel() + + mockStore := &state.MemoryStore{} + manager := NewManager("test-secret") + manager.SetStateStore(mockStore) + + mockSlackClient := &slack.Client{} + + // Register workspace + manager.RegisterWorkspace(context.Background(), "T456", mockSlackClient) + + // Verify client has state store + manager.mu.Lock() + client := manager.clients["T456"] + manager.mu.Unlock() + + if client == nil { + t.Fatal("Expected client to be registered") + } + + client.stateStoreMu.RLock() + stateStore := client.stateStore + client.stateStoreMu.RUnlock() + + if stateStore == nil { + t.Error("Expected state store to be set on client") + } +} + +// TestWorkspaceMetadata_JSON tests JSON marshaling/unmarshaling. +func TestWorkspaceMetadata_JSON(t *testing.T) { + t.Parallel() + + original := &WorkspaceMetadata{ + TeamID: "T123", + TeamName: "Test Workspace", + BotUserID: "UBOT123", + } + + // Marshal + data, err := json.Marshal(original) + if err != nil { + t.Fatalf("Failed to marshal metadata: %v", err) + } + + // Unmarshal + var restored WorkspaceMetadata + if err := json.Unmarshal(data, &restored); err != nil { + t.Fatalf("Failed to unmarshal metadata: %v", err) + } + + // Verify + if restored.TeamID != original.TeamID { + t.Errorf("TeamID mismatch: expected %s, got %s", original.TeamID, restored.TeamID) + } + if restored.TeamName != original.TeamName { + t.Errorf("TeamName mismatch: expected %s, got %s", original.TeamName, restored.TeamName) + } + if restored.BotUserID != original.BotUserID { + t.Errorf("BotUserID mismatch: expected %s, got %s", original.BotUserID, restored.BotUserID) + } +} + +// TestManager_SetReportHandler_NewClient tests that new clients get the handler. +func TestManager_SetReportHandler_NewClient(t *testing.T) { + t.Parallel() + + manager := NewManager("test-secret") + + // Set report handler before any clients exist + handlerCalled := false + handler := func(ctx context.Context, teamID, userID string) error { + handlerCalled = true + return nil + } + + manager.SetReportHandler(handler) + + // Now create a client (this would normally happen via Client() method) + newClient := &Client{teamID: "T789"} + + // Manually simulate what Client() does - set the handler + manager.mu.Lock() + if manager.reportHandler != nil { + newClient.SetReportHandler(manager.reportHandler) + } + manager.mu.Unlock() + + // Verify handler was set + newClient.reportHandlerMu.RLock() + if newClient.reportHandler == nil { + t.Error("Expected report handler to be set on new client") + } + newClient.reportHandlerMu.RUnlock() + + // Call handler + _ = newClient.reportHandler(context.Background(), "T789", "U789") + + if !handlerCalled { + t.Error("Expected handler to be called") + } +} diff --git a/pkg/slack/mock_builders_test.go b/pkg/slack/mock_builders_test.go index 97e3540..00b5e43 100644 --- a/pkg/slack/mock_builders_test.go +++ b/pkg/slack/mock_builders_test.go @@ -226,6 +226,29 @@ func (b *MockAPIBuilder) WithGetConversationsError(err error) *MockAPIBuilder { return b } +// WithPublishViewSuccess configures the mock to successfully publish views. +func (b *MockAPIBuilder) WithPublishViewSuccess() *MockAPIBuilder { + b.mock.publishViewFunc = func(ctx context.Context, request slack.PublishViewContextRequest) (*slack.ViewResponse, error) { + return &slack.ViewResponse{}, nil + } + return b +} + +// WithPublishViewError configures the mock to fail when publishing views. +func (b *MockAPIBuilder) WithPublishViewError(err error) *MockAPIBuilder { + b.mock.publishViewFunc = func(ctx context.Context, request slack.PublishViewContextRequest) (*slack.ViewResponse, error) { + return nil, err + } + return b +} + +// WithUserTimezone configures the user timezone returned by the mock. +func (b *MockAPIBuilder) WithUserTimezone(timezone string) *MockAPIBuilder { + // UserTimezone is a Client method, not an API method + // We'll need to handle this at the Client level in tests + return b +} + // Build returns the configured mockAPI. func (b *MockAPIBuilder) Build() *mockAPI { return b.mock diff --git a/pkg/slack/oauth.go b/pkg/slack/oauth.go index 838df0d..9182781 100644 --- a/pkg/slack/oauth.go +++ b/pkg/slack/oauth.go @@ -200,7 +200,7 @@ func (*OAuthHandler) writeSuccessPage(writer http.ResponseWriter, teamName strin
-Ready to Review is now supercharging
+reviewGOOSE:Slack is now active in
🚀 - Your dev team just got faster. + You'll know instantly when you're blocking a PR. 🚀
- +