Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion src/sentry/api/endpoints/project_rule_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,11 @@ def put(self, request: Request, project, rule) -> Response:
if data.get("pending_save"):
client = RedisRuleStatus()
kwargs.update({"uuid": client.uuid, "rule_id": rule.id})
find_channel_id_for_rule.apply_async(kwargs=kwargs)
# Serialize Actor object to string identifier for task queueing
task_kwargs = kwargs.copy()
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to copy the kwargs if this is the last time they could possibly be modified in this function? We'd overwrite the owner from earlier with json serializable version

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need to, but it's effectively free and keeps this mutation just for the task, new uses of this dict can be added later without needing to know the task requirements.

if owner:
task_kwargs["owner"] = owner.identifier
find_channel_id_for_rule.apply_async(kwargs=task_kwargs)

context = {"uuid": client.uuid}
return Response(context, status=202)
Expand Down
6 changes: 5 additions & 1 deletion src/sentry/api/endpoints/project_rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -833,7 +833,11 @@ def post(self, request: Request, project) -> Response:
client = RedisRuleStatus()
uuid_context = {"uuid": client.uuid}
kwargs.update(uuid_context)
find_channel_id_for_rule.apply_async(kwargs=kwargs)
# Serialize Actor object to string identifier for task queueing
task_kwargs = kwargs.copy()
if owner:
task_kwargs["owner"] = owner.identifier
find_channel_id_for_rule.apply_async(kwargs=task_kwargs)
return Response(uuid_context, status=202)

try:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from sentry.silo.base import SiloMode
from sentry.tasks.base import instrumented_task
from sentry.taskworker.namespaces import integrations_tasks
from sentry.types.actor import Actor

logger = logging.getLogger("sentry.integrations.slack.tasks")

Expand Down Expand Up @@ -109,6 +110,11 @@ def find_channel_id_for_rule(
kwargs["actions"] = actions
kwargs["project"] = project

# Deserialize owner string identifier back to Actor object
owner = kwargs.get("owner")
if owner and isinstance(owner, str):
kwargs["owner"] = Actor.from_identifier(owner)

if rule_id:
rule = Rule.objects.get(id=rule_id)
rule = ProjectRuleUpdater(
Expand Down
3 changes: 1 addition & 2 deletions tests/sentry/api/endpoints/test_project_rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
from sentry.testutils.cases import APITestCase
from sentry.testutils.helpers import install_slack, with_feature
from sentry.testutils.silo import assume_test_silo_mode
from sentry.types.actor import Actor
from sentry.users.models.user import User


Expand Down Expand Up @@ -751,7 +750,7 @@ def test_kicks_off_slack_async_job(
"actions": payload.get("actions", []),
"frequency": payload.get("frequency"),
"user_id": self.user.id,
"owner": Actor.from_id(user_id=self.user.id),
"owner": f"user:{self.user.id}",
"uuid": "abc123",
}
call_args = mock_find_channel_id_for_alert_rule.call_args[1]["kwargs"]
Expand Down
81 changes: 81 additions & 0 deletions tests/sentry/integrations/slack/tasks/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,45 @@ def test_task_new_rule_project_id(self, mock_set_value: MagicMock) -> None:
]
assert rule.created_by_id == self.user.id

@responses.activate
@patch.object(RedisRuleStatus, "set_value", return_value=None)
def test_task_new_rule_with_owner(self, mock_set_value: MagicMock) -> None:
"""Test that owner identifier string is deserialized to Actor correctly."""
team = self.create_team(organization=self.organization)
owner_identifier = f"team:{team.id}"

data = {
"name": "New Rule with Owner",
"environment": None,
"project_id": self.project.id,
"action_match": "all",
"filter_match": "all",
"conditions": [
{"id": "sentry.rules.conditions.first_seen_event.FirstSeenEventCondition"}
],
"actions": [
{
"channel": "#my-channel",
"id": "sentry.integrations.slack.notify_action.SlackNotifyServiceAction",
"tags": "",
"workspace": self.integration.id,
}
],
"frequency": 5,
"uuid": self.uuid,
"user_id": self.user.id,
"owner": owner_identifier,
}

with self.tasks():
find_channel_id_for_rule(**data)

rule = Rule.objects.exclude(label__in=[DEFAULT_RULE_LABEL]).get(project_id=self.project.id)
mock_set_value.assert_called_with("success", rule.id)
assert rule.label == "New Rule with Owner"
assert rule.owner_team_id == team.id
assert rule.owner_user_id is None

@responses.activate
@patch.object(RedisRuleStatus, "set_value", return_value=None)
def test_task_existing_rule(self, mock_set_value: MagicMock) -> None:
Expand Down Expand Up @@ -205,6 +244,48 @@ def test_task_existing_rule(self, mock_set_value: MagicMock) -> None:
}
]

@responses.activate
@patch.object(RedisRuleStatus, "set_value", return_value=None)
def test_task_existing_rule_with_owner(self, mock_set_value: MagicMock) -> None:
"""Test that owner identifier string is deserialized to Actor correctly during update."""
action_data = {"id": "sentry.rules.actions.notify_event.NotifyEventAction"}
condition_data = {"id": "sentry.rules.conditions.every_event.EveryEventCondition"}
rule = Rule.objects.create(
project=self.project, data={"actions": [action_data], "conditions": [condition_data]}
)

owner_identifier = f"user:{self.user.id}"

data = {
"name": "Updated Rule with Owner",
"environment": None,
"project_id": self.project.id,
"action_match": "all",
"filter_match": "all",
"conditions": [condition_data],
"actions": [
{
"channel": "#my-channel",
"id": "sentry.integrations.slack.notify_action.SlackNotifyServiceAction",
"tags": "",
"workspace": self.integration.id,
}
],
"frequency": 5,
"uuid": self.uuid,
"rule_id": rule.id,
"owner": owner_identifier,
}

with self.tasks():
find_channel_id_for_rule(**data)

updated_rule = Rule.objects.get(id=rule.id)
mock_set_value.assert_called_with("success", rule.id)
assert updated_rule.label == "Updated Rule with Owner"
assert updated_rule.owner_user_id == self.user.id
assert updated_rule.owner_team_id is None

@responses.activate
@patch.object(RedisRuleStatus, "set_value", return_value=None)
@patch(
Expand Down
Loading