Skip to content

Commit

Permalink
option to set limit for number of issues returned
Browse files Browse the repository at this point in the history
  • Loading branch information
cathteng committed Jul 16, 2024
1 parent 129178f commit 8d48f2b
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 24 deletions.
17 changes: 14 additions & 3 deletions src/sentry/api/endpoints/organization_pull_request_file_issues.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,15 @@
from sentry.tasks.integrations.github.language_parsers import PATCH_PARSERS
from sentry.tasks.integrations.github.open_pr_comment import (
get_projects_and_filenames_from_source_file,
get_top_5_issues_by_count_for_file,
get_top_issues_by_count_for_file,
)


class PullRequestFileSerializer(serializers.Serializer):
filename = serializers.CharField(required=True)
repo = serializers.CharField(required=True)
patch = serializers.CharField(required=True)
limit = serializers.IntegerField(required=False, default=5)

def validate_filename(self, value):
if not value:
Expand All @@ -34,6 +35,12 @@ def validate_filename(self, value):

return value

def validate_limit(self, value):
if value and value < 1 or value > 100:
raise serializers.ValidationError("Issue count must be between 1 and 100")

return value


@region_silo_endpoint
class OrganizationPullRequestFilesIssuesEndpoint(OrganizationEndpoint):
Expand All @@ -50,6 +57,7 @@ def post(self, request: Request, organization: Organization) -> Response:
filename = serializer.validated_data["filename"]
repo_name = serializer.validated_data["repo"]
patch = serializer.validated_data["patch"]
limit = serializer.validated_data["limit"]

projects, sentry_filenames = get_projects_and_filenames_from_source_file(
org_id=organization.id, repo_name=repo_name, pr_filename=filename
Expand All @@ -66,8 +74,11 @@ def post(self, request: Request, organization: Organization) -> Response:
if not len(function_names):
return Response([])

top_issues = get_top_5_issues_by_count_for_file(
list(projects), list(sentry_filenames), list(function_names)
top_issues = get_top_issues_by_count_for_file(
projects=list(projects),
sentry_filenames=list(sentry_filenames),
function_names=list(function_names),
limit=limit,
)

group_id_to_info = {}
Expand Down
11 changes: 7 additions & 4 deletions src/sentry/tasks/integrations/github/open_pr_comment.py
Original file line number Diff line number Diff line change
Expand Up @@ -280,8 +280,11 @@ def get_projects_and_filenames_from_source_file(
return project_list, sentry_filenames


def get_top_5_issues_by_count_for_file(
projects: list[Project], sentry_filenames: list[str], function_names: list[str]
def get_top_issues_by_count_for_file(
projects: list[Project],
sentry_filenames: list[str],
function_names: list[str],
limit: int | None = 5,
) -> list[dict[str, Any]]:
"""
Given a list of projects, Github filenames reverse-codemapped into filenames in Sentry,
Expand Down Expand Up @@ -401,7 +404,7 @@ def get_top_5_issues_by_count_for_file(
]
)
.set_orderby([OrderBy(Column("event_count"), Direction.DESC)])
.set_limit(5)
.set_limit(limit)
)

request = SnubaRequest(
Expand Down Expand Up @@ -554,7 +557,7 @@ def open_pr_comment_workflow(pr_id: int) -> None:
if not len(function_names):
continue

top_issues = get_top_5_issues_by_count_for_file(
top_issues = get_top_issues_by_count_for_file(
list(projects), list(sentry_filenames), list(function_names)
)
if not len(top_issues):
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from unittest.mock import patch

from rest_framework import status

from sentry.models.group import Group
from sentry.testutils.cases import APITestCase
from tests.sentry.tasks.integrations.github.test_open_pr_comment import CreateEventTestCase
Expand Down Expand Up @@ -69,6 +71,42 @@ def test_simple(self, mock_reverse_codemappings):
f"http://testserver/organizations/{self.organization.slug}/issues/{self.group_id_1}/",
]

def test_limit_validation(self, mock_reverse_codemappings):
mock_reverse_codemappings.return_value = ([self.project], ["foo.py"])

patch = """@@ -36,6 +36,7 @@\n def blue(self):"""
self.get_error_response(
self.organization.slug,
**{
"filename": "foo.py",
"repo": self.gh_repo.name,
"patch": patch,
"limit": -1,
},
status_code=status.HTTP_400_BAD_REQUEST,
)

self.get_error_response(
self.organization.slug,
**{
"filename": "foo.py",
"repo": self.gh_repo.name,
"patch": patch,
"limit": 101,
},
status_code=status.HTTP_400_BAD_REQUEST,
)

self.get_success_response(
self.organization.slug,
**{
"filename": "foo.py",
"repo": self.gh_repo.name,
"patch": patch,
"limit": 100,
},
)

def test_no_codemappings(self, mock_reverse_codemappings):
mock_reverse_codemappings.return_value = ([], [])

Expand Down
34 changes: 17 additions & 17 deletions tests/sentry/tasks/integrations/github/test_open_pr_comment.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
get_issue_table_contents,
get_pr_files,
get_projects_and_filenames_from_source_file,
get_top_5_issues_by_count_for_file,
get_top_issues_by_count_for_file,
open_pr_comment_workflow,
safe_for_comment,
)
Expand Down Expand Up @@ -367,7 +367,7 @@ def test_simple(self):
group_id = [
self._create_event(function_names=["blue", "planet"], user_id=str(i)) for i in range(7)
][0].group.id
top_5_issues = get_top_5_issues_by_count_for_file(
top_5_issues = get_top_issues_by_count_for_file(
[self.project], ["baz.py"], ["world", "planet"]
)

Expand All @@ -394,7 +394,7 @@ def test_javascript_simple(self):
)
for i in range(6)
][0].group.id
top_5_issues = get_top_5_issues_by_count_for_file(
top_5_issues = get_top_issues_by_count_for_file(
[self.project], ["baz.js"], ["world", "planet"]
)
top_5_issue_ids = [issue["group_id"] for issue in top_5_issues]
Expand All @@ -420,7 +420,7 @@ def test_php_simple(self):
)
for i in range(6)
][0].group.id
top_5_issues = get_top_5_issues_by_count_for_file(
top_5_issues = get_top_issues_by_count_for_file(
[self.project], ["baz.php"], ["world", "planet"]
)
top_5_issue_ids = [issue["group_id"] for issue in top_5_issues]
Expand All @@ -446,7 +446,7 @@ def test_ruby_simple(self):
)
for i in range(6)
][0].group.id
top_5_issues = get_top_5_issues_by_count_for_file(
top_5_issues = get_top_issues_by_count_for_file(
[self.project], ["baz.rb"], ["world", "planet"]
)
top_5_issue_ids = [issue["group_id"] for issue in top_5_issues]
Expand All @@ -460,13 +460,13 @@ def test_filters_resolved_issue(self):
group.status = GroupStatus.RESOLVED
group.save()

top_5_issues = get_top_5_issues_by_count_for_file([self.project], ["baz.py"], ["world"])
top_5_issues = get_top_issues_by_count_for_file([self.project], ["baz.py"], ["world"])
assert len(top_5_issues) == 0

def test_filters_handled_issue(self):
group_id = self._create_event(filenames=["bar.py", "baz.py"], handled=True).group.id

top_5_issues = get_top_5_issues_by_count_for_file([self.project], ["baz.py"], ["world"])
top_5_issues = get_top_issues_by_count_for_file([self.project], ["baz.py"], ["world"])
top_5_issue_ids = [issue["group_id"] for issue in top_5_issues]
assert group_id != self.group_id
assert top_5_issue_ids == [self.group_id]
Expand All @@ -475,7 +475,7 @@ def test_project_group_id_mismatch(self):
# we fetch all group_ids that belong to the projects passed into the function
self._create_event(project_id=self.another_org_project.id)

top_5_issues = get_top_5_issues_by_count_for_file([self.project], ["baz.py"], ["world"])
top_5_issues = get_top_issues_by_count_for_file([self.project], ["baz.py"], ["world"])
top_5_issue_ids = [issue["group_id"] for issue in top_5_issues]
assert top_5_issue_ids == [self.group_id]

Expand All @@ -484,7 +484,7 @@ def test_filename_mismatch(self):
filenames=["foo.py", "bar.py"],
).group.id

top_5_issues = get_top_5_issues_by_count_for_file([self.project], ["baz.py"], ["world"])
top_5_issues = get_top_issues_by_count_for_file([self.project], ["baz.py"], ["world"])
top_5_issue_ids = [issue["group_id"] for issue in top_5_issues]
assert group_id != self.group_id
assert top_5_issue_ids == [self.group_id]
Expand All @@ -494,7 +494,7 @@ def test_function_name_mismatch(self):
function_names=["world", "hello"],
).group.id

top_5_issues = get_top_5_issues_by_count_for_file([self.project], ["baz.py"], ["world"])
top_5_issues = get_top_issues_by_count_for_file([self.project], ["baz.py"], ["world"])
top_5_issue_ids = [issue["group_id"] for issue in top_5_issues]
assert group_id != self.group_id
assert top_5_issue_ids == [self.group_id]
Expand All @@ -504,7 +504,7 @@ def test_not_first_frame(self):
function_names=["world", "hello"], filenames=["baz.py", "bar.py"], culprit="hi"
).group.id

top_5_issues = get_top_5_issues_by_count_for_file([self.project], ["baz.py"], ["world"])
top_5_issues = get_top_issues_by_count_for_file([self.project], ["baz.py"], ["world"])
top_5_issue_ids = [issue["group_id"] for issue in top_5_issues]
function_names = [issue["function_name"] for issue in top_5_issues]
assert group_id != self.group_id
Expand All @@ -516,7 +516,7 @@ def test_not_within_frame_limit(self):
filenames = ["baz.py"] + ["foo.py" for _ in range(STACKFRAME_COUNT)]
group_id = self._create_event(function_names=function_names, filenames=filenames).group.id

top_5_issues = get_top_5_issues_by_count_for_file([self.project], ["baz.py"], ["world"])
top_5_issues = get_top_issues_by_count_for_file([self.project], ["baz.py"], ["world"])
top_5_issue_ids = [issue["group_id"] for issue in top_5_issues]
assert group_id != self.group_id
assert top_5_issue_ids == [self.group_id]
Expand All @@ -526,7 +526,7 @@ def test_event_too_old(self):
timestamp=iso_format(before_now(days=15)), filenames=["bar.py", "baz.py"]
).group.id

top_5_issues = get_top_5_issues_by_count_for_file([self.project], ["baz.py"], ["world"])
top_5_issues = get_top_issues_by_count_for_file([self.project], ["baz.py"], ["world"])
top_5_issue_ids = [issue["group_id"] for issue in top_5_issues]
assert group_id != self.group_id
assert top_5_issue_ids == [self.group_id]
Expand All @@ -553,7 +553,7 @@ def test_squashes_same_title_culprit_issues(self):
for i in range(5)
][0].group_id

top_5_issues = get_top_5_issues_by_count_for_file(
top_5_issues = get_top_issues_by_count_for_file(
[self.project], ["baz.py"], ["world", "planet"]
)
top_5_issue_ids = [issue["group_id"] for issue in top_5_issues]
Expand Down Expand Up @@ -607,7 +607,7 @@ def test_fetches_top_five_issues(self):
# unrelated issue with same stack trace in different project
self._create_event(project_id=self.another_org_project.id)

top_5_issues = get_top_5_issues_by_count_for_file(
top_5_issues = get_top_issues_by_count_for_file(
[self.project], ["baz.py"], ["world", "planet"]
)
top_5_issue_ids = [issue["group_id"] for issue in top_5_issues]
Expand Down Expand Up @@ -659,7 +659,7 @@ def test_get_issue_table_contents(self):
for i in range(2)
][0].group.id

top_5_issues = get_top_5_issues_by_count_for_file(
top_5_issues = get_top_issues_by_count_for_file(
[self.project], ["baz.py"], ["world", "planet"]
)
affected_users = [6, 5, 4, 3, 2]
Expand Down Expand Up @@ -821,7 +821,7 @@ def test_comment_format_missing_language(self):
@patch(
"sentry.tasks.integrations.github.language_parsers.PythonParser.extract_functions_from_patch"
)
@patch("sentry.tasks.integrations.github.open_pr_comment.get_top_5_issues_by_count_for_file")
@patch("sentry.tasks.integrations.github.open_pr_comment.get_top_issues_by_count_for_file")
@patch("sentry.tasks.integrations.github.open_pr_comment.safe_for_comment")
@patch("sentry.tasks.integrations.github.utils.metrics")
class TestOpenPRCommentWorkflow(IntegrationTestCase, CreateEventTestCase):
Expand Down

0 comments on commit 8d48f2b

Please sign in to comment.