feat(billing): add billing alerts schema#66353
Open
willwearing wants to merge 4 commits into
Open
Conversation
2 tasks
Contributor
|
Reviews (1): Last reviewed commit: "feat(billing): add billing alerts schema" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+98
to
+100
| @cached_property | ||
| def organization(self) -> Organization: | ||
| return Organization.objects.get(id=self.organization_id) |
Contributor
There was a problem hiding this comment.
@cached_property
Organization.objects.get(...) is a synchronous ORM call. Any async view or async Celery task that accesses alert_config.organization will raise SynchronousOnlyOperation. Given PostHog's growing async surface, this property either needs to be fetched up-front in the calling code or wrapped with sync_to_async at the call site before this model is used in async paths.
| consecutive_failures = models.PositiveIntegerField(default=0) | ||
|
|
||
| created_at = models.DateTimeField(auto_now_add=True) | ||
| updated_at = models.DateTimeField(auto_now=True, null=True, blank=True) |
Contributor
There was a problem hiding this comment.
auto_now=True always sets this field on save(), so the database column is never NULL in practice. The null=True annotation is misleading — it suggests the column might be NULL when the ORM never writes NULL for it. Removing it makes the intent clearer.
Suggested change
| updated_at = models.DateTimeField(auto_now=True, null=True, blank=True) | |
| updated_at = models.DateTimeField(auto_now=True) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+102
to
+121
| def clean(self) -> None: | ||
| super().clean() | ||
|
|
||
| if self.baseline_window_days < 1: | ||
| raise ValidationError({"baseline_window_days": "Must be at least 1."}) | ||
| if self.check_interval_hours < 1: | ||
| raise ValidationError({"check_interval_hours": "Must be at least 1."}) | ||
| if self.minimum_value < 0: | ||
| raise ValidationError({"minimum_value": "Must be greater than or equal to 0."}) | ||
|
|
||
| if self.threshold_type == self.ThresholdType.RELATIVE_INCREASE: | ||
| if self.threshold_percentage is None: | ||
| raise ValidationError({"threshold_percentage": "Required for relative increase alerts."}) | ||
| if self.threshold_percentage <= 0: | ||
| raise ValidationError({"threshold_percentage": "Must be greater than 0."}) | ||
| elif self.threshold_type in (self.ThresholdType.ABSOLUTE_VALUE, self.ThresholdType.ABSOLUTE_INCREASE): | ||
| if self.threshold_value is None: | ||
| raise ValidationError({"threshold_value": "Required for absolute threshold alerts."}) | ||
| if self.threshold_value < 0: | ||
| raise ValidationError({"threshold_value": "Must be greater than or equal to 0."}) |
Contributor
There was a problem hiding this comment.
clean() doesn't cross-invalidate the non-applicable threshold field
When threshold_type changes (e.g. from ABSOLUTE_VALUE to RELATIVE_INCREASE), threshold_value is not required to be None. A stale non-NULL threshold_value will sit alongside threshold_percentage in the DB. Any service code that picks its threshold from the model without re-checking threshold_type first could silently use the wrong value.
Contributor
|
Size Change: 0 B Total Size: 64.2 MB ℹ️ View Unchanged
|
Contributor
Migration SQL ChangesHey 👋, we've detected some migrations on this PR. Here's the SQL output for each migration, make sure they make sense:
|
Contributor
🔍 Migration Risk AnalysisWe've analyzed your migrations for potential risks. Summary: 1 Safe | 0 Needs Review | 0 Blocked ✅ SafeBrief or no lock, backwards compatible Last updated: 2026-06-26 15:08 UTC (e237688) |
Contributor
|
Reviews (2): Last reviewed commit: "Merge branch 'master' into will/billing-..." | Re-trigger Greptile |
Contributor
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Billing alerts need durable tables before the service and UI code can safely deploy. The schema needs to land separately because migration jobs are not orchestrated with service deployments.
Changes
Adds the Billing alerts Django app shell, model definitions, and initial migration for alert configurations and alert events.
The migration creates new tables only. Foreign keys to
posthog.Teamusedb_constraint=Falseto avoid locking the hot parent table during deployment.Feature branch to be merged in following this: #66355
How did you test this code?
As an agent, I ran:
sqlmigrateconfirmed the migration emits no FK constraint toposthog_team.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Automatic notifications
Docs update
No docs update in this migration-only PR.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Codex split this out from the billing alerts feature PR so the schema can deploy before service code. Skills invoked: GitHub publish workflow, GitHub Actions CI fix workflow, and /django-migrations. The key safety choice is keeping
db_constraint=Falseon the hot-table Team references while preserving the event-to-alert FK inside the new tables.