Skip to content

🔧 chore: fix msteams client typing #89449

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

Merged
merged 6 commits into from
Apr 14, 2025
Merged

🔧 chore: fix msteams client typing #89449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3adf0f4
:wrench: chore: fix msteams client typing
iamrajjoshi Apr 11, 2025
433d8e9
:wrench: chore: remove protocol
iamrajjoshi Apr 12, 2025
87ef361
:wrench: chore: remove protocol
iamrajjoshi Apr 12, 2025
0cc02c2
Merge branch 'master' into raj/fix-msteams-client-typing
iamrajjoshi Apr 14, 2025
540f4c7
:bug: fix: tests
iamrajjoshi Apr 14, 2025
cbc93f7
:wrench: chore: fix inheritence order
iamrajjoshi Apr 14, 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
1 change: 0 additions & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@ module = [
"sentry.integrations.github.integration",
"sentry.integrations.gitlab.issues",
"sentry.integrations.jira.integration",
"sentry.integrations.msteams.client",
"sentry.integrations.msteams.notifications",
"sentry.integrations.pagerduty.actions.form",
"sentry.integrations.pipeline",
Expand Down
26 changes: 17 additions & 9 deletions src/sentry/integrations/msteams/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from __future__ import annotations

import time
from abc import ABC
from urllib.parse import urlencode

from requests import PreparedRequest
Expand All @@ -11,14 +12,15 @@
from sentry.integrations.services.integration import integration_service
from sentry.integrations.services.integration.model import RpcIntegration
from sentry.shared_integrations.client.proxy import IntegrationProxyClient, infer_org_integration
from sentry.shared_integrations.exceptions import IntegrationError
from sentry.silo.base import SiloMode, control_silo_function

# five minutes which is industry standard clock skew tolerance
CLOCK_SKEW = 60 * 5


# MsTeamsClientMixin abstract client does not handle setting the base url or auth token
class MsTeamsClientMixin:
# MsTeamsClientABC abstract client does not handle setting the base url or auth token
class MsTeamsClientABC(ApiClient, ABC):
Copy link
Member

Choose a reason for hiding this comment

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

ideally the mixin would be converted to free functions since it's not really sound (and this patch adds a diamond inheritance) -- but this is an improvement at least

integration_name = "msteams"
TEAM_URL = "/v3/teams/%s"
CHANNEL_URL = "/v3/teams/%s/conversations"
Expand All @@ -33,7 +35,7 @@ def get_channel_list(self, team_id: str):

def get_member_list(self, team_id: str, continuation_token: str | None = None):
url = self.MEMBER_URL % team_id
params = {"pageSize": 500}
params: dict[str, int | str] = {"pageSize": 500}
if continuation_token:
params["continuationToken"] = continuation_token
return self.get(url, params=params)
Expand Down Expand Up @@ -70,7 +72,7 @@ def update_card(self, conversation_id: str, activity_id: str, card):

# MsTeamsPreInstallClient is used with the access token and service url as arguments to the constructor
# It will not handle token refreshing
class MsTeamsPreInstallClient(ApiClient, MsTeamsClientMixin):
class MsTeamsPreInstallClient(MsTeamsClientABC):
integration_name = "msteams"

def __init__(self, access_token: str, service_url: str):
Expand All @@ -84,7 +86,7 @@ def request(self, method, path, data=None, params=None):


# MsTeamsClient is used with an existing integration object and handles token refreshing
class MsTeamsClient(IntegrationProxyClient, MsTeamsClientMixin):
class MsTeamsClient(MsTeamsClientABC, IntegrationProxyClient):
integration_name = "msteams"

def __init__(self, integration: Integration | RpcIntegration):
Expand Down Expand Up @@ -117,10 +119,16 @@ def access_token(self):
access_token = token_data["access_token"]
new_metadata.update(token_data)

self.integration = integration_service.update_integration(
integration_id=self.integration.id,
metadata=new_metadata,
)
if (
updated_integration := integration_service.update_integration(
integration_id=self.integration.id,
metadata=new_metadata,
)
) is None:
# This should never happen, but if it does, fail loudly
raise IntegrationError("Integration not found, failed to refresh access token")

self.integration = updated_integration
return access_token

@control_silo_function
Expand Down
4 changes: 2 additions & 2 deletions tests/sentry/integrations/msteams/notifications/test_assigned.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@


@patch(
"sentry.integrations.msteams.MsTeamsClientMixin.get_user_conversation_id",
"sentry.integrations.msteams.MsTeamsClientABC.get_user_conversation_id",
Mock(return_value="some_conversation_id"),
)
@patch("sentry.integrations.msteams.MsTeamsClientMixin.send_card")
@patch("sentry.integrations.msteams.MsTeamsClientABC.send_card")
class MSTeamsAssignedNotificationTest(MSTeamsActivityNotificationTest):
def test_assigned(self, mock_send_card: MagicMock):
"""
Expand Down
4 changes: 2 additions & 2 deletions tests/sentry/integrations/msteams/notifications/test_deploy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@


@patch(
"sentry.integrations.msteams.MsTeamsClientMixin.get_user_conversation_id",
"sentry.integrations.msteams.MsTeamsClientABC.get_user_conversation_id",
Mock(return_value="some_conversation_id"),
)
@patch("sentry.integrations.msteams.MsTeamsClientMixin.send_card")
@patch("sentry.integrations.msteams.MsTeamsClientABC.send_card")
class MSTeamsDeployNotificationTest(MSTeamsActivityNotificationTest):
@skip("Flaky test")
def test_deploy(self, mock_send_card: MagicMock):
Expand Down
4 changes: 2 additions & 2 deletions tests/sentry/integrations/msteams/notifications/test_escalating.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@


@patch(
"sentry.integrations.msteams.MsTeamsClientMixin.get_user_conversation_id",
"sentry.integrations.msteams.MsTeamsClientABC.get_user_conversation_id",
Mock(return_value="some_conversation_id"),
)
@patch("sentry.integrations.msteams.MsTeamsClientMixin.send_card")
@patch("sentry.integrations.msteams.MsTeamsClientABC.send_card")
class MSTeamsEscalatingNotificationTest(MSTeamsActivityNotificationTest):
def test_note(self, mock_send_card: MagicMock):
"""
Expand Down
4 changes: 2 additions & 2 deletions tests/sentry/integrations/msteams/notifications/test_issue_alert.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@


@patch(
"sentry.integrations.msteams.MsTeamsClientMixin.get_user_conversation_id",
"sentry.integrations.msteams.MsTeamsClientABC.get_user_conversation_id",
Mock(return_value="some_conversation_id"),
)
@patch("sentry.integrations.msteams.MsTeamsClientMixin.send_card")
@patch("sentry.integrations.msteams.MsTeamsClientABC.send_card")
class MSTeamsIssueAlertNotificationTest(MSTeamsActivityNotificationTest):
def test_issue_alert_user(self, mock_send_card: MagicMock):
"""Test that issue alerts are sent to a MS Teams user."""
Expand Down
4 changes: 2 additions & 2 deletions tests/sentry/integrations/msteams/notifications/test_note.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@


@patch(
"sentry.integrations.msteams.MsTeamsClientMixin.get_user_conversation_id",
"sentry.integrations.msteams.MsTeamsClientABC.get_user_conversation_id",
Mock(return_value="some_conversation_id"),
)
@patch("sentry.integrations.msteams.MsTeamsClientMixin.send_card")
@patch("sentry.integrations.msteams.MsTeamsClientABC.send_card")
class MSTeamsNoteNotificationTest(MSTeamsActivityNotificationTest):
def test_note(self, mock_send_card: MagicMock):
"""
Expand Down
4 changes: 2 additions & 2 deletions tests/sentry/integrations/msteams/notifications/test_regression.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@


@patch(
"sentry.integrations.msteams.MsTeamsClientMixin.get_user_conversation_id",
"sentry.integrations.msteams.MsTeamsClientABC.get_user_conversation_id",
Mock(return_value="some_conversation_id"),
)
@patch("sentry.integrations.msteams.MsTeamsClientMixin.send_card")
@patch("sentry.integrations.msteams.MsTeamsClientABC.send_card")
class MSTeamsRegressionNotificationTest(MSTeamsActivityNotificationTest):
def test_regression(self, mock_send_card: MagicMock):
"""
Expand Down
4 changes: 2 additions & 2 deletions tests/sentry/integrations/msteams/notifications/test_resolved.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@


@patch(
"sentry.integrations.msteams.MsTeamsClientMixin.get_user_conversation_id",
"sentry.integrations.msteams.MsTeamsClientABC.get_user_conversation_id",
Mock(return_value="some_conversation_id"),
)
@patch("sentry.integrations.msteams.MsTeamsClientMixin.send_card")
@patch("sentry.integrations.msteams.MsTeamsClientABC.send_card")
class MSTeamsResolvedNotificationTest(MSTeamsActivityNotificationTest):
def test_resolved(self, mock_send_card: MagicMock):
"""
Expand Down
4 changes: 2 additions & 2 deletions tests/sentry/integrations/msteams/notifications/test_unassigned.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@


@patch(
"sentry.integrations.msteams.MsTeamsClientMixin.get_user_conversation_id",
"sentry.integrations.msteams.MsTeamsClientABC.get_user_conversation_id",
Mock(return_value="some_conversation_id"),
)
@patch("sentry.integrations.msteams.MsTeamsClientMixin.send_card")
@patch("sentry.integrations.msteams.MsTeamsClientABC.send_card")
class MSTeamsUnassignedNotificationTest(MSTeamsActivityNotificationTest):
def test_unassigned(self, mock_send_card: MagicMock):
"""
Expand Down
10 changes: 10 additions & 0 deletions tests/sentry/integrations/msteams/test_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

from sentry.integrations.models.integration import Integration
from sentry.integrations.msteams.client import MsTeamsClient
from sentry.shared_integrations.exceptions import IntegrationError
from sentry.silo.base import SiloMode
from sentry.silo.util import (
PROXY_BASE_PATH,
Expand Down Expand Up @@ -82,6 +83,15 @@ def test_token_refreshes(self):
"service_url": "https://smba.trafficmanager.net/amer/",
}

@responses.activate
def test_token_refreshes_with_integration_not_found(self):
self.integration.delete()
with patch("time.time") as mock_time:
mock_time.return_value = self.expires_at
# accessing the property should refresh the token
with pytest.raises(IntegrationError):
self.msteams_client.access_token

@responses.activate
def test_no_token_refresh(self):
with patch("time.time") as mock_time:
Expand Down
8 changes: 4 additions & 4 deletions tests/sentry/integrations/msteams/test_notifications.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,14 @@
[DummyNotification],
)
@patch(
"sentry.integrations.msteams.MsTeamsClientMixin.get_user_conversation_id",
"sentry.integrations.msteams.MsTeamsClientABC.get_user_conversation_id",
Mock(return_value="some_conversation_id"),
)
@patch(
"sentry.integrations.msteams.MsTeamsClientMixin.get_member_list",
"sentry.integrations.msteams.MsTeamsClientABC.get_member_list",
Mock(return_value={"members": [{"user": "some_user", "tenantId": "some_tenant_id"}]}),
)
@patch("sentry.integrations.msteams.MsTeamsClientMixin.send_card")
@patch("sentry.integrations.msteams.MsTeamsClientABC.send_card")
class MSTeamsNotificationTest(TestCase):
def _install_msteams_personal(self):
self.tenant_id = "50cccd00-7c9c-4b32-8cda-58a084f9334a"
Expand Down Expand Up @@ -124,7 +124,7 @@ def test_missing_tenant_id(self, mock_send_card: MagicMock):
self._install_msteams_team()

with patch(
"sentry.integrations.msteams.MsTeamsClientMixin.get_user_conversation_id",
"sentry.integrations.msteams.MsTeamsClientABC.get_user_conversation_id",
) as mock_get_user_conversation_id:
mock_get_user_conversation_id.return_value = "some_conversation_id"

Expand Down
Loading