ref(hybrid-cloud): use organization_slug in MonitorDetails #42300
Conversation
github-actions
bot
added
the
Scope: Backend
| monitor = Monitor.objects.get(id=monitor.id) | ||
| assert monitor.status == MonitorStatus.PENDING_DELETION | ||
| # ScheduledDeletion only available in control silo | ||
| assert ScheduledDeletion.objects.filter( |
There was a problem hiding this comment.
ScheduledDeletion is a control silo only model so this test cannot be made region stable until a service exists for this
RyanSkonnord
left a comment
RyanSkonnord
left a comment
There was a problem hiding this comment.
Some notes for possible refactoring; otherwise looks good.
| """ | ||
| if organization_slug: | ||
| if project.organization.slug != organization_slug: | ||
| return self.respond_invalid() |
There was a problem hiding this comment.
Does "details": "Invalid monitor" make sense here? Or is it just following a convention from other endpoints?
There was a problem hiding this comment.
this is sort of following convention, but my thinking here was that if the organization slug is incorrect, then it's an invalid monitor for that organization?
| """ | ||
| if organization_slug: | ||
| if project.organization.slug != organization_slug: | ||
| return self.respond_invalid() |
There was a problem hiding this comment.
I'd like us to think about a way to abstract out these three duplicated lines, especially across other, similar endpoints. A utility function for the slug comparison is a good start. It would be even better if we can get rid of the "if...return" by raising an exception to trigger the 400 response. To generalize across other endpoints, a mixin superclass might be the way to go. We can work on this later -- just something to think about for now.
cathteng
deleted the
cathyteng17/hc/convert-orgless-monitor-details-api
branch
cathteng
changed the title
Add
organization_slugto the url for the MonitorDetails endpoint. Keeps the original orgless URL for now.For HC-512