Skip to content

[WEB-2600] fix: estimate point deletion #5762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pushya22 merged 2 commits into from
Oct 7, 2024
Merged

[WEB-2600] fix: estimate point deletion #5762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85e813c
chore: only delete the cascade fields
NarayanBavisetti Oct 7, 2024
a65b03b
chore: logged the issue activity
NarayanBavisetti Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 62 additions & 2 deletions apiserver/plane/app/views/estimate/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import random
import string
import json

# Django imports
from django.utils import timezone

# Third party imports
from rest_framework.response import Response
Expand All @@ -19,6 +23,7 @@
EstimateReadSerializer,
)
from plane.utils.cache import invalidate_cache
from plane.bgtasks.issue_activities_task import issue_activity


def generate_random_name(length=10):
Expand Down Expand Up @@ -249,11 +254,66 @@ def destroy(
)
# update all the issues with the new estimate
if new_estimate_id:
_ = Issue.objects.filter(
issues = Issue.objects.filter(
project_id=project_id,
workspace__slug=slug,
estimate_point_id=estimate_point_id,
)
Comment on lines +257 to +261
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure Issues are Updated to Remove Deleted Estimate Point

In the else block where new_estimate_id is not provided, the issues are not updated to remove the reference to the deleted estimate_point_id. This means the issues will still reference an estimate point that no longer exists, which could lead to integrity issues.

To fix this, add an issues.update(estimate_point_id=None) after the loop in the else block to set the estimate_point_id to None for all affected issues:

issues.update(estimate_point_id=None)

This ensures that all issues previously associated with the deleted estimate point no longer hold invalid references.

for issue in issues:
issue_activity.delay(
type="issue.activity.updated",
requested_data=json.dumps(
{
"estimate_point": (
str(new_estimate_id)
if new_estimate_id
else None
),
}
),
actor_id=str(request.user.id),
issue_id=issue.id,
project_id=str(project_id),
current_instance=json.dumps(
{
"estimate_point": (
str(issue.estimate_point_id)
if issue.estimate_point_id
else None
),
}
),
epoch=int(timezone.now().timestamp()),
)
issues.update(estimate_point_id=new_estimate_id)
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consistent Issue Updates

Ensure that the issues.update(...) method is called in both the if and else blocks and placed outside the loop. In the current code, the update in the else block is missing.

Add the following after line 316:

issues.update(estimate_point_id=None)

This ensures that all issues have their estimate_point_id set to None when no new_estimate_id is provided.

Also applies to: 317-317

else:
issues = Issue.objects.filter(
project_id=project_id,
workspace__slug=slug,
estimate_point_id=estimate_point_id,
).update(estimate_point_id=new_estimate_id)
)
for issue in issues:
issue_activity.delay(
type="issue.activity.updated",
requested_data=json.dumps(
{
"estimate_point": None,
}
),
actor_id=str(request.user.id),
issue_id=issue.id,
project_id=str(project_id),
current_instance=json.dumps(
{
"estimate_point": (
str(issue.estimate_point_id)
if issue.estimate_point_id
else None
),
}
),
epoch=int(timezone.now().timestamp()),
)
Comment on lines +257 to +316
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor Repetitive Code in destroy Method

The code inside the if new_estimate_id and else blocks shares significant similarities, particularly in how issues are retrieved and how issue_activity.delay is called. This duplication can make the code harder to maintain and more prone to errors.

You can refactor the code to eliminate duplication:

issues = Issue.objects.filter(
    project_id=project_id,
    workspace__slug=slug,
    estimate_point_id=estimate_point_id,
)

new_estimate_point = str(new_estimate_id) if new_estimate_id else None

for issue in issues:
    issue_activity.delay(
        type="issue.activity.updated",
        requested_data=json.dumps({"estimate_point": new_estimate_point}),
        actor_id=str(request.user.id),
        issue_id=issue.id,
        project_id=str(project_id),
        current_instance=json.dumps(
            {
                "estimate_point": (
                    str(issue.estimate_point_id)
                    if issue.estimate_point_id
                    else None
                )
            }
        ),
        epoch=int(timezone.now().timestamp()),
    )

if new_estimate_id:
    issues.update(estimate_point_id=new_estimate_id)
else:
    issues.update(estimate_point_id=None)

This refactoring reduces code duplication and makes the logic clearer.


# delete the estimate point
old_estimate_point = EstimatePoint.objects.filter(
Expand Down
2 changes: 1 addition & 1 deletion apiserver/plane/app/views/project/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ def create(self, request, slug):
status=status.HTTP_410_GONE,
)

@allow_permission([ROLE.ADMIN, ROLE.MEMBER], level="WORKSPACE")
@allow_permission([ROLE.ADMIN])
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Restricted access to ADMIN role only

The partial_update method now only allows ADMIN role access. This change enhances security but may impact existing workflows where MEMBER roles previously had update permissions.

Please ensure this change aligns with your project's access control requirements and update any related documentation or user guides accordingly.

def partial_update(self, request, slug, pk=None):
try:
workspace = Workspace.objects.get(slug=slug)
Expand Down
34 changes: 21 additions & 13 deletions apiserver/plane/bgtasks/deletion_task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from django.utils import timezone
from django.apps import apps
from django.conf import settings
from django.db import models
from django.core.exceptions import ObjectDoesNotExist

# Third party imports
Expand All @@ -18,17 +19,25 @@ def soft_delete_related_objects(
for field in related_fields:
if field.one_to_many or field.one_to_one:
try:
if field.one_to_many:
related_objects = getattr(instance, field.name).all()
elif field.one_to_one:
related_object = getattr(instance, field.name)
related_objects = (
[related_object] if related_object is not None else []
)
for obj in related_objects:
if obj:
obj.deleted_at = timezone.now()
obj.save(using=using)
# Check if the field has CASCADE on delete
if (
hasattr(field.remote_field, "on_delete")
and field.remote_field.on_delete == models.CASCADE
):
if field.one_to_many:
related_objects = getattr(instance, field.name).all()
elif field.one_to_one:
related_object = getattr(instance, field.name)
related_objects = (
[related_object]
if related_object is not None
else []
)

for obj in related_objects:
if obj:
obj.deleted_at = timezone.now()
obj.save(using=using)
except ObjectDoesNotExist:
pass

Expand Down Expand Up @@ -154,8 +163,7 @@ def hard_delete():
if hasattr(model, "deleted_at"):
# Get all instances where 'deleted_at' is greater than 30 days ago
_ = model.all_objects.filter(
deleted_at__lt=timezone.now()
- timezone.timedelta(days=days)
deleted_at__lt=timezone.now() - timezone.timedelta(days=days)
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider Consolidating Hard Deletion Logic

In the hard_delete function, you individually delete instances of specific models and then iterate over all models with a deleted_at field to delete any remaining instances. This could lead to redundant deletion operations and impact performance.

Suggestion:

Evaluate whether the individual deletions of specific models are necessary if the final loop sufficiently handles the hard deletion of all models with a deleted_at field. Consolidating the deletion logic might improve performance and simplify maintenance.

).delete()

return
10 changes: 3 additions & 7 deletions apiserver/plane/bgtasks/issue_activities_task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ def track_estimate_points(
IssueActivity(
issue_id=issue_id,
actor_id=actor_id,
verb="updated",
verb="removed" if new_estimate is None else "updated",
old_identifier=(
current_instance.get("estimate_point")
if current_instance.get("estimate_point") is not None
Expand Down Expand Up @@ -1700,16 +1700,12 @@ def issue_activity(
event=(
"issue_comment"
if activity.field == "comment"
else "inbox_issue"
if inbox
else "issue"
else "inbox_issue" if inbox else "issue"
),
event_id=(
activity.issue_comment_id
if activity.field == "comment"
else inbox
if inbox
else activity.issue_id
else inbox if inbox else activity.issue_id
Comment on lines +1703 to +1708
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor nested ternary operators for improved readability

The nested ternary operators in the assignments of event and event_id reduce code readability and can make maintenance challenging. Consider refactoring the code to enhance clarity.

Apply this diff to improve readability:

             if len(issue_activities_created):
                 for activity in issue_activities_created:
+                    if activity.field == "comment":
+                        event = "issue_comment"
+                        event_id = activity.issue_comment_id
+                    elif inbox:
+                        event = "inbox_issue"
+                        event_id = inbox
+                    else:
+                        event = "issue"
+                        event_id = activity.issue_id
                     webhook_activity.delay(
-                        event=(
-                            "issue_comment"
-                            if activity.field == "comment"
-                            else "inbox_issue" if inbox else "issue"
-                        ),
-                        event_id=(
-                            activity.issue_comment_id
-                            if activity.field == "comment"
-                            else inbox if inbox else activity.issue_id
-                        ),
+                        event=event,
+                        event_id=event_id,
                         verb=activity.verb,
                         field=(
                             "description"
                             if activity.field == "comment"
                             else activity.field
                         ),
                         old_value=(
                             activity.old_value
                             if activity.old_value != ""
                             else None
                         ),
                         new_value=(
                             activity.new_value
                             if activity.new_value != ""
                             else None
                         ),
                         actor_id=activity.actor_id,
                         current_site=origin,
                         slug=activity.workspace.slug,
                         old_identifier=activity.old_identifier,
                         new_identifier=activity.new_identifier,
                     )

This refactoring assigns event and event_id before the webhook_activity.delay call, enhancing the readability and maintainability of the code.

📝 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.

Suggested change
else "inbox_issue" if inbox else "issue"
),
event_id=(
activity.issue_comment_id
if activity.field == "comment"
else inbox
if inbox
else activity.issue_id
else inbox if inbox else activity.issue_id
if activity.field == "comment":
event = "issue_comment"
event_id = activity.issue_comment_id
elif inbox:
event = "inbox_issue"
event_id = inbox
else:
event = "issue"
event_id = activity.issue_id
webhook_activity.delay(
event=event,
event_id=event_id,
verb=activity.verb,
field=(
"description"
if activity.field == "comment"
else activity.field
),
old_value=(
activity.old_value
if activity.old_value != ""
else None
),
new_value=(
activity.new_value
if activity.new_value != ""
else None
),
actor_id=activity.actor_id,
current_site=origin,
slug=activity.workspace.slug,
old_identifier=activity.old_identifier,
new_identifier=activity.new_identifier,
)

),
verb=activity.verb,
field=(
Expand Down