feat(ui): Add project id to URL when visiting Issue Details di… (#13875)

There is still a problem when visiting an issue details page with a direct URL and when a user does not have access to multiple projects. When this happens the global selection header tries to select a seemingly random project.

This PR makes a few changes -- if there is no existing `project` URL param and you visit an issue details page, it will add the issues project id to the URL. This way when you go back to the issues stream it will have the proper project in context (instead of selecting a random project).

This also fixes the project selector on issue details: there is an async loading state that we are not handling well. The project selector flashes from "All Projects" (and unlocked) to a locked state with project slug. This will make the project selector aware of its "loading" state and show an empty string until we have the project slug.

src/sentry/static/sentry/app/components/organizations/globalSelectionHeader/index.jsx

@@ -33,6 +33,10 @@ import withProjects from 'app/utils/withProjects';
 
 import {getStateFromQuery} from './utils';
 
+function getProjectIdFromProject(project) {
+  return parseInt(project.id, 10);
+}
+
 class GlobalSelectionHeader extends React.Component {
   static propTypes = {
     organization: SentryTypes.Organization,
@@ -42,6 +46,15 @@ class GlobalSelectionHeader extends React.Component {
      * List of projects to display in project selector
      */
     projects: PropTypes.arrayOf(SentryTypes.Project).isRequired,
+
+    /**
+     * A project will be forced from parent component (selection is disabled, and if user
+     * does not have multi-project support enabled, it will not try to auto select a project).
+     *
+     * Project will be specified in the prop `forceProject` (since its data is async)
+     */
+    shouldForceProject: PropTypes.bool,
+
     /**
      * If a forced project is passed, selection is disabled
      */
@@ -52,20 +65,40 @@ class GlobalSelectionHeader extends React.Component {
      */
     selection: SentryTypes.GlobalSelection,
 
-    // Display Environment selector?
+    /**
+     * Display Environment selector?
+     */
     showEnvironmentSelector: PropTypes.bool,
 
-    // Display Environment selector?
+    /**
+     * Display Environment selector?
+     */
     showDateSelector: PropTypes.bool,
 
-    // Disable automatic routing
+    /**
+     * Disable automatic routing
+     */
     hasCustomRouting: PropTypes.bool,
 
-    // Reset these URL params when we fire actions
-    // (custom routing only)
+    /**
+     * Reset these URL params when we fire actions
+     * (custom routing only)
+     */
     resetParamsOnChange: PropTypes.arrayOf(PropTypes.string),
 
-    // Props passed to child components //
+    /**
+     * GlobalSelectionStore is not always initialized (e.g. Group Details) before this is rendered
+     *
+     * This component intentionally attempts to sync store --> URL Parameter
+     * only when mounted, except when this prop changes.
+     *
+     * XXX: This comes from GlobalSelectionStore and currently does not reset,
+     * so it happens at most once. Can add a reset as needed.
+     */
+    forceUrlSync: PropTypes.bool,
+
+    /// Props passed to child components ///
+
     /**
      * Show absolute date selectors
      */
@@ -75,15 +108,6 @@ class GlobalSelectionHeader extends React.Component {
      */
     showRelative: PropTypes.bool,
 
-    // GlobalSelectionStore is not always initialized (e.g. Group Details) before this is rendered
-    //
-    // This component intentionally attempts to sync store --> URL Parameter
-    // only when mounted, except when this prop changes.
-    //
-    // XXX: This comes from GlobalSelectionStore and currently does not reset,
-    // so it happens at most once. Can add a reset as needed.
-    forceUrlSync: PropTypes.bool,
-
     // Callbacks //
     onChangeProjects: PropTypes.func,
     onUpdateProjects: PropTypes.func,
@@ -110,7 +134,14 @@ class GlobalSelectionHeader extends React.Component {
       return;
     }
 
-    const {location, params, organization, selection} = this.props;
+    const {
+      location,
+      params,
+      organization,
+      selection,
+      shouldForceProject,
+      forceProject,
+    } = this.props;
 
     const hasMultipleProjectFeature = this.hasMultipleProjectSelection();
 
@@ -133,12 +164,7 @@ class GlobalSelectionHeader extends React.Component {
       if (hasMultipleProjectFeature) {
         updateProjects(requestedProjects);
       } else {
-        const allowedProjects =
-          requestedProjects.length > 0
-            ? requestedProjects.slice(0, 1)
-            : this.getFirstProject();
-        updateProjects(allowedProjects);
-        updateParams({project: allowedProjects}, this.getRouter());
+        this.enforceSingleProject({requestedProjects, shouldForceProject, forceProject});
       }
     } else if (params && params.orgId === organization.slug) {
       // Otherwise, if organization has NOT changed,
@@ -147,19 +173,12 @@ class GlobalSelectionHeader extends React.Component {
       // e.g. when switching to a new view that uses this component,
       // update URL parameters to reflect current store
       const {datetime, environments, projects} = selection;
+      const otherParams = {environment: environments, ...datetime};
 
       if (hasMultipleProjectFeature || projects.length === 1) {
-        updateParamsWithoutHistory(
-          {project: projects, environment: environments, ...datetime},
-          this.getRouter()
-        );
+        updateParamsWithoutHistory({project: projects, ...otherParams}, this.getRouter());
       } else {
-        const allowedProjects = this.getFirstProject();
-        updateProjects(allowedProjects);
-        updateParams(
-          {project: allowedProjects, environment: environments, ...datetime},
-          this.getRouter()
-        );
+        this.enforceSingleProject({shouldForceProject, forceProject}, otherParams);
       }
     }
   }
@@ -216,13 +235,32 @@ class GlobalSelectionHeader extends React.Component {
   }
 
   componentDidUpdate(prevProps) {
-    const {hasCustomRouting, location, forceUrlSync, selection} = this.props;
+    const {
+      hasCustomRouting,
+      location,
+      selection,
+      forceUrlSync,
+      forceProject,
+    } = this.props;
 
     if (hasCustomRouting) {
       return;
     }
 
-    // Kind of gross
+    // This means that previously forceProject was falsey (e.g. loading) and now
+    // we have the project to force.
+    //
+    // If user does not have multiple project selection, we need to save the forced
+    // project into the store (if project is not in URL params), otherwise
+    // there will be weird behavior in this component since it just picks a project
+    if (!this.hasMultipleProjectSelection() && forceProject && !prevProps.forceProject) {
+      // Make sure a project isn't specified in query param already, since it should take precendence
+      const {project} = getStateFromQuery(location.query);
+      if (!project) {
+        this.enforceSingleProject({forceProject});
+      }
+    }
+
     if (forceUrlSync && !prevProps.forceUrlSync) {
       const {project, environment} = getStateFromQuery(location.query);
 
@@ -249,6 +287,38 @@ class GlobalSelectionHeader extends React.Component {
     return new Set(this.props.organization.features).has('global-views');
   };
 
+  /**
+   * If user does not have access to `global-views` (e.g. multi project select), then
+   * we update URL params with 1) `props.forceProject`, 2) requested projects from URL params,
+   * 3) first project user is a member of from org
+   */
+  enforceSingleProject = (
+    {requestedProjects, shouldForceProject, forceProject} = {},
+    otherParams
+  ) => {
+    let newProject;
+
+    // This is the case where we *want* to force project, but we are still loading
+    // the forced project's details
+    if (shouldForceProject && !forceProject) {
+      return;
+    }
+
+    if (forceProject) {
+      // this takes precendence over the other options
+      newProject = [getProjectIdFromProject(forceProject)];
+    } else if (requestedProjects && requestedProjects.length > 0) {
+      // If there is a list of projects from URL params, select first project from that list
+      newProject = [requestedProjects[0]];
+    } else {
+      // Otherwise, get first project from org that the user is a member of
+      newProject = this.getFirstProject();
+    }
+
+    updateProjects(newProject);
+    updateParamsWithoutHistory({project: newProject, ...otherParams}, this.getRouter());
+  };
+
   /**
    * Identifies the query params (that are relevant to this component) that have changed
    *
@@ -390,7 +460,7 @@ class GlobalSelectionHeader extends React.Component {
 
   getFirstProject = () => {
     return flatten(this.getProjects())
-      .map(p => parseInt(p.id, 10))
+      .map(getProjectIdFromProject)
       .slice(0, 1);
   };
 
@@ -412,6 +482,7 @@ class GlobalSelectionHeader extends React.Component {
   render() {
     const {
       className,
+      shouldForceProject,
       forceProject,
       organization,
       showAbsolute,
@@ -430,9 +501,10 @@ class GlobalSelectionHeader extends React.Component {
     return (
       <Header className={className}>
         <HeaderItemPosition>
-          {forceProject && this.getBackButton()}
+          {shouldForceProject && this.getBackButton()}
           <MultipleProjectSelector
             organization={organization}
+            shouldForceProject={shouldForceProject}
             forceProject={forceProject}
             projects={projects}
             nonMemberProjects={nonMemberProjects}

src/sentry/static/sentry/app/components/organizations/multipleProjectSelector.jsx

@@ -25,6 +25,7 @@ export default class MultipleProjectSelector extends React.PureComponent {
     onChange: PropTypes.func,
     onUpdate: PropTypes.func,
     multi: PropTypes.bool,
+    shouldForceProject: PropTypes.bool,
     forceProject: SentryTypes.Project,
   };
 
@@ -135,6 +136,7 @@ export default class MultipleProjectSelector extends React.PureComponent {
       nonMemberProjects,
       multi,
       organization,
+      shouldForceProject,
       forceProject,
     } = this.props;
     const selectedProjectIds = new Set(value);
@@ -145,14 +147,23 @@ export default class MultipleProjectSelector extends React.PureComponent {
       selectedProjectIds.has(parseInt(project.id, 10))
     );
 
-    return forceProject ? (
+    // `forceProject` can be undefined if it is loading the project
+    // We are intentionally using an empty string as its "loading" state
+
+    return shouldForceProject ? (
       <StyledHeaderItem
         icon={<StyledInlineSvg src="icon-project" />}
         locked={true}
-        lockedMessage={t(`This issue is unique to the ${forceProject.slug} project`)}
-        settingsLink={`/settings/${organization.slug}/projects/${forceProject.slug}/`}
+        lockedMessage={
+          forceProject
+            ? t(`This issue is unique to the ${forceProject.slug} project`)
+            : t('This issue is unique to a project')
+        }
+        settingsLink={
+          forceProject && `/settings/${organization.slug}/projects/${forceProject.slug}/`
+        }
       >
-        {forceProject.slug}
+        {forceProject ? forceProject.slug : ''}
       </StyledHeaderItem>
     ) : (
       <StyledProjectSelector

src/sentry/static/sentry/app/views/organizationGroupDetails/groupDetails.jsx

@@ -254,6 +254,7 @@ const GroupDetails = createReactClass({
             organization={organization}
             forceProject={project}
             showDateSelector={false}
+            shouldForceProject
           />
         )}
         {isLoading ? (

tests/js/spec/components/organizations/globalSelectionHeader.spec.jsx

@@ -46,7 +46,6 @@ describe('GlobalSelectionHeader', function() {
       location: {query: {}},
     },
   });
-  jest.spyOn(ProjectsStore, 'getAll').mockImplementation(() => organization.projects);
 
   beforeAll(function() {
     jest.spyOn(globalActions, 'updateDateTime');
@@ -55,6 +54,7 @@ describe('GlobalSelectionHeader', function() {
   });
 
   beforeEach(function() {
+    jest.spyOn(ProjectsStore, 'getAll').mockImplementation(() => organization.projects);
     GlobalSelectionStore.reset();
     [
       globalActions.updateDateTime,
@@ -482,6 +482,7 @@ describe('GlobalSelectionHeader', function() {
     const wrapper = mount(
       <GlobalSelectionHeader
         organization={initialData.organization}
+        shouldForceProject
         forceProject={initialData.organization.projects[0]}
       />,
       initialData.routerContext
@@ -502,6 +503,160 @@ describe('GlobalSelectionHeader', function() {
     });
   });
 
+  describe('without global-views (multi-project feature)', function() {
+    describe('without existing URL params', function() {
+      let wrapper;
+      const initialData = initializeOrg({
+        projects: [
+          {id: 0, slug: 'random project', isMember: true},
+          {id: 1, slug: 'staging-project', environments: ['staging']},
+          {id: 2, slug: 'prod-project', environments: ['prod']},
+        ],
+        router: {
+          location: {query: {}},
+          params: {orgId: 'org-slug'},
+        },
+      });
+
+      const createWrapper = props => {
+        wrapper = mount(
+          <GlobalSelectionHeader
+            params={{orgId: initialData.organization.slug}}
+            organization={initialData.organization}
+            {...props}
+          />,
+          initialData.routerContext
+        );
+        return wrapper;
+      };
+
+      beforeEach(function() {
+        jest
+          .spyOn(ProjectsStore, 'getAll')
+          .mockImplementation(() => initialData.organization.projects);
+        initialData.router.push.mockClear();
+        initialData.router.replace.mockClear();
+      });
+
+      it('uses first project in org projects when mounting', async function() {
+        createWrapper();
+
+        await tick();
+        wrapper.update();
+
+        expect(initialData.router.replace).toHaveBeenLastCalledWith({
+          pathname: undefined,
+          query: {project: [0], environment: []},
+        });
+      });
+
+      it('appends projectId to URL when `forceProject` becomes available (async)', async function() {
+        // forceProject generally starts undefined
+        createWrapper({shouldForceProject: true});
+
+        wrapper.setProps({
+          forceProject: initialData.organization.projects[1],
+        });
+
+        wrapper.update();
+
+        expect(initialData.router.replace).toHaveBeenLastCalledWith({
+          pathname: undefined,
+          query: {project: [1]},
+        });
+      });
+
+      it('does not append projectId to URL when `forceProject` becomes available but project id already exists in URL', async function() {
+        // forceProject generally starts undefined
+        createWrapper({shouldForceProject: true});
+
+        wrapper.setContext({
+          router: {
+            ...initialData.router,
+            location: {
+              ...initialData.router.location,
+              query: {
+                project: 321,
+              },
+            },
+          },
+        });
+        wrapper.setProps({
+          forceProject: initialData.organization.projects[1],
+        });
+
+        wrapper.update();
+
+        expect(initialData.router.replace).not.toHaveBeenCalled();
+      });
+
+      it('appends projectId to URL when mounted with `forceProject`', async function() {
+        // forceProject generally starts undefined
+        createWrapper({
+          shouldForceProject: true,
+          forceProject: initialData.organization.projects[1],
+        });
+
+        wrapper.update();
+
+        expect(initialData.router.replace).toHaveBeenLastCalledWith({
+          pathname: undefined,
+          query: {project: [1], environment: []},
+        });
+      });
+    });
+
+    describe('with existing URL params', function() {
+      let wrapper;
+      const initialData = initializeOrg({
+        projects: [
+          {id: 0, slug: 'random project', isMember: true},
+          {id: 1, slug: 'staging-project', environments: ['staging']},
+          {id: 2, slug: 'prod-project', environments: ['prod']},
+        ],
+        router: {
+          location: {query: {statsPeriod: '90d'}},
+          params: {orgId: 'org-slug'},
+        },
+      });
+      jest
+        .spyOn(ProjectsStore, 'getAll')
+        .mockImplementation(() => initialData.organization.projects);
+
+      const createWrapper = props => {
+        wrapper = mount(
+          <GlobalSelectionHeader
+            params={{orgId: initialData.organization.slug}}
+            organization={initialData.organization}
+            {...props}
+          />,
+          initialData.routerContext
+        );
+        return wrapper;
+      };
+
+      beforeEach(function() {
+        initialData.router.push.mockClear();
+        initialData.router.replace.mockClear();
+      });
+
+      it('appends projectId to URL when mounted with `forceProject`', async function() {
+        // forceProject generally starts undefined
+        createWrapper({
+          shouldForceProject: true,
+          forceProject: initialData.organization.projects[1],
+        });
+
+        wrapper.update();
+
+        expect(initialData.router.replace).toHaveBeenLastCalledWith({
+          pathname: undefined,
+          query: {project: [1], statsPeriod: '90d'},
+        });
+      });
+    });
+  });
+
   describe('projects list', function() {
     let wrapper, memberProject, nonMemberProject, initialData;
     beforeEach(function() {