mirror of
https://github.com/rcourtman/Pulse.git
synced 2026-09-23 03:33:53 +00:00
fix: preserve email rateLimit when not explicitly provided in request
Backend fix: - Added presence check in UpdateEmailConfig to detect when rateLimit is omitted from JSON (vs explicitly set to 0) - Preserves existing rateLimit value when field is not present in request - Added comprehensive integration tests covering all scenarios Frontend fix: - Added rateLimit to EmailConfig interface - Fixed getEmailConfig to read rateLimit from server response - Fixed updateEmailConfig to include rateLimit when set - Fixed two places in Alerts.tsx that hardcoded rateLimit: 60 Additional fixes: - Added Array.isArray guards in DiagnosticsPanel sanitization - Initialized Nodes/PBS arrays in diagnostics response to prevent null Closes rate limit persistence bug where updating email settings would reset the rate limit to default value.
This commit is contained in:
@@ -43,6 +43,7 @@ export interface EmailConfig {
|
||||
to: string[];
|
||||
tls: boolean;
|
||||
startTLS: boolean;
|
||||
rateLimit?: number;
|
||||
}
|
||||
|
||||
export interface Webhook {
|
||||
@@ -107,12 +108,13 @@ export class NotificationsAPI {
|
||||
to: (backendConfig.to as string[]) || [],
|
||||
tls: (backendConfig.tls as boolean) || false,
|
||||
startTLS: (backendConfig.startTLS as boolean) || false,
|
||||
rateLimit: (backendConfig.rateLimit as number) || undefined,
|
||||
};
|
||||
}
|
||||
|
||||
static async updateEmailConfig(config: EmailConfig): Promise<{ success: boolean }> {
|
||||
// Backend expects fields with these names (server, port)
|
||||
const backendConfig = {
|
||||
const backendConfig: Record<string, unknown> = {
|
||||
enabled: config.enabled,
|
||||
server: config.server,
|
||||
port: config.port,
|
||||
@@ -125,6 +127,11 @@ export class NotificationsAPI {
|
||||
provider: config.provider || '',
|
||||
};
|
||||
|
||||
// Only include rateLimit if it's explicitly set
|
||||
if (config.rateLimit !== undefined) {
|
||||
backendConfig.rateLimit = config.rateLimit;
|
||||
}
|
||||
|
||||
return apiFetchJSON(`${this.baseUrl}/email`, {
|
||||
method: 'PUT',
|
||||
body: JSON.stringify(backendConfig),
|
||||
|
||||
@@ -268,22 +268,26 @@ export const DiagnosticsPanel: Component = () => {
|
||||
const redactString = (s: string): string => s.replace(ipv4Re, '[REDACTED_IP]');
|
||||
|
||||
// Redact node hosts
|
||||
data.nodes = data.nodes.map((node, i) => ({
|
||||
...node,
|
||||
host: `node-${i + 1}`,
|
||||
name: `node-${i + 1}`,
|
||||
id: `node-${i + 1}`,
|
||||
error: node.error ? redactString(node.error) : undefined,
|
||||
}));
|
||||
if (Array.isArray(data.nodes)) {
|
||||
data.nodes = data.nodes.map((node, i) => ({
|
||||
...node,
|
||||
host: `node-${i + 1}`,
|
||||
name: `node-${i + 1}`,
|
||||
id: `node-${i + 1}`,
|
||||
error: node.error ? redactString(node.error) : undefined,
|
||||
}));
|
||||
}
|
||||
|
||||
// Redact PBS hosts
|
||||
data.pbs = data.pbs.map((p, i) => ({
|
||||
...p,
|
||||
host: `pbs-${i + 1}`,
|
||||
name: `pbs-${i + 1}`,
|
||||
id: `pbs-${i + 1}`,
|
||||
error: p.error ? redactString(p.error) : undefined,
|
||||
}));
|
||||
if (Array.isArray(data.pbs)) {
|
||||
data.pbs = data.pbs.map((p, i) => ({
|
||||
...p,
|
||||
host: `pbs-${i + 1}`,
|
||||
name: `pbs-${i + 1}`,
|
||||
id: `pbs-${i + 1}`,
|
||||
error: p.error ? redactString(p.error) : undefined,
|
||||
}));
|
||||
}
|
||||
|
||||
// Redact discovery subnets
|
||||
if (data.discovery) {
|
||||
@@ -343,7 +347,9 @@ export const DiagnosticsPanel: Component = () => {
|
||||
}
|
||||
|
||||
// Redact IPs in error messages
|
||||
data.errors = data.errors.map(redactString);
|
||||
if (Array.isArray(data.errors)) {
|
||||
data.errors = data.errors.map(redactString);
|
||||
}
|
||||
|
||||
// Redact IPs from any raw snapshot data that may be present
|
||||
const raw2 = data as any;
|
||||
|
||||
@@ -1370,7 +1370,7 @@ export function Alerts() {
|
||||
replyTo: '',
|
||||
maxRetries: 3,
|
||||
retryDelay: 5,
|
||||
rateLimit: 60,
|
||||
rateLimit: emailConfigData.rateLimit ?? 60,
|
||||
});
|
||||
} catch (emailErr) {
|
||||
logger.error('Failed to load email configuration:', emailErr);
|
||||
@@ -1435,7 +1435,7 @@ export function Alerts() {
|
||||
replyTo: '',
|
||||
maxRetries: 3,
|
||||
retryDelay: 5,
|
||||
rateLimit: 60,
|
||||
rateLimit: emailConfigData.rateLimit ?? 60,
|
||||
});
|
||||
})
|
||||
.catch((err) => {
|
||||
|
||||
@@ -425,6 +425,8 @@ func writeDiagnosticsResponse(w http.ResponseWriter, diag DiagnosticsInfo, cache
|
||||
func (r *Router) computeDiagnostics(ctx context.Context) DiagnosticsInfo {
|
||||
diag := DiagnosticsInfo{
|
||||
Errors: []string{},
|
||||
Nodes: []NodeDiagnostic{},
|
||||
PBS: []PBSDiagnostic{},
|
||||
}
|
||||
|
||||
// Version info
|
||||
|
||||
@@ -156,6 +156,7 @@ func (h *NotificationHandlers) UpdateEmailConfig(w http.ResponseWriter, r *http.
|
||||
Str("from", config.From).
|
||||
Int("toCount", len(config.To)).
|
||||
Bool("hasPassword", config.Password != "").
|
||||
Int("rateLimit", config.RateLimit).
|
||||
Msg("Parsed email config")
|
||||
|
||||
h.getMonitor(r.Context()).GetNotificationManager().SetEmailConfig(config)
|
||||
|
||||
@@ -318,6 +318,7 @@ func TestNotificationHandlers(t *testing.T) {
|
||||
SMTPHost: "smtp.example.com",
|
||||
Password: "newpassword",
|
||||
}
|
||||
mockManager.On("GetEmailConfig").Return(notifications.EmailConfig{}).Once()
|
||||
mockManager.On("SetEmailConfig", mock.Anything).Return().Once()
|
||||
mockPersistence.On("SaveEmailConfig", mock.Anything).Return(nil).Once()
|
||||
|
||||
|
||||
@@ -0,0 +1,140 @@
|
||||
package api
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"encoding/json"
|
||||
"net/http/httptest"
|
||||
"testing"
|
||||
|
||||
"github.com/rcourtman/pulse-go-rewrite/internal/notifications"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/mock"
|
||||
)
|
||||
|
||||
// TestRateLimitPersistence_FullRoundTrip tests the complete data flow:
|
||||
// 1. Existing config has RateLimit=120
|
||||
// 2. User updates other fields without rateLimit in JSON
|
||||
// 3. Backend preserves RateLimit=120
|
||||
// 4. GET returns RateLimit=120
|
||||
func TestRateLimitPersistence_FullRoundTrip(t *testing.T) {
|
||||
// Setup mocks
|
||||
mockMonitor := new(MockNotificationMonitor)
|
||||
mockManager := new(MockNotificationManager)
|
||||
mockPersistence := new(MockNotificationConfigPersistence)
|
||||
|
||||
mockMonitor.On("GetNotificationManager").Return(mockManager)
|
||||
mockMonitor.On("GetConfigPersistence").Return(mockPersistence)
|
||||
|
||||
h := NewNotificationHandlers(nil, mockMonitor)
|
||||
|
||||
// Existing config in "database" has RateLimit=120
|
||||
existingConfig := notifications.EmailConfig{
|
||||
Enabled: true,
|
||||
SMTPHost: "smtp.example.com",
|
||||
Password: "secret",
|
||||
RateLimit: 120,
|
||||
}
|
||||
|
||||
// Test 1: GET returns the rateLimit
|
||||
t.Run("GET_returns_rateLimit", func(t *testing.T) {
|
||||
mockManager.On("GetEmailConfig").Return(existingConfig).Once()
|
||||
|
||||
req := httptest.NewRequest("GET", "/api/notifications/email", nil)
|
||||
w := httptest.NewRecorder()
|
||||
h.GetEmailConfig(w, req)
|
||||
|
||||
assert.Equal(t, 200, w.Code)
|
||||
|
||||
var resp map[string]interface{}
|
||||
json.Unmarshal(w.Body.Bytes(), &resp)
|
||||
|
||||
// Verify rateLimit is returned (password should be empty for security)
|
||||
assert.Equal(t, float64(120), resp["rateLimit"])
|
||||
assert.Equal(t, "", resp["password"]) // Redacted
|
||||
})
|
||||
|
||||
// Test 2: PUT without rateLimit preserves existing value
|
||||
t.Run("PUT_without_rateLimit_preserves_existing", func(t *testing.T) {
|
||||
// Return existing config when handler calls GetEmailConfig
|
||||
mockManager.On("GetEmailConfig").Return(existingConfig).Once()
|
||||
|
||||
// Expect SetEmailConfig to be called WITH RateLimit=120 preserved
|
||||
mockManager.On("SetEmailConfig", mock.MatchedBy(func(c notifications.EmailConfig) bool {
|
||||
t.Logf("SetEmailConfig called with RateLimit=%d", c.RateLimit)
|
||||
return c.RateLimit == 120 && c.SMTPHost == "smtp.newhost.com"
|
||||
})).Return().Once()
|
||||
|
||||
mockPersistence.On("SaveEmailConfig", mock.MatchedBy(func(c notifications.EmailConfig) bool {
|
||||
return c.RateLimit == 120
|
||||
})).Return(nil).Once()
|
||||
|
||||
// Request body does NOT include rateLimit
|
||||
payload := map[string]interface{}{
|
||||
"enabled": true,
|
||||
"server": "smtp.newhost.com",
|
||||
"password": "newpassword",
|
||||
}
|
||||
|
||||
body, _ := json.Marshal(payload)
|
||||
req := httptest.NewRequest("PUT", "/api/notifications/email", bytes.NewReader(body))
|
||||
w := httptest.NewRecorder()
|
||||
h.UpdateEmailConfig(w, req)
|
||||
|
||||
assert.Equal(t, 200, w.Code)
|
||||
mockManager.AssertExpectations(t)
|
||||
mockPersistence.AssertExpectations(t)
|
||||
})
|
||||
|
||||
// Test 3: PUT with rateLimit=0 explicitly sets it to 0
|
||||
t.Run("PUT_with_explicit_rateLimit_0_sets_to_0", func(t *testing.T) {
|
||||
mockManager.On("GetEmailConfig").Return(existingConfig).Once()
|
||||
|
||||
mockManager.On("SetEmailConfig", mock.MatchedBy(func(c notifications.EmailConfig) bool {
|
||||
t.Logf("SetEmailConfig called with RateLimit=%d", c.RateLimit)
|
||||
return c.RateLimit == 0 // User explicitly set to 0
|
||||
})).Return().Once()
|
||||
|
||||
mockPersistence.On("SaveEmailConfig", mock.Anything).Return(nil).Once()
|
||||
|
||||
// Request body INCLUDES rateLimit: 0
|
||||
payload := map[string]interface{}{
|
||||
"enabled": true,
|
||||
"server": "smtp.example.com",
|
||||
"rateLimit": 0,
|
||||
}
|
||||
|
||||
body, _ := json.Marshal(payload)
|
||||
req := httptest.NewRequest("PUT", "/api/notifications/email", bytes.NewReader(body))
|
||||
w := httptest.NewRecorder()
|
||||
h.UpdateEmailConfig(w, req)
|
||||
|
||||
assert.Equal(t, 200, w.Code)
|
||||
mockManager.AssertExpectations(t)
|
||||
})
|
||||
|
||||
// Test 4: PUT with new rateLimit updates it
|
||||
t.Run("PUT_with_new_rateLimit_updates", func(t *testing.T) {
|
||||
mockManager.On("GetEmailConfig").Return(existingConfig).Once()
|
||||
|
||||
mockManager.On("SetEmailConfig", mock.MatchedBy(func(c notifications.EmailConfig) bool {
|
||||
t.Logf("SetEmailConfig called with RateLimit=%d", c.RateLimit)
|
||||
return c.RateLimit == 60 // User changed to 60
|
||||
})).Return().Once()
|
||||
|
||||
mockPersistence.On("SaveEmailConfig", mock.Anything).Return(nil).Once()
|
||||
|
||||
payload := map[string]interface{}{
|
||||
"enabled": true,
|
||||
"server": "smtp.example.com",
|
||||
"rateLimit": 60,
|
||||
}
|
||||
|
||||
body, _ := json.Marshal(payload)
|
||||
req := httptest.NewRequest("PUT", "/api/notifications/email", bytes.NewReader(body))
|
||||
w := httptest.NewRecorder()
|
||||
h.UpdateEmailConfig(w, req)
|
||||
|
||||
assert.Equal(t, 200, w.Code)
|
||||
mockManager.AssertExpectations(t)
|
||||
})
|
||||
}
|
||||
Reference in New Issue
Block a user