feat(ACI): Add IncidentGroupOpenPeriod lookup table #88856
Conversation
github-actions
bot
added
the
Scope: Backend
|
This PR has a migration; here is the generated SQL for --
-- Create model IncidentGroupOpenPeriod
--
CREATE TABLE "workflow_engine_incidentgroupopenperiod" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "date_updated" timestamp with time zone NOT NULL, "date_added" timestamp with time zone NOT NULL, "incident_id" bigint NULL UNIQUE, "group_open_period_id" bigint NOT NULL UNIQUE);
ALTER TABLE "workflow_engine_incidentgroupopenperiod" ADD CONSTRAINT "workflow_engine_inci_group_open_period_id_b5c3857c_fk_sentry_gr" FOREIGN KEY ("group_open_period_id") REFERENCES "sentry_groupopenperiod" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "workflow_engine_incidentgroupopenperiod" VALIDATE CONSTRAINT "workflow_engine_inci_group_open_period_id_b5c3857c_fk_sentry_gr"; |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #88856 +/- ##
===========================================
+ Coverage 32.97% 87.74% +54.76%
===========================================
Files 8512 10080 +1568
Lines 475426 570753 +95327
Branches 22372 22372
===========================================
+ Hits 156780 500808 +344028
+ Misses 318247 69546 -248701
Partials 399 399 |
| UniqueConstraint( | ||
| fields=["incident_id", "group_open_period"], | ||
| name="workflow_engine_uniq_incident_groupopenperiod", | ||
| ) |
There was a problem hiding this comment.
What unique constraint do we want here? Should there only ever be one incident id in the table, and one group open period in the table?
We need either both the uniques on the columns, or this unique, depending on what you need here
There was a problem hiding this comment.
Should there only ever be one incident id in the table, and one group open period in the table?
Yes to this ^ should I remove the UniqueConstraint?
There was a problem hiding this comment.
Yeah, removed the combined constraint, it's not necessary
|
|
||
| __relocation_scope__ = RelocationScope.Excluded | ||
|
|
||
| incident_id = BoundedBigIntegerField(null=True, unique=True) |
There was a problem hiding this comment.
Should this be nullable? I guess it's null for incidents created after we stop writing to Incident?
There was a problem hiding this comment.
yeah nullable just in case we need to fudge the id
Add another lookup table between incidents and groupopenperiods to support the old endpoints without having to rely on the old models. More context in https://www.notion.so/sentry/Old-Endpoints-New-Models-1bd8b10e4b5d803d80a9d9d05b6b88b4?pvs=4#1c38b10e4b5d80ab8bb4cba4b55503c2
Add another lookup table between incidents and groupopenperiods to support the old endpoints without having to rely on the old models. More context in https://www.notion.so/sentry/Old-Endpoints-New-Models-1bd8b10e4b5d803d80a9d9d05b6b88b4?pvs=4#1c38b10e4b5d80ab8bb4cba4b55503c2