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
131 changes: 131 additions & 0 deletions static/app/components/pipeline/pipelineIntegrationVsts.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
import {act, render, screen, userEvent} from 'sentry-test/reactTestingLibrary';

import {vstsIntegrationPipeline} from './pipelineIntegrationVsts';
import type {PipelineStepProps} from './types';

const VstsOAuthLoginStep = vstsIntegrationPipeline.steps[0].component;
const VstsAccountSelectionStep = vstsIntegrationPipeline.steps[1].component;

function makeStepProps<D, A>(
overrides: Partial<PipelineStepProps<D, A>> & {stepData: D}
): PipelineStepProps<D, A> {
return {
advance: jest.fn(),
advanceError: null,
isAdvancing: false,
stepIndex: 0,
totalSteps: 2,
...overrides,
};
}

let mockPopup: Window;

function dispatchPipelineMessage({
data,
origin = document.location.origin,
source = mockPopup,
}: {
data: Record<string, string>;
origin?: string;
source?: Window | MessageEventSource | null;
}) {
act(() => {
const event = new MessageEvent('message', {data, origin});
Object.defineProperty(event, 'source', {value: source});
window.dispatchEvent(event);
});
}

beforeEach(() => {
mockPopup = {
closed: false,
close: jest.fn(),
focus: jest.fn(),
} as unknown as Window;
jest.spyOn(window, 'open').mockReturnValue(mockPopup);
});

afterEach(() => {
jest.restoreAllMocks();
});

describe('VstsOAuthLoginStep', () => {
it('renders the OAuth login step for Azure DevOps', () => {
render(
<VstsOAuthLoginStep
{...makeStepProps({
stepData: {
oauthUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/authorize',
},
})}
/>
);

expect(
screen.getByRole('button', {name: 'Authorize Azure DevOps'})
).toBeInTheDocument();
});

it('calls advance with code and state on OAuth callback', async () => {
const advance = jest.fn();
render(
<VstsOAuthLoginStep
{...makeStepProps({
stepData: {
oauthUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/authorize',
},
advance,
})}
/>
);

await userEvent.click(screen.getByRole('button', {name: 'Authorize Azure DevOps'}));

dispatchPipelineMessage({
data: {
_pipeline_source: 'sentry-pipeline',
code: 'auth-code-123',
state: 'state-xyz',
},
});

expect(advance).toHaveBeenCalledWith({
code: 'auth-code-123',
state: 'state-xyz',
});
});
});

describe('VstsAccountSelectionStep', () => {
it('shows a no accounts message when no Azure DevOps organizations are available', () => {
render(<VstsAccountSelectionStep {...makeStepProps({stepData: {accounts: []}})} />);

expect(
screen.getByText(
'No Azure DevOps organizations were found for this account. Make sure you are an owner or admin on the Azure DevOps organization you want to connect.'
)
).toBeInTheDocument();
});

it('calls advance when selecting an Azure DevOps organization', async () => {
const advance = jest.fn();
render(
<VstsAccountSelectionStep
{...makeStepProps({
stepData: {
accounts: [{accountId: 'acct-1', accountName: 'MyVSTSAccount'}],
},
advance,
})}
/>
);

await userEvent.click(
screen.getByRole('button', {name: 'Select Azure DevOps organization'})
);
await userEvent.click(await screen.findByText('MyVSTSAccount'));

expect(advance).toHaveBeenCalledWith({account: 'acct-1'});
});
});
106 changes: 106 additions & 0 deletions static/app/components/pipeline/pipelineIntegrationVsts.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
import {useCallback} from 'react';

import {Alert} from '@sentry/scraps/alert';
import {Stack} from '@sentry/scraps/layout';
import {Text} from '@sentry/scraps/text';

import {DropdownMenu} from 'sentry/components/dropdownMenu';
import {t} from 'sentry/locale';
import type {IntegrationWithConfig} from 'sentry/types/integrations';

import type {OAuthCallbackData} from './shared/oauthLoginStep';
import {OAuthLoginStep} from './shared/oauthLoginStep';
import type {PipelineDefinition, PipelineStepProps} from './types';
import {pipelineComplete} from './types';

interface VstsAccount {
accountId: string;
accountName: string;
}

interface VstsAccountSelectionStepData {
accounts?: VstsAccount[];
}

interface VstsAccountSelectionAdvanceData {
account: string;
}

function VstsOAuthLoginStep({
stepData,
advance,
isAdvancing,
}: PipelineStepProps<{oauthUrl?: string}, {code: string; state: string}>) {
const handleOAuthCallback = useCallback(
(data: OAuthCallbackData) => {
advance({code: data.code, state: data.state});
},
[advance]
);

return (
<OAuthLoginStep
oauthUrl={stepData.oauthUrl}
isLoading={isAdvancing}
serviceName="Azure DevOps"
onOAuthCallback={handleOAuthCallback}
/>
);
}

function VstsAccountSelectionStep({
stepData,
advance,
isAdvancing,
}: PipelineStepProps<VstsAccountSelectionStepData, VstsAccountSelectionAdvanceData>) {
const accounts = stepData.accounts ?? [];

if (accounts.length === 0) {
return (
<Alert variant="info">
{t(
'No Azure DevOps organizations were found for this account. Make sure you are an owner or admin on the Azure DevOps organization you want to connect.'
)}
</Alert>
);
}

return (
<Stack gap="lg" align="start">
<Text>
{t('Select the Azure DevOps organization you want to connect to Sentry.')}
</Text>
<DropdownMenu
triggerLabel={t('Select Azure DevOps organization')}
items={accounts.map(account => ({
key: account.accountId,
label: account.accountName,
}))}
isDisabled={isAdvancing}
onAction={key => {
advance({account: key as string});
}}
/>
Comment on lines +73 to +83
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The onAction handler is passed as a top-level prop to DropdownMenu, but Sentry's implementation never invokes it, so advance is never called.
Severity: HIGH

Suggested Fix

Move onAction to each item in the items array instead of passing it as a top-level DropdownMenu prop: items={accounts.map(account => ({ key: account.accountId, label: account.accountName, onAction: () => advance({account: account.accountId}) }))} and remove the top-level onAction prop.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: static/app/components/pipeline/pipelineIntegrationVsts.tsx#L73-L83

Potential issue: In `VstsAccountSelectionStep`, the `onAction` callback is passed as a
top-level prop to the `<DropdownMenu>` component (line 80). However, Sentry's custom
`DropdownMenuItem` always overrides react-aria's menu-level `onAction` by passing its
own `actionHandler` to `useMenuItem({ onAction: actionHandler })` (item.tsx:178). This
`actionHandler` only calls the **item-level** `onAction?.()` from `node.value`
(item.tsx:133), which is undefined here since items don't define `onAction`.
Consequently, selecting an account does nothing — `advance` is never called, and the
pipeline can never progress past the account selection step.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a real issue. The top-level onAction prop flows through {...props} into DropdownMenuList, which passes it to react-aria's useMenu(). React-aria fires the menu-level onAction(key) when any item is selected, independently of item-level onAction. The test in pipelineIntegrationVsts.spec.tsx confirms this works correctly.

</Stack>
);
}

export const vstsIntegrationPipeline = {
type: 'integration',
provider: 'vsts',
actionTitle: t('Installing Azure DevOps Integration'),
getCompletionData: pipelineComplete<IntegrationWithConfig>,
completionView: null,
steps: [
{
stepId: 'oauth_login',
shortDescription: t('Authorizing via Azure DevOps OAuth'),
component: VstsOAuthLoginStep,
},
{
stepId: 'account_selection',
shortDescription: t('Selecting Azure DevOps organization'),
component: VstsAccountSelectionStep,
},
],
} as const satisfies PipelineDefinition;
2 changes: 2 additions & 0 deletions static/app/components/pipeline/registry.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {bitbucketIntegrationPipeline} from './pipelineIntegrationBitbucket';
import {githubIntegrationPipeline} from './pipelineIntegrationGitHub';
import {gitlabIntegrationPipeline} from './pipelineIntegrationGitLab';
import {slackIntegrationPipeline} from './pipelineIntegrationSlack';
import {vstsIntegrationPipeline} from './pipelineIntegrationVsts';

/**
* All registered pipeline definitions.
Expand All @@ -15,6 +16,7 @@ export const PIPELINE_REGISTRY = [
githubIntegrationPipeline,
gitlabIntegrationPipeline,
slackIntegrationPipeline,
vstsIntegrationPipeline,
] as const;

type AllPipelines = (typeof PIPELINE_REGISTRY)[number];
Expand Down
Loading