Browse Source

ref(workflow): Refactor assignee out of assignee selector (#43405)

Pulls the assignedTo out of the dropdown render props. Tried to pull the
entire groupStore out of there but listening to group store for trigger
changes is still needed for the loading state will have to take that out
after
Scott Cooper 2 years ago
parent
commit
8fce1b23b2

+ 1 - 0
static/app/components/assigneeSelector.spec.jsx

@@ -82,6 +82,7 @@ describe('AssigneeSelector', () => {
     TeamStore.reset();
     TeamStore.setTeams([TEAM_1]);
     GroupStore.reset();
+    GroupStore.loadInitialData([GROUP_1, GROUP_2]);
 
     jest.spyOn(MemberListStore, 'getAll').mockImplementation(() => null);
     jest.spyOn(ProjectsStore, 'getAll').mockImplementation(() => [PROJECT_1]);

+ 75 - 54
static/app/components/assigneeSelector.tsx

@@ -8,43 +8,46 @@ import {
 } from 'sentry/components/assigneeSelectorDropdown';
 import ActorAvatar from 'sentry/components/avatar/actorAvatar';
 import SuggestedAvatarStack from 'sentry/components/avatar/suggestedAvatarStack';
-import DropdownBubble from 'sentry/components/dropdownBubble';
 import ExternalLink from 'sentry/components/links/externalLink';
 import LoadingIndicator from 'sentry/components/loadingIndicator';
 import Tooltip from 'sentry/components/tooltip';
 import {IconChevron, IconUser} from 'sentry/icons';
 import {t, tct, tn} from 'sentry/locale';
+import GroupStore from 'sentry/stores/groupStore';
+import {useLegacyStore} from 'sentry/stores/useLegacyStore';
 import space from 'sentry/styles/space';
-import type {Actor, SuggestedOwnerReason} from 'sentry/types';
+import type {Actor, Group, SuggestedOwnerReason} from 'sentry/types';
 import useOrganization from 'sentry/utils/useOrganization';
 
 interface AssigneeSelectorProps
-  extends Omit<AssigneeSelectorDropdownProps, 'children' | 'organization'> {
+  extends Omit<
+    AssigneeSelectorDropdownProps,
+    'children' | 'organization' | 'assignedTo'
+  > {
   noDropdown?: boolean;
 }
 
-function AssigneeSelector({noDropdown, ...props}: AssigneeSelectorProps) {
-  const organization = useOrganization();
-
-  function getActorElement(
-    assignedTo?: Actor,
-    suggestedActors: SuggestedAssignee[] = []
-  ) {
-    const suggestedReasons: Record<SuggestedOwnerReason, React.ReactNode> = {
-      suspectCommit: tct('Based on [commit:commit data]', {
-        commit: (
-          <TooltipSubExternalLink href="https://docs.sentry.io/product/sentry-basics/integrate-frontend/configure-scms/" />
-        ),
-      }),
-      releaseCommit: '',
-      ownershipRule: t('Matching Issue Owners Rule'),
-      codeowners: t('Matching Codeowners Rule'),
-    };
-    const assignedToSuggestion = suggestedActors.find(
-      actor => actor.id === assignedTo?.id
-    );
-
-    return assignedTo ? (
+function AssigneeAvatar({
+  assignedTo,
+  suggestedActors = [],
+}: {
+  assignedTo?: Actor;
+  suggestedActors?: SuggestedAssignee[];
+}) {
+  const suggestedReasons: Record<SuggestedOwnerReason, React.ReactNode> = {
+    suspectCommit: tct('Based on [commit:commit data]', {
+      commit: (
+        <TooltipSubExternalLink href="https://docs.sentry.io/product/sentry-basics/integrate-frontend/configure-scms/" />
+      ),
+    }),
+    releaseCommit: '',
+    ownershipRule: t('Matching Issue Owners Rule'),
+    codeowners: t('Matching Codeowners Rule'),
+  };
+  const assignedToSuggestion = suggestedActors.find(actor => actor.id === assignedTo?.id);
+
+  if (assignedTo) {
+    return (
       <ActorAvatar
         actor={assignedTo}
         className="avatar"
@@ -63,7 +66,11 @@ function AssigneeSelector({noDropdown, ...props}: AssigneeSelectorProps) {
           </TooltipWrapper>
         }
       />
-    ) : suggestedActors && suggestedActors.length > 0 ? (
+    );
+  }
+
+  if (suggestedActors.length > 0) {
+    return (
       <SuggestedAvatarStack
         size={28}
         owners={suggestedActors}
@@ -86,36 +93,53 @@ function AssigneeSelector({noDropdown, ...props}: AssigneeSelectorProps) {
           </TooltipWrapper>
         }
       />
-    ) : (
-      <Tooltip
-        isHoverable
-        skipWrapper
-        title={
-          <TooltipWrapper>
-            <div>{t('Unassigned')}</div>
-            <TooltipSubtext>
-              {tct(
-                'You can auto-assign issues by adding [issueOwners:Issue Owner rules].',
-                {
-                  issueOwners: (
-                    <TooltipSubExternalLink href="https://docs.sentry.io/product/error-monitoring/issue-owners/" />
-                  ),
-                }
-              )}
-            </TooltipSubtext>
-          </TooltipWrapper>
-        }
-      >
-        <StyledIconUser data-test-id="unassigned" size="md" color="gray400" />
-      </Tooltip>
     );
   }
 
+  return (
+    <Tooltip
+      isHoverable
+      skipWrapper
+      title={
+        <TooltipWrapper>
+          <div>{t('Unassigned')}</div>
+          <TooltipSubtext>
+            {tct(
+              'You can auto-assign issues by adding [issueOwners:Issue Owner rules].',
+              {
+                issueOwners: (
+                  <TooltipSubExternalLink href="https://docs.sentry.io/product/error-monitoring/issue-owners/" />
+                ),
+              }
+            )}
+          </TooltipSubtext>
+        </TooltipWrapper>
+      }
+    >
+      <StyledIconUser data-test-id="unassigned" size="md" color="gray400" />
+    </Tooltip>
+  );
+}
+
+function AssigneeSelector({noDropdown, ...props}: AssigneeSelectorProps) {
+  const organization = useOrganization();
+  const groups = useLegacyStore(GroupStore);
+  const group = groups.find(item => item.id === props.id) as Group;
+
   return (
     <AssigneeWrapper>
-      <AssigneeSelectorDropdown organization={organization} {...props}>
-        {({loading, isOpen, assignedTo, getActorProps, suggestedAssignees}) => {
-          const avatarElement = getActorElement(assignedTo, suggestedAssignees);
+      <AssigneeSelectorDropdown
+        organization={organization}
+        assignedTo={group.assignedTo}
+        {...props}
+      >
+        {({loading, isOpen, getActorProps, suggestedAssignees}) => {
+          const avatarElement = (
+            <AssigneeAvatar
+              assignedTo={group.assignedTo}
+              suggestedActors={suggestedAssignees}
+            />
+          );
 
           return (
             <Fragment>
@@ -147,9 +171,6 @@ const AssigneeWrapper = styled('div')`
   justify-content: flex-end;
 
   /* manually align menu underneath dropdown caret */
-  ${DropdownBubble} {
-    right: -14px;
-  }
 `;
 
 const StyledIconUser = styled(IconUser)`

+ 8 - 10
static/app/components/assigneeSelectorDropdown.tsx

@@ -45,13 +45,13 @@ type RenderProps = {
   isOpen: boolean;
   loading: boolean;
   suggestedAssignees: SuggestedAssignee[];
-  assignedTo?: Actor;
 };
 
 export interface AssigneeSelectorDropdownProps {
   children: (props: RenderProps) => React.ReactNode;
   id: string;
   organization: Organization;
+  assignedTo?: Actor;
   disabled?: boolean;
   memberList?: User[];
   onAssign?: (
@@ -64,7 +64,6 @@ export interface AssigneeSelectorDropdownProps {
 
 type State = {
   loading: boolean;
-  assignedTo?: Actor;
   memberList?: User[];
   suggestedOwners?: SuggestedOwner[] | null;
 };
@@ -95,7 +94,6 @@ export class AssigneeSelectorDropdown extends Component<
       const group = GroupStore.get(this.props.id);
       this.setState({
         loading,
-        assignedTo: group?.assignedTo,
         suggestedOwners: group?.owners,
       });
     }
@@ -126,7 +124,7 @@ export class AssigneeSelectorDropdown extends Component<
     if (currentMembers === undefined && nextState.memberList !== currentMembers) {
       return true;
     }
-    return !valueIsEqual(nextState.assignedTo, this.state.assignedTo, true);
+    return !valueIsEqual(this.props.assignedTo, nextProps.assignedTo, true);
   }
 
   componentWillUnmount() {
@@ -162,7 +160,6 @@ export class AssigneeSelectorDropdown extends Component<
     }
     const group = GroupStore.get(this.props.id);
     this.setState({
-      assignedTo: group?.assignedTo,
       suggestedOwners: group?.owners,
       loading: GroupStore.hasStatus(this.props.id, 'assignTo'),
     });
@@ -320,7 +317,7 @@ export class AssigneeSelectorDropdown extends Component<
   renderSuggestedAssigneeNodes(): React.ComponentProps<
     typeof DropdownAutoComplete
   >['items'] {
-    const {assignedTo} = this.state;
+    const {assignedTo} = this.props;
     const textReason: Record<SuggestedOwnerReason, string> = {
       suspectCommit: t('Suspect Commit'),
       releaseCommit: t('Suspect Release'),
@@ -531,10 +528,12 @@ export class AssigneeSelectorDropdown extends Component<
   }
 
   render() {
-    const {disabled, children} = this.props;
-    const {loading, assignedTo} = this.state;
+    const {disabled, children, assignedTo} = this.props;
+    const {loading} = this.state;
     const memberList = this.memberList();
 
+    const suggestedAssignees = this.getSuggestedAssignees();
+
     return (
       <DropdownAutoComplete
         disabled={disabled}
@@ -572,8 +571,7 @@ export class AssigneeSelectorDropdown extends Component<
             loading,
             isOpen,
             getActorProps,
-            assignedTo,
-            suggestedAssignees: this.getSuggestedAssignees(),
+            suggestedAssignees,
           })
         }
       </DropdownAutoComplete>

+ 26 - 14
static/app/components/group/assignedTo.spec.tsx

@@ -92,17 +92,21 @@ describe('Group > AssignedTo', () => {
   });
 
   it('can assign team', async () => {
+    const assignedGroup: Group = {
+      ...GROUP_1,
+      assignedTo: {...TEAM_1, type: 'team'},
+    };
     const assignMock = MockApiClient.addMockResponse({
       method: 'PUT',
       url: `/issues/${GROUP_1.id}/`,
-      body: {
-        ...GROUP_1,
-        assignedTo: {...TEAM_1, type: 'team'},
-      },
-    });
-    render(<AssignedTo project={project} group={GROUP_1} event={event} />, {
-      organization,
+      body: assignedGroup,
     });
+    const {rerender} = render(
+      <AssignedTo project={project} group={GROUP_1} event={event} />,
+      {
+        organization,
+      }
+    );
     await openMenu();
     expect(screen.queryByTestId('loading-indicator')).not.toBeInTheDocument();
 
@@ -118,24 +122,30 @@ describe('Group > AssignedTo', () => {
       )
     );
 
+    // Group changes are passed down from parent component
+    rerender(<AssignedTo project={project} group={assignedGroup} event={event} />);
     expect(await screen.findByText(team1slug)).toBeInTheDocument();
     // TEAM_1 initials
     expect(screen.getByTestId('assignee-selector')).toHaveTextContent('CT');
   });
 
   it('successfully clears assignment', async () => {
+    const assignedGroup: Group = {
+      ...GROUP_1,
+      assignedTo: {...TEAM_1, type: 'team'},
+    };
     const assignMock = MockApiClient.addMockResponse({
       method: 'PUT',
       url: `/issues/${GROUP_1.id}/`,
-      body: {
-        ...GROUP_1,
-        assignedTo: {...TEAM_1, type: 'team'},
-      },
+      body: assignedGroup,
     });
 
-    render(<AssignedTo project={project} group={GROUP_1} event={event} />, {
-      organization,
-    });
+    const {rerender} = render(
+      <AssignedTo project={project} group={GROUP_1} event={event} />,
+      {
+        organization,
+      }
+    );
     await openMenu();
 
     // Assign first item in list, which is TEAM_1
@@ -150,6 +160,8 @@ describe('Group > AssignedTo', () => {
       )
     );
 
+    // Group changes are passed down from parent component
+    rerender(<AssignedTo project={project} group={assignedGroup} event={event} />);
     await openMenu();
     userEvent.click(screen.getByRole('button', {name: 'Clear Assignee'}));
 

+ 9 - 8
static/app/components/group/assignedTo.tsx

@@ -127,13 +127,13 @@ function getOwnerList(
   }));
 }
 
-export function getAssignedToDisplayName(group: Group, assignedTo?: Actor) {
-  if (assignedTo?.type === 'team') {
+export function getAssignedToDisplayName(group: Group) {
+  if (group.assignedTo?.type === 'team') {
     const team = TeamStore.getById(group.assignedTo.id);
     return `#${team?.slug ?? group.assignedTo.name}`;
   }
-  if (assignedTo?.type === 'user') {
-    const user = MemberListStore.getById(assignedTo.id);
+  if (group.assignedTo?.type === 'user') {
+    const user = MemberListStore.getById(group.assignedTo.id);
     return user?.name ?? group.assignedTo.name;
   }
 
@@ -215,16 +215,17 @@ function AssignedTo({group, project, event, disableDropdown = false}: AssignedTo
           owners={owners}
           disabled={disableDropdown}
           id={group.id}
+          assignedTo={group.assignedTo}
         >
-          {({loading, assignedTo, isOpen, getActorProps}) => (
+          {({loading, isOpen, getActorProps}) => (
             <DropdownButton data-test-id="assignee-selector" {...getActorProps({})}>
               <ActorWrapper>
                 {loading ? (
                   <StyledLoadingIndicator mini size={24} />
-                ) : assignedTo ? (
+                ) : group.assignedTo ? (
                   <ActorAvatar
                     data-test-id="assigned-avatar"
-                    actor={assignedTo}
+                    actor={group.assignedTo}
                     hasTooltip={false}
                     size={24}
                   />
@@ -233,7 +234,7 @@ function AssignedTo({group, project, event, disableDropdown = false}: AssignedTo
                     <IconUser size="md" />
                   </IconWrapper>
                 )}
-                <ActorName>{getAssignedToDisplayName(group, assignedTo)}</ActorName>
+                <ActorName>{getAssignedToDisplayName(group)}</ActorName>
               </ActorWrapper>
               {!disableDropdown && (
                 <IconChevron

+ 12 - 0
static/app/stores/groupStore.spec.tsx

@@ -216,5 +216,17 @@ describe('GroupStore', function () {
         expect(GroupStore.trigger).toHaveBeenCalledWith(new Set(['1', '2', '3']));
       });
     });
+
+    describe('onAssignToSuccess()', function () {
+      it("should treat undefined itemIds argument as 'all'", function () {
+        GroupStore.items = [g('1')];
+        const assignedGroup = g('1', {assignedTo: TestStubs.User({type: 'user'})});
+        GroupStore.onAssignToSuccess('1337', '1', assignedGroup);
+
+        expect(GroupStore.trigger).toHaveBeenCalledTimes(1);
+        expect(GroupStore.trigger).toHaveBeenCalledWith(new Set(['1']));
+        expect(GroupStore.items[0]).toEqual(assignedGroup);
+      });
+    });
   });
 });

+ 4 - 3
static/app/stores/groupStore.tsx

@@ -323,11 +323,12 @@ const storeConfig: GroupStoreDefinition = {
   },
 
   onAssignToSuccess(_changeId, itemId, response) {
-    const item = this.get(itemId);
-    if (!item) {
+    const idx = this.items.findIndex(i => i.id === itemId);
+    if (idx === -1) {
       return;
     }
-    item.assignedTo = response.assignedTo;
+
+    this.items[idx] = {...this.items[idx], assignedTo: response.assignedTo};
     this.clearStatus(itemId, 'assignTo');
     this.updateItems([itemId]);
   },

+ 1 - 1
static/app/views/eventsV2/table/quickContext/issueContext.tsx

@@ -147,7 +147,7 @@ function IssueContext(props: BaseContextProps) {
               <IconUser size="md" />
             </StyledIconWrapper>
           )}
-          {getAssignedToDisplayName(issue, issue.assignedTo)}
+          {getAssignedToDisplayName(issue)}
         </AssignedToBody>
       </IssueContextContainer>
     );