From dbaf405399b77d2752a72a92c21235623c94eef7 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Wed, 20 Jun 2018 12:55:16 -0700 Subject: [PATCH 1/8] feat(api): Change `ResourceMoved` to return a consistent object --- src/sentry/api/bases/project.py | 2 +- src/sentry/api/exceptions.py | 15 +++++++++++---- .../sentry/api/endpoints/test_project_details.py | 2 +- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/sentry/api/bases/project.py b/src/sentry/api/bases/project.py index fc8f56e0565e56..da5f7b44a99ed3 100644 --- a/src/sentry/api/bases/project.py +++ b/src/sentry/api/bases/project.py @@ -121,7 +121,7 @@ def convert_args(self, request, organization_slug, project_slug, *args, **kwargs redirect_slug=project_slug ) - raise ResourceMoved(detail={'slug': redirect.project.slug}) + raise ResourceMoved(redirect.project.slug) except ProjectRedirect.DoesNotExist: raise ResourceDoesNotExist diff --git a/src/sentry/api/exceptions.py b/src/sentry/api/exceptions.py index 8c057867234312..8ed488049ab0bc 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,17 @@ def __init__(self, code=None, message=None, detail=None, **kwargs): super(SentryAPIException, self).__init__(detail=detail) +class ResourceMoved(SentryAPIException): + status_code = status.HTTP_302_FOUND + code = 'project-moved' + message = 'Project slug was renamed' + + def __init__(self, new_slug): + super(ResourceMoved, self).__init__( + slug=new_slug + ) + + class SsoRequired(SentryAPIException): status_code = status.HTTP_401_UNAUTHORIZED code = 'sso-required' diff --git a/tests/sentry/api/endpoints/test_project_details.py b/tests/sentry/api/endpoints/test_project_details.py index 4971aee1a01330..0b852b6fdc01f2 100644 --- a/tests/sentry/api/endpoints/test_project_details.py +++ b/tests/sentry/api/endpoints/test_project_details.py @@ -78,7 +78,7 @@ 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['detail']['extra']['slug'] == 'foobar' class ProjectUpdateTest(APITestCase): From 2a42b7699292d1776c5b24977ba516478302ce42 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Wed, 20 Jun 2018 16:01:00 -0700 Subject: [PATCH 2/8] feat(ui): Handle project redirect in API client Previously we would only be handling API responses with a 302 in `ProjectContext`, which is only a subset of requests that can return a 302. Move the handling up to the API client. Also changes the 302 response body to be an object more consistent with our error response objects. Fixes JAVASCRIPT-3JM --- .../static/sentry/app/__mocks__/api.jsx | 7 +- .../sentry/app/actionCreators/modal.jsx | 8 ++ src/sentry/static/sentry/app/api.jsx | 32 ++++++- .../components/modals/redirectToProject.jsx | 94 +++++++++++++++++++ .../sentry/app/constants/apiErrorCodes.jsx | 3 + .../app/views/projects/projectContext.jsx | 26 +---- tests/js/spec/api.spec.jsx | 19 ++++ .../modals/redirectToProject.spec.jsx | 43 +++++++++ tests/js/spec/views/projectTeams.spec.jsx | 1 + .../views/projects/projectContext.spec.jsx | 67 +++++++++++-- 10 files changed, 264 insertions(+), 36 deletions(-) create mode 100644 src/sentry/static/sentry/app/components/modals/redirectToProject.jsx create mode 100644 src/sentry/static/sentry/app/constants/apiErrorCodes.jsx create mode 100644 tests/js/spec/components/modals/redirectToProject.spec.jsx 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..cfa086835f1072 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,20 @@ 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); + 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 +84,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 +105,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..3f3064ec32db58 100644 --- a/src/sentry/static/sentry/app/views/projects/projectContext.jsx +++ b/src/sentry/static/sentry/app/views/projects/projectContext.jsx @@ -44,7 +44,6 @@ const ProjectContext = createReactClass({ projectId: PropTypes.string, orgId: PropTypes.string, location: PropTypes.object, - router: PropTypes.object, }, childContextTypes: { @@ -197,31 +196,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/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..6d00b07fc8f26a 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 = [ @@ -19,16 +21,13 @@ describe('projectContext component', function() { const project = TestStubs.Project(); const org = TestStubs.Organization(); - it('redirects for renamed projects', function() { + it('displays error on 404s', 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 +42,60 @@ 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/`); + 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(); }); }); From 8d45f85f075641e6b307180a2f2a66224f43a02f Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Wed, 20 Jun 2018 19:05:25 -0700 Subject: [PATCH 3/8] use a real redirect --- src/sentry/api/bases/project.py | 13 ++- src/sentry/api/exceptions.py | 11 +- .../app/views/projects/projectContext.jsx | 104 ++++++++---------- .../__snapshots__/index.spec.jsx.snap | 2 + .../views/onboarding/configure/index.spec.jsx | 2 + .../views/projectGeneralSettings.spec.jsx | 20 ++-- .../views/projects/projectContext.spec.jsx | 22 +++- 7 files changed, 97 insertions(+), 77 deletions(-) diff --git a/src/sentry/api/bases/project.py b/src/sentry/api/bases/project.py index da5f7b44a99ed3..da0a92ab71ee2b 100644 --- a/src/sentry/api/bases/project.py +++ b/src/sentry/api/bases/project.py @@ -1,5 +1,7 @@ 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 @@ -120,8 +122,8 @@ def convert_args(self, request, organization_slug, project_slug, *args, **kwargs organization__slug=organization_slug, redirect_slug=project_slug ) - - raise ResourceMoved(redirect.project.slug) + new_url = request.path.replace('projects/%s/%s/' % (organization_slug, project_slug), 'projects/%s/%s/' % (organization_slug, redirect.project.slug)) + raise ResourceMoved(new_url) except ProjectRedirect.DoesNotExist: raise ResourceDoesNotExist @@ -139,3 +141,10 @@ 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, ResourceMoved): + response = Response({}, status=exc.detail.status_code) + response['Location'] = exc.detail['extra']['url'] + return response + raise exc diff --git a/src/sentry/api/exceptions.py b/src/sentry/api/exceptions.py index 8ed488049ab0bc..8b3f21a17ebf30 100644 --- a/src/sentry/api/exceptions.py +++ b/src/sentry/api/exceptions.py @@ -25,13 +25,14 @@ def __init__(self, code=None, message=None, detail=None, **kwargs): class ResourceMoved(SentryAPIException): - status_code = status.HTTP_302_FOUND - code = 'project-moved' - message = 'Project slug was renamed' + status_code = status.HTTP_307_TEMPORARY_REDIRECT + # code/message currently don't get used + code = 'resource-moved' + message = 'Resource has been moved' - def __init__(self, new_slug): + def __init__(self, new_url): super(ResourceMoved, self).__init__( - slug=new_slug + url=new_url ) diff --git a/src/sentry/static/sentry/app/views/projects/projectContext.jsx b/src/sentry/static/sentry/app/views/projects/projectContext.jsx index 3f3064ec32db58..990bec406df053 100644 --- a/src/sentry/static/sentry/app/views/projects/projectContext.jsx +++ b/src/sentry/static/sentry/app/views/projects/projectContext.jsx @@ -138,7 +138,6 @@ const ProjectContext = createReactClass({ let {orgId, projectId, location, skipReload} = this.props; // we fetch core access/information from the global organization data let activeProject = this.identifyProject(); - let hasAccess = activeProject && activeProject.hasAccess; this.setState(state => ({ // if `skipReload` is true, then don't change loading state @@ -147,66 +146,53 @@ const ProjectContext = createReactClass({ project: activeProject, })); - if (activeProject && hasAccess) { - setActiveProject(null); - const projectRequest = this.api.requestPromise(`/projects/${orgId}/${projectId}/`); - - const environmentRequest = this.api.requestPromise( - this.getEnvironmentListEndpoint() - ); - - Promise.all([projectRequest, environmentRequest]).then( - ([project, envs]) => { - this.setState({ - loading: false, - project, - error: false, - errorType: null, - }); - - // assuming here that this means the project is considered the active project - setActiveProject(project); - - // If an environment is specified in the query string, load it instead of default - const queryEnv = location.query.environment; - // The default environment cannot be "" (No Environment) - const {defaultEnvironment} = project; - const envName = typeof queryEnv === 'undefined' ? defaultEnvironment : queryEnv; - loadEnvironments(envs, envName); - }, - () => { - this.setState({ - loading: false, - error: false, - errorType: ERROR_TYPES.UNKNOWN, - }); + setActiveProject(null); + const projectRequest = this.api.requestPromise(`/projects/${orgId}/${projectId}/`); + + const environmentRequest = this.api.requestPromise(this.getEnvironmentListEndpoint()); + + Promise.all([projectRequest, environmentRequest]).then( + ([project, envs]) => { + this.setState({ + loading: false, + project, + error: false, + errorType: null, + }); + + // assuming here that this means the project is considered the active project + setActiveProject(project); + + // If an environment is specified in the query string, load it instead of default + const queryEnv = location.query.environment; + // The default environment cannot be "" (No Environment) + const {defaultEnvironment} = project; + const envName = typeof queryEnv === 'undefined' ? defaultEnvironment : queryEnv; + loadEnvironments(envs, envName); + + // TODO(dcramer): move member list to organization level + this.api.request(this.getMemberListEndpoint(), { + success: data => { + MemberListStore.loadInitialData(data.filter(m => m.user).map(m => m.user)); + }, + }); + }, + resp => { + let errorType = ERROR_TYPES.UNKNOWN; + + if (resp.status === 404) { + errorType = ERROR_TYPES.PROJECT_NOT_FOUND; + } else if (activeProject && !activeProject.isMember) { + errorType = ERROR_TYPES.MISSING_MEMBERSHIP; } - ); - // TODO(dcramer): move member list to organization level - this.api.request(this.getMemberListEndpoint(), { - success: data => { - MemberListStore.loadInitialData(data.filter(m => m.user).map(m => m.user)); - }, - }); - } else if (activeProject && !activeProject.isMember) { - this.setState({ - loading: false, - error: true, - errorType: ERROR_TYPES.MISSING_MEMBERSHIP, - }); - } else { - // 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, - }); - }, - }); - } + this.setState({ + loading: false, + error: true, + errorType, + }); + } + ); }, getEnvironmentListEndpoint() { 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/projects/projectContext.spec.jsx b/tests/js/spec/views/projects/projectContext.spec.jsx index 6d00b07fc8f26a..7302b1d7384713 100644 --- a/tests/js/spec/views/projects/projectContext.spec.jsx +++ b/tests/js/spec/views/projects/projectContext.spec.jsx @@ -16,12 +16,27 @@ 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('displays error on 404s', function() { + it('displays error on 404s', async function() { const router = TestStubs.router(); MockApiClient.addMockResponse({ @@ -47,6 +62,9 @@ describe('projectContext component', function() { childContextTypes: {organization: SentryTypes.Organization}, }); + await tick(); + wrapper.update(); + expect(wrapper.state('error')).toBe(true); expect(wrapper.state('loading')).toBe(false); expect(wrapper.state('errorType')).toBe('PROJECT_NOT_FOUND'); From d23f7f9f9c2c2a8175a3c0c854170d481b62c984 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Thu, 21 Jun 2018 10:41:33 -0700 Subject: [PATCH 4/8] use full request path, make sure new url is different than requested url before raising a ResourceMoved --- src/sentry/api/bases/project.py | 17 +++++++++++++---- .../api/endpoints/test_project_details.py | 4 ++-- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/sentry/api/bases/project.py b/src/sentry/api/bases/project.py index da0a92ab71ee2b..681ac145f46c19 100644 --- a/src/sentry/api/bases/project.py +++ b/src/sentry/api/bases/project.py @@ -122,8 +122,17 @@ def convert_args(self, request, organization_slug, project_slug, *args, **kwargs organization__slug=organization_slug, redirect_slug=project_slug ) - new_url = request.path.replace('projects/%s/%s/' % (organization_slug, project_slug), 'projects/%s/%s/' % (organization_slug, redirect.project.slug)) - raise ResourceMoved(new_url) + + # 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 ResourceMoved(new_url) + + # otherwise project doesn't exist + raise ResourceDoesNotExist except ProjectRedirect.DoesNotExist: raise ResourceDoesNotExist @@ -144,7 +153,7 @@ def convert_args(self, request, organization_slug, project_slug, *args, **kwargs def handle_exception(self, request, exc): if isinstance(exc, ResourceMoved): - response = Response({}, status=exc.detail.status_code) + response = Response({}, status=exc.status_code) response['Location'] = exc.detail['extra']['url'] return response - raise exc + return super(ProjectEndpoint, self).handle_exception(request, exc) diff --git a/tests/sentry/api/endpoints/test_project_details.py b/tests/sentry/api/endpoints/test_project_details.py index 0b852b6fdc01f2..ae605a3cc4272f 100644 --- a/tests/sentry/api/endpoints/test_project_details.py +++ b/tests/sentry/api/endpoints/test_project_details.py @@ -77,8 +77,8 @@ def test_project_renamed_302(self): self.client.put(url, data={'slug': 'foobar'}) response = self.client.get(url) - assert response.status_code == 302 - assert response.data['detail']['extra']['slug'] == 'foobar' + assert response.status_code == 307 + assert response['Location'] == 'http://testserver/api/0/projects/%s/%s/' % (project.organization.slug, 'foobar') class ProjectUpdateTest(APITestCase): From 76afb228c93660fd22d8a0e80ea30116181458da Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Thu, 21 Jun 2018 15:02:15 -0700 Subject: [PATCH 5/8] fix response to have the same body (also attach additional `detail` object) --- src/sentry/api/bases/project.py | 7 +++++-- src/sentry/api/exceptions.py | 7 ++++--- tests/sentry/api/endpoints/test_project_details.py | 4 +++- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/sentry/api/bases/project.py b/src/sentry/api/bases/project.py index 681ac145f46c19..35c973c4919ef1 100644 --- a/src/sentry/api/bases/project.py +++ b/src/sentry/api/bases/project.py @@ -129,7 +129,7 @@ def convert_args(self, request, organization_slug, project_slug, *args, **kwargs # Resource was moved/renamed if the requested url is different than the new url if requested_url != new_url: - raise ResourceMoved(new_url) + raise ResourceMoved(new_url, redirect.project.slug) # otherwise project doesn't exist raise ResourceDoesNotExist @@ -153,7 +153,10 @@ def convert_args(self, request, organization_slug, project_slug, *args, **kwargs def handle_exception(self, request, exc): if isinstance(exc, ResourceMoved): - response = Response({}, status=exc.status_code) + 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 8b3f21a17ebf30..a6f7350c8e2f79 100644 --- a/src/sentry/api/exceptions.py +++ b/src/sentry/api/exceptions.py @@ -25,14 +25,15 @@ def __init__(self, code=None, message=None, detail=None, **kwargs): class ResourceMoved(SentryAPIException): - status_code = status.HTTP_307_TEMPORARY_REDIRECT + 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): + def __init__(self, new_url, slug): super(ResourceMoved, self).__init__( - url=new_url + url=new_url, + slug=slug, ) diff --git a/tests/sentry/api/endpoints/test_project_details.py b/tests/sentry/api/endpoints/test_project_details.py index ae605a3cc4272f..dc0d715351e451 100644 --- a/tests/sentry/api/endpoints/test_project_details.py +++ b/tests/sentry/api/endpoints/test_project_details.py @@ -77,7 +77,9 @@ def test_project_renamed_302(self): self.client.put(url, data={'slug': 'foobar'}) response = self.client.get(url) - assert response.status_code == 307 + assert response.status_code == 302 + 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') From be1ef3c66c0de1c50d8a6668cdfa9c170c0f3c52 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Thu, 26 Jul 2018 10:19:19 -0700 Subject: [PATCH 6/8] rename to ProjectMoved --- src/sentry/api/bases/project.py | 11 +++++++---- src/sentry/api/exceptions.py | 4 ++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/sentry/api/bases/project.py b/src/sentry/api/bases/project.py index 35c973c4919ef1..a224e949705d6b 100644 --- a/src/sentry/api/bases/project.py +++ b/src/sentry/api/bases/project.py @@ -4,7 +4,7 @@ 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 @@ -125,11 +125,14 @@ def convert_args(self, request, organization_slug, project_slug, *args, **kwargs # 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)) + 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 ResourceMoved(new_url, redirect.project.slug) + raise ProjectMoved(new_url, redirect.project.slug) # otherwise project doesn't exist raise ResourceDoesNotExist @@ -152,7 +155,7 @@ def convert_args(self, request, organization_slug, project_slug, *args, **kwargs return (args, kwargs) def handle_exception(self, request, exc): - if isinstance(exc, ResourceMoved): + if isinstance(exc, ProjectMoved): response = Response({ 'slug': exc.detail['extra']['slug'], 'detail': exc.detail diff --git a/src/sentry/api/exceptions.py b/src/sentry/api/exceptions.py index a6f7350c8e2f79..f07eba258594e0 100644 --- a/src/sentry/api/exceptions.py +++ b/src/sentry/api/exceptions.py @@ -24,14 +24,14 @@ def __init__(self, code=None, message=None, detail=None, **kwargs): super(SentryAPIException, self).__init__(detail=detail) -class ResourceMoved(SentryAPIException): +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(ResourceMoved, self).__init__( + super(ProjectMoved, self).__init__( url=new_url, slug=slug, ) From 1e569e1eaa02fce1194cf292b11123afd7a36660 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Thu, 26 Jul 2018 10:19:45 -0700 Subject: [PATCH 7/8] remove unused var --- .../app/views/projects/projectContext.jsx | 105 ++++++++++-------- 1 file changed, 59 insertions(+), 46 deletions(-) diff --git a/src/sentry/static/sentry/app/views/projects/projectContext.jsx b/src/sentry/static/sentry/app/views/projects/projectContext.jsx index 990bec406df053..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'; @@ -138,6 +137,7 @@ const ProjectContext = createReactClass({ let {orgId, projectId, location, skipReload} = this.props; // we fetch core access/information from the global organization data let activeProject = this.identifyProject(); + let hasAccess = activeProject && activeProject.hasAccess; this.setState(state => ({ // if `skipReload` is true, then don't change loading state @@ -146,53 +146,66 @@ const ProjectContext = createReactClass({ project: activeProject, })); - setActiveProject(null); - const projectRequest = this.api.requestPromise(`/projects/${orgId}/${projectId}/`); - - const environmentRequest = this.api.requestPromise(this.getEnvironmentListEndpoint()); - - Promise.all([projectRequest, environmentRequest]).then( - ([project, envs]) => { - this.setState({ - loading: false, - project, - error: false, - errorType: null, - }); - - // assuming here that this means the project is considered the active project - setActiveProject(project); - - // If an environment is specified in the query string, load it instead of default - const queryEnv = location.query.environment; - // The default environment cannot be "" (No Environment) - const {defaultEnvironment} = project; - const envName = typeof queryEnv === 'undefined' ? defaultEnvironment : queryEnv; - loadEnvironments(envs, envName); - - // TODO(dcramer): move member list to organization level - this.api.request(this.getMemberListEndpoint(), { - success: data => { - MemberListStore.loadInitialData(data.filter(m => m.user).map(m => m.user)); - }, - }); - }, - resp => { - let errorType = ERROR_TYPES.UNKNOWN; - - if (resp.status === 404) { - errorType = ERROR_TYPES.PROJECT_NOT_FOUND; - } else if (activeProject && !activeProject.isMember) { - errorType = ERROR_TYPES.MISSING_MEMBERSHIP; + if (activeProject && hasAccess) { + setActiveProject(null); + const projectRequest = this.api.requestPromise(`/projects/${orgId}/${projectId}/`); + + const environmentRequest = this.api.requestPromise( + this.getEnvironmentListEndpoint() + ); + + Promise.all([projectRequest, environmentRequest]).then( + ([project, envs]) => { + this.setState({ + loading: false, + project, + error: false, + errorType: null, + }); + + // assuming here that this means the project is considered the active project + setActiveProject(project); + + // If an environment is specified in the query string, load it instead of default + const queryEnv = location.query.environment; + // The default environment cannot be "" (No Environment) + const {defaultEnvironment} = project; + const envName = typeof queryEnv === 'undefined' ? defaultEnvironment : queryEnv; + loadEnvironments(envs, envName); + }, + () => { + this.setState({ + loading: false, + error: false, + errorType: ERROR_TYPES.UNKNOWN, + }); } + ); - this.setState({ - loading: false, - error: true, - errorType, - }); - } - ); + // TODO(dcramer): move member list to organization level + this.api.request(this.getMemberListEndpoint(), { + success: data => { + MemberListStore.loadInitialData(data.filter(m => m.user).map(m => m.user)); + }, + }); + } else if (activeProject && !activeProject.isMember) { + this.setState({ + loading: false, + error: true, + errorType: ERROR_TYPES.MISSING_MEMBERSHIP, + }); + } else { + // 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, + }); + }, + }); + } }, getEnvironmentListEndpoint() { From 6809c66f27fc6dace9809edfaf07ca6e0b24b9b0 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Thu, 26 Jul 2018 10:50:12 -0700 Subject: [PATCH 8/8] add comment --- src/sentry/static/sentry/app/api.jsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/sentry/static/sentry/app/api.jsx b/src/sentry/static/sentry/app/api.jsx index cfa086835f1072..1f3c717383f9af 100644 --- a/src/sentry/static/sentry/app/api.jsx +++ b/src/sentry/static/sentry/app/api.jsx @@ -64,6 +64,9 @@ export class Client { */ 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);