-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(onboarding): add messaging integration onboarding to project creation #79194
base: master
Are you sure you want to change the base?
Changes from 5 commits
8aa9f38
43e4ea7
565c186
35032f2
a9db89b
68d0db2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -35,6 +35,10 @@ import useApi from 'sentry/utils/useApi'; | |||||||||||||||||||||||
import {useLocation} from 'sentry/utils/useLocation'; | ||||||||||||||||||||||||
import useOrganization from 'sentry/utils/useOrganization'; | ||||||||||||||||||||||||
import {useTeams} from 'sentry/utils/useTeams'; | ||||||||||||||||||||||||
import { | ||||||||||||||||||||||||
MultipleCheckboxOptions, | ||||||||||||||||||||||||
useCreateNotificationAction, | ||||||||||||||||||||||||
} from 'sentry/views/projectInstall/issueAlertNotificationOptions'; | ||||||||||||||||||||||||
import IssueAlertOptions, { | ||||||||||||||||||||||||
MetricValues, | ||||||||||||||||||||||||
RuleAction, | ||||||||||||||||||||||||
|
@@ -82,6 +86,18 @@ function CreateProject() { | |||||||||||||||||||||||
undefined | ||||||||||||||||||||||||
); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const { | ||||||||||||||||||||||||
createNotificationAction, | ||||||||||||||||||||||||
actions: alertNotificationActions, | ||||||||||||||||||||||||
provider, | ||||||||||||||||||||||||
integration, | ||||||||||||||||||||||||
channel, | ||||||||||||||||||||||||
setActions: setAlertNotificationActions, | ||||||||||||||||||||||||
setProvider, | ||||||||||||||||||||||||
setIntegration, | ||||||||||||||||||||||||
setChannel, | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you actually need to destructure all these? Are you able to just have it look like this
Suggested change
|
||||||||||||||||||||||||
} = useCreateNotificationAction(); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const frameworkSelectionEnabled = !!organization?.features.includes( | ||||||||||||||||||||||||
'onboarding-sdk-selection' | ||||||||||||||||||||||||
); | ||||||||||||||||||||||||
|
@@ -90,6 +106,7 @@ function CreateProject() { | |||||||||||||||||||||||
async (selectedFramework?: OnboardingSelectedSDK) => { | ||||||||||||||||||||||||
const {slug} = organization; | ||||||||||||||||||||||||
const { | ||||||||||||||||||||||||
shouldCreateRule, | ||||||||||||||||||||||||
shouldCreateCustomRule, | ||||||||||||||||||||||||
name, | ||||||||||||||||||||||||
conditions, | ||||||||||||||||||||||||
|
@@ -121,7 +138,7 @@ function CreateProject() { | |||||||||||||||||||||||
}, | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
let ruleId: string | undefined; | ||||||||||||||||||||||||
const ruleIds: string[] = []; | ||||||||||||||||||||||||
if (shouldCreateCustomRule) { | ||||||||||||||||||||||||
const ruleData = await api.requestPromise( | ||||||||||||||||||||||||
`/projects/${organization.slug}/${projectData.slug}/rules/`, | ||||||||||||||||||||||||
|
@@ -136,7 +153,26 @@ function CreateProject() { | |||||||||||||||||||||||
}, | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
); | ||||||||||||||||||||||||
ruleId = ruleData.id; | ||||||||||||||||||||||||
ruleIds.push(ruleData.id); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
if ( | ||||||||||||||||||||||||
organization.features.includes( | ||||||||||||||||||||||||
'messaging-integration-onboarding-project-creation' | ||||||||||||||||||||||||
) && | ||||||||||||||||||||||||
shouldCreateRule | ||||||||||||||||||||||||
) { | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this check with the flag be part of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can just pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in which case the function just becomes a no-op |
||||||||||||||||||||||||
const ruleData = await createNotificationAction({ | ||||||||||||||||||||||||
api, | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you'll need to pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I tried to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you want |
||||||||||||||||||||||||
name, | ||||||||||||||||||||||||
organizationSlug: organization.slug, | ||||||||||||||||||||||||
projectSlug: projectData.slug, | ||||||||||||||||||||||||
conditions, | ||||||||||||||||||||||||
actionMatch, | ||||||||||||||||||||||||
frequency, | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
if (ruleData) { | ||||||||||||||||||||||||
ruleIds.push(ruleData.id); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
trackAnalytics('project_creation_page.created', { | ||||||||||||||||||||||||
organization, | ||||||||||||||||||||||||
|
@@ -147,7 +183,7 @@ function CreateProject() { | |||||||||||||||||||||||
: 'No Rule', | ||||||||||||||||||||||||
project_id: projectData.id, | ||||||||||||||||||||||||
platform: selectedPlatform.key, | ||||||||||||||||||||||||
rule_id: ruleId || '', | ||||||||||||||||||||||||
rule_ids: ruleIds, | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
ProjectsStore.onCreateSuccess(projectData, organization.slug); | ||||||||||||||||||||||||
|
@@ -192,7 +228,15 @@ function CreateProject() { | |||||||||||||||||||||||
} | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
}, | ||||||||||||||||||||||||
[api, alertRuleConfig, organization, platform, projectName, team] | ||||||||||||||||||||||||
[ | ||||||||||||||||||||||||
api, | ||||||||||||||||||||||||
alertRuleConfig, | ||||||||||||||||||||||||
organization, | ||||||||||||||||||||||||
platform, | ||||||||||||||||||||||||
projectName, | ||||||||||||||||||||||||
team, | ||||||||||||||||||||||||
createNotificationAction, | ||||||||||||||||||||||||
] | ||||||||||||||||||||||||
); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const handleProjectCreation = useCallback(async () => { | ||||||||||||||||||||||||
|
@@ -259,7 +303,7 @@ function CreateProject() { | |||||||||||||||||||||||
setProjectName(newName); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const {shouldCreateCustomRule, conditions} = alertRuleConfig || {}; | ||||||||||||||||||||||||
const {shouldCreateRule, shouldCreateCustomRule, conditions} = alertRuleConfig || {}; | ||||||||||||||||||||||||
const canUserCreateProject = canCreateProject(organization); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const canCreateTeam = organization.access.includes('project:admin'); | ||||||||||||||||||||||||
|
@@ -269,11 +313,18 @@ function CreateProject() { | |||||||||||||||||||||||
const isMissingProjectName = projectName === ''; | ||||||||||||||||||||||||
const isMissingAlertThreshold = | ||||||||||||||||||||||||
shouldCreateCustomRule && !conditions?.every?.(condition => condition.value); | ||||||||||||||||||||||||
const isMissingMessagingIntegrationChannel = | ||||||||||||||||||||||||
shouldCreateRule && | ||||||||||||||||||||||||
alertNotificationActions?.some( | ||||||||||||||||||||||||
action => action === MultipleCheckboxOptions.INTEGRATION | ||||||||||||||||||||||||
) && | ||||||||||||||||||||||||
!channel; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const formErrorCount = [ | ||||||||||||||||||||||||
isMissingTeam, | ||||||||||||||||||||||||
isMissingProjectName, | ||||||||||||||||||||||||
isMissingAlertThreshold, | ||||||||||||||||||||||||
isMissingMessagingIntegrationChannel, | ||||||||||||||||||||||||
].filter(value => value).length; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const canSubmitForm = !inFlight && canUserCreateProject && formErrorCount === 0; | ||||||||||||||||||||||||
|
@@ -285,6 +336,10 @@ function CreateProject() { | |||||||||||||||||||||||
submitTooltipText = t('Please provide a project name'); | ||||||||||||||||||||||||
} else if (isMissingAlertThreshold) { | ||||||||||||||||||||||||
submitTooltipText = t('Please provide an alert threshold'); | ||||||||||||||||||||||||
} else if (isMissingMessagingIntegrationChannel) { | ||||||||||||||||||||||||
submitTooltipText = t( | ||||||||||||||||||||||||
'Please provide an integration channel for alert notifications' | ||||||||||||||||||||||||
); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const alertFrequencyDefaultValues = useMemo(() => { | ||||||||||||||||||||||||
|
@@ -348,6 +403,16 @@ function CreateProject() { | |||||||||||||||||||||||
{...alertFrequencyDefaultValues} | ||||||||||||||||||||||||
platformLanguage={platform?.language as SupportedLanguages} | ||||||||||||||||||||||||
onChange={updatedData => setAlertRuleConfig(updatedData)} | ||||||||||||||||||||||||
notificationProps={{ | ||||||||||||||||||||||||
actions: alertNotificationActions, | ||||||||||||||||||||||||
channel, | ||||||||||||||||||||||||
integration, | ||||||||||||||||||||||||
provider, | ||||||||||||||||||||||||
setActions: setAlertNotificationActions, | ||||||||||||||||||||||||
setChannel, | ||||||||||||||||||||||||
setIntegration, | ||||||||||||||||||||||||
setProvider, | ||||||||||||||||||||||||
}} | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you make my change suggested here https://github.com/getsentry/sentry/pull/79194/files#r1805048688 could this just become
Suggested change
|
||||||||||||||||||||||||
/> | ||||||||||||||||||||||||
<StyledListItem>{t('Name your project and assign it a team')}</StyledListItem> | ||||||||||||||||||||||||
<CreateProjectForm | ||||||||||||||||||||||||
|
@@ -405,11 +470,15 @@ function CreateProject() { | |||||||||||||||||||||||
|
||||||||||||||||||||||||
{errors && ( | ||||||||||||||||||||||||
<Alert type="error"> | ||||||||||||||||||||||||
{Object.keys(errors).map(key => ( | ||||||||||||||||||||||||
<div key={key}> | ||||||||||||||||||||||||
<strong>{startCase(key)}</strong>: {errors[key]} | ||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||
))} | ||||||||||||||||||||||||
{Object.keys(errors).map(key => { | ||||||||||||||||||||||||
const label = | ||||||||||||||||||||||||
key === 'actions' ? t('Notify via integration') : startCase(key); | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's definitely improve this and create a key → error text mapping if we can |
||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||
<div key={key}> | ||||||||||||||||||||||||
<strong>{label}</strong>: {errors[key]} | ||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||
); | ||||||||||||||||||||||||
})} | ||||||||||||||||||||||||
</Alert> | ||||||||||||||||||||||||
)} | ||||||||||||||||||||||||
</List> | ||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
import {GitHubIntegrationProviderFixture} from 'sentry-fixture/githubIntegrationProvider'; | ||
import {OrganizationFixture} from 'sentry-fixture/organization'; | ||
import {OrganizationIntegrationsFixture} from 'sentry-fixture/organizationIntegrations'; | ||
|
||
import {render, screen, userEvent} from 'sentry-test/reactTestingLibrary'; | ||
|
||
import type {OrganizationIntegration} from 'sentry/types/integrations'; | ||
import IssueAlertNotificationOptions from 'sentry/views/projectInstall/issueAlertNotificationOptions'; | ||
|
||
describe('MessagingIntegrationAlertRule', function () { | ||
const organization = OrganizationFixture({ | ||
features: ['messaging-integration-onboarding-project-creation'], | ||
}); | ||
let mockResponse: jest.Mock<any>; | ||
let integrations: OrganizationIntegration[] = []; | ||
const mockSetAction = jest.fn(); | ||
|
||
const notificationProps = { | ||
actions: [], | ||
channel: 'channel', | ||
integration: undefined, | ||
provider: 'slack', | ||
setActions: mockSetAction, | ||
setChannel: jest.fn(), | ||
setIntegration: jest.fn(), | ||
setProvider: jest.fn(), | ||
}; | ||
|
||
const getComponent = () => <IssueAlertNotificationOptions {...notificationProps} />; | ||
|
||
beforeEach(function () { | ||
integrations = [ | ||
OrganizationIntegrationsFixture({ | ||
name: "Moo Deng's Workspace", | ||
status: 'disabled', | ||
}), | ||
OrganizationIntegrationsFixture({ | ||
name: "Moo Waan's Workspace", | ||
status: 'disabled', | ||
}), | ||
]; | ||
mockResponse = MockApiClient.addMockResponse({ | ||
url: `/organizations/${organization.slug}/integrations/?integrationType=messaging`, | ||
body: integrations, | ||
}); | ||
}); | ||
|
||
afterEach(function () { | ||
MockApiClient.clearMockResponses(); | ||
}); | ||
|
||
it('renders setup button if no integrations are active', async function () { | ||
const providers = (providerKey: string) => [ | ||
GitHubIntegrationProviderFixture({key: providerKey}), | ||
]; | ||
const providerKeys = ['slack', 'discord', 'msteams']; | ||
const mockResponses: jest.Mock<any>[] = []; | ||
providerKeys.forEach(providerKey => { | ||
mockResponses.push( | ||
MockApiClient.addMockResponse({ | ||
url: `/organizations/${organization.slug}/config/integrations/?provider_key=${providerKey}`, | ||
body: {providers: providers(providerKey)}, | ||
}) | ||
); | ||
}); | ||
render(getComponent(), {organization: organization}); | ||
await screen.findByText(/notify via email/i); | ||
expect(screen.queryByText(/notify via integration/i)).not.toBeInTheDocument(); | ||
await screen.findByRole('button', {name: /connect to messaging/i}); | ||
expect(mockResponse).toHaveBeenCalled(); | ||
mockResponses.forEach(mock => { | ||
expect(mock).toHaveBeenCalled(); | ||
}); | ||
}); | ||
|
||
it('renders alert configuration if integration is installed', async function () { | ||
integrations.push( | ||
OrganizationIntegrationsFixture({ | ||
name: "Moo Toon's Workspace", | ||
status: 'active', | ||
}) | ||
); | ||
render(getComponent(), {organization: organization}); | ||
await screen.findByText(/notify via email/i); | ||
await screen.findByText(/notify via integration/i); | ||
expect(mockResponse).toHaveBeenCalled(); | ||
}); | ||
|
||
it('calls setter when new integration option is selected', async function () { | ||
integrations.push( | ||
OrganizationIntegrationsFixture({ | ||
name: "Moo Toon's Workspace", | ||
status: 'active', | ||
}) | ||
); | ||
render(getComponent(), {organization: organization}); | ||
await screen.findByText(/notify via email/i); | ||
await screen.findByText(/notify via integration/i); | ||
await userEvent.click(screen.getByText(/notify via integration/i)); | ||
expect(mockSetAction).toHaveBeenCalled(); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we actually need the regex here?