[WEB-5312] migration: reply for work item comments #8072
Conversation
sync: issue_comment and description
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
WalkthroughAdds a Description relation and a self-referential parent field to IssueComment, updates IssueComment.save() to create/update Description records, adds a management command to backfill Description for existing comments, and includes unit tests and a Django migration for the schema changes. Changes
Sequence DiagramsequenceDiagram
participant User
participant IssueComment
participant Description
participant DB
rect rgba(200,220,255,0.25)
note over User,IssueComment: Creation flow
User->>IssueComment: create with comment_html/json/stripped
IssueComment->>IssueComment: save() detects new
IssueComment->>Description: build Description from comment fields
Description->>DB: create Description
DB-->>Description: return id
IssueComment->>IssueComment: set description_id
IssueComment->>DB: save IssueComment
DB-->>User: return created IssueComment
end
rect rgba(220,255,220,0.25)
note over User,IssueComment: Update flow
User->>IssueComment: update comment_html/json/stripped
IssueComment->>IssueComment: save() detects changes
alt Description exists
IssueComment->>Description: update fields
Description->>DB: save
else Description missing (legacy)
IssueComment->>Description: create new Description
Description->>DB: create
DB-->>Description: return id
IssueComment->>IssueComment: set description_id
end
IssueComment->>DB: save IssueComment
DB-->>User: return updated IssueComment
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/plane/db/migrations/0109_issuecomment_description.py(1 hunks)apps/api/plane/db/models/issue.py(3 hunks)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/db/models/issue.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-23T18:18:06.875Z
Learnt from: NarayanBavisetti
Repo: makeplane/plane PR: 7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
Applied to files:
apps/api/plane/db/models/issue.py
🧬 Code graph analysis (1)
apps/api/plane/db/models/issue.py (1)
apps/api/plane/db/models/page.py (2)
save(66-73)save(171-178)
⏰ 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). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/api/plane/db/models/issue.py (2)
20-20: LGTM!The import is correctly placed and follows the existing module import pattern.
450-452: Verify the cascade direction.The
on_delete=CASCADEmeans that deleting aDescriptionwill also delete theIssueComment. Typically, the comment should control the description lifecycle (i.e., useon_delete=CASCADEon the description side), not the other way around. Please confirm this is intentional.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py (1)
20-48: Consider adding progress tracking and error handling.The batch processing logic is sound, but operational visibility could be improved:
- Progress tracking: Show batch number and records processed (e.g., "Processed batch 3/10: 1500/5000 records")
- Error handling: Add try-except around bulk operations with appropriate logging
- Informative messages: Include counts in success messages
These improvements would make the command more robust and easier to monitor during execution.
Example enhancement for progress tracking:
+ processed = 0 for i in range(0, total, batch_size): with transaction.atomic(): comments = list(issue_comments[i : i + batch_size]) + batch_count = len(comments) descriptions = [ Description( created_at=comment.created_at, updated_at=comment.updated_at, description_json=comment.comment_json, description_html=comment.comment_html, description_stripped=comment.comment_stripped, project_id=comment.project_id, created_by_id=comment.created_by_id, updated_by_id=comment.updated_by_id, workspace_id=comment.workspace_id, ) for comment in comments ] created_descriptions = Description.objects.bulk_create(descriptions) comments_to_update = [] for comment, description in zip(comments, created_descriptions): comment.description_id = description.id comments_to_update.append(comment) - IssueComment.objects.bulk_update(comments_to_update, ["description"]) + IssueComment.objects.bulk_update(comments_to_update, ["description_id"]) + processed += batch_count - self.stdout.write(self.style.SUCCESS("Successfully Copied IssueComment to Description")) + self.stdout.write( + self.style.SUCCESS( + f"Processed {processed}/{total} comments ({(processed/total)*100:.1f}%)" + ) + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py (1)
apps/api/plane/db/models/issue.py (1)
IssueComment(446-509)
⏰ 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). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py (3)
1-7: LGTM!All necessary imports are present and correctly structured.
10-11: LGTM!Class definition and help text are appropriate.
13-18: LGTM!Query filters correctly target non-deleted comments without descriptions, and the batch size of 500 is reasonable for this migration.
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/api/plane/db/models/issue.py (1)
488-491: Assign the newdescription_idon the instance.Because
description_idis cached before the create call, this branch leavesself.description_idasNone. The very next save on the same instance re-enters this block, creating additionalDescriptionrows, and any in-memory access toself.descriptionright aftersave()fails. Please set the field on the instance after updating the DB.if description_id: Description.objects.filter(pk=self.description_id).update(**description_defaults) else: description = Description.objects.create(**description_defaults) - IssueComment.objects.filter(pk=self.pk).update(description_id=description.id) + IssueComment.objects.filter(pk=self.pk).update(description_id=description.id) + self.description_id = description.id
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py(1 hunks)apps/api/plane/db/models/issue.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-23T18:18:06.875Z
Learnt from: NarayanBavisetti
Repo: makeplane/plane PR: 7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
Applied to files:
apps/api/plane/db/models/issue.py
🧬 Code graph analysis (1)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py (1)
apps/api/plane/db/models/issue.py (1)
IssueComment(446-501)
⏰ 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). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py
Outdated
Show resolved
Hide resolved
sangeethailango
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py (2)
16-21: Consider adding explicit ordering for deterministic batch processing.While the
IssueCommentmodel has a default ordering of("-created_at",), explicitly specifying.order_by("id")or.order_by("created_at")in batch processing ensures deterministic results across different database configurations and makes the code's intent clearer.Apply this diff:
while True: comments = list( - IssueComment.objects.filter(deleted_at__isnull=True, description_id__isnull=True)[:batch_size] + IssueComment.objects.filter( + deleted_at__isnull=True, description_id__isnull=True + ).order_by("id")[:batch_size] ) if not comments: break
13-48: Add progress tracking and final summary for better observability.The current implementation only logs success after each batch without indicating progress. Consider adding batch counts and a final summary message to help monitor the migration's progress and completion.
Apply this diff:
def handle(self, *args, **kwargs): batch_size = 500 + total_processed = 0 while True: comments = list( IssueComment.objects.filter(deleted_at__isnull=True, description_id__isnull=True)[:batch_size] ) if not comments: break with transaction.atomic(): descriptions = [ Description( created_at=comment.created_at, updated_at=comment.updated_at, description_json=comment.comment_json, description_html=comment.comment_html, description_stripped=comment.comment_stripped, project_id=comment.project_id, created_by_id=comment.created_by_id, updated_by_id=comment.updated_by_id, workspace_id=comment.workspace_id, ) for comment in comments ] created_descriptions = Description.objects.bulk_create(descriptions) comments_to_update = [] for comment, description in zip(comments, created_descriptions): comment.description_id = description.id comments_to_update.append(comment) IssueComment.objects.bulk_update(comments_to_update, ["description_id"]) - self.stdout.write(self.style.SUCCESS("Successfully Copied IssueComment to Description")) + total_processed += len(comments) + self.stdout.write( + self.style.SUCCESS( + f"Processed batch of {len(comments)} comments (total: {total_processed})" + ) + ) + + self.stdout.write( + self.style.SUCCESS( + f"Migration complete. Total IssueComments processed: {total_processed}" + ) + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py (1)
apps/api/plane/db/models/issue.py (1)
IssueComment(446-501)
⏰ 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). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py (2)
1-8: LGTM!The imports are appropriate for a Django management command that performs batch operations.
10-11: LGTM!The command class is properly structured with appropriate help text.
sangeethailango
changed the title
sangeethailango
changed the title
sangeethailango
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py (2)
17-19: Add explicit ordering for determinism.The queryset lacks an
ORDER BYclause. While the filter ondescription_id__isnull=Trueensures each batch is distinct, an explicit order improves consistency and debuggability across database engines.Apply this diff to add ordering:
comments = list( - IssueComment.objects.filter(deleted_at__isnull=True, description_id__isnull=True)[:batch_size] + IssueComment.objects.filter( + deleted_at__isnull=True, description_id__isnull=True + ).order_by("id")[:batch_size] )
48-48: Consider adding progress messages.For large datasets, printing progress after each batch would improve observability during migration execution.
Example enhancement:
IssueComment.objects.bulk_update(comments_to_update, ["description_id"]) self.stdout.write(f"Processed {len(comments)} comments...") self.stdout.write(self.style.SUCCESS("Successfully Copied IssueComment to Description"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py (1)
apps/api/plane/db/models/issue.py (1)
IssueComment(446-501)
⏰ 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). (3)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (1)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py (1)
1-11: LGTM!Imports and command setup are clean and appropriate.
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py
Outdated
Show resolved
Hide resolved
chore: test cases
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/plane/db/models/issue.py (1)
471-478: Consider consistency with existing patterns for empty descriptions.The code sets
comment_stripped = ""whencomment_htmlis empty (line 472), but other models in the codebase (e.g.,Page.save()at apps/api/plane/db/models/page.py:65-72) setdescription_stripped = Nonefor empty HTML:self.description_stripped = ( None if (self.description_html == "" or self.description_html is None) else strip_tags(self.description_html) )This inconsistency may cause unexpected behavior. Consider aligning with the established pattern.
Minor: Use
self.pkinstead ofself.idon line 476.Line 476 uses
self.idwhen fetching the old comment. While functionally equivalent,self.pkis the more canonical Django pattern and should be preferred for consistency.Note: The early
super().save()call (line 478) correctly addresses the timing issues flagged in previous reviews.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/plane/db/models/issue.py(3 hunks)apps/api/plane/tests/unit/models/test_issue_comment_modal.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-23T18:18:06.875Z
Learnt from: NarayanBavisetti
Repo: makeplane/plane PR: 7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
Applied to files:
apps/api/plane/db/models/issue.py
🧬 Code graph analysis (2)
apps/api/plane/db/models/issue.py (1)
apps/api/plane/db/models/page.py (2)
save(66-73)save(171-178)
apps/api/plane/tests/unit/models/test_issue_comment_modal.py (4)
apps/api/plane/db/models/issue.py (4)
IssueComment(446-522)Issue(105-251)save(179-247)save(471-512)apps/api/plane/db/models/project.py (1)
Project(65-159)apps/api/plane/db/models/workspace.py (1)
Workspace(115-178)apps/api/plane/tests/conftest.py (1)
create_user(33-42)
⏰ 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). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
apps/api/plane/db/models/issue.py (1)
450-452: LGTM: Description field definition is correct.The OneToOneField definition properly establishes the relationship with appropriate cascade behavior and null support for legacy data.
apps/api/plane/tests/unit/models/test_issue_comment_modal.py (5)
6-48: LGTM: Test fixtures are well-structured.The fixtures properly set up the test environment with appropriate dependencies and use the existing
create_userfixture from conftest.
54-85: LGTM: Comprehensive test coverage for creation flow.The test properly verifies that Description is automatically created and all fields are synchronized correctly during IssueComment creation.
86-196: LGTM: Excellent coverage of update scenarios.These tests comprehensively verify the synchronization behavior for updates, including:
- Full updates (HTML + JSON)
- Partial updates (only HTML changed)
- No-op scenarios (no changes)
The tests align well with the change detection logic implemented in the model's
save()method.
197-264: LGTM: Strong coverage for edge cases.The legacy data test properly simulates the scenario where Description is missing and verifies it's created on update. The HTML stripping test ensures proper text extraction from HTML tags.
265-289: Test expectation is correct—no issue found.The test at line 289 correctly expects
description.description_stripped == None. WhenIssueComment.save()creates a Description with emptydescription_html, theDescriptionmodel's ownsave()method (apps/api/plane/db/models/description.py:18-23) explicitly converts empty descriptions toNone:self.description_stripped = ( None if (self.description_html == "" or self.description_html is None) else strip_tags(self.description_html) )Since
description_html=""is passed to the Description, itssave()method setsdescription_stripped=None, aligning with the test expectation and matching the pattern used in other models (Page,PageVersion,Sticky,Draft).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py (2)
13-23: Excellent fix to the batch processing logic!The while-True loop correctly pulls fresh batches until none remain, which resolves the previous issue where rows were being skipped. The ordering by
created_atand absence ofdeleted_atfilter align with the reviewer's feedback and the model's save() behavior.Optional enhancement: Add progress reporting.
For better observability during migration, consider logging the batch progress:
while True: comments = list( IssueComment.objects.filter(description_id__isnull=True).order_by("created_at")[:batch_size] ) if not comments: break + + self.stdout.write(f"Processing batch of {len(comments)} comments...") with transaction.atomic():
24-48: Correct field mapping and bulk operations!The Description field mappings are consistent with the
IssueComment.save()logic, and the bulk_update correctly uses"description_id"(addressing the previous issue). The transaction.atomic() ensures each batch is processed atomically.Optional enhancement: Add error handling for robustness.
While transaction rollback provides safety, explicit error handling could improve observability:
with transaction.atomic(): + try: descriptions = [ Description( created_at=comment.created_at, updated_at=comment.updated_at, description_json=comment.comment_json, description_html=comment.comment_html, description_stripped=comment.comment_stripped, project_id=comment.project_id, created_by_id=comment.created_by_id, updated_by_id=comment.updated_by_id, workspace_id=comment.workspace_id, ) for comment in comments ] created_descriptions = Description.objects.bulk_create(descriptions) comments_to_update = [] for comment, description in zip(comments, created_descriptions): comment.description_id = description.id comments_to_update.append(comment) IssueComment.objects.bulk_update(comments_to_update, ["description_id"]) + except Exception as e: + self.stdout.write(self.style.ERROR(f"Error processing batch: {e}")) + raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py (1)
apps/api/plane/db/models/issue.py (1)
IssueComment(446-522)
⏰ 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). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/api/plane/db/management/commands/copy_issue_comment_to_description.py (2)
1-11: LGTM! Clean setup.The imports and command class definition are appropriate and well-structured.
49-49: LGTM!Clear success message confirms completion.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/api/plane/db/migrations/0109_issuecomment_description_and_parent_id.py (1)
14-18: Consider SET_NULL instead of CASCADE for the description field.The
on_delete=CASCADEmeans that if a Description record is accidentally deleted, the IssueComment will also be deleted. Since IssueComment semantically "owns" the Description (not the reverse), consider usingon_delete=models.SET_NULLto preserve the comment if the Description is deleted, oron_delete=models.PROTECTto prevent orphaning.Apply this diff if you want to use SET_NULL:
migrations.AddField( model_name='issuecomment', name='description', - field=models.OneToOneField(null=True, on_delete=django.db.models.deletion.CASCADE, related_name='issue_comment_description', to='db.description'), + field=models.OneToOneField(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='issue_comment_description', to='db.description'), ),Can you verify whether Description records might be deleted independently, and whether IssueComment should survive such deletions?
apps/api/plane/db/models/issue.py (1)
478-481: Minor: Race condition in change detection.Fetching
old_issue_commentat line 479 beforesuper().save()creates a small race window where another process could update the comment between the fetch and save, causing your change detection to compare against stale data. For most use cases this is acceptable, but for strict consistency you could store original values in__init__or use Django's built-in field tracking (e.g.,django-model-utilsFieldTracker).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/plane/db/migrations/0109_issuecomment_description_and_parent_id.py(1 hunks)apps/api/plane/db/models/issue.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-23T18:18:06.875Z
Learnt from: NarayanBavisetti
Repo: makeplane/plane PR: 7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
Applied to files:
apps/api/plane/db/models/issue.py
🧬 Code graph analysis (1)
apps/api/plane/db/models/issue.py (2)
apps/api/plane/db/models/page.py (2)
save(66-73)save(171-178)apps/api/plane/db/models/cycle.py (1)
save(84-93)
⏰ 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). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
apps/api/plane/db/migrations/0109_issuecomment_description_and_parent_id.py (1)
19-23: Verify CASCADE deletion for nested comment replies.The
on_delete=models.CASCADEfor the self-referential parent field means deleting a parent comment will recursively delete all child replies. If this is intentional (nested comment threads should be removed together), this is correct. However, if replies should be preserved when a parent is deleted (perhaps re-parenting them or making them top-level), considerSET_NULLinstead.Can you confirm that deleting a parent comment should also delete all its replies?
apps/api/plane/db/models/issue.py (2)
499-509: Excellent: Change detection addresses previous feedback.The implementation now properly detects which fields have changed and only updates the Description when necessary. This directly addresses the feedback from dheeru0198 about avoiding unnecessary syncs.
Based on learnings.
493-497: LGTM: Correct creation and update order.Creating the Description after
super().save()ensures timestamps are populated, and usingupdate_fields=["description_id"]on the second save prevents infinite recursion. This properly addresses previous timing issues.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/plane/db/models/issue.py (1)
450-452: Verify the on_delete direction for the description field.The
on_delete=models.CASCADEon the description field means that if a Description record is deleted, the IssueComment will also be deleted. Since the save() method treats Description as a dependent entity managed by IssueComment, this direction seems counterintuitive. If the Description is accidentally deleted, the entire comment would be lost.Consider using
on_delete=models.PROTECToron_delete=models.SET_NULLto prevent the IssueComment from being deleted if the Description is removed, or verify that CASCADE is the intended behavior given the architecture.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/db/models/issue.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-23T18:18:06.875Z
Learnt from: NarayanBavisetti
Repo: makeplane/plane PR: 7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
Applied to files:
apps/api/plane/db/models/issue.py
🧬 Code graph analysis (1)
apps/api/plane/db/models/issue.py (4)
apps/api/plane/db/models/page.py (2)
save(66-73)save(171-178)apps/api/plane/db/models/cycle.py (1)
save(84-93)apps/api/plane/db/models/view.py (1)
save(74-90)apps/api/plane/db/models/module.py (1)
save(111-120)
Description
Summary by CodeRabbit
New Features
Tests
Chores