ref(models): Update CodeReviewEvent per reviewer feedback#109401
Closed
ref(models): Update CodeReviewEvent per reviewer feedback#109401
Conversation
Address post-merge feedback on CodeReviewEvent from PR #108531: - Use DefaultFieldsModel base class for standard date_added/date_updated fields (wedamija, Dan Fuller) - Add FlexibleForeignKey for organization and repository instead of plain integer fields, enabling cascade deletion (wedamija, Dan Fuller, Armen) - Change __relocation_scope__ from Excluded to Organization so data is preserved during org relocation (markstory) - Add backup test coverage in create_exhaustive_organization() The migration uses SeparateDatabaseAndState because the underlying column names (organization_id, repository_id) don't change — only Django's field representation and the addition of FK constraints. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
This PR has a migration; here is the generated SQL for for --
-- Custom state/database change combination
--
ALTER TABLE "sentry_code_review_event" ADD CONSTRAINT "sentry_code_review_ev_organization_id_fk" FOREIGN KEY ("organization_id") REFERENCES "sentry_organization" ("id") DEFERRABLE INITIALLY DEFERRED;
ALTER TABLE "sentry_code_review_event" ADD CONSTRAINT "sentry_code_review_ev_repository_id_fk" FOREIGN KEY ("repository_id") REFERENCES "sentry_repository" ("id") DEFERRABLE INITIALLY DEFERRED;
CREATE INDEX "sentry_code_review_ev_repository_id_fk_idx" ON "sentry_code_review_event" ("repository_id");
--
-- Custom state/database change combination
--
ALTER TABLE "sentry_code_review_event" ADD COLUMN "date_updated" timestamp with time zone DEFAULT (STATEMENT_TIMESTAMP()) NOT NULL;
--
-- Alter field date_added on codereviewevent
--
DROP INDEX CONCURRENTLY IF EXISTS "sentry_code_review_event_date_added_cd622aec";
--
-- Create index sentry_code_date_ad_a2451c_idx on field(s) date_added of model codereviewevent
--
CREATE INDEX CONCURRENTLY "sentry_code_date_ad_a2451c_idx" ON "sentry_code_review_event" ("date_added"); |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Address post-merge reviewer feedback on
CodeReviewEventfrom PR #108531 (and related discussion in #108533 / Slack). No data has been written to the table yet, making this a safe schema adjustment.Changes:
DefaultFieldsModelbase class — Provides standarddate_added(auto_now_add) anddate_updated(auto_now) fields, replacing the manualdate_added. Recommended by wedamija and Dan Fuller.FlexibleForeignKeyfor organization and repository — Adds FK constraints for cascade deletion support and referential integrity. The underlying column names (organization_id,repository_id) are unchanged, so all existing code using_idaccessors continues to work. Recommended by wedamija, Dan Fuller, and Armen.RelocationScope.Organization— Ensures data is preserved when orgs move between regions. Confirmed by markstory.CodeReviewEventinstance increate_exhaustive_organization()to satisfy the exhaustive/uniqueness backup test requirements.Important note:
Django can't auto-detect that the raw ID fields are being converted to ForeignKeys (same column names under the hood).
The migration uses
SeparateDatabaseAndStatebecause the column names don't change — only Django's field representation and the addition of FK constraints. Composite index names are identical since they're derived from column names (not field names).