Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref: fix typing of jira_server.integration #86388

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

ref: fix typing of jira_server.integration #86388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab6c159
ref: fix typing of jira_server.integration
asottile-sentry Mar 5, 2025
e92b9f5
:knife: regenerate mypy module blocklist
getsantry[bot] Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,13 +148,11 @@ module = [
"sentry.integrations.jira.integration",
"sentry.integrations.jira.webhooks.base",
"sentry.integrations.jira.webhooks.issue_updated",
"sentry.integrations.jira_server.integration",
"sentry.integrations.msteams.client",
"sentry.integrations.msteams.integration",
"sentry.integrations.msteams.notifications",
"sentry.integrations.pagerduty.actions.form",
"sentry.integrations.pagerduty.client",
"sentry.integrations.pagerduty.integration",
"sentry.integrations.pipeline",
"sentry.integrations.slack.message_builder.notifications.issues",
"sentry.integrations.slack.notifications",
Expand Down
3 changes: 1 addition & 2 deletions src/sentry/integrations/api/serializers/models/integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,7 @@ def serialize(
config_data = obj.config if include_config else None
else:
try:
# just doing this to avoid querying for an object we already have
installation._org_integration = obj
installation.org_integration = obj
config_data = installation.get_config_data() if include_config else None # type: ignore[assignment]
dynamic_display_information = installation.get_dynamic_display_information()
except ApiError as e:
Expand Down
26 changes: 12 additions & 14 deletions src/sentry/integrations/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,22 +356,18 @@ class IntegrationInstallation(abc.ABC):
def __init__(self, model: RpcIntegration | Integration, organization_id: int) -> None:
self.model = model
self.organization_id = organization_id
self._org_integration: RpcOrganizationIntegration | None

@property
def org_integration(self) -> RpcOrganizationIntegration | None:
@cached_property
def org_integration(self) -> RpcOrganizationIntegration:
from sentry.integrations.services.integration import integration_service

if not hasattr(self, "_org_integration"):
self._org_integration = integration_service.get_organization_integration(
integration_id=self.model.id,
organization_id=self.organization_id,
)
return self._org_integration

@org_integration.setter
def org_integration(self, org_integration: RpcOrganizationIntegration) -> None:
self._org_integration = org_integration
integration = integration_service.get_organization_integration(
integration_id=self.model.id,
organization_id=self.organization_id,
)
if integration is None:
raise NotFound("missing org_integration")
return integration
Comment on lines -359 to +370
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the "big" change in this changeset that I'm not certain about -- it makes it so that we can't construct an integration with a missing OrganizationIntegration

I'm not certain that this is correct -- but it seems most of the integrations do not handle the case where this is missing and as far as I can understand it doesn't really make sense to have an integration not associated with an organization -- will need the domain owners to confirm this idea though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I brought this up with my team to double check, and we think this is a fair assumption to make. Most of our integrations operate with the assumption that we have an org integration already, so it should be fine to handle the potentially few cases that don't explicitly with a try/catch.


@cached_property
def organization(self) -> RpcOrganization:
Expand Down Expand Up @@ -401,10 +397,12 @@ def update_organization_config(self, data: MutableMapping[str, Any]) -> None:

config = self.org_integration.config
config.update(data)
self.org_integration = integration_service.update_organization_integration(
org_integration = integration_service.update_organization_integration(
org_integration_id=self.org_integration.id,
config=config,
)
if org_integration is not None:
self.org_integration = org_integration

def get_config_data(self) -> Mapping[str, str]:
if not self.org_integration:
Expand Down
104 changes: 85 additions & 19 deletions src/sentry/integrations/jira_server/integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import logging
import re
from collections.abc import Mapping, Sequence
from typing import Any
from typing import Any, NotRequired, TypedDict
from urllib.parse import urlparse

from cryptography.hazmat.backends import default_backend
Expand Down Expand Up @@ -114,6 +114,49 @@
)


class _Project(TypedDict):
value: str
label: str


class _AddDropDown(TypedDict):
emptyMessage: str
noResultsMessage: str
items: list[_Project]


class _Choices(TypedDict):
choices: list[tuple[str, str]]
placeholder: str


class _MappedSelectors(TypedDict):
on_resolve: _Choices
on_unresolve: _Choices


class _ColumnLabels(TypedDict):
on_resolve: str
on_unresolve: str


class _Config(TypedDict):
name: str
type: str
label: str
help: str | str
placeholder: NotRequired[str]
choices: NotRequired[list[tuple[str, str]]]
addButtonText: NotRequired[str]
addDropdown: NotRequired[_AddDropDown]
mappedSelectors: NotRequired[_MappedSelectors]
columnLabels: NotRequired[_ColumnLabels]
mappedColumnLabel: NotRequired[str]
formatMessageValue: NotRequired[bool]
disabled: NotRequired[bool]
disabledReason: NotRequired[str]


class InstallationForm(forms.Form):
url = forms.CharField(
label=_("Jira URL"),
Expand Down Expand Up @@ -200,6 +243,9 @@ def dispatch(self, request: HttpRequest, pipeline: Pipeline) -> HttpResponseBase
return pipeline.next_step()

config = pipeline.fetch_state("installation_data")
if config is None:
return pipeline.error("Missing installation_data")

client = JiraServerSetupClient(
config.get("url"),
config.get("consumer_key"),
Expand Down Expand Up @@ -237,6 +283,9 @@ class OAuthCallbackView(PipelineView):
@method_decorator(csrf_exempt)
def dispatch(self, request: HttpRequest, pipeline: Pipeline) -> HttpResponseBase:
config = pipeline.fetch_state("installation_data")
if config is None:
return pipeline.error("Missing installation_data")

client = JiraServerSetupClient(
config.get("url"),
config.get("consumer_key"),
Expand Down Expand Up @@ -298,7 +347,7 @@ def get_client(self):
)

def get_organization_config(self):
configuration = [
configuration: list[_Config] = [
{
"name": self.outbound_status_key,
"type": "choice_mapper",
Expand Down Expand Up @@ -384,7 +433,9 @@ def get_organization_config(self):
configuration[0]["mappedSelectors"]["on_resolve"]["choices"] = statuses
configuration[0]["mappedSelectors"]["on_unresolve"]["choices"] = statuses

projects = [{"value": p["id"], "label": p["name"]} for p in client.get_projects_list()]
projects: list[_Project] = [
{"value": p["id"], "label": p["name"]} for p in client.get_projects_list()
]
configuration[0]["addDropdown"]["items"] = projects
except ApiError:
configuration[0]["disabled"] = True
Expand All @@ -395,9 +446,12 @@ def get_organization_config(self):
context = organization_service.get_organization_by_id(
id=self.organization_id, include_teams=False, include_projects=False
)
organization = context.organization
if context is not None:
organization = context.organization
has_issue_sync = features.has("organizations:integrations-issue-sync", organization)
else:
has_issue_sync = False

has_issue_sync = features.has("organizations:integrations-issue-sync", organization)
if not has_issue_sync:
for field in configuration:
field["disabled"] = True
Expand Down Expand Up @@ -450,10 +504,12 @@ def update_organization_config(self, data):
data[self.issues_ignored_fields_key] = ignored_fields_list

config.update(data)
self.org_integration = integration_service.update_organization_integration(
org_integration = integration_service.update_organization_integration(
org_integration_id=self.org_integration.id,
config=config,
)
if org_integration is not None:
self.org_integration = org_integration

def get_config_data(self):
config = self.org_integration.config
Expand All @@ -473,7 +529,7 @@ def get_config_data(self):
)
return config

def sync_metadata(self):
def sync_metadata(self) -> None:
client = self.get_client()

try:
Expand All @@ -491,7 +547,11 @@ def sync_metadata(self):
avatar = projects[0]["avatarUrls"]["48x48"]
self.model.metadata.update({"icon": avatar})

self.model.save()
integration_service.update_integration(
integration_id=self.model.id,
name=self.model.name,
metadata=self.model.metadata,
)
Comment on lines +550 to +554
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this change made to address the case where model is an RpcIntegration?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep -- it would otherwise crash with AttributeError: save


def get_link_issue_config(self, group, **kwargs):
fields = super().get_link_issue_config(group, **kwargs)
Expand Down Expand Up @@ -564,8 +624,9 @@ def create_comment(self, issue_id, user_id, group_note):
quoted_comment = self.create_comment_attribution(user_id, comment)
return self.get_client().create_comment(issue_id, quoted_comment)

def create_comment_attribution(self, user_id, comment_text):
def create_comment_attribution(self, user_id: int, comment_text: str) -> str:
user = user_service.get_user(user_id=user_id)
assert user is not None
attribution = f"{user.name} wrote:\n\n"
return f"{attribution}{{quote}}{comment_text}{{quote}}"

Expand Down Expand Up @@ -651,13 +712,15 @@ def build_dynamic_field(self, field_meta, group=None):
or schema["type"] == "issuelink"
):
fieldtype = "select"
organization = (
group.organization
if group
else organization_service.get_organization_by_id(
if group is not None:
organization = group.organization
else:
ctx = organization_service.get_organization_by_id(
id=self.organization_id, include_teams=False, include_projects=False
).organization
)
)
assert ctx is not None
organization = ctx.organization

fkwargs["url"] = self.search_url(organization.slug)
fkwargs["choices"] = []
elif schema["type"] in ["timetracking"]:
Expand Down Expand Up @@ -717,7 +780,7 @@ def get_projects(self, cached=True):
return jira_projects

@all_silo_function
def get_create_issue_config(self, group: Group | None, user: RpcUser | User, **kwargs):
def get_create_issue_config(self, group: Group | None, user: User, **kwargs):
"""
We use the `group` to get three things: organization_slug, project
defaults, and default title and description. In the case where we're
Expand Down Expand Up @@ -1284,9 +1347,12 @@ def create_webhook(self, external_id, webhook_secret, install, credentials):
"jira-server.webhook.failed",
extra={"error": str(err), "external_id": external_id},
)
try:
details = next(x for x in err.json["messages"][0].values())
except (KeyError, TypeError, StopIteration):
if err.json is None:
details = ""
else:
try:
details = next(x for x in err.json["messages"][0].values())
except (KeyError, TypeError, StopIteration):
details = ""
message = f"Could not create issue webhook in Jira. {details}"
raise IntegrationError(message)
4 changes: 3 additions & 1 deletion src/sentry/integrations/mixins/issues.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,10 +222,12 @@ def store_issue_last_defaults(self, project: Project, user: RpcUser | User, data
new_config.setdefault("project_issue_defaults", {}).setdefault(
str(project.id), {}
).update(project_defaults)
self.org_integration = integration_service.update_organization_integration(
org_integration = integration_service.update_organization_integration(
org_integration_id=self.org_integration.id,
config=new_config,
)
if org_integration is not None:
self.org_integration = org_integration

user_persisted_fields = self.get_persisted_user_default_config_fields()
if user_persisted_fields:
Expand Down
33 changes: 0 additions & 33 deletions tests/sentry/integrations/github/test_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,39 +627,6 @@ def test_get_stacktrace_link_file_doesnt_exists(self):

assert not result

@responses.activate
def test_get_stacktrace_link_no_org_integration(self):
self.assert_setup_flow()
integration = Integration.objects.get(provider=self.provider.key)

with assume_test_silo_mode(SiloMode.REGION):
repo = Repository.objects.create(
organization_id=self.organization.id,
name="Test-Organization/foo",
url="https://github.com/Test-Organization/foo",
provider="integrations:github",
external_id=123,
config={"name": "Test-Organization/foo"},
integration_id=integration.id,
)
path = "README.md"
version = "master"
default = "master"
responses.add(
responses.HEAD,
self.base_url + f"/repos/{repo.name}/contents/{path}?ref={version}",
status=404,
)
OrganizationIntegration.objects.get(
integration=integration, organization_id=self.organization.id
).delete()
installation = get_installation_of_type(
GitHubIntegration, integration, self.organization.id
)
result = installation.get_stacktrace_link(repo, path, default, version)

assert not result

@responses.activate
def test_get_stacktrace_link_use_default_if_version_404(self):
self.assert_setup_flow()
Expand Down
35 changes: 0 additions & 35 deletions tests/sentry/integrations/github_enterprise/test_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,41 +332,6 @@ def test_get_stacktrace_link_file_doesnt_exists(self, get_jwt, _):

assert not result

@patch("sentry.integrations.github_enterprise.integration.get_jwt", return_value="jwt_token_1")
@patch("sentry.integrations.github_enterprise.client.get_jwt", return_value="jwt_token_1")
@responses.activate
def test_get_stacktrace_link_no_org_integration(self, get_jwt, _):
self.assert_setup_flow()
integration = Integration.objects.get(provider=self.provider.key)

with assume_test_silo_mode(SiloMode.REGION):
repo = Repository.objects.create(
organization_id=self.organization.id,
name="Test-Organization/foo",
url="https://github.example.org/Test-Organization/foo",
provider="integrations:github_enterprise",
external_id=123,
config={"name": "Test-Organization/foo"},
integration_id=integration.id,
)
path = "README.md"
version = "master"
default = "master"
responses.add(
responses.HEAD,
self.base_url + f"/repos/{repo.name}/contents/{path}?ref={version}",
status=404,
)
OrganizationIntegration.objects.get(
integration=integration, organization_id=self.organization.id
).delete()
installation = get_installation_of_type(
GitHubEnterpriseIntegration, integration, self.organization.id
)
result = installation.get_stacktrace_link(repo, path, default, version)

assert not result

@patch("sentry.integrations.github_enterprise.integration.get_jwt", return_value="jwt_token_1")
@patch("sentry.integrations.github_enterprise.client.get_jwt", return_value="jwt_token_1")
@responses.activate
Expand Down
Loading
Loading