migration: added webhook version, navigation related fields and allowed_rate_limit for APIToken #8339
Conversation
📝 WalkthroughWalkthroughAdds four new model fields via a Django migration and bulk-populates one JSON field: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/plane/db/migrations/0113_webhook_version.py(1 hunks)apps/api/plane/db/models/webhook.py(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/api/plane/db/models/webhook.py (1)
41-41:versionfield definition is valid and consistent with the existing model
version = models.CharField(default="v1", max_length=50)looks good: it satisfies Django’sCharFieldrequirements, provides a sensible default for existing rows, and doesn’t affect current uniqueness semantics on(workspace, url, deleted_at). If you expect only a small, fixed set of version values (e.g.,v1,v2), you could optionally addchoiceslater, but it’s not required for this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/plane/db/migrations/0113_webhook_version.py(1 hunks)apps/api/plane/db/models/webhook.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/plane/db/migrations/0113_webhook_version.py
⏰ 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). (1)
- GitHub Check: Analyze (javascript)
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on January 20. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/api/plane/db/migrations/0113_webhook_version.py (2)
7-14: Clarify the intentional difference from the model default.The migration's
get_default_feature_tours()returnsTruefor all features, while the model's version inworkspace.pyreturnsFalse. This appears intentional—existing users have implicitly "completed" the tours, while new users should see them. Consider adding a brief comment to document this distinction:def get_default_feature_tours(): + # Existing users: mark tours as completed (True) since they've been using the app + # Note: New records use the model default (False) so new users see the tours return { "work_items": True, "cycles": True, "modules": True, "intake": True, "pages": True, }
17-21: Consider batching for large datasets.While
bulk_updateis generally efficient, a single.update()call on a very large table could cause lock contention or timeout issues. If theWorkspaceUserPropertiestable could have millions of rows, consider batching:🔎 Optional: Batched update for large tables
def populate_feature_tours(apps, _schema_editor): WorkspaceUserProperties = apps.get_model('db', 'WorkspaceUserProperties') default_value = get_default_feature_tours() - # Use bulk update for better performance - WorkspaceUserProperties.objects.all().update(feature_tours=default_value) + # Batch update to avoid lock contention on large tables + batch_size = 10000 + while True: + ids = list( + WorkspaceUserProperties.objects.filter(feature_tours__isnull=True) + .values_list('id', flat=True)[:batch_size] + ) + if not ids: + break + WorkspaceUserProperties.objects.filter(id__in=ids).update(feature_tours=default_value)For most deployments this is likely fine as-is, but worth considering if you expect a large number of records.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/plane/db/migrations/0113_webhook_version.pyapps/api/plane/db/models/user.pyapps/api/plane/db/models/workspace.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T07:29:36.083Z
Learnt from: dheeru0198
Repo: makeplane/plane PR: 7625
File: apps/api/plane/bgtasks/workspace_seed_task.py:95-98
Timestamp: 2025-09-12T07:29:36.083Z
Learning: In the Plane codebase, workspace seed data should have project features (cycle_view, module_view, issue_views_view) enabled by default, even when regular project creation has these features disabled. This provides users with a complete demo experience in the seeded workspace.
Applied to files:
apps/api/plane/db/models/workspace.pyapps/api/plane/db/migrations/0113_webhook_version.py
🧬 Code graph analysis (2)
apps/api/plane/db/models/workspace.py (1)
apps/api/plane/utils/exporters/schemas/base.py (1)
JSONField(127-140)
apps/api/plane/db/migrations/0113_webhook_version.py (2)
apps/api/plane/db/models/workspace.py (1)
WorkspaceUserProperties(313-355)apps/api/plane/utils/exporters/schemas/base.py (2)
BooleanField(97-105)JSONField(127-140)
⏰ 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). (1)
- GitHub Check: Build packages
🔇 Additional comments (4)
apps/api/plane/db/models/user.py (1)
221-223: LGTM!The new
is_navigation_tour_completedfield is well-placed and follows the existing pattern for tour-related fields in the Profile model (e.g.,is_tour_completedon line 197). The default value ofFalseis appropriate for a new feature flag.apps/api/plane/db/models/workspace.py (2)
115-123: LGTM!The
get_default_feature_tours()function follows the established pattern of other default generators in this file (e.g.,get_default_props,get_default_filters). ReturningFalsefor all features is appropriate for new records, as users should see the feature tours.
338-338: LGTM!The
feature_toursfield correctly uses a callable default, following Django best practices and the existing pattern in this model for other JSON fields.apps/api/plane/db/migrations/0113_webhook_version.py (1)
30-46: LGTM!The migration operations are well-structured:
- Field additions are correctly defined with appropriate defaults
- The
max_length=50was added to theCharField(addressing the past review)RunPython.noopis acceptable for the reverse since the field has a default value
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/plane/db/migrations/0113_webhook_version.pyapps/api/plane/db/models/api.py
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/plane/db/migrations/0113_webhook_version.py
⏰ 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: check:lint
- GitHub Check: check:types
- GitHub Check: Analyze (javascript)
sangeethailango
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/plane/db/migrations/0113_webhook_version.py (1)
31-50: Field definitions look good, minor indentation inconsistency.All field additions are correctly defined:
webhook.version: CharField with appropriate max_length (past issue resolved)profile.is_navigation_tour_completed: Standard BooleanFieldworkspaceuserproperties.product_tour: Correctly references model's default functionapitoken.allowed_rate_limit: Valid format per DRF standardsMinor note: Line 46 has an extra leading space before
migrations.AddField, making indentation inconsistent with lines 31, 36, and 41.Based on learnings, the "60/min" format is valid for Django REST Framework.
🔎 Optional indentation fix
- ), - migrations.AddField( + ), + migrations.AddField( model_name='apitoken',
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/plane/db/migrations/0113_webhook_version.pyapps/api/plane/db/models/user.pyapps/api/plane/db/models/workspace.py
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/plane/db/models/user.py
- apps/api/plane/db/models/workspace.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-23T14:18:32.899Z
Learnt from: dheeru0198
Repo: makeplane/plane PR: 8339
File: apps/api/plane/db/models/api.py:35-35
Timestamp: 2025-12-23T14:18:32.899Z
Learning: Django REST Framework rate limit strings are flexible: only the first character of the time unit matters. Acceptable formats include: "60/s", "60/sec", "60/second" (all equivalent), "60/m", "60/min", "60/minute" (all equivalent), "60/h", "60/hr", "60/hour" (all equivalent), and "60/d", "60/day" (all equivalent). Abbreviations like "min" are valid and do not need to be changed to "minute". Apply this guidance to any Python files in the project that configure DRF throttling rules.
Applied to files:
apps/api/plane/db/migrations/0113_webhook_version.py
🧬 Code graph analysis (1)
apps/api/plane/db/migrations/0113_webhook_version.py (1)
apps/api/plane/db/models/workspace.py (2)
get_default_product_tour(115-122)WorkspaceUserProperties(313-355)
⏰ 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). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/api/plane/db/migrations/0113_webhook_version.py (2)
17-21: Implementation is sound, but depends on incorrect defaults.The bulk update approach is appropriate for performance. However, this function depends on
get_default_product_tour()which has inconsistent values compared to the model definition (see comment on lines 7-14).
51-51: RunPython operation is correctly positioned.The operation runs after the field is added, and using
noopfor reverse is appropriate since rolling back the migration would remove the field entirely.
| def get_default_product_tour(): | ||
| return { | ||
| "work_items": True, | ||
| "cycles": True, | ||
| "modules": True, | ||
| "intake": True, | ||
| "pages": True, | ||
| } |
There was a problem hiding this comment.
Critical: Inconsistent default values with model definition.
The migration's get_default_product_tour() returns all True values, but the model's version in apps/api/plane/db/models/workspace.py:114-121 returns all False values. This creates problematic behavior:
- Existing records updated by this migration will have all tours set to
True - New records created after migration will use the model's default (all
False)
This inconsistency means existing users will appear to have completed all tours, while new users won't. Please verify which default is intended and align both functions.
🔎 Proposed fix to align with model defaults
def get_default_product_tour():
return {
- "work_items": True,
- "cycles": True,
- "modules": True,
- "intake": True,
- "pages": True,
+ "work_items": False,
+ "cycles": False,
+ "modules": False,
+ "intake": False,
+ "pages": False,
}📝 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_default_product_tour(): | |
| return { | |
| "work_items": True, | |
| "cycles": True, | |
| "modules": True, | |
| "intake": True, | |
| "pages": True, | |
| } | |
| def get_default_product_tour(): | |
| return { | |
| "work_items": False, | |
| "cycles": False, | |
| "modules": False, | |
| "intake": False, | |
| "pages": False, | |
| } |
🤖 Prompt for AI Agents
In apps/api/plane/db/migrations/0113_webhook_version.py around lines 7 to 14,
the get_default_product_tour() currently returns all True but the model in
apps/api/plane/db/models/workspace.py lines 114-121 uses all False; update the
migration to match the model defaults (set all keys to False) or, if the
intended default is True, update the model instead — choose the correct intended
default, make both functions identical, and run tests/migrations to ensure
consistency.
Description
This pull request contains model changes :
versionfield has been added in the webhook,feature_toursin WorkspaceUserPropertiesis_navigation_tour_completedin the Profile modelallowed_rate_limitin APITokenSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.