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
9 changes: 5 additions & 4 deletions frontend/src/__tests__/api-analytics.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,24 +20,25 @@ describe('getAnalyticsTrends', () => {
expect(apiRequest).toHaveBeenCalledWith('/analytics/trends');
});

test('forwards account_id, provider and the window, URL-encoded', async () => {
test('forwards account_ids, provider and the window, URL-encoded', async () => {
await getAnalyticsTrends({
accountID: 'acc/1 x',
accountIDs: ['acc/1 x', 'acc2'],
provider: 'aws',
start: '2026-05-01T00:00:00.000Z',
end: '2026-10-23T12:00:00.000Z',
});
const url = (apiRequest as jest.Mock).mock.calls[0][0] as string;
const qs = new URLSearchParams(url.split('?')[1]);
expect(url.startsWith('/analytics/trends?')).toBe(true);
expect(qs.get('account_id')).toBe('acc/1 x');
expect(qs.get('account_ids')).toBe('acc/1 x,acc2');
expect(qs.has('account_id')).toBe(false);
expect(qs.get('provider')).toBe('aws');
expect(qs.get('start')).toBe('2026-05-01T00:00:00.000Z');
expect(qs.get('end')).toBe('2026-10-23T12:00:00.000Z');
});

test('omits empty filter values', async () => {
await getAnalyticsTrends({ accountID: '', provider: '' });
await getAnalyticsTrends({ accountIDs: [], provider: '' });
expect(apiRequest).toHaveBeenCalledWith('/analytics/trends');
});
});
15 changes: 15 additions & 0 deletions frontend/src/__tests__/savings-history.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,21 @@ describe('Savings History Module', () => {
);
});

test('forwards the full account selection when two or more are selected', async () => {
addPurchasesTab(true);
(state.getCurrentAccountIDs as jest.Mock).mockReturnValue(['acct-A', 'acct-B']);

initSavingsHistory();
const accountCb = (state.subscribeAccount as jest.Mock).mock.calls[0]?.[0] as () => void;
(getSavingsAnalytics as jest.Mock).mockClear();
accountCb();
await new Promise((r) => setTimeout(r, 0));

expect(getSavingsAnalytics).toHaveBeenCalledWith(
expect.objectContaining({ account_ids: ['acct-A', 'acct-B'] })
);
});

test('provider chip change re-queries the chart when purchases-tab is active', async () => {
addPurchasesTab(true);
(state.getCurrentProvider as jest.Mock).mockReturnValue('aws');
Expand Down
6 changes: 3 additions & 3 deletions frontend/src/__tests__/trends.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,11 @@ describe('loadTrends', () => {
expect(config.data.datasets[0].data).toEqual([null, null, null, null, 400, 400]);
});

test('forwards the single account chip and the window, never the provider when unset', async () => {
(state.getCurrentAccountIDs as jest.Mock).mockReturnValue(['acct-uuid-1']);
test('forwards every selected account and the window, never the provider when unset', async () => {
(state.getCurrentAccountIDs as jest.Mock).mockReturnValue(['acct-uuid-1', 'acct-uuid-2']);
await loadTrends();
const arg = (api.getAnalyticsTrends as jest.Mock).mock.calls[0][0];
expect(arg.accountID).toBe('acct-uuid-1');
expect(arg.accountIDs).toEqual(['acct-uuid-1', 'acct-uuid-2']);
expect(arg.provider).toBeUndefined();
expect(arg.start).toBe(trendsWindow(6).start.toISOString());
});
Expand Down
6 changes: 3 additions & 3 deletions frontend/src/api/analytics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ export interface AnalyticsTrendsResponse {
}

export interface AnalyticsTrendsFilter {
/** Top-bar account chip (cloud_accounts UUID); omitted means all accessible. */
accountID?: string;
/** Selected cloud_accounts UUIDs, sent as account_ids; empty or omitted means all accessible. */
accountIDs?: string[];
/** Narrows by_service server-side; omitted means every provider. */
provider?: string;
/** RFC 3339 window bounds. The backend caps the window at 366 days. */
Expand All @@ -68,7 +68,7 @@ export interface AnalyticsTrendsFilter {
*/
export async function getAnalyticsTrends(filter: AnalyticsTrendsFilter = {}): Promise<AnalyticsTrendsResponse> {
const params = new URLSearchParams();
if (filter.accountID) params.set('account_id', filter.accountID);
if (filter.accountIDs && filter.accountIDs.length > 0) params.set('account_ids', filter.accountIDs.join(','));
if (filter.provider) params.set('provider', filter.provider);
if (filter.start) params.set('start', filter.start);
if (filter.end) params.set('end', filter.end);
Expand Down
10 changes: 4 additions & 6 deletions frontend/src/modules/savings-history.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,9 @@ export async function loadSavingsHistory(): Promise<void> {
const period = periodSelect.value;
const { start, end, interval } = getPeriodDates(period);

// Honour the global topbar filter chips (issue #503). The account chip
// is single-select, and the backend's /history/analytics takes a single
// account_id (see handler_analytics.go), so we forward the only selected
// ID, mirroring dashboard.ts loadSavingsTrendChart. The provider chip is
// forwarded too and the backend now applies it as a provider WHERE filter
// Honour the global topbar filter chips (issue #503). Every selected
// account is forwarded as account_ids; the backend scope-checks each id
// and sums them. The provider chip is forwarded too and the backend now applies it as a provider WHERE filter
// on the savings-history query (issue #498, QA 2.3).
const currentProvider = state.getCurrentProvider();
const currentAccountIDs = state.getCurrentAccountIDs();
Expand All @@ -123,7 +121,7 @@ export async function loadSavingsHistory(): Promise<void> {
end: end.toISOString(),
interval,
...(currentProvider ? { provider: currentProvider } : {}),
...(currentAccountIDs.length === 1 ? { account_ids: currentAccountIDs } : {}),
...(currentAccountIDs.length > 0 ? { account_ids: currentAccountIDs } : {}),
});

if (!data.data_points || data.data_points.length === 0) {
Expand Down
6 changes: 2 additions & 4 deletions frontend/src/trends.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,6 @@ export async function loadTrends(): Promise<void> {

const provider = state.getCurrentProvider();
const accountIDs = state.getCurrentAccountIDs();
// The account chip is single-select; forward only when exactly one is set.
const accountID = accountIDs.length === 1 ? accountIDs[0] : undefined;
const { start, end } = trendsWindow(selectedPeriodMonths());

const seq = ++loadSeq;
Expand All @@ -91,14 +89,14 @@ export async function loadTrends(): Promise<void> {

try {
const data = await api.getAnalyticsTrends({
accountID,
accountIDs,
provider: provider || undefined,
start: start.toISOString(),
end: end.toISOString(),
});
if (seq !== loadSeq) return;
teardownSkeleton(container);
renderTrends(container, data, provider, monthKeysBetween(start, end), Boolean(provider || accountID));
renderTrends(container, data, provider, monthKeysBetween(start, end), Boolean(provider || accountIDs.length > 0));
} catch (error) {
if (seq !== loadSeq) return;
teardownSkeleton(container);
Expand Down
7 changes: 7 additions & 0 deletions internal/api/coverage_gaps_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,13 @@ func TestParseAccountIDs(t *testing.T) {
}
}

func TestParseAccountIDs_Canonical(t *testing.T) {
id := "abcdefab-abcd-abcd-abcd-abcdefabcdef"
ids, err := parseAccountIDs(" " + strings.ToUpper(id) + " ,\t" + id)
require.NoError(t, err)
assert.Equal(t, []string{id, id}, ids)
}

// TestParseAccountIDs_Cap verifies the MaxAccountIDsPerRequest guard
// blocks input amplification — an unbounded account_ids list could fan
// out into thousands of per-account DB queries / cloud API calls.
Expand Down
28 changes: 9 additions & 19 deletions internal/api/handler_analytics.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ package api
import (
"context"
"fmt"
"slices"
"time"

"github.com/LeanerCloud/cloud-commitments-platform/internal/analytics"
Expand Down Expand Up @@ -150,13 +149,13 @@ func (h *Handler) getHistoryAnalytics(ctx context.Context, req *events.LambdaFun
return nil, err
}

// For scoped users we require account_id and validate it's in their scope.
// We don't (yet) support analytics across a subset — the underlying
// aggregate takes a single account_id. An unrestricted/admin session
// can pass "" to mean "all accessible accounts". The requested account (a
// top-bar chip UUID, a raw external id, or "" for all) resolves to the
// dual-column filter inputs so rows that carry only the external account_id
// (cloud_account_id NULL) are aggregated (issue #701/#498/#866).
// Scoped users must name at least one account (account_ids or the legacy
// account_id) and every named account must be in their scope. An
// unrestricted/admin session can omit both to mean "all accessible
// accounts". The requested accounts (top-bar chip UUIDs, or a raw external
// id via the legacy param) resolve to the dual-column filter inputs so rows
// that carry only the external account_id (cloud_account_id NULL) are
// aggregated (issue #701/#498/#866).
requested, err := requestedAnalyticsAccounts(params)
if err != nil {
return nil, err
Expand Down Expand Up @@ -301,19 +300,10 @@ func (h *Handler) resolveAnalyticsAccountFilter(ctx context.Context, session *Se
// mergeAccountFilter unions one account's dual-column filter into the
// accumulated one, dropping duplicates.
func mergeAccountFilter(uuids []string, ext map[string][]string, addUUIDs []string, addExt map[string][]string) (mergedUUIDs []string, mergedExt map[string][]string) {
for _, u := range addUUIDs {
if !slices.Contains(uuids, u) {
uuids = append(uuids, u)
}
}
uuids = appendMissing(uuids, addUUIDs...)
for provider, ids := range addExt {
if ext == nil {
ext = map[string][]string{}
}
for _, id := range ids {
if !slices.Contains(ext[provider], id) {
ext[provider] = append(ext[provider], id)
}
ext = addExternalIDForProvider(ext, provider, id)
}
}
return uuids, ext
Expand Down
29 changes: 29 additions & 0 deletions internal/api/handler_analytics_account_ids_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,3 +127,32 @@ func TestAnalytics_AccountIDs_DuplicatesCollapse(t *testing.T) {
assert.Equal(t, []string{idsAcctA}, uuids)
assert.Equal(t, map[string][]string{"aws": {"111111111111"}}, externals)
}

// Issue #1452: parseAccountIDs canonicalises (trim + lowercase), so an
// uppercase or padded id behaves exactly like its lowercase form, including
// for a scoped user's own account.
func TestAnalytics_AccountIDs_UppercasePaddedCanonical(t *testing.T) {
t.Parallel()
// Hex letters so ToUpper really changes the ids.
const idA, idB = "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", "bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb"
accounts := []config.CloudAccount{
{ID: idA, Name: "a", Provider: "aws", ExternalID: "111111111111"},
{ID: idB, Name: "b", Provider: "aws", ExternalID: "222222222222"},
}
upper := " " + strings.ToUpper(idA) + " , " + strings.ToUpper(idB)
lower := idA + "," + idB
for _, path := range analyticsPaths {
for name, scope := range map[string][]string{"unrestricted": nil, "scoped": {idA, idB}} {
t.Run(path+" "+name, func(t *testing.T) {
t.Parallel()
wantStatus, wantUUIDs, wantExt := analyticsParamsCall(t, path, scope, map[string]string{"account_ids": lower}, accounts)
status, uuids, externals := analyticsParamsCall(t, path, scope, map[string]string{"account_ids": upper}, accounts)
assert.Equal(t, 200, wantStatus)
assert.Equal(t, wantStatus, status)
assert.Equal(t, []string{idA, idB}, uuids)
assert.Equal(t, wantUUIDs, uuids)
assert.Equal(t, wantExt, externals)
})
}
}
}
17 changes: 17 additions & 0 deletions internal/api/handler_history_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package api
import (
"context"
"errors"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -1082,6 +1083,22 @@ func TestHandler_getHistory_FilterParams(t *testing.T) {
assert.False(t, ids["exec-nil-account"], "execution with NULL exec.CloudAccountID AND no rec-level CloudAccountID must still be excluded (effective account is empty)")
})

t.Run("uppercase and padded account_ids are canonicalised (issue #1452)", func(t *testing.T) {
mockStore, handler, req, ctx := newHandler(t)

uuidA := "aaaaaaaa-1111-1111-1111-111111111111"
uuidB := "bbbbbbbb-2222-2222-2222-222222222222"

mockStore.On("ListPurchaseHistoryPage", ctx, config.PurchaseHistoryFilter{AccountIDs: []string{uuidA, uuidB}, Limit: historyStreamBatch(config.DefaultListLimit)}, noCursor).
Return([]config.PurchaseHistoryRecord{{AccountID: "acc-A", PurchaseID: "p-A"}}, nil).Once()
mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseExecution{}, nil)

result, err := handler.getHistory(ctx, req, map[string]string{"account_ids": " " + strings.ToUpper(uuidA) + " , " + strings.ToUpper(uuidB)})
require.NoError(t, err)
require.Len(t, result.(HistoryResponse).Purchases, 1)
mockStore.AssertExpectations(t)
})

t.Run("legacy singular account_id is folded into the dual-column filter", func(t *testing.T) {
// account_id (singular) is an AWS-style external account number (not a
// known cloud_accounts UUID in this fixture). resolveHistoryAccountFilter
Expand Down
3 changes: 2 additions & 1 deletion internal/api/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,7 @@ func validateRequestBodySize(body string) error {
const MaxAccountIDsPerRequest = 200

// parseAccountIDs splits a comma-separated account_ids query parameter into a slice.
// Ids are trimmed and lowercased so every caller compares canonical UUIDs.
// Empty entries are removed. Returns nil when the input is empty.
// Returns an error if any value is not a valid UUID, or if the list
// exceeds MaxAccountIDsPerRequest entries.
Expand Down Expand Up @@ -547,7 +548,7 @@ func parseAccountIDs(raw string) ([]string, error) {
if err := validateUUID(trimmed); err != nil {
return nil, fmt.Errorf("invalid account_ids: %q is not a valid UUID", trimmed)
}
ids = append(ids, trimmed)
ids = append(ids, strings.ToLower(trimmed))
}
return ids, nil
}
Expand Down
Loading