Skip to content

Commit ea2f79e

Browse files
committed
docs(ui): trim the Azure onboarding comments to what they explain
- Drop comments that restate the code or the test title they sit under - Keep each rationale where it belongs instead of repeating it per call site - Reattach the AWS hierarchy fixture doc to the fixture it documents
1 parent 475b7c5 commit ea2f79e

16 files changed

Lines changed: 93 additions & 154 deletions

File tree

ui/__tests__/msw/handlers/organizations.fixtures.ts

Lines changed: 17 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -143,10 +143,7 @@ export interface FixtureDiscovery {
143143
result: unknown;
144144
/** Machine error code — never user copy. */
145145
error: string | null;
146-
/**
147-
* Sanitized human message the server sends alongside the code. Optional: it
148-
* only exists for codes the API decided to explain itself.
149-
*/
146+
/** Sanitized human message the server sends alongside the code, when it has one. */
150147
errorMessage?: string | null;
151148
}
152149

@@ -478,10 +475,9 @@ export const AZURE_SUBSCRIPTION_DISABLED =
478475
"66666666-6666-4666-8666-666666666666";
479476

480477
/**
481-
* Blocked reasons Azure discovery reports. It reuses GCP's `*_conflict`
482-
* vocabulary for the three linkage/type conflicts and adds one of its own:
483-
* `subscription_not_enabled`, raised whenever `state != "Enabled"`, which is the
484-
* only reason that can block a subscription with no provider involved at all.
478+
* Blocked reasons Azure discovery reports: GCP's `*_conflict` vocabulary for the
479+
* three linkage/type conflicts, plus its own `subscription_not_enabled`, raised
480+
* whenever `state != "Enabled"`.
485481
*/
486482
export const AZURE_BLOCKED_REASON = {
487483
ORGANIZATION: "organization_conflict",
@@ -495,7 +491,7 @@ interface AzureResultOverrides {
495491
replaceSubscriptionIds?: string[];
496492
}
497493

498-
/** Pinned to the app's wire interfaces, for the reason `GcpFixtureDiscoveryResult` is. */
494+
/** Pinned to the app's wire interfaces, for the same reason `GcpFixtureDiscoveryResult` is. */
499495
type AzureFixtureDiscoveryResult = Omit<
500496
AzureDiscoveryResult,
501497
"subscriptions"
@@ -506,11 +502,9 @@ type AzureFixtureDiscoveryResult = Omit<
506502
};
507503

508504
/**
509-
* The Azure discovery result as the API shapes it: management groups carry
510-
* canonical resource IDs in `id`/`parent_id`, subscriptions are identified by
511-
* their UUID and parent through their group's resource ID, and `display_name` is
512-
* the only human label. Subscription UUIDs are long by nature, so the id-column
513-
* overflow case needs no special candidate here.
505+
* The Azure discovery result as the API shapes it: groups carry canonical resource
506+
* IDs in `id`/`parent_id`, subscriptions are identified by their UUID. Those UUIDs
507+
* are long by nature, so the id-column overflow case needs no special candidate.
514508
*/
515509
export const buildAzureDiscoveryResult = ({
516510
replaceSubscriptionIds = [],
@@ -596,8 +590,6 @@ export const buildAzureDiscoveryResult = ({
596590
provider_secret_state: PROVIDER_SECRET_STATE.WILL_REPLACE,
597591
}),
598592
),
599-
// Blocked with no provider and no conflict: the state check alone is
600-
// enough, which is the class the linkage-conflict cases cannot cover.
601593
subscription(
602594
AZURE_SUBSCRIPTION_DISABLED,
603595
"Dormant Sandbox",
@@ -783,13 +775,13 @@ export const azureOnboardingFixture = (
783775
};
784776
};
785777

778+
/** The AWS organization identifier of `awsHierarchyFixture`. */
779+
export const AWS_HIERARCHY_ORG_EXTERNAL_ID = "o-aws0abcdef";
780+
786781
/**
787782
* A providers-page hierarchy world with a fully onboarded AWS organization
788783
* (two OUs, three providers). Used for the providers-table grouping tests.
789784
*/
790-
/** The AWS organization identifier of `awsHierarchyFixture`. */
791-
export const AWS_HIERARCHY_ORG_EXTERNAL_ID = "o-aws0abcdef";
792-
793785
export const awsHierarchyFixture = (
794786
overrides: Partial<OrgFixture> = {},
795787
): OrgFixture => {
@@ -898,9 +890,8 @@ export const mixedHierarchyFixture = (
898890
kind: NODE_KIND.MANAGEMENT_GROUP,
899891
name: AZURE_HIERARCHY_GROUP_NAME,
900892
externalId: AZURE_HIERARCHY_GROUP,
901-
// The tenant-root Management Group is never persisted as a node — nodes
902-
// exist only for selected descendant groups and their ancestors — so it
903-
// appears here as a parent id that resolves to no node row.
893+
// The tenant-root Management Group is never persisted as a node, so this
894+
// parent id deliberately resolves to no node row.
904895
parentExternalId: AZURE_ROOT_GROUP,
905896
organizationId: azureOrgId,
906897
providerIds: ["azp-1"],
@@ -988,12 +979,10 @@ export const mixedHierarchyFixture = (
988979
};
989980

990981
/**
991-
* An `org_type` this build has no onboarding flow for. Every value of
992-
* `ORGANIZATION_TYPE` is onboardable now that Azure has a flow, so the
993-
* display-only behaviour has to be exercised through a type the enum itself does
994-
* not carry — which is also the real case: the enum mirrors a server-side one.
995-
* `oraclecloud` is a real provider type, so the provider rows underneath it still
996-
* render coherently.
982+
* An `org_type` this build has no onboarding flow for. Every `ORGANIZATION_TYPE`
983+
* value is onboardable now, so this has to come from outside the enum — which
984+
* mirrors the real case, the enum tracking a server-side one. `oraclecloud` is a
985+
* real provider type, so its provider rows still render coherently.
997986
*/
998987
export const DISPLAY_ONLY_ORG_TYPE = "oraclecloud";
999988
export const DISPLAY_ONLY_ORG_NAME = "My Oracle Cloud Tenancy";

ui/__tests__/msw/handlers/organizations.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -507,8 +507,7 @@ export const handlersForOrganizations = (
507507
result:
508508
fx.discovery.status === "succeeded" ? fx.discovery.result : {},
509509
error: fx.discovery.error,
510-
// Machine code and human message are separate fields; the message
511-
// is only sent for the codes the API explains itself.
510+
// Machine code and human message are separate fields on the wire.
512511
error_message: fx.discovery.errorMessage ?? null,
513512
inserted_at: TS,
514513
updated_at: TS,

ui/actions/organizations/organizations.test.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -86,10 +86,9 @@ describe("organizations actions", () => {
8686
});
8787

8888
it("rejects an organization type with no onboarding flow instead of coercing it", async () => {
89-
// Given a form asking for a type this build cannot onboard. `oraclecloud` is
90-
// a real provider the API can report an organization for — display supports
91-
// it, onboarding does not — so it is the exact boundary a blind cast would
92-
// let through.
89+
// Given a type this build cannot onboard. `oraclecloud` is real — display
90+
// supports it, onboarding does not — the exact boundary a blind cast lets
91+
// through.
9392
const formData = new FormData();
9493
formData.set("name", "Tenancy");
9594
formData.set("externalId", "ocid1.tenancy.oc1..aaaa1111");

ui/app/(prowler)/providers/providers-page.harness.tsx

Lines changed: 11 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,8 @@ interface MountOptions {
4040

4141
/**
4242
* Attributes a `POST /organizations` carries. `root_external_id` is deliberately
43-
* absent: the API derives every type's root, so a test asserting the client does
44-
* not send one reads it off the parsed body, not off this shape.
43+
* absent: a test asserting the client sends none reads the parsed body, not this
44+
* shape.
4545
*/
4646
export interface OrganizationCreateAttributes {
4747
name?: string;
@@ -267,11 +267,7 @@ export class ProvidersPageHarness extends BrowserHarness<OrgFixture> {
267267

268268
// --- Wizard: Azure setup step ------------------------------------------
269269

270-
/**
271-
* Fill the Azure organization-details phase. The tenant is the only identifier
272-
* collected: onboarding is always scoped to the tenant root Management Group,
273-
* which the API derives from it.
274-
*/
270+
/** Fill the Azure organization-details phase (the tenant is the only id asked for). */
275271
async fillAzureOrgDetails(tenantId: string, name?: string): Promise<void> {
276272
const tenantInput = await this.waitFor(() => this.inputByName("tenantId"));
277273
await this.user.fill(tenantInput, tenantId);
@@ -369,8 +365,7 @@ export class ProvidersPageHarness extends BrowserHarness<OrgFixture> {
369365

370366
/**
371367
* Whether the pre-apply warning states how many already-onboarded candidates the
372-
* apply would overwrite, and names them. Noun-bound per organization type: the
373-
* warning saying the wrong noun is the failure this pins.
368+
* apply would overwrite, and names them — in the noun this organization type uses.
374369
*/
375370
private hasOverwriteWarningFor(
376371
count: number,
@@ -392,7 +387,7 @@ export class ProvidersPageHarness extends BrowserHarness<OrgFixture> {
392387
return this.hasOverwriteWarningFor(projectCount, names, "project");
393388
}
394389

395-
/** The GCP counterpart above, in Azure's noun. */
390+
/** The same warning, in Azure's noun. */
396391
hasApplySubscriptionOverwriteWarning(
397392
subscriptionCount: number,
398393
names: string[] = [],
@@ -425,10 +420,7 @@ export class ProvidersPageHarness extends BrowserHarness<OrgFixture> {
425420
await this.waitForText(reason, timeoutMs);
426421
}
427422

428-
/**
429-
* Whether that reason is showing — the negative half, for asserting which of
430-
* the competing failure strings the user was actually given.
431-
*/
423+
/** Whether that reason is showing — the negative half of the assertion. */
432424
hasDiscoveryFailureReason(reason: RegExp): boolean {
433425
return this.containsText(reason);
434426
}
@@ -484,11 +476,9 @@ export class ProvidersPageHarness extends BrowserHarness<OrgFixture> {
484476
}
485477

486478
/**
487-
* A container row identifies itself by its uid: a GCP folder ref, an AWS OU id,
488-
* or an Azure Management Group resource id. Anchored, because it is matched
489-
* against one element's own value — never the whole row, whose uid and name
490-
* columns are adjacent and would run together as
491-
* "…/managementGroups/archived" + "Archived".
479+
* A container row's uid: a GCP folder ref, an AWS OU id, or an Azure Management
480+
* Group resource id. Anchored because it is matched against one element's own
481+
* value — the adjacent uid and name columns would otherwise run together.
492482
*/
493483
private static readonly CONTAINER_UID =
494484
/^(?:folders\/\d+|ou-[\w-]+|\/providers\/Microsoft\.Management\/managementGroups\/[\w.()-]+)$/;
@@ -574,8 +564,8 @@ export class ProvidersPageHarness extends BrowserHarness<OrgFixture> {
574564
const row = this.containerRows.find((item) =>
575565
(item.textContent ?? "").includes(containerLabel),
576566
);
577-
// The note is the icon-only `role="img"`; the id column carries the role too,
578-
// so that it can name itself with the canonical id, and comes first in the row.
567+
// The id column carries `role="img"` too, and comes first in the row, so the
568+
// note is the one matched by its svg.
579569
return (
580570
row?.querySelector('[role="img"]:has(svg)')?.getAttribute("aria-label") ??
581571
null

ui/app/(prowler)/providers/providers-page.integration.test.tsx

Lines changed: 14 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -165,11 +165,7 @@ const AZURE_CLIENT_ID = "99999999-9999-4999-8999-999999999999";
165165
const AZURE_CLIENT_SECRET = "azure-client-secret";
166166
const AZURE_ORG_DOCS = "prowler-cloud-azure-management-groups";
167167

168-
/**
169-
* Drive a fresh Azure org onboarding up to the authentication submit. The tenant
170-
* is the only identifier collected: onboarding is always scoped to the tenant
171-
* root Management Group, which the API derives from it.
172-
*/
168+
/** Drive a fresh Azure org onboarding up to the authentication submit. */
173169
async function authenticateAzureOrg(
174170
harness: ProvidersPageHarness,
175171
{ name }: { name?: string } = {},
@@ -688,9 +684,8 @@ describe("Organization onboarding wizard", () => {
688684
expect(harness.hasSelectedSubscriptionCount(2, 2)).toBe(true);
689685
expect(harness.usesAccountWording()).toBe(false);
690686

691-
// The tenant is the whole identity on the wire: onboarding is only ever
692-
// scoped to the tenant root Management Group, and the API derives that root
693-
// itself — a client that sends one is claiming a choice it does not have.
687+
// The tenant is the whole identity on the wire: the API derives the root
688+
// Management Group itself, so the client must not send one.
694689
const created = await harness.createdOrganizationAttributes();
695690
expect(created?.org_type).toBe(ORGANIZATION_TYPE.AZURE);
696691
expect(created?.external_id).toBe(AZURE_TENANT_ID);
@@ -719,8 +714,8 @@ describe("Organization onboarding wizard", () => {
719714
"POST",
720715
"/organization-secrets",
721716
);
722-
// The tenant lives on the organization, so repeating it inside the secret
723-
// would be a second, silently divergent source of truth.
717+
// The tenant lives on the organization; repeating it inside the secret
718+
// would be a second source of truth.
724719
expect(secret?.data.attributes.secret).toEqual({
725720
client_id: AZURE_CLIENT_ID,
726721
client_secret: AZURE_CLIENT_SECRET,
@@ -772,9 +767,8 @@ describe("Organization onboarding wizard", () => {
772767
AZURE_GROUP_PLATFORM,
773768
]);
774769

775-
// Every group's canonical id repeats the same ARM prefix, which is all a
776-
// 176px column can show, so the id column reads the trailing Management
777-
// Group name instead, keeping the canonical id as the accessible name.
770+
// Every group's canonical id repeats the same ARM prefix, so the id column
771+
// shows the trailing name and keeps the canonical id as its accessible name.
778772
expect(harness.containerIdLabel(AZURE_GROUP_ENGINEERING)).toBe(
779773
"engineering",
780774
);
@@ -811,8 +805,6 @@ describe("Organization onboarding wizard", () => {
811805
const harness = new ProvidersPageHarness(azureOnboardingFixture());
812806
await onboardAzureToSelection(harness);
813807

814-
// A subscription-less group still reaches the tree, and has to say so in
815-
// Azure's nouns.
816808
expect(harness.isContainerInert(AZURE_EMPTY_GROUP_NAME)).toBe(true);
817809
expect(harness.inertContainerNote(AZURE_EMPTY_GROUP_NAME)).toBe(
818810
"No subscriptions available to select in this management group.",
@@ -835,8 +827,7 @@ describe("Organization onboarding wizard", () => {
835827
),
836828
).toBe(true);
837829

838-
// The row collapses and re-expands rather than selecting: an inert group that
839-
// could not be opened would never explain itself.
830+
// The row collapses and re-expands rather than selecting.
840831
await harness.clickContainerRow(AZURE_BLOCKED_GROUP_NAME);
841832
await harness.waitForTransition();
842833
expect(
@@ -1033,8 +1024,8 @@ describe("Organization onboarding wizard", () => {
10331024
const harness = new ProvidersPageHarness(fixture);
10341025
await authenticateAzureOrg(harness);
10351026

1036-
// A code we curate copy for outranks the server's own message: ours names the
1037-
// fix, and the framing is right — the credentials are fine here.
1027+
// Curated copy for a known code outranks the server's own message: ours
1028+
// names the fix, and the credentials are not the problem here.
10381029
await harness.waitForDiscoveryFailureReason(
10391030
/Grant it the Reader role at the Management Group level/,
10401031
);
@@ -1045,7 +1036,6 @@ describe("Organization onboarding wizard", () => {
10451036
).toBe(false);
10461037
await harness.waitForDiscoveryCount(1);
10471038

1048-
// Retry triggers a brand-new discovery, not a resumed poll.
10491039
await harness.retryDiscovery();
10501040
await harness.waitForDiscoveryCount(2);
10511041
}, 40000);
@@ -1064,9 +1054,8 @@ describe("Organization onboarding wizard", () => {
10641054
const harness = new ProvidersPageHarness(fixture);
10651055
await authenticateAzureOrg(harness);
10661056

1067-
// Codes outrun the copy table, so an unmapped one must still say something
1068-
// specific — the sanitized server message, never the raw code, and never
1069-
// "authentication failed" for a problem that is not the credentials.
1057+
// An unmapped code must still be specific: the sanitized server message,
1058+
// never the raw code and never "authentication failed".
10701059
await harness.waitForDiscoveryFailureReason(
10711060
/Azure throttled the Management Group read for this tenant\./,
10721061
);
@@ -1326,8 +1315,8 @@ describe("Providers page", () => {
13261315
"PATCH",
13271316
`/organizations/${AWS_HIERARCHY_ORG_ID}`,
13281317
);
1329-
// Blank falls back to the identifier, the same rule creation applies —
1330-
// the action rejects an empty name outright.
1318+
// Same rule as creation: the action rejects an empty name, so the client
1319+
// substitutes the identifier.
13311320
expect(body?.data.attributes.name).toBe(AWS_HIERARCHY_ORG_EXTERNAL_ID);
13321321
}, 30000);
13331322

ui/components/providers/organizations/hooks/org-setup-strategy.test.ts

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -39,22 +39,17 @@ const SETUP_DATA = {
3939
},
4040
} as const satisfies Record<OrgFlowType, OrgSetupSubmissionData>;
4141

42-
/**
43-
* The structure each flow's hierarchy is made of. A tenant has no folders and an
44-
* organization has no Management Groups, so copy for a code every provider
45-
* reports has to pick the right one.
46-
*/
42+
/** The structure each flow's hierarchy is made of — a tenant has no folders. */
4743
const HIERARCHY_WORDING = {
4844
[ORGANIZATION_TYPE.AWS]: "organizational unit hierarchy",
4945
[ORGANIZATION_TYPE.AZURE]: "Management Group hierarchy",
5046
[ORGANIZATION_TYPE.GCP]: "folder hierarchy",
5147
} as const satisfies Record<OrgFlowType, string>;
5248

5349
describe("bindOrgSetupStrategy", () => {
54-
// `hierarchy_depth_exceeded` is one API code for every provider. The
55-
// precedence chain (curated → server message → auth failure) is covered by the
56-
// submission hook's suite; what only an exhaustive per-flow check can catch is
57-
// one flow being told about another cloud's structure.
50+
// `hierarchy_depth_exceeded` is one API code for every provider. The precedence
51+
// chain is covered by the submission hook's suite; only an exhaustive per-flow
52+
// check catches one flow being told about another cloud's structure.
5853
it.each(ORG_FLOW_TYPES)(
5954
"describes %s's own hierarchy for the shared hierarchy_depth_exceeded code",
6055
(orgType) => {
@@ -86,10 +81,8 @@ describe("bindOrgSetupStrategy", () => {
8681
},
8782
);
8883

89-
// The API stores an Azure tenant as `str(UUID(...))` — canonical lowercase —
90-
// and `filter[external_id]` is an exact lookup, so a tenant typed in uppercase
91-
// has to reach find-or-create already folded. Unfolded, the second wizard run
92-
// misses its own organization and then collides on the POST.
84+
// `filter[external_id]` is an exact lookup, so an uppercase-typed tenant would
85+
// miss its own organization on a second run and then collide on the POST.
9386
it("folds an uppercase Azure tenant ID to the API's canonical form", () => {
9487
const bound = bindOrgSetupStrategy({
9588
...SETUP_DATA[ORGANIZATION_TYPE.AZURE],

0 commit comments

Comments
 (0)