Skip to content

Commit 7655417

Browse files
committed
fix: address review — plan swap detection and deduplicate API calls
- Fix swap detection for "plan" pseudo-trial: `sentry trial start my-org plan` now correctly auto-corrects to `sentry trial start plan my-org` instead of giving a confusing "Unknown trial name" error - Deduplicate getProductTrials to delegate to getCustomerTrialInfo instead of making its own identical API call - Add test for plan argument swap detection
1 parent 21435fc commit 7655417

File tree

3 files changed

+52
-25
lines changed

3 files changed

+52
-25
lines changed

src/commands/trial/start.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,16 @@ const NAMES_LIST = `${VALID_NAMES.join(", ")}, plan`;
4545
*
4646
* @returns Parsed name and optional org, plus any warning message
4747
*/
48+
/**
49+
* Check if a string is a valid trial name, including the "plan" pseudo-trial.
50+
*
51+
* Used for swap detection so `sentry trial start my-org plan` is auto-corrected
52+
* to `sentry trial start plan my-org`.
53+
*/
54+
function isValidTrialArg(name: string): boolean {
55+
return name === "plan" || isTrialName(name);
56+
}
57+
4858
function parseTrialStartArgs(
4959
first: string,
5060
second?: string
@@ -54,8 +64,8 @@ function parseTrialStartArgs(
5464
return { name: first };
5565
}
5666

57-
// Two args — check for swapped order
58-
const swapped = detectSwappedTrialArgs(first, second, isTrialName);
67+
// Two args — check for swapped order (includes "plan" pseudo-trial)
68+
const swapped = detectSwappedTrialArgs(first, second, isValidTrialArg);
5969
if (swapped) {
6070
return { name: swapped.name, org: swapped.org, warning: swapped.warning };
6171
}

src/lib/api/trials.ts

Lines changed: 16 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -15,48 +15,41 @@ import { getControlSiloUrl } from "../sentry-client.js";
1515
import { apiRequestToRegion } from "./infrastructure.js";
1616

1717
/**
18-
* Fetch all product trials for an organization.
18+
* Fetch full customer trial info including plan trial status.
1919
*
20-
* Fetches customer data from the internal `/customers/{org}/` endpoint
21-
* and returns the `productTrials` array. This is a getsentry SaaS-only
22-
* endpoint — self-hosted instances will 404, which callers should handle.
20+
* Returns the complete {@link CustomerTrialInfo} including both product trials
21+
* and plan-level trial info (`canTrial`, `isTrial`, `trialEnd`, `planDetails`).
2322
*
2423
* @param orgSlug - Organization slug
25-
* @returns Array of product trials (may be empty)
24+
* @returns Customer trial info with product trials, plan trial status, and plan details
2625
*/
27-
export async function getProductTrials(
26+
export async function getCustomerTrialInfo(
2827
orgSlug: string
29-
): Promise<ProductTrial[]> {
28+
): Promise<CustomerTrialInfo> {
3029
// /customers/ is a control silo endpoint (billing), not region-scoped
3130
const { data } = await apiRequestToRegion<CustomerTrialInfo>(
3231
getControlSiloUrl(),
3332
`/customers/${orgSlug}/`,
3433
{ schema: CustomerTrialInfoSchema }
3534
);
36-
return data.productTrials ?? [];
35+
return data;
3736
}
3837

3938
/**
40-
* Fetch full customer trial info including plan trial status.
41-
*
42-
* Returns the complete {@link CustomerTrialInfo} including both product trials
43-
* and plan-level trial info (`canTrial`, `isTrial`, `trialEnd`, `planDetails`),
44-
* which needs to display plan trials alongside product trials.
39+
* Fetch all product trials for an organization.
4540
*
46-
* For simpler use cases that only need product trials, use {@link getProductTrials}.
41+
* Convenience wrapper around {@link getCustomerTrialInfo} that returns
42+
* just the `productTrials` array. This is a getsentry SaaS-only endpoint —
43+
* self-hosted instances will 404, which callers should handle.
4744
*
4845
* @param orgSlug - Organization slug
49-
* @returns Customer trial info with product trials, plan trial status, and plan details
46+
* @returns Array of product trials (may be empty)
5047
*/
51-
export async function getCustomerTrialInfo(
48+
export async function getProductTrials(
5249
orgSlug: string
53-
): Promise<CustomerTrialInfo> {
54-
const { data } = await apiRequestToRegion<CustomerTrialInfo>(
55-
getControlSiloUrl(),
56-
`/customers/${orgSlug}/`,
57-
{ schema: CustomerTrialInfoSchema }
58-
);
59-
return data;
50+
): Promise<ProductTrial[]> {
51+
const info = await getCustomerTrialInfo(orgSlug);
52+
return info.productTrials ?? [];
6053
}
6154

6255
/**

test/commands/trial/start.test.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,30 @@ describe("trial start command", () => {
209209
expect(getProductTrialsSpy).toHaveBeenCalledWith("my-org");
210210
});
211211

212+
test("detects swapped arguments for plan pseudo-trial", async () => {
213+
resolveOrgSpy.mockResolvedValue({ org: "my-org" });
214+
const getInfoSpy = spyOn(
215+
apiClient,
216+
"getCustomerTrialInfo"
217+
).mockResolvedValue({
218+
productTrials: [],
219+
canTrial: true,
220+
isTrial: false,
221+
planDetails: { name: "Developer" },
222+
} as CustomerTrialInfo);
223+
224+
const { context } = createMockContext();
225+
const func = await startCommand.loader();
226+
// Swapped: org first, "plan" second
227+
await func.call(context, { json: true }, "my-org", "plan");
228+
229+
// Should resolve correctly despite swap
230+
expect(resolveOrgSpy).toHaveBeenCalledWith(
231+
expect.objectContaining({ org: "my-org" })
232+
);
233+
getInfoSpy.mockRestore();
234+
});
235+
212236
test("starts replays trial", async () => {
213237
const replaysTrial: ProductTrial = {
214238
...MOCK_TRIAL,

0 commit comments

Comments
 (0)