Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 112 additions & 0 deletions frontend/src/__tests__/settings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ describe('Settings Module', () => {
</select>
<input type="number" id="setting-default-coverage">
<input type="number" id="setting-notification-days">
<input type="number" id="setting-recs-stale-hours" value="24">
<select id="setting-recs-lookback-days">
<option value="7" selected>7 days</option>
<option value="30">30 days</option>
<option value="60">60 days</option>
</select>
<!-- AWS term/payment selects -->
<select id="aws-ec2-term"><option value="1">1</option><option value="3">3</option></select>
<select id="aws-rds-term"><option value="1">1</option><option value="3">3</option></select>
Expand Down Expand Up @@ -390,6 +396,44 @@ describe('Settings Module', () => {
expect(formEl?.classList.contains('hidden')).toBe(false);
});

// Issue #301: recommendations cycle params populated from API
test('populates recommendations_cache_stale_hours and recommendations_lookback_days', async () => {
(api.getConfig as jest.Mock).mockResolvedValue({
global: {
enabled_providers: ['aws'],
default_term: 3,
default_payment: 'all-upfront',
default_coverage: 80,
notification_days_before: 3,
recommendations_cache_stale_hours: 48,
recommendations_lookback_days: 30,
},
});

await loadGlobalSettings();

expect((document.getElementById('setting-recs-stale-hours') as HTMLInputElement).value).toBe('48');
expect((document.getElementById('setting-recs-lookback-days') as HTMLSelectElement).value).toBe('30');
});

test('populates recommendations fields with defaults when absent from API response', async () => {
(api.getConfig as jest.Mock).mockResolvedValue({
global: {
enabled_providers: ['aws'],
default_term: 3,
default_payment: 'all-upfront',
default_coverage: 80,
notification_days_before: 3,
// recommendations_cache_stale_hours and recommendations_lookback_days absent
},
});

await loadGlobalSettings();

expect((document.getElementById('setting-recs-stale-hours') as HTMLInputElement).value).toBe('24');
expect((document.getElementById('setting-recs-lookback-days') as HTMLSelectElement).value).toBe('7');
});

test('handles missing credentials config gracefully', async () => {
(api.getConfig as jest.Mock).mockResolvedValue({
global: {
Expand Down Expand Up @@ -474,6 +518,8 @@ describe('Settings Module', () => {
// doesn't include the new inputs (older test harness setup).
// The save helper reads missing elements as "empty" → default 7.
grace_period_days: { aws: 7, azure: 7, gcp: 7 },
recommendations_cache_stale_hours: 24,
recommendations_lookback_days: 7,
});
});

Expand Down Expand Up @@ -584,6 +630,60 @@ describe('Settings Module', () => {
// entries, net +2.
expect(api.updateServiceConfig).toHaveBeenCalledTimes(18);
});

// Issue #301: configurable recommendations cache-staleness threshold + lookback
test('sends recommendations_cache_stale_hours and recommendations_lookback_days', async () => {
(api.updateConfig as jest.Mock).mockResolvedValue({});
(document.getElementById('setting-recs-stale-hours') as HTMLInputElement).value = '48';
(document.getElementById('setting-recs-lookback-days') as HTMLSelectElement).value = '30';

const event = { preventDefault: jest.fn() } as unknown as Event;
await saveGlobalSettings(event);

const call = (api.updateConfig as jest.Mock).mock.calls[0][0];
expect(call.recommendations_cache_stale_hours).toBe(48);
expect(call.recommendations_lookback_days).toBe(30);
});

test('rejects out-of-range recommendations_cache_stale_hours and does not call updateConfig', async () => {
(api.updateConfig as jest.Mock).mockResolvedValue({});
(document.getElementById('setting-recs-stale-hours') as HTMLInputElement).value = '9999';

const event = { preventDefault: jest.fn() } as unknown as Event;
await saveGlobalSettings(event);

expect(api.updateConfig).not.toHaveBeenCalled();
expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({ kind: 'error' }));
});

test('rejects fractional recommendations_cache_stale_hours and does not call updateConfig', async () => {
// Pin the Number.isInteger guard so a future refactor can't silently
// truncate fractional input via parseInt and accept it as valid.
(api.updateConfig as jest.Mock).mockResolvedValue({});
(document.getElementById('setting-recs-stale-hours') as HTMLInputElement).value = '1.5';

const event = { preventDefault: jest.fn() } as unknown as Event;
await saveGlobalSettings(event);

expect(api.updateConfig).not.toHaveBeenCalled();
expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({ kind: 'error' }));
});

test('accepts 0 for recommendations_cache_stale_hours (disable sentinel)', async () => {
// Preserve the documented "0 = disable automatic background refresh"
// semantic. The validator must NOT reject 0; updateConfig must be
// called with the literal 0 so the persisted GlobalConfig disables
// the stale-while-revalidate background refresh.
(api.updateConfig as jest.Mock).mockResolvedValue({});
(document.getElementById('setting-recs-stale-hours') as HTMLInputElement).value = '0';

const event = { preventDefault: jest.fn() } as unknown as Event;
await saveGlobalSettings(event);

expect(api.updateConfig).toHaveBeenCalled();
const call = (api.updateConfig as jest.Mock).mock.calls[0][0];
expect(call.recommendations_cache_stale_hours).toBe(0);
});
}); // end saveGlobalSettings

describe('resetSettings', () => {
Expand Down Expand Up @@ -624,6 +724,18 @@ describe('Settings Module', () => {
expect((document.getElementById('setting-default-coverage') as HTMLInputElement).value).toBe('80');
expect((document.getElementById('setting-notification-days') as HTMLInputElement).value).toBe('3');
});

// Issue #301: recommendations cycle params reset to defaults
test('resets recommendations_cache_stale_hours to 24 and lookback to 7', async () => {
mockConfirmDialog.mockResolvedValueOnce(true);
(document.getElementById('setting-recs-stale-hours') as HTMLInputElement).value = '48';
(document.getElementById('setting-recs-lookback-days') as HTMLSelectElement).value = '60';

await resetSettings();

expect((document.getElementById('setting-recs-stale-hours') as HTMLInputElement).value).toBe('24');
expect((document.getElementById('setting-recs-lookback-days') as HTMLSelectElement).value).toBe('7');
});
});

describe('copyToClipboard', () => {
Expand Down
9 changes: 9 additions & 0 deletions frontend/src/api/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,15 @@ export interface Config {
ri_exchange_max_per_exchange_usd?: number;
ri_exchange_max_daily_usd?: number;
ri_exchange_lookback_days?: number;
// Age (hours) after which the recommendations cache triggers a background
// stale-while-revalidate refresh. 0 disables automatic background refresh;
// the cron scheduler and the manual Refresh button still work regardless.
// Valid range: 0–8760 (up to one year). Default: 24.
recommendations_cache_stale_hours?: number;
// AWS Cost Explorer lookback window (days) for fresh recommendations.
// Must be 7, 30, or 60 (AWS LookbackPeriodInDays enum). Default: 7.
// GCP CUD Recommender has no equivalent parameter; applies to AWS only.
recommendations_lookback_days?: number;
}

export interface ServiceConfig {
Expand Down
27 changes: 27 additions & 0 deletions frontend/src/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,15 @@ <h2>Global Configuration</h2>
<input type="number" id="setting-notification-days" value="3" min="1" max="30">
</div>
</div>
<div class="setting-row">
<div class="setting-info">
<label for="setting-recs-stale-hours">Recommendations Cache Stale Threshold (hours)</label>
<span class="info-icon">&#9432;<span class="tooltip-text">Age (in hours) at which the recommendations cache is considered stale and a background refresh fires automatically. Set to 0 to disable automatic background refresh (the cron scheduler and manual Refresh button still work). Valid range: 0&ndash;8760 (up to one year). Default: 24.</span></span>
</div>
<div class="setting-input">
<input type="number" id="setting-recs-stale-hours" value="24" min="0" max="8760" step="1">
</div>
</div>
</fieldset>

<div class="settings-buttons">
Expand Down Expand Up @@ -362,6 +371,24 @@ <h2>Purchasing Settings</h2>
</div>
</fieldset>

<fieldset class="settings-category" id="purchasing-recommendations-lookback">
<legend>Recommendations Lookback</legend>
<p class="settings-help">Controls how far back AWS Cost Explorer looks when computing fresh recommendations. Must be 7, 30, or 60 days (AWS Cost Explorer API constraint). GCP CUD Recommender has no equivalent parameter and always uses its own internal window.</p>
<div class="setting-row">
<div class="setting-info">
<label for="setting-recs-lookback-days"><span class="provider-badge aws">AWS</span> lookback period</label>
<span class="info-icon">&#9432;<span class="tooltip-text">AWS Cost Explorer LookbackPeriodInDays. A longer window smooths out utilisation spikes but may under-represent recent workload changes. Default: 7 days.</span></span>
</div>
<div class="setting-input">
<select id="setting-recs-lookback-days">
<option value="7" selected>7 days</option>
<option value="30">30 days</option>
<option value="60">60 days</option>
</select>
</div>
</div>
</fieldset>

<fieldset class="settings-category" id="purchasing-grace-period">
<legend>Post-Purchase Grace Period</legend>
<p class="settings-help">After a purchase is submitted, CUDly hides the just-covered capacity from the Recommendations view for this many days so you don't re-buy the same capacity while cloud-provider utilisation metrics catch up. Set to 0 to disable the feature for a provider. Range: 0–30 days.</p>
Expand Down
30 changes: 30 additions & 0 deletions frontend/src/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ const TRACKED_FIELDS = [
'setting-default-term', 'setting-default-payment', 'setting-default-coverage',
// Per-provider grace-period inputs
'setting-grace-aws', 'setting-grace-azure', 'setting-grace-gcp',
// Recommendations cycle params
'setting-recs-stale-hours', 'setting-recs-lookback-days',
// Per-service fields
...SERVICE_FIELDS.map(f => f.termId),
...SERVICE_FIELDS.filter(f => f.paymentId !== null).map(f => f.paymentId as string),
Expand Down Expand Up @@ -2120,6 +2122,16 @@ export async function loadGlobalSettings(): Promise<void> {
populateGraceInput('setting-grace-azure', gpMap['azure']);
populateGraceInput('setting-grace-gcp', gpMap['gcp']);

// Recommendations cycle params
const staleHoursInput = byId<HTMLInputElement>('setting-recs-stale-hours');
if (staleHoursInput) {
staleHoursInput.value = String(data.global.recommendations_cache_stale_hours ?? 24);
}
const lookbackSelect = byId<HTMLSelectElement>('setting-recs-lookback-days');
if (lookbackSelect) {
lookbackSelect.value = String(data.global.recommendations_lookback_days ?? 7);
}

// Update visibility based on loaded settings
updateProviderSettingsVisibility();
updateCollectionScheduleVisibility();
Expand Down Expand Up @@ -2288,6 +2300,17 @@ export async function saveGlobalSettings(e: Event): Promise<void> {
gracePeriodDays[provider] = v.value;
}

// Validate recommendations_cache_stale_hours before building the save payload.
// Use Number() so fractional input like "1.5" fails Number.isInteger instead of silently
// truncating to 1 as parseInt would.
const rawStaleHours = Number(byId<HTMLInputElement>('setting-recs-stale-hours')?.value ?? '24');
if (!Number.isFinite(rawStaleHours) || !Number.isInteger(rawStaleHours) || rawStaleHours < 0 || rawStaleHours > 8760) {
showToast({ message: 'Cache stale threshold must be a whole number between 0 and 8760 hours (0 = disable)', kind: 'error' });
if (saveBtn) saveBtn.disabled = false;
saveInFlight = false;
return;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

const settings: api.Config = {
enabled_providers: enabledProviders,
notification_email: byId<HTMLInputElement>('setting-notification-email')?.value || '',
Expand All @@ -2298,6 +2321,8 @@ export async function saveGlobalSettings(e: Event): Promise<void> {
default_coverage: parseInt(byId<HTMLInputElement>('setting-default-coverage')?.value || '80', 10),
notification_days_before: parseInt(byId<HTMLInputElement>('setting-notification-days')?.value || '3', 10),
grace_period_days: gracePeriodDays,
recommendations_cache_stale_hours: rawStaleHours,
recommendations_lookback_days: parseInt(byId<HTMLSelectElement>('setting-recs-lookback-days')?.value || '7', 10),
};

try {
Expand Down Expand Up @@ -2379,6 +2404,11 @@ export async function resetSettings(): Promise<void> {
populateGraceInput('setting-grace-aws', 7);
populateGraceInput('setting-grace-azure', 7);
populateGraceInput('setting-grace-gcp', 7);

const staleHoursInput = byId<HTMLInputElement>('setting-recs-stale-hours');
if (staleHoursInput) staleHoursInput.value = '24';
const lookbackSelect = byId<HTMLSelectElement>('setting-recs-lookback-days');
if (lookbackSelect) lookbackSelect.value = '7';
}

/**
Expand Down
6 changes: 6 additions & 0 deletions frontend/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,12 @@ export interface GlobalConfig {
// suppression feature. Keys: 'aws' / 'azure' / 'gcp'. Missing keys
// fall back to the backend default (7). Explicit 0 = disabled.
grace_period_days?: Record<string, number>;
// Age (hours) after which the recommendations cache triggers a background
// stale-while-revalidate refresh. 0 disables automatic background refresh.
// Valid range: 0–8760. Default: 24.
recommendations_cache_stale_hours?: number;
// AWS Cost Explorer lookback window (days). One of 7, 30, or 60. Default: 7.
recommendations_lookback_days?: number;
}

// API Keys types
Expand Down
37 changes: 37 additions & 0 deletions internal/api/handler_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,39 @@ func (h *Handler) getConfig(ctx context.Context) (*ConfigResponse, error) {
}, nil
}

// preserveOmittedRecommendationFields merges persisted GlobalConfig values
// for the two cycle-parameter fields when the request body omits them.
// Without this merge, a partial PUT would silently zero out
// RecommendationsCacheStaleHours / RecommendationsLookbackDays, which both
// have meaningful 0-vs-omitted semantics that json.Unmarshal can't represent
// directly. Errors from GetGlobalConfig fall through: the request body's
// zero values then flow into Validate() which rejects out-of-range
// lookback days, matching the pre-fix behaviour. Extracted from
// updateConfig to keep that function under the cyclomatic-complexity gate
// after the merge logic was added (PR #308 CodeRabbit pass-2 review).
func (h *Handler) preserveOmittedRecommendationFields(ctx context.Context, cfg *config.GlobalConfig, body string) error {
var present map[string]json.RawMessage
if err := json.Unmarshal([]byte(body), &present); err != nil {
return NewClientError(400, "invalid request body")
}
_, hasStale := present["recommendations_cache_stale_hours"]
_, hasLookback := present["recommendations_lookback_days"]
if hasStale && hasLookback {
return nil
}
existing, gcErr := h.config.GetGlobalConfig(ctx)
if gcErr != nil || existing == nil {
return nil
}
if !hasStale {
cfg.RecommendationsCacheStaleHours = existing.RecommendationsCacheStaleHours
}
if !hasLookback {
cfg.RecommendationsLookbackDays = existing.RecommendationsLookbackDays
}
return nil
}

func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionURLRequest) (*StatusResponse, error) {
// Require update:config permission
if _, err := h.requirePermission(ctx, req, "update", "config"); err != nil {
Expand All @@ -53,6 +86,10 @@ func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionUR
return nil, NewClientError(400, "invalid request body")
}

if err := h.preserveOmittedRecommendationFields(ctx, &cfg, req.Body); err != nil {
return nil, err
}

// Validate the configuration
if err := cfg.Validate(); err != nil {
return nil, NewClientError(400, fmt.Sprintf("validation error: %s", err))
Expand Down
Loading