chore: add sync jobs for issue_version and issue_description_version tables #6199
Conversation
|
sainath seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce new functionalities for synchronizing issue descriptions and versions in a Django application using Celery tasks. Two new files, Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
apiserver/plane/db/migrations/0087_remove_issueversion_description_and_more.py (1)
38-104: Adjust lines exceeding the maximum line lengthSeveral lines exceed the maximum line length of 88 characters, as indicated by the static analysis tool. Consider wrapping these lines to improve code readability and comply with PEP 8 style guidelines.
For example, in line 38:
- field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='versions', to='db.issueactivity'), + field=models.ForeignKey( + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name='versions', + to='db.issueactivity' + ),Apply similar adjustments to other lines exceeding the maximum length.
🧰 Tools
🪛 Ruff (0.8.2)
38-38: Line too long (141 > 88)
(E501)
58-58: Line too long (141 > 88)
(E501)
63-63: Line too long (99 > 88)
(E501)
64-64: Line too long (101 > 88)
(E501)
65-65: Line too long (103 > 88)
(E501)
66-66: Line too long (140 > 88)
(E501)
74-74: Line too long (94 > 88)
(E501)
75-75: Line too long (200 > 88)
(E501)
76-76: Line too long (144 > 88)
(E501)
77-77: Line too long (206 > 88)
(E501)
78-78: Line too long (138 > 88)
(E501)
90-90: Line too long (99 > 88)
(E501)
91-91: Line too long (101 > 88)
(E501)
92-92: Line too long (103 > 88)
(E501)
93-93: Line too long (140 > 88)
(E501)
98-98: Line too long (91 > 88)
(E501)
99-99: Line too long (200 > 88)
(E501)
100-100: Line too long (142 > 88)
(E501)
101-101: Line too long (165 > 88)
(E501)
102-102: Line too long (143 > 88)
(E501)
103-103: Line too long (206 > 88)
(E501)
104-104: Line too long (149 > 88)
(E501)
apiserver/plane/bgtasks/issue_version_sync.py (1)
69-85: Refactorget_owner_idto avoid code duplicationThe
get_owner_idfunction is duplicated in bothissue_version_sync.pyandissue_description_version_sync.py. Consider moving this function to a shared utility module to promote code reusability and maintainability.apiserver/plane/db/management/commands/sync_issue_version.py (1)
8-10: Enhance command documentationThe help text should provide more details about the expected input formats and their purpose.
Consider expanding the help text:
class Command(BaseCommand): - help = "Creates IssueVersion records for existing Issues in batches" + help = """ + Creates IssueVersion records for existing Issues in batches. + + Required inputs: + - batch_size: Positive integer specifying how many issues to process per batch + - batch_countdown: Non-negative integer specifying delay (in seconds) between batches + """apiserver/plane/db/models/sticky.py (2)
18-19: Add color format validationThe color fields should validate hex color codes or predefined color names.
Consider adding a validator:
import re from django.core.validators import RegexValidator color_validator = RegexValidator( re.compile('^#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$'), 'Enter a valid hex color code (e.g. #FF0000 or #F00)', 'invalid_color' )Then apply it to the fields:
- color = models.CharField(max_length=255, blank=True, null=True) - background_color = models.CharField(max_length=255, blank=True, null=True) + color = models.CharField(max_length=7, blank=True, null=True, validators=[color_validator]) + background_color = models.CharField(max_length=7, blank=True, null=True, validators=[color_validator])
12-15: Review storage strategy for multiple description formatsThe model stores the description in multiple formats (JSON, HTML, stripped text, binary) which could lead to storage inefficiency and consistency issues.
Consider:
- Using computed properties instead of stored fields where possible
- Implementing signals to automatically update derived fields
- Documenting the purpose of each format, especially the binary field
- Adding integrity checks to ensure consistency between formats
Example implementation:
from django.db.models.signals import pre_save from django.dispatch import receiver import html2text @receiver(pre_save, sender=Sticky) def update_description_formats(sender, instance, **kwargs): if instance.description_html: # Strip HTML to get plain text h = html2text.HTML2Text() h.ignore_links = True instance.description_stripped = h.handle(instance.description_html).strip()apiserver/plane/db/models/user.py (1)
29-35: Add documentation to explain the mobile onboarding differences.Please add a docstring to explain why the mobile onboarding steps differ from the default onboarding (specifically, why 'workspace_invite' is omitted).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apiserver/plane/bgtasks/issue_description_version_sync.py(1 hunks)apiserver/plane/bgtasks/issue_version_sync.py(1 hunks)apiserver/plane/db/management/commands/sync_issue_description_version.py(1 hunks)apiserver/plane/db/management/commands/sync_issue_version.py(1 hunks)apiserver/plane/db/migrations/0087_remove_issueversion_description_and_more.py(1 hunks)apiserver/plane/db/models/__init__.py(2 hunks)apiserver/plane/db/models/issue.py(4 hunks)apiserver/plane/db/models/sticky.py(1 hunks)apiserver/plane/db/models/user.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
apiserver/plane/db/migrations/0087_remove_issueversion_description_and_more.py
38-38: Line too long (141 > 88)
(E501)
58-58: Line too long (141 > 88)
(E501)
63-63: Line too long (99 > 88)
(E501)
64-64: Line too long (101 > 88)
(E501)
65-65: Line too long (103 > 88)
(E501)
66-66: Line too long (140 > 88)
(E501)
74-74: Line too long (94 > 88)
(E501)
75-75: Line too long (200 > 88)
(E501)
76-76: Line too long (144 > 88)
(E501)
77-77: Line too long (206 > 88)
(E501)
78-78: Line too long (138 > 88)
(E501)
90-90: Line too long (99 > 88)
(E501)
91-91: Line too long (101 > 88)
(E501)
92-92: Line too long (103 > 88)
(E501)
93-93: Line too long (140 > 88)
(E501)
98-98: Line too long (91 > 88)
(E501)
99-99: Line too long (200 > 88)
(E501)
100-100: Line too long (142 > 88)
(E501)
101-101: Line too long (165 > 88)
(E501)
102-102: Line too long (143 > 88)
(E501)
103-103: Line too long (206 > 88)
(E501)
104-104: Line too long (149 > 88)
(E501)
apiserver/plane/db/models/issue.py
18-18: .base.BaseModel imported but unused
Remove unused import: .base.BaseModel
(F401)
🔇 Additional comments (10)
apiserver/plane/bgtasks/issue_description_version_sync.py (3)
16-32: LGTM: get_owner_id function is properly implemented
The get_owner_id function correctly determines the owner of an issue by prioritizing updated_by_id, then created_by_id, and finally falling back to a project admin if necessary.
34-116: LGTM: sync_issue_description_version function is well-structured
The sync_issue_description_version function efficiently processes issues in batches, creates IssueDescriptionVersion records, and schedules subsequent batches when necessary. Error handling and transaction management are appropriately implemented.
118-122: LGTM: schedule_issue_description_version function correctly schedules tasks
The schedule_issue_description_version function accurately triggers the sync_issue_description_version task with the specified parameters.
apiserver/plane/db/migrations/0087_remove_issueversion_description_and_more.py (1)
18-112: LGTM: Migration script correctly alters the database schema
The migration script appropriately removes obsolete fields, adds new fields, and creates new models. The operations are well-defined and align with the intended database schema changes.
🧰 Tools
🪛 Ruff (0.8.2)
38-38: Line too long (141 > 88)
(E501)
58-58: Line too long (141 > 88)
(E501)
63-63: Line too long (99 > 88)
(E501)
64-64: Line too long (101 > 88)
(E501)
65-65: Line too long (103 > 88)
(E501)
66-66: Line too long (140 > 88)
(E501)
74-74: Line too long (94 > 88)
(E501)
75-75: Line too long (200 > 88)
(E501)
76-76: Line too long (144 > 88)
(E501)
77-77: Line too long (206 > 88)
(E501)
78-78: Line too long (138 > 88)
(E501)
90-90: Line too long (99 > 88)
(E501)
91-91: Line too long (101 > 88)
(E501)
92-92: Line too long (103 > 88)
(E501)
93-93: Line too long (140 > 88)
(E501)
98-98: Line too long (91 > 88)
(E501)
99-99: Line too long (200 > 88)
(E501)
100-100: Line too long (142 > 88)
(E501)
101-101: Line too long (165 > 88)
(E501)
102-102: Line too long (143 > 88)
(E501)
103-103: Line too long (206 > 88)
(E501)
104-104: Line too long (149 > 88)
(E501)
apiserver/plane/bgtasks/issue_version_sync.py (3)
28-67: LGTM: issue_task function properly logs issue versions
The issue_task function correctly identifies changes in issues and logs versions accordingly. The logic for updating existing versions or creating new ones is sound.
87-142: LGTM: get_related_data function efficiently retrieves related data
The get_related_data function effectively gathers related records such as cycle issues, assignees, labels, modules, and activities, optimizing data retrieval for batch processing.
193-247: LGTM: sync_issue_version function handles batch processing effectively
The sync_issue_version function processes issues in batches, creates IssueVersion records, and schedules subsequent tasks appropriately. Error handling and transaction management are properly implemented.
apiserver/plane/db/models/issue.py (1)
Line range hint 671-793: LGTM: Updates to IssueVersion and addition of IssueDescriptionVersion
The modifications to the IssueVersion class and the introduction of the IssueDescriptionVersion class are well-structured. The fields and methods align with the requirements for tracking issue versions and descriptions.
apiserver/plane/db/models/sticky.py (1)
15-15: Verify security implications of BinaryField
The description_binary field could pose security risks if not properly handled.
Let's check for any potential security issues in the codebase:
✅ Verification successful
BinaryField implementation appears secure with proper handling
The implementation shows appropriate security measures:
- Binary data is properly streamed using StreamingHttpResponse with correct content-type
- Base64 encoding/decoding is used for data transfer
- Access control checks are in place before data retrieval
- No direct binary data exposure in serializers
- Data is handled through proper Django model fields and controlled endpoints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BinaryField usage and potential security risks
# Check for direct binary field access in views
echo "Checking views for binary field access..."
rg -l "description_binary" --type py | grep "views"
# Check for proper content-type handling in responses
echo "Checking response handling..."
rg -A 5 "description_binary" --type py | grep -B 2 -A 2 "HttpResponse"
# Look for file upload handling
echo "Checking file upload handling..."
rg -l "multipart/form-data" --type py
Length of output: 469
Script:
#!/bin/bash
# Let's examine the specific usage of description_binary in views
# Check the implementation in issue.py
echo "Checking issue.py implementation:"
rg -B 5 -A 10 "description_binary" apiserver/plane/space/views/issue.py
# Check the implementation in base.py
echo "Checking base.py implementation:"
rg -B 5 -A 10 "description_binary" apiserver/plane/app/views/page/base.py
# Check for any serializer handling of this field
echo "Checking serializers:"
rg -B 2 -A 2 "description_binary" --type py | grep -B 2 -A 2 "serializers"
# Check for any validation or sanitization
echo "Checking for validation:"
rg -B 2 -A 2 "description_binary" --type py | grep -B 2 -A 2 "clean|validate"
Length of output: 3148
apiserver/plane/db/models/__init__.py (1)
44-45: LGTM! Import statements are properly organized.
The new imports for IssueVersion, IssueDescriptionVersion, and Sticky are correctly placed and maintain the file's organization.
Also applies to: 82-83
apiserver/plane/db/management/commands/sync_issue_description_version.py
Show resolved
Hide resolved
Description
Added synchronization jobs for
issue_versionandissue_description_versiontables. These jobs ensure data consistency by syncing information across the relevant tables.Type of Change
Summary by CodeRabbit