Skip to content
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

fix(app-platform): Allow removal of Internal Apps #13534

Merged
merged 1 commit into from
Jun 5, 2019
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
3 changes: 1 addition & 2 deletions src/sentry/api/endpoints/sentry_app_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
from sentry.api.bases.sentryapps import SentryAppBaseEndpoint
from sentry.api.serializers import serialize
from sentry.api.serializers.rest_framework import SentryAppSerializer
from sentry.constants import SentryAppStatus
from sentry.mediators.sentry_apps import Updater, Destroyer


Expand Down Expand Up @@ -68,7 +67,7 @@ def delete(self, request, sentry_app):
actor=request.user):
return Response(status=404)

if sentry_app.status == SentryAppStatus.UNPUBLISHED:
if sentry_app.is_unpublished or sentry_app.is_internal:
Destroyer.run(
user=request.user,
sentry_app=sentry_app,
Expand Down
12 changes: 7 additions & 5 deletions src/sentry/mediators/sentry_app_installations/destroyer.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class Destroyer(Mediator):
install = Param('sentry.models.SentryAppInstallation')
user = Param('sentry.models.User')
request = Param('rest_framework.request.Request', required=False)
notify = Param(bool, default=True)

def call(self):
self._destroy_grant()
Expand All @@ -32,11 +33,12 @@ def _destroy_service_hooks(self):
service_hooks.Destroyer.run(service_hook=hook)

def _destroy_installation(self):
InstallationNotifier.run(
install=self.install,
user=self.user,
action='deleted',
)
if self.notify:
InstallationNotifier.run(
install=self.install,
user=self.user,
action='deleted',
)
self.install.delete()

def audit(self):
Expand Down
5 changes: 4 additions & 1 deletion src/sentry/mediators/sentry_apps/destroyer.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,22 @@
class Destroyer(Mediator):
sentry_app = Param('sentry.models.SentryApp')
request = Param('rest_framework.request.Request', required=False)
user = Param('sentry.models.User')

def call(self):
self._destroy_sentry_app_installations()
self._destroy_api_application()
self._destroy_sentry_app_installations()
self._destroy_proxy_user()
self._destroy_sentry_app()
return self.sentry_app

def _destroy_sentry_app_installations(self):
for install in self.sentry_app.installations.all():
notify = False if self.sentry_app.is_internal else True
sentry_app_installations.Destroyer.run(
install=install,
user=self.sentry_app.proxy_user,
notify=notify,
)

def _destroy_api_application(self):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import LoadingIndicator from 'app/components/loadingIndicator';
import MigrationWarnings from 'app/views/organizationIntegrations/migrationWarnings';
import PermissionAlert from 'app/views/settings/organization/permissionAlert';
import ProviderRow from 'app/views/organizationIntegrations/providerRow';
import {removeSentryApp} from 'app/actionCreators/sentryApps';
import SentryAppInstallations from 'app/views/organizationIntegrations/sentryAppInstallations';
import SentryApplicationRow from 'app/views/settings/organizationDeveloperSettings/sentryApplicationRow';
import SentryTypes from 'app/sentryTypes';
Expand Down Expand Up @@ -168,19 +169,31 @@ class OrganizationIntegrations extends AsyncComponent {

renderInternalSentryApps(app, key) {
const {organization} = this.props;

return (
<SentryApplicationRow
key={`sentry-app-row-${key}`}
data-test-id="internal-integration-row"
api={this.api}
showPublishStatus
isInternal
onRemoveApp={() => this.onRemoveInternalApp(app)}
organization={organization}
app={app}
/>
);
}

onRemoveInternalApp = app => {
const apps = this.state.orgOwnedApps.filter(a => a.slug !== app.slug);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary for this, but it might be nice to have a OwnedIntegrations store so we can just do something like OwnedIntegrationsStore.get(app.slug).

removeSentryApp(this.api, app).then(
() => {
this.setState({orgOwnedApps: apps});
},
() => {}
);
};

renderBody() {
const {reloading, orgOwnedApps, publishedApps, appInstalls} = this.state;
const published = publishedApps || [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,32 @@ describe('OrganizationIntegrations', () => {
wrapper.find('Panel [data-test-id="internal-integration-row"]').exists()
).toBe(true);
});

it('removes an internal app', async function() {
const internalApp = {...sentryApp, status: 'internal'};
Client.addMockResponse({
url: `/organizations/${org.slug}/sentry-apps/`,
body: [internalApp],
});
Client.addMockResponse({
url: '/sentry-apps/',
body: [],
});
Client.addMockResponse({
url: `/sentry-apps/${internalApp.slug}/`,
method: 'DELETE',
statusCode: 200,
});

wrapper = mount(
<OrganizationIntegrations organization={org} params={params} />,
routerContext
);
wrapper.instance().onRemoveInternalApp(internalApp);
await tick();
wrapper.update();
expect(wrapper.instance().state.orgOwnedApps).toHaveLength(0);
});
});

describe('with installed integrations', () => {
Expand Down
27 changes: 27 additions & 0 deletions tests/sentry/mediators/sentry_app_installations/test_destroyer.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,33 @@ def test_deletes_service_hooks(self):

assert not ServiceHook.objects.filter(pk=hook.id).exists()

@responses.activate
@patch('sentry.mediators.sentry_app_installations.InstallationNotifier.run')
def test_sends_notification(self, run):
with self.tasks():
responses.add(responses.POST, 'https://example.com/webhook')
request = self.make_request(user=self.user, method='GET')
Destroyer.run(
install=self.install,
user=self.user,
request=request,
)
run.assert_called_once_with(install=self.install, user=self.user, action='deleted')

@responses.activate
@patch('sentry.mediators.sentry_app_installations.InstallationNotifier.run')
def test_notify_false_does_not_send_notification(self, run):
with self.tasks():
responses.add(responses.POST, 'https://example.com/webhook')
request = self.make_request(user=self.user, method='GET')
Destroyer.run(
install=self.install,
user=self.user,
request=request,
notify=False,
)
assert not run.called

@responses.activate
def test_creates_audit_log_entry(self):
responses.add(responses.POST, 'https://example.com/webhook')
Expand Down
13 changes: 12 additions & 1 deletion tests/sentry/mediators/sentry_apps/test_destroyer.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def setUp(self):
scopes=('project:read',),
)

self.destroyer = Destroyer(sentry_app=self.sentry_app)
self.destroyer = Destroyer(sentry_app=self.sentry_app, user=self.user)

def test_deletes_app_installations(self):
install = self.create_sentry_app_installation(
Expand All @@ -30,6 +30,17 @@ def test_deletes_app_installations(self):
self.destroyer.call()
assert not SentryAppInstallation.objects.filter(pk=install.id).exists()

@patch('sentry.mediators.sentry_app_installations.Destroyer.run')
def test_passes_notify_false_if_app_internal(self, run):
self.create_project(organization=self.org)
internal = self.create_internal_integration(organization=self.org)
Destroyer.run(sentry_app=internal, user=self.user)
run.assert_called_with(
install=internal.installations.first(),
user=internal.proxy_user,
notify=False,
)

def test_deletes_api_application(self):
application = self.sentry_app.application

Expand Down