smsgateway/client.go (3)

54-59: Add godoc comment for the ListWebhooks method.

While the implementation is correct, adding documentation would improve the API's usability. Consider adding a comment describing the method's purpose, return values, and possible errors.

+// ListWebhooks retrieves all registered webhooks.
+// Returns a slice of Webhook objects or an error if the request fails.
 func (c *Client) ListWebhooks(ctx context.Context) ([]Webhook, error) {

---

61-66: Add input validation and documentation.

The implementation looks good, but consider these improvements:

1. Add godoc comment describing the method
2. Validate the webhook parameter before making the request

+// RegisterWebhook registers a new webhook.
+// Returns the registered webhook with server-assigned fields or an error if the request fails.
 func (c *Client) RegisterWebhook(ctx context.Context, webhook Webhook) (Webhook, error) {
+    if webhook.URL == "" {
+        return Webhook{}, fmt.Errorf("webhook URL is required")
+    }
     path := "/webhooks"

---

68-72: Add input validation and documentation.

The implementation correctly uses URL escaping, but consider these improvements:

1. Add godoc comment describing the method
2. Validate the webhookID parameter is not empty

+// DeleteWebhook removes a webhook with the specified ID.
+// Returns an error if the deletion fails or if the webhook doesn't exist.
 func (c *Client) DeleteWebhook(ctx context.Context, webhookID string) error {
+    if webhookID == "" {
+        return fmt.Errorf("webhookID is required")
+    }
     path := fmt.Sprintf("/webhooks/%s", url.PathEscape(webhookID))

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

smsgateway/domain_webhooks.go (1)

38-38: Document ID field requirements in struct comments.

While the omitempty tag is appropriate for the ID field, it would be helpful to document when this field is expected to be populated vs. when it should be empty. For example:

• Required when: retrieving existing webhooks
• Optional when: registering new webhooks

Add this documentation above the ID field:

 // The unique identifier of the webhook.
+// This field is populated by the server when retrieving existing webhooks.
+// When registering new webhooks, this field should be omitted.
 ID string `json:"id,omitempty" validate:"max=36" example:"123e4567-e89b-12d3-a456-426614174000"`

smsgateway/client.go (2)

74-78: Add validation for webhookID parameter.

The webhookID parameter should be validated before making the request to prevent unnecessary API calls.

 func (c *Client) DeleteWebhook(ctx context.Context, webhookID string) error {
+    if webhookID == "" {
+        return fmt.Errorf("webhookID cannot be empty")
+    }
     path := fmt.Sprintf("/webhooks/%s", url.PathEscape(webhookID))
     return c.doRequest(ctx, http.MethodDelete, path, map[string]string{}, nil, nil)
 }

---

54-61: Add context timeout examples in method documentation.

The methods accept a context parameter but don't provide examples of how to use it for timeouts.

Add usage examples to the documentation:

 // ListWebhooks retrieves all registered webhooks.
 // Returns a slice of Webhook objects or an error if the request fails.
+//
+// Example with timeout:
+//  ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+//  defer cancel()
+//  webhooks, err := client.ListWebhooks(ctx)
 func (c *Client) ListWebhooks(ctx context.Context) ([]Webhook, error) {

Also applies to: 63-70, 72-78

smsgateway/client_test.go (2)

172-202: Add more test cases for ListWebhooks.

The test coverage could be improved by adding cases for:

• Empty webhook list
• Server errors (500, 503)
• Context cancellation

 tests := []struct {
     name    string
     c       *Client
+    ctx     context.Context
     want    []Webhook
     wantErr bool
+    errMsg  string
 }{
     {
         name: "Success",
         c:    client,
+        ctx:  context.Background(),
         want: []Webhook{
             {
                 ID:    "123",
                 URL:   "https://example.com",
                 Event: WebhookEventSmsDelivered,
             },
         },
         wantErr: false,
     },
+    {
+        name: "Empty List",
+        c:    client,
+        ctx:  context.Background(),
+        want: []Webhook{},
+        wantErr: false,
+    },
+    {
+        name: "Context Cancelled",
+        c:    client,
+        ctx:  func() context.Context {
+            ctx, cancel := context.WithCancel(context.Background())
+            cancel()
+            return ctx
+        }(),
+        want: nil,
+        wantErr: true,
+        errMsg: "context canceled",
+    },
 }

---

205-276: Add test cases for invalid webhook events in RegisterWebhook.

The test should verify that the server validates webhook events.

 tests := []struct {
     name    string
     c       *Client
     args    args
     want    Webhook
     wantErr bool
+    errMsg  string
 }{
+    {
+        name: "Invalid Event",
+        c:    client,
+        args: args{
+            webhook: Webhook{
+                URL:   "https://example.com",
+                Event: "invalid:event",
+            },
+        },
+        want:    Webhook{},
+        wantErr: true,
+        errMsg:  "invalid webhook event",
+    },
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

smsgateway/domain_webhooks.go (1)

26-28: Remove redundant documentation.

The second comment block repeats the first one without adding new information.

 // WebhookEventTypes returns a slice of all supported webhook event types.
-//
-// Returns a slice of all supported webhook event types.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

smsgateway/domain_webhooks_test.go (1)

38-44: Enhance test coverage with additional validations.

While the current implementation is functional, consider enhancing it with additional validations:

 func TestWebhookEventTypes(t *testing.T) {
+    eventTypes := smsgateway.WebhookEventTypes()
+    
+    // Verify non-empty return
+    if len(eventTypes) == 0 {
+        t.Error("WebhookEventTypes() returned empty slice")
+    }
+
+    // Verify required event types are present
+    requiredEvents := []smsgateway.WebhookEvent{
+        smsgateway.WebhookEventSmsDelivered,
+        // Add other required event types
+    }
+    
+    eventSet := make(map[smsgateway.WebhookEvent]struct{})
+    for _, v := range eventTypes {
+        eventSet[v] = struct{}{}
         if !smsgateway.IsValidWebhookEvent(v) {
-            t.Errorf("event type %s is not valid", v)
+            t.Errorf("event type %q is not valid", v)
         }
     }
+    
+    for _, required := range requiredEvents {
+        if _, exists := eventSet[required]; !exists {
+            t.Errorf("required event type %q is missing", required)
+        }
+    }
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

smsgateway/client.go (1)

54-61: Consider adding pagination and filtering support.

While the current implementation is functional, consider:

1. Adding pagination support to handle large numbers of webhooks efficiently
2. Supporting query parameters for filtering webhooks by event type or URL

This would improve scalability and usability of the API.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro