diff --git a/src/sentry/api/bases/project.py b/src/sentry/api/bases/project.py index fc8f56e0565e56..a224e949705d6b 100644 --- a/src/sentry/api/bases/project.py +++ b/src/sentry/api/bases/project.py @@ -1,8 +1,10 @@ from __future__ import absolute_import +from rest_framework.response import Response + from sentry import roles from sentry.api.base import Endpoint -from sentry.api.exceptions import ResourceDoesNotExist, ResourceMoved +from sentry.api.exceptions import ResourceDoesNotExist, ProjectMoved from sentry.app import raven from sentry.auth.superuser import is_active_superuser from sentry.models import OrganizationMember, Project, ProjectStatus, ProjectRedirect @@ -121,7 +123,19 @@ def convert_args(self, request, organization_slug, project_slug, *args, **kwargs redirect_slug=project_slug ) - raise ResourceMoved(detail={'slug': redirect.project.slug}) + # get full path so that we keep query strings + requested_url = request.get_full_path() + new_url = requested_url.replace( + 'projects/%s/%s/' % + (organization_slug, project_slug), 'projects/%s/%s/' % + (organization_slug, redirect.project.slug)) + + # Resource was moved/renamed if the requested url is different than the new url + if requested_url != new_url: + raise ProjectMoved(new_url, redirect.project.slug) + + # otherwise project doesn't exist + raise ResourceDoesNotExist except ProjectRedirect.DoesNotExist: raise ResourceDoesNotExist @@ -139,3 +153,13 @@ def convert_args(self, request, organization_slug, project_slug, *args, **kwargs kwargs['project'] = project return (args, kwargs) + + def handle_exception(self, request, exc): + if isinstance(exc, ProjectMoved): + response = Response({ + 'slug': exc.detail['extra']['slug'], + 'detail': exc.detail + }, status=exc.status_code) + response['Location'] = exc.detail['extra']['url'] + return response + return super(ProjectEndpoint, self).handle_exception(request, exc) diff --git a/src/sentry/api/exceptions.py b/src/sentry/api/exceptions.py index 8c057867234312..f07eba258594e0 100644 --- a/src/sentry/api/exceptions.py +++ b/src/sentry/api/exceptions.py @@ -9,10 +9,6 @@ class ResourceDoesNotExist(APIException): status_code = status.HTTP_404_NOT_FOUND -class ResourceMoved(APIException): - status_code = status.HTTP_302_FOUND - - class SentryAPIException(APIException): code = '' message = '' @@ -28,6 +24,19 @@ def __init__(self, code=None, message=None, detail=None, **kwargs): super(SentryAPIException, self).__init__(detail=detail) +class ProjectMoved(SentryAPIException): + status_code = status.HTTP_302_FOUND + # code/message currently don't get used + code = 'resource-moved' + message = 'Resource has been moved' + + def __init__(self, new_url, slug): + super(ProjectMoved, self).__init__( + url=new_url, + slug=slug, + ) + + class SsoRequired(SentryAPIException): status_code = status.HTTP_401_UNAUTHORIZED code = 'sso-required' diff --git a/src/sentry/static/sentry/app/__mocks__/api.jsx b/src/sentry/static/sentry/app/__mocks__/api.jsx index a67883b4853223..db731cc5272404 100644 --- a/src/sentry/static/sentry/app/__mocks__/api.jsx +++ b/src/sentry/static/sentry/app/__mocks__/api.jsx @@ -62,7 +62,10 @@ class Client { } wrapCallback(id, error) { - return (...args) => respond(Client.mockAsync, error, ...args); + return (...args) => { + if (this.hasProjectBeenRenamed(...args)) return; + respond(Client.mockAsync, error, ...args); + }; } requestPromise(url, options) { @@ -125,5 +128,7 @@ Client.prototype.uniqueId = RealClient.Client.prototype.uniqueId; Client.prototype.bulkUpdate = RealClient.Client.prototype.bulkUpdate; Client.prototype._chain = RealClient.Client.prototype._chain; Client.prototype._wrapRequest = RealClient.Client.prototype._wrapRequest; +Client.prototype.hasProjectBeenRenamed = + RealClient.Client.prototype.hasProjectBeenRenamed; export {Client}; diff --git a/src/sentry/static/sentry/app/actionCreators/modal.jsx b/src/sentry/static/sentry/app/actionCreators/modal.jsx index 83ea740e7cbe63..a7ff15720a2ebb 100644 --- a/src/sentry/static/sentry/app/actionCreators/modal.jsx +++ b/src/sentry/static/sentry/app/actionCreators/modal.jsx @@ -100,3 +100,11 @@ export function openIntegrationDetails(options = {}) { }); }); } + +export function redirectToProject(newProjectSlug) { + import(/* webpackChunkName: "RedirectToProjectModal" */ 'app/components/modals/redirectToProject') + .then(mod => mod.default) + .then(Modal => { + openModal(deps => , {}); + }); +} diff --git a/src/sentry/static/sentry/app/api.jsx b/src/sentry/static/sentry/app/api.jsx index d36dff27889b44..1f3c717383f9af 100644 --- a/src/sentry/static/sentry/app/api.jsx +++ b/src/sentry/static/sentry/app/api.jsx @@ -2,7 +2,12 @@ import $ from 'jquery'; import {isUndefined, isNil} from 'lodash'; import idx from 'idx'; -import {openSudo} from 'app/actionCreators/modal'; +import { + PROJECT_MOVED, + SUDO_REQUIRED, + SUPERUSER_REQUIRED, +} from 'app/constants/apiErrorCodes'; +import {openSudo, redirectToProject} from 'app/actionCreators/modal'; import GroupActions from 'app/actions/groupActions'; export class Request { @@ -53,6 +58,23 @@ export class Client { return s4() + s4() + '-' + s4() + '-' + s4() + '-' + s4() + '-' + s4() + s4() + s4(); } + /** + * Check if the API response says project has been renamed. + * If so, redirect user to new project slug + */ + hasProjectBeenRenamed(response) { + let code = response && idx(response, _ => _.responseJSON.detail.code); + + // XXX(billy): This actually will never happen because we can't intercept the 302 + // jQuery ajax will follow the redirect by default... + if (code !== PROJECT_MOVED) return false; + + let slug = response && idx(response, _ => _.responseJSON.detail.extra.slug); + + redirectToProject(slug); + return true; + } + wrapCallback(id, func, cleanup) { /*eslint consistent-return:0*/ if (isUndefined(func)) { @@ -65,6 +87,11 @@ export class Client { delete this.activeRequests[id]; } if (req && req.alive) { + // Check if API response is a 302 -- means project slug was renamed and user + // needs to be redirected + if (this.hasProjectBeenRenamed(...args)) return; + + // Call success callback return func.apply(req, args); } }; @@ -81,12 +108,12 @@ export class Client { handleRequestError({id, path, requestOptions}, response, ...responseArgs) { let code = response && idx(response, _ => _.responseJSON.detail.code); - let isSudoRequired = code === 'sudo-required' || code === 'superuser-required'; + let isSudoRequired = code === SUDO_REQUIRED || code === SUPERUSER_REQUIRED; if (isSudoRequired) { openSudo({ - superuser: code === 'superuser-required', - sudo: code === 'sudo-required', + superuser: code === SUPERUSER_REQUIRED, + sudo: code === SUDO_REQUIRED, retryRequest: () => { return this.requestPromise(path, requestOptions) .then((...args) => { diff --git a/src/sentry/static/sentry/app/components/modals/redirectToProject.jsx b/src/sentry/static/sentry/app/components/modals/redirectToProject.jsx new file mode 100644 index 00000000000000..dea89a285d3051 --- /dev/null +++ b/src/sentry/static/sentry/app/components/modals/redirectToProject.jsx @@ -0,0 +1,94 @@ +import {Flex} from 'grid-emotion'; +import {withRouter} from 'react-router'; +import PropTypes from 'prop-types'; +import React from 'react'; +import styled from 'react-emotion'; + +import {t, tct} from 'app/locale'; +import Button from 'app/components/buttons/button'; +import Text from 'app/components/text'; +import recreateRoute from 'app/utils/recreateRoute'; + +class RedirectToProjectModal extends React.Component { + static propTypes = { + /** + * New slug to redirect to + */ + slug: PropTypes.string.isRequired, + + Header: PropTypes.oneOfType([PropTypes.func, PropTypes.node]).isRequired, + Body: PropTypes.oneOfType([PropTypes.func, PropTypes.node]).isRequired, + }; + + constructor(props) { + super(props); + + this.state = { + timer: 5, + }; + } + + componentDidMount() { + setInterval(() => { + if (this.state.timer <= 1) { + window.location.assign(this.getNewPath()); + return; + } + + this.setState(state => ({ + timer: state.timer - 1, + })); + }, 1000); + } + + getNewPath() { + let {params, slug} = this.props; + + return recreateRoute('', { + ...this.props, + params: { + ...params, + projectId: slug, + }, + }); + } + + render() { + let {slug, Header, Body} = this.props; + return ( + +
{t('Redirecting to New Project...')}
+ + +
+ +

{t('The project slug has been changed.')}

+ +

+ {tct( + 'You will be redirected to the new project [project] in [timer] seconds...', + { + project: {slug}, + timer: `${this.state.timer}`, + } + )} +

+ + + + +
+ + + ); + } +} + +export default withRouter(RedirectToProjectModal); +export {RedirectToProjectModal}; + +const ButtonWrapper = styled(Flex)` + justify-content: flex-end; +`; diff --git a/src/sentry/static/sentry/app/constants/apiErrorCodes.jsx b/src/sentry/static/sentry/app/constants/apiErrorCodes.jsx new file mode 100644 index 00000000000000..13077878b7305f --- /dev/null +++ b/src/sentry/static/sentry/app/constants/apiErrorCodes.jsx @@ -0,0 +1,3 @@ +export const SUDO_REQUIRED = 'sudo-required'; +export const SUPERUSER_REQUIRED = 'superuser-required'; +export const PROJECT_MOVED = 'project-moved'; diff --git a/src/sentry/static/sentry/app/views/projects/projectContext.jsx b/src/sentry/static/sentry/app/views/projects/projectContext.jsx index a1c4a3010f3a11..792e0b4d52c0be 100644 --- a/src/sentry/static/sentry/app/views/projects/projectContext.jsx +++ b/src/sentry/static/sentry/app/views/projects/projectContext.jsx @@ -15,7 +15,6 @@ import MemberListStore from 'app/stores/memberListStore'; import MissingProjectMembership from 'app/components/missingProjectMembership'; import OrganizationState from 'app/mixins/organizationState'; import ProjectsStore from 'app/stores/projectsStore'; -import recreateRoute from 'app/utils/recreateRoute'; import SentryTypes from 'app/sentryTypes'; import withProjects from 'app/utils/withProjects'; @@ -44,7 +43,6 @@ const ProjectContext = createReactClass({ projectId: PropTypes.string, orgId: PropTypes.string, location: PropTypes.object, - router: PropTypes.object, }, childContextTypes: { @@ -197,31 +195,16 @@ const ProjectContext = createReactClass({ errorType: ERROR_TYPES.MISSING_MEMBERSHIP, }); } else { - // The project may have been renamed, attempt to lookup the project, if - // we 302 we will receive the moved project slug and can update update - // our route accordingly. - const lookupHandler = resp => { - const {status, responseJSON} = resp; - - if (status !== 302 || !responseJSON || !responseJSON.detail) { + // The request is a 404 or other error + this.api.request(`/projects/${orgId}/${projectId}/`, { + error: () => { this.setState({ loading: false, error: true, errorType: ERROR_TYPES.PROJECT_NOT_FOUND, }); - return; - } - - this.props.router.replace( - recreateRoute('', { - ...this.props, - params: {...this.props.params, projectId: responseJSON.detail.slug}, - }) - ); - }; - - // The request ill 404 or 302 - this.api.request(`/projects/${orgId}/${projectId}/`, {error: lookupHandler}); + }, + }); } }, diff --git a/tests/js/spec/api.spec.jsx b/tests/js/spec/api.spec.jsx index d6207abc6b38bb..8da318c5eed265 100644 --- a/tests/js/spec/api.spec.jsx +++ b/tests/js/spec/api.spec.jsx @@ -1,6 +1,8 @@ import $ from 'jquery'; + import {Client, Request, paramsToQueryArgs} from 'app/api'; import GroupActions from 'app/actions/groupActions'; +import {PROJECT_MOVED} from 'app/constants/apiErrorCodes'; jest.unmock('app/api'); @@ -92,6 +94,23 @@ describe('api', function() { }); }); + it('does not call success callback if 302 was returned because of a project slug change', function() { + let successCb = jest.fn(); + api.activeRequests = {id: {alive: true}}; + api.wrapCallback('id', successCb)({ + responseJSON: { + detail: { + code: PROJECT_MOVED, + message: '...', + extra: { + slug: 'new-slug', + }, + }, + }, + }); + expect(successCb).not.toHaveBeenCalled(); + }); + it('handles error callback', function() { sandbox.stub(api, 'wrapCallback', (id, func) => func); let errorCb = jest.fn(); diff --git a/tests/js/spec/components/modals/redirectToProject.spec.jsx b/tests/js/spec/components/modals/redirectToProject.spec.jsx new file mode 100644 index 00000000000000..1d495b17702360 --- /dev/null +++ b/tests/js/spec/components/modals/redirectToProject.spec.jsx @@ -0,0 +1,43 @@ +import {Modal} from 'react-bootstrap'; +import React from 'react'; + +import {RedirectToProjectModal} from 'app/components/modals/redirectToProject'; +import {mount} from 'enzyme'; + +jest.unmock('app/utils/recreateRoute'); +describe('RedirectToProjectModal', function() { + jest.useFakeTimers(); + const routes = [ + {path: '/', childRoutes: []}, + {name: 'Organizations', path: ':orgId/', childRoutes: []}, + {name: 'Projects', path: ':projectId/', childRoutes: []}, + ]; + + beforeEach(function() { + sinon.stub(window.location, 'assign'); + }); + + afterEach(function() { + window.location.assign.restore(); + }); + + it('has timer to redirect to new slug after mounting', function() { + mount( + , + TestStubs.routerContext() + ); + + jest.advanceTimersByTime(4900); + expect(window.location.assign.calledOnce).toBe(false); + + jest.advanceTimersByTime(200); + expect(window.location.assign.calledOnce).toBe(true); + expect(window.location.assign.calledWith('/org-slug/new-slug/')).toBe(true); + }); +}); diff --git a/tests/js/spec/views/onboarding/configure/__snapshots__/index.spec.jsx.snap b/tests/js/spec/views/onboarding/configure/__snapshots__/index.spec.jsx.snap index 7edfb7f724d8d7..8b08f673c08243 100644 --- a/tests/js/spec/views/onboarding/configure/__snapshots__/index.spec.jsx.snap +++ b/tests/js/spec/views/onboarding/configure/__snapshots__/index.spec.jsx.snap @@ -91,6 +91,7 @@ exports[`Configure should render correctly render() should render platform docs "hasAccess": true, "id": "testProject", "isBookmarked": false, + "isMember": true, "name": "Test Project", "slug": "project-slug", "teams": Array [ @@ -123,6 +124,7 @@ exports[`Configure should render correctly render() should render platform docs "hasAccess": true, "id": "testProject", "isBookmarked": false, + "isMember": true, "name": "Test Project", "slug": "project-slug", "teams": Array [ diff --git a/tests/js/spec/views/onboarding/configure/index.spec.jsx b/tests/js/spec/views/onboarding/configure/index.spec.jsx index 8ae16d1e90be75..25e1852960029e 100644 --- a/tests/js/spec/views/onboarding/configure/index.spec.jsx +++ b/tests/js/spec/views/onboarding/configure/index.spec.jsx @@ -57,6 +57,7 @@ describe('Configure should render correctly', function() { slug: 'project-slug', id: 'testProject', hasAccess: true, + isMember: true, isBookmarked: false, teams: [ { @@ -157,6 +158,7 @@ describe('Configure should render correctly', function() { id: 'testProject', hasAccess: true, isBookmarked: false, + isMember: true, teams: [ { id: 'coolteam', diff --git a/tests/js/spec/views/projectGeneralSettings.spec.jsx b/tests/js/spec/views/projectGeneralSettings.spec.jsx index a4a29e6e035dba..259b9667add274 100644 --- a/tests/js/spec/views/projectGeneralSettings.spec.jsx +++ b/tests/js/spec/views/projectGeneralSettings.spec.jsx @@ -219,15 +219,6 @@ describe('projectGeneralSettings', function() { expect(putMock).not.toHaveBeenCalled(); wrapper.find('SaveButton').simulate('click'); - await tick(); - // :( - await tick(); - wrapper.update(); - // updates ProjectsStore - expect(ProjectsStore.itemsById['2'].slug).toBe('new-project'); - expect(browserHistory.replace).toHaveBeenCalled(); - expect(wrapper.find('Input[name="slug"]').prop('value')).toBe('new-project'); - // fetches new slug let newProjectGet = MockApiClient.addMockResponse({ url: `/projects/${org.slug}/new-project/`, @@ -245,9 +236,20 @@ describe('projectGeneralSettings', function() { body: [], }); + await tick(); + // :( + await tick(); + wrapper.update(); + // updates ProjectsStore + expect(ProjectsStore.itemsById['2'].slug).toBe('new-project'); + expect(browserHistory.replace).toHaveBeenCalled(); + expect(wrapper.find('Input[name="slug"]').prop('value')).toBe('new-project'); + wrapper.setProps({ projectId: 'new-project', }); + await tick(); + wrapper.update(); expect(newProjectGet).toHaveBeenCalled(); expect(newProjectEnv).toHaveBeenCalled(); expect(newProjectMembers).toHaveBeenCalled(); diff --git a/tests/js/spec/views/projectTeams.spec.jsx b/tests/js/spec/views/projectTeams.spec.jsx index 2f8a5ee827a65d..a63bb50ac964f6 100644 --- a/tests/js/spec/views/projectTeams.spec.jsx +++ b/tests/js/spec/views/projectTeams.spec.jsx @@ -5,6 +5,7 @@ import {Client} from 'app/api'; import ProjectTeams from 'app/views/settings/project/projectTeams'; import {openCreateTeamModal} from 'app/actionCreators/modal'; +jest.unmock('app/actionCreators/modal'); jest.mock('app/actionCreators/modal', () => ({ openCreateTeamModal: jest.fn(), })); diff --git a/tests/js/spec/views/projects/projectContext.spec.jsx b/tests/js/spec/views/projects/projectContext.spec.jsx index 05bb24f12a3ea4..7302b1d7384713 100644 --- a/tests/js/spec/views/projects/projectContext.spec.jsx +++ b/tests/js/spec/views/projects/projectContext.spec.jsx @@ -1,11 +1,13 @@ import React from 'react'; import {mount} from 'enzyme'; -import {Client} from 'app/api'; import {ProjectContext} from 'app/views/projects/projectContext'; import SentryTypes from 'app/sentryTypes'; jest.unmock('app/utils/recreateRoute'); +jest.mock('app/actionCreators/modal', () => ({ + redirectToProject: jest.fn(), +})); describe('projectContext component', function() { const routes = [ @@ -14,21 +16,33 @@ describe('projectContext component', function() { {name: 'Projects', path: ':projectId/', childRoutes: []}, ]; - const location = {}; + const location = {query: {}}; const project = TestStubs.Project(); const org = TestStubs.Organization(); + beforeEach(function() { + MockApiClient.clearMockResponses(); + [project.slug, 'new-slug'].forEach(slug => { + MockApiClient.addMockResponse({ + url: `/projects/${org.slug}/${slug}/members/`, + method: 'GET', + body: [], + }); + MockApiClient.addMockResponse({ + url: `/projects/${org.slug}/${slug}/environments/`, + method: 'GET', + body: [], + }); + }); + }); - it('redirects for renamed projects', function() { + it('displays error on 404s', async function() { const router = TestStubs.router(); - Client.addMockResponse({ + MockApiClient.addMockResponse({ url: `/projects/${org.slug}/${project.slug}/`, method: 'GET', - statusCode: 302, - body: { - detail: {slug: 'renamed-slug'}, - }, + statusCode: 404, }); const projectContext = ( @@ -43,12 +57,63 @@ describe('projectContext component', function() { /> ); - mount(projectContext, { + const wrapper = mount(projectContext, { context: {organization: org}, childContextTypes: {organization: SentryTypes.Organization}, }); - expect(router.replace).toHaveBeenCalledTimes(1); - expect(router.replace).toHaveBeenCalledWith(`/${org.slug}/renamed-slug/`); + await tick(); + wrapper.update(); + + expect(wrapper.state('error')).toBe(true); + expect(wrapper.state('loading')).toBe(false); + expect(wrapper.state('errorType')).toBe('PROJECT_NOT_FOUND'); + }); + + it('fetches data again if projectId changes', function() { + const router = TestStubs.router(); + let fetchMock = MockApiClient.addMockResponse({ + url: `/projects/${org.slug}/${project.slug}/`, + method: 'GET', + statusCode: 200, + body: project, + }); + + const projectContext = ( + + ); + + const wrapper = mount(projectContext, { + context: {organization: org}, + childContextTypes: {organization: SentryTypes.Organization}, + }); + + expect(fetchMock).toHaveBeenCalledTimes(1); + + // Nothing should happen if we update and projectId is the same + wrapper.update(); + expect(fetchMock).toHaveBeenCalledTimes(1); + + fetchMock = MockApiClient.addMockResponse({ + url: `/projects/${org.slug}/new-slug/`, + method: 'GET', + statusCode: 200, + body: TestStubs.Project({slug: 'new-slug'}), + }); + + wrapper.setProps({ + projectId: 'new-slug', + }); + wrapper.update(); + + expect(fetchMock).toHaveBeenCalled(); }); }); diff --git a/tests/sentry/api/endpoints/test_project_details.py b/tests/sentry/api/endpoints/test_project_details.py index 4971aee1a01330..dc0d715351e451 100644 --- a/tests/sentry/api/endpoints/test_project_details.py +++ b/tests/sentry/api/endpoints/test_project_details.py @@ -78,7 +78,9 @@ def test_project_renamed_302(self): response = self.client.get(url) assert response.status_code == 302 - assert response.data['detail']['slug'] == 'foobar' + assert response.data['slug'] == 'foobar' + assert response.data['detail']['extra']['url'] == '/api/0/projects/%s/%s/' % (project.organization.slug, 'foobar') + assert response['Location'] == 'http://testserver/api/0/projects/%s/%s/' % (project.organization.slug, 'foobar') class ProjectUpdateTest(APITestCase):