chore: issue description version history #6129
Conversation
WalkthroughThe changes introduce a new asynchronous task, 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: 1
🧹 Outside diff range and nitpick comments (6)
apiserver/plane/db/models/issue.py (4)
741-743: Consider importing models directly to improve readability and performance.Using
apps.get_model()to retrieveModuleIssueandCycleIssuemodels can impact performance and reduce code readability. If there are no circular dependency issues, consider importing these models directly at the top of the file.Apply this change to import the models directly:
# At the top of the file from plane.db.models import ModuleIssue, CycleIssue # Then, you can use the models directly without `apps.get_model()` # Remove these lines inside the method - ModuleIssue = apps.get_model("db.ModuleIssue") - CycleIssue = apps.get_model("db.CycleIssue")
Line range hint
750-775: Incorrect retrieval of module IDs; usemodule_idinstead ofid.When fetching module IDs associated with the issue, you should retrieve the
module_idfield from theModuleIssuemodel. Currently, you are retrieving theids of theModuleIssueinstances, not theModuleinstances.Apply this diff to fix the issue:
modules=list( - ModuleIssue.objects.filter(issue=issue).values_list("id", flat=True) + ModuleIssue.objects.filter(issue=issue).values_list("module_id", flat=True) ),
770-770: Consider usingupdated_atinstead oflast_saved_atto avoid redundancy.Since
IssueVersioninheritscreated_atandupdated_atfromProjectBaseModel, thelast_saved_atfield may be redundant. Consider usingupdated_atto track when the version was last saved.You can remove the
last_saved_atfield and update references accordingly:
- Remove
last_saved_atfrom the model fields.- Replace
last_saved_atwithupdated_atin your code.
Line range hint
741-775: Add unit tests for thelog_issue_versionmethod.To ensure the correctness and reliability of the version logging functionality, consider adding unit tests for the
log_issue_versionmethod. Tests should cover various scenarios, including issues with or without cycles, modules, assignees, and labels.apiserver/plane/bgtasks/issue_version_task.py (2)
32-32: Consider tracking changes to additional fields for comprehensive versioning.Currently, version logging is triggered only when
description_htmlchanges. To ensure a complete version history, consider checking for changes in other significant fields such asname,priority, orstate.Modify the condition to check multiple fields:
if ( current_instance.get("description_html") != issue.description_html or current_instance.get("name") != issue.name or current_instance.get("priority") != issue.priority # Add other fields as necessary ): # Proceed with version logging
63-63: Ensure consistent parameter naming for clarity.In
IssueVersion.log_issue_version(issue, user_id), theuserparameter is actually a user ID (UUID). For clarity, consider renaming the parameter fromusertouser_idin the method definition to reflect its actual content.Update the method signature and references:
@classmethod -def log_issue_version(cls, issue, user): +def log_issue_version(cls, issue, user_id): # ... - owned_by=user, + owned_by=user_id,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apiserver/plane/app/views/issue/base.py(2 hunks)apiserver/plane/bgtasks/issue_version_task.py(1 hunks)apiserver/plane/db/models/__init__.py(1 hunks)apiserver/plane/db/models/issue.py(4 hunks)
🔇 Additional comments (3)
apiserver/plane/db/models/issue.py (1)
12-12: Importing apps from django.apps is appropriate.
The import statement change to from django.apps import apps is acceptable and allows for dynamic model retrieval using apps.get_model().
apiserver/plane/db/models/__init__.py (1)
44-44: Importing IssueVersion model is appropriate.
The addition of IssueVersion to the imports integrates the new model into the application's models module correctly.
apiserver/plane/app/views/issue/base.py (1)
656-658: Asynchronous version logging is correctly integrated.
The addition of issue_version_task.delay() in the partial_update method ensures that issue versioning is handled asynchronously after an update, which is appropriate for maintaining responsiveness.
| json.loads(existing_instance) if existing_instance is not None else {} | ||
| ) |
There was a problem hiding this comment.
Add error handling for JSON parsing of existing_instance.
Parsing existing_instance without handling potential JSONDecodeError exceptions may lead to crashes if invalid JSON is provided. Consider adding a try-except block to handle this scenario gracefully.
Apply this diff to fix the issue:
try:
current_instance = (
- json.loads(existing_instance) if existing_instance is not None else {}
+ json.loads(existing_instance) if existing_instance else {}
)
+except json.JSONDecodeError:
+ current_instance = {}
+ # Optionally, log the error or handle it appropriatelyCommittable suggestion skipped: line range outside the PR's diff.
chore:
Summary by CodeRabbit
New Features
IssueVersionentity to enhance issue management functionality.Bug Fixes
log_issue_versionmethod to ensure accurate timestamps and efficient processing of related entity IDs.Chores