feat(codeowners): Display alert with code owner errors (#27144)

Objective:
We should allow users to successfully upload CodeOwners, even if not all External Teams/Users have been mapped. For large orgs, atleast some external actors will be mapped and they can get some value from the feature.
Then when users view the page, we should show a warning that there are errors associated with their Code Owners.

docs-ui/components/alert.stories.js

@@ -1,9 +1,8 @@
-import React from 'react';
 import styled from '@emotion/styled';
 
 import Alert from 'app/components/alert';
 import ExternalLink from 'app/components/links/externalLink';
-import {IconCheckmark, IconInfo, IconNot, IconWarning} from 'app/icons';
+import {IconCheckmark, IconInfo, IconLightning, IconNot, IconWarning} from 'app/icons';
 import space from 'app/styles/space';
 
 export default {
@@ -98,6 +97,48 @@ System.parameters = {
   },
 };
 
+export const Expandable = () => {
+  return (
+    <Grid>
+      <Alert
+        type="info"
+        icon={<IconInfo size="md" />}
+        expand={[<div key="1">Here is some details</div>]}
+      >
+        Expandable Alert
+      </Alert>
+
+      <Alert
+        type="success"
+        icon={<IconCheckmark size="md" />}
+        expand={[<div key="1">Here is some details</div>]}
+        expandIcon={<IconLightning size="md" />}
+      >
+        Expandable Alert with Custom Expand Icon
+      </Alert>
+
+      <Alert
+        type="warning"
+        icon={<IconWarning size="md" />}
+        expand={[<div key="1">Here is some details</div>]}
+        expandIcon={<IconCheckmark size="md" />}
+        onExpandIconClick={() => {}}
+      >
+        Expandable Alert with Custom Expand Icon behaviour
+      </Alert>
+    </Grid>
+  );
+};
+
+Expandable.storyName = 'Expandable';
+Expandable.parameters = {
+  docs: {
+    description: {
+      story: 'Expand with details',
+    },
+  },
+};
+
 const Grid = styled('div')`
   display: grid;
   grid-gap: ${space(1)};

src/sentry/api/endpoints/project_codeowners.py

@@ -1,5 +1,5 @@
 import logging
-from typing import Any, List, Mapping, MutableMapping, Sequence, Union
+from typing import Any, Mapping, MutableMapping, Sequence, Union
 
 from rest_framework import serializers, status
 from rest_framework.exceptions import PermissionDenied
@@ -18,9 +18,8 @@ from sentry.models import (
     ProjectCodeOwners,
     RepositoryProjectPathConfig,
     UserEmail,
-    actor_type_to_string,
 )
-from sentry.ownership.grammar import convert_codeowners_syntax, parse_code_owners
+from sentry.ownership.grammar import convert_codeowners_syntax
 from sentry.utils import metrics
 
 logger = logging.getLogger(__name__)
@@ -31,22 +30,14 @@ def validate_association(
     associations: Sequence[Union[UserEmail, ExternalActor]],
     type: str,
 ) -> Sequence[str]:
+    raw_items_set = {str(item) for item in raw_items}
     if type == "emails":
         # associations are UserEmail objects
-        sentry_items = [item.email for item in associations]
+        sentry_items = {item.email for item in associations}
     else:
         # associations are ExternalActor objects
-        sentry_items = [item.external_name for item in associations]
-
-    diff = [str(item) for item in raw_items if item not in sentry_items]
-    unique_diff = list(dict.fromkeys(diff).keys())
-
-    if len(unique_diff):
-        return [
-            f'The following {type} do not have an association in Sentry: {", ".join(unique_diff)}.'
-        ]
-
-    return []
+        sentry_items = {item.external_name for item in associations}
+    return list(raw_items_set.difference(sentry_items))
 
 
 class ProjectCodeOwnerSerializer(CamelSnakeModelSerializer):  # type: ignore
@@ -70,49 +61,12 @@ class ProjectCodeOwnerSerializer(CamelSnakeModelSerializer):  # type: ignore
         if not attrs.get("raw", "").strip():
             return attrs
 
-        external_association_err: List[str] = []
-        # Get list of team/user names from CODEOWNERS file
-        team_names, usernames, emails = parse_code_owners(attrs["raw"])
-
-        # Check if there exists Sentry users with the emails listed in CODEOWNERS
-        user_emails = UserEmail.objects.filter(
-            email__in=emails,
-            user__sentry_orgmember_set__organization=self.context["project"].organization,
-        )
-
-        user_emails_diff = validate_association(emails, user_emails, "emails")
-        external_association_err.extend(user_emails_diff)
-
-        # Check if the usernames have an association
-        external_actors = ExternalActor.objects.filter(
-            external_name__in=usernames + team_names,
-            organization=self.context["project"].organization,
+        # Ignore association errors and continue parsing CODEOWNERS for valid lines.
+        # Allow users to incrementally fix association errors; for CODEOWNERS with many external mappings.
+        associations, _ = ProjectCodeOwners.validate_codeowners_associations(
+            attrs, self.context["project"]
         )
 
-        external_users_diff = validate_association(usernames, external_actors, "usernames")
-        external_association_err.extend(external_users_diff)
-
-        external_teams_diff = validate_association(team_names, external_actors, "team names")
-        external_association_err.extend(external_teams_diff)
-
-        if len(external_association_err):
-            raise serializers.ValidationError({"raw": "\n".join(external_association_err)})
-
-        # Convert CODEOWNERS into IssueOwner syntax
-        users_dict = {}
-        teams_dict = {}
-        for external_actor in external_actors:
-            type = actor_type_to_string(external_actor.actor.type)
-            if type == "user":
-                user = external_actor.actor.resolve()
-                users_dict[external_actor.external_name] = user.email
-            elif type == "team":
-                team = external_actor.actor.resolve()
-                teams_dict[external_actor.external_name] = f"#{team.slug}"
-
-        emails_dict = {email: email for email in emails}
-        associations = {**users_dict, **teams_dict, **emails_dict}
-
         issue_owner_rules = convert_codeowners_syntax(
             attrs["raw"], associations, attrs["code_mapping_id"]
         )
@@ -193,6 +147,8 @@ class ProjectCodeOwnersEndpoint(ProjectEndpoint, ProjectOwnershipMixin, ProjectC
             raise PermissionDenied
 
         expand = request.GET.getlist("expand", [])
+        expand.append("errors")
+
         codeowners = list(ProjectCodeOwners.objects.filter(project=project))
 
         return Response(
@@ -206,7 +162,7 @@ class ProjectCodeOwnersEndpoint(ProjectEndpoint, ProjectOwnershipMixin, ProjectC
 
     def post(self, request: Request, project: Project) -> Response:
         """
-        Upload a CODEWONERS for project
+        Upload a CODEOWNERS for project
         `````````````
 
         :pparam string organization_slug: the slug of the organization.
@@ -220,9 +176,13 @@ class ProjectCodeOwnersEndpoint(ProjectEndpoint, ProjectOwnershipMixin, ProjectC
             raise PermissionDenied
 
         serializer = ProjectCodeOwnerSerializer(
-            context={"ownership": self.get_ownership(project), "project": project},
+            context={
+                "ownership": self.get_ownership(project),
+                "project": project,
+            },
             data={**request.data},
         )
+
         if serializer.is_valid():
             project_codeowners = serializer.save()
             self.track_response_code("create", status.HTTP_201_CREATED)
@@ -238,7 +198,7 @@ class ProjectCodeOwnersEndpoint(ProjectEndpoint, ProjectOwnershipMixin, ProjectC
                     project_codeowners,
                     request.user,
                     serializer=projectcodeowners_serializers.ProjectCodeOwnersSerializer(
-                        expand=["ownershipSyntax"]
+                        expand=["ownershipSyntax", "errors"]
                     ),
                 ),
                 status=status.HTTP_201_CREATED,

src/sentry/api/endpoints/project_codeowners_details.py

@@ -9,6 +9,7 @@ from sentry.api.bases.project import ProjectEndpoint
 from sentry.api.endpoints.project_ownership import ProjectOwnershipMixin
 from sentry.api.exceptions import ResourceDoesNotExist
 from sentry.api.serializers import serialize
+from sentry.api.serializers.models import projectcodeowners as projectcodeowners_serializers
 from sentry.models import ProjectCodeOwners
 
 from .project_codeowners import ProjectCodeOwnerSerializer, ProjectCodeOwnersMixin
@@ -67,7 +68,16 @@ class ProjectCodeOwnersDetailsEndpoint(
                 codeowners_id=updated_codeowners.id,
             )
             self.track_response_code("update", status.HTTP_200_OK)
-            return Response(serialize(updated_codeowners, request.user), status=status.HTTP_200_OK)
+            return Response(
+                serialize(
+                    updated_codeowners,
+                    request.user,
+                    serializer=projectcodeowners_serializers.ProjectCodeOwnersSerializer(
+                        expand=["ownershipSyntax", "errors"]
+                    ),
+                ),
+                status=status.HTTP_200_OK,
+            )
 
         self.track_response_code("update", status.HTTP_400_BAD_REQUEST)
         return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)

src/sentry/api/endpoints/project_ownership.py

@@ -15,7 +15,7 @@ class ProjectOwnershipSerializer(serializers.Serializer):
     autoAssignment = serializers.BooleanField()
 
     def validate(self, attrs):
-        if not attrs.get("raw", "").strip():
+        if "raw" not in attrs:
             return attrs
         try:
             rules = parse_rules(attrs["raw"])

src/sentry/api/serializers/models/projectcodeowners.py

@@ -52,4 +52,8 @@ class ProjectCodeOwnersSerializer(Serializer):
         if "ownershipSyntax" in self.expand:
             data["ownershipSyntax"] = convert_schema_to_rules_text(obj.schema)
 
+        if "errors" in self.expand:
+            _, errors = ProjectCodeOwners.validate_codeowners_associations(data, obj.project)
+            data["errors"] = errors
+
         return data

src/sentry/models/projectcodeowners.py

@@ -33,7 +33,7 @@ class ProjectCodeOwners(DefaultFieldsModel):
 
     @classmethod
     def get_cache_key(self, project_id):
-        return f"projectcodewoners_project_id:1:{project_id}"
+        return f"projectcodeowners_project_id:1:{project_id}"
 
     @classmethod
     def get_codeowners_cached(self, project_id):
@@ -53,3 +53,55 @@ class ProjectCodeOwners(DefaultFieldsModel):
                 codeowners = False
             cache.set(cache_key, codeowners, READ_CACHE_DURATION)
         return codeowners or None
+
+    @classmethod
+    def validate_codeowners_associations(self, attrs, project):
+        from sentry.api.endpoints.project_codeowners import validate_association
+        from sentry.models import ExternalActor, UserEmail, actor_type_to_string
+        from sentry.ownership.grammar import parse_code_owners
+
+        # Get list of team/user names from CODEOWNERS file
+        team_names, usernames, emails = parse_code_owners(attrs["raw"])
+
+        # Check if there exists Sentry users with the emails listed in CODEOWNERS
+        user_emails = UserEmail.objects.filter(
+            email__in=emails,
+            user__sentry_orgmember_set__organization=project.organization,
+        )
+
+        # Check if the usernames/teamnames have an association
+        external_actors = ExternalActor.objects.filter(
+            external_name__in=usernames + team_names,
+            organization=project.organization,
+        )
+
+        # Convert CODEOWNERS into IssueOwner syntax
+        users_dict = {}
+        teams_dict = {}
+        teams_without_access = []
+        for external_actor in external_actors:
+            type = actor_type_to_string(external_actor.actor.type)
+            if type == "user":
+                user = external_actor.actor.resolve()
+                users_dict[external_actor.external_name] = user.email
+            elif type == "team":
+                team = external_actor.actor.resolve()
+                # make sure the sentry team has access to the project
+                # tied to the codeowner
+                if project in team.get_projects():
+                    teams_dict[external_actor.external_name] = f"#{team.slug}"
+                else:
+                    teams_without_access.append(f"#{team.slug}")
+
+        emails_dict = {item.email: item.email for item in user_emails}
+        associations = {**users_dict, **teams_dict, **emails_dict}
+
+        errors = {
+            "missing_user_emails": validate_association(emails, user_emails, "emails"),
+            "missing_external_users": validate_association(usernames, external_actors, "usernames"),
+            "missing_external_teams": validate_association(
+                team_names, external_actors, "team names"
+            ),
+            "teams_without_access": teams_without_access,
+        }
+        return associations, errors

src/sentry/ownership/grammar.py

@@ -292,8 +292,23 @@ def convert_codeowners_syntax(data, associations, code_mapping):
             continue
 
         path, *codeowners = rule.split()
-        sentry_assignees = [associations[owner] for owner in codeowners]
-        formatted_path = path.replace(code_mapping.source_root, code_mapping.stack_root, 1)
-        result += f'path:{formatted_path} {" ".join(sentry_assignees)}\n'
+        sentry_assignees = []
+
+        for owner in codeowners:
+            try:
+                sentry_assignees.append(associations[owner])
+            except KeyError:
+                # We allow users to upload an incomplete codeowner file,
+                # meaning they may not have all the associations mapped.
+                # If this is the case, we simply skip this line when
+                # converting to issue owner syntax
+
+                # TODO(meredith): log and/or collect analytics for when
+                # we skip associations
+                continue
+
+        if sentry_assignees:
+            formatted_path = path.replace(code_mapping.source_root, code_mapping.stack_root, 1)
+            result += f'path:{formatted_path} {" ".join(sentry_assignees)}\n'
 
     return result

static/app/components/alert.tsx

@@ -1,8 +1,9 @@
-import * as React from 'react';
+import React, {useState} from 'react';
 import {css} from '@emotion/react';
 import styled from '@emotion/styled';
 import classNames from 'classnames';
 
+import {IconChevron} from 'app/icons';
 import space from 'app/styles/space';
 import {Theme} from 'app/utils/theme';
 
@@ -10,6 +11,9 @@ type Props = {
   type?: keyof Theme['alert'];
   icon?: React.ReactNode;
   system?: boolean;
+  expand?: React.ReactNode[];
+  expandIcon?: React.ReactNode;
+  onExpandIconClick?: () => void;
 };
 
 type AlertProps = Omit<React.HTMLProps<HTMLDivElement>, keyof Props> & Props;
@@ -60,6 +64,7 @@ const getSystemAlertColorStyles = ({
 
 const alertStyles = ({theme, type = DEFAULT_TYPE, system}: Props & {theme: Theme}) => css`
   display: flex;
+  flex-direction: column;
   margin: 0 0 ${space(3)};
   padding: ${space(1.5)} ${space(2)};
   font-size: 15px;
@@ -79,24 +84,65 @@ const alertStyles = ({theme, type = DEFAULT_TYPE, system}: Props & {theme: Theme
 
 const StyledTextBlock = styled('span')`
   line-height: 1.5;
-  flex-grow: 1;
   position: relative;
-  margin: auto;
+  margin-right: auto;
 `;
 
+const MessageContainer = styled('div')`
+  display: flex;
+  width: 100%;
+`;
+
+const ExpandContainer = styled('div')`
+  display: grid;
+  grid-template-columns: minmax(${space(4)}, 1fr) 30fr 1fr;
+  grid-template-areas: '. details details';
+  padding: ${space(1.5)} 0;
+`;
+const DetailsContainer = styled('div')`
+  grid-area: details;
+`;
+
+const ExpandIcon = styled(props => (
+  <IconWrapper {...props}>{<IconChevron size="md" />}</IconWrapper>
+))`
+  transform: ${props => (props.isExpanded ? 'rotate(0deg)' : 'rotate(180deg)')};
+  cursor: pointer;
+  justify-self: flex-end;
+`;
 const Alert = styled(
   ({
     type,
     icon,
     children,
     className,
+    expand,
+    expandIcon,
+    onExpandIconClick,
     system: _system, // don't forward to `div`
     ...props
   }: AlertProps) => {
+    const [isExpanded, setIsExpanded] = useState(false);
+    const showExpand = expand && expand.length;
+    const showExpandItems = showExpand && isExpanded;
+    const handleOnExpandIconClick = onExpandIconClick ? onExpandIconClick : setIsExpanded;
+
     return (
       <div className={classNames(type ? `ref-${type}` : '', className)} {...props}>
-        {icon && <IconWrapper>{icon}</IconWrapper>}
-        <StyledTextBlock>{children}</StyledTextBlock>
+        <MessageContainer>
+          {icon && <IconWrapper>{icon}</IconWrapper>}
+          <StyledTextBlock>{children}</StyledTextBlock>
+          {showExpand && (
+            <div onClick={() => handleOnExpandIconClick(!isExpanded)}>
+              {expandIcon || <ExpandIcon isExpanded={isExpanded} />}
+            </div>
+          )}
+        </MessageContainer>
+        {showExpandItems && (
+          <ExpandContainer>
+            <DetailsContainer>{(expand || []).map(item => item)}</DetailsContainer>
+          </ExpandContainer>
+        )}
       </div>
     );
   }

static/app/types/index.tsx

@@ -2140,7 +2140,13 @@ export type CodeOwners = {
   dateCreated: string;
   dateUpdated: string;
   provider: 'github' | 'gitlab';
-  codeMapping?: RepositoryProjectPathConfig[];
+  codeMapping?: RepositoryProjectPathConfig;
+  errors: {
+    missing_external_teams: string[];
+    missing_external_users: string[];
+    missing_user_emails: string[];
+    teams_without_access: string[];
+  };
 };
 
 export type KeyValueListData = {

static/app/views/performance/traceDetails/transactionDetail.tsx

@@ -17,7 +17,7 @@ import {
 } from 'app/components/performance/waterfall/rowDetails';
 import {generateIssueEventTarget} from 'app/components/quickTrace/utils';
 import {PAGE_URL_PARAM} from 'app/constants/globalSelectionHeader';
-import {IconAnchor, IconChevron, IconWarning} from 'app/icons';
+import {IconAnchor, IconWarning} from 'app/icons';
 import {t, tn} from 'app/locale';
 import space from 'app/styles/space';
 import {Organization} from 'app/types';
@@ -37,22 +37,9 @@ type Props = {
   scrollToHash: (hash: string) => void;
 };
 
-type State = {
-  errorsOpened: boolean;
-};
-
-class TransactionDetail extends Component<Props, State> {
-  state: State = {
-    errorsOpened: false,
-  };
-
-  toggleErrors = () => {
-    this.setState(({errorsOpened}) => ({errorsOpened: !errorsOpened}));
-  };
-
+class TransactionDetail extends Component<Props> {
   renderTransactionErrors() {
     const {organization, transaction} = this.props;
-    const {errorsOpened} = this.state;
     const {errors} = transaction;
 
     if (errors.length === 0) {
@@ -60,32 +47,29 @@ class TransactionDetail extends Component<Props, State> {
     }
 
     return (
-      <Alert system type="error" icon={<IconWarning size="md" />}>
+      <Alert
+        system
+        type="error"
+        icon={<IconWarning size="md" />}
+        expand={errors.map(error => (
+          <ErrorMessageContent key={error.event_id}>
+            <ErrorDot level={error.level} />
+            <ErrorLevel>{error.level}</ErrorLevel>
+            <ErrorTitle>
+              <Link to={generateIssueEventTarget(error, organization)}>
+                {error.title}
+              </Link>
+            </ErrorTitle>
+          </ErrorMessageContent>
+        ))}
+      >
         <ErrorMessageTitle>
           {tn(
             'An error event occurred in this transaction.',
             '%s error events occurred in this transaction.',
             errors.length
           )}
-          <Toggle priority="link" onClick={this.toggleErrors}>
-            <IconChevron direction={errorsOpened ? 'up' : 'down'} />
-          </Toggle>
         </ErrorMessageTitle>
-        {errorsOpened && (
-          <ErrorMessageContent>
-            {errors.map(error => (
-              <Fragment key={error.event_id}>
-                <ErrorDot level={error.level} />
-                <ErrorLevel>{error.level}</ErrorLevel>
-                <ErrorTitle>
-                  <Link to={generateIssueEventTarget(error, organization)}>
-                    {error.title}
-                  </Link>
-                </ErrorTitle>
-              </Fragment>
-            ))}
-          </ErrorMessageContent>
-        )}
       </Alert>
     );
   }
@@ -276,12 +260,4 @@ const StyledButton = styled(Button)`
   right: ${space(0.5)};
 `;
 
-const Toggle = styled(Button)`
-  font-weight: bold;
-  color: ${p => p.theme.subText};
-  :hover {
-    color: ${p => p.theme.textColor};
-  }
-`;
-
 export default TransactionDetail;