-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(crons): allow team and user names to be used in checkin payload #95778
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.
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
fix(crons): allow team and user names to be used in checkin payload #95778
Conversation
f23f84a
to
9c50d52
Compare
9c50d52
to
0ee09b9
Compare
0ee09b9
to
d95558b
Compare
d95558b
to
3c913e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Test Fails to Refresh Database State Before Assertion
The test_user_name_as_owner
test is missing a monitor.refresh_from_db()
call before asserting the monitor's owner fields. This causes the test to check the stale in-memory object instead of the actual database state after check-in processing, unlike the test_team_name_as_owner
test and other similar tests which correctly refresh. Consequently, the test could pass even if the monitor's ownership was not correctly updated in the database.
tests/sentry/monitors/consumers/test_monitor_consumer.py#L960-L985
sentry/tests/sentry/monitors/consumers/test_monitor_consumer.py
Lines 960 to 985 in 3c913e1
def test_user_name_as_owner(self): | |
named_user = self.create_user( | |
"admin2@localhost", | |
username="test_user", | |
is_superuser=True, | |
is_staff=True, | |
is_sentry_app=False, | |
) | |
monitor = self._create_monitor(slug="my-monitor", owner_user_id=named_user.id) | |
self.send_checkin( | |
"my-monitor", | |
monitor_config={ | |
"schedule": {"type": "crontab", "value": "13 * * * *"}, | |
"owner": f"user:{named_user.username}", | |
}, | |
) | |
checkin = MonitorCheckIn.objects.get(guid=self.guid) | |
assert checkin.status == CheckInStatus.OK | |
monitor_environment = MonitorEnvironment.objects.get(id=checkin.monitor_environment.id) | |
assert monitor_environment.status == MonitorStatus.OK | |
assert monitor.owner_user_id == named_user.id | |
assert monitor.owner_team_id is None | |
Bug: User Parsing Fails with Team Prefix
The Actor.from_identifier
method incorrectly parses "user:" prefixed identifiers when the remainder starts with "team:". After stripping the "user:" prefix, the id
variable is reassigned to the remainder (e.g., "team:myteam"). The logic then incorrectly proceeds to check for a "team:" prefix, causing the input to be parsed as a team name ("myteam") instead of a user's username. This results in an incorrect team lookup instead of the intended user lookup.
src/sentry/types/actor.py#L202-L222
sentry/src/sentry/types/actor.py
Lines 202 to 222 in 3c913e1
if id.startswith("user:"): | |
remainder = id[5:] | |
if remainder.isdigit(): | |
return cls(id=int(remainder), actor_type=ActorType.USER) | |
# pass this on to get to the user lookup below | |
id = remainder | |
if id.startswith("team:"): | |
remainder = id[5:] | |
if remainder.isdigit(): | |
return cls(id=int(remainder), actor_type=ActorType.TEAM) | |
if organization_id is not None: | |
try: | |
team = Team.objects.get(name=remainder, organization_id=organization_id) | |
return cls(id=team.id, actor_type=ActorType.TEAM) | |
except Team.DoesNotExist: | |
pass | |
raise cls.InvalidActor(f"Unable to resolve team name: {remainder}") |
Was this report helpful? Give feedback by reacting with 👍 or 👎
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #95778 +/- ##
==========================================
+ Coverage 74.83% 83.94% +9.10%
==========================================
Files 10516 10572 +56
Lines 607243 610241 +2998
Branches 23913 23913
==========================================
+ Hits 454435 512242 +57807
+ Misses 152421 97612 -54809
Partials 387 387 |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
No description provided.