From a99f3e93003392470bdba00afc9d62c73b7ac545 Mon Sep 17 00:00:00 2001 From: Priscila Oliveira Date: Wed, 16 Oct 2024 13:18:59 +0200 Subject: [PATCH 01/67] ref(quick-start): Expose new feature flag (#79177) --- src/sentry/features/temporary.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/features/temporary.py b/src/sentry/features/temporary.py index 9c25551aae218a..a76af8cd77f7a4 100644 --- a/src/sentry/features/temporary.py +++ b/src/sentry/features/temporary.py @@ -352,7 +352,7 @@ def register_temporary_features(manager: FeatureManager): manager.add("organizations:project-event-date-limit", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) manager.add("organizations:project-templates", OrganizationFeature, FeatureHandlerStrategy.INTERNAL, api_expose=False) # Enable the new quick start guide - manager.add("organizations:quick-start-updates", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) + manager.add("organizations:quick-start-updates", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True) # Enable the new Related Events feature manager.add("organizations:related-events", OrganizationFeature, FeatureHandlerStrategy.INTERNAL, api_expose=False) # Enable related issues feature From 9c40e4d39e7ba18107af25acc7fbcc1285f541d0 Mon Sep 17 00:00:00 2001 From: Klaus Rettinghaus Date: Wed, 16 Oct 2024 13:51:51 +0200 Subject: [PATCH 02/67] chore(comments): fix some typos and unify comments (#78999) This PR fixes some minor typos I stumbled upon and unifies most of the `TODO` comments to be highlighted. ### Legal Boilerplate Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms. --- src/flagpole/conditions.py | 2 +- src/sentry/api/endpoints/admin_project_configs.py | 2 +- src/sentry/api/endpoints/organization_events_trace.py | 2 +- src/sentry/api/endpoints/organization_events_trends.py | 2 +- src/sentry/api/endpoints/organization_events_trends_v2.py | 2 +- src/sentry/api/endpoints/organization_metrics_tag_details.py | 2 +- src/sentry/api/endpoints/organization_metrics_tags.py | 2 +- src/sentry/api/paginator.py | 2 +- src/sentry/api/serializers/models/organization.py | 2 +- src/sentry/api/serializers/models/project.py | 2 +- src/sentry/buffer/redis.py | 2 +- src/sentry/db/models/fields/node.py | 2 +- src/sentry/deletions/defaults/repository.py | 2 +- src/sentry/eventstore/models.py | 2 +- src/sentry/integrations/gitlab/webhooks.py | 2 +- .../slack/message_builder/notifications/rule_save_edit.py | 2 +- src/sentry/issues/endpoints/group_notes_details.py | 2 +- src/sentry/search/events/builder/base.py | 2 +- src/sentry/search/snuba/executors.py | 2 +- src/sentry/sentry_metrics/indexer/cache.py | 2 +- src/sentry/snuba/metrics/utils.py | 2 +- src/sentry/statistical_detectors/issue_platform_adapter.py | 2 +- src/sentry/tasks/summaries/weekly_reports.py | 2 +- src/sentry/users/api/endpoints/user_authenticator_details.py | 2 +- src/sentry/users/api/serializers/user.py | 2 +- .../performance_issues/detectors/consecutive_db_detector.py | 2 +- src/sentry/utils/performance_issues/performance_problem.py | 2 +- src/sentry/utils/snuba.py | 4 ++-- tests/snuba/api/endpoints/test_organization_events.py | 2 +- .../endpoints/test_organization_events_has_measurements.py | 2 +- 30 files changed, 31 insertions(+), 31 deletions(-) diff --git a/src/flagpole/conditions.py b/src/flagpole/conditions.py index ff3e8c7404cd3e..ac530ffbeb6de8 100644 --- a/src/flagpole/conditions.py +++ b/src/flagpole/conditions.py @@ -20,7 +20,7 @@ class ConditionOperatorKind(str, Enum): """Provided a single value, check if the property (a list) is not included""" EQUALS = "equals" - """Comprare a value to another. Values are compared with types""" + """Compare a value to another. Values are compared with types""" NOT_EQUALS = "not_equals" """Compare a value to not be equal to another. Values are compared with types""" diff --git a/src/sentry/api/endpoints/admin_project_configs.py b/src/sentry/api/endpoints/admin_project_configs.py index a33b9a6d5a2596..3b46ff73c17ac0 100644 --- a/src/sentry/api/endpoints/admin_project_configs.py +++ b/src/sentry/api/endpoints/admin_project_configs.py @@ -46,7 +46,7 @@ def get(self, request: Request) -> Response: else: configs[key] = None - # TODO if we don't think we'll add anything to the endpoint + # TODO: if we don't think we'll add anything to the endpoint # we may as well return just the configs return Response({"configs": configs}, status=200) diff --git a/src/sentry/api/endpoints/organization_events_trace.py b/src/sentry/api/endpoints/organization_events_trace.py index d539dc89331fbf..92f9f881500a99 100644 --- a/src/sentry/api/endpoints/organization_events_trace.py +++ b/src/sentry/api/endpoints/organization_events_trace.py @@ -762,7 +762,7 @@ def build_span_query(trace_id: str, spans_params: SnubaParams, query_spans: list sentry_sdk.set_measurement("trace_view.spans.span_minimum", span_minimum) sentry_sdk.set_tag("trace_view.split_by_char.optimization", len(query_spans) > span_minimum) if len(query_spans) > span_minimum: - # TODO because we're not doing an IN on a list of literals, snuba will not optimize the query with the HexInt + # TODO: because we're not doing an IN on a list of literals, snuba will not optimize the query with the HexInt # column processor which means we won't be taking advantage of the span_id index but if we only do this when we # have a lot of query_spans we should have a great performance improvement still once we do that we can simplify # this code and always apply this optimization diff --git a/src/sentry/api/endpoints/organization_events_trends.py b/src/sentry/api/endpoints/organization_events_trends.py index 0d92aa34063208..e2cded11a2ee81 100644 --- a/src/sentry/api/endpoints/organization_events_trends.py +++ b/src/sentry/api/endpoints/organization_events_trends.py @@ -54,7 +54,7 @@ class TrendColumns(TypedDict): TREND_TYPES = [IMPROVED, REGRESSION] -# TODO move this to the builder file and introduce a top-events version instead +# TODO: move this to the builder file and introduce a top-events version instead class TrendQueryBuilder(DiscoverQueryBuilder): def convert_aggregate_filter_to_condition( self, aggregate_filter: AggregateFilter diff --git a/src/sentry/api/endpoints/organization_events_trends_v2.py b/src/sentry/api/endpoints/organization_events_trends_v2.py index e293806d073fa4..95ca60a010f0af 100644 --- a/src/sentry/api/endpoints/organization_events_trends_v2.py +++ b/src/sentry/api/endpoints/organization_events_trends_v2.py @@ -177,7 +177,7 @@ def get_timeseries(top_events, _, rollup, zerofill_results): results[result_key]["data"].append(row) else: discarded += 1 - # TODO filter out entries that don't have transaction or trend_function + # TODO: filter out entries that don't have transaction or trend_function logger.warning( "trends.top-events.timeseries.key-mismatch", extra={ diff --git a/src/sentry/api/endpoints/organization_metrics_tag_details.py b/src/sentry/api/endpoints/organization_metrics_tag_details.py index 58ccb308646115..01b4e3d4dba634 100644 --- a/src/sentry/api/endpoints/organization_metrics_tag_details.py +++ b/src/sentry/api/endpoints/organization_metrics_tag_details.py @@ -41,7 +41,7 @@ def get(self, request: Request, organization: Organization, tag_name: str) -> Re for project in projects ): if len(metric_names) == 1 and metric_names[0].startswith("d:eap"): - # TODO hack for EAP, hardcode some metric names + # TODO: hack for EAP, hardcode some metric names if tag_name == "color": return Response( [ diff --git a/src/sentry/api/endpoints/organization_metrics_tags.py b/src/sentry/api/endpoints/organization_metrics_tags.py index 041eb403727ac8..52d0cde0c3a01d 100644 --- a/src/sentry/api/endpoints/organization_metrics_tags.py +++ b/src/sentry/api/endpoints/organization_metrics_tags.py @@ -58,7 +58,7 @@ def get(self, request: Request, organization: Organization) -> Response: for project in projects ): if metric_name.startswith("d:eap"): - # TODO hack for EAP, return a fixed list + # TODO: hack for EAP, return a fixed list return Response([Tag(key="color"), Tag(key="location")]) try: diff --git a/src/sentry/api/paginator.py b/src/sentry/api/paginator.py index 61684a9161f3e7..173ce25e871486 100644 --- a/src/sentry/api/paginator.py +++ b/src/sentry/api/paginator.py @@ -537,7 +537,7 @@ def get_result(self, limit, cursor=None): prev=Cursor(0, max(0, offset - limit), True, offset > 0), next=Cursor(0, max(0, offset + limit), False, has_more), ) - # TODO use Cursor.value as the `end` argument to data_fn() so that + # TODO: use Cursor.value as the `end` argument to data_fn() so that # subsequent pages returned using these cursors are using the same end # date for queries, this should stop drift from new incoming events. diff --git a/src/sentry/api/serializers/models/organization.py b/src/sentry/api/serializers/models/organization.py index aede5826daeda2..906781cfcee906 100644 --- a/src/sentry/api/serializers/models/organization.py +++ b/src/sentry/api/serializers/models/organization.py @@ -421,7 +421,7 @@ def serialize( class _DetailedOrganizationSerializerResponseOptional(OrganizationSerializerResponse, total=False): - role: Any # TODO replace with enum/literal + role: Any # TODO: replace with enum/literal orgRole: str uptimeAutodetection: bool diff --git a/src/sentry/api/serializers/models/project.py b/src/sentry/api/serializers/models/project.py index 821d3ccf82c0df..3465fa28b34c95 100644 --- a/src/sentry/api/serializers/models/project.py +++ b/src/sentry/api/serializers/models/project.py @@ -275,7 +275,7 @@ class ProjectSerializerResponse(ProjectSerializerBaseResponse): isPublic: bool avatar: SerializedAvatarFields color: str - status: str # TODO enum/literal + status: str # TODO: enum/literal @register(Project) diff --git a/src/sentry/buffer/redis.py b/src/sentry/buffer/redis.py index 8b19cbf05e9e30..5b0ffbd01aa252 100644 --- a/src/sentry/buffer/redis.py +++ b/src/sentry/buffer/redis.py @@ -34,7 +34,7 @@ # load everywhere _last_validation_log: float | None = None Pipeline = Any -# TODO type Pipeline instead of using Any here +# TODO: type Pipeline instead of using Any here def _get_model_key(model: type[models.Model]) -> str: diff --git a/src/sentry/db/models/fields/node.py b/src/sentry/db/models/fields/node.py index 7e3844319f67ee..c58cad00fbb329 100644 --- a/src/sentry/db/models/fields/node.py +++ b/src/sentry/db/models/fields/node.py @@ -192,7 +192,7 @@ def to_python(self, value): try: value = pickle.loads(decompress(value)) except Exception as e: - # TODO this is a bit dangerous as a failure to read/decode the + # TODO: this is a bit dangerous as a failure to read/decode the # node_id will end up with this record being replaced with an # empty value under a new key, potentially orphaning an # original value in nodestore. OTOH if we can't decode the info diff --git a/src/sentry/deletions/defaults/repository.py b/src/sentry/deletions/defaults/repository.py index 960befb3b93d01..41d2adedc16009 100644 --- a/src/sentry/deletions/defaults/repository.py +++ b/src/sentry/deletions/defaults/repository.py @@ -29,7 +29,7 @@ def get_child_relations(self, instance: Repository) -> list[BaseRelation]: return _get_repository_child_relations(instance) def delete_instance(self, instance: Repository) -> None: - # TODO child_relations should also send pending_delete so we + # TODO: child_relations should also send pending_delete so we # don't have to do this here. pending_delete.send(sender=type(instance), instance=instance, actor=self.get_actor()) diff --git a/src/sentry/eventstore/models.py b/src/sentry/eventstore/models.py index b8bda1fc180cf4..6014a7f2ff0884 100644 --- a/src/sentry/eventstore/models.py +++ b/src/sentry/eventstore/models.py @@ -602,7 +602,7 @@ def group_id(self) -> int | None: def group_id(self, value: int | None) -> None: self._group_id = value - # TODO We need a better way to cache these properties. functools + # TODO: We need a better way to cache these properties. functools # doesn't quite do the trick as there is a reference bug with unsaved # models. But the current _group_cache thing is also clunky because these # properties need to be stripped out in __getstate__. diff --git a/src/sentry/integrations/gitlab/webhooks.py b/src/sentry/integrations/gitlab/webhooks.py index 063cbe02fa008c..ad3a507153d766 100644 --- a/src/sentry/integrations/gitlab/webhooks.py +++ b/src/sentry/integrations/gitlab/webhooks.py @@ -168,7 +168,7 @@ def __call__( authors = {} - # TODO gitlab only sends a max of 20 commits. If a push contains + # TODO: gitlab only sends a max of 20 commits. If a push contains # more commits they provide a total count and require additional API # requests to fetch the commit details for commit in event.get("commits", []): diff --git a/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py b/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py index 74c202dde397ec..0241d925a561fe 100644 --- a/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py +++ b/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py @@ -48,7 +48,7 @@ def build(self) -> SlackBlock: else: rule_text = "*Alert rule updated*\n\n" rule_text += f"{rule_url} in the {project_url} project was recently updated." - # TODO potentially use old name if it's changed? + # TODO: potentially use old name if it's changed? blocks.append(self.get_markdown_block(rule_text)) diff --git a/src/sentry/issues/endpoints/group_notes_details.py b/src/sentry/issues/endpoints/group_notes_details.py index 65fb6012f2eedf..7097802a0f0f05 100644 --- a/src/sentry/issues/endpoints/group_notes_details.py +++ b/src/sentry/issues/endpoints/group_notes_details.py @@ -84,7 +84,7 @@ def put(self, request: Request, group, note_id) -> Response: if serializer.is_valid(): payload = serializer.validated_data - # TODO adding mentions to a note doesn't send notifications. Should it? + # TODO: adding mentions to a note doesn't send notifications. Should it? # Remove mentions as they shouldn't go into the database payload.pop("mentions", []) diff --git a/src/sentry/search/events/builder/base.py b/src/sentry/search/events/builder/base.py index c1b7c3f899efbe..b02a6cf7b181fe 100644 --- a/src/sentry/search/events/builder/base.py +++ b/src/sentry/search/events/builder/base.py @@ -309,7 +309,7 @@ def resolve_time_conditions(self) -> None: self.end = self.params.end def resolve_column_name(self, col: str) -> str: - # TODO when utils/snuba.py becomes typed don't need this extra annotation + # TODO: when utils/snuba.py becomes typed don't need this extra annotation column_resolver: Callable[[str], str] = resolve_column(self.dataset) column_name = column_resolver(col) # If the original column was passed in as tag[X], then there won't be a conflict diff --git a/src/sentry/search/snuba/executors.py b/src/sentry/search/snuba/executors.py index 7c3e1e9017ea8f..8f7fe5086eab0b 100644 --- a/src/sentry/search/snuba/executors.py +++ b/src/sentry/search/snuba/executors.py @@ -1027,7 +1027,7 @@ def query( # * we started with Postgres candidates and so only do one Snuba query max # * the paginator is returning enough results to satisfy the query (>= the limit) # * there are no more groups in Snuba to post-filter - # TODO do we actually have to rebuild this SequencePaginator every time + # TODO: do we actually have to rebuild this SequencePaginator every time # or can we just make it after we've broken out of the loop? paginator_results = SequencePaginator( [(score, id) for (id, score) in result_groups], reverse=True, **paginator_options diff --git a/src/sentry/sentry_metrics/indexer/cache.py b/src/sentry/sentry_metrics/indexer/cache.py index 0676acf5e59d7d..4ced794ad54026 100644 --- a/src/sentry/sentry_metrics/indexer/cache.py +++ b/src/sentry/sentry_metrics/indexer/cache.py @@ -284,7 +284,7 @@ def resolve(self, use_case_id: UseCaseID, org_id: int, string: str) -> int | Non _INDEXER_CACHE_RESOLVE_METRIC, tags={"cache_hit": "false", "use_case": use_case_id.value}, ) - # TODO this random rollout is backwards + # TODO: this random rollout is backwards if random.random() >= options.get( "sentry-metrics.indexer.disable-memcache-replenish-rollout" ): diff --git a/src/sentry/snuba/metrics/utils.py b/src/sentry/snuba/metrics/utils.py index d68225a3676d2a..91e1f3e2a580b8 100644 --- a/src/sentry/snuba/metrics/utils.py +++ b/src/sentry/snuba/metrics/utils.py @@ -487,7 +487,7 @@ def to_intervals( assert interval_seconds > 0 # horrible hack for backward compatibility - # TODO Try to fix this upstream + # TODO: Try to fix this upstream if start is None or end is None: return None, None, 0 diff --git a/src/sentry/statistical_detectors/issue_platform_adapter.py b/src/sentry/statistical_detectors/issue_platform_adapter.py index f1c1173d57ca1d..30c937ae50f545 100644 --- a/src/sentry/statistical_detectors/issue_platform_adapter.py +++ b/src/sentry/statistical_detectors/issue_platform_adapter.py @@ -24,7 +24,7 @@ def send_regression_to_platform(regression: BreakpointData): displayed_new_baseline = round(float(regression["aggregate_range_2"]), 2) # For legacy reasons, we're passing project id as project - # TODO fix this in the breakpoint microservice and in trends v2 + # TODO: fix this in the breakpoint microservice and in trends v2 project_id = int(regression["project"]) issue_type: type[GroupType] = PerformanceP95EndpointRegressionGroupType diff --git a/src/sentry/tasks/summaries/weekly_reports.py b/src/sentry/tasks/summaries/weekly_reports.py index a3f6d464eb792e..bc6b79a874c381 100644 --- a/src/sentry/tasks/summaries/weekly_reports.py +++ b/src/sentry/tasks/summaries/weekly_reports.py @@ -320,7 +320,7 @@ def send_email(self, template_ctx: Mapping[str, Any], user_id: int) -> None: user_project_count=template_ctx["user_project_count"], ) - # TODO see if we can use the UUID to track if the email was sent or not + # TODO: see if we can use the UUID to track if the email was sent or not logger.info( "weekly_report.send_email", extra={ diff --git a/src/sentry/users/api/endpoints/user_authenticator_details.py b/src/sentry/users/api/endpoints/user_authenticator_details.py index f0dd463623587a..731ce1a8118026 100644 --- a/src/sentry/users/api/endpoints/user_authenticator_details.py +++ b/src/sentry/users/api/endpoints/user_authenticator_details.py @@ -131,7 +131,7 @@ def put( :auth required: """ - # TODO temporary solution for both renaming and regenerating recovery code. + # TODO: temporary solution for both renaming and regenerating recovery code. # Need to find new home for regenerating recovery codes as it doesn't really do what put is supposed to do try: authenticator = Authenticator.objects.get(user=user, id=auth_id) diff --git a/src/sentry/users/api/serializers/user.py b/src/sentry/users/api/serializers/user.py index 87f2b7a54bdbd6..4d987a9bc19c62 100644 --- a/src/sentry/users/api/serializers/user.py +++ b/src/sentry/users/api/serializers/user.py @@ -61,7 +61,7 @@ class _Identity(TypedDict): class _UserOptions(TypedDict): theme: str # TODO: enum/literal for theme options language: str - stacktraceOrder: int # TODO enum/literal + stacktraceOrder: int # TODO: enum/literal defaultIssueEvent: str timezone: str clock24Hours: bool diff --git a/src/sentry/utils/performance_issues/detectors/consecutive_db_detector.py b/src/sentry/utils/performance_issues/detectors/consecutive_db_detector.py index 907b2cbf112ce4..4307e668b631f9 100644 --- a/src/sentry/utils/performance_issues/detectors/consecutive_db_detector.py +++ b/src/sentry/utils/performance_issues/detectors/consecutive_db_detector.py @@ -125,7 +125,7 @@ def _store_performance_problem(self) -> None: self.stored_problems[fingerprint] = PerformanceProblem( fingerprint, "db", - desc=query, # TODO - figure out which query to use for description + desc=query, # TODO: figure out which query to use for description type=PerformanceConsecutiveDBQueriesGroupType, cause_span_ids=cause_span_ids, parent_span_ids=None, diff --git a/src/sentry/utils/performance_issues/performance_problem.py b/src/sentry/utils/performance_issues/performance_problem.py index ecd5b4a276b654..11c978539f3468 100644 --- a/src/sentry/utils/performance_issues/performance_problem.py +++ b/src/sentry/utils/performance_issues/performance_problem.py @@ -18,7 +18,7 @@ class PerformanceProblem: # The actual bad spans offender_span_ids: Sequence[str] # Evidence to be used for the group - # TODO make evidence_data and evidence_display required once all detectors have been migrated to platform + # TODO: make evidence_data and evidence_display required once all detectors have been migrated to platform # We can't make it required until we stop loading these from nodestore via EventPerformanceProblem, # since there's legacy data in there that won't have these fields. # So until we disable transaction based perf issues we'll need to keep this optional. diff --git a/src/sentry/utils/snuba.py b/src/sentry/utils/snuba.py index 1eb08360d4ec69..eacf788dc98864 100644 --- a/src/sentry/utils/snuba.py +++ b/src/sentry/utils/snuba.py @@ -787,7 +787,7 @@ def _prepare_query_params(query_params: SnubaQueryParams, referrer: str | None = "groupby": query_params.groupby, "conditions": query_params_conditions, "aggregations": query_params.aggregations, - "granularity": query_params.rollup, # TODO name these things the same + "granularity": query_params.rollup, # TODO: name these things the same } ) kwargs = {k: v for k, v in kwargs.items() if v is not None} @@ -1644,7 +1644,7 @@ def aliased_query_params( ) -# TODO (evanh) Since we are assuming that all string values are columns, +# TODO: (evanh) Since we are assuming that all string values are columns, # this will get tricky if we ever have complex columns where there are # string arguments to the functions that aren't columns def resolve_complex_column(col, resolve_func, ignored): diff --git a/tests/snuba/api/endpoints/test_organization_events.py b/tests/snuba/api/endpoints/test_organization_events.py index a23f3339c1e6ae..22dd30c889c777 100644 --- a/tests/snuba/api/endpoints/test_organization_events.py +++ b/tests/snuba/api/endpoints/test_organization_events.py @@ -82,7 +82,7 @@ def _setup_user_misery( self, per_transaction_threshold: bool = False, project: Project | None = None ) -> None: _project = project or self.project - # If duration is > 300 * 4 then the user is fruistrated + # If duration is > 300 * 4 then the user is frustrated # There's a total of 4 users and three of them reach the frustration threshold events = [ ("one", 300), diff --git a/tests/snuba/api/endpoints/test_organization_events_has_measurements.py b/tests/snuba/api/endpoints/test_organization_events_has_measurements.py index 7622c050dc7cb8..9c4768c0f094a2 100644 --- a/tests/snuba/api/endpoints/test_organization_events_has_measurements.py +++ b/tests/snuba/api/endpoints/test_organization_events_has_measurements.py @@ -108,7 +108,7 @@ def test_no_events(self): assert response.data == {"measurements": False} def test_has_event_but_no_web_measurements(self): - # make sure the transaction doesnt have measurements + # make sure the transaction doesn't have measurements self.transaction_data["measurements"] = {} self.store_event(self.transaction_data, self.project.id) From 8b18aa35b8bdd748e21f7d346e804341ab341bc6 Mon Sep 17 00:00:00 2001 From: Priscila Oliveira Date: Wed, 16 Oct 2024 14:05:22 +0200 Subject: [PATCH 03/67] feat(quick-start): Add logic to skeleton - Part 1 (#79158) --- .../onboardingWizard/newSidebar.tsx | 252 ++++++++++++++---- .../onboardingWizard/taskConfig.tsx | 61 ++++- .../app/components/onboardingWizard/utils.tsx | 2 +- static/app/types/onboarding.tsx | 13 + 4 files changed, 270 insertions(+), 58 deletions(-) diff --git a/static/app/components/onboardingWizard/newSidebar.tsx b/static/app/components/onboardingWizard/newSidebar.tsx index 0c945e6ba3ceea..ea3f45a1dcff5a 100644 --- a/static/app/components/onboardingWizard/newSidebar.tsx +++ b/static/app/components/onboardingWizard/newSidebar.tsx @@ -1,18 +1,73 @@ -import {Fragment, useState} from 'react'; +import { + Fragment, + useCallback, + useContext, + useEffect, + useMemo, + useRef, + useState, +} from 'react'; import {css} from '@emotion/react'; import styled from '@emotion/styled'; +import partition from 'lodash/partition'; +import {navigateTo} from 'sentry/actionCreators/navigation'; +import {updateOnboardingTask} from 'sentry/actionCreators/onboardingTasks'; import {Chevron} from 'sentry/components/chevron'; import InteractionStateLayer from 'sentry/components/interactionStateLayer'; +import { + OnboardingContext, + type OnboardingContextProps, +} from 'sentry/components/onboarding/onboardingContext'; +import {findCompleteTasks, taskIsDone} from 'sentry/components/onboardingWizard/utils'; import ProgressRing from 'sentry/components/progressRing'; import SidebarPanel from 'sentry/components/sidebar/sidebarPanel'; import type {CommonSidebarProps} from 'sentry/components/sidebar/types'; import {Tooltip} from 'sentry/components/tooltip'; import {IconCheckmark} from 'sentry/icons'; import {t, tct} from 'sentry/locale'; +import DemoWalkthroughStore from 'sentry/stores/demoWalkthroughStore'; import pulsingIndicatorStyles from 'sentry/styles/pulsingIndicator'; import {space} from 'sentry/styles/space'; +import {type OnboardingTask, OnboardingTaskGroup} from 'sentry/types/onboarding'; +import type {Organization} from 'sentry/types/organization'; +import type {Project} from 'sentry/types/project'; +import {trackAnalytics} from 'sentry/utils/analytics'; import {isDemoWalkthrough} from 'sentry/utils/demoMode'; +import useApi from 'sentry/utils/useApi'; +import useOrganization from 'sentry/utils/useOrganization'; +import useProjects from 'sentry/utils/useProjects'; +import useRouter from 'sentry/utils/useRouter'; + +import {getMergedTasks} from './taskConfig'; + +/** + * How long (in ms) to delay before beginning to mark tasks complete + */ +const INITIAL_MARK_COMPLETE_TIMEOUT = 600; + +/** + * How long (in ms) to delay between marking each unseen task as complete. + */ +const COMPLETION_SEEN_TIMEOUT = 800; + +function useOnboardingTasks( + organization: Organization, + projects: Project[], + onboardingContext: OnboardingContextProps +) { + return useMemo(() => { + const all = getMergedTasks({ + organization, + projects, + onboardingContext, + }).filter(task => task.display); + return { + allTasks: all, + basicTasks: all.filter(task => task.group === OnboardingTaskGroup.BASIC), + }; + }, [organization, projects, onboardingContext]); +} function getPanelDescription(walkthrough: boolean) { if (walkthrough) { @@ -28,16 +83,56 @@ function getPanelDescription(walkthrough: boolean) { } interface TaskProps { - description: string; - title: string; + hidePanel: () => void; + task: OnboardingTask; status?: 'waiting' | 'completed'; } -function Task({title, description, status}: TaskProps) { +function Task({task, status, hidePanel}: TaskProps) { + const organization = useOrganization(); + const router = useRouter(); + + const handleClick = useCallback( + (e: React.MouseEvent) => { + trackAnalytics('quick_start.task_card_clicked', { + organization, + todo_id: task.task, + todo_title: task.title, + action: 'clickthrough', + }); + + e.stopPropagation(); + + if (isDemoWalkthrough()) { + DemoWalkthroughStore.activateGuideAnchor(task.task); + } + + if (task.actionType === 'external') { + window.open(task.location, '_blank'); + } + + if (task.actionType === 'action') { + task.action(router); + } + + if (task.actionType === 'app') { + // Convert all paths to a location object + let to = + typeof task.location === 'string' ? {pathname: task.location} : task.location; + // Add referrer to all links + to = {...to, query: {...to.query, referrer: 'onboarding_task'}}; + + navigateTo(to, router); + } + hidePanel(); + }, + [task, organization, router, hidePanel] + ); + if (status === 'completed') { return ( - {title} + {task.title} ); @@ -45,11 +140,11 @@ function Task({title, description, status}: TaskProps) { if (status === 'waiting') { return ( - +
- {title} -

{description}

+ {task.title} +

{task.description}

@@ -59,11 +154,11 @@ function Task({title, description, status}: TaskProps) { } return ( - +
- {title} -

{description}

+ {task.title} +

{task.description}

 ); @@ -71,20 +166,18 @@ function Task({title, description, status}: TaskProps) { interface TaskGroupProps { description: string; + hidePanel: () => void; + tasks: OnboardingTask[]; title: string; - totalCompletedTasks: number; - totalTasks: number; expanded?: boolean; } -function TaskGroup({ - title, - description, - totalCompletedTasks, - totalTasks, - expanded, -}: TaskGroupProps) { +function TaskGroup({title, description, tasks, expanded, hidePanel}: TaskGroupProps) { const [isExpanded, setIsExpanded] = useState(expanded); + const [completedTasks, incompletedTasks] = partition(tasks, task => + findCompleteTasks(task) + ); + return ( setIsExpanded(!isExpanded)}> @@ -105,22 +198,32 @@ function TaskGroup({ {tct('[totalCompletedTasks] out of [totalTasks] tasks completed', { - totalCompletedTasks, - totalTasks, + totalCompletedTasks: completedTasks.length, + totalTasks: tasks.length, })} - - - - {t('Completed')} - - + {incompletedTasks.map(task => ( + + ))} + {completedTasks.length > 0 && ( + + {t('Completed')} + {completedTasks.map(task => ( + + ))} + + )} )} @@ -133,8 +236,69 @@ interface NewSidebarProps extends Pick(); + const markCompletionSeenTimeout = useRef(); + + function completionTimeout(time: number): Promise { + window.clearTimeout(markCompletionTimeout.current); + return new Promise(resolve => { + markCompletionTimeout.current = window.setTimeout(resolve, time); + }); + } + + function seenTimeout(time: number): Promise { + window.clearTimeout(markCompletionSeenTimeout.current); + return new Promise(resolve => { + markCompletionSeenTimeout.current = window.setTimeout(resolve, time); + }); + } + + const markTasksAsSeen = useCallback( + async function () { + const unseenTasks = allTasks + .filter(task => taskIsDone(task) && !task.completionSeen) + .map(task => task.task); + + // Incrementally mark tasks as seen. This gives the card completion + // animations time before we move each task into the completed section. + for (const task of unseenTasks) { + await seenTimeout(COMPLETION_SEEN_TIMEOUT); + updateOnboardingTask(api, organization, {task, completionSeen: true}); + } + }, + [api, organization, allTasks] + ); + + const markSeenOnOpen = useCallback( + async function () { + // Add a minor delay to marking tasks complete to account for the animation + // opening of the sidebar panel + await completionTimeout(INITIAL_MARK_COMPLETE_TIMEOUT); + markTasksAsSeen(); + }, + [markTasksAsSeen] + ); + + useEffect(() => { + markSeenOnOpen(); + + return () => { + window.clearTimeout(markCompletionTimeout.current); + window.clearTimeout(markCompletionSeenTimeout.current); + }; + }, [markSeenOnOpen]); return (

{description}

- - - + {basicTasks.length && ( + + )} ); diff --git a/static/app/components/onboardingWizard/taskConfig.tsx b/static/app/components/onboardingWizard/taskConfig.tsx index d9314500789b94..da03e47881783a 100644 --- a/static/app/components/onboardingWizard/taskConfig.tsx +++ b/static/app/components/onboardingWizard/taskConfig.tsx @@ -5,7 +5,10 @@ import {navigateTo} from 'sentry/actionCreators/navigation'; import type {Client} from 'sentry/api'; import type {OnboardingContextProps} from 'sentry/components/onboarding/onboardingContext'; import {filterSupportedTasks} from 'sentry/components/onboardingWizard/filterSupportedTasks'; -import {taskIsDone} from 'sentry/components/onboardingWizard/utils'; +import { + hasQuickStartUpdatesFeature, + taskIsDone, +} from 'sentry/components/onboardingWizard/utils'; import {filterProjects} from 'sentry/components/performanceOnboarding/utils'; import {SidebarPanelKey} from 'sentry/components/sidebar/types'; import {sourceMaps} from 'sentry/data/platformCategories'; @@ -18,7 +21,7 @@ import type { OnboardingTask, OnboardingTaskDescriptor, } from 'sentry/types/onboarding'; -import {OnboardingTaskKey} from 'sentry/types/onboarding'; +import {OnboardingTaskGroup, OnboardingTaskKey} from 'sentry/types/onboarding'; import type {Organization} from 'sentry/types/organization'; import type {Project} from 'sentry/types/project'; import {isDemoWalkthrough} from 'sentry/utils/demoMode'; @@ -168,14 +171,19 @@ export function getOnboardingTasks({ { task: OnboardingTaskKey.FIRST_PROJECT, title: t('Create a project'), - description: t( - "Monitor in seconds by adding a simple lines of code to your project. It's as easy as microwaving leftover pizza." - ), + description: hasQuickStartUpdatesFeature(organization) + ? t( + "Monitor in seconds by adding a few lines of code to your project. It's as easy as microwaving leftover pizza." + ) + : t( + "Monitor in seconds by adding a simple lines of code to your project. It's as easy as microwaving leftover pizza." + ), skippable: false, requisites: [], actionType: 'app', location: `/organizations/${organization.slug}/projects/new/`, display: true, + group: OnboardingTaskGroup.BASIC, }, { task: OnboardingTaskKey.FIRST_EVENT, @@ -204,6 +212,7 @@ export function getOnboardingTasks({ ) : null ), + group: OnboardingTaskGroup.BASIC, }, { task: OnboardingTaskKey.INVITE_MEMBER, @@ -216,6 +225,7 @@ export function getOnboardingTasks({ actionType: 'action', action: () => openInviteMembersModal({source: 'onboarding_widget'}), display: true, + group: OnboardingTaskGroup.BASIC, }, { task: OnboardingTaskKey.FIRST_INTEGRATION, @@ -227,7 +237,34 @@ export function getOnboardingTasks({ requisites: [OnboardingTaskKey.FIRST_PROJECT, OnboardingTaskKey.FIRST_EVENT], actionType: 'app', location: `/settings/${organization.slug}/integrations/`, - display: true, + display: !hasQuickStartUpdatesFeature(organization), + group: OnboardingTaskGroup.BASIC, + }, + { + task: OnboardingTaskKey.REAL_TIME_NOTIFICATIONS, + title: t('Real-time notifications'), + description: t( + 'Triage and resolving issues faster by integrating Sentry with messaging platforms like Slack, Discord and MS Teams.' + ), + skippable: true, + requisites: [OnboardingTaskKey.FIRST_PROJECT, OnboardingTaskKey.FIRST_EVENT], + actionType: 'app', + location: `/settings/${organization.slug}/integrations/?category=chat`, + display: hasQuickStartUpdatesFeature(organization), + group: OnboardingTaskGroup.BASIC, + }, + { + task: OnboardingTaskKey.LINK_SENTRY_TO_SOURCE_CODE, + title: t('Link Sentry to Source Code'), + description: t( + 'Resolve bugs faster with commit data and stack trace linking to your source code in GitHub, Gitlab and more.' + ), + skippable: true, + requisites: [OnboardingTaskKey.FIRST_PROJECT, OnboardingTaskKey.FIRST_EVENT], + actionType: 'app', + location: `/settings/${organization.slug}/integrations/?category=codeowners`, + display: hasQuickStartUpdatesFeature(organization), + group: OnboardingTaskGroup.BASIC, }, { task: OnboardingTaskKey.SECOND_PLATFORM, @@ -362,14 +399,19 @@ export function getOnboardingTasks({ { task: OnboardingTaskKey.RELEASE_TRACKING, title: t('Track releases'), - description: t( - 'Take an in-depth look at the health of each and every release with crash analytics, errors, related issues and suspect commits.' - ), + description: hasQuickStartUpdatesFeature(organization) + ? t( + 'Identify which release introduced an issue and track release health with crash analytics, errors, and adoption data.' + ) + : t( + 'Take an in-depth look at the health of each and every release with crash analytics, errors, related issues and suspect commits.' + ), skippable: true, requisites: [OnboardingTaskKey.FIRST_PROJECT, OnboardingTaskKey.FIRST_EVENT], actionType: 'app', location: `/settings/${organization.slug}/projects/:projectId/release-tracking/`, display: true, + group: OnboardingTaskGroup.BASIC, }, { task: OnboardingTaskKey.SOURCEMAPS, @@ -418,6 +460,7 @@ export function getOnboardingTasks({ actionType: 'app', location: getIssueAlertUrl({projects, organization, onboardingContext}), display: true, + group: OnboardingTaskGroup.BASIC, }, { task: OnboardingTaskKey.METRIC_ALERT, diff --git a/static/app/components/onboardingWizard/utils.tsx b/static/app/components/onboardingWizard/utils.tsx index 38c1e9b9213352..1374b3fc828b03 100644 --- a/static/app/components/onboardingWizard/utils.tsx +++ b/static/app/components/onboardingWizard/utils.tsx @@ -14,5 +14,5 @@ export const findUpcomingTasks = (task: OnboardingTask) => task.requisiteTasks.length > 0 && !findCompleteTasks(task); export function hasQuickStartUpdatesFeature(organization: Organization) { - return organization.features.includes('quick-start-updates'); + return organization.features?.includes('quick-start-updates'); } diff --git a/static/app/types/onboarding.tsx b/static/app/types/onboarding.tsx index d0d8df11eb9674..967c0228368b41 100644 --- a/static/app/types/onboarding.tsx +++ b/static/app/types/onboarding.tsx @@ -9,6 +9,13 @@ import type {Organization} from './organization'; import type {PlatformIntegration, PlatformKey, Project} from './project'; import type {AvatarUser} from './user'; +// TODO(priscilawebdev): Define the groups we would like to display +export enum OnboardingTaskGroup { + BASIC = 'basic', + NEXT = 'next', + LEVEL_UP = 'level_up', +} + export enum OnboardingTaskKey { FIRST_PROJECT = 'create_project', FIRST_EVENT = 'send_first_event', @@ -23,6 +30,8 @@ export enum OnboardingTaskKey { FIRST_TRANSACTION = 'setup_transactions', METRIC_ALERT = 'setup_metric_alert_rules', USER_SELECTED_PROJECTS = 'setup_userselected_projects', + REAL_TIME_NOTIFICATIONS = 'setup_real_time_notifications', + LINK_SENTRY_TO_SOURCE_CODE = 'link_sentry_to_source_code', /// Customized card that shows the selected integrations during onboarding INTEGRATIONS = 'integrations', /// Regular card that tells the user to setup integrations if no integrations were selected during onboarding @@ -68,6 +77,10 @@ interface OnboardingTaskDescriptorBase { * An extra component that may be rendered within the onboarding task item. */ SupplementComponent?: React.ComponentType; + /** + * The group that this task belongs to, e.g. basic and level up + */ + group?: OnboardingTaskGroup; /** * If a render function was provided, it will be used to render the entire card, * and the card will be rendered before any other cards regardless of completion status. From 65292fcff4b7d2ee4b78e09d938085e51f47db98 Mon Sep 17 00:00:00 2001 From: Priscila Oliveira Date: Wed, 16 Oct 2024 14:23:26 +0200 Subject: [PATCH 04/67] ref(quick-start): Apply skeleton feedback (#79178) --- static/app/components/onboardingWizard/newSidebar.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/static/app/components/onboardingWizard/newSidebar.tsx b/static/app/components/onboardingWizard/newSidebar.tsx index ea3f45a1dcff5a..58491d5f679406 100644 --- a/static/app/components/onboardingWizard/newSidebar.tsx +++ b/static/app/components/onboardingWizard/newSidebar.tsx @@ -180,12 +180,12 @@ function TaskGroup({title, description, tasks, expanded, hidePanel}: TaskGroupPr return ( - setIsExpanded(!isExpanded)}> + setIsExpanded(!isExpanded)}> - +
{title}

{description}

- +
Date: Wed, 16 Oct 2024 10:22:06 -0400 Subject: [PATCH 05/67] feat(dashboards): Implement widget frame full screen view button feature (#79051) Very simple, just shows a special button in the top right. Contributes to https://github.com/getsentry/sentry/issues/77779 --- .../widgets/common/widgetFrame.spec.tsx | 21 +++++++++++++++++++ .../widgets/common/widgetFrame.stories.tsx | 18 ++++++++++++++++ .../dashboards/widgets/common/widgetFrame.tsx | 19 +++++++++++++++-- 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/static/app/views/dashboards/widgets/common/widgetFrame.spec.tsx b/static/app/views/dashboards/widgets/common/widgetFrame.spec.tsx index ef9ae46c41d625..cb4dd47517c857 100644 --- a/static/app/views/dashboards/widgets/common/widgetFrame.spec.tsx +++ b/static/app/views/dashboards/widgets/common/widgetFrame.spec.tsx @@ -83,4 +83,25 @@ describe('WidgetFrame', () => { expect(onAction2).toHaveBeenCalledTimes(1); }); }); + + describe('Full Screen View Button', () => { + it('Renders a full screen view button', async () => { + const onFullScreenViewClick = jest.fn(); + const {rerender} = render(); + + expect( + screen.queryByRole('button', {name: 'Open Full-Screen View'}) + ).not.toBeInTheDocument(); + + rerender( + + ); + + const $button = screen.getByRole('button', {name: 'Open Full-Screen View'}); + expect($button).toBeInTheDocument(); + await userEvent.click($button); + + expect(onFullScreenViewClick).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/static/app/views/dashboards/widgets/common/widgetFrame.stories.tsx b/static/app/views/dashboards/widgets/common/widgetFrame.stories.tsx index f01b73e7e09361..b5e5dd411043a2 100644 --- a/static/app/views/dashboards/widgets/common/widgetFrame.stories.tsx +++ b/static/app/views/dashboards/widgets/common/widgetFrame.stories.tsx @@ -127,6 +127,24 @@ export default storyBook(WidgetFrame, story => { ); }); + + story('Full Screen View Button', () => { + return ( + +

+ supports a onOpenFullScreenView{' '} + prop. This is a special action that always appears as an individual icon to the + right of the normal actions. +

+ + + + {}} /> + + + + ); + }); }); const NormalWidget = styled('div')` diff --git a/static/app/views/dashboards/widgets/common/widgetFrame.tsx b/static/app/views/dashboards/widgets/common/widgetFrame.tsx index c34f78528b04f8..26418f9fe581cb 100644 --- a/static/app/views/dashboards/widgets/common/widgetFrame.tsx +++ b/static/app/views/dashboards/widgets/common/widgetFrame.tsx @@ -5,7 +5,7 @@ import {HeaderTitle} from 'sentry/components/charts/styles'; import {DropdownMenu, type MenuItemProps} from 'sentry/components/dropdownMenu'; import QuestionTooltip from 'sentry/components/questionTooltip'; import {Tooltip} from 'sentry/components/tooltip'; -import {IconEllipsis, IconWarning} from 'sentry/icons'; +import {IconEllipsis, IconExpand, IconWarning} from 'sentry/icons'; import {t} from 'sentry/locale'; import {space} from 'sentry/styles/space'; @@ -19,6 +19,7 @@ export interface Props extends StateProps { actions?: MenuItemProps[]; children?: React.ReactNode; description?: string; + onFullScreenViewClick?: () => void; title?: string; warnings?: string[]; } @@ -55,7 +56,9 @@ export function WidgetFrame(props: Props) { {props.title} - {(props.description || (actions && actions.length > 0)) && ( + {(props.description || + props.onFullScreenViewClick || + (actions && actions.length > 0)) && ( {props.description && ( @@ -86,6 +89,18 @@ export function WidgetFrame(props: Props) { position="bottom-end" /> ) : null} + + {props.onFullScreenViewClick && ( + ); diff --git a/static/app/views/app/index.tsx b/static/app/views/app/index.tsx index f616a08b965de1..bc96d394af9631 100644 --- a/static/app/views/app/index.tsx +++ b/static/app/views/app/index.tsx @@ -246,16 +246,16 @@ function App({children, params}: Props) { - - + + {renderBody()} - - + + From 6e88055bd286bbea8bbb23d9e948a4e6620a67a2 Mon Sep 17 00:00:00 2001 From: Scott Cooper Date: Wed, 16 Oct 2024 10:51:37 -0700 Subject: [PATCH 26/67] fix(issues): Shrink big context icons (#79171) --- .../highlights/highlightsIconSummary.tsx | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/static/app/components/events/highlights/highlightsIconSummary.tsx b/static/app/components/events/highlights/highlightsIconSummary.tsx index dd9992e73fcb82..a44ecbe7e30242 100644 --- a/static/app/components/events/highlights/highlightsIconSummary.tsx +++ b/static/app/components/events/highlights/highlightsIconSummary.tsx @@ -48,8 +48,10 @@ export function HighlightsIconSummary({event}: HighlightsIconSummaryProps) { {items.map((item, index) => ( {item.icon} - {item.title} - {item.subtitle} + + {item.title} + {item.subtitle && {item.subtitle}} + ))} @@ -61,32 +63,32 @@ export function HighlightsIconSummary({event}: HighlightsIconSummaryProps) { const IconBar = styled('div')` position: relative; - padding: ${space(2)} ${space(0.5)}; + padding: ${space(1)} ${space(0.5)}; `; const IconSummary = styled('div')` + display: flex; + align-items: center; + gap: ${space(1)}; flex: none; - display: grid; - grid-template: 1fr 1fr / auto 1fr; - grid-column-gap: ${space(1)}; - grid-row-gap: ${space(0.5)}; +`; + +const IconDescription = styled('div')` + display: flex; + flex-direction: column; + gap: ${space(0.5)}; `; const IconWrapper = styled('div')` - grid-area: 1 / 1 / 3 / 2; - align-self: center; + flex: none; `; const IconTitle = styled('div')` - grid-area: 1 / 2 / 2 / 3; - align-self: self-end; line-height: 1; `; const IconSubtitle = styled('div')` - grid-area: 2 / 2 / 3 / 3; color: ${p => p.theme.subText}; font-size: ${p => p.theme.fontSizeSmall}; line-height: 1; - align-self: self-start; `; From 0f87ce6a9338333614e564d50bc3fec99dc5b4dc Mon Sep 17 00:00:00 2001 From: Michelle Zhang <56095982+michellewzhang@users.noreply.github.com> Date: Wed, 16 Oct 2024 10:58:08 -0700 Subject: [PATCH 27/67] fix(flag): actually increase buffer size to 100 (#79196) and remove the default buffer size --- static/app/actionCreators/organization.tsx | 2 +- static/app/utils/featureObserver.ts | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/static/app/actionCreators/organization.tsx b/static/app/actionCreators/organization.tsx index 2b9c8e033c64bf..46882a81c2e82d 100644 --- a/static/app/actionCreators/organization.tsx +++ b/static/app/actionCreators/organization.tsx @@ -42,7 +42,7 @@ async function fetchOrg( } FeatureFlagOverrides.singleton().loadOrg(org); - FeatureObserver.singleton().observeFlags({organization: org, bufferSize: 10}); + FeatureObserver.singleton().observeFlags({organization: org, bufferSize: 100}); OrganizationStore.onUpdate(org, {replace: true}); setActiveOrganization(org); diff --git a/static/app/utils/featureObserver.ts b/static/app/utils/featureObserver.ts index 3c06203daef8da..9d757e241c465a 100644 --- a/static/app/utils/featureObserver.ts +++ b/static/app/utils/featureObserver.ts @@ -1,7 +1,6 @@ import type {Flags} from 'sentry/types/event'; import type {Organization} from 'sentry/types/organization'; -const DEFAULT_BUFFER_SIZE = 100; let __SINGLETON: FeatureObserver | null = null; export default class FeatureObserver { @@ -27,10 +26,10 @@ export default class FeatureObserver { public observeFlags({ organization, - bufferSize = DEFAULT_BUFFER_SIZE, + bufferSize, }: { + bufferSize: number; organization: Organization; - bufferSize?: number; }) { const FLAGS = this.FEATURE_FLAGS; From ba4be16bf9883307ce61f03dcd9c8d812488806d Mon Sep 17 00:00:00 2001 From: Malachi Willey Date: Wed, 16 Oct 2024 10:58:49 -0700 Subject: [PATCH 28/67] feat(issue-stream): Clicking dead space navigates to issue instead of selecting it (#79119) --- static/app/components/eventOrGroupHeader.tsx | 36 +++----- static/app/components/eventOrGroupTitle.tsx | 6 +- static/app/components/overlay.tsx | 2 +- static/app/components/stream/group.spec.tsx | 40 ++++++++- static/app/components/stream/group.tsx | 92 +++++++++++++++++--- static/app/utils/isCtrlKeyPressed.tsx | 2 +- static/app/views/issueList/utils.tsx | 50 +++++++++++ 7 files changed, 187 insertions(+), 41 deletions(-) diff --git a/static/app/components/eventOrGroupHeader.tsx b/static/app/components/eventOrGroupHeader.tsx index 7c37a8352ed935..bf2046d4ad5275 100644 --- a/static/app/components/eventOrGroupHeader.tsx +++ b/static/app/components/eventOrGroupHeader.tsx @@ -15,6 +15,7 @@ import type {Organization} from 'sentry/types/organization'; import {getLocation, getMessage, isTombstone} from 'sentry/utils/events'; import {useLocation} from 'sentry/utils/useLocation'; import withOrganization from 'sentry/utils/withOrganization'; +import {createIssueLink} from 'sentry/views/issueList/utils'; import EventTitleError from './eventTitleError'; @@ -71,8 +72,7 @@ function EventOrGroupHeader({ } function getTitle() { - const {id, status} = data as Group; - const {eventID: latestEventId, groupID} = data as Event; + const {status} = data as Group; const commonEleProps = { 'data-test-id': status === 'resolved' ? 'resolved-issue' : null, @@ -84,32 +84,18 @@ function EventOrGroupHeader({ ); } - // If we have passed in a custom event ID, use it; otherwise use default - const finalEventId = eventId ?? latestEventId; - return ( {getTitleChildren()} diff --git a/static/app/components/eventOrGroupTitle.tsx b/static/app/components/eventOrGroupTitle.tsx index 3fa32eeec96bc5..d3a0cc4ea74b05 100644 --- a/static/app/components/eventOrGroupTitle.tsx +++ b/static/app/components/eventOrGroupTitle.tsx @@ -41,7 +41,11 @@ function EventOrGroupTitle({ groupingCurrentLevel={data.metadata?.current_level} query={query} > - {hasNewLayout ? {titleLabel} : titleLabel} + {hasNewLayout ? ( + {titleLabel} + ) : ( + titleLabel + )} ) : ( titleLabel diff --git a/static/app/components/overlay.tsx b/static/app/components/overlay.tsx index bf29d2407ef63b..26e0ecbf1ca09b 100644 --- a/static/app/components/overlay.tsx +++ b/static/app/components/overlay.tsx @@ -124,7 +124,7 @@ const Overlay = styled( : {style}; return ( - + {defined(arrowProps) && } {children} diff --git a/static/app/components/stream/group.spec.tsx b/static/app/components/stream/group.spec.tsx index 98e9c5268d33d4..a0a888642b2c95 100644 --- a/static/app/components/stream/group.spec.tsx +++ b/static/app/components/stream/group.spec.tsx @@ -1,9 +1,17 @@ import {GroupFixture} from 'sentry-fixture/group'; import {OrganizationFixture} from 'sentry-fixture/organization'; import {ProjectFixture} from 'sentry-fixture/project'; +import {RouterFixture} from 'sentry-fixture/routerFixture'; import {initializeOrg} from 'sentry-test/initializeOrg'; -import {act, render, screen, userEvent, within} from 'sentry-test/reactTestingLibrary'; +import { + act, + render, + screen, + userEvent, + waitFor, + within, +} from 'sentry-test/reactTestingLibrary'; import StreamGroup from 'sentry/components/stream/group'; import GroupStore from 'sentry/stores/groupStore'; @@ -183,4 +191,34 @@ describe('StreamGroup', function () { expect(screen.getByRole('time', {name: 'First Seen'})).toHaveTextContent('1w'); expect(screen.getByRole('time', {name: 'Last Seen'})).toHaveTextContent('1d'); }); + + it('navigates to issue with correct params when clicked', async function () { + const router = RouterFixture(); + render( + , + { + router, + organization: OrganizationFixture({ + features: ['issue-stream-table-layout'], + }), + } + ); + + await userEvent.click(screen.getByTestId('group')); + + await waitFor(() => { + expect(router.push).toHaveBeenCalledWith({ + pathname: '/organizations/org-slug/issues/1337/', + query: { + _allp: 1, + query: 'is:unresolved is:for_review assigned_or_suggested:[me, none]', + referrer: 'issue-stream', + stream_index: undefined, + }, + }); + }); + }); }); diff --git a/static/app/components/stream/group.tsx b/static/app/components/stream/group.tsx index bd66979cb3ab9f..52e3631c269587 100644 --- a/static/app/components/stream/group.tsx +++ b/static/app/components/stream/group.tsx @@ -14,6 +14,7 @@ import EventOrGroupExtraDetails from 'sentry/components/eventOrGroupExtraDetails import EventOrGroupHeader from 'sentry/components/eventOrGroupHeader'; import {AssigneeSelector} from 'sentry/components/group/assigneeSelector'; import {getBadgeProperties} from 'sentry/components/group/inboxBadges/statusBadge'; +import InteractionStateLayer from 'sentry/components/interactionStateLayer'; import type {GroupListColumn} from 'sentry/components/issues/groupList'; import Link from 'sentry/components/links/link'; import PanelItem from 'sentry/components/panels/panelItem'; @@ -45,9 +46,13 @@ import {trackAnalytics} from 'sentry/utils/analytics'; import {isDemoWalkthrough} from 'sentry/utils/demoMode'; import EventView from 'sentry/utils/discover/eventView'; import {SavedQueryDatasets} from 'sentry/utils/discover/types'; +import {isCtrlKeyPressed} from 'sentry/utils/isCtrlKeyPressed'; import {getConfigForIssueType} from 'sentry/utils/issueTypeConfig'; import {useMutation} from 'sentry/utils/queryClient'; import type RequestError from 'sentry/utils/requestError/requestError'; +import normalizeUrl from 'sentry/utils/url/normalizeUrl'; +import {useLocation} from 'sentry/utils/useLocation'; +import {useNavigate} from 'sentry/utils/useNavigate'; import useOrganization from 'sentry/utils/useOrganization'; import usePageFilters from 'sentry/utils/usePageFilters'; import withOrganization from 'sentry/utils/withOrganization'; @@ -56,6 +61,7 @@ import {hasDatasetSelector} from 'sentry/views/dashboards/utils'; import GroupPriority from 'sentry/views/issueDetails/groupPriority'; import {COLUMN_BREAKPOINTS} from 'sentry/views/issueList/actions/utils'; import { + createIssueLink, DISCOVER_EXCLUSION_FIELDS, getTabs, isForReviewQuery, @@ -160,6 +166,8 @@ function BaseGroupRow({ showLastTriggered = false, onPriorityChange, }: Props) { + const navigate = useNavigate(); + const location = useLocation(); const groups = useLegacyStore(GroupStore); const group = useMemo( () => groups.find(item => item.id === id) as Group | undefined, @@ -226,37 +234,32 @@ function BaseGroupRow({ }, }); - const wrapperToggle = useCallback( + const clickHasBeenHandled = useCallback( (evt: React.MouseEvent) => { const targetElement = evt.target as Partial; if (!group) { - return; + return true; } // Ignore clicks on links if (targetElement?.tagName?.toLowerCase() === 'a') { - return; + return true; } // Ignore clicks on the selection checkbox if (targetElement?.tagName?.toLowerCase() === 'input') { - return; + return true; } let e = targetElement; while (e.parentElement) { if (e?.tagName?.toLowerCase() === 'a') { - return; + return true; } e = e.parentElement!; } - if (evt.shiftKey) { - SelectedGroupStore.shiftToggleItems(group.id); - window.getSelection()?.removeAllRanges(); - } else { - SelectedGroupStore.toggleSelect(group.id); - } + return false; }, [group] ); @@ -528,15 +531,61 @@ function BaseGroupRow({ ); + const onClick = (e: React.MouseEvent) => { + if (displayReprocessingLayout) { + return; + } + + const handled = clickHasBeenHandled(e); + + if (handled) { + return; + } + + if (canSelect && e.shiftKey) { + SelectedGroupStore.shiftToggleItems(group.id); + window.getSelection()?.removeAllRanges(); + return; + } + + if (canSelect && isCtrlKeyPressed(e)) { + SelectedGroupStore.toggleSelect(group.id); + return; + } + + if (hasNewLayout) { + navigate( + normalizeUrl( + createIssueLink({ + data: group, + organization, + referrer, + streamIndex: index, + location, + query, + }) + ) + ); + return; + } + + if (!canSelect) { + return; + } + + SelectedGroupStore.toggleSelect(group.id); + }; + return ( + {hasNewLayout && } {canSelect && ( p.hasNewLayout && css` + cursor: pointer; padding: ${space(1)} 0; min-height: 66px; + + /* Adds underline to issue title when active */ + &:hover { + [data-issue-title-primary] { + text-decoration: underline; + } + } + + /* Disables the hover effect when hovering over dropdown buttons and checkboxes */ + &:has(button:hover, input:hover, [data-overlay]:hover) { + [data-layer] { + display: none; + } + + [data-issue-title-primary] { + text-decoration: none; + } + } `} ${p => diff --git a/static/app/utils/isCtrlKeyPressed.tsx b/static/app/utils/isCtrlKeyPressed.tsx index ec42536d1c7863..d0f32dd25264cd 100644 --- a/static/app/utils/isCtrlKeyPressed.tsx +++ b/static/app/utils/isCtrlKeyPressed.tsx @@ -8,7 +8,7 @@ import {isMac} from '@react-aria/utils'; * * [1] https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/selection/src/utils.ts */ -export function isCtrlKeyPressed(e: React.KeyboardEvent) { +export function isCtrlKeyPressed(e: React.KeyboardEvent | React.MouseEvent) { if (isMac()) { return e.metaKey; } diff --git a/static/app/views/issueList/utils.tsx b/static/app/views/issueList/utils.tsx index 400e5cb3ad4e3a..232de49093202c 100644 --- a/static/app/views/issueList/utils.tsx +++ b/static/app/views/issueList/utils.tsx @@ -1,6 +1,11 @@ +import type {Location, LocationDescriptorObject} from 'history'; + import ExternalLink from 'sentry/components/links/externalLink'; import {DEFAULT_QUERY} from 'sentry/constants'; import {t, tct} from 'sentry/locale'; +import type {Event} from 'sentry/types/event'; +import type {Group, GroupTombstoneHelper} from 'sentry/types/group'; +import type {Organization} from 'sentry/types/organization'; export enum Query { FOR_REVIEW = 'is:unresolved is:for_review assigned_or_suggested:[me, my_teams, none]', @@ -250,3 +255,48 @@ function getIssueGroupFilter(group: IssueGroup): string { export function getSearchForIssueGroup(group: IssueGroup): string { return `?${new URLSearchParams(`query=is:unresolved+${getIssueGroupFilter(group)}`)}`; } + +export function createIssueLink({ + organization, + data, + eventId, + referrer, + streamIndex, + location, + query, +}: { + data: Event | Group | GroupTombstoneHelper; + location: Location; + organization: Organization; + eventId?: string; + query?: string; + referrer?: string; + streamIndex?: number; +}): LocationDescriptorObject { + const {id} = data as Group; + const {eventID: latestEventId, groupID} = data as Event; + + // If we have passed in a custom event ID, use it; otherwise use default + const finalEventId = eventId ?? latestEventId; + + return { + pathname: `/organizations/${organization.slug}/issues/${ + latestEventId ? groupID : id + }/${finalEventId ? `events/${finalEventId}/` : ''}`, + query: { + referrer: referrer || 'event-or-group-header', + stream_index: streamIndex, + query, + // This adds sort to the query if one was selected from the + // issues list page + ...(location.query.sort !== undefined ? {sort: location.query.sort} : {}), + // This appends _allp to the URL parameters if they have no + // project selected ("all" projects included in results). This is + // so that when we enter the issue details page and lock them to + // a project, we can properly take them back to the issue list + // page with no project selected (and not the locked project + // selected) + ...(location.query.project !== undefined ? {} : {_allp: 1}), + }, + }; +} From 8158cf83ae5ae5c3aeac116e3c1bf94e09c32879 Mon Sep 17 00:00:00 2001 From: anthony sottile <103459774+asottile-sentry@users.noreply.github.com> Date: Wed, 16 Oct 2024 14:12:10 -0400 Subject: [PATCH 29/67] ref: remove/replace calls to iso_format(...) in relay / symbolicator tests (#79060) automated using https://github.com/getsentry/sentry/pull/78931#issue-2578827921 --- .../lang/java/test_plugin.py | 28 +++++++++---------- .../lang/javascript/test_example.py | 4 +-- .../lang/javascript/test_plugin.py | 4 +-- tests/relay_integration/test_integration.py | 10 +++---- .../test_metrics_extraction.py | 14 +++++----- tests/symbolicator/test_payload_full.py | 10 +++---- 6 files changed, 35 insertions(+), 35 deletions(-) diff --git a/tests/relay_integration/lang/java/test_plugin.py b/tests/relay_integration/lang/java/test_plugin.py index 0b579817ef6c8d..283d69414d9227 100644 --- a/tests/relay_integration/lang/java/test_plugin.py +++ b/tests/relay_integration/lang/java/test_plugin.py @@ -9,7 +9,7 @@ from sentry.models.debugfile import ProjectDebugFile from sentry.models.files.file import File from sentry.testutils.cases import TransactionTestCase -from sentry.testutils.helpers.datetime import before_now, iso_format +from sentry.testutils.helpers.datetime import before_now from sentry.testutils.relay import RelayStoreHelper from sentry.testutils.skips import requires_symbolicator from sentry.utils import json @@ -469,7 +469,7 @@ def test_basic_resolving(self): } ] }, - "timestamp": iso_format(before_now(seconds=1)), + "timestamp": before_now(seconds=1).isoformat(), } event = self.post_and_retrieve_event(event_data) @@ -543,7 +543,7 @@ def test_resolving_does_not_fail_when_no_value(self): } ] }, - "timestamp": iso_format(before_now(seconds=1)), + "timestamp": before_now(seconds=1).isoformat(), } event = self.post_and_retrieve_event(event_data) @@ -613,7 +613,7 @@ def test_resolving_does_not_fail_when_no_module_or_function(self): } ] }, - "timestamp": iso_format(before_now(seconds=1)), + "timestamp": before_now(seconds=1).isoformat(), } event = self.post_and_retrieve_event(event_data) @@ -702,7 +702,7 @@ def test_sets_inapp_after_resolving(self): } ] }, - "timestamp": iso_format(before_now(seconds=1)), + "timestamp": before_now(seconds=1).isoformat(), } event = self.post_and_retrieve_event(event_data) @@ -769,7 +769,7 @@ def test_resolving_inline(self): } ] }, - "timestamp": iso_format(before_now(seconds=1)), + "timestamp": before_now(seconds=1).isoformat(), } event = self.post_and_retrieve_event(event_data) @@ -864,7 +864,7 @@ def test_resolving_inline_with_native_frames(self): } ] }, - "timestamp": iso_format(before_now(seconds=1)), + "timestamp": before_now(seconds=1).isoformat(), } event = self.post_and_retrieve_event(event_data) @@ -970,7 +970,7 @@ def test_error_on_resolving(self): } ] }, - "timestamp": iso_format(before_now(seconds=1)), + "timestamp": before_now(seconds=1).isoformat(), } event = self.post_and_retrieve_event(event_data) @@ -1087,7 +1087,7 @@ def test_basic_source_lookup(self): } ] }, - "timestamp": iso_format(before_now(seconds=1)), + "timestamp": before_now(seconds=1).isoformat(), } event = self.post_and_retrieve_event(event_data) @@ -1405,7 +1405,7 @@ def test_source_lookup_with_proguard(self): } ] }, - "timestamp": iso_format(before_now(seconds=1)), + "timestamp": before_now(seconds=1).isoformat(), } event = self.post_and_retrieve_event(event_data) @@ -1540,7 +1540,7 @@ def test_invalid_exception(self): {"type": "RemoteException", "module": "android.os"}, ] }, - "timestamp": iso_format(before_now(seconds=1)), + "timestamp": before_now(seconds=1).isoformat(), } event = self.post_and_retrieve_event(event_data) @@ -1583,7 +1583,7 @@ def test_is_jvm_event(self): } ] }, - "timestamp": iso_format(before_now(seconds=1)), + "timestamp": before_now(seconds=1), } assert is_jvm_event(event) @@ -1612,7 +1612,7 @@ def test_is_jvm_event(self): } ] }, - "timestamp": iso_format(before_now(seconds=1)), + "timestamp": before_now(seconds=1), } # has no platform assert is_jvm_event(event) @@ -1643,7 +1643,7 @@ def test_is_jvm_event(self): } ] }, - "timestamp": iso_format(before_now(seconds=1)), + "timestamp": before_now(seconds=1), } # has no modules assert not is_jvm_event(event) diff --git a/tests/relay_integration/lang/javascript/test_example.py b/tests/relay_integration/lang/javascript/test_example.py index 199319e062ded3..469daae99c1fd8 100644 --- a/tests/relay_integration/lang/javascript/test_example.py +++ b/tests/relay_integration/lang/javascript/test_example.py @@ -5,7 +5,7 @@ from sentry.models.files.file import File from sentry.models.release import Release from sentry.models.releasefile import ReleaseFile -from sentry.testutils.helpers.datetime import before_now, iso_format +from sentry.testutils.helpers.datetime import before_now from sentry.testutils.pytest.fixtures import django_db_all from sentry.testutils.relay import RelayStoreHelper from sentry.testutils.skips import requires_kafka, requires_symbolicator @@ -71,7 +71,7 @@ def test_sourcemap_expansion(self): ) data = { - "timestamp": iso_format(before_now(minutes=1)), + "timestamp": before_now(minutes=1).isoformat(), "message": "hello", "platform": "javascript", "release": "abc", diff --git a/tests/relay_integration/lang/javascript/test_plugin.py b/tests/relay_integration/lang/javascript/test_plugin.py index aaee1124f7b4c3..137ab65785be1a 100644 --- a/tests/relay_integration/lang/javascript/test_plugin.py +++ b/tests/relay_integration/lang/javascript/test_plugin.py @@ -22,7 +22,7 @@ from sentry.models.release import Release from sentry.models.releasefile import ReleaseFile, update_artifact_index from sentry.tasks.assemble import assemble_artifacts -from sentry.testutils.helpers.datetime import before_now, iso_format +from sentry.testutils.helpers.datetime import before_now from sentry.testutils.pytest.fixtures import django_db_all from sentry.testutils.relay import RelayStoreHelper from sentry.testutils.skips import requires_kafka, requires_symbolicator @@ -111,7 +111,7 @@ def initialize(self, default_projectkey, default_project, set_sentry_option, liv self.project = default_project self.projectkey = default_projectkey self.organization = self.project.organization - self.min_ago = iso_format(before_now(minutes=1)) + self.min_ago = before_now(minutes=1).isoformat() # We disable scraping per-test when necessary. self.project.update_option("sentry:scrape_javascript", True) diff --git a/tests/relay_integration/test_integration.py b/tests/relay_integration/test_integration.py index 811f29e77218c6..ebbaaf4bf16bfb 100644 --- a/tests/relay_integration/test_integration.py +++ b/tests/relay_integration/test_integration.py @@ -7,7 +7,7 @@ from sentry.models.eventattachment import EventAttachment from sentry.tasks.relay import invalidate_project_config from sentry.testutils.cases import TransactionTestCase -from sentry.testutils.helpers.datetime import before_now, iso_format, timestamp_format +from sentry.testutils.helpers.datetime import before_now, timestamp_format from sentry.testutils.relay import RelayStoreHelper from sentry.testutils.skips import requires_kafka @@ -17,7 +17,7 @@ class SentryRemoteTest(RelayStoreHelper, TransactionTestCase): # used to be test_ungzipped_data def test_simple_data(self): - event_data = {"message": "hello", "timestamp": iso_format(before_now(seconds=1))} + event_data = {"message": "hello", "timestamp": before_now(seconds=1).isoformat()} event = self.post_and_retrieve_event(event_data) assert event.message == "hello" @@ -143,8 +143,8 @@ def test_transaction(self): "event_id": "d2132d31b39445f1938d7e21b6bf0ec4", "type": "transaction", "transaction": "/organizations/:orgId/performance/:eventSlug/", - "start_timestamp": iso_format(before_now(minutes=1, milliseconds=500)), - "timestamp": iso_format(before_now(minutes=1)), + "start_timestamp": before_now(minutes=1, milliseconds=500).isoformat(), + "timestamp": before_now(minutes=1).isoformat(), "contexts": { "trace": { "trace_id": "ff62a8b040f340bda5d830223def1d81", @@ -234,6 +234,6 @@ def test_project_config_compression(self): with mock.patch( "sentry.api.endpoints.relay.project_configs.RelayProjectConfigsEndpoint.post" ): - event_data = {"message": "hello", "timestamp": iso_format(before_now(seconds=1))} + event_data = {"message": "hello", "timestamp": before_now(seconds=1)} event = self.post_and_retrieve_event(event_data) assert event.message == "hello" diff --git a/tests/relay_integration/test_metrics_extraction.py b/tests/relay_integration/test_metrics_extraction.py index f173c9a43de735..cc578a0390c6bc 100644 --- a/tests/relay_integration/test_metrics_extraction.py +++ b/tests/relay_integration/test_metrics_extraction.py @@ -6,7 +6,7 @@ from sentry.sentry_metrics.indexer.strings import SHARED_STRINGS from sentry.testutils.cases import TransactionTestCase -from sentry.testutils.helpers.datetime import before_now, iso_format +from sentry.testutils.helpers.datetime import before_now from sentry.testutils.helpers.features import Feature from sentry.testutils.helpers.options import override_options from sentry.testutils.relay import RelayStoreHelper @@ -29,8 +29,8 @@ def test_all_transaction_metrics_emitted(self): "type": "transaction", "transaction": "foo", "transaction_info": {"source": "url"}, # 'transaction' tag not extracted - "timestamp": iso_format(before_now(seconds=1)), - "start_timestamp": iso_format(before_now(seconds=2)), + "timestamp": before_now(seconds=1), + "start_timestamp": before_now(seconds=2), "contexts": { "trace": { "trace_id": 32 * "b", @@ -62,8 +62,8 @@ def test_all_transaction_metrics_emitted(self): "op": op, "trace_id": 32 * "b", "span_id": 16 * "1", - "start_timestamp": iso_format(before_now(seconds=2)), - "timestamp": iso_format(before_now(seconds=1)), + "start_timestamp": before_now(seconds=2), + "timestamp": before_now(seconds=1), } for op in ("db", "http", "resource", "browser", "ui") ], @@ -118,8 +118,8 @@ def test_histogram_outliers(self): "type": "transaction", "transaction": "foo", "transaction_info": {"source": "url"}, # 'transaction' tag not extracted - "timestamp": iso_format(before_now(seconds=1)), - "start_timestamp": iso_format(before_now(seconds=2)), + "timestamp": before_now(seconds=1).isoformat(), + "start_timestamp": before_now(seconds=2).isoformat(), "platform": "javascript", "contexts": { "trace": { diff --git a/tests/symbolicator/test_payload_full.py b/tests/symbolicator/test_payload_full.py index 144f5c4f82e84f..b4a5f549265ce2 100644 --- a/tests/symbolicator/test_payload_full.py +++ b/tests/symbolicator/test_payload_full.py @@ -19,7 +19,7 @@ from sentry.models.files.file import File from sentry.testutils.cases import TransactionTestCase from sentry.testutils.factories import get_fixture_path -from sentry.testutils.helpers.datetime import before_now, iso_format +from sentry.testutils.helpers.datetime import before_now from sentry.testutils.relay import RelayStoreHelper from sentry.testutils.skips import requires_kafka, requires_symbolicator from sentry.utils import json @@ -73,7 +73,7 @@ } ] }, - "timestamp": iso_format(before_now(seconds=1)), + "timestamp": before_now(seconds=1).isoformat(), } @@ -90,7 +90,7 @@ class SymbolicatorResolvingIntegrationTest(RelayStoreHelper, TransactionTestCase @pytest.fixture(autouse=True) def initialize(self, live_server): self.project.update_option("sentry:builtin_symbol_sources", []) - self.min_ago = iso_format(before_now(minutes=1)) + self.min_ago = before_now(minutes=1).isoformat() with patch("sentry.auth.system.is_internal_ip", return_value=True), self.options( {"system.url-prefix": live_server.url} @@ -189,7 +189,7 @@ def test_debug_id_resolving(self): "value": "Fatal Error: EXCEPTION_ACCESS_VIOLATION_WRITE", }, "platform": "native", - "timestamp": iso_format(before_now(seconds=1)), + "timestamp": before_now(seconds=1).isoformat(), } event = self.post_and_retrieve_event(event_data) @@ -266,7 +266,7 @@ def test_resolving_with_candidates_sentry_source(self): "value": "Fatal Error: EXCEPTION_ACCESS_VIOLATION_WRITE", }, "platform": "native", - "timestamp": iso_format(before_now(seconds=1)), + "timestamp": before_now(seconds=1).isoformat(), } event = self.post_and_retrieve_event(event_data) From 265beefcf8ea45b578a79ae8fcb46dd809ec53e3 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 16 Oct 2024 11:18:59 -0700 Subject: [PATCH 30/67] ref(alerts): Don't allow 'is:unresolved' queries for dynamic alerts (#79172) When you create a dynamic alert we query snuba for 28 days of historical data for the given metric and filter (if using). If you're using the `is:unresolved` filter and then resolve or archive an issue (or issues), Seer's historical data no longer matches Sentry's because it's frozen in time from when the alert was created. This means that Seer potentially thinks the threshold for an anomaly is higher than it really is, and it may miss alerting when it should have. If we prevent using `is:unresolved` for dynamic alerts we do risk the inverse problem but the results aren't as bad - we'd prefer to err on alerting too frequently rather than not frequently enough. If the user finds it to be a problem, they can change the `sensitivity` (if it's not already as low as it can go). Another option would be to re-query Snuba and update Seer every time an issue is resolved or archived and we have a dynamic alert in the same project, but the risks with this approach are potentially missing data due to Snuba rate limits and we're not sure if Seer can keep up with this increased volume. Additionally, this may only be a problem for a small period of time in a given alert's lifecycle - presumably (need to confirm with Seer team) the initial historical data will be replaced with the subscription processor data over time. Closes https://getsentry.atlassian.net/browse/ALRT-344 --- src/sentry/incidents/logic.py | 6 ++++- .../test_organization_alert_rule_details.py | 25 +++++++++++++++++++ tests/sentry/incidents/test_logic.py | 8 +++--- 3 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/sentry/incidents/logic.py b/src/sentry/incidents/logic.py index 20b2a3a69abbf7..27c5a1bb354fcf 100644 --- a/src/sentry/incidents/logic.py +++ b/src/sentry/incidents/logic.py @@ -578,10 +578,12 @@ def create_alert_rule( resolution = time_window # NOTE: we hardcode seasonality for EA seasonality = AlertRuleSeasonality.AUTO - if not (sensitivity): + if not sensitivity: raise ValidationError("Dynamic alerts require a sensitivity level") if time_window not in DYNAMIC_TIME_WINDOWS: raise ValidationError(INVALID_TIME_WINDOW) + if "is:unresolved" in query: + raise ValidationError("Dynamic alerts do not support 'is:unresolved' queries") else: resolution = get_alert_resolution(time_window, organization) seasonality = None @@ -917,6 +919,8 @@ def update_alert_rule( raise ResourceDoesNotExist( "Your organization does not have access to this feature." ) + if query and "is:unresolved" in query: + raise ValidationError("Dynamic alerts do not support 'is:unresolved' queries") # NOTE: if adding a new metric alert type, take care to check that it's handled here project = projects[0] if projects else alert_rule.projects.get() update_rule_data(alert_rule, project, snuba_query, updated_fields, updated_query_fields) diff --git a/tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py b/tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py index 906774313c63bb..e17b2b9bb2cf99 100644 --- a/tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py +++ b/tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py @@ -372,6 +372,18 @@ def test_dynamic_detection_type(self, mock_seer_request): time_window=1, ) + with pytest.raises( + ValidationError, match="Dynamic alerts do not support 'is:unresolved' queries" + ): + rule = self.create_alert_rule( + seasonality=AlertRuleSeasonality.AUTO, + sensitivity=AlertRuleSensitivity.HIGH, + threshold_type=AlertRuleThresholdType.ABOVE_AND_BELOW, + detection_type=AlertRuleDetectionType.DYNAMIC, + time_window=30, + query="is:unresolved", + ) + @with_feature("organizations:anomaly-detection-alerts") @with_feature("organizations:anomaly-detection-rollout") @with_feature("organizations:incidents") @@ -931,6 +943,19 @@ def test_anomaly_detection_alert_update_validation_error(self, mock_seer_request # We don't call send_historical_data_to_seer if we encounter a validation error. assert mock_seer_request.call_count == 0 + data2 = self.get_serialized_alert_rule() + data2["query"] = "is:unresolved" + + resp = self.get_error_response( + self.organization.slug, + alert_rule.id, + status_code=400, + **data2, + ) + assert resp.data[0] == "Dynamic alerts do not support 'is:unresolved' queries" + # We don't call send_historical_data_to_seer if we encounter a validation error. + assert mock_seer_request.call_count == 0 + def test_delete_action(self): self.create_member( user=self.user, organization=self.organization, role="owner", teams=[self.team] diff --git a/tests/sentry/incidents/test_logic.py b/tests/sentry/incidents/test_logic.py index 399c5f2607c80f..2985b592c7cd22 100644 --- a/tests/sentry/incidents/test_logic.py +++ b/tests/sentry/incidents/test_logic.py @@ -1703,12 +1703,12 @@ def test_update_dynamic_alerts(self, mock_seer_request): # update query update_alert_rule( dynamic_rule, - query="is:unresolved", + query="message:*post_process*", detection_type=AlertRuleDetectionType.DYNAMIC, ) assert mock_seer_request.call_count == 1 snuba_query.refresh_from_db() - assert snuba_query.query == "is:unresolved" + assert snuba_query.query == "message:*post_process*" mock_seer_request.reset_mock() # update aggregate update_alert_rule( @@ -1962,7 +1962,7 @@ def test_update_alert_rule_anomaly_detection_seer_timeout_max_retry( update_alert_rule( dynamic_rule, time_window=30, - query="is:unresolved", + query="message:*post_process*", detection_type=AlertRuleDetectionType.DYNAMIC, sensitivity=AlertRuleSensitivity.HIGH, seasonality=AlertRuleSeasonality.AUTO, @@ -1982,7 +1982,7 @@ def test_update_alert_rule_anomaly_detection_seer_timeout_max_retry( update_alert_rule( dynamic_rule, time_window=30, - query="is:unresolved", + query="message:*post_process*", detection_type=AlertRuleDetectionType.DYNAMIC, sensitivity=AlertRuleSensitivity.HIGH, seasonality=AlertRuleSeasonality.AUTO, From b923a6faefb1d5848aef3720aa097310870f1d9b Mon Sep 17 00:00:00 2001 From: Catherine Lee <55311782+c298lee@users.noreply.github.com> Date: Wed, 16 Oct 2024 14:29:33 -0400 Subject: [PATCH 31/67] chore(replay): Remove a11y from replays (#79155) Removes all things a11y from replays Relates to https://github.com/getsentry/team-replay/issues/485 --- .../utils/analytics/replayAnalyticsEvents.tsx | 5 - .../app/utils/replays/hooks/useA11yData.tsx | 38 --- .../replays/hooks/useActiveReplayTab.tsx | 1 - static/app/utils/replays/hydrateA11yFrame.tsx | 86 ------- static/app/utils/replays/types.tsx | 4 +- .../accessibility/accessibilityFilters.tsx | 56 ---- .../accessibility/accessibilityHeaderCell.tsx | 52 ---- .../accessibilityRefetchBanner.tsx | 79 ------ .../accessibility/accessibilityTableCell.tsx | 155 ----------- .../accessibility/details/components.tsx | 119 --------- .../detail/accessibility/details/content.tsx | 28 -- .../detail/accessibility/details/index.tsx | 38 --- .../detail/accessibility/details/sections.tsx | 47 ---- .../replays/detail/accessibility/index.tsx | 242 ------------------ .../accessibility/useAccessibilityFilters.tsx | 144 ----------- .../accessibility/useSortAccessibility.tsx | 112 -------- .../views/replays/detail/layout/focusArea.tsx | 3 - .../views/replays/detail/layout/focusTabs.tsx | 36 +-- 18 files changed, 2 insertions(+), 1243 deletions(-) delete mode 100644 static/app/utils/replays/hooks/useA11yData.tsx delete mode 100644 static/app/utils/replays/hydrateA11yFrame.tsx delete mode 100644 static/app/views/replays/detail/accessibility/accessibilityFilters.tsx delete mode 100644 static/app/views/replays/detail/accessibility/accessibilityHeaderCell.tsx delete mode 100644 static/app/views/replays/detail/accessibility/accessibilityRefetchBanner.tsx delete mode 100644 static/app/views/replays/detail/accessibility/accessibilityTableCell.tsx delete mode 100644 static/app/views/replays/detail/accessibility/details/components.tsx delete mode 100644 static/app/views/replays/detail/accessibility/details/content.tsx delete mode 100644 static/app/views/replays/detail/accessibility/details/index.tsx delete mode 100644 static/app/views/replays/detail/accessibility/details/sections.tsx delete mode 100644 static/app/views/replays/detail/accessibility/index.tsx delete mode 100644 static/app/views/replays/detail/accessibility/useAccessibilityFilters.tsx delete mode 100644 static/app/views/replays/detail/accessibility/useSortAccessibility.tsx diff --git a/static/app/utils/analytics/replayAnalyticsEvents.tsx b/static/app/utils/analytics/replayAnalyticsEvents.tsx index 49b6864e628caa..5656b41779f069 100644 --- a/static/app/utils/analytics/replayAnalyticsEvents.tsx +++ b/static/app/utils/analytics/replayAnalyticsEvents.tsx @@ -3,10 +3,6 @@ import type {Output} from 'sentry/views/replays/detail/network/details/getOutput import type {ReferrerTableType} from 'sentry/views/replays/replayTable/tableCell'; export type ReplayEventParameters = { - 'replay.accessibility-issue-clicked': { - issue_description: string; - issue_impact: string | undefined; - }; 'replay.canvas-detected-banner-clicked': { sdk_needs_update?: boolean; }; @@ -132,7 +128,6 @@ export type ReplayEventParameters = { export type ReplayEventKey = keyof ReplayEventParameters; export const replayEventMap: Record = { - 'replay.accessibility-issue-clicked': 'Clicked Replay Accessibility Issue', 'replay.canvas-detected-banner-clicked': 'Clicked Canvas Detected in Replay Banner', 'replay.details-data-loaded': 'Replay Details Data Loaded', 'replay.details-has-hydration-error': 'Replay Details Has Hydration Error', diff --git a/static/app/utils/replays/hooks/useA11yData.tsx b/static/app/utils/replays/hooks/useA11yData.tsx deleted file mode 100644 index 8da1f6883a5b93..00000000000000 --- a/static/app/utils/replays/hooks/useA11yData.tsx +++ /dev/null @@ -1,38 +0,0 @@ -import {useMemo} from 'react'; - -import {useReplayContext} from 'sentry/components/replays/replayContext'; -import {useQuery} from 'sentry/utils/queryClient'; -import type {RawA11yResponse} from 'sentry/utils/replays/hydrateA11yFrame'; -import hydrateA11yFrame from 'sentry/utils/replays/hydrateA11yFrame'; -import useApi from 'sentry/utils/useApi'; -import useOrganization from 'sentry/utils/useOrganization'; -import useProjects from 'sentry/utils/useProjects'; - -export default function useA11yData() { - const api = useApi(); - const organization = useOrganization(); - const {currentTime, replay} = useReplayContext(); - const {projects} = useProjects(); - const replayRecord = replay?.getReplay(); - const startTimestampMs = replayRecord?.started_at.getTime(); - const project = projects.find(p => p.id === replayRecord?.project_id); - const unixTimestamp = ((startTimestampMs || 0) + currentTime) / 1000; - const {data, ...rest} = useQuery({ - queryKey: [ - `/projects/${organization.slug}/${project?.slug}/replays/${replayRecord?.id}/accessibility-issues/`, - ], - queryFn: ({queryKey: [url]}) => - api.requestPromise(String(url), { - method: 'GET', - query: {timestamp: unixTimestamp}, - }), - staleTime: 0, - enabled: Boolean(project) && Boolean(replayRecord), - }); - - const hydrated = useMemo( - () => data?.data?.flatMap(record => hydrateA11yFrame(record, startTimestampMs ?? 0)), - [data?.data, startTimestampMs] - ); - return {data: hydrated, dataOffsetMs: currentTime, ...rest}; -} diff --git a/static/app/utils/replays/hooks/useActiveReplayTab.tsx b/static/app/utils/replays/hooks/useActiveReplayTab.tsx index f0b4feb611eb56..241c95bfd6bb28 100644 --- a/static/app/utils/replays/hooks/useActiveReplayTab.tsx +++ b/static/app/utils/replays/hooks/useActiveReplayTab.tsx @@ -3,7 +3,6 @@ import {useCallback} from 'react'; import useUrlParams from 'sentry/utils/useUrlParams'; export enum TabKey { - A11Y = 'a11y', BREADCRUMBS = 'breadcrumbs', CONSOLE = 'console', ERRORS = 'errors', diff --git a/static/app/utils/replays/hydrateA11yFrame.tsx b/static/app/utils/replays/hydrateA11yFrame.tsx deleted file mode 100644 index 214d42a42e9edc..00000000000000 --- a/static/app/utils/replays/hydrateA11yFrame.tsx +++ /dev/null @@ -1,86 +0,0 @@ -export interface RawA11yResponse { - data: RawA11yFrame[]; -} - -export interface RawA11yFrame { - elements: A11yIssueElement[]; - help: string; - help_url: string; - id: string; - timestamp: number; - impact?: 'minor' | 'moderate' | 'serious' | 'critical'; -} - -interface A11yIssueElement { - alternatives: A11yIssueElementAlternative[]; - element: string; - target: string[]; -} - -interface A11yIssueElementAlternative { - id: string; - message: string; -} - -type Overwrite = Pick> & U; - -export type HydratedA11yFrame = Overwrite< - Omit, - { - /** - * For compatibility with Frames, to highlight the element within the replay - */ - data: { - element: A11yIssueElement; - label: string; - }; - /** - * Rename `help` to conform to ReplayFrame basics. - */ - description: string; - /** - * The specific element instance - */ - element: A11yIssueElement; - /** - * The difference in timestamp and replay.started_at, in millieseconds - */ - offsetMs: number; - /** - * The Date when the a11yIssue happened - */ - timestamp: Date; - /** - * Alias of timestamp, in milliseconds - */ - timestampMs: number; - } ->; - -export default function hydrateA11yFrame( - raw: RawA11yFrame, - startTimestampMs: number -): HydratedA11yFrame[] { - return raw.elements.map((element): HydratedA11yFrame => { - const timestamp = new Date(raw.timestamp); - const timestampMs = timestamp.getTime(); - const elementWithoutIframe = { - ...element, - target: element.target[0] === 'iframe' ? element.target.slice(1) : element.target, - }; - return { - data: { - element: elementWithoutIframe, - label: raw.id, - }, - description: raw.help, - element: elementWithoutIframe, - help_url: raw.help_url, - id: raw.id, - impact: raw.impact, - offsetMs: timestampMs - startTimestampMs, - timestamp, - timestampMs, - }; - }); -} diff --git a/static/app/utils/replays/types.tsx b/static/app/utils/replays/types.tsx index 2c004272f2bd83..2e23e4c542cc7f 100644 --- a/static/app/utils/replays/types.tsx +++ b/static/app/utils/replays/types.tsx @@ -15,8 +15,6 @@ import type { } from '@sentry/react'; import invariant from 'invariant'; -import type {HydratedA11yFrame} from 'sentry/utils/replays/hydrateA11yFrame'; - export type Dimensions = { height: number; width: number; @@ -444,7 +442,7 @@ export type ErrorFrame = Overwrite< } >; -export type ReplayFrame = BreadcrumbFrame | ErrorFrame | SpanFrame | HydratedA11yFrame; +export type ReplayFrame = BreadcrumbFrame | ErrorFrame | SpanFrame; interface VideoFrame { container: string; diff --git a/static/app/views/replays/detail/accessibility/accessibilityFilters.tsx b/static/app/views/replays/detail/accessibility/accessibilityFilters.tsx deleted file mode 100644 index 78acd48707ed95..00000000000000 --- a/static/app/views/replays/detail/accessibility/accessibilityFilters.tsx +++ /dev/null @@ -1,56 +0,0 @@ -import type {SelectOption} from 'sentry/components/compactSelect'; -import {CompactSelect} from 'sentry/components/compactSelect'; -import SearchBar from 'sentry/components/searchBar'; -import {t} from 'sentry/locale'; -import type useAccessibilityFilters from 'sentry/views/replays/detail/accessibility/useAccessibilityFilters'; -import FiltersGrid from 'sentry/views/replays/detail/filtersGrid'; - -type Props = { - accessibilityData: undefined | unknown[]; -} & ReturnType; - -function AccessibilityFilters({ - getImpactLevels, - getIssueTypes, - accessibilityData, - searchTerm, - selectValue, - setFilters, - setSearchTerm, -}: Props) { - const impactLevels = getImpactLevels(); - const issueTypes = getIssueTypes(); - - return ( - - []) => void} - options={[ - { - label: t('Impact Level'), - options: impactLevels, - }, - { - label: t('Type'), - options: issueTypes, - }, - ]} - size="sm" - triggerLabel={selectValue?.length === 0 ? t('Any') : null} - triggerProps={{prefix: t('Filter')}} - value={selectValue} - /> - - - ); -} - -export default AccessibilityFilters; diff --git a/static/app/views/replays/detail/accessibility/accessibilityHeaderCell.tsx b/static/app/views/replays/detail/accessibility/accessibilityHeaderCell.tsx deleted file mode 100644 index 52c2ff445231a5..00000000000000 --- a/static/app/views/replays/detail/accessibility/accessibilityHeaderCell.tsx +++ /dev/null @@ -1,52 +0,0 @@ -import type {ComponentProps, CSSProperties, ReactNode} from 'react'; -import {forwardRef} from 'react'; - -import HeaderCell from 'sentry/components/replays/virtualizedGrid/headerCell'; -import type {Tooltip} from 'sentry/components/tooltip'; -import {t} from 'sentry/locale'; -import type useSortAccessibility from 'sentry/views/replays/detail/accessibility/useSortAccessibility'; - -type SortConfig = ReturnType['sortConfig']; -type Props = { - handleSort: ReturnType['handleSort']; - index: number; - sortConfig: SortConfig; - style: CSSProperties; -}; - -const COLUMNS: { - field: SortConfig['by']; - label: ReactNode; - tooltipTitle?: ComponentProps['title']; -}[] = [ - { - field: 'impact', - label: '', - }, - { - field: 'id', - label: t('Type'), - }, - {field: 'element', label: t('Element')}, -]; - -export const COLUMN_COUNT = COLUMNS.length; - -const AccessibilityHeaderCell = forwardRef( - ({handleSort, index, sortConfig, style}: Props, ref) => { - const {field, label, tooltipTitle} = COLUMNS[index]; - return ( - - ); - } -); - -export default AccessibilityHeaderCell; diff --git a/static/app/views/replays/detail/accessibility/accessibilityRefetchBanner.tsx b/static/app/views/replays/detail/accessibility/accessibilityRefetchBanner.tsx deleted file mode 100644 index 519f61016e1dfa..00000000000000 --- a/static/app/views/replays/detail/accessibility/accessibilityRefetchBanner.tsx +++ /dev/null @@ -1,79 +0,0 @@ -import {useCallback, useState} from 'react'; -import styled from '@emotion/styled'; - -import {Button} from 'sentry/components/button'; -import {Flex} from 'sentry/components/container/flex'; -import {useReplayContext} from 'sentry/components/replays/replayContext'; -import Well from 'sentry/components/well'; -import {t, tct} from 'sentry/locale'; -import {space} from 'sentry/styles/space'; -import formatDuration from 'sentry/utils/duration/formatDuration'; -import TimestampButton from 'sentry/views/replays/detail/timestampButton'; - -interface Props { - initialOffsetMs: number; - refetch: () => void; -} - -export default function AccessibilityRefetchBanner({initialOffsetMs, refetch}: Props) { - const {currentTime, replay, setCurrentTime, isPlaying, togglePlayPause} = - useReplayContext(); - - const startTimestampMs = replay?.getReplay()?.started_at?.getTime() ?? 0; - const [lastOffsetMs, setLastOffsetMs] = useState(initialOffsetMs); - - const handleClickRefetch = useCallback(() => { - togglePlayPause(false); - setLastOffsetMs(currentTime); - refetch(); - }, [currentTime, refetch, togglePlayPause]); - - const handleClickTimestamp = useCallback(() => { - setCurrentTime(lastOffsetMs); - }, [setCurrentTime, lastOffsetMs]); - - const now = formatDuration({ - duration: [currentTime, 'ms'], - precision: 'sec', - style: 'hh:mm:ss', - }); - return ( - - - - {tct('Results as of [lastRuntime]', { - lastRuntime: ( - - ), - })} - - - - - ); -} - -const StyledWell = styled(Well)` - margin-bottom: 0; - border-radius: ${p => p.theme.borderRadiusTop}; -`; - -const StyledTimestampButton = styled(TimestampButton)` - align-self: center; - align-items: center; -`; diff --git a/static/app/views/replays/detail/accessibility/accessibilityTableCell.tsx b/static/app/views/replays/detail/accessibility/accessibilityTableCell.tsx deleted file mode 100644 index f7888f6140fa2c..00000000000000 --- a/static/app/views/replays/detail/accessibility/accessibilityTableCell.tsx +++ /dev/null @@ -1,155 +0,0 @@ -import type {ComponentProps, CSSProperties, ReactNode} from 'react'; -import {forwardRef} from 'react'; -import styled from '@emotion/styled'; -import classNames from 'classnames'; - -import {Cell, Text} from 'sentry/components/replays/virtualizedGrid/bodyCell'; -import TextOverflow from 'sentry/components/textOverflow'; -import {Tooltip} from 'sentry/components/tooltip'; -import {IconFire, IconInfo, IconWarning} from 'sentry/icons'; -import {space} from 'sentry/styles/space'; -import type useCrumbHandlers from 'sentry/utils/replays/hooks/useCrumbHandlers'; -import type {HydratedA11yFrame} from 'sentry/utils/replays/hydrateA11yFrame'; -import useUrlParams from 'sentry/utils/useUrlParams'; -import type useSortAccessibility from 'sentry/views/replays/detail/accessibility/useSortAccessibility'; - -const EMPTY_CELL = '--'; - -interface Props extends ReturnType { - a11yIssue: HydratedA11yFrame; - columnIndex: number; - currentHoverTime: number | undefined; - currentTime: number; - onClickCell: (props: {dataIndex: number; rowIndex: number}) => void; - rowIndex: number; - sortConfig: ReturnType['sortConfig']; - style: CSSProperties; -} - -const AccessibilityTableCell = forwardRef( - ( - { - a11yIssue, - columnIndex, - currentHoverTime, - currentTime, - onClickCell, - onMouseEnter, - onMouseLeave, - rowIndex, - sortConfig, - style, - }: Props, - ref - ) => { - // Rows include the sortable header, the dataIndex does not - const dataIndex = rowIndex - 1; - - const {getParamValue} = useUrlParams('a_detail_row', ''); - const isSelected = getParamValue() === String(dataIndex); - - const IMPACT_ICON_MAPPING: Record = { - minor: , - moderate: , - serious: , - critical: , - }; - - const hasOccurred = currentTime >= a11yIssue.offsetMs; - const isBeforeHover = - currentHoverTime === undefined || currentHoverTime >= a11yIssue.offsetMs; - - const isByTimestamp = sortConfig.by === 'timestampMs'; - const isAsc = isByTimestamp ? sortConfig.asc : undefined; - const columnProps = { - className: classNames({ - beforeCurrentTime: isByTimestamp - ? isAsc - ? hasOccurred - : !hasOccurred - : undefined, - afterCurrentTime: isByTimestamp - ? isAsc - ? !hasOccurred - : hasOccurred - : undefined, - beforeHoverTime: - isByTimestamp && currentHoverTime !== undefined - ? isAsc - ? isBeforeHover - : !isBeforeHover - : undefined, - afterHoverTime: - isByTimestamp && currentHoverTime !== undefined - ? isAsc - ? !isBeforeHover - : isBeforeHover - : undefined, - }), - hasOccurred: isByTimestamp ? hasOccurred : undefined, - isSelected, - onClick: () => onClickCell({dataIndex, rowIndex}), - onMouseEnter: () => onMouseEnter(a11yIssue), - onMouseLeave: () => onMouseLeave(a11yIssue), - ref, - style, - } as ComponentProps; - - const renderFns = [ - () => ( - - - {a11yIssue.impact ? ( - - {IMPACT_ICON_MAPPING[a11yIssue.impact]} - - ) : ( - EMPTY_CELL - )} - - - ), - () => ( - - {a11yIssue.id ?? EMPTY_CELL} - - ), - () => ( - - - - {a11yIssue.element.element ?? EMPTY_CELL} - - - - ), - ]; - - return renderFns[columnIndex](); - } -); - -export default AccessibilityTableCell; - -const StyledTextOverflow = styled(TextOverflow)` - padding-right: ${space(1)}; -`; - -const StyledCell = styled(Cell)<{ - impact: HydratedA11yFrame['impact']; - isRowSelected: boolean; -}>` - background: ${p => - p.isSelected - ? p.theme.purple300 - : p.impact === 'serious' - ? p.theme.yellow100 - : p.impact === 'critical' - ? p.theme.red100 - : 'transparent'}; -`; diff --git a/static/app/views/replays/detail/accessibility/details/components.tsx b/static/app/views/replays/detail/accessibility/details/components.tsx deleted file mode 100644 index 1438d368f34217..00000000000000 --- a/static/app/views/replays/detail/accessibility/details/components.tsx +++ /dev/null @@ -1,119 +0,0 @@ -import type {ReactNode} from 'react'; -import {Fragment, useState} from 'react'; -import styled from '@emotion/styled'; - -import {KeyValueTable, KeyValueTableRow} from 'sentry/components/keyValueTable'; -import {IconChevron} from 'sentry/icons'; -import {t} from 'sentry/locale'; -import {space} from 'sentry/styles/space'; - -export const Indent = styled('div')` - padding-left: ${space(4)}; -`; - -const NotFoundText = styled('span')` - color: ${p => p.theme.subText}; - font-size: ${p => p.theme.fontSizeSmall}; -`; - -export type KeyValueTuple = { - key: string; - value: string | ReactNode; - type?: 'warning' | 'error'; -}; - -export function keyValueTableOrNotFound(data: KeyValueTuple[], notFoundText: string) { - return data.length ? ( - - {data.map(({key, value, type}) => ( - {value}} - /> - ))} - - ) : ( - - {notFoundText} - - ); -} - -const ValueContainer = styled('span')` - overflow: auto; -`; - -const SectionTitle = styled('dt')``; - -const SectionTitleExtra = styled('span')` - flex-grow: 1; - text-align: right; - font-weight: ${p => p.theme.fontWeightNormal}; -`; - -const SectionData = styled('dd')` - font-size: ${p => p.theme.fontSizeExtraSmall}; -`; - -const ToggleButton = styled('button')` - background: ${p => p.theme.background}; - border: 0; - color: ${p => p.theme.headingColor}; - font-size: ${p => p.theme.fontSizeSmall}; - font-weight: ${p => p.theme.fontWeightBold}; - line-height: ${p => p.theme.text.lineHeightBody}; - - width: 100%; - display: flex; - align-items: center; - justify-content: flex-start; - gap: ${space(1)}; - - padding: ${space(0.5)} ${space(1)}; - - :hover { - background: ${p => p.theme.backgroundSecondary}; - } -`; - -export function SectionItem({ - children, - title, - titleExtra, -}: { - children: ReactNode; - title: ReactNode; - titleExtra?: ReactNode; -}) { - const [isOpen, setIsOpen] = useState(true); - - return ( - - - setIsOpen(!isOpen)}> - - {title} - {titleExtra ? {titleExtra} : null} - - - {isOpen ? children : null} - - ); -} - -const StyledKeyValueTable = styled(KeyValueTable)` - & > dt { - font-size: ${p => p.theme.fontSizeSmall}; - padding-left: ${space(4)}; - } - & > dd { - ${p => p.theme.overflowEllipsis}; - font-size: ${p => p.theme.fontSizeSmall}; - display: flex; - justify-content: flex-end; - white-space: normal; - text-align: right; - } -`; diff --git a/static/app/views/replays/detail/accessibility/details/content.tsx b/static/app/views/replays/detail/accessibility/details/content.tsx deleted file mode 100644 index 2702d05c473345..00000000000000 --- a/static/app/views/replays/detail/accessibility/details/content.tsx +++ /dev/null @@ -1,28 +0,0 @@ -import styled from '@emotion/styled'; - -import type {SectionProps} from 'sentry/views/replays/detail/accessibility/details/sections'; -import { - ElementSection, - GeneralSection, -} from 'sentry/views/replays/detail/accessibility/details/sections'; -import FluidHeight from 'sentry/views/replays/detail/layout/fluidHeight'; - -type Props = SectionProps; - -export default function AccessibilityDetailsContent(props: Props) { - return ( - - - - - - - ); -} - -const OverflowFluidHeight = styled(FluidHeight)` - overflow: auto; -`; -const SectionList = styled('dl')` - margin: 0; -`; diff --git a/static/app/views/replays/detail/accessibility/details/index.tsx b/static/app/views/replays/detail/accessibility/details/index.tsx deleted file mode 100644 index 3563a53bc7de75..00000000000000 --- a/static/app/views/replays/detail/accessibility/details/index.tsx +++ /dev/null @@ -1,38 +0,0 @@ -import {Fragment} from 'react'; - -import DetailsSplitDivider from 'sentry/components/replays/virtualizedGrid/detailsSplitDivider'; -import type {HydratedA11yFrame} from 'sentry/utils/replays/hydrateA11yFrame'; -import type {useResizableDrawer} from 'sentry/utils/useResizableDrawer'; -import AccessibilityDetailsContent from 'sentry/views/replays/detail/accessibility/details/content'; - -type Props = { - item: null | HydratedA11yFrame; - onClose: () => void; -} & Omit, 'size'>; - -function AccessibilityDetails({ - isHeld, - item, - onClose, - onDoubleClick, - onMouseDown, -}: Props) { - if (!item) { - return null; - } - - return ( - - - - - - ); -} - -export default AccessibilityDetails; diff --git a/static/app/views/replays/detail/accessibility/details/sections.tsx b/static/app/views/replays/detail/accessibility/details/sections.tsx deleted file mode 100644 index 1bbd12812a965b..00000000000000 --- a/static/app/views/replays/detail/accessibility/details/sections.tsx +++ /dev/null @@ -1,47 +0,0 @@ -import beautify from 'js-beautify'; - -import {CodeSnippet} from 'sentry/components/codeSnippet'; -import ExternalLink from 'sentry/components/links/externalLink'; -import {t} from 'sentry/locale'; -import type {HydratedA11yFrame} from 'sentry/utils/replays/hydrateA11yFrame'; -import type {KeyValueTuple} from 'sentry/views/replays/detail/accessibility/details/components'; -import { - keyValueTableOrNotFound, - SectionItem, -} from 'sentry/views/replays/detail/accessibility/details/components'; - -export type SectionProps = { - item: HydratedA11yFrame; -}; - -export function ElementSection({item}: SectionProps) { - return ( - - - {beautify.html(item.element.element, {indent_size: 2})} - - - ); -} - -export function GeneralSection({item}: SectionProps) { - const data: KeyValueTuple[] = [ - { - key: t('Impact'), - value: item.impact, - type: item.impact === 'critical' ? 'warning' : undefined, - }, - {key: t('Type'), value: item.id}, - { - key: t('Help'), - value: {item.description}, - }, - {key: t('Path'), value: item.element.target.join(' ')}, - ]; - - return ( - - {keyValueTableOrNotFound(data, t('Missing details'))} - - ); -} diff --git a/static/app/views/replays/detail/accessibility/index.tsx b/static/app/views/replays/detail/accessibility/index.tsx deleted file mode 100644 index 39817e80102a33..00000000000000 --- a/static/app/views/replays/detail/accessibility/index.tsx +++ /dev/null @@ -1,242 +0,0 @@ -import {useCallback, useEffect, useMemo, useRef, useState} from 'react'; -import type {GridCellProps} from 'react-virtualized'; -import {AutoSizer, CellMeasurer, MultiGrid} from 'react-virtualized'; -import styled from '@emotion/styled'; - -import Placeholder from 'sentry/components/placeholder'; -import JumpButtons from 'sentry/components/replays/jumpButtons'; -import {useReplayContext} from 'sentry/components/replays/replayContext'; -import useJumpButtons from 'sentry/components/replays/useJumpButtons'; -import {GridTable} from 'sentry/components/replays/virtualizedGrid/gridTable'; -import {OverflowHidden} from 'sentry/components/replays/virtualizedGrid/overflowHidden'; -import {SplitPanel} from 'sentry/components/replays/virtualizedGrid/splitPanel'; -import useDetailsSplit from 'sentry/components/replays/virtualizedGrid/useDetailsSplit'; -import {t} from 'sentry/locale'; -import {trackAnalytics} from 'sentry/utils/analytics'; -import useA11yData from 'sentry/utils/replays/hooks/useA11yData'; -import useCrumbHandlers from 'sentry/utils/replays/hooks/useCrumbHandlers'; -import useCurrentHoverTime from 'sentry/utils/replays/playback/providers/useCurrentHoverTime'; -import useOrganization from 'sentry/utils/useOrganization'; -import AccessibilityFilters from 'sentry/views/replays/detail/accessibility/accessibilityFilters'; -import AccessibilityHeaderCell, { - COLUMN_COUNT, -} from 'sentry/views/replays/detail/accessibility/accessibilityHeaderCell'; -import AccessibilityRefetchBanner from 'sentry/views/replays/detail/accessibility/accessibilityRefetchBanner'; -import AccessibilityTableCell from 'sentry/views/replays/detail/accessibility/accessibilityTableCell'; -import AccessibilityDetails from 'sentry/views/replays/detail/accessibility/details'; -import useAccessibilityFilters from 'sentry/views/replays/detail/accessibility/useAccessibilityFilters'; -import useSortAccessibility from 'sentry/views/replays/detail/accessibility/useSortAccessibility'; -import FilterLoadingIndicator from 'sentry/views/replays/detail/filterLoadingIndicator'; -import FluidHeight from 'sentry/views/replays/detail/layout/fluidHeight'; -import NoRowRenderer from 'sentry/views/replays/detail/noRowRenderer'; -import useVirtualizedGrid from 'sentry/views/replays/detail/useVirtualizedGrid'; - -const HEADER_HEIGHT = 25; -const BODY_HEIGHT = 25; - -const RESIZEABLE_HANDLE_HEIGHT = 105; - -const cellMeasurer = { - defaultHeight: BODY_HEIGHT, - defaultWidth: 100, - fixedHeight: true, -}; - -function AccessibilityList() { - const organization = useOrganization(); - const {currentTime} = useReplayContext(); - const [currentHoverTime] = useCurrentHoverTime(); - const {onMouseEnter, onMouseLeave, onClickTimestamp} = useCrumbHandlers(); - - const { - dataOffsetMs, - data: accessibilityData, - isPending, - isRefetching, - refetch, - } = useA11yData(); - - const [scrollToRow, setScrollToRow] = useState(undefined); - - const filterProps = useAccessibilityFilters({ - accessibilityData: accessibilityData || [], - }); - const {items: filteredItems, searchTerm, setSearchTerm} = filterProps; - const clearSearchTerm = () => setSearchTerm(''); - const {handleSort, items, sortConfig} = useSortAccessibility({items: filteredItems}); - - const containerRef = useRef(null); - const gridRef = useRef(null); - const deps = useMemo(() => [items, searchTerm], [items, searchTerm]); - const {cache, getColumnWidth, onScrollbarPresenceChange, onWrapperResize} = - useVirtualizedGrid({ - cellMeasurer, - gridRef, - columnCount: COLUMN_COUNT, - dynamicColumnIndex: 2, - deps, - }); - - const { - onClickCell, - onCloseDetailsSplit, - resizableDrawerProps, - selectedIndex, - splitSize, - } = useDetailsSplit({ - containerRef, - handleHeight: RESIZEABLE_HANDLE_HEIGHT, - frames: accessibilityData, - urlParamName: 'a_detail_row', - onShowDetails: useCallback( - ({dataIndex, rowIndex}) => { - setScrollToRow(rowIndex); - - const item = items[dataIndex]; - trackAnalytics('replay.accessibility-issue-clicked', { - organization, - issue_description: item.description, - issue_impact: item.impact, - }); - }, - [items, organization] - ), - }); - - useEffect(() => { - if (isRefetching) { - onCloseDetailsSplit(); - } - }, [isRefetching, onCloseDetailsSplit]); - - const { - handleClick: onClickToJump, - onSectionRendered, - showJumpDownButton, - showJumpUpButton, - } = useJumpButtons({ - currentTime, - frames: filteredItems, - isTable: true, - setScrollToRow, - }); - - const cellRenderer = ({columnIndex, rowIndex, key, style, parent}: GridCellProps) => { - const a11yIssue = items[rowIndex - 1]; - - return ( - - {({measure: _, registerChild}) => - rowIndex === 0 ? ( - e && registerChild?.(e)} - handleSort={handleSort} - index={columnIndex} - sortConfig={sortConfig} - style={{...style, height: HEADER_HEIGHT}} - /> - ) : ( - e && registerChild?.(e)} - rowIndex={rowIndex} - sortConfig={sortConfig} - style={{...style, height: BODY_HEIGHT}} - /> - ) - } - - ); - }; - - return ( - - - - - - - - {accessibilityData && !isRefetching ? ( - - - {({height, width}) => ( - ( - - {t('No accessibility problems detected')} - - )} - onScrollbarPresenceChange={onScrollbarPresenceChange} - onScroll={() => { - if (scrollToRow !== undefined) { - setScrollToRow(undefined); - } - }} - onSectionRendered={onSectionRendered} - overscanColumnCount={COLUMN_COUNT} - overscanRowCount={5} - rowCount={items.length + 1} - rowHeight={({index}) => (index === 0 ? HEADER_HEIGHT : BODY_HEIGHT)} - scrollToRow={scrollToRow} - width={width} - /> - )} - - {sortConfig.by === 'timestamp' && items.length ? ( - - ) : null} - - ) : ( - - )} - - - - - ); -} - -const StyledGridTable = styled(GridTable)` - border-radius: ${p => p.theme.borderRadiusBottom}; - border-top: none; -`; - -export default AccessibilityList; diff --git a/static/app/views/replays/detail/accessibility/useAccessibilityFilters.tsx b/static/app/views/replays/detail/accessibility/useAccessibilityFilters.tsx deleted file mode 100644 index b59c3dab09da09..00000000000000 --- a/static/app/views/replays/detail/accessibility/useAccessibilityFilters.tsx +++ /dev/null @@ -1,144 +0,0 @@ -import {useCallback, useMemo} from 'react'; - -import type {SelectOption} from 'sentry/components/compactSelect'; -import {decodeList, decodeScalar} from 'sentry/utils/queryString'; -import useFiltersInLocationQuery from 'sentry/utils/replays/hooks/useFiltersInLocationQuery'; -import type {HydratedA11yFrame} from 'sentry/utils/replays/hydrateA11yFrame'; -import {filterItems} from 'sentry/views/replays/detail/utils'; - -export interface AccessibilitySelectOption extends SelectOption { - qs: 'f_a_impact' | 'f_a_type'; -} - -const DEFAULT_FILTERS = { - f_a_impact: [], - f_a_type: [], -} as Record; - -export type FilterFields = { - f_a_impact: string[]; - f_a_search: string; - f_a_type: string[]; - a_detail_row?: string; -}; - -type Options = { - accessibilityData: HydratedA11yFrame[]; -}; - -type Return = { - getImpactLevels: () => AccessibilitySelectOption[]; - getIssueTypes: () => AccessibilitySelectOption[]; - items: HydratedA11yFrame[]; - searchTerm: string; - selectValue: string[]; - setFilters: (val: AccessibilitySelectOption[]) => void; - setSearchTerm: (searchTerm: string) => void; -}; - -const FILTERS = { - impact: (item: HydratedA11yFrame, impacts: string[]) => - impacts.length === 0 || impacts.includes(item.impact ?? ''), - type: (item: HydratedA11yFrame, types: string[]) => - types.length === 0 || types.includes(item.id), - searchTerm: (item: HydratedA11yFrame, searchTerm: string) => { - return JSON.stringify(item).toLowerCase().includes(searchTerm); - }, -}; - -const IMPACT_SORT_ORDER = { - minor: 3, - moderate: 3, - serious: 2, - critical: 1, -}; - -function useAccessibilityFilters({accessibilityData}: Options): Return { - const {setFilter, query} = useFiltersInLocationQuery(); - - const impact = useMemo(() => decodeList(query.f_a_impact), [query.f_a_impact]); - const type = useMemo(() => decodeList(query.f_a_type), [query.f_a_type]); - const searchTerm = decodeScalar(query.f_a_search, '')?.toLowerCase(); - - const setFilterAndClearDetails = useCallback( - arg => { - setFilter({ - ...arg, - a_detail_row: undefined, - }); - }, - [setFilter] - ); - - const items = useMemo( - () => - filterItems({ - items: accessibilityData, - filterFns: FILTERS, - filterVals: {impact, type, searchTerm}, - }), - [accessibilityData, impact, type, searchTerm] - ); - - const getImpactLevels = useCallback( - () => - Array.from( - new Set(accessibilityData.map(data => String(data.impact)).concat(impact)) - ) - .filter(Boolean) - .sort((a, b) => (IMPACT_SORT_ORDER[a] < IMPACT_SORT_ORDER[b] ? -1 : 1)) - .map( - (value): AccessibilitySelectOption => ({ - value, - label: value, - qs: 'f_a_impact', - }) - ), - [accessibilityData, impact] - ); - - const getIssueTypes = useCallback( - () => - Array.from(new Set(accessibilityData.map(data => data.id).concat(type))) - .sort() - .map( - (value): AccessibilitySelectOption => ({ - value, - label: value, - qs: 'f_a_type', - }) - ), - [accessibilityData, type] - ); - - const setSearchTerm = useCallback( - (f_a_search: string) => - setFilterAndClearDetails({f_a_search: f_a_search || undefined}), - [setFilterAndClearDetails] - ); - - const setFilters = useCallback( - (value: AccessibilitySelectOption[]) => { - const groupedValues = value.reduce((state, selection) => { - return { - ...state, - [selection.qs]: [...state[selection.qs], selection.value], - }; - }, DEFAULT_FILTERS); - setFilterAndClearDetails(groupedValues); - }, - [setFilterAndClearDetails] - ); - - return { - getImpactLevels, - getIssueTypes, - items, - searchTerm, - selectValue: [...impact, ...type], - setFilters, - setSearchTerm, - }; -} - -export default useAccessibilityFilters; diff --git a/static/app/views/replays/detail/accessibility/useSortAccessibility.tsx b/static/app/views/replays/detail/accessibility/useSortAccessibility.tsx deleted file mode 100644 index 4785cb7b837b52..00000000000000 --- a/static/app/views/replays/detail/accessibility/useSortAccessibility.tsx +++ /dev/null @@ -1,112 +0,0 @@ -import {useCallback, useMemo} from 'react'; - -import type {HydratedA11yFrame} from 'sentry/utils/replays/hydrateA11yFrame'; -import useUrlParams from 'sentry/utils/useUrlParams'; - -interface SortConfig { - asc: boolean; - by: keyof HydratedA11yFrame | string; - getValue: (row: HydratedA11yFrame) => any; -} - -const IMPACT_SORT = { - minor: 0, - moderate: 1, - serious: 2, - critical: 3, -}; - -const SortStrategies: Record any> = { - impact: row => IMPACT_SORT[row.impact], - id: row => row.id, - element: row => row.element, - timestampMs: row => row.timestampMs, -}; - -const DEFAULT_ASC = 'false'; -const DEFAULT_BY = 'impact'; - -type Opts = {items: HydratedA11yFrame[]}; - -function useSortAccessibility({items}: Opts) { - const {getParamValue: getSortAsc, setParamValue: setSortAsc} = useUrlParams( - 's_a_asc', - DEFAULT_ASC - ); - const {getParamValue: getSortBy, setParamValue: setSortBy} = useUrlParams( - 's_a_by', - DEFAULT_BY - ); - const {setParamValue: setDetailRow} = useUrlParams('a_detail_row', ''); - - const sortAsc = getSortAsc(); - const sortBy = getSortBy(); - - const sortConfig = useMemo( - () => - ({ - asc: sortAsc === 'true', - by: sortBy, - getValue: SortStrategies[sortBy], - }) as SortConfig, - [sortAsc, sortBy] - ); - - const sortedItems = useMemo( - () => sortAccessibility(items, sortConfig), - [items, sortConfig] - ); - - const handleSort = useCallback( - (fieldName: keyof typeof SortStrategies) => { - if (sortConfig.by === fieldName) { - setSortAsc(sortConfig.asc ? 'false' : 'true'); - } else { - setSortAsc('true'); - setSortBy(fieldName); - } - setDetailRow(''); - }, - [sortConfig, setSortAsc, setSortBy, setDetailRow] - ); - - return { - handleSort, - items: sortedItems, - sortConfig, - }; -} - -function sortAccessibility( - accessibility: HydratedA11yFrame[], - sortConfig: SortConfig -): HydratedA11yFrame[] { - return [...accessibility].sort((a, b) => { - let valueA = sortConfig.getValue(a); - let valueB = sortConfig.getValue(b); - - valueA = typeof valueA === 'string' ? valueA.toUpperCase() : valueA; - valueB = typeof valueB === 'string' ? valueB.toUpperCase() : valueB; - - // if the values are not defined, we want to push them to the bottom of the list - if (valueA === undefined) { - return 1; - } - - if (valueB === undefined) { - return -1; - } - - if (valueA === valueB) { - return 0; - } - - if (sortConfig.asc) { - return valueA > valueB ? 1 : -1; - } - - return valueB > valueA ? 1 : -1; - }); -} - -export default useSortAccessibility; diff --git a/static/app/views/replays/detail/layout/focusArea.tsx b/static/app/views/replays/detail/layout/focusArea.tsx index 397e3ba12bcf11..f471a97c42a1a8 100644 --- a/static/app/views/replays/detail/layout/focusArea.tsx +++ b/static/app/views/replays/detail/layout/focusArea.tsx @@ -1,5 +1,4 @@ import useActiveReplayTab, {TabKey} from 'sentry/utils/replays/hooks/useActiveReplayTab'; -import A11y from 'sentry/views/replays/detail/accessibility/index'; import Breadcrumbs from 'sentry/views/replays/detail/breadcrumbs'; import Console from 'sentry/views/replays/detail/console'; import ErrorList from 'sentry/views/replays/detail/errorList/index'; @@ -20,8 +19,6 @@ export default function FocusArea({ const {getActiveTab} = useActiveReplayTab({isVideoReplay}); switch (getActiveTab()) { - case TabKey.A11Y: - return ; case TabKey.NETWORK: return ; case TabKey.TRACE: diff --git a/static/app/views/replays/detail/layout/focusTabs.tsx b/static/app/views/replays/detail/layout/focusTabs.tsx index 4def49b97df3b6..baf6c0901134cb 100644 --- a/static/app/views/replays/detail/layout/focusTabs.tsx +++ b/static/app/views/replays/detail/layout/focusTabs.tsx @@ -1,12 +1,7 @@ import type {ReactNode} from 'react'; -import {Fragment} from 'react'; -import styled from '@emotion/styled'; -import FeatureBadge from 'sentry/components/badge/featureBadge'; -import ExternalLink from 'sentry/components/links/externalLink'; import ListLink from 'sentry/components/links/listLink'; import ScrollableTabs from 'sentry/components/replays/scrollableTabs'; -import {Tooltip} from 'sentry/components/tooltip'; import {t} from 'sentry/locale'; import {trackAnalytics} from 'sentry/utils/analytics'; import useActiveReplayTab, {TabKey} from 'sentry/utils/replays/hooks/useActiveReplayTab'; @@ -18,36 +13,13 @@ function getReplayTabs({ }: { isVideoReplay: boolean; }): Record { - // For video replays, we hide the a11y and memory tabs (not applicable for mobile) + // For video replays, we hide the memory tab (not applicable for mobile) return { [TabKey.BREADCRUMBS]: t('Breadcrumbs'), [TabKey.CONSOLE]: t('Console'), [TabKey.NETWORK]: t('Network'), [TabKey.ERRORS]: t('Errors'), [TabKey.TRACE]: t('Trace'), - [TabKey.A11Y]: isVideoReplay ? null : ( - - { - e.stopPropagation(); - }} - > - {t('What is accessibility?')} - - } - > - {t('Accessibility')} - - - - ), [TabKey.MEMORY]: isVideoReplay ? null : t('Memory'), [TabKey.TAGS]: t('Tags'), }; @@ -92,10 +64,4 @@ function FocusTabs({className, isVideoReplay}: Props) { ); } -const FlexFeatureBadge = styled(FeatureBadge)` - & > span { - display: flex; - } -`; - export default FocusTabs; From 094a03198e6c4d088d83a7f8210b04c1acf9f5b3 Mon Sep 17 00:00:00 2001 From: Evan Purkhiser Date: Wed, 16 Oct 2024 14:34:45 -0400 Subject: [PATCH 32/67] ref(crons): Remove usage of crons-write-user-feedback flag (#79135) --- src/sentry/monitors/processing_errors/manager.py | 4 +--- .../sentry/monitors/processing_errors/test_manager.py | 11 +---------- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/sentry/monitors/processing_errors/manager.py b/src/sentry/monitors/processing_errors/manager.py index 0d6e5c08fdad6c..af4d1e839710ab 100644 --- a/src/sentry/monitors/processing_errors/manager.py +++ b/src/sentry/monitors/processing_errors/manager.py @@ -10,7 +10,7 @@ from redis.client import StrictRedis from rediscluster import RedisCluster -from sentry import analytics, features +from sentry import analytics from sentry.models.organization import Organization from sentry.models.project import Project from sentry.monitors.models import Monitor @@ -180,8 +180,6 @@ def handle_processing_errors(item: CheckinItem, error: ProcessingErrorsException try: project = Project.objects.get_from_cache(id=item.message["project_id"]) organization = Organization.objects.get_from_cache(id=project.organization_id) - if not features.has("organizations:crons-write-user-feedback", organization): - return metrics.incr( "monitors.checkin.handle_processing_error", diff --git a/tests/sentry/monitors/processing_errors/test_manager.py b/tests/sentry/monitors/processing_errors/test_manager.py index bcd5872ee3d8ae..9629d716acada5 100644 --- a/tests/sentry/monitors/processing_errors/test_manager.py +++ b/tests/sentry/monitors/processing_errors/test_manager.py @@ -229,20 +229,11 @@ def test(self, mock_record): handle_processing_errors( build_checkin_item( message_overrides={"project_id": self.project.id}, + payload_overrides={"monitor_slug": monitor.slug}, ), exception, ) errors = get_errors_for_monitor(monitor) - assert not errors - with self.feature("organizations:crons-write-user-feedback"): - handle_processing_errors( - build_checkin_item( - message_overrides={"project_id": self.project.id}, - payload_overrides={"monitor_slug": monitor.slug}, - ), - exception, - ) - errors = get_errors_for_monitor(monitor) assert len(errors) == 1 mock_record.assert_called_with( From 6d007c01548dfab19ef95a83d3948e766b1196c3 Mon Sep 17 00:00:00 2001 From: Evan Purkhiser Date: Wed, 16 Oct 2024 14:34:56 -0400 Subject: [PATCH 33/67] Reapply "ref(uptime): Remove uptime-display-wizard-create flag usage" (#79132) This reverts commit 6d05f347150ac6b8acd0ac0617fa65d9f7cade62. --- static/app/views/alerts/wizard/index.spec.tsx | 4 ++-- static/app/views/alerts/wizard/options.tsx | 10 ++++------ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/static/app/views/alerts/wizard/index.spec.tsx b/static/app/views/alerts/wizard/index.spec.tsx index d039917dcd7ac1..dd291fbc52f85a 100644 --- a/static/app/views/alerts/wizard/index.spec.tsx +++ b/static/app/views/alerts/wizard/index.spec.tsx @@ -70,10 +70,11 @@ describe('AlertWizard', () => { expect(screen.getByText('Errors')).toBeInTheDocument(); expect(screen.getByText('Sessions')).toBeInTheDocument(); expect(screen.getByText('Performance')).toBeInTheDocument(); + expect(screen.getByText('Uptime Monitoring')).toBeInTheDocument(); expect(screen.getByText('LLM Monitoring')).toBeInTheDocument(); expect(screen.getByText('Custom')).toBeInTheDocument(); const alertGroups = screen.getAllByRole('radiogroup'); - expect(alertGroups.length).toEqual(5); + expect(alertGroups.length).toEqual(6); }); it('should only render alerts for errors in self-hosted errors only', () => { @@ -112,7 +113,6 @@ describe('AlertWizard', () => { 'incidents', 'performance-view', 'crash-rate-alerts', - 'uptime-display-wizard-create', ], access: ['org:write', 'alerts:write'], }, diff --git a/static/app/views/alerts/wizard/options.tsx b/static/app/views/alerts/wizard/options.tsx index 04d93bb1dc0ad6..f12fd4989cbd23 100644 --- a/static/app/views/alerts/wizard/options.tsx +++ b/static/app/views/alerts/wizard/options.tsx @@ -144,12 +144,10 @@ export const getAlertWizardCategories = (org: Organization) => { }); } - if (org.features.includes('uptime-display-wizard-create')) { - result.push({ - categoryHeading: t('Uptime Monitoring'), - options: ['uptime_monitor'], - }); - } + result.push({ + categoryHeading: t('Uptime Monitoring'), + options: ['uptime_monitor'], + }); result.push({ categoryHeading: hasCustomMetrics(org) ? t('Metrics') : t('Custom'), options: [hasCustomMetrics(org) ? 'custom_metrics' : 'custom_transactions'], From 9eb335530970f91c4eb57016528296988aef0d4e Mon Sep 17 00:00:00 2001 From: anthony sottile <103459774+asottile-sentry@users.noreply.github.com> Date: Wed, 16 Oct 2024 14:35:40 -0400 Subject: [PATCH 34/67] ref: remove deprecated SENTRY_URL_PREFIX (#79203) nothing reads this, it has been warning since 2015 -- it accesses the database at import time --- src/sentry/options/manager.py | 5 ----- src/sentry/runner/initializer.py | 16 ---------------- tests/sentry/runner/test_initializer.py | 15 --------------- 3 files changed, 36 deletions(-) diff --git a/src/sentry/options/manager.py b/src/sentry/options/manager.py index f8d2b98df824ba..aeaf12ff785948 100644 --- a/src/sentry/options/manager.py +++ b/src/sentry/options/manager.py @@ -298,11 +298,6 @@ def get(self, key: str, silent=False): if not (opt.flags & FLAG_NOSTORE): result = self.store.get(opt, silent=silent) if result is not None: - # HACK(mattrobenolt): SENTRY_URL_PREFIX must be kept in sync - # when reading values from the database. This should - # be replaced by a signal. - if key == "system.url-prefix": - settings.SENTRY_URL_PREFIX = result return result # Some values we don't want to allow them to be configured through diff --git a/src/sentry/runner/initializer.py b/src/sentry/runner/initializer.py index 9408bd5b5f51a1..d94b8433d1fb06 100644 --- a/src/sentry/runner/initializer.py +++ b/src/sentry/runner/initializer.py @@ -203,15 +203,6 @@ def bootstrap_options(settings: Any, config: str | None = None) -> None: # these will be validated later after bootstrapping for k, v in options.items(): settings.SENTRY_OPTIONS[k] = v - # If SENTRY_URL_PREFIX is used in config, show deprecation warning and - # set the newer SENTRY_OPTIONS['system.url-prefix']. Needs to be here - # to check from the config file directly before the django setup is done. - # TODO: delete when SENTRY_URL_PREFIX is removed - if k == "SENTRY_URL_PREFIX": - warnings.warn( - DeprecatedSettingWarning("SENTRY_URL_PREFIX", "SENTRY_OPTIONS['system.url-prefix']") - ) - settings.SENTRY_OPTIONS["system.url-prefix"] = v # Now go back through all of SENTRY_OPTIONS and promote # back into settings. This catches the case when values are defined @@ -582,13 +573,6 @@ def apply_legacy_settings(settings: Any) -> None: # option.) settings.SENTRY_REDIS_OPTIONS = options.get("redis.clusters")["default"] - if not hasattr(settings, "SENTRY_URL_PREFIX"): - url_prefix = options.get("system.url-prefix", silent=True) - if not url_prefix: - # HACK: We need to have some value here for backwards compatibility - url_prefix = "http://sentry.example.com" - settings.SENTRY_URL_PREFIX = url_prefix - if settings.TIME_ZONE != "UTC": # non-UTC timezones are not supported show_big_error("TIME_ZONE should be set to UTC") diff --git a/tests/sentry/runner/test_initializer.py b/tests/sentry/runner/test_initializer.py index 46dbd95420a22c..cf1acc31ebca3c 100644 --- a/tests/sentry/runner/test_initializer.py +++ b/tests/sentry/runner/test_initializer.py @@ -185,21 +185,6 @@ def test_bootstrap_options_empty_file(settings, config_yml): assert settings.SENTRY_OPTIONS == {} -def test_legacy_url_prefix_warning(settings, config_yml): - config_yml.write( - """\ - SENTRY_URL_PREFIX: http://sentry.example.com - """ - ) - with pytest.warns(DeprecatedSettingWarning) as warninfo: - bootstrap_options(settings, str(config_yml)) - assert settings.SENTRY_OPTIONS["system.url-prefix"] == "http://sentry.example.com" - assert settings.SENTRY_OPTIONS["SENTRY_URL_PREFIX"] == "http://sentry.example.com" - _assert_settings_warnings( - warninfo, {("SENTRY_URL_PREFIX", "SENTRY_OPTIONS['system.url-prefix']")} - ) - - def test_apply_legacy_settings(settings): settings.ALLOWED_HOSTS = [] settings.SENTRY_USE_QUEUE = True From 68e5b1d57db6710405266e403cc21ff1dbbfa8f6 Mon Sep 17 00:00:00 2001 From: anthony sottile <103459774+asottile-sentry@users.noreply.github.com> Date: Wed, 16 Oct 2024 14:37:17 -0400 Subject: [PATCH 35/67] ref: avoid accessing options at module scope to build logo url (#79207) (this accessed the database at module scope to look up the option value) --- src/sentry/integrations/metric_alerts.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/sentry/integrations/metric_alerts.py b/src/sentry/integrations/metric_alerts.py index e311869ea9e088..fb544a6bd9b951 100644 --- a/src/sentry/integrations/metric_alerts.py +++ b/src/sentry/integrations/metric_alerts.py @@ -26,7 +26,6 @@ "percentage(sessions_crashed, sessions)": "% sessions crash free rate", "percentage(users_crashed, users)": "% users crash free rate", } -LOGO_URL = absolute_uri(get_asset_url("sentry", "images/sentry-email-avatar.png")) # These should be the same as the options in the frontend # COMPARISON_DELTA_OPTIONS TEXT_COMPARISON_DELTA = { @@ -39,6 +38,10 @@ } +def logo_url() -> str: + return absolute_uri(get_asset_url("sentry", "images/sentry-email-avatar.png")) + + def get_metric_count_from_incident(incident: Incident) -> str: """Returns the current or last count of an incident aggregate.""" incident_trigger = ( @@ -144,7 +147,7 @@ def incident_attachment_info( return { "title": title, "text": text, - "logo_url": LOGO_URL, + "logo_url": logo_url(), "status": status, "ts": incident.date_started, "title_link": title_link, @@ -232,7 +235,7 @@ def metric_alert_attachment_info( return { "title": title, "text": text, - "logo_url": LOGO_URL, + "logo_url": logo_url(), "status": status, "date_started": date_started, "last_triggered_date": last_triggered_date, From 987652b0ccdd047e671e2afda6600c79decf61da Mon Sep 17 00:00:00 2001 From: anthony sottile <103459774+asottile-sentry@users.noreply.github.com> Date: Wed, 16 Oct 2024 14:44:08 -0400 Subject: [PATCH 36/67] ref: type iso_format(...) (#79209) I'm planning on removing this function, typing it helps ease that transition and avoid a bunch of manual fixes later --- src/sentry/testutils/helpers/datetime.py | 2 +- tests/snuba/api/serializers/test_group.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/sentry/testutils/helpers/datetime.py b/src/sentry/testutils/helpers/datetime.py index c68d1db640b2c4..da1af95d8c5c51 100644 --- a/src/sentry/testutils/helpers/datetime.py +++ b/src/sentry/testutils/helpers/datetime.py @@ -8,7 +8,7 @@ __all__ = ["iso_format", "before_now", "timestamp_format"] -def iso_format(date): +def iso_format(date: datetime) -> str: return date.isoformat()[:19] diff --git a/tests/snuba/api/serializers/test_group.py b/tests/snuba/api/serializers/test_group.py index 90fe986d0cffe0..955cf14a5bcc4c 100644 --- a/tests/snuba/api/serializers/test_group.py +++ b/tests/snuba/api/serializers/test_group.py @@ -408,7 +408,8 @@ def test_seen_stats(self): # result is rounded down to nearest second assert iso_format(result["lastSeen"]) == iso_format(self.min_ago) assert iso_format(result["firstSeen"]) == iso_format(group_env.first_seen) - assert iso_format(group_env2.first_seen) > iso_format(group_env.first_seen) + assert group_env2.first_seen is not None + assert group_env2.first_seen > group_env.first_seen assert result["userCount"] == 3 result = serialize( From 45cd89a1da658918ae91fa8a0a9231231a5b76dd Mon Sep 17 00:00:00 2001 From: Lyn Nagara <1779792+lynnagara@users.noreply.github.com> Date: Wed, 16 Oct 2024 11:48:15 -0700 Subject: [PATCH 37/67] ref: cleanup option used for rollout (#79202) this has now been successfully rolled out in prod, we can clean up the option --- src/sentry/options/defaults.py | 8 -------- src/sentry/tasks/base.py | 13 +++++-------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/src/sentry/options/defaults.py b/src/sentry/options/defaults.py index eedde491bb11c9..0c8807fd51fe5e 100644 --- a/src/sentry/options/defaults.py +++ b/src/sentry/options/defaults.py @@ -2793,11 +2793,3 @@ default=True, flags=FLAG_AUTOMATOR_MODIFIABLE, ) - -# TODO: Temporary, to be removed -register( - "split_queue_task_router.enable", - type=Bool, - default=False, - flags=FLAG_AUTOMATOR_MODIFIABLE, -) diff --git a/src/sentry/tasks/base.py b/src/sentry/tasks/base.py index a5648f2bf39db5..6559ca3b38d983 100644 --- a/src/sentry/tasks/base.py +++ b/src/sentry/tasks/base.py @@ -12,7 +12,6 @@ from django.conf import settings from django.db.models import Model -from sentry import options from sentry.celery import app from sentry.silo.base import SiloLimit, SiloMode from sentry.utils import metrics @@ -137,13 +136,11 @@ def _wrapped(*args, **kwargs): # If the split task router is configured for the task, always use queues defined # in the split task configuration if name in settings.CELERY_SPLIT_QUEUE_TASK_ROUTES: - # TODO: remove this option once rolled out - if options.get("split_queue_task_router.enable"): - q = kwargs.pop("queue") - if q: - logger.warning( - "ignoring queue: %s, using value from CELERY_SPLIT_QUEUE_TASK_ROUTES", q - ) + q = kwargs.pop("queue") + if q: + logger.warning( + "ignoring queue: %s, using value from CELERY_SPLIT_QUEUE_TASK_ROUTES", q + ) # We never use result backends in Celery. Leaving `trail=True` means that if we schedule # many tasks from a parent task, each task leaks memory. This can lead to the scheduler From 972fcc9199a32c8d7188f9d06cad69ec092ca8cf Mon Sep 17 00:00:00 2001 From: Malachi Willey Date: Wed, 16 Oct 2024 11:49:56 -0700 Subject: [PATCH 38/67] fix(issue-stream): Hide log level dot when not applicable to issue type (#79214) --- .../app/components/eventOrGroupExtraDetails.tsx | 4 ++-- static/app/components/events/eventMessage.tsx | 16 +++------------- static/app/utils/events.tsx | 14 ++++++++++++++ 3 files changed, 19 insertions(+), 15 deletions(-) diff --git a/static/app/components/eventOrGroupExtraDetails.tsx b/static/app/components/eventOrGroupExtraDetails.tsx index 9ebeecbe80a69e..c4745583a7ac5a 100644 --- a/static/app/components/eventOrGroupExtraDetails.tsx +++ b/static/app/components/eventOrGroupExtraDetails.tsx @@ -20,7 +20,7 @@ import type {Event} from 'sentry/types/event'; import type {Group} from 'sentry/types/group'; import type {Organization} from 'sentry/types/organization'; import {defined} from 'sentry/utils'; -import {getTitle} from 'sentry/utils/events'; +import {eventTypeHasLogLevel, getTitle} from 'sentry/utils/events'; import useReplayCountForIssues from 'sentry/utils/replayCount/useReplayCountForIssues'; import {projectCanLinkToReplay} from 'sentry/utils/replays/projectSupportsReplay'; import withOrganization from 'sentry/utils/withOrganization'; @@ -86,7 +86,7 @@ function EventOrGroupExtraDetails({ const hasNewLayout = organization.features.includes('issue-stream-table-layout'); const {subtitle} = getTitle(data); - const level = 'level' in data ? data.level : null; + const level = eventTypeHasLogLevel(data.type) && 'level' in data ? data.level : null; const items = [ hasNewLayout && level ? : null, diff --git a/static/app/components/events/eventMessage.tsx b/static/app/components/events/eventMessage.tsx index 8eedf3f10658f4..589296db0392f6 100644 --- a/static/app/components/events/eventMessage.tsx +++ b/static/app/components/events/eventMessage.tsx @@ -4,9 +4,9 @@ import ErrorLevel from 'sentry/components/events/errorLevel'; import UnhandledTag from 'sentry/components/group/inboxBadges/unhandledTag'; import {t} from 'sentry/locale'; import {space} from 'sentry/styles/space'; -import {type Event, EventOrGroupType, type Level} from 'sentry/types/event'; +import type {Event, EventOrGroupType, Level} from 'sentry/types/event'; import type {BaseGroup, GroupTombstoneHelper} from 'sentry/types/group'; -import {getTitle} from 'sentry/utils/events'; +import {eventTypeHasLogLevel, getTitle} from 'sentry/utils/events'; import useOrganization from 'sentry/utils/useOrganization'; import {Divider} from 'sentry/views/issueDetails/divider'; import {useHasStreamlinedUI} from 'sentry/views/issueDetails/utils'; @@ -24,16 +24,6 @@ type Props = { showUnhandled?: boolean; }; -const EVENT_TYPES_WITH_LOG_LEVEL = new Set([ - EventOrGroupType.ERROR, - EventOrGroupType.CSP, - EventOrGroupType.EXPECTCT, - EventOrGroupType.DEFAULT, - EventOrGroupType.EXPECTSTAPLE, - EventOrGroupType.HPKP, - EventOrGroupType.NEL, -]); - function EventMessage({ data, className, @@ -51,7 +41,7 @@ function EventMessage({ 'issue-stream-table-layout' ); - const showEventLevel = level && EVENT_TYPES_WITH_LOG_LEVEL.has(type); + const showEventLevel = level && eventTypeHasLogLevel(type); const {subtitle} = getTitle(data); const renderedMessage = message ? ( {message} diff --git a/static/app/utils/events.tsx b/static/app/utils/events.tsx index 3e9256edc2b239..855ddd284ce1da 100644 --- a/static/app/utils/events.tsx +++ b/static/app/utils/events.tsx @@ -24,6 +24,20 @@ import {getDaysSinceDatePrecise} from 'sentry/utils/getDaysSinceDate'; import {isMobilePlatform, isNativePlatform} from 'sentry/utils/platform'; import {getReplayIdFromEvent} from 'sentry/utils/replays/getReplayIdFromEvent'; +const EVENT_TYPES_WITH_LOG_LEVEL = new Set([ + EventOrGroupType.ERROR, + EventOrGroupType.CSP, + EventOrGroupType.EXPECTCT, + EventOrGroupType.DEFAULT, + EventOrGroupType.EXPECTSTAPLE, + EventOrGroupType.HPKP, + EventOrGroupType.NEL, +]); + +export function eventTypeHasLogLevel(type: EventOrGroupType) { + return EVENT_TYPES_WITH_LOG_LEVEL.has(type); +} + export function isTombstone( maybe: BaseGroup | Event | GroupTombstoneHelper ): maybe is GroupTombstoneHelper { From 862c533dcb39381f8b6d2a8b349b33f31011de79 Mon Sep 17 00:00:00 2001 From: Rohan Agarwal <47861399+roaga@users.noreply.github.com> Date: Wed, 16 Oct 2024 11:58:39 -0700 Subject: [PATCH 39/67] fix(autofix): Allow github extension orgs to use autofix (#79211) We check for orgs that we gave permission to use the github extension as giving autofix consent, even if they haven't given the standard gen AI consent. --- src/sentry/api/endpoints/seer_rpc.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/sentry/api/endpoints/seer_rpc.py b/src/sentry/api/endpoints/seer_rpc.py index f468d0f5ae5a38..8ad06295ed6c8a 100644 --- a/src/sentry/api/endpoints/seer_rpc.py +++ b/src/sentry/api/endpoints/seer_rpc.py @@ -17,6 +17,7 @@ from rest_framework.response import Response from sentry_sdk import Scope, capture_exception +from sentry import options from sentry.api.api_owners import ApiOwner from sentry.api.api_publish_status import ApiPublishStatus from sentry.api.authentication import AuthenticationSiloLimit, StandardAuthentication @@ -153,8 +154,9 @@ def get_organization_slug(*, org_id: int) -> dict: def get_organization_autofix_consent(*, org_id: int) -> dict: org: Organization = Organization.objects.get(id=org_id) consent = org.get_option("sentry:gen_ai_consent", False) + github_extension_enabled = org_id in options.get("github-extension.enabled-orgs") return { - "consent": consent, + "consent": consent or github_extension_enabled, } From aabab252a14c1318adfdf6bcd01dd10fe558d6fb Mon Sep 17 00:00:00 2001 From: Nate Moore Date: Wed, 16 Oct 2024 12:00:26 -0700 Subject: [PATCH 40/67] fix(style): remove scrollbar-gutter property (#79215) The [`scrollbar-gutter`](https://developer.mozilla.org/en-US/docs/Web/CSS/scrollbar-gutter#stable) property added in #77665 is causing some layout issues. It also should have been feature flagged, but was incorrectly added to the global layout styles. Thanks for flagging @scttcper! ![image](https://github.com/user-attachments/assets/6fcc1224-f664-4623-8e69-f266c0caef30) --- static/less/base.less | 1 - 1 file changed, 1 deletion(-) diff --git a/static/less/base.less b/static/less/base.less index dae234bf567fcb..d96c1dd87365bc 100644 --- a/static/less/base.less +++ b/static/less/base.less @@ -17,7 +17,6 @@ html { -webkit-text-size-adjust: 100%; -webkit-tap-highlight-color: rgba(0, 0, 0, 0); height: 100%; - scrollbar-gutter: stable; } body { From 87f993d52d1939807bb597c1d530ebd39f8b40f4 Mon Sep 17 00:00:00 2001 From: Scott Cooper Date: Wed, 16 Oct 2024 12:00:43 -0700 Subject: [PATCH 41/67] fix(alerts): Remove extra metric alert details padding (#79107) --- static/app/views/alerts/rules/metric/details/body.tsx | 11 +++++------ .../app/views/alerts/rules/metric/details/sidebar.tsx | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/static/app/views/alerts/rules/metric/details/body.tsx b/static/app/views/alerts/rules/metric/details/body.tsx index 07e6cd4caae3a5..c85c7ca2eaa3c5 100644 --- a/static/app/views/alerts/rules/metric/details/body.tsx +++ b/static/app/views/alerts/rules/metric/details/body.tsx @@ -170,12 +170,11 @@ export default function MetricDetailsBody({ return ( - - {isCustomMetricAlert(rule.aggregate) && - !isInsightsMetricAlert(rule.aggregate) && ( - - )} - + {isCustomMetricAlert(rule.aggregate) && !isInsightsMetricAlert(rule.aggregate) && ( + + + + )} {selectedIncident?.alertRule.status === AlertRuleStatus.SNAPSHOT && ( diff --git a/static/app/views/alerts/rules/metric/details/sidebar.tsx b/static/app/views/alerts/rules/metric/details/sidebar.tsx index 40529ae9027103..40479111377531 100644 --- a/static/app/views/alerts/rules/metric/details/sidebar.tsx +++ b/static/app/views/alerts/rules/metric/details/sidebar.tsx @@ -146,7 +146,7 @@ export function MetricDetailsSidebar({ const ownerId = rule.owner?.split(':')[1]; const teamActor = ownerId && {type: 'team' as Actor['type'], id: ownerId, name: ''}; - let conditionType; + let conditionType: React.ReactNode; const activationCondition = rule.monitorType === MonitorType.ACTIVATED && typeof rule.activationCondition !== 'undefined' && From 8922176d50cc582cd36b27b304415894b0438c9d Mon Sep 17 00:00:00 2001 From: edwardgou-sentry <83961295+edwardgou-sentry@users.noreply.github.com> Date: Wed, 16 Oct 2024 15:17:05 -0400 Subject: [PATCH 42/67] feat(alerts): Add EAP as a supported alert dataset (#78968) Adds `PerformanceSpansEAPEntitySubscription` and support for EAP dataset as an alert type. Also updates the AlertRule serializer to short circuit validation on the aggregate field, since that validation uses very old logic which doesn't support EAP. This should be fine because the query validation goes through the actual query builder process which should provide enough validation already. It's probably also ok to skip some validation since these changes are only intended for internal EAP prototyping. --- src/sentry/incidents/logic.py | 27 ++++++++++++- src/sentry/incidents/serializers/__init__.py | 6 ++- .../incidents/serializers/alert_rule.py | 6 +++ src/sentry/snuba/entity_subscription.py | 40 +++++++++++++++++++ 4 files changed, 77 insertions(+), 2 deletions(-) diff --git a/src/sentry/incidents/logic.py b/src/sentry/incidents/logic.py index 27c5a1bb354fcf..35b68b43422438 100644 --- a/src/sentry/incidents/logic.py +++ b/src/sentry/incidents/logic.py @@ -479,6 +479,7 @@ class AlertRuleNameAlreadyUsedError(Exception): Dataset.Transactions: SnubaQuery.Type.PERFORMANCE, Dataset.PerformanceMetrics: SnubaQuery.Type.PERFORMANCE, Dataset.Metrics: SnubaQuery.Type.CRASH_RATE, + Dataset.EventsAnalyticsPlatform: SnubaQuery.Type.PERFORMANCE, } @@ -1845,6 +1846,22 @@ def get_opsgenie_teams(organization_id: int, integration_id: int) -> list[tuple[ "measurements.score.total", ], } +EAP_COLUMNS = [ + "span.duration", + "span.self_time", +] +EAP_FUNCTIONS = [ + "count", + "avg", + "p50", + "p75", + "p90", + "p95", + "p99", + "p100", + "max", + "min", +] def get_column_from_aggregate(aggregate: str, allow_mri: bool) -> str | None: @@ -1857,6 +1874,11 @@ def get_column_from_aggregate(aggregate: str, allow_mri: bool) -> str | None: or match.group("function") in METRICS_LAYER_UNSUPPORTED_TRANSACTION_METRICS_FUNCTIONS ): return None if match.group("columns") == "" else match.group("columns") + + # Skip additional validation for EAP queries. They don't exist in the old logic. + if match and match.group("function") in EAP_FUNCTIONS and match.group("columns") in EAP_COLUMNS: + return match.group("columns") + if allow_mri: mri_column = _get_column_from_aggregate_with_mri(aggregate) # Only if the column was allowed, we return it, otherwise we fallback to the old logic. @@ -1889,7 +1911,9 @@ def _get_column_from_aggregate_with_mri(aggregate: str) -> str | None: return columns -def check_aggregate_column_support(aggregate: str, allow_mri: bool = False) -> bool: +def check_aggregate_column_support( + aggregate: str, allow_mri: bool = False, allow_eap: bool = False +) -> bool: # TODO(ddm): remove `allow_mri` once the experimental feature flag is removed. column = get_column_from_aggregate(aggregate, allow_mri) match = is_function(aggregate) @@ -1904,6 +1928,7 @@ def check_aggregate_column_support(aggregate: str, allow_mri: bool = False) -> b isinstance(function, str) and column in INSIGHTS_FUNCTION_VALID_ARGS_MAP.get(function, []) ) + or (column in EAP_COLUMNS and allow_eap) ) diff --git a/src/sentry/incidents/serializers/__init__.py b/src/sentry/incidents/serializers/__init__.py index 061c29461acd3e..58a4bd86171ef0 100644 --- a/src/sentry/incidents/serializers/__init__.py +++ b/src/sentry/incidents/serializers/__init__.py @@ -26,7 +26,11 @@ } QUERY_TYPE_VALID_DATASETS = { SnubaQuery.Type.ERROR: {Dataset.Events}, - SnubaQuery.Type.PERFORMANCE: {Dataset.Transactions, Dataset.PerformanceMetrics}, + SnubaQuery.Type.PERFORMANCE: { + Dataset.Transactions, + Dataset.PerformanceMetrics, + Dataset.EventsAnalyticsPlatform, + }, SnubaQuery.Type.CRASH_RATE: {Dataset.Metrics}, } diff --git a/src/sentry/incidents/serializers/alert_rule.py b/src/sentry/incidents/serializers/alert_rule.py index f68911f9f0851b..756d74ef08c1d6 100644 --- a/src/sentry/incidents/serializers/alert_rule.py +++ b/src/sentry/incidents/serializers/alert_rule.py @@ -165,11 +165,17 @@ def validate_aggregate(self, aggregate): self.context["organization"], actor=self.context.get("user", None), ) + allow_eap = features.has( + "organizations:alerts-eap", + self.context["organization"], + actor=self.context.get("user", None), + ) try: if not check_aggregate_column_support( aggregate, allow_mri=allow_mri, + allow_eap=allow_eap, ): raise serializers.ValidationError( "Invalid Metric: We do not currently support this field." diff --git a/src/sentry/snuba/entity_subscription.py b/src/sentry/snuba/entity_subscription.py index 487426bf33d543..417e0d1697859c 100644 --- a/src/sentry/snuba/entity_subscription.py +++ b/src/sentry/snuba/entity_subscription.py @@ -16,6 +16,7 @@ from sentry.search.events.builder.base import BaseQueryBuilder from sentry.search.events.builder.discover import DiscoverQueryBuilder from sentry.search.events.builder.metrics import AlertMetricsQueryBuilder +from sentry.search.events.builder.spans_indexed import SpansEAPQueryBuilder from sentry.search.events.types import ParamsType, QueryBuilderConfig from sentry.sentry_metrics.use_case_id_registry import UseCaseID from sentry.sentry_metrics.utils import ( @@ -47,6 +48,7 @@ EntityKey.GenericMetricsGauges: "timestamp", EntityKey.MetricsCounters: "timestamp", EntityKey.MetricsSets: "timestamp", + EntityKey.EAPSpans: "timestamp", } CRASH_RATE_ALERT_AGGREGATE_RE = ( r"^percentage\([ ]*(sessions_crashed|users_crashed)[ ]*\,[ ]*(sessions|users)[ ]*\)" @@ -217,6 +219,41 @@ class PerformanceTransactionsEntitySubscription(BaseEventsAndTransactionEntitySu dataset = Dataset.Transactions +class PerformanceSpansEAPEntitySubscription(BaseEventsAndTransactionEntitySubscription): + query_type = SnubaQuery.Type.PERFORMANCE + dataset = Dataset.EventsAnalyticsPlatform + + def build_query_builder( + self, + query: str, + project_ids: list[int], + environment: Environment | None, + params: ParamsType | None = None, + skip_field_validation_for_entity_subscription_deletion: bool = False, + ) -> BaseQueryBuilder: + if params is None: + params = {} + + params["project_id"] = project_ids + + query = apply_dataset_query_conditions(self.query_type, query, self.event_types) + if environment: + params["environment"] = environment.name + + return SpansEAPQueryBuilder( + dataset=Dataset(self.dataset.value), + query=query, + selected_columns=[self.aggregate], + params=params, + offset=None, + limit=None, + config=QueryBuilderConfig( + skip_time_conditions=True, + skip_field_validation_for_entity_subscription_deletion=skip_field_validation_for_entity_subscription_deletion, + ), + ) + + class BaseMetricsEntitySubscription(BaseEntitySubscription, ABC): def __init__( self, aggregate: str, time_window: int, extra_fields: _EntitySpecificParams | None = None @@ -453,6 +490,7 @@ def get_snql_aggregations(self) -> list[str]: MetricsSetsEntitySubscription, PerformanceTransactionsEntitySubscription, PerformanceMetricsEntitySubscription, + PerformanceSpansEAPEntitySubscription, ] @@ -476,6 +514,8 @@ def get_entity_subscription( entity_subscription_cls = PerformanceTransactionsEntitySubscription elif dataset in (Dataset.Metrics, Dataset.PerformanceMetrics): entity_subscription_cls = PerformanceMetricsEntitySubscription + elif dataset == Dataset.EventsAnalyticsPlatform: + entity_subscription_cls = PerformanceSpansEAPEntitySubscription if query_type == SnubaQuery.Type.CRASH_RATE: entity_key = determine_crash_rate_alert_entity(aggregate) if entity_key == EntityKey.MetricsCounters: From b53585743fb2aad026b037763067a95449d19b2e Mon Sep 17 00:00:00 2001 From: Colton Allen Date: Wed, 16 Oct 2024 14:17:20 -0500 Subject: [PATCH 43/67] feat(flags): Add audit log flag filtering (#78934) Allows responses to be filtered by flag. Closes: https://github.com/getsentry/sentry/issues/78933 --------- Co-authored-by: Michelle Zhang <56095982+michellewzhang@users.noreply.github.com> --- src/sentry/flags/docs/api.md | 1 + src/sentry/flags/endpoints/logs.py | 4 +++ tests/sentry/flags/endpoints/test_logs.py | 31 +++++++++++++++++++++-- 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/sentry/flags/docs/api.md b/src/sentry/flags/docs/api.md index 5667acfaf81aa7..6a42c59fe9ed9d 100644 --- a/src/sentry/flags/docs/api.md +++ b/src/sentry/flags/docs/api.md @@ -13,6 +13,7 @@ This document is structured by resource with each resource having actions that c ## Flag Logs [/organizations//flags/logs/] - Parameters + - flag (optional, string) - The flag name to filter the result by. Can be specified multiple times. - start (optional, string) - ISO 8601 format (`YYYY-MM-DDTHH:mm:ss.sssZ`) - end (optional, string) - ISO 8601 format. Required if `start` is set. - statsPeriod (optional, string) - A positive integer suffixed with a unit type. diff --git a/src/sentry/flags/endpoints/logs.py b/src/sentry/flags/endpoints/logs.py index 8da767df45f7f9..45dfeea606ec21 100644 --- a/src/sentry/flags/endpoints/logs.py +++ b/src/sentry/flags/endpoints/logs.py @@ -61,6 +61,10 @@ def get(self, request: Request, organization: Organization) -> Response: organization_id=organization.id, ) + flags = request.GET.getlist("flag") + if flags: + queryset = queryset.filter(flag__in=flags) + return self.paginate( request=request, queryset=queryset, diff --git a/tests/sentry/flags/endpoints/test_logs.py b/tests/sentry/flags/endpoints/test_logs.py index 6efc54bde2f70d..92f6587f5c93a6 100644 --- a/tests/sentry/flags/endpoints/test_logs.py +++ b/tests/sentry/flags/endpoints/test_logs.py @@ -36,7 +36,6 @@ def test_get(self): result = response.json() assert len(result["data"]) == 1 - assert result["data"][0]["id"] == 1 assert result["data"][0]["action"] == "created" assert "created_at" in result["data"][0] assert result["data"][0]["created_by"] == "a@b.com" @@ -44,6 +43,35 @@ def test_get(self): assert result["data"][0]["flag"] == "hello" assert result["data"][0]["tags"] == {"commit_sha": "123"} + def test_get_filter_by_flag(self): + FlagAuditLogModel( + action=0, + created_at=datetime.now(timezone.utc), + created_by="a@b.com", + created_by_type=0, + flag="hello", + organization_id=self.organization.id, + tags={}, + ).save() + + FlagAuditLogModel( + action=0, + created_at=datetime.now(timezone.utc), + created_by="a@b.com", + created_by_type=0, + flag="world", + organization_id=self.organization.id, + tags={}, + ).save() + + with self.feature(self.features): + response = self.client.get(self.url + "?flag=world") + assert response.status_code == 200 + + result = response.json() + assert len(result["data"]) == 1 + assert result["data"][0]["flag"] == "world" + def test_get_unauthorized_organization(self): org = self.create_organization() url = reverse(self.endpoint, args=(org.id,)) @@ -125,7 +153,6 @@ def test_get(self): assert response.status_code == 200 result = response.json() - assert result["data"]["id"] == 4 assert result["data"]["action"] == "created" assert "created_at" in result["data"] assert result["data"]["created_by"] == "a@b.com" From 7788236efc20e73b5f2064bebed71f89612fed53 Mon Sep 17 00:00:00 2001 From: Dominik Buszowiecki <44422760+DominikB2014@users.noreply.github.com> Date: Wed, 16 Oct 2024 15:27:46 -0400 Subject: [PATCH 44/67] fix(insights): duration distribution chart displays non in domain views (#79210) --- .../insights/pages/ai/aiOverviewPage.tsx | 29 +++++++++++------ .../pages/backend/backendOverviewPage.tsx | 31 ++++++++++++------- .../pages/frontend/frontendOverviewPage.tsx | 29 +++++++++++------ .../pages/mobile/mobileOverviewPage.tsx | 29 +++++++++++------ 4 files changed, 77 insertions(+), 41 deletions(-) diff --git a/static/app/views/insights/pages/ai/aiOverviewPage.tsx b/static/app/views/insights/pages/ai/aiOverviewPage.tsx index aa517302178c1e..85b6989ff1d450 100644 --- a/static/app/views/insights/pages/ai/aiOverviewPage.tsx +++ b/static/app/views/insights/pages/ai/aiOverviewPage.tsx @@ -12,7 +12,10 @@ import {ProjectPageFilter} from 'sentry/components/organizations/projectPageFilt import TransactionNameSearchBar from 'sentry/components/performance/searchBar'; import SentryDocumentTitle from 'sentry/components/sentryDocumentTitle'; import {trackAnalytics} from 'sentry/utils/analytics'; -import {canUseMetricsData} from 'sentry/utils/performance/contexts/metricsEnhancedSetting'; +import { + canUseMetricsData, + useMEPSettingContext, +} from 'sentry/utils/performance/contexts/metricsEnhancedSetting'; import {PageAlert, usePageAlert} from 'sentry/utils/performance/contexts/pageAlert'; import {PerformanceDisplayProvider} from 'sentry/utils/performance/contexts/performanceDisplayContext'; import {MutableSearch} from 'sentry/utils/tokenizeSearch'; @@ -28,6 +31,7 @@ import {AI_LANDING_TITLE} from 'sentry/views/insights/pages/ai/settings'; import {OVERVIEW_PAGE_TITLE} from 'sentry/views/insights/pages/settings'; import {generateGenericPerformanceEventView} from 'sentry/views/performance/data'; import {TripleChartRow} from 'sentry/views/performance/landing/widgets/components/widgetChartRow'; +import {filterAllowedChartsMetrics} from 'sentry/views/performance/landing/widgets/utils'; import {PerformanceWidgetSetting} from 'sentry/views/performance/landing/widgets/widgetDefinitions'; import Onboarding from 'sentry/views/performance/onboarding'; import Table from 'sentry/views/performance/table'; @@ -54,6 +58,7 @@ function AiOverviewPage() { const {projects} = useProjects(); const onboardingProject = useOnboardingProject(); const navigate = useNavigate(); + const mepSetting = useMEPSettingContext(); const withStaticFilters = canUseMetricsData(organization); const eventView = generateGenericPerformanceEventView( @@ -76,15 +81,19 @@ function AiOverviewPage() { const showOnboarding = onboardingProject !== undefined; - const tripleChartRowCharts = [ - PerformanceWidgetSetting.TPM_AREA, - PerformanceWidgetSetting.DURATION_HISTOGRAM, - PerformanceWidgetSetting.P50_DURATION_AREA, - PerformanceWidgetSetting.P75_DURATION_AREA, - PerformanceWidgetSetting.P95_DURATION_AREA, - PerformanceWidgetSetting.P99_DURATION_AREA, - PerformanceWidgetSetting.FAILURE_RATE_AREA, - ]; + const tripleChartRowCharts = filterAllowedChartsMetrics( + organization, + [ + PerformanceWidgetSetting.TPM_AREA, + PerformanceWidgetSetting.DURATION_HISTOGRAM, + PerformanceWidgetSetting.P50_DURATION_AREA, + PerformanceWidgetSetting.P75_DURATION_AREA, + PerformanceWidgetSetting.P95_DURATION_AREA, + PerformanceWidgetSetting.P99_DURATION_AREA, + PerformanceWidgetSetting.FAILURE_RATE_AREA, + ], + mepSetting + ); const sharedProps = {eventView, location, organization, withStaticFilters}; diff --git a/static/app/views/insights/pages/backend/backendOverviewPage.tsx b/static/app/views/insights/pages/backend/backendOverviewPage.tsx index 3ab775bfe0601d..283ab5b72c6214 100644 --- a/static/app/views/insights/pages/backend/backendOverviewPage.tsx +++ b/static/app/views/insights/pages/backend/backendOverviewPage.tsx @@ -12,7 +12,10 @@ import {ProjectPageFilter} from 'sentry/components/organizations/projectPageFilt import TransactionNameSearchBar from 'sentry/components/performance/searchBar'; import SentryDocumentTitle from 'sentry/components/sentryDocumentTitle'; import {trackAnalytics} from 'sentry/utils/analytics'; -import {canUseMetricsData} from 'sentry/utils/performance/contexts/metricsEnhancedSetting'; +import { + canUseMetricsData, + useMEPSettingContext, +} from 'sentry/utils/performance/contexts/metricsEnhancedSetting'; import {PageAlert, usePageAlert} from 'sentry/utils/performance/contexts/pageAlert'; import {PerformanceDisplayProvider} from 'sentry/utils/performance/contexts/performanceDisplayContext'; import {MutableSearch} from 'sentry/utils/tokenizeSearch'; @@ -31,6 +34,7 @@ import { DoubleChartRow, TripleChartRow, } from 'sentry/views/performance/landing/widgets/components/widgetChartRow'; +import {filterAllowedChartsMetrics} from 'sentry/views/performance/landing/widgets/utils'; import {PerformanceWidgetSetting} from 'sentry/views/performance/landing/widgets/widgetDefinitions'; import Onboarding from 'sentry/views/performance/onboarding'; import Table from 'sentry/views/performance/table'; @@ -60,6 +64,7 @@ function BackendOverviewPage() { const {projects} = useProjects(); const onboardingProject = useOnboardingProject(); const navigate = useNavigate(); + const mepSetting = useMEPSettingContext(); const withStaticFilters = canUseMetricsData(organization); const eventView = generateBackendPerformanceEventView( @@ -91,16 +96,20 @@ function BackendOverviewPage() { PerformanceWidgetSetting.SLOW_HTTP_OPS, PerformanceWidgetSetting.SLOW_DB_OPS, ]; - const tripleChartRowCharts = [ - PerformanceWidgetSetting.TPM_AREA, - PerformanceWidgetSetting.DURATION_HISTOGRAM, - PerformanceWidgetSetting.P50_DURATION_AREA, - PerformanceWidgetSetting.P75_DURATION_AREA, - PerformanceWidgetSetting.P95_DURATION_AREA, - PerformanceWidgetSetting.P99_DURATION_AREA, - PerformanceWidgetSetting.FAILURE_RATE_AREA, - PerformanceWidgetSetting.APDEX_AREA, - ]; + const tripleChartRowCharts = filterAllowedChartsMetrics( + organization, + [ + PerformanceWidgetSetting.TPM_AREA, + PerformanceWidgetSetting.DURATION_HISTOGRAM, + PerformanceWidgetSetting.P50_DURATION_AREA, + PerformanceWidgetSetting.P75_DURATION_AREA, + PerformanceWidgetSetting.P95_DURATION_AREA, + PerformanceWidgetSetting.P99_DURATION_AREA, + PerformanceWidgetSetting.FAILURE_RATE_AREA, + PerformanceWidgetSetting.APDEX_AREA, + ], + mepSetting + ); if (organization.features.includes('insights-initial-modules')) { doubleChartRowCharts.unshift( diff --git a/static/app/views/insights/pages/frontend/frontendOverviewPage.tsx b/static/app/views/insights/pages/frontend/frontendOverviewPage.tsx index 883c4c6fabb39a..a3424c4f760c42 100644 --- a/static/app/views/insights/pages/frontend/frontendOverviewPage.tsx +++ b/static/app/views/insights/pages/frontend/frontendOverviewPage.tsx @@ -12,7 +12,10 @@ import {ProjectPageFilter} from 'sentry/components/organizations/projectPageFilt import TransactionNameSearchBar from 'sentry/components/performance/searchBar'; import SentryDocumentTitle from 'sentry/components/sentryDocumentTitle'; import {trackAnalytics} from 'sentry/utils/analytics'; -import {canUseMetricsData} from 'sentry/utils/performance/contexts/metricsEnhancedSetting'; +import { + canUseMetricsData, + useMEPSettingContext, +} from 'sentry/utils/performance/contexts/metricsEnhancedSetting'; import {PageAlert, usePageAlert} from 'sentry/utils/performance/contexts/pageAlert'; import {PerformanceDisplayProvider} from 'sentry/utils/performance/contexts/performanceDisplayContext'; import {MutableSearch} from 'sentry/utils/tokenizeSearch'; @@ -32,6 +35,7 @@ import { DoubleChartRow, TripleChartRow, } from 'sentry/views/performance/landing/widgets/components/widgetChartRow'; +import {filterAllowedChartsMetrics} from 'sentry/views/performance/landing/widgets/utils'; import {PerformanceWidgetSetting} from 'sentry/views/performance/landing/widgets/widgetDefinitions'; import Onboarding from 'sentry/views/performance/onboarding'; import Table from 'sentry/views/performance/table'; @@ -59,6 +63,7 @@ function FrontendOverviewPage() { const {projects} = useProjects(); const onboardingProject = useOnboardingProject(); const navigate = useNavigate(); + const mepSetting = useMEPSettingContext(); const withStaticFilters = canUseMetricsData(organization); const eventView = generateFrontendOtherPerformanceEventView( @@ -94,15 +99,19 @@ function FrontendOverviewPage() { PerformanceWidgetSetting.SLOW_HTTP_OPS, PerformanceWidgetSetting.SLOW_RESOURCE_OPS, ]; - const tripleChartRowCharts = [ - PerformanceWidgetSetting.TPM_AREA, - PerformanceWidgetSetting.DURATION_HISTOGRAM, - PerformanceWidgetSetting.P50_DURATION_AREA, - PerformanceWidgetSetting.P75_DURATION_AREA, - PerformanceWidgetSetting.P95_DURATION_AREA, - PerformanceWidgetSetting.P99_DURATION_AREA, - PerformanceWidgetSetting.FAILURE_RATE_AREA, - ]; + const tripleChartRowCharts = filterAllowedChartsMetrics( + organization, + [ + PerformanceWidgetSetting.TPM_AREA, + PerformanceWidgetSetting.DURATION_HISTOGRAM, + PerformanceWidgetSetting.P50_DURATION_AREA, + PerformanceWidgetSetting.P75_DURATION_AREA, + PerformanceWidgetSetting.P95_DURATION_AREA, + PerformanceWidgetSetting.P99_DURATION_AREA, + PerformanceWidgetSetting.FAILURE_RATE_AREA, + ], + mepSetting + ); if (organization.features.includes('insights-initial-modules')) { doubleChartRowCharts.unshift(PerformanceWidgetSetting.MOST_TIME_CONSUMING_DOMAINS); diff --git a/static/app/views/insights/pages/mobile/mobileOverviewPage.tsx b/static/app/views/insights/pages/mobile/mobileOverviewPage.tsx index 3a60c01d968e03..2339fd5c82a663 100644 --- a/static/app/views/insights/pages/mobile/mobileOverviewPage.tsx +++ b/static/app/views/insights/pages/mobile/mobileOverviewPage.tsx @@ -11,7 +11,10 @@ import {ProjectPageFilter} from 'sentry/components/organizations/projectPageFilt import TransactionNameSearchBar from 'sentry/components/performance/searchBar'; import SentryDocumentTitle from 'sentry/components/sentryDocumentTitle'; import {trackAnalytics} from 'sentry/utils/analytics'; -import {canUseMetricsData} from 'sentry/utils/performance/contexts/metricsEnhancedSetting'; +import { + canUseMetricsData, + useMEPSettingContext, +} from 'sentry/utils/performance/contexts/metricsEnhancedSetting'; import {PageAlert, usePageAlert} from 'sentry/utils/performance/contexts/pageAlert'; import {PerformanceDisplayProvider} from 'sentry/utils/performance/contexts/performanceDisplayContext'; import {MutableSearch} from 'sentry/utils/tokenizeSearch'; @@ -35,6 +38,7 @@ import { DoubleChartRow, TripleChartRow, } from 'sentry/views/performance/landing/widgets/components/widgetChartRow'; +import {filterAllowedChartsMetrics} from 'sentry/views/performance/landing/widgets/utils'; import {PerformanceWidgetSetting} from 'sentry/views/performance/landing/widgets/widgetDefinitions'; import Onboarding from 'sentry/views/performance/onboarding'; import Table from 'sentry/views/performance/table'; @@ -73,6 +77,7 @@ function MobileOverviewPage() { const {projects} = useProjects(); const onboardingProject = useOnboardingProject(); const navigate = useNavigate(); + const mepSetting = useMEPSettingContext(); const withStaticFilters = canUseMetricsData(organization); @@ -94,15 +99,19 @@ function MobileOverviewPage() { PerformanceWidgetSetting.MOST_SLOW_FRAMES, PerformanceWidgetSetting.MOST_FROZEN_FRAMES, ]; - const tripleChartRowCharts = [ - PerformanceWidgetSetting.TPM_AREA, - PerformanceWidgetSetting.DURATION_HISTOGRAM, - PerformanceWidgetSetting.P50_DURATION_AREA, - PerformanceWidgetSetting.P75_DURATION_AREA, - PerformanceWidgetSetting.P95_DURATION_AREA, - PerformanceWidgetSetting.P99_DURATION_AREA, - PerformanceWidgetSetting.FAILURE_RATE_AREA, - ]; + const tripleChartRowCharts = filterAllowedChartsMetrics( + organization, + [ + PerformanceWidgetSetting.TPM_AREA, + PerformanceWidgetSetting.DURATION_HISTOGRAM, + PerformanceWidgetSetting.P50_DURATION_AREA, + PerformanceWidgetSetting.P75_DURATION_AREA, + PerformanceWidgetSetting.P95_DURATION_AREA, + PerformanceWidgetSetting.P99_DURATION_AREA, + PerformanceWidgetSetting.FAILURE_RATE_AREA, + ], + mepSetting + ); if (organization.features.includes('mobile-vitals')) { columnTitles = [...columnTitles.slice(0, 5), 'ttid', ...columnTitles.slice(5, 0)]; From 9c03c0e7d08779d8045629b9e5474e852aaf8c54 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 16 Oct 2024 12:48:43 -0700 Subject: [PATCH 45/67] chore(alerts): Remove noisy alert warning flag (#79033) This flag has been released for a long time now and can be removed. Removing in options automator [here](https://github.com/getsentry/sentry-options-automator/pull/2478). I'll follow up to remove the flag after these are both merged. --- src/sentry/features/temporary.py | 2 +- static/app/views/alerts/create.spec.tsx | 5 ++--- static/app/views/alerts/rules/issue/index.tsx | 1 - tests/sentry/api/serializers/test_organization.py | 1 + 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/sentry/features/temporary.py b/src/sentry/features/temporary.py index b5e315e1aeb6e3..c147fe42d052a4 100644 --- a/src/sentry/features/temporary.py +++ b/src/sentry/features/temporary.py @@ -214,7 +214,7 @@ def register_temporary_features(manager: FeatureManager): manager.add("organizations:navigation-sidebar-v2", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True) manager.add("organizations:new-page-filter", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, default=True, api_expose=True) # Display warning banner for every event issue alerts - manager.add("organizations:noisy-alert-warning", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True) + manager.add("organizations:noisy-alert-warning", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True, default=True) # Notify all project members when fallthrough is disabled, instead of just the auto-assignee manager.add("organizations:notification-all-recipients", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) # Drop obsoleted status changes in occurence consumer diff --git a/static/app/views/alerts/create.spec.tsx b/static/app/views/alerts/create.spec.tsx index dbf6c44e4c388a..cb3ce94dd880aa 100644 --- a/static/app/views/alerts/create.spec.tsx +++ b/static/app/views/alerts/create.spec.tsx @@ -678,8 +678,7 @@ describe('ProjectAlertsCreate', function () { method: 'POST', body: ProjectAlertRuleFixture(), }); - - createWrapper({organization: {features: ['noisy-alert-warning']}}); + createWrapper(); await userEvent.click((await screen.findAllByLabelText('Delete Node'))[0]); await selectEvent.select(screen.getByText('Add action...'), [ @@ -711,7 +710,7 @@ describe('ProjectAlertsCreate', function () { }); it('does not display noisy alert banner for legacy integrations', async function () { - createWrapper({organization: {features: ['noisy-alert-warning']}}); + createWrapper(); await userEvent.click((await screen.findAllByLabelText('Delete Node'))[0]); await selectEvent.select(screen.getByText('Add action...'), [ diff --git a/static/app/views/alerts/rules/issue/index.tsx b/static/app/views/alerts/rules/issue/index.tsx index ad43574e022d60..852c2ca40be75f 100644 --- a/static/app/views/alerts/rules/issue/index.tsx +++ b/static/app/views/alerts/rules/issue/index.tsx @@ -926,7 +926,6 @@ class IssueRuleEditor extends DeprecatedAsyncView { ]; return ( - this.props.organization.features.includes('noisy-alert-warning') && !!rule && !isSavedAlertRule(rule) && rule.conditions.length === 0 && diff --git a/tests/sentry/api/serializers/test_organization.py b/tests/sentry/api/serializers/test_organization.py index d7f5f3a2275034..56a8cdc75694dc 100644 --- a/tests/sentry/api/serializers/test_organization.py +++ b/tests/sentry/api/serializers/test_organization.py @@ -88,6 +88,7 @@ def test_simple(self): "invite-members", "minute-resolution-sessions", "new-page-filter", + "noisy-alert-warning", "open-membership", "relay", "shared-issues", From af0b3e8e036899f781797b4db35e80bb9d6e36dd Mon Sep 17 00:00:00 2001 From: Lyn Nagara <1779792+lynnagara@users.noreply.github.com> Date: Wed, 16 Oct 2024 12:54:55 -0700 Subject: [PATCH 46/67] ref: clean up queue_name from save_event_transaction (#79205) routing for this task is done by the split queue task router now, remove the hardcoded queue name --- src/sentry/tasks/store.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/sentry/tasks/store.py b/src/sentry/tasks/store.py index 02c1b74183b4a7..c2fe5942144098 100644 --- a/src/sentry/tasks/store.py +++ b/src/sentry/tasks/store.py @@ -609,7 +609,6 @@ def save_event( @instrumented_task( name="sentry.tasks.store.save_event_transaction", - queue="events.save_event_transaction", time_limit=65, soft_time_limit=60, silo_mode=SiloMode.REGION, From ace5b846bb27805abd43b122b0f19d1b306db888 Mon Sep 17 00:00:00 2001 From: getsentry-bot Date: Wed, 16 Oct 2024 20:00:06 +0000 Subject: [PATCH 47/67] Revert "ref: clean up queue_name from save_event_transaction (#79205)" This reverts commit af0b3e8e036899f781797b4db35e80bb9d6e36dd. Co-authored-by: lynnagara <1779792+lynnagara@users.noreply.github.com> --- src/sentry/tasks/store.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sentry/tasks/store.py b/src/sentry/tasks/store.py index c2fe5942144098..02c1b74183b4a7 100644 --- a/src/sentry/tasks/store.py +++ b/src/sentry/tasks/store.py @@ -609,6 +609,7 @@ def save_event( @instrumented_task( name="sentry.tasks.store.save_event_transaction", + queue="events.save_event_transaction", time_limit=65, soft_time_limit=60, silo_mode=SiloMode.REGION, From e2df3f1a1457a43dcc2a7b2f3944627a97522262 Mon Sep 17 00:00:00 2001 From: Christinarlong <60594860+Christinarlong@users.noreply.github.com> Date: Wed, 16 Oct 2024 13:08:00 -0700 Subject: [PATCH 48/67] ref(mediators): Make validator into a dataclass (#79116) --- src/sentry/mediators/__init__.py | 1 - .../mediators/token_exchange/__init__.py | 2 - .../endpoints/sentry_app_authorizations.py | 2 +- .../token_exchange/grant_exchanger.py | 24 ++++++------ .../sentry_apps/token_exchange/refresher.py | 13 ++++--- .../token_exchange/util.py | 0 .../token_exchange/validator.py | 39 ++++++++++--------- src/sentry/web/frontend/oauth_token.py | 10 +++-- .../test_sentry_app_authorizations.py | 2 +- .../token_exchange/test_grant_exchanger.py | 8 ---- .../token_exchange/test_refresher.py | 8 ---- .../token_exchange/test_validator.py | 14 +++---- 12 files changed, 54 insertions(+), 69 deletions(-) rename src/sentry/{mediators => sentry_apps}/token_exchange/util.py (100%) rename src/sentry/{mediators => sentry_apps}/token_exchange/validator.py (55%) rename tests/sentry/{mediators => sentry_apps}/token_exchange/test_validator.py (86%) diff --git a/src/sentry/mediators/__init__.py b/src/sentry/mediators/__init__.py index df677f82741301..8e848d43bc83e3 100644 --- a/src/sentry/mediators/__init__.py +++ b/src/sentry/mediators/__init__.py @@ -1,3 +1,2 @@ from .mediator import Mediator # NOQA from .param import Param # NOQA -from .token_exchange.util import AUTHORIZATION, REFRESH, GrantTypes # noqa: F401 diff --git a/src/sentry/mediators/token_exchange/__init__.py b/src/sentry/mediators/token_exchange/__init__.py index 46a4f7637503ea..e69de29bb2d1d6 100644 --- a/src/sentry/mediators/token_exchange/__init__.py +++ b/src/sentry/mediators/token_exchange/__init__.py @@ -1,2 +0,0 @@ -from .util import AUTHORIZATION, REFRESH, GrantTypes, token_expiration # NOQA -from .validator import Validator # NOQA diff --git a/src/sentry/sentry_apps/api/endpoints/sentry_app_authorizations.py b/src/sentry/sentry_apps/api/endpoints/sentry_app_authorizations.py index 1ff39ed50c16f4..2bc2d56a81701f 100644 --- a/src/sentry/sentry_apps/api/endpoints/sentry_app_authorizations.py +++ b/src/sentry/sentry_apps/api/endpoints/sentry_app_authorizations.py @@ -10,10 +10,10 @@ from sentry.api.serializers.models.apitoken import ApiTokenSerializer from sentry.auth.services.auth.impl import promote_request_api_user from sentry.coreapi import APIUnauthorized -from sentry.mediators.token_exchange.util import GrantTypes from sentry.sentry_apps.api.bases.sentryapps import SentryAppAuthorizationsBaseEndpoint from sentry.sentry_apps.token_exchange.grant_exchanger import GrantExchanger from sentry.sentry_apps.token_exchange.refresher import Refresher +from sentry.sentry_apps.token_exchange.util import GrantTypes logger = logging.getLogger(__name__) diff --git a/src/sentry/sentry_apps/token_exchange/grant_exchanger.py b/src/sentry/sentry_apps/token_exchange/grant_exchanger.py index 589a3126ec0785..6a6089c2f4f514 100644 --- a/src/sentry/sentry_apps/token_exchange/grant_exchanger.py +++ b/src/sentry/sentry_apps/token_exchange/grant_exchanger.py @@ -6,14 +6,14 @@ from sentry import analytics from sentry.coreapi import APIUnauthorized -from sentry.mediators.token_exchange.util import token_expiration -from sentry.mediators.token_exchange.validator import Validator from sentry.models.apiapplication import ApiApplication from sentry.models.apigrant import ApiGrant from sentry.models.apitoken import ApiToken from sentry.sentry_apps.models.sentry_app import SentryApp from sentry.sentry_apps.models.sentry_app_installation import SentryAppInstallation from sentry.sentry_apps.services.app import RpcSentryAppInstallation +from sentry.sentry_apps.token_exchange.util import token_expiration +from sentry.sentry_apps.token_exchange.validator import Validator from sentry.silo.safety import unguarded_write from sentry.users.models.user import User @@ -31,15 +31,14 @@ class GrantExchanger: def run(self): with transaction.atomic(using=router.db_for_write(ApiToken)): - self._validate() - token = self._create_token() + if self._validate(): + token = self._create_token() - # Once it's exchanged it's no longer valid and should not be - # exchangeable, so we delete it. - self._delete_grant() - self.record_analytics() - - return token + # Once it's exchanged it's no longer valid and should not be + # exchangeable, so we delete it. + self._delete_grant() + self.record_analytics() + return token def record_analytics(self) -> None: analytics.record( @@ -48,14 +47,15 @@ def record_analytics(self) -> None: exchange_type="authorization", ) - def _validate(self) -> None: - Validator.run(install=self.install, client_id=self.client_id, user=self.user) + def _validate(self) -> bool: + is_valid = Validator(install=self.install, client_id=self.client_id, user=self.user).run() if not self._grant_belongs_to_install() or not self._sentry_app_user_owns_grant(): raise APIUnauthorized("Forbidden grant") if not self._grant_is_active(): raise APIUnauthorized("Grant has already expired.") + return is_valid def _grant_belongs_to_install(self) -> bool: return self.grant.sentry_app_installation.id == self.install.id diff --git a/src/sentry/sentry_apps/token_exchange/refresher.py b/src/sentry/sentry_apps/token_exchange/refresher.py index f6881700eb09ab..9b8ed3364e00f7 100644 --- a/src/sentry/sentry_apps/token_exchange/refresher.py +++ b/src/sentry/sentry_apps/token_exchange/refresher.py @@ -5,13 +5,13 @@ from sentry import analytics from sentry.coreapi import APIUnauthorized -from sentry.mediators.token_exchange.util import token_expiration -from sentry.mediators.token_exchange.validator import Validator from sentry.models.apiapplication import ApiApplication from sentry.models.apitoken import ApiToken from sentry.sentry_apps.models.sentry_app import SentryApp from sentry.sentry_apps.models.sentry_app_installation import SentryAppInstallation from sentry.sentry_apps.services.app import RpcSentryAppInstallation +from sentry.sentry_apps.token_exchange.util import token_expiration +from sentry.sentry_apps.token_exchange.validator import Validator from sentry.users.models.user import User @@ -28,8 +28,8 @@ class Refresher: def run(self) -> ApiToken: with transaction.atomic(router.db_for_write(ApiToken)): - self._validate() - self.token.delete() + if self._validate(): + self.token.delete() self.record_analytics() return self._create_new_token() @@ -41,11 +41,12 @@ def record_analytics(self) -> None: exchange_type="refresh", ) - def _validate(self) -> None: - Validator.run(install=self.install, client_id=self.client_id, user=self.user) + def _validate(self) -> bool: + is_valid = Validator(install=self.install, client_id=self.client_id, user=self.user).run() if self.token.application != self.application: raise APIUnauthorized("Token does not belong to the application") + return is_valid def _create_new_token(self) -> ApiToken: token = ApiToken.objects.create( diff --git a/src/sentry/mediators/token_exchange/util.py b/src/sentry/sentry_apps/token_exchange/util.py similarity index 100% rename from src/sentry/mediators/token_exchange/util.py rename to src/sentry/sentry_apps/token_exchange/util.py diff --git a/src/sentry/mediators/token_exchange/validator.py b/src/sentry/sentry_apps/token_exchange/validator.py similarity index 55% rename from src/sentry/mediators/token_exchange/validator.py rename to src/sentry/sentry_apps/token_exchange/validator.py index 4b88fac49e4e3b..6d0cf170dcd37a 100644 --- a/src/sentry/mediators/token_exchange/validator.py +++ b/src/sentry/sentry_apps/token_exchange/validator.py @@ -1,53 +1,54 @@ -from django.db import router +from dataclasses import dataclass + from django.utils.functional import cached_property from sentry.coreapi import APIUnauthorized -from sentry.mediators.mediator import Mediator -from sentry.mediators.param import Param from sentry.models.apiapplication import ApiApplication from sentry.sentry_apps.models.sentry_app import SentryApp from sentry.sentry_apps.services.app import RpcSentryAppInstallation from sentry.users.models.user import User -class Validator(Mediator): +@dataclass +class Validator: """ Validates general authorization params for all types of token exchanges. """ - install = Param(RpcSentryAppInstallation) - client_id = Param(str) - user = Param(User) - using = router.db_for_write(User) + install: RpcSentryAppInstallation + client_id: str + user: User - def call(self): + def run(self) -> bool: self._validate_is_sentry_app_making_request() self._validate_app_is_owned_by_user() self._validate_installation() return True - def _validate_is_sentry_app_making_request(self): + def _validate_is_sentry_app_making_request(self) -> None: if not self.user.is_sentry_app: - raise APIUnauthorized + raise APIUnauthorized("User is not a Sentry App") - def _validate_app_is_owned_by_user(self): + def _validate_app_is_owned_by_user(self) -> None: if self.sentry_app.proxy_user != self.user: - raise APIUnauthorized + raise APIUnauthorized("Sentry App does not belong to given user") - def _validate_installation(self): + def _validate_installation(self) -> None: if self.install.sentry_app.id != self.sentry_app.id: - raise APIUnauthorized + raise APIUnauthorized( + f"Sentry App Installation is not for Sentry App: {self.sentry_app.slug}" + ) @cached_property - def sentry_app(self): + def sentry_app(self) -> SentryApp: try: return self.application.sentry_app except SentryApp.DoesNotExist: - raise APIUnauthorized + raise APIUnauthorized("Sentry App does not exist") @cached_property - def application(self): + def application(self) -> ApiApplication: try: return ApiApplication.objects.get(client_id=self.client_id) except ApiApplication.DoesNotExist: - raise APIUnauthorized + raise APIUnauthorized("Application does not exist") diff --git a/src/sentry/web/frontend/oauth_token.py b/src/sentry/web/frontend/oauth_token.py index ee8dea005f5a6f..109babbecf7a26 100644 --- a/src/sentry/web/frontend/oauth_token.py +++ b/src/sentry/web/frontend/oauth_token.py @@ -9,10 +9,10 @@ from rest_framework.request import Request from sentry import options -from sentry.mediators.token_exchange.util import GrantTypes from sentry.models.apiapplication import ApiApplication, ApiApplicationStatus from sentry.models.apigrant import ApiGrant from sentry.models.apitoken import ApiToken +from sentry.sentry_apps.token_exchange.util import GrantTypes from sentry.utils import json, metrics from sentry.web.frontend.base import control_silo_view from sentry.web.frontend.openidtoken import OpenIDToken @@ -157,9 +157,11 @@ def process_token_details( token_information = { "access_token": token.token, "refresh_token": token.refresh_token, - "expires_in": int((token.expires_at - timezone.now()).total_seconds()) - if token.expires_at - else None, + "expires_in": ( + int((token.expires_at - timezone.now()).total_seconds()) + if token.expires_at + else None + ), "expires_at": token.expires_at, "token_type": "bearer", "scope": " ".join(token.get_scopes()), diff --git a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_authorizations.py b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_authorizations.py index 9836e317648cfe..7ea1fcb1941648 100644 --- a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_authorizations.py +++ b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_authorizations.py @@ -3,9 +3,9 @@ from django.urls import reverse from django.utils import timezone -from sentry.mediators.token_exchange.util import GrantTypes from sentry.models.apiapplication import ApiApplication from sentry.models.apitoken import ApiToken +from sentry.sentry_apps.token_exchange.util import GrantTypes from sentry.silo.base import SiloMode from sentry.testutils.cases import APITestCase from sentry.testutils.silo import assume_test_silo_mode, control_silo_test diff --git a/tests/sentry/sentry_apps/token_exchange/test_grant_exchanger.py b/tests/sentry/sentry_apps/token_exchange/test_grant_exchanger.py index 4cc154720867d7..2385ab75cf14f5 100644 --- a/tests/sentry/sentry_apps/token_exchange/test_grant_exchanger.py +++ b/tests/sentry/sentry_apps/token_exchange/test_grant_exchanger.py @@ -35,14 +35,6 @@ def test_adds_token_to_installation(self): token = self.grant_exchanger.run() assert SentryAppInstallation.objects.get(id=self.install.id).api_token == token - @patch("sentry.mediators.token_exchange.Validator.run") - def test_validate_generic_token_exchange_requirements(self, validator): - self.grant_exchanger.run() - - validator.assert_called_once_with( - install=self.install, client_id=self.client_id, user=self.user - ) - def test_grant_must_belong_to_installations(self): other_install = self.create_sentry_app_installation(prevent_token_exchange=True) self.grant_exchanger.code = other_install.api_grant.code diff --git a/tests/sentry/sentry_apps/token_exchange/test_refresher.py b/tests/sentry/sentry_apps/token_exchange/test_refresher.py index 6ff4723803cf89..3624b56895129a 100644 --- a/tests/sentry/sentry_apps/token_exchange/test_refresher.py +++ b/tests/sentry/sentry_apps/token_exchange/test_refresher.py @@ -42,14 +42,6 @@ def test_deletes_refreshed_token(self): self.refresher.run() assert not ApiToken.objects.filter(id=self.token.id).exists() - @patch("sentry.mediators.token_exchange.Validator.run") - def test_validates_generic_token_exchange_requirements(self, validator): - self.refresher.run() - - validator.assert_called_once_with( - install=self.install, client_id=self.client_id, user=self.user - ) - def test_validates_token_belongs_to_sentry_app(self): refresh_token = ApiToken.objects.create( user=self.user, diff --git a/tests/sentry/mediators/token_exchange/test_validator.py b/tests/sentry/sentry_apps/token_exchange/test_validator.py similarity index 86% rename from tests/sentry/mediators/token_exchange/test_validator.py rename to tests/sentry/sentry_apps/token_exchange/test_validator.py index 1d81eaae8f892e..d3e921bcb303e0 100644 --- a/tests/sentry/mediators/token_exchange/test_validator.py +++ b/tests/sentry/sentry_apps/token_exchange/test_validator.py @@ -3,9 +3,9 @@ import pytest from sentry.coreapi import APIUnauthorized -from sentry.mediators.token_exchange.validator import Validator from sentry.sentry_apps.models.sentry_app import SentryApp from sentry.sentry_apps.services.app import app_service +from sentry.sentry_apps.token_exchange.validator import Validator from sentry.testutils.cases import TestCase from sentry.testutils.silo import control_silo_test @@ -24,35 +24,35 @@ def setUp(self): ) def test_happy_path(self): - assert self.validator.call() + assert self.validator.run() def test_request_must_be_made_by_sentry_app(self): self.validator.user = self.create_user() with pytest.raises(APIUnauthorized): - self.validator.call() + self.validator.run() def test_request_user_must_own_sentry_app(self): self.validator.user = self.create_user(is_sentry_app=True) with pytest.raises(APIUnauthorized): - self.validator.call() + self.validator.run() def test_installation_belongs_to_sentry_app_with_client_id(self): self.validator.install = self.create_sentry_app_installation() with pytest.raises(APIUnauthorized): - self.validator.call() + self.validator.run() @patch("sentry.models.ApiApplication.sentry_app") def test_raises_when_sentry_app_cannot_be_found(self, sentry_app): sentry_app.side_effect = SentryApp.DoesNotExist() with pytest.raises(APIUnauthorized): - self.validator.call() + self.validator.run() def test_raises_with_invalid_client_id(self): self.validator.client_id = "123" with pytest.raises(APIUnauthorized): - self.validator.call() + self.validator.run() From ddb945d49267bcd0ad085b6f11f872f4747ba23b Mon Sep 17 00:00:00 2001 From: Scott Cooper Date: Wed, 16 Oct 2024 13:19:10 -0700 Subject: [PATCH 49/67] ref(members): Convert member settings to FC (#78365) --- .../organizationMemberDetail.spec.tsx | 426 +++++++------ .../organizationMemberDetail.tsx | 591 +++++++++--------- 2 files changed, 505 insertions(+), 512 deletions(-) diff --git a/static/app/views/settings/organizationMembers/organizationMemberDetail.spec.tsx b/static/app/views/settings/organizationMembers/organizationMemberDetail.spec.tsx index caf0a0a2a63764..45fc1e2e12f627 100644 --- a/static/app/views/settings/organizationMembers/organizationMemberDetail.spec.tsx +++ b/static/app/views/settings/organizationMembers/organizationMemberDetail.spec.tsx @@ -18,7 +18,9 @@ import selectEvent from 'sentry-test/selectEvent'; import {updateMember} from 'sentry/actionCreators/members'; import TeamStore from 'sentry/stores/teamStore'; -import OrganizationMemberDetail from 'sentry/views/settings/organizationMembers/organizationMemberDetail'; +import type {Member} from 'sentry/types/organization'; + +import OrganizationMemberDetail from './organizationMemberDetail'; jest.mock('sentry/actionCreators/members', () => ({ updateMember: jest.fn().mockReturnValue(new Promise(() => {})), @@ -138,18 +140,18 @@ describe('OrganizationMemberDetail', function () { }); it('changes org role to owner', async function () { - const {router, routerProps} = initializeOrg({organization}); + const {router} = initializeOrg({ + organization, + router: {params: {memberId: member.id}}, + }); - render( - , - { - router, - organization, - } - ); + render(, { + router, + organization, + }); - // Should have 4 roles - const radios = screen.getAllByRole('radio'); + // Should have 5 roles + const radios = await screen.findAllByRole('radio'); expect(radios).toHaveLength(5); // Click last radio @@ -170,18 +172,18 @@ describe('OrganizationMemberDetail', function () { }); it('leaves a team', async function () { - const {router, routerProps} = initializeOrg({organization}); + const {router} = initializeOrg({ + organization, + router: {params: {memberId: member.id}}, + }); - render( - , - { - router, - organization, - } - ); + render(, { + router, + organization, + }); // Remove our one team - await userEvent.click(screen.getByRole('button', {name: 'Remove'})); + await userEvent.click(await screen.findByRole('button', {name: 'Remove'})); // Save Member await userEvent.click(screen.getByRole('button', {name: 'Save Member'})); @@ -196,36 +198,33 @@ describe('OrganizationMemberDetail', function () { ); }); - it('cannot leave idp-provisioned team', function () { - const {router, routerProps} = initializeOrg({organization}); + it('cannot leave idp-provisioned team', async function () { + const {router} = initializeOrg({ + organization, + router: {params: {memberId: idpTeamMember.id}}, + }); - render( - , - { - router, - organization, - } - ); + render(, { + router, + organization, + }); - expect(screen.getByRole('button', {name: 'Remove'})).toBeDisabled(); + expect(await screen.findByRole('button', {name: 'Remove'})).toBeDisabled(); }); it('joins a team and assign a team-role', async function () { - const {router, routerProps} = initializeOrg({organization}); + const {router} = initializeOrg({ + organization, + router: {params: {memberId: member.id}}, + }); - render( - , - { - router, - organization, - } - ); + render(, { + router, + organization, + }); // Should have one team enabled - expect(screen.getByTestId('team-row-for-member')).toBeInTheDocument(); + expect(await screen.findByTestId('team-row-for-member')).toBeInTheDocument(); // Select new team to join // Open the dropdown @@ -254,17 +253,17 @@ describe('OrganizationMemberDetail', function () { }); it('cannot join idp-provisioned team', async function () { - const {router, routerProps} = initializeOrg({organization}); + const {router} = initializeOrg({ + organization, + router: {params: {memberId: member.id}}, + }); - render( - , - { - router, - organization, - } - ); + render(, { + router, + organization, + }); - await userEvent.click(screen.getByText('Add Team')); + await userEvent.click(await screen.findByText('Add Team')); await userEvent.hover(screen.getByText('#idp-member-team')); expect( await screen.findByText( @@ -301,19 +300,19 @@ describe('OrganizationMemberDetail', function () { }); }); - it('can not change roles, teams, or save', function () { - const {router, routerProps} = initializeOrg({organization}); + it('can not change roles, teams, or save', async function () { + const {router} = initializeOrg({ + organization, + router: {params: {memberId: member.id}}, + }); - render( - , - { - router, - organization, - } - ); + render(, { + router, + organization, + }); // Should have 4 roles - const radios = screen.getAllByRole('radio'); + const radios = await screen.findAllByRole('radio'); expect(radios.at(0)).toHaveAttribute('readonly'); // Save Member @@ -348,38 +347,36 @@ describe('OrganizationMemberDetail', function () { }); }); - it('display pending status', function () { - const {router, routerProps} = initializeOrg({organization}); + it('display pending status', async function () { + const {router} = initializeOrg({ + organization, + router: {params: {memberId: pendingMember.id}}, + }); - render( - , - { - router, - organization, - } - ); + render(, { + router, + organization, + }); - expect(screen.getByTestId('member-status')).toHaveTextContent('Invitation Pending'); + expect(await screen.findByTestId('member-status')).toHaveTextContent( + 'Invitation Pending' + ); }); - it('display expired status', function () { - const {router, routerProps} = initializeOrg({organization}); + it('display expired status', async function () { + const {router} = initializeOrg({ + organization, + router: {params: {memberId: expiredMember.id}}, + }); - render( - , - { - router, - organization, - } - ); + render(, { + router, + organization, + }); - expect(screen.getByTestId('member-status')).toHaveTextContent('Invitation Expired'); + expect(await screen.findByTestId('member-status')).toHaveTextContent( + 'Invitation Expired' + ); }); }); @@ -410,37 +407,34 @@ describe('OrganizationMemberDetail', function () { }); }); - it('shows for pending', function () { - const {router, routerProps} = initializeOrg({organization}); + it('shows for pending', async function () { + const {router} = initializeOrg({ + organization, + router: {params: {memberId: pendingMember.id}}, + }); - render( - , - { - router, - organization, - } - ); + render(, { + router, + organization, + }); - expect(screen.getByRole('button', {name: 'Resend Invite'})).toBeInTheDocument(); + expect( + await screen.findByRole('button', {name: 'Resend Invite'}) + ).toBeInTheDocument(); }); - it('does not show for expired', function () { - const {router, routerProps} = initializeOrg({organization}); + it('does not show for expired', async function () { + const {router} = initializeOrg({ + organization, + router: {params: {memberId: expiredMember.id}}, + }); - render( - , - { - router, - organization, - } - ); + render(, { + router, + organization, + }); + await screen.findAllByRole('radio'); expect( screen.queryByRole('button', {name: 'Resend Invite'}) ).not.toBeInTheDocument(); @@ -520,69 +514,79 @@ describe('OrganizationMemberDetail', function () { }); }); - const button = () => - screen.queryByRole('button', {name: 'Reset two-factor authentication'}); const tooltip = () => screen.queryByTestId('reset-2fa-tooltip'); - const expectButtonEnabled = () => { - expect(button()).toHaveTextContent('Reset two-factor authentication'); - expect(button()).toBeEnabled(); + const expectButtonEnabled = async () => { + const button = await screen.findByRole('button', { + name: 'Reset two-factor authentication', + }); + expect(button).toHaveTextContent('Reset two-factor authentication'); + expect(button).toBeEnabled(); expect(tooltip()).not.toBeInTheDocument(); }; - const expectButtonDisabled = async title => { - expect(button()).toHaveTextContent('Reset two-factor authentication'); - expect(button()).toBeDisabled(); + const expectButtonDisabled = async (title: string) => { + const button = await screen.findByRole('button', { + name: 'Reset two-factor authentication', + }); + expect(button).toHaveTextContent('Reset two-factor authentication'); + expect(button).toBeDisabled(); - await userEvent.hover(button() as Element); + await userEvent.hover(button); expect(await screen.findByText(title)).toBeInTheDocument(); }; - it('does not show for pending member', function () { - const {router, routerProps} = initializeOrg({organization}); + it('does not show for pending member', async function () { + const {router} = initializeOrg({ + organization, + router: {params: {memberId: pendingMember.id}}, + }); - render( - , - { - router, - organization, - } - ); - expect(button()).not.toBeInTheDocument(); + render(, { + router, + organization, + }); + + expect( + await screen.findByRole('button', {name: 'Resend Invite'}) + ).toBeInTheDocument(); + expect( + screen.queryByRole('button', {name: 'Reset two-factor authentication'}) + ).not.toBeInTheDocument(); }); it('shows tooltip for joined member without permission to edit', async function () { - const {router, routerProps} = initializeOrg({organization}); + const {router} = initializeOrg({ + organization, + router: {params: {memberId: noAccess.id}}, + }); - render( - , - { - router, - organization, - } - ); + render(, { + router, + organization, + }); await expectButtonDisabled('You do not have permission to perform this action'); }); it('shows tooltip for member without 2fa', async function () { - const {router, routerProps} = initializeOrg({organization}); + const {router} = initializeOrg({ + organization, + router: {params: {memberId: no2fa.id}}, + }); - render( - , - { - router, - organization, - } - ); + render(, { + router, + organization, + }); await expectButtonDisabled('Not enrolled in two-factor authentication'); }); it('can reset member 2FA', async function () { - const {router, routerProps} = initializeOrg({organization}); + const {router} = initializeOrg({ + organization, + router: {params: {memberId: has2fa.id}}, + }); const deleteMocks = (has2fa.user?.authenticators || []).map(auth => MockApiClient.addMockResponse({ @@ -591,17 +595,16 @@ describe('OrganizationMemberDetail', function () { }) ); - render( - , - { - router, - organization, - } - ); + render(, { + router, + organization, + }); renderGlobalModal(); - expectButtonEnabled(); - await userEvent.click(button() as Element); + await expectButtonEnabled(); + await userEvent.click( + screen.getByRole('button', {name: 'Reset two-factor authentication'}) + ); await userEvent.click(screen.getByRole('button', {name: 'Confirm'})); @@ -611,18 +614,15 @@ describe('OrganizationMemberDetail', function () { }); it('shows tooltip for member in multiple orgs', async function () { - const {router, routerProps} = initializeOrg({organization}); - - render( - , - { - router, - organization, - } - ); + const {router} = initializeOrg({ + organization, + router: {params: {memberId: multipleOrgs.id}}, + }); + + render(, { + router, + organization, + }); await expectButtonDisabled( 'Cannot be reset since user is in more than one organization' ); @@ -630,19 +630,19 @@ describe('OrganizationMemberDetail', function () { it('shows tooltip for member in 2FA required org', async function () { organization.require2FA = true; - const {router, routerProps} = initializeOrg({organization}); + const {router} = initializeOrg({ + organization, + router: {params: {memberId: has2fa.id}}, + }); MockApiClient.addMockResponse({ url: `/organizations/${organization.slug}/members/${has2fa.id}/`, body: has2fa, }); - render( - , - { - router, - organization, - } - ); + render(, { + router, + organization, + }); await expectButtonDisabled( 'Cannot be reset since two-factor is required for this organization' ); @@ -696,21 +696,21 @@ describe('OrganizationMemberDetail', function () { }); it('does not overwrite team-roles for org members', async () => { - const {router, routerProps} = initializeOrg({organization}); + const {router} = initializeOrg({ + organization, + router: {params: {memberId: member.id}}, + }); - render( - , - { - router, - organization, - } - ); + render(, { + router, + organization, + }); // Role info box is hidden expect(screen.queryByTestId('alert-role-overwrite')).not.toBeInTheDocument(); // Dropdown has correct value set - const teamRow = screen.getByTestId('team-row-for-member'); + const teamRow = await screen.findByTestId('team-row-for-member'); const teamRoleSelect = within(teamRow).getByText('Contributor'); // Dropdown options are not visible @@ -726,23 +726,19 @@ describe('OrganizationMemberDetail', function () { }); it('overwrite team-roles for org admin/manager/owner', async () => { - const {router, routerProps} = initializeOrg({organization}); - - async function testForOrgRole(testMember) { + async function testForOrgRole(testMember: Member) { + const {router} = initializeOrg({ + organization, + router: {params: {memberId: testMember.id}}, + }); cleanup(); - render( - , - { - router, - organization, - } - ); + render(, { + router, + organization, + }); // Role info box is showed - expect(screen.queryByTestId('alert-role-overwrite')).toBeInTheDocument(); + expect(await screen.findByTestId('alert-role-overwrite')).toBeInTheDocument(); // Dropdown has correct value set const teamRow = screen.getByTestId('team-row-for-member'); @@ -762,23 +758,23 @@ describe('OrganizationMemberDetail', function () { }); it('overwrites when changing from member to manager', async () => { - const {router, routerProps} = initializeOrg({organization}); - - render( - , - { - router, - organization, - } - ); + const {router} = initializeOrg({ + organization, + router: {params: {memberId: member.id}}, + }); - // Role info box is hidden - expect(screen.queryByTestId('alert-role-overwrite')).not.toBeInTheDocument(); + render(, { + router, + organization, + }); // Dropdown has correct value set - const teamRow = screen.getByTestId('team-row-for-member'); + const teamRow = await screen.findByTestId('team-row-for-member'); const teamRoleSelect = within(teamRow).getByText('Contributor'); + // Role info box is hidden + expect(screen.queryByTestId('alert-role-overwrite')).not.toBeInTheDocument(); + // Change member to owner const orgRoleRadio = screen.getAllByRole('radio'); expect(orgRoleRadio).toHaveLength(5); diff --git a/static/app/views/settings/organizationMembers/organizationMemberDetail.tsx b/static/app/views/settings/organizationMembers/organizationMemberDetail.tsx index 8deb228ad49818..530af906140aaf 100644 --- a/static/app/views/settings/organizationMembers/organizationMemberDetail.tsx +++ b/static/app/views/settings/organizationMembers/organizationMemberDetail.tsx @@ -1,4 +1,4 @@ -import {Fragment} from 'react'; +import {Fragment, useEffect, useMemo, useState} from 'react'; import styled from '@emotion/styled'; import * as Sentry from '@sentry/react'; import isEqual from 'lodash/isEqual'; @@ -17,22 +17,32 @@ import NotFound from 'sentry/components/errors/notFound'; import FieldGroup from 'sentry/components/forms/fieldGroup'; import HookOrDefault from 'sentry/components/hookOrDefault'; import ExternalLink from 'sentry/components/links/externalLink'; +import LoadingError from 'sentry/components/loadingError'; +import LoadingIndicator from 'sentry/components/loadingIndicator'; import Panel from 'sentry/components/panels/panel'; import PanelBody from 'sentry/components/panels/panelBody'; import PanelHeader from 'sentry/components/panels/panelHeader'; import PanelItem from 'sentry/components/panels/panelItem'; +import SentryDocumentTitle from 'sentry/components/sentryDocumentTitle'; import {Tooltip} from 'sentry/components/tooltip'; import {IconRefresh} from 'sentry/icons'; import {t, tct} from 'sentry/locale'; import {space} from 'sentry/styles/space'; -import type {RouteComponentProps} from 'sentry/types/legacyReactRouter'; -import type {Member, Organization} from 'sentry/types/organization'; +import type {Member} from 'sentry/types/organization'; import isMemberDisabledFromLimit from 'sentry/utils/isMemberDisabledFromLimit'; +import { + type ApiQueryKey, + setApiQueryData, + useApiQuery, + useMutation, + useQueryClient, +} from 'sentry/utils/queryClient'; +import type RequestError from 'sentry/utils/requestError/requestError'; import Teams from 'sentry/utils/teams'; -import normalizeUrl from 'sentry/utils/url/normalizeUrl'; -import withOrganization from 'sentry/utils/withOrganization'; -import type {AsyncViewState} from 'sentry/views/deprecatedAsyncView'; -import DeprecatedAsyncView from 'sentry/views/deprecatedAsyncView'; +import useApi from 'sentry/utils/useApi'; +import {useNavigate} from 'sentry/utils/useNavigate'; +import useOrganization from 'sentry/utils/useOrganization'; +import {useParams} from 'sentry/utils/useParams'; import SettingsPageHeader from 'sentry/views/settings/components/settingsPageHeader'; import TeamSelectForMember from 'sentry/views/settings/components/teamSelect/teamSelectForMember'; @@ -45,178 +55,166 @@ const TWO_FACTOR_REQUIRED = t( 'Cannot be reset since two-factor is required for this organization' ); -type RouteParams = { - memberId: string; -}; - -interface Props extends RouteComponentProps { - organization: Organization; -} - -interface State extends AsyncViewState { - member: Member | null; - orgRole: Member['orgRole']; // Form state - teamRoles: Member['teamRoles']; // Form state -} - const DisabledMemberTooltip = HookOrDefault({ hookName: 'component:disabled-member-tooltip', defaultComponent: ({children}) => {children}, }); -class OrganizationMemberDetail extends DeprecatedAsyncView { - get hasTeamRoles() { - const {organization} = this.props; - return organization.features.includes('team-roles'); +function MemberStatus({ + member, + memberDeactivated, +}: { + member: Member; + memberDeactivated: boolean; +}) { + if (memberDeactivated) { + return ( + + {t('Deactivated')} + + ); } - - getDefaultState(): State { - return { - ...super.getDefaultState(), - member: null, - orgRole: '', - teamRoles: [], - }; + if (member.expired) { + return {t('Invitation Expired')}; } - - getEndpoints(): ReturnType { - const {organization, params} = this.props; - return [ - ['member', `/organizations/${organization.slug}/members/${params.memberId}/`], - ]; + if (member.pending) { + return {t('Invitation Pending')}; } + return t('Active'); +} - onRequestSuccess({data, stateKey}: {data: Member; stateKey: string}) { - if (stateKey === 'member') { - const {orgRole, teamRoles} = data; - this.setState({ - orgRole, - teamRoles, - }); - } - } +const getMemberQueryKey = (orgSlug: string, memberId: string): ApiQueryKey => [ + `/organizations/${orgSlug}/members/${memberId}/`, +]; - handleSave = async () => { - const {organization, params} = this.props; - const {orgRole, teamRoles} = this.state; +function OrganizationMemberDetailContent({member}: {member: Member}) { + const api = useApi(); + const queryClient = useQueryClient(); + const organization = useOrganization(); + const navigate = useNavigate(); - addLoadingMessage(t('Saving...')); - this.setState({busy: true}); + const [orgRole, setOrgRole] = useState(''); + const [teamRoles, setTeamRoles] = useState([]); + const hasTeamRoles = organization.features.includes('team-roles'); - try { - const updatedMember = await updateMember(this.api, { - orgId: organization.slug, - memberId: params.memberId, - data: {orgRole, teamRoles} as any, - }); - this.setState({ - member: updatedMember, - orgRole: updatedMember.orgRole, - teamRoles: updatedMember.teamRoles, - busy: false, - }); - addSuccessMessage(t('Saved')); - } catch (resp) { - const errorMessage = resp?.responseJSON?.detail || t('Could not save...'); - this.setState({busy: false}); - addErrorMessage(errorMessage); + useEffect(() => { + if (member) { + setOrgRole(member.orgRole); + setTeamRoles(member.teamRoles); } - }; - - handleInvite = async (regenerate: boolean) => { - const {organization, params} = this.props; - - addLoadingMessage(t('Sending invite...')); - - this.setState({busy: true}); + }, [member]); - try { - const data = await resendMemberInvite(this.api, { + const {mutate: updatedMember, isPending: isSaving} = useMutation({ + mutationFn: () => { + return updateMember(api, { orgId: organization.slug, - memberId: params.memberId, - regenerate, + memberId: member.id, + data: {orgRole, teamRoles} as any, }); - - addSuccessMessage(t('Sent invite!')); - - if (regenerate) { - this.setState(state => ({member: {...state.member, ...data}})); - } - } catch (_err) { - addErrorMessage(t('Could not send invite')); + }, + onMutate: () => { + addLoadingMessage(t('Saving\u2026')); + }, + onSuccess: data => { + addSuccessMessage(t('Saved')); + setApiQueryData( + queryClient, + getMemberQueryKey(organization.slug, member.id), + data + ); + }, + onError: error => { + addErrorMessage( + (error?.responseJSON?.detail as string) ?? t('Failed to update member') + ); + }, + }); + + const {mutate: inviteMember, isPending: isInviting} = useMutation( + { + mutationFn: () => { + return resendMemberInvite(api, { + orgId: organization.slug, + memberId: member.id, + regenerate: true, + }); + }, + onSuccess: data => { + addSuccessMessage(t('Sent invite!')); + + setApiQueryData( + queryClient, + getMemberQueryKey(organization.slug, member.id), + data + ); + }, + onError: () => { + addErrorMessage(t('Could not send invite')); + }, } - - this.setState({busy: false}); - }; - - handle2faReset = async () => { - const {organization, router} = this.props; - const {user} = this.state.member!; - - const requests = - user?.authenticators?.map(auth => - removeAuthenticator(this.api, user.id, auth.id) - ) ?? []; - - try { - await Promise.all(requests); - router.push(normalizeUrl(`/settings/${organization.slug}/members/`)); + ); + + const {mutate: reset2fa, isPending: isResetting2fa} = useMutation({ + mutationFn: () => { + const {user} = member; + const promises = + user?.authenticators?.map(auth => removeAuthenticator(api, user.id, auth.id)) ?? + []; + return Promise.all(promises); + }, + onSuccess: () => { addSuccessMessage(t('All authenticators have been removed')); - } catch (err) { + navigate(`/settings/${organization.slug}/members/`); + }, + onError: error => { addErrorMessage(t('Error removing authenticators')); - Sentry.captureException(err); - } - }; + Sentry.captureException(error); + }, + }); - onAddTeam = (teamSlug: string) => { - const teamRoles = [...this.state.teamRoles]; - const i = teamRoles.findIndex(r => r.teamSlug === teamSlug); + const onAddTeam = (teamSlug: string) => { + const newTeamRoles = [...teamRoles]; + const i = newTeamRoles.findIndex(r => r.teamSlug === teamSlug); if (i !== -1) { return; } - teamRoles.push({teamSlug, role: null}); - this.setState({teamRoles}); + newTeamRoles.push({teamSlug, role: null}); + setTeamRoles(newTeamRoles); }; - onRemoveTeam = (teamSlug: string) => { - const teamRoles = this.state.teamRoles.filter(r => r.teamSlug !== teamSlug); - this.setState({teamRoles}); + const onRemoveTeam = (teamSlug: string) => { + const newTeamRoles = teamRoles.filter(r => r.teamSlug !== teamSlug); + setTeamRoles(newTeamRoles); }; - onChangeOrgRole = orgRole => this.setState({orgRole}); - - onChangeTeamRole = (teamSlug: string, role: string) => { - if (!this.hasTeamRoles) { + const onChangeTeamRole = (teamSlug: string, role: string) => { + if (!hasTeamRoles) { return; } - const teamRoles = [...this.state.teamRoles]; - const i = teamRoles.findIndex(r => r.teamSlug === teamSlug); + const newTeamRoles = [...teamRoles]; + const i = newTeamRoles.findIndex(r => r.teamSlug === teamSlug); if (i === -1) { return; } - teamRoles[i] = {...teamRoles[i], role}; - this.setState({teamRoles}); + newTeamRoles[i] = {...newTeamRoles[i], role}; + setTeamRoles(newTeamRoles); }; - showResetButton = () => { - const {organization} = this.props; - const {member} = this.state; - const {user} = member!; + const showResetButton = useMemo(() => { + const {user} = member; if (!user || !user.authenticators || organization.require2FA) { return false; } const hasAuth = user.authenticators.length >= 1; return hasAuth && user.canReset2fa; - }; + }, [member, organization.require2FA]); - getTooltip = (): string => { - const {organization} = this.props; - const {member} = this.state; - const {user} = member!; + const getTooltip = (): string => { + const {user} = member; if (!user) { return ''; @@ -238,12 +236,7 @@ class OrganizationMemberDetail extends DeprecatedAsyncView { return ''; }; - get memberDeactivated() { - return isMemberDisabledFromLimit(this.state.member); - } - - get hasFormChanged() { - const {member, orgRole, teamRoles} = this.state; + function hasFormChanged() { if (!member) { return false; } @@ -255,175 +248,179 @@ class OrganizationMemberDetail extends DeprecatedAsyncView { return false; } - renderMemberStatus(member: Member) { - if (this.memberDeactivated) { - return ( - - {t('Deactivated')} - - ); - } - if (member.expired) { - return {t('Invitation Expired')}; - } - if (member.pending) { - return {t('Invitation Pending')}; - } - return t('Active'); - } - - renderBody() { - const {organization} = this.props; - const {member, orgRole, teamRoles} = this.state; - - if (!member) { - return ; - } - - const {access, orgRoleList} = organization; - const canEdit = access.includes('org:write') && !this.memberDeactivated; - const isPartnershipUser = member.flags['partnership:restricted'] === true; - - const {email, expired, pending} = member; - const canResend = !expired; - const showAuth = !pending; - - const showResendButton = (member.pending || member.expired) && canResend; - - return ( - - -
{member.name}
- {t('Member Settings')} - - } - /> - - - - {t('Basics')} - - {showResendButton && ( - - )} - + const memberDeactivated = isMemberDisabledFromLimit(member); + const canEdit = organization.access.includes('org:write') && !memberDeactivated; + const isPartnershipUser = member.flags['partnership:restricted'] === true; + + const {email, expired, pending} = member; + const canResend = !expired; + const showAuth = !pending; + + const showResendButton = (member.pending || member.expired) && canResend; + + return ( + + + +
{member.name}
+ {t('Member Settings')} + + } + /> + + + + {t('Basics')} + + {showResendButton && ( + + )} + - - -
+ + +
+
+ {t('Email')}
- {t('Email')} -
- {email} -
+ {email}
-
- {t('Status')} -
- {this.renderMemberStatus(member)} -
+
+
+ {t('Status')} +
+
+
+
+ {t('Added')}
- {t('Added')} -
- -
+
-
- + +
+ + + + + {showAuth && ( + + {t('Authentication')} + + + + reset2fa()} + > + + + + - - {showAuth && ( - - {t('Authentication')} - - - - - - - - - - + )} + + { + setOrgRole(newOrgRole); + }} + helpText={ + isPartnershipUser + ? t('You cannot make changes to this partner-provisioned user.') + : undefined + } + /> + + + {({initiallyLoaded}) => ( + )} + + +
+ +
+ + ); +} - - - - {({initiallyLoaded}) => ( - - - - )} - - -
- -
- - ); +function OrganizationMemberDetail() { + const params = useParams<{memberId: string}>(); + const organization = useOrganization(); + + const { + data: member, + isPending, + isError, + refetch, + } = useApiQuery(getMemberQueryKey(organization.slug, params.memberId), { + staleTime: 0, + }); + + if (isPending) { + return ; + } + + if (isError) { + return ; + } + + if (!member) { + return ; } + + return ; } -export default withOrganization(OrganizationMemberDetail); +export default OrganizationMemberDetail; const ExtraHeaderText = styled('div')` color: ${p => p.theme.gray300}; From 273ea7e4a5760ccf14871df0ec672e83ac1f0d9c Mon Sep 17 00:00:00 2001 From: Michelle Zhang <56095982+michellewzhang@users.noreply.github.com> Date: Wed, 16 Oct 2024 13:28:08 -0700 Subject: [PATCH 50/67] ref(flags): change help tooltip for flags section (#79168) the tooltip won't be around when the new issues UI becomes default, but for now change the clarifying help text since we updated the buffer size. --- .../app/components/events/featureFlags/eventFeatureFlagList.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/static/app/components/events/featureFlags/eventFeatureFlagList.tsx b/static/app/components/events/featureFlags/eventFeatureFlagList.tsx index e6715d4b4bc9aa..30c1e9977d9ae6 100644 --- a/static/app/components/events/featureFlags/eventFeatureFlagList.tsx +++ b/static/app/components/events/featureFlags/eventFeatureFlagList.tsx @@ -197,7 +197,7 @@ export function EventFeatureFlagList({ Date: Wed, 16 Oct 2024 16:37:16 -0400 Subject: [PATCH 51/67] Fix 'accross' typo (#79204) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit accross→across ### Legal Boilerplate Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms. --- .../newTraceDetails/traceTypeWarnings/errorsOnlyWarnings.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/static/app/views/performance/newTraceDetails/traceTypeWarnings/errorsOnlyWarnings.tsx b/static/app/views/performance/newTraceDetails/traceTypeWarnings/errorsOnlyWarnings.tsx index 00b465859ab0b2..11f25fcaddb119 100644 --- a/static/app/views/performance/newTraceDetails/traceTypeWarnings/errorsOnlyWarnings.tsx +++ b/static/app/views/performance/newTraceDetails/traceTypeWarnings/errorsOnlyWarnings.tsx @@ -98,7 +98,7 @@ function PerformanceSetupBanner({ { From ff078fc0ce52c6776fe6b0539f1c5e72f595f634 Mon Sep 17 00:00:00 2001 From: Lyn Nagara <1779792+lynnagara@users.noreply.github.com> Date: Wed, 16 Oct 2024 13:40:57 -0700 Subject: [PATCH 52/67] ref: try to remove the save_event_transaction queue name (again) (#79226) reissue of https://github.com/getsentry/sentry/pull/79205 with a fix to instrumented_task decorator --- src/sentry/tasks/base.py | 7 ++----- src/sentry/tasks/store.py | 1 - 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/sentry/tasks/base.py b/src/sentry/tasks/base.py index 6559ca3b38d983..3b703d27fd9fb5 100644 --- a/src/sentry/tasks/base.py +++ b/src/sentry/tasks/base.py @@ -135,12 +135,9 @@ def _wrapped(*args, **kwargs): # If the split task router is configured for the task, always use queues defined # in the split task configuration - if name in settings.CELERY_SPLIT_QUEUE_TASK_ROUTES: + if name in settings.CELERY_SPLIT_QUEUE_TASK_ROUTES and "queue" in kwargs: q = kwargs.pop("queue") - if q: - logger.warning( - "ignoring queue: %s, using value from CELERY_SPLIT_QUEUE_TASK_ROUTES", q - ) + logger.warning("ignoring queue: %s, using value from CELERY_SPLIT_QUEUE_TASK_ROUTES", q) # We never use result backends in Celery. Leaving `trail=True` means that if we schedule # many tasks from a parent task, each task leaks memory. This can lead to the scheduler diff --git a/src/sentry/tasks/store.py b/src/sentry/tasks/store.py index 02c1b74183b4a7..c2fe5942144098 100644 --- a/src/sentry/tasks/store.py +++ b/src/sentry/tasks/store.py @@ -609,7 +609,6 @@ def save_event( @instrumented_task( name="sentry.tasks.store.save_event_transaction", - queue="events.save_event_transaction", time_limit=65, soft_time_limit=60, silo_mode=SiloMode.REGION, From cccaf969246325cc49515e4506fc170d8fd8592f Mon Sep 17 00:00:00 2001 From: getsentry-bot Date: Wed, 16 Oct 2024 20:41:05 +0000 Subject: [PATCH 53/67] Revert "chore(deps): Upgrade Sentry Python SDK to 2.16.0 (#78998)" This reverts commit 6b158e896368c32bb482b30626e938947fbf2c03. Co-authored-by: asottile-sentry <103459774+asottile-sentry@users.noreply.github.com> --- requirements-base.txt | 2 +- requirements-dev-frozen.txt | 2 +- requirements-frozen.txt | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/requirements-base.txt b/requirements-base.txt index 424e66c07b355f..525c5cee503f7e 100644 --- a/requirements-base.txt +++ b/requirements-base.txt @@ -70,7 +70,7 @@ sentry-ophio==1.0.0 sentry-protos>=0.1.26 sentry-redis-tools>=0.1.7 sentry-relay>=0.9.2 -sentry-sdk>=2.16.0 +sentry-sdk>=2.15.0 slack-sdk>=3.27.2 snuba-sdk>=3.0.43 simplejson>=3.17.6 diff --git a/requirements-dev-frozen.txt b/requirements-dev-frozen.txt index ef327eb93fbbe8..d820341fd9ebf6 100644 --- a/requirements-dev-frozen.txt +++ b/requirements-dev-frozen.txt @@ -187,7 +187,7 @@ sentry-ophio==1.0.0 sentry-protos==0.1.26 sentry-redis-tools==0.1.7 sentry-relay==0.9.2 -sentry-sdk==2.16.0 +sentry-sdk==2.15.0 sentry-usage-accountant==0.0.10 simplejson==3.17.6 six==1.16.0 diff --git a/requirements-frozen.txt b/requirements-frozen.txt index f9eab384702f8d..48830815d3a111 100644 --- a/requirements-frozen.txt +++ b/requirements-frozen.txt @@ -128,7 +128,7 @@ sentry-ophio==1.0.0 sentry-protos==0.1.26 sentry-redis-tools==0.1.7 sentry-relay==0.9.2 -sentry-sdk==2.16.0 +sentry-sdk==2.15.0 sentry-usage-accountant==0.0.10 simplejson==3.17.6 six==1.16.0 From b59ae686b6b7d52c9a3194de48db525f6f4be29a Mon Sep 17 00:00:00 2001 From: Nate Moore Date: Wed, 16 Oct 2024 14:20:57 -0700 Subject: [PATCH 54/67] fix(nav): remove animated highlight (#79223) Removes the animated nav highlight indicator, preferring non-animated hover indicators powered by `InteractionStateLayer`. https://github.com/user-attachments/assets/734ee723-e27f-4361-9914-985a378a93a4 Closes [ALRT-333](https://getsentry.atlassian.net/browse/ALRT-333). [ALRT-333]: https://getsentry.atlassian.net/browse/ALRT-333?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --- static/app/components/nav/sidebar.tsx | 51 ++++---------- static/app/components/nav/submenu.tsx | 46 ++++--------- static/app/components/nav/useNavIndicator.tsx | 66 ------------------- 3 files changed, 23 insertions(+), 140 deletions(-) delete mode 100644 static/app/components/nav/useNavIndicator.tsx diff --git a/static/app/components/nav/sidebar.tsx b/static/app/components/nav/sidebar.tsx index 142864fa9e286a..7856cb2fa940ba 100644 --- a/static/app/components/nav/sidebar.tsx +++ b/static/app/components/nav/sidebar.tsx @@ -1,12 +1,11 @@ import {Fragment} from 'react'; import styled from '@emotion/styled'; -import {motion} from 'framer-motion'; import Feature from 'sentry/components/acl/feature'; +import InteractionStateLayer from 'sentry/components/interactionStateLayer'; import Link from 'sentry/components/links/link'; import {useNavContext} from 'sentry/components/nav/context'; import Submenu from 'sentry/components/nav/submenu'; -import {useNavIndicator} from 'sentry/components/nav/useNavIndicator'; import { isNavItemActive, isNonEmptyArray, @@ -103,19 +102,24 @@ function SidebarItem({item}: {item: NavSidebarItem}) { return ( - + {item.icon} {item.label} - + ); } +const SidebarLink = styled(Link)` + position: relative; +`; + const SidebarItemWrapper = styled('li')` svg { --size: 14px; @@ -138,17 +142,6 @@ const SidebarItemWrapper = styled('li')` font-size: ${theme.fontSizeMedium}; font-weight: ${theme.fontWeightNormal}; line-height: 177.75%; - border: 1px solid transparent; - - &:hover { - color: var(--color-hover, ${theme.white}); - } - - &.active { - color: var(--color-hover, ${theme.white}); - background: rgba(255, 255, 255, 0.1); - border: 1px solid rgba(255, 255, 255, 0.06); - } & > * { pointer-events: none; @@ -167,19 +160,6 @@ const SidebarItemWrapper = styled('li')` } `; -const SidebarIndicator = styled(motion.span)` - position: absolute; - left: 0; - right: 0; - opacity: 0; - pointer-events: none; - margin-inline: ${space(1)}; - width: 58px; - height: 52px; - background: rgba(255, 255, 255, 0.1); - border-radius: ${theme.borderRadius}; -`; - const SidebarFooterWrapper = styled('div')` position: relative; border-top: 1px solid ${theme.translucentGray200}; @@ -198,22 +178,13 @@ const SidebarHeader = styled('header')` `; function SidebarBody({children}) { - const {indicatorProps, containerProps} = useNavIndicator(); - // div wrapper is needed to for indicator positioning - return ( -
- - {children} -
- ); + return {children}; } function SidebarFooter({children}) { - const {indicatorProps, containerProps} = useNavIndicator(); return ( - - {children} + {children} ); } diff --git a/static/app/components/nav/submenu.tsx b/static/app/components/nav/submenu.tsx index 3f5d23b6f65095..c550ff32ce871f 100644 --- a/static/app/components/nav/submenu.tsx +++ b/static/app/components/nav/submenu.tsx @@ -1,11 +1,10 @@ import {Fragment} from 'react'; import styled from '@emotion/styled'; -import {motion} from 'framer-motion'; import Feature from 'sentry/components/acl/feature'; +import InteractionStateLayer from 'sentry/components/interactionStateLayer'; import Link from 'sentry/components/links/link'; import {useNavContext} from 'sentry/components/nav/context'; -import {useNavIndicator} from 'sentry/components/nav/useNavIndicator'; import type {NavSubmenuItem} from 'sentry/components/nav/utils'; import { isNavItemActive, @@ -41,7 +40,7 @@ function Submenu() { export default Submenu; -const SubmenuWrapper = styled(motion.div)` +const SubmenuWrapper = styled('div')` position: relative; border-right: 1px solid ${p => p.theme.translucentGray200}; background: ${p => p.theme.surface300}; @@ -64,18 +63,23 @@ function SubmenuItem({item}: {item: NavSubmenuItem}) { return ( - + {item.label} - + ); } +const SubmenuLink = styled(Link)` + position: relative; +`; + const SubmenuItemList = styled('ul')` list-style: none; margin: 0; @@ -98,14 +102,8 @@ const SubmenuItemWrapper = styled('li')` font-weight: ${p => p.theme.fontWeightNormal}; line-height: 177.75%; margin-inline: ${space(1)}; - border: 1px solid transparent; border-radius: ${p => p.theme.borderRadius}; - &:hover { - color: ${p => p.theme.gray500}; - /* background: rgba(62, 52, 70, 0.09); */ - } - &.active { color: ${p => p.theme.gray500}; background: rgba(62, 52, 70, 0.09); @@ -114,18 +112,6 @@ const SubmenuItemWrapper = styled('li')` } `; -const SubmenuIndicator = styled(motion.span)` - position: absolute; - left: 0; - right: 0; - opacity: 0; - pointer-events: none; - margin-inline: ${space(1)}; - height: 32px; - background: ${p => p.theme.translucentGray100}; - border-radius: ${p => p.theme.borderRadius}; -`; - const SubmenuFooterWrapper = styled('div')` position: relative; border-top: 1px solid ${p => p.theme.translucentGray200}; @@ -137,21 +123,13 @@ const SubmenuFooterWrapper = styled('div')` `; function SubmenuBody({children}) { - const {indicatorProps, containerProps} = useNavIndicator(); - return ( -
- - {children} -
- ); + return {children}; } function SubmenuFooter({children}) { - const {indicatorProps, containerProps} = useNavIndicator(); return ( - - {children} + {children} ); } diff --git a/static/app/components/nav/useNavIndicator.tsx b/static/app/components/nav/useNavIndicator.tsx deleted file mode 100644 index 752063944cbffc..00000000000000 --- a/static/app/components/nav/useNavIndicator.tsx +++ /dev/null @@ -1,66 +0,0 @@ -import { - type ComponentProps, - type HTMLAttributes, - type PointerEvent, - useCallback, -} from 'react'; -import {useFocusWithin, useHover} from '@react-aria/interactions'; -import {mergeProps} from '@react-aria/utils'; -import {type motion, useMotionValue, useReducedMotion, useSpring} from 'framer-motion'; - -export interface IndicatorResult { - containerProps: HTMLAttributes; - indicatorProps: ComponentProps; -} - -export function useNavIndicator(): IndicatorResult { - const prefersReducedMotion = useReducedMotion(); - const ty = useMotionValue(0); - const y = useSpring( - ty, - prefersReducedMotion - ? {duration: 0} - : { - mass: 0.5, - damping: 15, - stiffness: 200, - restDelta: 0.05, - } - ); - const {isHovered, hoverProps} = useHover({}); - const {focusWithinProps} = useFocusWithin({}); - - const handlePointerMove = useCallback( - (e: PointerEvent) => { - if (e.target instanceof HTMLAnchorElement) { - ty.set(e.target.offsetTop); - return; - } - }, - [ty] - ); - - const handlePointerEnter = useCallback( - (e: PointerEvent) => { - if (e.target instanceof HTMLUListElement) { - y.jump(e.target.querySelector('a')!.offsetTop); - } else if (e.target instanceof HTMLElement) { - y.jump(e.target.offsetTop); - } - }, - [y] - ); - - return { - containerProps: mergeProps(hoverProps, focusWithinProps, { - onPointerEnter: handlePointerEnter, - onPointerMove: handlePointerMove, - }), - indicatorProps: { - 'aria-hidden': 'true', - style: {y}, - animate: {opacity: isHovered ? 1 : 0}, - transition: {duration: 0.2}, - }, - }; -} From 5ed1ade73630fe9dc4ff852cc3a2373929b7a707 Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Wed, 16 Oct 2024 14:22:56 -0700 Subject: [PATCH 55/67] :wrench: chore(data-secrecy): update data secrecy tests & attempt to fix delete endpoint (#79228) i am trying to use another way to get the waiver entry and delete it because i have a bizarre issue that i can't replicate where the delete method returns a 404 when the waiver exists. also improved the tests --- src/sentry/data_secrecy/api/waive_data_secrecy.py | 6 +++--- .../data_secrecy/api/test_waive_data_secrecy.py | 12 +++++++++++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/sentry/data_secrecy/api/waive_data_secrecy.py b/src/sentry/data_secrecy/api/waive_data_secrecy.py index 8af1d9bab7e480..c539f80e24a441 100644 --- a/src/sentry/data_secrecy/api/waive_data_secrecy.py +++ b/src/sentry/data_secrecy/api/waive_data_secrecy.py @@ -119,12 +119,12 @@ def put(self, request: Request, organization: Organization): serialize(ds, request.user, DataSecrecyWaiverSerializer()), status=status.HTTP_200_OK ) - def delete(self, request: Request, organization): + def delete(self, request: Request, organization: Organization): """ Reinstates data secrecy for an organization. """ try: - ds = get_object_or_404(DataSecrecyWaiver, organization=organization) + ds = DataSecrecyWaiver.objects.get(organization=organization) ds.delete() self.create_audit_entry( @@ -136,7 +136,7 @@ def delete(self, request: Request, organization): {"detail": "Data secrecy has been reinstated."}, status=status.HTTP_204_NO_CONTENT, ) - except Http404: + except DataSecrecyWaiver.DoesNotExist: return Response( {"detail": "No data secrecy waiver found for this organization."}, status=status.HTTP_404_NOT_FOUND, diff --git a/tests/sentry/data_secrecy/api/test_waive_data_secrecy.py b/tests/sentry/data_secrecy/api/test_waive_data_secrecy.py index 4c931117298a72..f6978fd5709c7f 100644 --- a/tests/sentry/data_secrecy/api/test_waive_data_secrecy.py +++ b/tests/sentry/data_secrecy/api/test_waive_data_secrecy.py @@ -89,6 +89,11 @@ def test_put_update(self): assert ds.access_start == datetime.now(tz=timezone.utc) + timedelta(days=1) assert ds.access_end == datetime.now(tz=timezone.utc) + timedelta(days=2) + assert_org_audit_log_exists( + organization=self.org, + event=audit_log.get_event_id("DATA_SECRECY_WAIVED"), + ) + @freeze_time(datetime(2024, 7, 18, 0, 0, 0, tzinfo=timezone.utc)) def test_put_invalid_dates(self): self.body_params["accessEnd"] = self.body_params["accessStart"] @@ -120,10 +125,15 @@ def test_delete_existing_waiver(self): ) with outbox_runner(): - self.get_success_response(self.org.slug, method="delete") + self.get_success_response(self.org.slug, method="delete", status_code=204) assert DataSecrecyWaiver.objects.filter(organization=self.org).first() is None + assert_org_audit_log_exists( + organization=self.org, + event=audit_log.get_event_id("DATA_SECRECY_REINSTATED"), + ) + @freeze_time(datetime(2024, 7, 18, 0, 0, 0, tzinfo=timezone.utc)) def test_delete_non_existing_waiver(self): self.get_error_response(self.org.slug, method="delete", status_code=404) From ca4f5d12f4bed3a6dcd9a1fec7f54acaf8b0d91f Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Wed, 16 Oct 2024 14:24:19 -0700 Subject: [PATCH 56/67] fix(replay): handle null user fields as a special case in search config (#79054) Follow-up to https://github.com/getsentry/sentry/pull/78642. Closes https://github.com/getsentry/sentry/issues/78286 Because user.[ip, email, username, id] are nullable strings, and we aggregate the segments with `anyIf`, we can't use the row-by-row scalar config to filter on them. For the aggregate config, we need to handle the special case of null values. A replay's field is null if and only if all its segments have field = null. If you try the [user.email:""](https://sentry.sentry.io/replays/?cursor=0%3A0%3A1&project=11276&query=user.email%3A%22%22&statsPeriod=7d&utc=true) filter in sentry right now, and paginate results, you can see many replays with an email are returned. We might even be returning all of them. --- .../usecases/query/conditions/aggregate.py | 68 ++++++++------ .../replays/usecases/query/configs/scalar.py | 15 +--- .../replays/test_organization_replay_index.py | 88 +++++++++++++------ 3 files changed, 106 insertions(+), 65 deletions(-) diff --git a/src/sentry/replays/usecases/query/conditions/aggregate.py b/src/sentry/replays/usecases/query/conditions/aggregate.py index f5079b059ee923..ccbfd43b899b93 100644 --- a/src/sentry/replays/usecases/query/conditions/aggregate.py +++ b/src/sentry/replays/usecases/query/conditions/aggregate.py @@ -25,7 +25,7 @@ from uuid import UUID -from snuba_sdk import And, Condition, Function, Op, Or +from snuba_sdk import And, Condition, Or from snuba_sdk.expressions import Expression from sentry.replays.lib.new_query.conditions import ( @@ -40,6 +40,14 @@ from sentry.replays.lib.new_query.utils import contains, does_not_contain +def _nonempty_str(expression: Expression) -> Condition: + return StringScalar.visit_neq(expression, "") + + +def _nonnull_ipv4(expression: Expression) -> Condition: + return IPv4Scalar.visit_neq(expression, None) + + class SumOfIntegerIdScalar(GenericBase): @staticmethod def visit_eq(expression: Expression, value: int) -> Condition: @@ -61,65 +69,75 @@ def visit_not_in(expression: Expression, value: list[int]) -> Condition: class SumOfIPv4Scalar(GenericBase): @staticmethod def visit_eq(expression: Expression, value: str | None) -> Condition: + if value is None: + return does_not_contain(_nonnull_ipv4(expression)) return contains(IPv4Scalar.visit_eq(expression, value)) @staticmethod def visit_neq(expression: Expression, value: str | None) -> Condition: + if value is None: + return contains(_nonnull_ipv4(expression)) return does_not_contain(IPv4Scalar.visit_eq(expression, value)) @staticmethod def visit_in(expression: Expression, value_list: list[str | None]) -> Condition: + nonempty_case = contains( + IPv4Scalar.visit_in(expression, [v for v in value_list if v is not None]) + ) if None in value_list: - contains_cond = contains( - IPv4Scalar.visit_in(expression, [v for v in value_list if v is not None]) - ) - return Or( - conditions=[ - contains_cond, - Condition(Function("isNull", parameters=[expression]), Op.EQ, 1), - ] - ) - return contains(IPv4Scalar.visit_in(expression, value_list)) + return Or(conditions=[SumOfIPv4Scalar.visit_eq(expression, None), nonempty_case]) + return nonempty_case @staticmethod def visit_not_in(expression: Expression, value_list: list[str | None]) -> Condition: + nonempty_case = does_not_contain( + IPv4Scalar.visit_in(expression, [v for v in value_list if v is not None]) + ) if None in value_list: - does_not_contain_cond = does_not_contain( - IPv4Scalar.visit_in(expression, [v for v in value_list if v is not None]) - ) - return And( - conditions=[ - does_not_contain_cond, - Condition(Function("isNull", parameters=[expression]), Op.EQ, 0), - ] - ) - return does_not_contain(IPv4Scalar.visit_in(expression, value_list)) + return And(conditions=[SumOfIPv4Scalar.visit_neq(expression, None), nonempty_case]) + return nonempty_case class SumOfStringScalar(GenericBase): @staticmethod def visit_eq(expression: Expression, value: str) -> Condition: + if value == "": + return does_not_contain(_nonempty_str(expression)) return contains(StringScalar.visit_eq(expression, value)) @staticmethod def visit_neq(expression: Expression, value: str) -> Condition: + if value == "": + return contains(_nonempty_str(expression)) return does_not_contain(StringScalar.visit_eq(expression, value)) @staticmethod def visit_match(expression: Expression, value: str) -> Condition: + # Assumes this is only called on wildcard strings, so `value` is non-empty. return contains(StringScalar.visit_match(expression, value)) @staticmethod def visit_not_match(expression: Expression, value: str) -> Condition: + # Assumes this is only called on wildcard strings, so `value` is non-empty. return does_not_contain(StringScalar.visit_match(expression, value)) @staticmethod - def visit_in(expression: Expression, value: list[str]) -> Condition: - return contains(StringScalar.visit_in(expression, value)) + def visit_in(expression: Expression, value_list: list[str]) -> Condition: + nonempty_case = contains( + StringScalar.visit_in(expression, [v for v in value_list if v != ""]) + ) + if "" in value_list: + return Or(conditions=[SumOfStringScalar.visit_eq(expression, ""), nonempty_case]) + return nonempty_case @staticmethod - def visit_not_in(expression: Expression, value: list[str]) -> Condition: - return does_not_contain(StringScalar.visit_in(expression, value)) + def visit_not_in(expression: Expression, value_list: list[str]) -> Condition: + nonempty_case = does_not_contain( + StringScalar.visit_in(expression, [v for v in value_list if v != ""]) + ) + if "" in value_list: + return And(conditions=[SumOfStringScalar.visit_neq(expression, ""), nonempty_case]) + return nonempty_case class SumOfStringArray(GenericBase): diff --git a/src/sentry/replays/usecases/query/configs/scalar.py b/src/sentry/replays/usecases/query/configs/scalar.py index 44bf302ebb8c48..ecacafd47cecf3 100644 --- a/src/sentry/replays/usecases/query/configs/scalar.py +++ b/src/sentry/replays/usecases/query/configs/scalar.py @@ -7,19 +7,13 @@ from sentry.api.event_search import ParenExpression, SearchFilter from sentry.replays.lib.new_query.conditions import ( - IPv4Scalar, NonEmptyStringScalar, StringArray, StringScalar, UUIDArray, ) -from sentry.replays.lib.new_query.fields import ( - FieldProtocol, - NullableStringColumnField, - StringColumnField, - UUIDColumnField, -) -from sentry.replays.lib.new_query.parsers import parse_ipv4, parse_str, parse_uuid +from sentry.replays.lib.new_query.fields import FieldProtocol, StringColumnField, UUIDColumnField +from sentry.replays.lib.new_query.parsers import parse_str, parse_uuid from sentry.replays.lib.selector.parse import parse_selector from sentry.replays.usecases.query.conditions import ( ClickSelectorComposite, @@ -68,10 +62,6 @@ def string_field(column_name: str) -> StringColumnField: "error_ids": ComputedField(parse_uuid, ErrorIdScalar), "trace_ids": UUIDColumnField("trace_ids", parse_uuid, UUIDArray), "urls": StringColumnField("urls", parse_str, StringArray), - "user.email": StringColumnField("user_email", parse_str, NonEmptyStringScalar), - "user.id": StringColumnField("user_id", parse_str, NonEmptyStringScalar), - "user.ip_address": NullableStringColumnField("ip_address_v4", parse_ipv4, IPv4Scalar), - "user.username": StringColumnField("user_name", parse_str, NonEmptyStringScalar), } # Aliases @@ -79,7 +69,6 @@ def string_field(column_name: str) -> StringColumnField: varying_search_config["trace_id"] = varying_search_config["trace_ids"] varying_search_config["trace"] = varying_search_config["trace_ids"] varying_search_config["url"] = varying_search_config["urls"] -varying_search_config["user.ip"] = varying_search_config["user.ip_address"] varying_search_config["*"] = TagField(query=TagScalar) diff --git a/tests/sentry/replays/test_organization_replay_index.py b/tests/sentry/replays/test_organization_replay_index.py index e6c9747e4b95f1..77f18d6f5c6ba5 100644 --- a/tests/sentry/replays/test_organization_replay_index.py +++ b/tests/sentry/replays/test_organization_replay_index.py @@ -616,11 +616,21 @@ def test_get_replays_user_filters(self): "duration:[16,17]", "!duration:[16,18]", "user.id:123", - "user:username123", + "user.id:1*3", + "user.id:[4000, 123]", + "!user.id:[321, 1230]", + "user:username123", # user is an alias for user.username "user.username:username123", + "user.username:*3", + "user.username:[username123, bob456]", + "!user.username:[bob456, bob123]", "user.email:username@example.com", "user.email:*@example.com", + "user.email:[user2@example.com, username@example.com]", + "!user.email:[user2@example.com]", "user.ip:127.0.0.1", + "user.ip:[127.0.0.1, 10.0.4.4]", + "!user.ip:[127.1.1.1, 10.0.4.4]", "sdk.name:sentry.javascript.react", "os.name:macOS", "os.version:15", @@ -718,6 +728,8 @@ def test_get_replays_user_filters(self): "activity:<2", "viewed_by_id:2", "seen_by_id:2", + "user.email:[user2@example.com]", + "!user.email:[username@example.com, user2@example.com]", ] for query in null_queries: response = self.client.get(self.url + f"?field=id&query={query}") @@ -1717,7 +1729,16 @@ def test_query_invalid_ipv4_addresses(self): response = self.client.get(self.url + f"?field=id&query={query}") assert response.status_code == 400 - def test_query_null_ipv4(self): + def _test_empty_filters(self, query_key, field, null_value, nonnull_value): + """ + Tests filters on a nullable field such as user.email:"", !user.email:"", user.email:["", ...]. + Due to clickhouse aggregations, these queries are handled as a special case which needs testing. + + @param query_key name of field in URL query string, ex `user.email`. + @param field name of kwarg used for testutils.mock_replay, ex `user_email`. + @param null_value null value for this field, stored by Snuba processor (ex: null user_email is translated to ""). + @param nonnull_value a non-null value to use for testing. + """ project = self.create_project(teams=[self.team]) replay1_id = uuid.uuid4().hex @@ -1725,41 +1746,39 @@ def test_query_null_ipv4(self): seq1_timestamp = datetime.datetime.now() - datetime.timedelta(seconds=22) seq2_timestamp = datetime.datetime.now() - datetime.timedelta(seconds=5) - self.store_replays(mock_replay(seq1_timestamp, project.id, replay1_id, ipv4="127.1.42.0")) - self.store_replays(mock_replay(seq2_timestamp, project.id, replay1_id, ipv4="127.1.42.0")) - self.store_replays(mock_replay(seq1_timestamp, project.id, replay2_id, ipv4=None)) - self.store_replays(mock_replay(seq2_timestamp, project.id, replay2_id, ipv4=None)) + self.store_replays( + mock_replay(seq1_timestamp, project.id, replay1_id, **{field: null_value}) + ) + self.store_replays( + mock_replay(seq2_timestamp, project.id, replay1_id, **{field: nonnull_value}) + ) + + self.store_replays( + mock_replay(seq1_timestamp, project.id, replay2_id, **{field: null_value}) + ) + self.store_replays( + mock_replay(seq2_timestamp, project.id, replay2_id, **{field: null_value}) + ) with self.feature(self.features): - null_ip_query = 'user.ip:""' - response = self.client.get(self.url + f"?field=id&query={null_ip_query}") + null_query = f'{query_key}:""' + response = self.client.get(self.url + f"?field=id&query={null_query}") assert response.status_code == 200 data = response.json()["data"] assert len(data) == 1 assert data[0]["id"] == replay2_id - negated_query = "!" + null_ip_query - response = self.client.get(self.url + f"?field=id&query={negated_query}") + non_null_query = "!" + null_query + response = self.client.get(self.url + f"?field=id&query={non_null_query}") assert response.status_code == 200 data = response.json()["data"] assert len(data) == 1 assert data[0]["id"] == replay1_id - def test_query_contains_null_ipv4(self): - project = self.create_project(teams=[self.team]) - - replay1_id = uuid.uuid4().hex - replay2_id = uuid.uuid4().hex - seq1_timestamp = datetime.datetime.now() - datetime.timedelta(seconds=22) - seq2_timestamp = datetime.datetime.now() - datetime.timedelta(seconds=5) - - self.store_replays(mock_replay(seq1_timestamp, project.id, replay1_id, ipv4="127.1.42.0")) - self.store_replays(mock_replay(seq2_timestamp, project.id, replay1_id, ipv4="127.1.42.0")) - self.store_replays(mock_replay(seq1_timestamp, project.id, replay2_id, ipv4=None)) - self.store_replays(mock_replay(seq2_timestamp, project.id, replay2_id, ipv4=None)) - - with self.feature(self.features): - list_queries = ['user.ip:[127.1.42.0, ""]', 'user.ip:["127.1.42.0", ""]'] + list_queries = [ + f'{query_key}:[{nonnull_value}, ""]', + f'{query_key}:["{nonnull_value}", ""]', + ] for query in list_queries: response = self.client.get(self.url + f"?field=id&query={query}") assert response.status_code == 200 @@ -1767,13 +1786,24 @@ def test_query_contains_null_ipv4(self): assert len(data) == 2 assert {item["id"] for item in data} == {replay1_id, replay2_id} - negated_queries = ["!" + query for query in list_queries] - for query in negated_queries: + for query in ["!" + query for query in list_queries]: response = self.client.get(self.url + f"?field=id&query={query}") assert response.status_code == 200 data = response.json()["data"] assert len(data) == 0 + def test_query_empty_email(self): + self._test_empty_filters("user.email", "user_email", "", "andrew@example.com") + + def test_query_empty_ipv4(self): + self._test_empty_filters("user.ip", "ipv4", None, "127.0.0.1") + + def test_query_empty_username(self): + self._test_empty_filters("user.username", "user_name", "", "andrew1") + + def test_query_empty_user_id(self): + self._test_empty_filters("user.id", "user_id", "", "12ef6") + def test_query_branches_computed_activity_conditions(self): project = self.create_project(teams=[self.team]) @@ -2169,3 +2199,7 @@ def features(self): "organizations:session-replay": True, "organizations:session-replay-materialized-view": True, } + + def _test_empty_filters(self, *args, **kwargs): + # Skipping these tests since they fail. MV is unused and soon to be removed. + pass From 939be86fe916546f6020dcddeba2f5207071e8d8 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Wed, 16 Oct 2024 15:07:05 -0700 Subject: [PATCH 57/67] feat(workflow_engine): Add way to hook logic into `Detector` (#78540) This adds some ways that we can hook custom logic into the detector. Looking at the options, it feels like splitting this logic into different handlers makes sense. Doesn't have to be a function - we can implement classes so that we can share logic. --- src/sentry/incidents/grouptype.py | 32 ++++ src/sentry/issues/grouptype.py | 4 + src/sentry/testutils/factories.py | 6 +- .../workflow_engine/models/data_source.py | 11 +- src/sentry/workflow_engine/models/detector.py | 80 ++++++++- .../workflow_engine/models/detector_state.py | 4 +- .../workflow_engine/processors/detector.py | 36 ++++ .../processors/test_detector.py | 155 ++++++++++++++++++ 8 files changed, 319 insertions(+), 9 deletions(-) create mode 100644 src/sentry/incidents/grouptype.py create mode 100644 src/sentry/workflow_engine/processors/detector.py create mode 100644 tests/sentry/workflow_engine/processors/test_detector.py diff --git a/src/sentry/incidents/grouptype.py b/src/sentry/incidents/grouptype.py new file mode 100644 index 00000000000000..7a7d0dc6900d52 --- /dev/null +++ b/src/sentry/incidents/grouptype.py @@ -0,0 +1,32 @@ +from dataclasses import dataclass + +from sentry.incidents.utils.types import QuerySubscriptionUpdate +from sentry.issues.grouptype import GroupCategory, GroupType +from sentry.ratelimits.sliding_windows import Quota +from sentry.types.group import PriorityLevel +from sentry.workflow_engine.models import DataPacket +from sentry.workflow_engine.models.detector import DetectorEvaluationResult, DetectorHandler + + +# TODO: This will be a stateful detector when we build that abstraction +class MetricAlertDetectorHandler(DetectorHandler[QuerySubscriptionUpdate]): + def evaluate( + self, data_packet: DataPacket[QuerySubscriptionUpdate] + ) -> list[DetectorEvaluationResult]: + # TODO: Implement + return [] + + +# Example GroupType and detector handler for metric alerts. We don't create these issues yet, but we'll use something +# like these when we're sending issues as alerts +@dataclass(frozen=True) +class MetricAlertFire(GroupType): + type_id = 8001 + slug = "metric_alert_fire" + description = "Metric alert fired" + category = GroupCategory.METRIC_ALERT.value + creation_quota = Quota(3600, 60, 100) + default_priority = PriorityLevel.HIGH + enable_auto_resolve = False + enable_escalation_detection = False + detector_handler = MetricAlertDetectorHandler diff --git a/src/sentry/issues/grouptype.py b/src/sentry/issues/grouptype.py index 1c19909b0a452e..37f8ab41eb16e4 100644 --- a/src/sentry/issues/grouptype.py +++ b/src/sentry/issues/grouptype.py @@ -22,6 +22,7 @@ from sentry.models.organization import Organization from sentry.models.project import Project from sentry.users.models.user import User + from sentry.workflow_engine.models.detector import DetectorHandler import logging logger = logging.getLogger(__name__) @@ -35,6 +36,7 @@ class GroupCategory(Enum): REPLAY = 5 FEEDBACK = 6 UPTIME = 7 + METRIC_ALERT = 8 GROUP_CATEGORIES_CUSTOM_EMAIL = ( @@ -152,8 +154,10 @@ class GroupType: enable_auto_resolve: bool = True # Allow escalation forecasts and detection enable_escalation_detection: bool = True + # Quota around many of these issue types can be created per project in a given time window creation_quota: Quota = Quota(3600, 60, 5) # default 5 per hour, sliding window of 60 seconds notification_config: NotificationConfig = NotificationConfig() + detector_handler: type[DetectorHandler] | None = None def __init_subclass__(cls: type[GroupType], **kwargs: Any) -> None: super().__init_subclass__(**kwargs) diff --git a/src/sentry/testutils/factories.py b/src/sentry/testutils/factories.py index f61a2e5db4340e..8eefba329bf02f 100644 --- a/src/sentry/testutils/factories.py +++ b/src/sentry/testutils/factories.py @@ -2139,7 +2139,11 @@ def create_detector( if name is None: name = petname.generate(2, " ", letters=10).title() return Detector.objects.create( - organization=organization, name=name, owner_user_id=owner_user_id, owner_team=owner_team + organization=organization, + name=name, + owner_user_id=owner_user_id, + owner_team=owner_team, + **kwargs, ) @staticmethod diff --git a/src/sentry/workflow_engine/models/data_source.py b/src/sentry/workflow_engine/models/data_source.py index 4fa93e397afedd..13c409865ae574 100644 --- a/src/sentry/workflow_engine/models/data_source.py +++ b/src/sentry/workflow_engine/models/data_source.py @@ -1,4 +1,5 @@ -from typing import Protocol +import dataclasses +from typing import Generic, TypeVar from django.db import models @@ -11,9 +12,13 @@ ) from sentry.workflow_engine.models.data_source_detector import DataSourceDetector +T = TypeVar("T") -class DataPacket(Protocol): - query_id: int + +@dataclasses.dataclass +class DataPacket(Generic[T]): + query_id: str + packet: T @region_silo_model diff --git a/src/sentry/workflow_engine/models/detector.py b/src/sentry/workflow_engine/models/detector.py index e2d988a93d9c60..dcabca1c66f54a 100644 --- a/src/sentry/workflow_engine/models/detector.py +++ b/src/sentry/workflow_engine/models/detector.py @@ -1,10 +1,24 @@ +from __future__ import annotations + +import abc +import dataclasses +import logging +from typing import TYPE_CHECKING, Any, Generic, TypeVar + from django.db import models from django.db.models import UniqueConstraint from sentry.backup.scopes import RelocationScope from sentry.db.models import DefaultFieldsModel, FlexibleForeignKey, region_silo_model +from sentry.issues import grouptype from sentry.models.owner_base import OwnerModel -from sentry.workflow_engine.models.data_source_detector import DataSourceDetector +from sentry.types.group import PriorityLevel +from sentry.workflow_engine.models import DataPacket + +if TYPE_CHECKING: + from sentry.workflow_engine.models.detector_state import DetectorStatus + +logger = logging.getLogger(__name__) @region_silo_model @@ -15,7 +29,9 @@ class Detector(DefaultFieldsModel, OwnerModel): name = models.CharField(max_length=200) # The data sources that the detector is watching - data_sources = models.ManyToManyField("workflow_engine.DataSource", through=DataSourceDetector) + data_sources = models.ManyToManyField( + "workflow_engine.DataSource", through="workflow_engine.DataSourceDetector" + ) # The conditions that must be met for the detector to be considered 'active' # This will emit an event for the workflow to process @@ -35,3 +51,63 @@ class Meta(OwnerModel.Meta): name="workflow_engine_detector_org_name", ) ] + + @property + def detector_handler(self) -> DetectorHandler | None: + group_type = grouptype.registry.get_by_slug(self.type) + if not group_type: + logger.error( + "No registered grouptype for detector", + extra={ + "group_type": str(group_type), + "detector_id": self.id, + "detector_type": self.type, + }, + ) + return None + + if not group_type.detector_handler: + logger.error( + "Registered grouptype for detector has no detector_handler", + extra={ + "group_type": str(group_type), + "detector_id": self.id, + "detector_type": self.type, + }, + ) + return None + return group_type.detector_handler(self) + + +@dataclasses.dataclass(frozen=True) +class DetectorStateData: + group_key: str | None + active: bool + status: DetectorStatus + # Stateful detectors always process data packets in order. Once we confirm that a data packet has been fully + # processed and all workflows have been done, this value will be used by the stateful detector to prevent + # reprocessing + dedupe_value: int + # Stateful detectors allow various counts to be tracked. We need to update these after we process workflows, so + # include the updates in the state + counter_updates: dict[str, int] + + +@dataclasses.dataclass(frozen=True) +class DetectorEvaluationResult: + is_active: bool + priority: PriorityLevel + data: Any + state_update_data: DetectorStateData | None = None + + +T = TypeVar("T") + + +class DetectorHandler(abc.ABC, Generic[T]): + def __init__(self, detector: Detector): + self.detector = detector + + @abc.abstractmethod + def evaluate(self, data_packet: DataPacket[T]) -> list[DetectorEvaluationResult]: + pass diff --git a/src/sentry/workflow_engine/models/detector_state.py b/src/sentry/workflow_engine/models/detector_state.py index cf95b16c492290..dc68964d23dd1c 100644 --- a/src/sentry/workflow_engine/models/detector_state.py +++ b/src/sentry/workflow_engine/models/detector_state.py @@ -5,8 +5,6 @@ from sentry.backup.scopes import RelocationScope from sentry.db.models import DefaultFieldsModel, FlexibleForeignKey, region_silo_model -from .detector import Detector - class DetectorStatus(StrEnum): OK = "ok" @@ -16,7 +14,7 @@ class DetectorStatus(StrEnum): class DetectorState(DefaultFieldsModel): __relocation_scope__ = RelocationScope.Organization - detector = FlexibleForeignKey(Detector) + detector = FlexibleForeignKey("workflow_engine.Detector") # This key is used when a detector is using group-by # allows us to link to a specific group from a single detector diff --git a/src/sentry/workflow_engine/processors/detector.py b/src/sentry/workflow_engine/processors/detector.py new file mode 100644 index 00000000000000..ac34d2c44a2f23 --- /dev/null +++ b/src/sentry/workflow_engine/processors/detector.py @@ -0,0 +1,36 @@ +import logging + +from sentry.workflow_engine.models import Detector +from sentry.workflow_engine.models.data_source import DataPacket +from sentry.workflow_engine.models.detector import DetectorEvaluationResult + +logger = logging.getLogger(__name__) + + +def process_detectors( + data_packet: DataPacket, detectors: list[Detector] +) -> list[tuple[Detector, list[DetectorEvaluationResult]]]: + results = [] + for detector in detectors: + handler = detector.detector_handler + if not handler: + continue + detector_results = handler.evaluate(data_packet) + detector_group_keys = set() + for result in detector_results: + if result.state_update_data: + if result.state_update_data.group_key in detector_group_keys: + # This shouldn't happen - log an error and continue on, but we should investigate this. + logger.error( + "Duplicate detector state group keys found", + extra={ + "detector_id": detector.id, + "group_key": result.state_update_data.group_key, + }, + ) + detector_group_keys.add(result.state_update_data.group_key) + + if detector_results: + results.append((detector, detector_results)) + + return results diff --git a/tests/sentry/workflow_engine/processors/test_detector.py b/tests/sentry/workflow_engine/processors/test_detector.py new file mode 100644 index 00000000000000..f835b1d56d65c1 --- /dev/null +++ b/tests/sentry/workflow_engine/processors/test_detector.py @@ -0,0 +1,155 @@ +from unittest import mock + +from sentry.issues.grouptype import GroupCategory, GroupType +from sentry.types.group import PriorityLevel +from sentry.workflow_engine.models import DataPacket +from sentry.workflow_engine.models.detector import ( + DetectorEvaluationResult, + DetectorHandler, + DetectorStateData, +) +from sentry.workflow_engine.models.detector_state import DetectorStatus +from sentry.workflow_engine.processors.detector import process_detectors +from tests.sentry.issues.test_grouptype import BaseGroupTypeTest + + +class TestProcessDetectors(BaseGroupTypeTest): + def setUp(self): + super().setUp() + + class NoHandlerGroupType(GroupType): + type_id = 1 + slug = "no_handler" + description = "no handler" + category = GroupCategory.METRIC_ALERT.value + + class MockDetectorHandler(DetectorHandler[dict]): + def evaluate(self, data_packet: DataPacket[dict]) -> list[DetectorEvaluationResult]: + return [DetectorEvaluationResult(True, PriorityLevel.HIGH, data_packet)] + + class HandlerGroupType(GroupType): + type_id = 2 + slug = "handler" + description = "handler" + category = GroupCategory.METRIC_ALERT.value + detector_handler = MockDetectorHandler + + class MockDetectorStateHandler(DetectorHandler[dict]): + def evaluate(self, data_packet: DataPacket[dict]) -> list[DetectorEvaluationResult]: + group_keys = data_packet.packet.get("group_keys", [None]) + return [ + DetectorEvaluationResult( + True, + PriorityLevel.HIGH, + data_packet, + DetectorStateData( + group_key, + True, + DetectorStatus.OK, + 100, + {}, + ), + ) + for group_key in group_keys + ] + + class HandlerStateGroupType(GroupType): + type_id = 3 + slug = "handler_with_state" + description = "handler with state" + category = GroupCategory.METRIC_ALERT.value + detector_handler = MockDetectorStateHandler + + self.no_handler_type = NoHandlerGroupType + self.handler_type = HandlerGroupType + self.handler_state_type = HandlerStateGroupType + + def build_data_packet(self, **kwargs): + query_id = "1234" + return DataPacket[dict](query_id, {"query_id": query_id, "some": "data", **kwargs}) + + def test(self): + detector = self.create_detector(type=self.handler_type.slug) + data_packet = self.build_data_packet() + results = process_detectors(data_packet, [detector]) + assert results == [ + (detector, [DetectorEvaluationResult(True, PriorityLevel.HIGH, data_packet)]) + ] + + def test_state_results(self): + detector = self.create_detector(type=self.handler_state_type.slug) + data_packet = self.build_data_packet() + results = process_detectors(data_packet, [detector]) + result = DetectorEvaluationResult( + True, + PriorityLevel.HIGH, + data_packet, + DetectorStateData(None, True, DetectorStatus.OK, 100, {}), + ) + assert results == [ + ( + detector, + [result], + ) + ] + + def test_state_results_multi_group(self): + detector = self.create_detector(type=self.handler_state_type.slug) + data_packet = self.build_data_packet(group_keys=["group_1", "group_2"]) + results = process_detectors(data_packet, [detector]) + result_1 = DetectorEvaluationResult( + True, + PriorityLevel.HIGH, + data_packet, + DetectorStateData("group_1", True, DetectorStatus.OK, 100, {}), + ) + result_2 = DetectorEvaluationResult( + True, + PriorityLevel.HIGH, + data_packet, + DetectorStateData("group_2", True, DetectorStatus.OK, 100, {}), + ) + assert results == [ + ( + detector, + [result_1, result_2], + ) + ] + + def test_state_results_multi_group_dupe(self): + detector = self.create_detector(type=self.handler_state_type.slug) + data_packet = self.build_data_packet(group_keys=["dupe", "dupe"]) + with mock.patch("sentry.workflow_engine.processors.detector.logger") as mock_logger: + results = process_detectors(data_packet, [detector]) + assert mock_logger.error.call_args[0][0] == "Duplicate detector state group keys found" + result = DetectorEvaluationResult( + True, + PriorityLevel.HIGH, + data_packet, + DetectorStateData("dupe", True, DetectorStatus.OK, 100, {}), + ) + assert results == [ + ( + detector, + [result, result], + ) + ] + + def test_no_issue_type(self): + detector = self.create_detector(type="invalid slug") + data_packet = self.build_data_packet() + with mock.patch("sentry.workflow_engine.models.detector.logger") as mock_logger: + results = process_detectors(data_packet, [detector]) + assert mock_logger.error.call_args[0][0] == "No registered grouptype for detector" + assert results == [] + + def test_no_handler(self): + detector = self.create_detector(type=self.no_handler_type.slug) + data_packet = self.build_data_packet() + with mock.patch("sentry.workflow_engine.models.detector.logger") as mock_logger: + results = process_detectors(data_packet, [detector]) + assert ( + mock_logger.error.call_args[0][0] + == "Registered grouptype for detector has no detector_handler" + ) + assert results == [] From 1e506d7b19a6f139af422bd11a7b6d87b03d2194 Mon Sep 17 00:00:00 2001 From: Michael Sun <55160142+MichaelSun48@users.noreply.github.com> Date: Wed, 16 Oct 2024 15:40:38 -0700 Subject: [PATCH 58/67] chore(issue-stream): age -> first seen, seen -> last seen (#79139) closes #79120 closes #79121 * Reduced trend chart graph from 200px -> 175px * Age -> First Seen * Seen -> Last Seen * First/Last seen column width increased from 60px -> 75px ![image](https://github.com/user-attachments/assets/9817578f-e112-4144-9aa6-3f6fffe90562) --- static/app/components/stream/group.tsx | 6 +++--- static/app/views/issueList/actions/headers.tsx | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/static/app/components/stream/group.tsx b/static/app/components/stream/group.tsx index 52e3631c269587..b443e2fa6575ed 100644 --- a/static/app/components/stream/group.tsx +++ b/static/app/components/stream/group.tsx @@ -139,7 +139,7 @@ function GroupTimestamp({date, label}: {date: string | null; label: string}) { aria-label={label} tooltipPrefix={label} date={date} - suffix="" + suffix="ago" unitStyle="extraShort" /> ); @@ -892,7 +892,7 @@ const ChartWrapper = styled('div')<{margin: boolean; narrowGroups: boolean}>` `; const NarrowChartWrapper = styled('div')<{breakpoint: string}>` - width: 200px; + width: 175px; align-self: center; margin-right: ${space(2)}; @@ -904,7 +904,7 @@ const NarrowChartWrapper = styled('div')<{breakpoint: string}>` const TimestampWrapper = styled('div')<{breakpoint: string}>` display: flex; align-self: center; - width: 60px; + width: 75px; margin-right: ${space(2)}; @media (max-width: ${p => p.breakpoint}) { diff --git a/static/app/views/issueList/actions/headers.tsx b/static/app/views/issueList/actions/headers.tsx index c806fefcb9e531..f495544da1e5f5 100644 --- a/static/app/views/issueList/actions/headers.tsx +++ b/static/app/views/issueList/actions/headers.tsx @@ -85,11 +85,11 @@ function Headers({ {organization.features.includes('issue-stream-table-layout') ? ( - {t('Age')} + {t('First Seen')} - {t('Seen')} + {t('Last Seen')} @@ -143,7 +143,7 @@ const GraphHeaderWrapper = styled('div')<{isSavedSearchesOpen?: boolean}>` `; const NarrowGraphLabel = styled(IssueStreamHeaderLabel)` - width: 200px; + width: 175px; flex: 1; display: flex; justify-content: space-between; @@ -182,7 +182,7 @@ const GraphToggle = styled('a')<{active: boolean}>` `; const TimestampLabel = styled(IssueStreamHeaderLabel)` - width: 60px; + width: 75px; display: flex; flex-direction: row; justify-content: space-between; From 0661463609b3686407bd1c4264942f40d769155d Mon Sep 17 00:00:00 2001 From: Scott Cooper Date: Wed, 16 Oct 2024 15:41:49 -0700 Subject: [PATCH 59/67] fix(issues): Size of details container, all events overflow (#79229) --- .../views/issueDetails/groupEventDetails/groupEventDetails.tsx | 1 + static/app/views/issueDetails/streamline/eventDetails.tsx | 3 ++- static/app/views/issueDetails/streamline/eventList.tsx | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/static/app/views/issueDetails/groupEventDetails/groupEventDetails.tsx b/static/app/views/issueDetails/groupEventDetails/groupEventDetails.tsx index f7f34f1a2fbb58..c71f7b0bd9a4dc 100644 --- a/static/app/views/issueDetails/groupEventDetails/groupEventDetails.tsx +++ b/static/app/views/issueDetails/groupEventDetails/groupEventDetails.tsx @@ -240,6 +240,7 @@ const StyledLayoutBody = styled(Layout.Body)<{ ${p => p.hasStreamlinedUi && css` + min-height: 100vh; @media (min-width: ${p.theme.breakpoints.large}) { gap: ${space(1.5)}; display: ${p.sidebarOpen ? 'grid' : 'block'}; diff --git a/static/app/views/issueDetails/streamline/eventDetails.tsx b/static/app/views/issueDetails/streamline/eventDetails.tsx index f15b4d41e7c73c..155e3d6584d11a 100644 --- a/static/app/views/issueDetails/streamline/eventDetails.tsx +++ b/static/app/views/issueDetails/streamline/eventDetails.tsx @@ -123,7 +123,8 @@ export function EventDetails({ {/* TODO(issues): We should use the router for this */} {currentTab === Tab.EVENTS && ( - + {/* Overflow added only to this instance to scroll table. Acts weird with sticky nav. */} + diff --git a/static/app/views/issueDetails/streamline/eventList.tsx b/static/app/views/issueDetails/streamline/eventList.tsx index 57ea724afa5cc6..1d17a61d0ab3a9 100644 --- a/static/app/views/issueDetails/streamline/eventList.tsx +++ b/static/app/views/issueDetails/streamline/eventList.tsx @@ -178,6 +178,7 @@ const EventListHeaderItem = styled('div')` const StreamlineEventsTable = styled('div')` ${Panel} { border: 0; + margin-bottom: 0; } ${GridHead} { From 30dd4127916739c738072aaa4b56071c5b075ab9 Mon Sep 17 00:00:00 2001 From: getsentry-bot Date: Wed, 16 Oct 2024 22:44:17 +0000 Subject: [PATCH 60/67] Revert "ref(mediators): Make validator into a dataclass (#79116)" This reverts commit e2df3f1a1457a43dcc2a7b2f3944627a97522262. Co-authored-by: Christinarlong <60594860+Christinarlong@users.noreply.github.com> --- src/sentry/mediators/__init__.py | 1 + .../mediators/token_exchange/__init__.py | 2 + .../token_exchange/util.py | 0 .../token_exchange/validator.py | 39 +++++++++---------- .../endpoints/sentry_app_authorizations.py | 2 +- .../token_exchange/grant_exchanger.py | 24 ++++++------ .../sentry_apps/token_exchange/refresher.py | 13 +++---- src/sentry/web/frontend/oauth_token.py | 10 ++--- .../token_exchange/test_validator.py | 14 +++---- .../test_sentry_app_authorizations.py | 2 +- .../token_exchange/test_grant_exchanger.py | 8 ++++ .../token_exchange/test_refresher.py | 8 ++++ 12 files changed, 69 insertions(+), 54 deletions(-) rename src/sentry/{sentry_apps => mediators}/token_exchange/util.py (100%) rename src/sentry/{sentry_apps => mediators}/token_exchange/validator.py (55%) rename tests/sentry/{sentry_apps => mediators}/token_exchange/test_validator.py (86%) diff --git a/src/sentry/mediators/__init__.py b/src/sentry/mediators/__init__.py index 8e848d43bc83e3..df677f82741301 100644 --- a/src/sentry/mediators/__init__.py +++ b/src/sentry/mediators/__init__.py @@ -1,2 +1,3 @@ from .mediator import Mediator # NOQA from .param import Param # NOQA +from .token_exchange.util import AUTHORIZATION, REFRESH, GrantTypes # noqa: F401 diff --git a/src/sentry/mediators/token_exchange/__init__.py b/src/sentry/mediators/token_exchange/__init__.py index e69de29bb2d1d6..46a4f7637503ea 100644 --- a/src/sentry/mediators/token_exchange/__init__.py +++ b/src/sentry/mediators/token_exchange/__init__.py @@ -0,0 +1,2 @@ +from .util import AUTHORIZATION, REFRESH, GrantTypes, token_expiration # NOQA +from .validator import Validator # NOQA diff --git a/src/sentry/sentry_apps/token_exchange/util.py b/src/sentry/mediators/token_exchange/util.py similarity index 100% rename from src/sentry/sentry_apps/token_exchange/util.py rename to src/sentry/mediators/token_exchange/util.py diff --git a/src/sentry/sentry_apps/token_exchange/validator.py b/src/sentry/mediators/token_exchange/validator.py similarity index 55% rename from src/sentry/sentry_apps/token_exchange/validator.py rename to src/sentry/mediators/token_exchange/validator.py index 6d0cf170dcd37a..4b88fac49e4e3b 100644 --- a/src/sentry/sentry_apps/token_exchange/validator.py +++ b/src/sentry/mediators/token_exchange/validator.py @@ -1,54 +1,53 @@ -from dataclasses import dataclass - +from django.db import router from django.utils.functional import cached_property from sentry.coreapi import APIUnauthorized +from sentry.mediators.mediator import Mediator +from sentry.mediators.param import Param from sentry.models.apiapplication import ApiApplication from sentry.sentry_apps.models.sentry_app import SentryApp from sentry.sentry_apps.services.app import RpcSentryAppInstallation from sentry.users.models.user import User -@dataclass -class Validator: +class Validator(Mediator): """ Validates general authorization params for all types of token exchanges. """ - install: RpcSentryAppInstallation - client_id: str - user: User + install = Param(RpcSentryAppInstallation) + client_id = Param(str) + user = Param(User) + using = router.db_for_write(User) - def run(self) -> bool: + def call(self): self._validate_is_sentry_app_making_request() self._validate_app_is_owned_by_user() self._validate_installation() return True - def _validate_is_sentry_app_making_request(self) -> None: + def _validate_is_sentry_app_making_request(self): if not self.user.is_sentry_app: - raise APIUnauthorized("User is not a Sentry App") + raise APIUnauthorized - def _validate_app_is_owned_by_user(self) -> None: + def _validate_app_is_owned_by_user(self): if self.sentry_app.proxy_user != self.user: - raise APIUnauthorized("Sentry App does not belong to given user") + raise APIUnauthorized - def _validate_installation(self) -> None: + def _validate_installation(self): if self.install.sentry_app.id != self.sentry_app.id: - raise APIUnauthorized( - f"Sentry App Installation is not for Sentry App: {self.sentry_app.slug}" - ) + raise APIUnauthorized @cached_property - def sentry_app(self) -> SentryApp: + def sentry_app(self): try: return self.application.sentry_app except SentryApp.DoesNotExist: - raise APIUnauthorized("Sentry App does not exist") + raise APIUnauthorized @cached_property - def application(self) -> ApiApplication: + def application(self): try: return ApiApplication.objects.get(client_id=self.client_id) except ApiApplication.DoesNotExist: - raise APIUnauthorized("Application does not exist") + raise APIUnauthorized diff --git a/src/sentry/sentry_apps/api/endpoints/sentry_app_authorizations.py b/src/sentry/sentry_apps/api/endpoints/sentry_app_authorizations.py index 2bc2d56a81701f..1ff39ed50c16f4 100644 --- a/src/sentry/sentry_apps/api/endpoints/sentry_app_authorizations.py +++ b/src/sentry/sentry_apps/api/endpoints/sentry_app_authorizations.py @@ -10,10 +10,10 @@ from sentry.api.serializers.models.apitoken import ApiTokenSerializer from sentry.auth.services.auth.impl import promote_request_api_user from sentry.coreapi import APIUnauthorized +from sentry.mediators.token_exchange.util import GrantTypes from sentry.sentry_apps.api.bases.sentryapps import SentryAppAuthorizationsBaseEndpoint from sentry.sentry_apps.token_exchange.grant_exchanger import GrantExchanger from sentry.sentry_apps.token_exchange.refresher import Refresher -from sentry.sentry_apps.token_exchange.util import GrantTypes logger = logging.getLogger(__name__) diff --git a/src/sentry/sentry_apps/token_exchange/grant_exchanger.py b/src/sentry/sentry_apps/token_exchange/grant_exchanger.py index 6a6089c2f4f514..589a3126ec0785 100644 --- a/src/sentry/sentry_apps/token_exchange/grant_exchanger.py +++ b/src/sentry/sentry_apps/token_exchange/grant_exchanger.py @@ -6,14 +6,14 @@ from sentry import analytics from sentry.coreapi import APIUnauthorized +from sentry.mediators.token_exchange.util import token_expiration +from sentry.mediators.token_exchange.validator import Validator from sentry.models.apiapplication import ApiApplication from sentry.models.apigrant import ApiGrant from sentry.models.apitoken import ApiToken from sentry.sentry_apps.models.sentry_app import SentryApp from sentry.sentry_apps.models.sentry_app_installation import SentryAppInstallation from sentry.sentry_apps.services.app import RpcSentryAppInstallation -from sentry.sentry_apps.token_exchange.util import token_expiration -from sentry.sentry_apps.token_exchange.validator import Validator from sentry.silo.safety import unguarded_write from sentry.users.models.user import User @@ -31,14 +31,15 @@ class GrantExchanger: def run(self): with transaction.atomic(using=router.db_for_write(ApiToken)): - if self._validate(): - token = self._create_token() + self._validate() + token = self._create_token() - # Once it's exchanged it's no longer valid and should not be - # exchangeable, so we delete it. - self._delete_grant() - self.record_analytics() - return token + # Once it's exchanged it's no longer valid and should not be + # exchangeable, so we delete it. + self._delete_grant() + self.record_analytics() + + return token def record_analytics(self) -> None: analytics.record( @@ -47,15 +48,14 @@ def record_analytics(self) -> None: exchange_type="authorization", ) - def _validate(self) -> bool: - is_valid = Validator(install=self.install, client_id=self.client_id, user=self.user).run() + def _validate(self) -> None: + Validator.run(install=self.install, client_id=self.client_id, user=self.user) if not self._grant_belongs_to_install() or not self._sentry_app_user_owns_grant(): raise APIUnauthorized("Forbidden grant") if not self._grant_is_active(): raise APIUnauthorized("Grant has already expired.") - return is_valid def _grant_belongs_to_install(self) -> bool: return self.grant.sentry_app_installation.id == self.install.id diff --git a/src/sentry/sentry_apps/token_exchange/refresher.py b/src/sentry/sentry_apps/token_exchange/refresher.py index 9b8ed3364e00f7..f6881700eb09ab 100644 --- a/src/sentry/sentry_apps/token_exchange/refresher.py +++ b/src/sentry/sentry_apps/token_exchange/refresher.py @@ -5,13 +5,13 @@ from sentry import analytics from sentry.coreapi import APIUnauthorized +from sentry.mediators.token_exchange.util import token_expiration +from sentry.mediators.token_exchange.validator import Validator from sentry.models.apiapplication import ApiApplication from sentry.models.apitoken import ApiToken from sentry.sentry_apps.models.sentry_app import SentryApp from sentry.sentry_apps.models.sentry_app_installation import SentryAppInstallation from sentry.sentry_apps.services.app import RpcSentryAppInstallation -from sentry.sentry_apps.token_exchange.util import token_expiration -from sentry.sentry_apps.token_exchange.validator import Validator from sentry.users.models.user import User @@ -28,8 +28,8 @@ class Refresher: def run(self) -> ApiToken: with transaction.atomic(router.db_for_write(ApiToken)): - if self._validate(): - self.token.delete() + self._validate() + self.token.delete() self.record_analytics() return self._create_new_token() @@ -41,12 +41,11 @@ def record_analytics(self) -> None: exchange_type="refresh", ) - def _validate(self) -> bool: - is_valid = Validator(install=self.install, client_id=self.client_id, user=self.user).run() + def _validate(self) -> None: + Validator.run(install=self.install, client_id=self.client_id, user=self.user) if self.token.application != self.application: raise APIUnauthorized("Token does not belong to the application") - return is_valid def _create_new_token(self) -> ApiToken: token = ApiToken.objects.create( diff --git a/src/sentry/web/frontend/oauth_token.py b/src/sentry/web/frontend/oauth_token.py index 109babbecf7a26..ee8dea005f5a6f 100644 --- a/src/sentry/web/frontend/oauth_token.py +++ b/src/sentry/web/frontend/oauth_token.py @@ -9,10 +9,10 @@ from rest_framework.request import Request from sentry import options +from sentry.mediators.token_exchange.util import GrantTypes from sentry.models.apiapplication import ApiApplication, ApiApplicationStatus from sentry.models.apigrant import ApiGrant from sentry.models.apitoken import ApiToken -from sentry.sentry_apps.token_exchange.util import GrantTypes from sentry.utils import json, metrics from sentry.web.frontend.base import control_silo_view from sentry.web.frontend.openidtoken import OpenIDToken @@ -157,11 +157,9 @@ def process_token_details( token_information = { "access_token": token.token, "refresh_token": token.refresh_token, - "expires_in": ( - int((token.expires_at - timezone.now()).total_seconds()) - if token.expires_at - else None - ), + "expires_in": int((token.expires_at - timezone.now()).total_seconds()) + if token.expires_at + else None, "expires_at": token.expires_at, "token_type": "bearer", "scope": " ".join(token.get_scopes()), diff --git a/tests/sentry/sentry_apps/token_exchange/test_validator.py b/tests/sentry/mediators/token_exchange/test_validator.py similarity index 86% rename from tests/sentry/sentry_apps/token_exchange/test_validator.py rename to tests/sentry/mediators/token_exchange/test_validator.py index d3e921bcb303e0..1d81eaae8f892e 100644 --- a/tests/sentry/sentry_apps/token_exchange/test_validator.py +++ b/tests/sentry/mediators/token_exchange/test_validator.py @@ -3,9 +3,9 @@ import pytest from sentry.coreapi import APIUnauthorized +from sentry.mediators.token_exchange.validator import Validator from sentry.sentry_apps.models.sentry_app import SentryApp from sentry.sentry_apps.services.app import app_service -from sentry.sentry_apps.token_exchange.validator import Validator from sentry.testutils.cases import TestCase from sentry.testutils.silo import control_silo_test @@ -24,35 +24,35 @@ def setUp(self): ) def test_happy_path(self): - assert self.validator.run() + assert self.validator.call() def test_request_must_be_made_by_sentry_app(self): self.validator.user = self.create_user() with pytest.raises(APIUnauthorized): - self.validator.run() + self.validator.call() def test_request_user_must_own_sentry_app(self): self.validator.user = self.create_user(is_sentry_app=True) with pytest.raises(APIUnauthorized): - self.validator.run() + self.validator.call() def test_installation_belongs_to_sentry_app_with_client_id(self): self.validator.install = self.create_sentry_app_installation() with pytest.raises(APIUnauthorized): - self.validator.run() + self.validator.call() @patch("sentry.models.ApiApplication.sentry_app") def test_raises_when_sentry_app_cannot_be_found(self, sentry_app): sentry_app.side_effect = SentryApp.DoesNotExist() with pytest.raises(APIUnauthorized): - self.validator.run() + self.validator.call() def test_raises_with_invalid_client_id(self): self.validator.client_id = "123" with pytest.raises(APIUnauthorized): - self.validator.run() + self.validator.call() diff --git a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_authorizations.py b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_authorizations.py index 7ea1fcb1941648..9836e317648cfe 100644 --- a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_authorizations.py +++ b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_authorizations.py @@ -3,9 +3,9 @@ from django.urls import reverse from django.utils import timezone +from sentry.mediators.token_exchange.util import GrantTypes from sentry.models.apiapplication import ApiApplication from sentry.models.apitoken import ApiToken -from sentry.sentry_apps.token_exchange.util import GrantTypes from sentry.silo.base import SiloMode from sentry.testutils.cases import APITestCase from sentry.testutils.silo import assume_test_silo_mode, control_silo_test diff --git a/tests/sentry/sentry_apps/token_exchange/test_grant_exchanger.py b/tests/sentry/sentry_apps/token_exchange/test_grant_exchanger.py index 2385ab75cf14f5..4cc154720867d7 100644 --- a/tests/sentry/sentry_apps/token_exchange/test_grant_exchanger.py +++ b/tests/sentry/sentry_apps/token_exchange/test_grant_exchanger.py @@ -35,6 +35,14 @@ def test_adds_token_to_installation(self): token = self.grant_exchanger.run() assert SentryAppInstallation.objects.get(id=self.install.id).api_token == token + @patch("sentry.mediators.token_exchange.Validator.run") + def test_validate_generic_token_exchange_requirements(self, validator): + self.grant_exchanger.run() + + validator.assert_called_once_with( + install=self.install, client_id=self.client_id, user=self.user + ) + def test_grant_must_belong_to_installations(self): other_install = self.create_sentry_app_installation(prevent_token_exchange=True) self.grant_exchanger.code = other_install.api_grant.code diff --git a/tests/sentry/sentry_apps/token_exchange/test_refresher.py b/tests/sentry/sentry_apps/token_exchange/test_refresher.py index 3624b56895129a..6ff4723803cf89 100644 --- a/tests/sentry/sentry_apps/token_exchange/test_refresher.py +++ b/tests/sentry/sentry_apps/token_exchange/test_refresher.py @@ -42,6 +42,14 @@ def test_deletes_refreshed_token(self): self.refresher.run() assert not ApiToken.objects.filter(id=self.token.id).exists() + @patch("sentry.mediators.token_exchange.Validator.run") + def test_validates_generic_token_exchange_requirements(self, validator): + self.refresher.run() + + validator.assert_called_once_with( + install=self.install, client_id=self.client_id, user=self.user + ) + def test_validates_token_belongs_to_sentry_app(self): refresh_token = ApiToken.objects.create( user=self.user, From bbf91bdcf1d47d0a29626eec32a748ce095fcdd3 Mon Sep 17 00:00:00 2001 From: Scott Cooper Date: Wed, 16 Oct 2024 15:44:41 -0700 Subject: [PATCH 61/67] feat(issues): Cleanup group details global styles (#79241) --- .../views/issueDetails/shortIdBreadcrumb.tsx | 1 - static/less/group-detail.less | 126 +----------------- static/less/layout.less | 14 -- 3 files changed, 3 insertions(+), 138 deletions(-) diff --git a/static/app/views/issueDetails/shortIdBreadcrumb.tsx b/static/app/views/issueDetails/shortIdBreadcrumb.tsx index 0372947b847faf..de26427ccdbd6c 100644 --- a/static/app/views/issueDetails/shortIdBreadcrumb.tsx +++ b/static/app/views/issueDetails/shortIdBreadcrumb.tsx @@ -57,7 +57,6 @@ export function ShortIdBreadcrumb({ /> ul { margin: 0 2px; } - .blame { - color: lighten(@gray, 5); - - a { - color: @gray; - } - - .icon-mark-github { - position: relative; - top: 1px; - } - } - .tooltip-inner { word-wrap: break-word; text-align: left; @@ -825,20 +755,6 @@ span.val { color: @gray-dark; } } - - .back-to, - .pull-right a { - font-size: 16px; - } - - .back-to { - border-left: 1px solid @trim; - padding: 2px 10px; - display: inline-block; - margin-left: 7px; - position: relative; - top: -3px; - } } } } @@ -1001,17 +917,6 @@ ul.crumbs { } } -/** -* Responsive medium screen -* ============================================================================ -*/ - -@media (max-width: 991px) { - .group-stats-column { - float: none; - } -} - /** * Responsive small screen * ============================================================================ @@ -1066,10 +971,6 @@ ul.crumbs { } } - .user-report { - padding: 15px 0; - } - .exception { margin: 0; padding: 15px 0; @@ -1092,26 +993,5 @@ ul.crumbs { border: 0; margin-bottom: 20px; } - - .detailed-error { - border-top: 1px solid @trim; - } - } - - // Context callout - - .context-summary { - flex-direction: column; - margin-top: 0; - padding: 0; - margin-bottom: 20px; - } - - .context { - overflow: auto; - - li { - width: 800px; - } } } diff --git a/static/less/layout.less b/static/less/layout.less index e8b0a44b8ad8d5..b44ef91a5eba98 100644 --- a/static/less/layout.less +++ b/static/less/layout.less @@ -75,20 +75,6 @@ body.narrow { color: @gray-dark; } } - - .back-to, - .pull-right a { - font-size: 16px; - } - - .back-to { - border-left: 1px solid @trim; - padding: 2px 10px; - display: inline-block; - margin-left: 7px; - position: relative; - top: -3px; - } } .with-padding { From d37bcbab1114ae5c5c7ec52489d1d3d0cfe0fdec Mon Sep 17 00:00:00 2001 From: getsentry-bot Date: Wed, 16 Oct 2024 22:48:57 +0000 Subject: [PATCH 62/67] Revert "ref(mediators): Make refresher mediator to dataclass (#79112)" This reverts commit 30a12e9e02a4b7cac3976fda70c61b59414ee39f. Co-authored-by: markstory <24086+markstory@users.noreply.github.com> --- src/sentry/mediators/__init__.py | 1 + .../mediators/token_exchange/__init__.py | 1 + .../token_exchange/refresher.py | 53 ++++++++++--------- .../endpoints/sentry_app_authorizations.py | 6 +-- .../token_exchange/test_refresher.py | 22 ++++---- 5 files changed, 44 insertions(+), 39 deletions(-) rename src/sentry/{sentry_apps => mediators}/token_exchange/refresher.py (67%) rename tests/sentry/{sentry_apps => mediators}/token_exchange/test_refresher.py (89%) diff --git a/src/sentry/mediators/__init__.py b/src/sentry/mediators/__init__.py index df677f82741301..57874a2ed719fd 100644 --- a/src/sentry/mediators/__init__.py +++ b/src/sentry/mediators/__init__.py @@ -1,3 +1,4 @@ from .mediator import Mediator # NOQA from .param import Param # NOQA +from .token_exchange.refresher import Refresher # noqa: F401 from .token_exchange.util import AUTHORIZATION, REFRESH, GrantTypes # noqa: F401 diff --git a/src/sentry/mediators/token_exchange/__init__.py b/src/sentry/mediators/token_exchange/__init__.py index 46a4f7637503ea..84bcc14774369d 100644 --- a/src/sentry/mediators/token_exchange/__init__.py +++ b/src/sentry/mediators/token_exchange/__init__.py @@ -1,2 +1,3 @@ +from .refresher import Refresher # NOQA from .util import AUTHORIZATION, REFRESH, GrantTypes, token_expiration # NOQA from .validator import Validator # NOQA diff --git a/src/sentry/sentry_apps/token_exchange/refresher.py b/src/sentry/mediators/token_exchange/refresher.py similarity index 67% rename from src/sentry/sentry_apps/token_exchange/refresher.py rename to src/sentry/mediators/token_exchange/refresher.py index f6881700eb09ab..08bdb2d0bcd547 100644 --- a/src/sentry/sentry_apps/token_exchange/refresher.py +++ b/src/sentry/mediators/token_exchange/refresher.py @@ -1,10 +1,10 @@ -from dataclasses import dataclass - -from django.db import router, transaction +from django.db import router from django.utils.functional import cached_property from sentry import analytics from sentry.coreapi import APIUnauthorized +from sentry.mediators.mediator import Mediator +from sentry.mediators.param import Param from sentry.mediators.token_exchange.util import token_expiration from sentry.mediators.token_exchange.validator import Validator from sentry.models.apiapplication import ApiApplication @@ -15,39 +15,42 @@ from sentry.users.models.user import User -@dataclass -class Refresher: +class Refresher(Mediator): """ Exchanges a Refresh Token for a new Access Token """ - install: RpcSentryAppInstallation - refresh_token: str - client_id: str - user: User - - def run(self) -> ApiToken: - with transaction.atomic(router.db_for_write(ApiToken)): - self._validate() - self.token.delete() + install = Param(RpcSentryAppInstallation) + refresh_token = Param(str) + client_id = Param(str) + user = Param(User) + using = router.db_for_write(User) - self.record_analytics() + def call(self): + self._validate() + self._delete_token() return self._create_new_token() - def record_analytics(self) -> None: + def record_analytics(self): analytics.record( "sentry_app.token_exchanged", sentry_app_installation_id=self.install.id, exchange_type="refresh", ) - def _validate(self) -> None: + def _validate(self): Validator.run(install=self.install, client_id=self.client_id, user=self.user) + self._validate_token_belongs_to_app() + + def _validate_token_belongs_to_app(self): if self.token.application != self.application: - raise APIUnauthorized("Token does not belong to the application") + raise APIUnauthorized + + def _delete_token(self): + self.token.delete() - def _create_new_token(self) -> ApiToken: + def _create_new_token(self): token = ApiToken.objects.create( user=self.user, application=self.application, @@ -61,22 +64,22 @@ def _create_new_token(self) -> ApiToken: return token @cached_property - def token(self) -> ApiToken: + def token(self): try: return ApiToken.objects.get(refresh_token=self.refresh_token) except ApiToken.DoesNotExist: - raise APIUnauthorized("Token does not exist") + raise APIUnauthorized @cached_property - def application(self) -> ApiApplication: + def application(self): try: return ApiApplication.objects.get(client_id=self.client_id) except ApiApplication.DoesNotExist: - raise APIUnauthorized("Application does not exist") + raise APIUnauthorized @property - def sentry_app(self) -> SentryApp: + def sentry_app(self): try: return self.application.sentry_app except SentryApp.DoesNotExist: - raise APIUnauthorized("Sentry App does not exist") + raise APIUnauthorized diff --git a/src/sentry/sentry_apps/api/endpoints/sentry_app_authorizations.py b/src/sentry/sentry_apps/api/endpoints/sentry_app_authorizations.py index 1ff39ed50c16f4..d705b2bd5a71e3 100644 --- a/src/sentry/sentry_apps/api/endpoints/sentry_app_authorizations.py +++ b/src/sentry/sentry_apps/api/endpoints/sentry_app_authorizations.py @@ -10,10 +10,10 @@ from sentry.api.serializers.models.apitoken import ApiTokenSerializer from sentry.auth.services.auth.impl import promote_request_api_user from sentry.coreapi import APIUnauthorized +from sentry.mediators.token_exchange.refresher import Refresher from sentry.mediators.token_exchange.util import GrantTypes from sentry.sentry_apps.api.bases.sentryapps import SentryAppAuthorizationsBaseEndpoint from sentry.sentry_apps.token_exchange.grant_exchanger import GrantExchanger -from sentry.sentry_apps.token_exchange.refresher import Refresher logger = logging.getLogger(__name__) @@ -41,12 +41,12 @@ def post(self, request: Request, installation) -> Response: user=promote_request_api_user(request), ).run() elif request.json_body.get("grant_type") == GrantTypes.REFRESH: - token = Refresher( + token = Refresher.run( install=installation, refresh_token=request.json_body.get("refresh_token"), client_id=request.json_body.get("client_id"), user=promote_request_api_user(request), - ).run() + ) else: return Response({"error": "Invalid grant_type"}, status=403) except APIUnauthorized as e: diff --git a/tests/sentry/sentry_apps/token_exchange/test_refresher.py b/tests/sentry/mediators/token_exchange/test_refresher.py similarity index 89% rename from tests/sentry/sentry_apps/token_exchange/test_refresher.py rename to tests/sentry/mediators/token_exchange/test_refresher.py index 6ff4723803cf89..6937c12705e3a6 100644 --- a/tests/sentry/sentry_apps/token_exchange/test_refresher.py +++ b/tests/sentry/mediators/token_exchange/test_refresher.py @@ -3,12 +3,12 @@ import pytest from sentry.coreapi import APIUnauthorized +from sentry.mediators.token_exchange.refresher import Refresher from sentry.models.apiapplication import ApiApplication from sentry.models.apitoken import ApiToken from sentry.sentry_apps.models.sentry_app import SentryApp from sentry.sentry_apps.models.sentry_app_installation import SentryAppInstallation from sentry.sentry_apps.services.app import app_service -from sentry.sentry_apps.token_exchange.refresher import Refresher from sentry.testutils.cases import TestCase from sentry.testutils.silo import control_silo_test @@ -32,19 +32,19 @@ def setUp(self): ) def test_happy_path(self): - assert self.refresher.run() + assert self.refresher.call() def test_adds_token_to_installation(self): - token = self.refresher.run() + token = self.refresher.call() assert SentryAppInstallation.objects.get(id=self.install.id).api_token == token def test_deletes_refreshed_token(self): - self.refresher.run() + self.refresher.call() assert not ApiToken.objects.filter(id=self.token.id).exists() @patch("sentry.mediators.token_exchange.Validator.run") def test_validates_generic_token_exchange_requirements(self, validator): - self.refresher.run() + self.refresher.call() validator.assert_called_once_with( install=self.install, client_id=self.client_id, user=self.user @@ -59,31 +59,31 @@ def test_validates_token_belongs_to_sentry_app(self): self.refresher.refresh_token = refresh_token with pytest.raises(APIUnauthorized): - self.refresher.run() + self.refresher.call() @patch("sentry.models.ApiToken.objects.get", side_effect=ApiToken.DoesNotExist) def test_token_must_exist(self, _): with pytest.raises(APIUnauthorized): - self.refresher.run() + self.refresher.call() @patch("sentry.models.ApiApplication.objects.get", side_effect=ApiApplication.DoesNotExist) def test_api_application_must_exist(self, _): with pytest.raises(APIUnauthorized): - self.refresher.run() + self.refresher.call() @patch("sentry.models.ApiApplication.sentry_app", side_effect=SentryApp.DoesNotExist) def test_sentry_app_must_exist(self, _): with pytest.raises(APIUnauthorized): - self.refresher.run() + self.refresher.call() @patch("sentry.analytics.record") def test_records_analytics(self, record): - Refresher( + Refresher.run( install=self.install, client_id=self.client_id, refresh_token=self.token.refresh_token, user=self.user, - ).run() + ) record.assert_called_with( "sentry_app.token_exchanged", From 337d501e2566513c72e95f5efd637f213ca49f7f Mon Sep 17 00:00:00 2001 From: Ryan Albrecht Date: Wed, 16 Oct 2024 16:04:53 -0700 Subject: [PATCH 63/67] feat: Convert subdomain origins into the root sentry.io origin when logged in to the toolbar (#79242) The situation is this: 1. When the toolbar renders into a page, it's also rendering a page from sentry.io into an iframe. That page is specifically `https://sentry.io/toolbar/:org/:project/iframe/` -> note: no subdomain 2. This page renders a button, which tries to open `https://sentry.io/toolbar/:org/:project/login-success/` -> note: no subdomain - We remove the subdomain here so that the login flow remembers what the target path is and redirects there. If we included subdomain then the redirect path wouldn't work and we endup at `/issues` - But it turns out we also redirect to the subdomain of the org as well, we now we've ended up at `https://:org.sentry.io/toolbar/:org/:project/login-success/` 3. This PR makes the change to convert `window.location.origin` from `org.sentry.io` into `sentry.io` so that we can call `postMessage` against the original origin from step 1 above. The origins need to match and now they will! --- src/sentry/templates/sentry/toolbar/login-success.html | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/sentry/templates/sentry/toolbar/login-success.html b/src/sentry/templates/sentry/toolbar/login-success.html index 6fbbd5508654ba..90529a776a2a28 100644 --- a/src/sentry/templates/sentry/toolbar/login-success.html +++ b/src/sentry/templates/sentry/toolbar/login-success.html @@ -16,7 +16,7 @@ {% script %}