[WEB-1435] chore: Update table columns and add commands for background job to populate issue data #6177
Conversation
…e-description-version
…e-description-version
…e-description-version
WalkthroughThe pull request introduces several new files and functionalities for managing issue descriptions and versions in a Django application using Celery for asynchronous task processing. Key features include tasks for synchronizing issue descriptions and versions, management commands for batch processing, and modifications to the database schema. New models and methods are added to enhance the handling of issue data, including error logging and validation. The changes also include migration scripts to update the database structure accordingly. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 9
🧹 Outside diff range and nitpick comments (7)
apiserver/plane/bgtasks/issue_description_version_sync.py (1)
72-72: Use logging instead of print statements for error messagesIn production code, it's recommended to use the
loggingmodule instead ofprint()statements for better control over log levels and outputs.Apply this diff to replace
print()withlogger.warning():+import logging +logger = logging.getLogger(__name__) ... if not issue.workspace_id or not issue.project_id: - print(f"Skipping {issue.id} - missing workspace_id or project_id") + logger.warning(f"Skipping {issue.id} - missing workspace_id or project_id") continue ... if owned_by_id is None: - print(f"Skipping issue {issue.id} - missing owned_by") + logger.warning(f"Skipping issue {issue.id} - missing owned_by") continueAlso applies to: 78-78
apiserver/plane/db/migrations/0087_remove_issueversion_description_and_more.py (2)
38-38: Line length exceeds the recommended maximum; consider wrapping long linesThe static analysis tool
Ruffhas flagged that several lines exceed the maximum recommended line length of 88 characters. Consider wrapping long lines to improve code readability.Apply line wrapping to the following lines:
- Line 38
- Line 43
- Line 63
- Lines 68 to 82
Example:
field=models.ForeignKey( null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='versions', to='db.issueactivity', ),Also applies to: 43-43, 63-63, 68-82
🧰 Tools
🪛 Ruff (0.8.0)
38-38: Line too long (141 > 88)
(E501)
71-71: Redundant 'unique=True' option on primary key field 'id'In the definition of the
idfield forIssueDescriptionVersion, specifyingunique=Trueis redundant since primary keys are inherently unique.Apply this diff to remove the redundant
unique=True:('id', models.UUIDField( db_index=True, default=uuid.uuid4, editable=False, primary_key=True, serialize=False, - unique=True ))🧰 Tools
🪛 Ruff (0.8.0)
71-71: Line too long (140 > 88)
(E501)
apiserver/plane/bgtasks/issue_version_sync.py (2)
79-82: Define role constants instead of hardcodingHardcoding role values like
20reduces code readability and makes maintenance harder. Consider defining role constants or an enumeration to represent user roles more clearly.For example, define a constant
ADMIN_ROLE = 20and update the code:+ADMIN_ROLE = 20 ... project_member = ProjectMember.objects.filter( project_id=issue.project_id, - role=20, # Admin role + role=ADMIN_ROLE, # Admin role ).first()
206-207: Use logging instead of print statementsUsing
loggingmodule to provide better control over logging levels and outputs.Apply this diff to replace
+import logging +logger = logging.getLogger(__name__) ... - print(f"Offset: {offset}") - print(f"Total Issues: {total_issues_count}") + logger.info(f"Offset: {offset}") + logger.info(f"Total Issues: {total_issues_count}") ... - print(f"Processed Issues: {end_offset}") + logger.info(f"Processed Issues: {end_offset}")Also applies to: 246-246
apiserver/plane/db/models/issue.py (1)
Line range hint
59-59: Pass a user instance tolog_issue_versionThe method
IssueVersion.log_issue_version(issue, user)expects a user instance, but a user ID might be passed instead. Ensure that a user instance is provided to avoid attribute errors.apiserver/plane/db/models/user.py (1)
189-192: Consider adding field documentation.The new mobile-specific fields would benefit from docstrings or comments explaining their purpose and relationships:
# mobile - is_mobile_onboarded = models.BooleanField(default=False) - mobile_onboarding_step = models.JSONField(default=get_mobile_default_onboarding) - mobile_timezone_auto_set = models.BooleanField(default=False) + # Tracks if the user has completed the mobile app onboarding process + is_mobile_onboarded = models.BooleanField(default=False) + # Stores the current step in the mobile onboarding flow + mobile_onboarding_step = models.JSONField(default=get_mobile_default_onboarding) + # Indicates if the timezone was automatically set by the mobile app + mobile_timezone_auto_set = models.BooleanField(default=False)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
apiserver/plane/bgtasks/issue_description_version_sync.py(1 hunks)apiserver/plane/bgtasks/issue_description_version_task.py(1 hunks)apiserver/plane/bgtasks/issue_version_sync.py(1 hunks)apiserver/plane/bgtasks/issue_version_task.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(1 hunks)apiserver/plane/db/models/issue.py(3 hunks)apiserver/plane/db/models/user.py(2 hunks)apiserver/plane/settings/common.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
apiserver/plane/db/migrations/0087_remove_issueversion_description_and_more.py
38-38: Line too long (141 > 88)
(E501)
43-43: Line too long (165 > 88)
(E501)
63-63: Line too long (141 > 88)
(E501)
68-68: Line too long (99 > 88)
(E501)
69-69: Line too long (101 > 88)
(E501)
70-70: Line too long (103 > 88)
(E501)
71-71: Line too long (140 > 88)
(E501)
76-76: Line too long (91 > 88)
(E501)
77-77: Line too long (200 > 88)
(E501)
78-78: Line too long (142 > 88)
(E501)
79-79: Line too long (165 > 88)
(E501)
80-80: Line too long (143 > 88)
(E501)
81-81: Line too long (206 > 88)
(E501)
82-82: Line too long (149 > 88)
(E501)
🔇 Additional comments (6)
apiserver/plane/bgtasks/issue_version_task.py (1)
49-90: Code logic in 'issue_version_task' appears correct and handles exceptions appropriately
The issue_version_task function effectively processes issue updates, handles exceptions, and maintains data integrity within a transaction.
apiserver/plane/bgtasks/issue_description_version_task.py (1)
44-85: Code logic in 'issue_description_version_task' appears correct and handles exceptions appropriately
The issue_description_version_task function effectively processes issue description updates, handles exceptions, and maintains data integrity within a transaction.
apiserver/plane/bgtasks/issue_description_version_sync.py (1)
16-115: Code logic in 'sync_issue_description_version' appears correct and handles batching appropriately
The sync_issue_description_version function efficiently processes issues in batches and schedules subsequent tasks as needed.
apiserver/plane/db/models/__init__.py (1)
44-45: LGTM!
The new imports for IssueVersion and IssueDescriptionVersion are properly organized within the issue module imports.
apiserver/plane/db/models/user.py (1)
29-34: LGTM!
The mobile onboarding function correctly omits the 'workspace_invite' step which is not applicable for mobile users.
apiserver/plane/settings/common.py (1)
265-267: LGTM!
The new Celery tasks for issue version synchronization are properly organized and categorized under "issue sync tasks".
| def get_changed_fields(current_issue: Dict[str, Any], issue: Issue) -> Dict[str, Any]: | ||
| return { | ||
| key: value | ||
| for key, value in current_issue.items() | ||
| if getattr(issue, key) != value | ||
| } |
There was a problem hiding this comment.
Handle missing attributes in 'getattr' to prevent AttributeError
In the get_changed_fields function, consider providing a default value to getattr(issue, key) to handle cases where the attribute may not exist in the Issue object, preventing potential AttributeErrors.
Apply this diff to handle missing attributes:
def get_changed_fields(current_issue: Dict[str, Any], issue: Issue) -> Dict[str, Any]:
return {
key: value
for key, value in current_issue.items()
- if getattr(issue, key) != value
+ if getattr(issue, key, None) != value
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_changed_fields(current_issue: Dict[str, Any], issue: Issue) -> Dict[str, Any]: | |
| return { | |
| key: value | |
| for key, value in current_issue.items() | |
| if getattr(issue, key) != value | |
| } | |
| def get_changed_fields(current_issue: Dict[str, Any], issue: Issue) -> Dict[str, Any]: | |
| return { | |
| key: value | |
| for key, value in current_issue.items() | |
| if getattr(issue, key, None) != value | |
| } |
| def update_version_fields( | ||
| version: IssueVersion, changed_fields: Dict[str, Any] | ||
| ) -> List[str]: | ||
| for key, value in changed_fields.items(): | ||
| setattr(version, key, value) | ||
|
|
||
| version.last_saved_at = timezone.now() | ||
| update_fields = list(changed_fields.keys()) + ["last_saved_at"] | ||
| return update_fields | ||
|
|
There was a problem hiding this comment.
Ensure only valid fields are updated in 'update_version_fields'
In the update_version_fields function, consider verifying that the keys in changed_fields correspond to valid fields of the IssueVersion model to prevent potential AttributeErrors or unintended attribute creation.
Apply this diff to validate fields:
def update_version_fields(
version: IssueVersion, changed_fields: Dict[str, Any]
) -> List[str]:
+ valid_fields = {field.name for field in IssueVersion._meta.get_fields()}
for key, value in changed_fields.items():
+ if key in valid_fields:
setattr(version, key, value)
+ else:
+ # Optionally, log or handle invalid field names
+ pass
version.last_saved_at = timezone.now()
update_fields = list(changed_fields.keys()) + ["last_saved_at"]
return update_fieldsCommittable suggestion skipped: line range outside the PR's diff.
| def should_update_existing_version( | ||
| version: IssueDescriptionVersion, user_id: str, max_time_difference: int = 600 | ||
| ) -> bool: | ||
| if not version: | ||
| return | ||
|
|
||
| time_difference = (timezone.now() - version.last_saved_at).total_seconds() | ||
| return ( | ||
| str(version.owned_by_id) == str(user_id) | ||
| and time_difference <= max_time_difference | ||
| ) | ||
|
|
There was a problem hiding this comment.
Ensure 'should_update_existing_version' consistently returns a boolean value
In should_update_existing_version, the function should return a boolean in all code paths to match the declared return type -> bool. Currently, if version is None, it returns None. Consider returning False instead.
Apply this diff to fix the return value:
def should_update_existing_version(
version: IssueDescriptionVersion, user_id: str, max_time_difference: int = 600
) -> bool:
if not version:
- return
+ return False
time_difference = (timezone.now() - version.last_saved_at).total_seconds()
return (
str(version.owned_by_id) == str(user_id)
and time_difference <= max_time_difference
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def should_update_existing_version( | |
| version: IssueDescriptionVersion, user_id: str, max_time_difference: int = 600 | |
| ) -> bool: | |
| if not version: | |
| return | |
| time_difference = (timezone.now() - version.last_saved_at).total_seconds() | |
| return ( | |
| str(version.owned_by_id) == str(user_id) | |
| and time_difference <= max_time_difference | |
| ) | |
| def should_update_existing_version( | |
| version: IssueDescriptionVersion, user_id: str, max_time_difference: int = 600 | |
| ) -> bool: | |
| if not version: | |
| return False | |
| time_difference = (timezone.now() - version.last_saved_at).total_seconds() | |
| return ( | |
| str(version.owned_by_id) == str(user_id) | |
| and time_difference <= max_time_difference | |
| ) |
| project_member = ProjectMember.objects.filter( | ||
| project_id=issue.project_id, | ||
| role=20, # Admin role | ||
| ).first() | ||
|
|
||
| return project_member.member_id if project_member else None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding role values; use constants or enumerations for 'Admin' role
In the get_owner_id function, avoid hardcoding role=20. Instead, define a constant or use an enumeration to represent the 'Admin' role for better readability and maintainability.
Apply this diff:
+from plane.db.models import ProjectMember
+ADMIN_ROLE = ProjectMember.Role.ADMIN
...
project_member = ProjectMember.objects.filter(
project_id=issue.project_id,
- role=20, # Admin role
+ role=ADMIN_ROLE,
).first()Ensure that ProjectMember.Role.ADMIN is defined appropriately in your models.
Committable suggestion skipped: line range outside the PR's diff.
| and str(issue_version.owned_by) == str(user_id) | ||
| and (timezone.now() - issue_version.last_saved_at).total_seconds() |
There was a problem hiding this comment.
Use owned_by_id for accurate comparison
Comparing str(issue_version.owned_by) with str(user_id) may not work as intended because issue_version.owned_by is a user instance, not an ID. To ensure proper comparison, compare the IDs directly using issue_version.owned_by_id == user_id.
Apply this diff to fix the comparison:
- and str(issue_version.owned_by) == str(user_id)
+ and issue_version.owned_by_id == user_id📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| and str(issue_version.owned_by) == str(user_id) | |
| and (timezone.now() - issue_version.last_saved_at).total_seconds() | |
| and issue_version.owned_by_id == user_id | |
| and (timezone.now() - issue_version.last_saved_at).total_seconds() |
| update_fields=list(updated_current_issue.keys()) + ["last_saved_at"] | ||
| ) | ||
| else: | ||
| IssueVersion.log_issue_version(issue, user_id) |
There was a problem hiding this comment.
Pass a user instance to log_issue_version
The method IssueVersion.log_issue_version(issue, user) expects a user instance, but user_id is being passed. This will cause issues when the method tries to assign owned_by=user. Fetch the user instance before passing it to the method.
Apply this diff to fix the issue:
+ from django.contrib.auth import get_user_model
+ User = get_user_model()
+ user = User.objects.get(id=user_id)
...
- IssueVersion.log_issue_version(issue, user_id)
+ IssueVersion.log_issue_version(issue, user)Committable suggestion skipped: line range outside the PR's diff.
| Module.objects.filter(issue=issue).values_list("id", flat=True) | ||
| ), | ||
| properties={}, |
There was a problem hiding this comment.
Fix module retrieval in log_issue_version
The current query Module.objects.filter(issue=issue) may not retrieve the correct modules associated with the issue because Module does not have a direct relationship with Issue. Instead, you should query using the related manager on the Issue instance.
Apply this diff to fix the module retrieval:
modules=list(
- Module.objects.filter(issue=issue).values_list("id", flat=True)
+ issue.module_set.values_list("id", flat=True)
),Assuming that there is a related name module_set through the ModuleIssue relationship.
Committable suggestion skipped: line range outside the PR's diff.
| batch_size = input("Enter the batch size: ") | ||
| batch_countdown = input("Enter the batch countdown: ") | ||
|
|
||
| schedule_issue_version.delay( | ||
| batch_size=int(batch_size), countdown=int(batch_countdown) | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for user inputs
Currently, the command does not validate user input for batch_size and batch_countdown. If the user enters a non-integer value, it will raise a ValueError. Consider adding validation to ensure robust error handling.
Apply this diff to add input validation:
def handle(self, *args, **options):
batch_size = input("Enter the batch size: ")
batch_countdown = input("Enter the batch countdown: ")
+ try:
+ batch_size = int(batch_size)
+ batch_countdown = int(batch_countdown)
+ except ValueError:
+ self.stderr.write("Please enter valid integer values for batch size and countdown.")
+ return
schedule_issue_version.delay(
- batch_size=int(batch_size), countdown=int(batch_countdown)
+ batch_size=batch_size, countdown=batch_countdown
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| batch_size = input("Enter the batch size: ") | |
| batch_countdown = input("Enter the batch countdown: ") | |
| schedule_issue_version.delay( | |
| batch_size=int(batch_size), countdown=int(batch_countdown) | |
| ) | |
| batch_size = input("Enter the batch size: ") | |
| batch_countdown = input("Enter the batch countdown: ") | |
| try: | |
| batch_size = int(batch_size) | |
| batch_countdown = int(batch_countdown) | |
| except ValueError: | |
| self.stderr.write("Please enter valid integer values for batch size and countdown.") | |
| return | |
| schedule_issue_version.delay( | |
| batch_size=batch_size, countdown=batch_countdown | |
| ) |
| batch_size = input("Enter the batch size: ") | ||
| batch_countdown = input("Enter the batch countdown: ") | ||
|
|
||
| schedule_issue_description_version.delay( | ||
| batch_size=int(batch_size), countdown=int(batch_countdown) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for user inputs
Similar to the issue in sync_issue_version.py, this command lacks input validation. If non-integer values are entered, a ValueError will occur. Implement input validation to handle such cases gracefully.
Apply this diff to add input validation:
def handle(self, *args, **options):
batch_size = input("Enter the batch size: ")
batch_countdown = input("Enter the batch countdown: ")
+ try:
+ batch_size = int(batch_size)
+ batch_countdown = int(batch_countdown)
+ except ValueError:
+ self.stderr.write("Please enter valid integer values for batch size and countdown.")
+ return
schedule_issue_description_version.delay(
- batch_size=int(batch_size), countdown=int(batch_countdown)
+ batch_size=batch_size, countdown=batch_countdown
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| batch_size = input("Enter the batch size: ") | |
| batch_countdown = input("Enter the batch countdown: ") | |
| schedule_issue_description_version.delay( | |
| batch_size=int(batch_size), countdown=int(batch_countdown) | |
| ) | |
| batch_size = input("Enter the batch size: ") | |
| batch_countdown = input("Enter the batch countdown: ") | |
| try: | |
| batch_size = int(batch_size) | |
| batch_countdown = int(batch_countdown) | |
| except ValueError: | |
| self.stderr.write("Please enter valid integer values for batch size and countdown.") | |
| return | |
| schedule_issue_description_version.delay( | |
| batch_size=batch_size, countdown=batch_countdown | |
| ) |
pushya22
changed the title
| @@ -178,6 +186,10 @@ class Profile(TimeAuditModel): | |||
| billing_address = models.JSONField(null=True) | |||
| has_billing_address = models.BooleanField(default=False) | |||
| company_name = models.CharField(max_length=255, blank=True) | |||
| # mobile | |||
| is_mobile_onboarded = models.BooleanField(default=False) | |||
There was a problem hiding this comment.
move these to user profile table.
Description
Updated the table columns for
issue_versionandissue_description_versiontables. Added commands to run the background job that populates data fromissue_tabletoissue_versionandissue_description_versiontables.Test Scenarios
issue_versionandissue_description_versiontables work as expected.issue_table.Summary by CodeRabbit
New Features
IssueDescriptionVersionto enhance issue management.Bug Fixes
Documentation
Chores