[WEB-2631] chore: changed the cascading logic for soft delete#5823
[WEB-2631] chore: changed the cascading logic for soft delete#5823NarayanBavisetti wants to merge 0 commit intopreviewfrom
Conversation
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested labels
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
🧹 Outside diff range and nitpick comments (1)
apiserver/plane/bgtasks/deletion_task.py (1)
24-25: Review cascading behavior and consider implementingrestore_related_objectsThe changes to
soft_delete_related_objectsmay have broader implications:
The modified condition for soft deletion might affect the cascading behavior. Ensure this aligns with the intended functionality across the application.
The
restore_related_objectsfunction is still not implemented. This could lead to inconsistencies between deletion and restoration processes.Consider the following improvements:
- Implement the
restore_related_objectsfunction to ensure consistency between deletion and restoration processes.- Add unit tests to verify the new soft deletion behavior, especially for edge cases where
on_deleteis not defined.- Update documentation to reflect the changes in cascading behavior during soft deletions.
Would you like assistance in implementing the
restore_related_objectsfunction or creating unit tests for the new soft deletion behavior?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apiserver/plane/bgtasks/deletion_task.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
apiserver/plane/bgtasks/deletion_task.py (1)
24-25:⚠️ Potential issueConsider using Django's
models.CASCADEinstead of string comparisonThe change in the conditional check for the
on_deleteattribute introduces a potential issue:
- The use of the string "CASCADE" instead of
models.CASCADEmight lead to inconsistencies if Django's internals change in the future.- The logic now includes cases where
on_deletedoesn't exist, which might unintentionally broaden the scope of soft deletion.Consider refactoring the condition to use Django's
models.CASCADE:from django.db import models # ... if ( not hasattr(field.remote_field, "on_delete") or field.remote_field.on_delete == models.CASCADE ): # ... rest of the codeThis change ensures consistency with Django's API and makes the code more robust against potential future changes in Django.
To ensure this change doesn't introduce unintended side effects, please run the following script to check for any other occurrences of string-based CASCADE comparisons:
✅ Verification successful
[/run_scripts]
Refactor to Use Django's
models.CASCADEThe review comment is valid. Only the identified line in
apiserver/plane/bgtasks/deletion_task.pyuses a string comparison foron_delete. To maintain consistency and robustness, update this line to use Django'smodels.CASCADE:from django.db import models # ... if ( not hasattr(field.remote_field, "on_delete") or field.remote_field.on_delete == models.CASCADE ): # ... rest of the code🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of string-based CASCADE comparisons rg --type python '"CASCADE"' apiserver/Length of output: 72
Script:
#!/bin/bash # Search for other occurrences of string-based CASCADE comparisons using the correct type rg --type py '"CASCADE"' apiserver/Length of output: 143
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
apiserver/plane/db/models/deploy_board.py (2)
23-23: LGTM: New "inbox" choice added to TYPE_CHOICESThe addition of "inbox" to
TYPE_CHOICESis consistent with the existing structure and naming convention. This change expands the options for theentity_namefield, potentially supporting new inbox-related functionality.Consider updating any relevant documentation or comments to reflect this new option and its intended use case.
Line range hint
1-63: Overall assessment: Changes align with PR objectivesThe modifications to the
DeployBoardmodel, including the addition of the "inbox" choice and theis_disabledfield, are consistent with the PR objectives of changing the cascading logic for soft delete. These changes appear to be part of a larger feature implementation or system update.Consider the following architectural points:
- Ensure that the new "inbox" type and the
is_disabledfield are properly handled in all relevant parts of the system, including views, serializers, and any caching mechanisms.- If the
is_disabledfield affects the soft delete functionality, make sure it's consistently used across related models and operations.- Update any documentation, API specifications, and test cases to reflect these new changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apiserver/plane/db/migrations/0081_remove_usernotificationpreference_comment_and_more.py (1 hunks)
- apiserver/plane/db/models/deploy_board.py (2 hunks)
- apiserver/plane/db/models/notification.py (1 hunks)
- apiserver/plane/db/models/user.py (0 hunks)
💤 Files with no reviewable changes (1)
- apiserver/plane/db/models/user.py
🧰 Additional context used
🔇 Additional comments (6)
apiserver/plane/db/models/deploy_board.py (1)
45-45: LGTM: Newis_disabledfield added to DeployBoard modelThe addition of the
is_disabledfield with a default value ofFalseis a good way to implement a disable/enable feature for deploy boards. This change allows for more flexible management of deploy boards.Please ensure the following:
- A corresponding migration file has been created for this model change.
- Any existing queries or business logic that interact with deploy boards have been updated to consider this new field.
- The UI and API have been updated to support this new field if necessary.
To verify the impact on existing code, you can run:
apiserver/plane/db/models/notification.py (2)
106-116:⚠️ Potential issueVerify the impact of changing default values for in-app notification preferences
Similar to the email preferences, the in-app notification preference fields are also initialized with
default=False. If in-app notifications were previously enabled by default, this change might disable them for existing users, potentially affecting their in-app experience.Ensure that this change is intentional and consider communicating it to users if necessary.
Run the following script to check the default values of previous in-app notification settings:
#!/bin/bash # Description: Verify previous default values for in-app notification preferences # Since in-app notification fields are new, confirm if they replace any old fields with different defaults rg --type py 'class UserNotificationPreference' -A 50 -- 'models/notification.py'This script helps to determine if the in-app notification preferences are replacing any existing fields and what their default values were.
93-103:⚠️ Potential issueVerify the impact of changing default values for email notification preferences
The new email notification preference fields are initialized with
default=False, whereas the previous fields haddefault=True. This change means that existing users who previously received email notifications will no longer receive them unless they manually update their preferences. This could lead to unintended disruptions in the user experience.Please verify if setting these defaults to
Falseis intentional and aligns with product requirements.Run the following script to identify the default values of the previous notification preference fields:
This script searches for instances where the old email notification preference fields were defined with
default=Trueto confirm the previous default settings.apiserver/plane/db/migrations/0081_remove_usernotificationpreference_comment_and_more.py (3)
63-67: Approved: Addition of 'is_disabled' field to 'deployboard' modelThe addition of the
is_disabledBoolean field with a default value ofFalseto thedeployboardmodel is appropriate and follows best practices.
68-97: Approved: Addition of new notification preference fieldsThe introduction of
email_*andin_app_*Boolean fields to theusernotificationpreferencemodel enhances the granularity of user notification settings. The fields are correctly implemented with default values ofFalse.Also applies to: 99-152
153-168:⚠️ Potential issueVerify that existing 'entity_name' values conform to new choices
Adding choices to the
entity_namefield in thedeployboardmodel restricts it to specific values. If existing records have values outside these choices, it could lead toIntegrityErrorexceptions when saving or accessing these records.Recommendation:
Ensure that all existing
entity_namevalues in the database match one of the new choices. Update any records with invalid values before applying the migration.Verification Script:
Action Required:
- Run the script to identify any hardcoded invalid
entity_nameassignments in the codebase.- Review and update any code or database records that do not conform to the new choices.
| # email preference fields | ||
| email_property_change = models.BooleanField(default=False) | ||
| email_state_change = models.BooleanField(default=False) | ||
| email_priority_change = models.BooleanField(default=False) | ||
| email_assignee_change = models.BooleanField(default=False) | ||
| email_start_target_date_change = models.BooleanField(default=False) | ||
| email_module_change = models.BooleanField(default=False) | ||
| email_cycle_change = models.BooleanField(default=False) | ||
| email_reactions = models.BooleanField(default=False) | ||
| email_comment = models.BooleanField(default=False) | ||
| email_mention = models.BooleanField(default=False) | ||
| email_issue_completed = models.BooleanField(default=False) | ||
|
|
||
| # in app preference fields | ||
| in_app_property_change = models.BooleanField(default=False) | ||
| in_app_state_change = models.BooleanField(default=False) | ||
| in_app_priority_change = models.BooleanField(default=False) | ||
| in_app_assignee_change = models.BooleanField(default=False) | ||
| in_app_start_target_date_change = models.BooleanField(default=False) | ||
| in_app_module_change = models.BooleanField(default=False) | ||
| in_app_cycle_change = models.BooleanField(default=False) | ||
| in_app_reactions = models.BooleanField(default=False) | ||
| in_app_comment = models.BooleanField(default=False) | ||
| in_app_mention = models.BooleanField(default=False) | ||
| in_app_issue_completed = models.BooleanField(default=False) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring notification preferences for scalability and maintainability
Adding numerous BooleanFields for each notification type leads to a bloated model and can make future extensions cumbersome. As the number of notification types grows, the model will become increasingly complex and harder to manage.
Consider the following refactoring options:
- Use a
JSONField: Store notification preferences in aJSONField, allowing for dynamic and flexible storage. This approach makes it easier to add new notification types without modifying the database schema. - Create a separate related model: Introduce a related model (e.g.,
NotificationPreference) with fields for the preference type and delivery method (email or in-app). Use a foreign key or many-to-many relationship to associate preferences with users.
Implementing one of these approaches can improve scalability, reduce code duplication, and enhance maintainability.
| migrations.RenameField( | ||
| model_name="usernotificationpreference", | ||
| old_name="property_change", | ||
| new_name="email_property_change", | ||
| ), | ||
| migrations.AlterField( | ||
| model_name="usernotificationpreference", | ||
| name="email_property_change", | ||
| field=models.BooleanField(default=False), | ||
| ), |
There was a problem hiding this comment.
Caution: Renaming and altering fields simultaneously may cause issues
Renaming a field and altering its type in the same migration can lead to problems on certain database backends, such as PostgreSQL. This is because the database might not correctly track the field's identity through both operations in a single migration.
Recommendation:
Consider splitting the RenameField and AlterField operations into separate migrations for each field. This ensures that the database correctly handles the field renaming before changing its type.
- First Migration: Perform the
RenameFieldoperation. - Second Migration: Apply the
AlterFieldto change the field type.
This approach minimizes the risk of data loss or migration errors.
Also applies to: 23-32, 33-42, 43-52, 53-62
NarayanBavisetti
force-pushed
the
chore/soft-deletion-cascading
branch
from
a5dc0c5 to
1f5cbe9
Compare
NarayanBavisetti
force-pushed
the
chore/soft-deletion-cascading
branch
from
1f5cbe9 to
cf53cdf
Compare
1 participant
chore:
Issue Link: WEB-2631
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
on_deleteattribute.