chore: webhook, comments migration #6523
Conversation
WalkthroughThis pull request introduces several database model modifications across multiple files in the Plane application. The changes primarily involve updating field types and constraints in models related to webhooks, user profiles, issue comments, and recent visits. Key modifications include changing the Changes
Possibly related PRs
Suggested Reviewers
Poem
✨ Finishing Touches
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
🧹 Nitpick comments (2)
apiserver/plane/db/migrations/0091_issuecomment_edited_at_and_more.py (1)
13-22: Determine the best usage for new fields.
edited_at: Consider settingauto_now=Trueif you want to automatically track the latest edit time.is_smooth_cursor_enabled: Defaulting toFalseis reasonable, but verify if environment-based toggling is required.apiserver/plane/db/models/issue.py (1)
470-470: LGTM! Consider auto-updating edited_at in the save method.The
edited_atfield is properly added to track comment edits.Consider updating the
edited_atfield automatically in the save method when the comment content changes:def save(self, *args, **kwargs): self.comment_stripped = ( strip_tags(self.comment_html) if self.comment_html != "" else "" ) + if not self._state.adding and ( + self.tracker.has_changed('comment_html') or + self.tracker.has_changed('comment_json') + ): + self.edited_at = timezone.now() return super(IssueComment, self).save(*args, **kwargs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apiserver/plane/app/views/webhook/base.py(1 hunks)apiserver/plane/bgtasks/webhook_task.py(4 hunks)apiserver/plane/db/migrations/0091_issuecomment_edited_at_and_more.py(1 hunks)apiserver/plane/db/models/issue.py(1 hunks)apiserver/plane/db/models/recent_visit.py(1 hunks)apiserver/plane/db/models/user.py(2 hunks)apiserver/plane/db/models/webhook.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (12)
apiserver/plane/db/models/recent_visit.py (1)
20-20: Verify if removing choices is intentional.By removing
choicesand allowing any string inentity_name, you risk inconsistent or unvalidated data. If broader entity names are needed, consider adding a custom validator or a new enumeration to maintain consistency.apiserver/plane/db/migrations/0091_issuecomment_edited_at_and_more.py (7)
1-2: Auto-generated migration header.No further action needed; this is a standard Django migration boilerplate.
3-4: Import statements.All necessary Django modules are properly imported.
6-7: Migration class definition.Straightforward Django migration class with no issues.
8-10: Dependency declaration.Dependencies are correctly listed to maintain proper migration order.
23-27: Consistency with model changes forentity_name.Matches the removal of
choicesinUserRecentVisit. Make sure the broader input range is intentional.
28-32: WebhookLog field alteration already noted.This confirms the change from
ForeignKeytoUUIDField. See previous comment about referencingself.webhook.url.
33-37: New unique constraint foraccount.Enforcing uniqueness on
('user', 'provider', 'provider_account_id')strengthens data integrity. Looks good.apiserver/plane/app/views/webhook/base.py (1)
123-123: LGTM! Field name aligned with model changes.The filter condition now correctly uses
webhookinstead ofwebhook_id, consistent with the WebhookLog model changes.Let's verify the model relationship:
✅ Verification successful
Field name alignment verified across model, view, and serializer
The
webhookfield is consistently defined as a UUIDField in the model and correctly used in the filter conditions, with proper read-only configuration in the serializer.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the WebhookLog model field definition ast-grep --pattern 'class WebhookLog { $$$ webhook = models.$_($$$) $$$ }'Length of output: 82
Script:
#!/bin/bash # Find the WebhookLog model definition rg "class WebhookLog" -A 10Length of output: 2015
apiserver/plane/db/models/user.py (2)
189-190: LGTM! New feature flag added.The
is_smooth_cursor_enabledboolean field is properly added with a default value of False.
229-229: Verify data integrity before applying the unique constraint.The unique constraint has been strengthened by including the 'user' field. While this improves data integrity by ensuring one account per user per provider, it could cause migration issues if duplicate records exist.
Run this script to check for potential conflicts:
✅ Verification successful
Constraint change is safe and improves data integrity
The unique constraint enhancement is safe and beneficial because:
- Social auth providers ensure account IDs are unique per user
- The constraint prevents security issues by ensuring one provider account can't be linked to multiple users
- The change is a logical strengthening of the existing constraint
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for duplicate provider accounts that would violate the new constraint # Use Python to analyze the data python3 << 'EOF' from django.db import connection with connection.cursor() as cursor: cursor.execute(""" SELECT user_id, provider, provider_account_id, COUNT(*) FROM accounts WHERE deleted_at IS NULL GROUP BY user_id, provider, provider_account_id HAVING COUNT(*) > 1 """) duplicates = cursor.fetchall() if duplicates: print("WARNING: Found duplicate records that would violate the new constraint:") for dup in duplicates: print(f"user_id: {dup[0]}, provider: {dup[1]}, provider_account_id: {dup[2]}, count: {dup[3]}") else: print("No duplicate records found. Safe to apply the constraint.") EOFLength of output: 128
Script:
#!/bin/bash # Check the model history and migrations echo "=== Checking model definition ===" rg -A 5 -B 5 "class Meta:" apiserver/plane/db/models/user.py echo -e "\n=== Checking migrations ===" fd -e py . apiserver/plane/db/migrations/ -x cat {} | rg -A 5 -B 5 "unique_together"Length of output: 38007
Script:
#!/bin/bash # Check for data migration code and related changes echo "=== Checking for migration handling code ===" rg -B 5 -A 5 "migrate.*account" apiserver/plane/db/migrations/ rg -l "provider.*account.*id" apiserver/plane/db/ echo -e "\n=== Checking Account model implementation ===" rg -B 10 -A 10 "class Account" apiserver/plane/db/models/Length of output: 1798
apiserver/plane/bgtasks/webhook_task.py (1)
139-139: LGTM! Consistent field name updates across webhook logging.The field name has been consistently updated from
webhook_idtowebhookin all WebhookLog creation calls, aligning with the model changes.Also applies to: 156-156, 307-307, 322-322
| @@ -66,7 +66,7 @@ class WebhookLog(BaseModel): | |||
| "db.Workspace", on_delete=models.CASCADE, related_name="webhook_logs" | |||
| ) | |||
| # Associated webhook | |||
| webhook = models.ForeignKey(Webhook, on_delete=models.CASCADE, related_name="logs") | |||
| webhook = models.UUIDField() | |||
There was a problem hiding this comment.
Fix broken reference in the __str__ method.
Switching webhook from a ForeignKey to a UUIDField invalidates calls to self.webhook.url. This will throw an error since self.webhook is now a UUID, not an object. Update the __str__ method to reference just the UUID or store the URL separately if needed.
- return f"{self.event_type} {str(self.webhook.url)}"
+ return f"{self.event_type} {str(self.webhook)}"Committable suggestion skipped: line range outside the PR's diff.
Description
this pull request contains new field changes for webhook, comment, profile and user visits.
Type of Change
Summary by CodeRabbit
Release Notes
New Features
edited_attimestamp for issue commentsis_smooth_cursor_enableduser profile settingDatabase Changes
Improvements