[WIKI-659] chore: added issue relation and page sort order#7784
[WIKI-659] chore: added issue relation and page sort order#7784sriramveeraghanta merged 7 commits intopreviewfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new "implemented_by" issue relation type and performs a data migration to set sort_order values for pages. The changes include updating the issue relation mapping utilities and removing the choices constraint from the relation_type field to allow for future extensibility.
- Added "implemented_by" and "implements" relation types to issue relation system
- Updated relation mapping functions to handle the new bidirectional relationship
- Performed data migration to assign sequential sort_order values to pages ordered by name
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/api/plane/utils/issue_relation_mapper.py | Added mapping for new "implemented_by"/"implements" relation types and fixed grammar in comment |
| apps/api/plane/db/models/issue.py | Added IMPLEMENTED_BY choice and removed choices constraint from relation_type field |
| apps/api/plane/db/migrations/0106_auto_20250912_0845.py | Migration to remove choices constraint and bulk update page sort_order values |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
NarayanBavisetti
changed the title
|
Pull Request Linked with Plane Work Items Comment Automatically Generated by Plane |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a Django migration creating ProjectWebhook with soft-delete-aware uniqueness, updates IssueRelation defaults and choices handling, introduces Page.DEFAULT_SORT_ORDER and a data migration to set Page.sort_order in batches, and extends issue relation mapping utilities for implemented_by/implements inverses. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Admin as Migration Runner
participant ORM as Django ORM
participant DB as Database
rect rgba(230,245,255,0.6)
note over Admin,DB: Schema changes
Admin->>DB: Create table project_webhooks
Admin->>DB: Add constraints (unique_together, partial unique)
Admin->>DB: Alter field Issuerelation.relation_type (default/max_length)
end
rect rgba(235,255,235,0.6)
note over Admin,DB: Data migration - Page.sort_order
Admin->>ORM: apps.get_model("db","Page")
ORM->>DB: SELECT id FROM pages ORDER BY name
DB-->>ORM: Page IDs
loop Batch of 3000
Admin->>DB: bulk_update sort_order = 100,200,...
end
note over Admin: Reverse sets sort_order to DEFAULT_SORT_ORDER
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…eplane/plane into chore-page-sort-order-data-migration
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/api/plane/db/models/issue.py (1)
297-301: Removing choices on IssueRelation.relation_type weakens data integrity — restore validation or add a DB check.Without choices/checks, arbitrary strings can be stored. Either:
- Re-introduce choices, and normalize inputs via the mapper; or
- Keep free-form but add a CheckConstraint enumerating allowed values.
Apply this diff to restore choices:
- relation_type = models.CharField( - max_length=20, - verbose_name="Issue Relation Type", - default=IssueRelationChoices.BLOCKED_BY, - ) + relation_type = models.CharField( + max_length=20, + choices=IssueRelationChoices.choices, + verbose_name="Issue Relation Type", + default=IssueRelationChoices.BLOCKED_BY, + )Optionally, also add a DB-level check (requires a migration):
# within IssueRelation.Meta.constraints models.CheckConstraint( name="issuerelation_relation_type_valid", check=models.Q( relation_type__in=[ "duplicate", "relates_to", "blocked_by", "start_before", "finish_before", "implemented_by", ] ), ),apps/api/plane/db/migrations/0106_auto_20250912_0845.py (1)
15-16: Replace magic numbers with named constants.Batch size 3000 and start 100 should be constants with a brief comment.
🧹 Nitpick comments (1)
apps/api/plane/db/migrations/0106_auto_20250912_0845.py (1)
21-22: Avoid loading all IDs into memory.Use QuerySet.iterator(chunk_size=…) to stream IDs.
Example (to be used in the top-level function):
page_ids = Page.objects.order_by("name").values_list("id", flat=True).iterator(chunk_size=5000)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/api/plane/db/migrations/0106_auto_20250912_0845.py(1 hunks)apps/api/plane/db/models/issue.py(1 hunks)apps/api/plane/db/models/webhook.py(2 hunks)apps/api/plane/utils/issue_relation_mapper.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/api/plane/db/models/webhook.py (1)
apps/api/plane/db/models/project.py (1)
ProjectBaseModel(178-191)
apps/api/plane/db/migrations/0106_auto_20250912_0845.py (1)
apps/api/plane/db/models/page.py (1)
Page(19-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
apps/api/plane/db/models/issue.py (1)
287-287: Enum addition for implemented_by looks good.Consistent with mapper changes; no concerns.
apps/api/plane/utils/issue_relation_mapper.py (2)
9-11: Inverse mappings for implemented_by/implements — LGTM.Symmetry is correct.
16-26: Normalization confirmed — no action required.
- apps/api/plane/app/views/issue/relation.py calls get_actual_relation(relation_type) before constructing/persisting IssueRelation (bulk_create at around line 236).
- The only other IssueRelation creations are in migrations (apps/api/plane/db/migrations/0043_… — uses literal relation_type in a migration), which is expected.
apps/api/plane/db/models/webhook.py (1)
96-113: Add CreateModel migration for ProjectWebhookNo CreateModel migration for project_webhooks found in apps/api/plane/db/migrations (ripgrep returned no matches). Deploys will fail when the ORM references this model — run python manage.py makemigrations and include the generated migration in this PR.
apps/api/plane/db/migrations/0106_auto_20250912_0845.py (1)
43-52: Make this migration reversible; move RunPython callables to module scope.
- Define module-level forward and reverse callables (e.g. forward_set_page_sort_order, reverse_set_page_sort_order), persist a pre-change backup of pages.sort_order, and update the migration to use migrations.RunPython(forward_set_page_sort_order, reverse_set_page_sort_order) so the change can be unapplied (or explicitly document why a noop reverse is acceptable).
- Update the PR description: the assignment is deterministic (ordered by Page.name), not “randomly assigning”.
File: apps/api/plane/db/migrations/0106_auto_20250912_0845.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/api/plane/db/models/issue.py (1)
297-301: Dropping choices removes validation; add back validation or enforce at DB.Removing choices allows arbitrary strings in relation_type, risking bad data. Either restore choices on the model or add a DB CheckConstraint to restrict values.
Apply one of the following:
- relation_type = models.CharField( - max_length=20, - verbose_name="Issue Relation Type", - default=IssueRelationChoices.BLOCKED_BY, - ) + relation_type = models.CharField( + max_length=20, + choices=IssueRelationChoices.choices, + verbose_name="Issue Relation Type", + default=IssueRelationChoices.BLOCKED_BY, + )Or, enforce at DB (follow‑up migration):
migrations.AddConstraint( model_name="issuerelation", constraint=models.CheckConstraint( name="issuerelation_relation_type_valid", check=models.Q( relation_type__in=[ "duplicate", "relates_to", "blocked_by", "start_before", "finish_before", "implemented_by", ] ), ), )apps/api/plane/db/migrations/0106_auto_20250912_0845.py (1)
142-148: Reinstate server‑side validation for relation_type.Altering the field without choices removes guardrails. Add a CheckConstraint here (or in a follow‑up migration) to restrict allowed values.
Apply (in this migration or next):
migrations.AlterField( model_name="issuerelation", name="relation_type", field=models.CharField( default="blocked_by", max_length=20, verbose_name="Issue Relation Type" ), ), + migrations.AddConstraint( + model_name="issuerelation", + constraint=models.CheckConstraint( + name="issuerelation_relation_type_valid", + check=models.Q( + relation_type__in=[ + "duplicate", + "relates_to", + "blocked_by", + "start_before", + "finish_before", + "implemented_by", + ] + ), + ), + ),
🧹 Nitpick comments (4)
apps/api/plane/db/models/page.py (1)
61-61: Optional: align sort_order constants project‑wide.Issue.sort_order and IssueVersion.sort_order still hard‑code 65535. Consider a shared constant (or reuse Page.DEFAULT_SORT_ORDER via a shared constants module) to keep defaults consistent.
apps/api/plane/utils/issue_relation_mapper.py (1)
15-26: Optional: add unit tests for new mappings.Add tests asserting:
- get_inverse_relation("implements") == "implemented_by"
- get_actual_relation("implements") == "implemented_by"
apps/api/plane/db/models/webhook.py (1)
96-113: Optional: add single‑column indexes if you query by one side frequently.If common queries filter by project or webhook alone (and include deleted rows), consider adding Index(fields=["project"]) and Index(fields=["webhook"]) for selectivity beyond the partial unique index.
apps/api/plane/db/migrations/0106_auto_20250912_0845.py (1)
11-23: Make data migration streaming and self‑documented.Avoid loading all page IDs into memory; stream with iterator() and name the magic numbers.
Apply:
- batch_size = 3000 - sort_order = 100 + BATCH_SIZE = 3000 + START = 100 + STEP = 100 + sort_order = START @@ - page_ids = list(Page.objects.all().order_by("name").values_list("id", flat=True)) + page_ids = ( + Page.objects.all() + .order_by("name") + .values_list("id", flat=True) + .iterator(chunk_size=10000) + ) @@ - if len(updated_pages) >= batch_size: + if len(updated_pages) >= BATCH_SIZE: Page.objects.bulk_update( - updated_pages, ["sort_order"], batch_size=batch_size + updated_pages, ["sort_order"], batch_size=BATCH_SIZE ) updated_pages = [] @@ - if updated_pages: - Page.objects.bulk_update(updated_pages, ["sort_order"], batch_size=batch_size) + if updated_pages: + Page.objects.bulk_update(updated_pages, ["sort_order"], batch_size=BATCH_SIZE)Also applies to: 24-34
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/plane/db/migrations/0106_auto_20250912_0845.py(1 hunks)apps/api/plane/db/models/issue.py(1 hunks)apps/api/plane/db/models/page.py(2 hunks)apps/api/plane/db/models/webhook.py(2 hunks)apps/api/plane/utils/issue_relation_mapper.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/api/plane/db/migrations/0106_auto_20250912_0845.py (1)
apps/api/plane/db/models/page.py (1)
Page(19-83)
apps/api/plane/db/models/webhook.py (1)
apps/api/plane/db/models/project.py (1)
ProjectBaseModel(178-191)
🔇 Additional comments (5)
apps/api/plane/db/models/page.py (1)
22-22: Good: centralized default.Introducing DEFAULT_SORT_ORDER improves readability and avoids magic numbers.
apps/api/plane/db/models/issue.py (1)
287-287: LGTM: new relation type added.IMPLEMENTED_BY fits the new mapping and UI wording.
apps/api/plane/utils/issue_relation_mapper.py (1)
9-11: LGTM: bidirectional mapping for implements/implemented_by.Inverse and storage mapping look correct and consistent with the enum.
Also applies to: 24-26
apps/api/plane/db/models/webhook.py (1)
96-113: LGTM: ProjectWebhook join model with partial unique constraint.Design aligns with ProjectBaseModel; constraint prevents dupes when not soft‑deleted.
apps/api/plane/db/migrations/0106_auto_20250912_0845.py (1)
8-34: Clarify intent: code sorts deterministically; PR description says “randomly assigning numbers.”The migration assigns ascending values by name (100, 200, ...), not random. Please update the PR description or the migration logic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/api/plane/db/models/issue.py (1)
297-301: Dropping choices invites invalid data; enforce allowed values.Without choices, any string persists. Either restore model-level choices or add a DB CheckConstraint (preferred if you want to accept aliases in the API but persist canonical values).
- relation_type = models.CharField( - max_length=20, - verbose_name="Issue Relation Type", - default=IssueRelationChoices.BLOCKED_BY, - ) + relation_type = models.CharField( + max_length=20, + verbose_name="Issue Relation Type", + default=IssueRelationChoices.BLOCKED_BY, + choices=IssueRelationChoices.choices, + )If you want to keep the field unconstrained at the model layer, add a CheckConstraint in migration (see my migration comment with a ready-to-drop-in op).
apps/api/plane/db/migrations/0106_auto_20250912_0845.py (1)
142-151: Add DB-level guard for IssueRelation.relation_type if model-level choices stay removed.This preserves data integrity even if callers send arbitrary strings.
migrations.AlterField( model_name="issuerelation", name="relation_type", field=models.CharField( default="blocked_by", max_length=20, verbose_name="Issue Relation Type" ), ), + migrations.AddConstraint( + model_name="issuerelation", + constraint=models.CheckConstraint( + name="issuerelation_relation_type_valid", + check=models.Q( + relation_type__in=[ + "duplicate", + "relates_to", + "blocked_by", + "start_before", + "finish_before", + "implemented_by", + ] + ), + ), + ),
🧹 Nitpick comments (2)
apps/api/plane/db/models/page.py (1)
61-61: Consider indexing and numeric type choice for sort_order.If sort_order is used in frequent ORDER BY/filters, add db_index=True and consider DecimalField for stability if you plan fractional inserts. Optional if performance is fine.
- sort_order = models.FloatField(default=DEFAULT_SORT_ORDER) + sort_order = models.FloatField(default=DEFAULT_SORT_ORDER, db_index=True)apps/api/plane/utils/issue_relation_mapper.py (1)
9-11: Inverse and canonical mapping for implemented_by/implements — good; keep constants in sync.Nice addition. To prevent drift, consider centralizing allowed canonical values (e.g., reuse IssueRelationChoices or a shared constants module if import cycles prevent importing models here).
Also applies to: 24-26
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/plane/db/migrations/0106_auto_20250912_0845.py(1 hunks)apps/api/plane/db/models/issue.py(1 hunks)apps/api/plane/db/models/page.py(2 hunks)apps/api/plane/db/models/webhook.py(2 hunks)apps/api/plane/utils/issue_relation_mapper.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/api/plane/db/migrations/0106_auto_20250912_0845.py (1)
apps/api/plane/db/models/page.py (1)
Page(19-83)
apps/api/plane/db/models/webhook.py (1)
apps/api/plane/db/models/project.py (1)
ProjectBaseModel(178-191)
🔇 Additional comments (4)
apps/api/plane/db/models/page.py (1)
22-22: Constant LGTM; avoid coupling migrations to this symbol.Defining DEFAULT_SORT_ORDER here is good. Ensure migrations don’t reference this class attribute directly (historical models won’t have it). Use the field’s historical default inside the migration instead. See my migration comment with a diff.
apps/api/plane/db/models/issue.py (1)
287-288: New relation choice “implemented_by” — looks good.apps/api/plane/db/models/webhook.py (1)
96-113: ```shell
#!/bin/bash
set -euo pipefailecho "PWD: $(pwd)"
echo
echo "=== apps/api/plane/db/models/webhook.py ==="
if [ -f "apps/api/plane/db/models/webhook.py" ]; then
sed -n '1,400p' apps/api/plane/db/models/webhook.py || true
else
echo "NOT FOUND: apps/api/plane/db/models/webhook.py"
fiecho
echo "=== locate ProjectBaseModel ==="
rg -n --hidden --no-ignore 'class ProjectBaseModel\b' -S || true
rg -n --hidden --no-ignore 'class ProjectBaseModel\b' -S -l || truefor f in $(rg -n --hidden --no-ignore 'class ProjectBaseModel\b' -S -l || true); do
echo
echo "---- $f ----"
sed -n '1,400p' "$f" || true
doneecho
echo "=== locate Webhook model ==="
rg -n --hidden --no-ignore 'class Webhook\b' -S || true
rg -n --hidden --no-ignore 'class Webhook\b' -S -l || truefor f in $(rg -n --hidden --no-ignore 'class Webhook\b' -S -l || true); do
echo
echo "---- $f ----"
sed -n '1,400p' "$f" || true
doneecho
echo "=== locate Project model ==="
rg -n --hidden --no-ignore 'class Project\b' -S || true
rg -n --hidden --no-ignore 'class Project\b' -S -l || truefor f in $(rg -n --hidden --no-ignore 'class Project\b' -S -l || true); do
echo
echo "---- $f ----"
sed -n '1,400p' "$f" || true
doneecho
echo "=== search for workspace handling in models ==="
rg -n --hidden --no-ignore '\bworkspace\b' apps -S -C2 || trueecho
echo "=== search for models implementing clean/save that mention workspace/project ==="
rg -n --hidden --no-ignore "def\s+(clean|save)\s*\(" apps/api/plane/db/models -S -C3 || trueecho
echo "=== done ==="</blockquote></details> <details> <summary>apps/api/plane/db/migrations/0106_auto_20250912_0845.py (1)</summary><blockquote> `14-23`: **Behavior mismatch with PR description — this is deterministic, not random.** The migration assigns ascending values by name; it’s not random. Either update the PR description or implement randomness (not recommended for reproducibility). </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
…#7784) * chore: added issue relation and page sort order * feat: add ProjectWebhook model to manage webhooks associated with projects * chore: updated the migration file * chore: added migration * chore: reverted the page base code * chore: added a variable for sort order in pages --------- Co-authored-by: pablohashescobar <nikhilschacko@gmail.com>
Description
this pull request does a data migration in the sort_order field in which we randomly assign the number in the sort_order. Also added a new choice in the issue relation.
Type of Change
Summary by CodeRabbit
New Features
Bug Fixes
Chores