Refactor webhook sender to add manager/preparer interfaces

- Split `sender.go` into orchestrator logic and Slack-specific implementation (`slack.go`)
- Introduce `Manager` and `Preparer` interfaces to decouple specific platform logic

Author: neilv-g

workers/webhook/pkg/webhook/sender.go

@@ -15,14 +15,10 @@
 package webhook
 
 import (
-	"bytes"
 	"context"
-	"encoding/json"
-	"errors"
 	"fmt"
 	"log/slog"
 	"net/http"
-	"net/url"
 	"time"
 
 	"github.com/GoogleChrome/webstatus.dev/lib/workertypes"
@@ -52,97 +48,36 @@ func NewSender(httpClient HTTPClient, stateManager ChannelStateManager, frontend
 	}
 }
 
-type SlackPayload struct {
-	Text string `json:"text"`
+type Manager interface {
+	Send(ctx context.Context) error
 }
 
-type webhookPreparer interface {
-	Prepare(ctx context.Context, job workertypes.IncomingWebhookDeliveryJob) (*http.Request, error)
-}
-
-type slackPreparer struct {
-	frontendBaseURL string
-}
-
-func (s *slackPreparer) Prepare(
-	ctx context.Context, job workertypes.IncomingWebhookDeliveryJob) (*http.Request, error) {
-	parsedURL, err := url.Parse(job.WebhookURL)
-	if err != nil || parsedURL.Scheme != "https" || parsedURL.Host != "hooks.slack.com" {
-		// Record permanent failure due to invalid URL
-		return nil, fmt.Errorf("invalid webhook URL: %s", job.WebhookURL)
-	}
-
-	var summary workertypes.EventSummary
-	if err := json.Unmarshal(job.SummaryRaw, &summary); err != nil {
-		return nil, fmt.Errorf("failed to unmarshal summary: %w", err)
-	}
-
-	resultsURL := fmt.Sprintf("%s/features?q=%s", s.frontendBaseURL, url.QueryEscape(job.Metadata.Query))
-
-	payload := SlackPayload{
-		Text: fmt.Sprintf("WebStatus.dev Notification: %s\nQuery: %s\nView Results: %s",
-			summary.Text, job.Metadata.Query, resultsURL),
-	}
-
-	payloadBytes, err := json.Marshal(payload)
-	if err != nil {
-		return nil, fmt.Errorf("failed to marshal slack payload: %w", err)
-	}
-
-	req, err := http.NewRequestWithContext(ctx, http.MethodPost, job.WebhookURL, bytes.NewBuffer(payloadBytes))
-	if err != nil {
-		return nil, fmt.Errorf("failed to create request: %w", err)
-	}
-	req.Header.Set("Content-Type", "application/json")
-
-	return req, nil
+type Preparer interface {
+	Prepare(job workertypes.IncomingWebhookDeliveryJob) (Manager, error)
 }
 
 func (s *Sender) SendWebhook(ctx context.Context, job workertypes.IncomingWebhookDeliveryJob) error {
 	slog.InfoContext(ctx, "sending webhook", "channelID", job.ChannelID, "url", job.WebhookURL)
 
-	var preparer webhookPreparer
+	var preparer Preparer
 	switch job.WebhookType {
 	case workertypes.WebhookTypeSlack:
-		preparer = &slackPreparer{frontendBaseURL: s.frontendBaseURL}
+		preparer = &slackPreparer{
+			frontendBaseURL: s.frontendBaseURL,
+			httpClient:      s.httpClient,
+			stateManager:    s.stateManager,
+		}
 	default:
 		err := fmt.Errorf("unsupported webhook type: %v", job.WebhookType)
 		_ = s.stateManager.RecordFailure(ctx, job.ChannelID, err, time.Now(), true, job.WebhookEventID)
 
 		return err
 	}
 
-	req, err := preparer.Prepare(ctx, job)
+	mgr, err := preparer.Prepare(job)
 	if err != nil {
-		// Preparation failures (like invalid payload or URL format) are typically permanent
-		_ = s.stateManager.RecordFailure(ctx, job.ChannelID, err, time.Now(), true, job.WebhookEventID)
-
 		return fmt.Errorf("failed to prepare webhook request: %w", err)
 	}
 
-	resp, err := s.httpClient.Do(req)
-	if err != nil {
-		// Transient error?
-		_ = s.stateManager.RecordFailure(ctx, job.ChannelID, err, time.Now(), false, job.WebhookEventID)
-
-		return fmt.Errorf("failed to send webhook: %w", err)
-	}
-	defer resp.Body.Close()
-
-	if resp.StatusCode >= 200 && resp.StatusCode < 300 {
-		// Success
-		_ = s.stateManager.RecordSuccess(ctx, job.ChannelID, time.Now(), job.WebhookEventID)
-
-		return nil
-	}
-
-	// Failure
-	errorMsg := fmt.Sprintf("webhook returned status code %d", resp.StatusCode)
-	webhookErr := errors.New(errorMsg)
-	isPermanent := resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusGone ||
-		resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden
-
-	_ = s.stateManager.RecordFailure(ctx, job.ChannelID, webhookErr, time.Now(), isPermanent, job.WebhookEventID)
-
-	return fmt.Errorf("webhook failed: %s", errorMsg)
+	return mgr.Send(ctx)
 }

workers/webhook/pkg/webhook/sender_test.go

@@ -17,6 +17,7 @@ package webhook
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"io"
 	"net/http"
 	"strings"
@@ -160,10 +161,6 @@ func TestSender_SendWebhook_Success(t *testing.T) {
 		t.Fatalf("SendWebhook failed: %v", err)
 	}
 
-	// Verify the payload text contains the link
-	// The mock doFunc already checked basic text, but let's verify exact format if needed.
-	// (Actual check is inside the mock doFunc redefined above if we want to be strict)
-
 	if len(mockState.successCalls) != 1 {
 		t.Errorf("expected 1 success call, got %d", len(mockState.successCalls))
 	}
@@ -252,3 +249,167 @@ func TestSender_SendWebhook_HTTPFailure(t *testing.T) {
 		t.Error("expected permanent failure for 404")
 	}
 }
+
+func TestSender_SendWebhook_UnsupportedType(t *testing.T) {
+	mockState := &mockChannelStateManager{
+		successCalls: nil,
+		failureCalls: nil,
+	}
+	sender := NewSender(nil, mockState, "https://webstatus.dev")
+
+	job := workertypes.IncomingWebhookDeliveryJob{
+		WebhookDeliveryJob: workertypes.WebhookDeliveryJob{
+			ChannelID:      "chan-1",
+			WebhookURL:     "https://example.com/webhook",
+			WebhookType:    "unknown",
+			SummaryRaw:     nil,
+			SubscriptionID: "sub-1",
+			Triggers:       nil,
+			Metadata: workertypes.DeliveryMetadata{
+				EventID:     "evt-1",
+				SearchID:    "search-1",
+				SearchName:  "",
+				Query:       "group:css",
+				Frequency:   workertypes.FrequencyImmediate,
+				GeneratedAt: time.Time{},
+			},
+		},
+		WebhookEventID: "evt-1",
+	}
+
+	err := sender.SendWebhook(context.Background(), job)
+	if err == nil {
+		t.Fatal("expected error, got nil")
+	}
+	if !strings.Contains(err.Error(), "unsupported webhook type") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+
+	if len(mockState.failureCalls) != 1 {
+		t.Errorf("expected 1 failure call, got %d", len(mockState.failureCalls))
+	}
+	if !mockState.failureCalls[0].isPermanent {
+		t.Error("expected permanent failure for unsupported type")
+	}
+}
+
+func TestSender_SendWebhook_InvalidSlackURL(t *testing.T) {
+	mockState := &mockChannelStateManager{
+		successCalls: nil,
+		failureCalls: nil,
+	}
+	sender := NewSender(nil, mockState, "https://webstatus.dev")
+
+	job := workertypes.IncomingWebhookDeliveryJob{
+		WebhookDeliveryJob: workertypes.WebhookDeliveryJob{
+			ChannelID:      "chan-1",
+			WebhookURL:     "https://not-slack.com/hook",
+			WebhookType:    workertypes.WebhookTypeSlack,
+			SummaryRaw:     nil,
+			SubscriptionID: "sub-1",
+			Triggers:       nil,
+			Metadata: workertypes.DeliveryMetadata{
+				EventID:     "evt-1",
+				SearchID:    "search-1",
+				SearchName:  "",
+				Query:       "group:css",
+				Frequency:   workertypes.FrequencyImmediate,
+				GeneratedAt: time.Time{},
+			},
+		},
+		WebhookEventID: "evt-1",
+	}
+
+	err := sender.SendWebhook(context.Background(), job)
+	if err == nil {
+		t.Fatal("expected error, got nil")
+	}
+
+	if len(mockState.failureCalls) != 1 {
+		t.Errorf("expected 1 failure call, got %d", len(mockState.failureCalls))
+	}
+	if !mockState.failureCalls[0].isPermanent {
+		t.Error("expected permanent failure for invalid URL")
+	}
+}
+
+func TestSender_SendWebhook_NetworkError(t *testing.T) {
+	mockHTTP := &mockHTTPClient{
+		doFunc: func(_ *http.Request) (*http.Response, error) {
+			return nil, fmt.Errorf("network error")
+		},
+	}
+	mockState := &mockChannelStateManager{
+		successCalls: nil,
+		failureCalls: nil,
+	}
+	sender := NewSender(mockHTTP, mockState, "https://webstatus.dev")
+
+	job := workertypes.IncomingWebhookDeliveryJob{
+		WebhookDeliveryJob: workertypes.WebhookDeliveryJob{
+			ChannelID:      "chan-1",
+			WebhookURL:     "https://hooks.slack.com/services/123",
+			WebhookType:    workertypes.WebhookTypeSlack,
+			SummaryRaw:     []byte(`{"text":"test"}`),
+			SubscriptionID: "sub-1",
+			Triggers:       nil,
+			Metadata: workertypes.DeliveryMetadata{
+				EventID:     "evt-1",
+				SearchID:    "search-1",
+				SearchName:  "",
+				Query:       "group:css",
+				Frequency:   workertypes.FrequencyImmediate,
+				GeneratedAt: time.Time{},
+			},
+		},
+		WebhookEventID: "evt-1",
+	}
+
+	err := sender.SendWebhook(context.Background(), job)
+	if err == nil {
+		t.Fatal("expected error, got nil")
+	}
+
+	if len(mockState.failureCalls) != 1 {
+		t.Errorf("expected 1 failure call, got %d", len(mockState.failureCalls))
+	}
+	if mockState.failureCalls[0].isPermanent {
+		t.Error("expected transient failure for network error")
+	}
+}
+
+func TestSender_SendWebhook_InvalidSummary(t *testing.T) {
+	mockState := &mockChannelStateManager{
+		successCalls: nil,
+		failureCalls: nil,
+	}
+	sender := NewSender(nil, mockState, "https://webstatus.dev")
+
+	job := workertypes.IncomingWebhookDeliveryJob{
+		WebhookDeliveryJob: workertypes.WebhookDeliveryJob{
+			ChannelID:      "chan-1",
+			WebhookURL:     "https://hooks.slack.com/services/123",
+			WebhookType:    workertypes.WebhookTypeSlack,
+			SummaryRaw:     []byte(`invalid json`),
+			SubscriptionID: "sub-1",
+			Triggers:       nil,
+			Metadata: workertypes.DeliveryMetadata{
+				EventID:     "evt-1",
+				SearchID:    "search-1",
+				SearchName:  "",
+				Query:       "group:css",
+				Frequency:   workertypes.FrequencyImmediate,
+				GeneratedAt: time.Time{},
+			},
+		},
+		WebhookEventID: "evt-1",
+	}
+
+	err := sender.SendWebhook(context.Background(), job)
+	if err == nil {
+		t.Fatal("expected error, got nil")
+	}
+	if !strings.Contains(err.Error(), "failed to unmarshal summary") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}