feat: workspace home preference model#6300
Conversation
WalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Workspace
participant WorkspaceHomePreference
User->>Workspace: Select Workspace
Workspace->>WorkspaceHomePreference: Retrieve User Preferences
WorkspaceHomePreference-->>Workspace: Return Configured Widgets
Workspace->>User: Display Personalized Home View
Possibly Related PRs
Suggested Reviewers
Poem
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: 0
🧹 Nitpick comments (4)
apiserver/plane/db/migrations/0089_workspacehomepreference_and_more.py (2)
38-45: Redundant constraints andunique_together:
You’re defining both a partialUniqueConstraintand aunique_togetherthat also includesdeleted_at. Although this is a valid approach, it might be worth confirming that this duplication is necessary. Typically, many teams prefer a single partial index with the condition to handle the uniqueness while ignoring deleted records.🧰 Tools
🪛 Ruff (0.8.2)
40-40: Line too long (221 > 88)
(E501)
19-29: Line length violations reported by Ruff (E501).
Be mindful that some lines exceed the recommended maximum length of 88 characters (as flagged by Ruff). While these style warnings don’t affect functionality, consider line-wrapping or reformatting to maintain consistent code style.Also applies to: 40-40
🧰 Tools
🪛 Ruff (0.8.2)
19-19: Line too long (99 > 88)
(E501)
20-20: Line too long (101 > 88)
(E501)
21-21: Line too long (103 > 88)
(E501)
22-22: Line too long (140 > 88)
(E501)
26-26: Line too long (200 > 88)
(E501)
27-27: Line too long (206 > 88)
(E501)
28-28: Line too long (166 > 88)
(E501)
29-29: Line too long (161 > 88)
(E501)
apiserver/plane/db/models/workspace.py (2)
369-377: Confirm overlap between unique constraints and the partial constraint.
As in the migration file, consider if you really need bothunique_togetherwithdeleted_atand a partial constraint ignoringdeleted_at. One partial unique constraint might suffice.
347-384: Line length violations (E501).
Some lines within this segment slightly exceed the style guidelines on max line length. If you wish to maintain compliance with Ruff or PEP 8, consider splitting longer lines or using multiline formatting for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apiserver/plane/db/migrations/0089_workspacehomepreference_and_more.py(1 hunks)apiserver/plane/db/models/workspace.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
apiserver/plane/db/migrations/0089_workspacehomepreference_and_more.py
19-19: Line too long (99 > 88)
(E501)
20-20: Line too long (101 > 88)
(E501)
21-21: Line too long (103 > 88)
(E501)
22-22: Line too long (140 > 88)
(E501)
26-26: Line too long (200 > 88)
(E501)
27-27: Line too long (206 > 88)
(E501)
28-28: Line too long (166 > 88)
(E501)
29-29: Line too long (161 > 88)
(E501)
40-40: Line too long (221 > 88)
(E501)
🔇 Additional comments (3)
apiserver/plane/db/migrations/0089_workspacehomepreference_and_more.py (1)
16-37: Migration logic is correct and aligns with Django best practices.
The creation of theWorkspaceHomePreferencemodel with well-defined fields, timestamps, and soft-deletion support is consistent. Adding a partial unique constraint withdeleted_at__isnull=Truehelps enforce uniqueness only for active records, which is highly recommended to preserve historical data.🧰 Tools
🪛 Ruff (0.8.2)
19-19: Line too long (99 > 88)
(E501)
20-20: Line too long (101 > 88)
(E501)
21-21: Line too long (103 > 88)
(E501)
22-22: Line too long (140 > 88)
(E501)
26-26: Line too long (200 > 88)
(E501)
27-27: Line too long (206 > 88)
(E501)
28-28: Line too long (166 > 88)
(E501)
29-29: Line too long (161 > 88)
(E501)
apiserver/plane/db/models/workspace.py (2)
347-367: Appropriate usage of TextChoices and JSONField.
DefiningHomeWidgetKeysas aTextChoicesenum is a clean way to maintain clarity for different widget keys. Using aJSONFieldforconfigis flexible for storing additional preference data. This approach is consistent with Django recommendations.
383-384: Clear and concise__str__method.
Your string representation is straightforward, which can be helpful for debugging logs and the Django admin.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apiserver/plane/db/migrations/0089_workspacehomepreference_and_more.py(1 hunks)apiserver/plane/db/models/page.py(1 hunks)
🔇 Additional comments (3)
apiserver/plane/db/models/page.py (1)
23-23: Use caution when changing field types from CharField to TextField.
If there's any logic or validation relying on themax_lengthproperty, you should ensure it still behaves as intended. Additionally, consider potential performance implications for indexes in larger fields if you plan to query thenamefield frequently.Would you like to inspect other references or constraints on the
namefield across the codebase?apiserver/plane/db/migrations/0089_workspacehomepreference_and_more.py (2)
16-92: New model addition looks good, but confirm usage aligns with requirements.
TheWorkspaceHomePreferencemodel and fields appear appropriate for storing user preferences. Ensure that the logic for reading and writing these preferences is integrated well, and that any sensitive data handled in theconfigJSONField is managed securely.
93-104: Check indexing and overlaps in unique constraints.
You’ve added a unique constraint with a condition ondeleted_atand also declaredunique_togetherwith("workspace", "user", "key", "deleted_at"). Verify the interaction between these constraints to avoid conflicts or unexpected database errors.Do you want me to generate a shell script to look for references to the
WorkspaceHomePreferencetable creation or usage to confirm no potential overlap issues with these constraints?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apiserver/plane/db/migrations/0089_workspacehomepreference_and_more.py (3)
46-48: Consider adding choices for thekeyfieldThe
keyfield appears to store preference types but lacks constraints on possible values. Consider adding choices to ensure data consistency.-"key", models.CharField(max_length=255)), +"key", models.CharField( + max_length=255, + choices=[ + ('dashboard', 'Dashboard'), + ('issues', 'Issues'), + # Add other valid preference keys + ] +)),
48-48: Add validation for theconfigJSONFieldThe
configfield uses a default empty dict but lacks schema validation. Consider adding JSON schema validation to ensure the stored configuration is valid.-"config", models.JSONField(default=dict)), +"config", models.JSONField( + default=dict, + validators=[validate_config_schema] # Add a validator function +)),
110-114: Consider index for sort_order fieldSince the sort_order field will likely be used for ordering queries, consider adding a database index to improve query performance.
- field=models.PositiveIntegerField(default=65535), + field=models.PositiveIntegerField(default=65535, db_index=True),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apiserver/plane/db/migrations/0089_workspacehomepreference_and_more.py(1 hunks)apiserver/plane/db/migrations/0090_workspacehomepreference_sort_order.py(1 hunks)apiserver/plane/db/models/workspace.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apiserver/plane/db/models/workspace.py
🧰 Additional context used
🪛 Ruff (0.8.2)
apiserver/plane/db/migrations/0090_workspacehomepreference_sort_order.py
3-3: django.db.models imported but unused
Remove unused import: django.db.models
(F401)
🔇 Additional comments (1)
apiserver/plane/db/migrations/0089_workspacehomepreference_and_more.py (1)
105-109: Verify data migration for Page.name fieldChanging from CharField to TextField might require data migration if there's existing data with specific formatting or if the old field had a max_length constraint.
Let's verify the existing data constraints:
#!/bin/bash # Check for existing Page model definition and any max_length constraints ast-grep --pattern 'class Page($$_): $$$ name = models.CharField($$$) $$$'
apiserver/plane/db/migrations/0090_workspacehomepreference_sort_order.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apiserver/plane/db/models/sticky.py (1)
10-10: Handlenull=Trueresponsibly.Allowing
null=Trueon aTextFieldmeansnamecould beNonein Python. Ensure that any references or string operations onself.namehandleNonewithout raising exceptions. If you intended to allow an empty string but not aNonevalue, useblank=Truewithoutnull=True.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apiserver/plane/db/migrations/0089_workspacehomepreference_and_more.py(1 hunks)apiserver/plane/db/models/sticky.py(1 hunks)
🔇 Additional comments (4)
apiserver/plane/db/migrations/0089_workspacehomepreference_and_more.py (4)
16-92: Validate the growing JSON structure inconfig.Since
configdefaults to an unbounded dictionary, confirm that you have sufficient validation and size checks in place to avoid inadvertently storing huge or malformed payloads that could degrade performance.
93-104: Examine potential redundancy in existing uniqueness constraints.You have both a conditional UniqueConstraint (line 93) and a
unique_together(line 101) on the same fields. This might be redundant, as previously noted. Consider removing one of them to keep your model constraints simpler and avoid confusion.
105-109: Confirm the migration from CharField to TextField.Switching from
CharFieldtoTextFieldrelaxes any length constraints. If the old schema enforced strict length limits, verify that no existing data or future usage relies on a maximum length.
110-114: Consistently manage nullable fields forSticky.Mirrors the change in
sticky.py: thenamefield is now nullable. Double-check any logic or templates that depend onnamebeing always present to handleNonevalues gracefully.
|
@pablohashescobar revert back page.py changes and regenerate the migration file. |
Not required. |
3 participants
Description
Created model and migration for storing home preferences.
Type of Change
Summary by CodeRabbit
New Features
Database Changes
PageandStickymodels to allow more flexible name field configurationsImprovements