Skip to content

Comments

[WEB-2479] fix: merge default and archived issue details endpoint.#5882

Merged
pushya22 merged 1 commit intopreviewfrom
fix/archived-issue-api
Oct 24, 2024
Merged

[WEB-2479] fix: merge default and archived issue details endpoint.#5882
pushya22 merged 1 commit intopreviewfrom
fix/archived-issue-api

Conversation

@prateekshourya29
Copy link
Member

@prateekshourya29 prateekshourya29 commented Oct 21, 2024

This PR merges the default and archived issue detail endpoints to resolve minor issues caused by incorrect API calls. It also removes the archived status check for a more streamlined approach.

Media

This is one of the above mentioned scenario:

Before

Screen.Recording.2024-10-21.at.6.00.12.PM.mov

After

Screen.Recording.2024-10-21.at.5.59.38.PM.mov

Issue link: WEB-2479

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced issue retrieval with detailed annotations for labels, assignees, and modules.
    • Added isArchived property to various components for better state management of issues.
  • Bug Fixes

    • Improved error handling in multiple components, ensuring clearer notifications for users.
  • Refactor

    • Updated service usage for retrieving archived issues, streamlining the data fetching process.
    • Simplified conditional logic in hooks and components for better readability and performance.
  • Documentation

    • Adjustments made to the interface and method signatures to reflect changes in issue status management.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 21, 2024

Walkthrough

The pull request introduces significant modifications to the IssueViewSet and related components in the codebase. Key changes include updates to the retrieve, partial_update, and create methods in the IssueViewSet to improve issue fetching and data annotation. The front-end components also see adjustments, particularly in how archived issues are handled and displayed, including the removal of certain properties and the introduction of new data structures. Overall, the changes enhance consistency and clarity in issue management across both the backend and frontend.

Changes

File Change Summary
apiserver/plane/app/views/issue/base.py Updated retrieve, partial_update, and create methods in IssueViewSet for clearer issue fetching and annotations.
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/archives/issues/(detail)/page.tsx Modified fetcher function in ArchivedIssueDetailsPage to remove hardcoded "ARCHIVED" string.
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/archives/issues/(detail)/header.tsx Replaced IssueArchiveService with IssueService for fetching archived issue details.
web/core/components/cycles/active-cycle/cycle-stats.tsx Updated setPeekIssue to include isArchived property when handling cycle issue details.
web/core/components/issues/issue-layouts/list/block.tsx Enhanced setPeekIssue function to include isArchived property in IssueBlock.
web/core/components/issues/issue-layouts/roots/archived-issue-layout-root.tsx Removed is_archived prop from IssuePeekOverview.
web/core/components/issues/peek-overview/root.tsx Removed is_archived from IIssuePeekOverview interface and adjusted logic to use peekIssue.isArchived.
web/core/components/issues/peek-overview/view.tsx Updated removeRoutePeekId to check if embedRemoveCurrentNotification is defined before calling it.
web/core/hooks/use-issue-peek-overview-redirection.tsx Refactored handleRedirection to use if-else and included isArchived in setPeekIssue.
web/core/store/issue/issue-details/issue.store.ts Renamed issueType to issueStatus in fetchIssue method and removed "ARCHIVED" option.
web/core/store/issue/issue-details/root.store.ts Added isArchived property to TPeekIssue type and updated fetchIssue method signature.

Possibly related PRs

Suggested labels

🐛bug, ⚙️backend, 🌐frontend

Suggested reviewers

  • sriramveeraghanta
  • SatishGandham

Poem

In the fields where issues play,
Changes bloom, bright as day.
Archived tales now clearer told,
With each update, new and bold.
Rabbits hop with joy and cheer,
For better management is here! 🐰✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🧹 Outside diff range and nitpick comments (6)
web/core/hooks/use-issue-peek-overview-redirection.tsx (1)

25-29: Approved: Improved readability and added context

The changes enhance the code in two ways:

  1. The if-else structure is more readable than the previous ternary operator.
  2. The addition of the isArchived property provides more context about the issue's status.

Consider making the isArchived check more explicit:

-setPeekIssue({ workspaceSlug, projectId: project_id, issueId: id, nestingLevel, isArchived: !!archived_at });
+setPeekIssue({ workspaceSlug, projectId: project_id, issueId: id, nestingLevel, isArchived: archived_at !== null });

This change makes it clearer that an issue is considered archived when archived_at is not null, which might be more intuitive for other developers.

web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/archives/issues/(detail)/header.tsx (1)

27-30: LGTM! Consider minor optimization.

The changes in the useSWR hook are consistent with the service refactoring and align with the PR objectives. The removal of the "ARCHIVED" parameter streamlines the issue retrieval process.

Consider moving the toString() calls to the service method implementation for better separation of concerns:

? () => issueService.retrieve(workspaceSlug, projectId, archivedIssueId)

This would allow the service to handle any necessary type conversions internally, keeping the component code cleaner.

web/core/components/issues/peek-overview/view.tsx (1)

Line range hint 1-269: Consider refactoring for improved maintainability

While the current change is beneficial, the overall complexity of the IssueView component is noteworthy. Consider refactoring this large component into smaller, more focused components. This could improve maintainability, testability, and separation of concerns. Some suggestions:

  1. Extract modal logic into separate components.
  2. Create a custom hook for peek overview logic.
  3. Separate the rendering logic for different peek modes into individual components.

This refactoring would align with the PR's goal of creating a more streamlined approach to handling issue details.

web/core/components/issues/issue-layouts/list/block.tsx (1)

80-86: LGTM! Consider using a more explicit boolean conversion.

The addition of the isArchived property to the setPeekIssue call aligns well with the PR objective of merging default and archived issue detail endpoints. This change ensures that the peek overview is aware of the issue's archived status, which can be beneficial for UI rendering or subsequent API calls.

For improved clarity, consider using a more explicit boolean conversion:

-      isArchived: !!issue.archived_at,
+      isArchived: issue.archived_at !== null,

This makes the intention clearer and avoids potential confusion with falsy values.

web/core/components/issues/peek-overview/root.tsx (2)

105-105: Consider enhancing error handling

While removing unused error parameters improves code cleanliness, completely omitting error logging might hinder future debugging efforts. Consider adding a generic error logging mechanism that doesn't expose sensitive information but still provides useful debug data.

For example, you could create a utility function:

const logError = (method: string, error: unknown) => {
  console.error(`Error in ${method}:`, error instanceof Error ? error.message : 'Unknown error');
};

Then use it in catch blocks:

catch (error) {
  logError('removeIssue', error);
  setToast({
    title: "Error!",
    type: TOAST_TYPE.ERROR,
    message: "Issue delete failed",
  });
  // ... rest of the code
}

This approach balances code cleanliness with maintainability.

Also applies to: 120-121, 127-127, 148-148, 174-174, 203-203, 245-245, 308-308


321-321: LGTM: Updated dependencies, minor suggestion

Good job adding is_draft to the dependencies array of the useMemo hook. This ensures that issueOperations is correctly memoized based on all its dependencies.

Minor suggestion: Consider maintaining a consistent order of dependencies across the component for better readability and to ensure consistent memoization. You might want to alphabetize the dependencies or group them by their source (e.g., props, hooks, etc.).

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between b745a29 and 999bf64.

📒 Files selected for processing (11)
  • apiserver/plane/app/views/issue/base.py (1 hunks)
  • web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/archives/issues/(detail)/[archivedIssueId]/page.tsx (1 hunks)
  • web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/archives/issues/(detail)/header.tsx (2 hunks)
  • web/core/components/cycles/active-cycle/cycle-stats.tsx (1 hunks)
  • web/core/components/issues/issue-layouts/list/block.tsx (1 hunks)
  • web/core/components/issues/issue-layouts/roots/archived-issue-layout-root.tsx (1 hunks)
  • web/core/components/issues/peek-overview/root.tsx (12 hunks)
  • web/core/components/issues/peek-overview/view.tsx (1 hunks)
  • web/core/hooks/use-issue-peek-overview-redirection.tsx (1 hunks)
  • web/core/store/issue/issue-details/issue.store.ts (3 hunks)
  • web/core/store/issue/issue-details/root.store.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (14)
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/archives/issues/(detail)/[archivedIssueId]/page.tsx (1)

32-32: Verify impact on archived issue retrieval and consider component naming.

The removal of the "ARCHIVED" parameter aligns with the PR objective to merge default and archived issue detail endpoints. This change simplifies the API call and creates a more unified approach to fetching issue details.

However, please ensure that:

  1. The backend API still correctly retrieves archived issues without this parameter.
  2. The component still displays archived issues as intended.

Additionally, if this component is no longer specific to archived issues, consider updating its name and file path to reflect its more general purpose.

To verify the impact of this change, please run the following script:

This script will help identify any remaining references to archived issues in the codebase, which may need to be updated for consistency with this change.

web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/archives/issues/(detail)/header.tsx (1)

Line range hint 1-85: Summary: Successful refactoring with minimal impact.

The changes in this file effectively implement the PR objectives by refactoring the issue service usage. The modifications are focused and maintain the overall structure of the component, suggesting a low-risk change. These updates should improve code consistency and simplify issue management across the application.

web/core/store/issue/issue-details/issue.store.ts (3)

85-85: LGTM! Consistent method signature update.

The fetchIssue method signature has been correctly updated to use issueStatus instead of issueType, maintaining consistency with the interface change. The default value of "DEFAULT" ensures backwards compatibility.


102-104: LGTM! Verify main issue service handling.

The simplification of the conditional logic aligns with the PR objective. The removal of the "ARCHIVED" check reduces complexity and potential branching in the code.

Please confirm that the main issue service (this.issueService.retrieve) is now capable of handling both default and archived issues. Run the following script to check the implementation of the retrieve method:

#!/bin/bash
# Check the implementation of the retrieve method in the IssueService
rg -A 10 'class IssueService' | rg -A 20 'retrieve\('

Line range hint 1-248: Overall assessment: Consistent simplification of issue status handling.

The changes in this file consistently remove the distinction between default and archived issues, aligning with the PR objectives. This simplification should lead to more streamlined issue management. However, it's crucial to ensure that this change is reflected consistently across the entire codebase and that the main issue service can now handle both default and archived issues appropriately.

To ensure consistency across the codebase, please run the following script to check for any remaining references to archived issues that might need updating:

#!/bin/bash
# Search for references to archived issues that might need updating
rg 'archived.*issue' --type ts
web/core/components/issues/peek-overview/view.tsx (1)

69-69: Improved null check for embedRemoveCurrentNotification

This change enhances the robustness of the removeRoutePeekId function by adding a null check for embedRemoveCurrentNotification. This prevents potential runtime errors if embedRemoveCurrentNotification is undefined, which aligns with the PR's objective of addressing minor issues and creating a more streamlined approach.

web/core/components/issues/peek-overview/root.tsx (5)

3-3: LGTM: Good addition of useCallback

Adding useCallback to the imports is a positive change. It sets the stage for optimizing the component's performance by memoizing callback functions.


49-52: LGTM: Optimized removeRoutePeekId with useCallback

Great job wrapping removeRoutePeekId with useCallback. This optimization can help prevent unnecessary re-renders in child components that receive this function as a prop. The dependency array is correctly populated with all the variables used within the function.


59-62: LGTM: Improved error handling and simplified condition

The changes in the fetch method are positive:

  1. The simplified condition for is_draft improves code readability.
  2. Adding error logging in the catch block is beneficial for debugging purposes.

These modifications enhance the overall quality and maintainability of the code.


347-347: LGTM: Updated is_archived prop usage

The change to is_archived={!!peekIssue.isArchived} is a good adaptation to the new data structure. Using the double negation (!!) ensures a boolean value, which is a best practice when working with potentially undefined values.

This change correctly aligns with the removal of the is_archived prop from the component's interface and the updates in the IssueDetail class mentioned in the summary.


Line range hint 1-354: Overall assessment: Good improvements with minor suggestions

The changes in this file generally improve code quality, performance, and consistency. Key improvements include:

  1. Addition of useCallback for performance optimization.
  2. Simplification of conditions and prop handling.
  3. Alignment with changes in related components and data structures.

Consider the following suggestions for further improvement:

  1. Enhance error handling to balance code cleanliness with debugging capabilities.
  2. Maintain consistent ordering of dependencies in hooks for better readability and reliable memoization.

These changes align well with the PR objectives of merging default and archived issue detail endpoints and streamlining the approach to handling issue details.

web/core/components/cycles/active-cycle/cycle-stats.tsx (1)

175-180: LGTM: Improved handling of archived issues

The addition of the isArchived property to the setPeekIssue call enhances the consistency in handling archived issues across components. This change aligns with the broader refactoring mentioned in the PR summary.

web/core/store/issue/issue-details/root.store.ts (2)

40-40: LGTM: Addition of isArchived property

The addition of the optional isArchived property to TPeekIssue type is a good change. It allows for indicating whether an issue is archived, which aligns with the PR objective of merging default and archived issue details endpoints. The optional nature of the property ensures backward compatibility.


Line range hint 1-456: Summary: Changes align with PR objectives

The modifications in this file, including the addition of the isArchived property to TPeekIssue and the update to the fetchIssue method signature, are consistent with the PR objective of merging default and archived issue details endpoints. These changes improve code clarity and structure without introducing backward compatibility issues.

<ArchivedIssueListLayout />
</div>
<IssuePeekOverview is_archived />
<IssuePeekOverview />
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Remaining usage of is_archived prop detected in the following files:

  • web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/archives/issues/(detail)/[archivedIssueId]/page.tsx
  • web/core/components/issues/issue-detail/root.tsx
  • web/core/components/issues/peek-overview/view.tsx
🔗 Analysis chain

Verify the removal of is_archived prop from IssuePeekOverview

The is_archived prop has been removed from the IssuePeekOverview component. This change aligns with the PR objective of merging the default and archived issue detail endpoints and removing the archived status check.

However, to ensure this change doesn't introduce any unintended side effects:

  1. Confirm that the IssuePeekOverview component can now handle both archived and non-archived issues correctly without this prop.
  2. Verify that this change is consistent with updates in the IssuePeekOverview component implementation.

To verify this change, please run the following script:

This script will help us confirm that the is_archived prop has been consistently removed from the IssuePeekOverview component and its usage across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining uses of 'is_archived' prop in IssuePeekOverview

# Search for IssuePeekOverview component definition
echo "IssuePeekOverview component definition:"
ast-grep --pattern 'const IssuePeekOverview = $_' -A 10

# Search for uses of IssuePeekOverview component
echo "\nUses of IssuePeekOverview component:"
rg '<IssuePeekOverview' -A 3 -g '!archived-issue-layout-root.tsx'

# Check if 'is_archived' is still used in the component
echo "\nChecking for 'is_archived' usage in IssuePeekOverview:"
rg 'is_archived' $(rg -l 'const IssuePeekOverview = $_')

Length of output: 8618

import { useProject } from "@/hooks/store";
// services
import { IssueArchiveService } from "@/services/issue";
import { IssueService } from "@/services/issue";
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Unresolved references to IssueArchiveService found.

The following files still reference IssueArchiveService:

  • web/core/services/issue/issue_archive.service.ts
  • web/core/store/issue/issue-details/issue.store.ts
  • web/core/store/issue/helpers/base-issues.store.ts

Please update these to use IssueService as per the refactoring.

🔗 Analysis chain

LGTM! Verify impact on other components.

The change from IssueArchiveService to IssueService aligns with the PR objective of merging default and archived issue endpoints. This refactoring improves code consistency and maintainability.

To ensure this change doesn't introduce unintended side effects, please run the following script to check for any remaining references to IssueArchiveService:

Also applies to: 18-18

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining references to IssueArchiveService

# Test: Search for IssueArchiveService usage
rg 'IssueArchiveService'

Length of output: 621

projectId: string,
issueId: string,
issueType?: "DEFAULT" | "DRAFT" | "ARCHIVED"
issueStatus?: "DEFAULT" | "DRAFT",
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Unresolved references to "ARCHIVED" found.

  • web/core/constants/issue.ts: ARCHIVED = "ARCHIVED",
🔗 Analysis chain

LGTM! Verify impact on codebase.

The simplification of the issueStatus parameter aligns with the PR objective. This change removes the distinction between default and archived issues at the interface level, which should simplify issue handling.

Please run the following script to ensure that there are no remaining references to the "ARCHIVED" status in the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to "ARCHIVED" status
rg '"ARCHIVED"' --type ts

Length of output: 4764


Script:

#!/bin/bash
# Search for any remaining references to "ARCHIVED" status
rg '"ARCHIVED"' --type ts

Length of output: 80


export const IssuePeekOverview: FC<IIssuePeekOverview> = observer((props) => {
const { embedIssue = false, embedRemoveCurrentNotification, is_archived = false, is_draft = false } = props;
const { embedIssue = false, embedRemoveCurrentNotification, is_draft = false } = props;
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

is_archived Prop Still in Use

The is_archived prop is still being referenced in multiple files, indicating incomplete removal. Please review and update all usages accordingly to prevent potential issues.

  • web/core/components/issues/peek-overview/root.tsx
  • web/core/components/issues/peek-overview/view.tsx
  • web/core/components/issues/issue-detail/root.tsx
  • web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/archives/issues/(detail)/[archivedIssueId]/page.tsx
🔗 Analysis chain

Verify the removal of is_archived prop

The removal of the is_archived prop aligns with the changes mentioned in the summary. This simplification is good, but please ensure that all uses of this prop throughout the codebase have been updated accordingly to prevent any potential bugs.

To verify this change, run the following command to check for any remaining uses of is_archived:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

rg "is_archived" --type tsx --type ts

Length of output: 65


Script:

#!/bin/bash
# Searching for 'is_archived' in .ts and .tsx files
rg "is_archived" --glob "*.ts" --glob "*.tsx"

Length of output: 2428

Comment on lines +255 to +256
issueStatus: "DEFAULT" | "DRAFT" = "DEFAULT"
) => this.issue.fetchIssue(workspaceSlug, projectId, issueId, issueStatus);
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Potential Issues: Incomplete fetchIssue Calls Identified

The following calls to fetchIssue are missing the issueStatus parameter:

  • web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/archives/issues/(detail)/[archivedIssueId])/page.tsx:32
  • web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/issues/(detail)/[issueId])/page.tsx:38
  • web/core/components/issues/issue-detail/root.tsx:86

Please update these calls to include the issueStatus argument as per the new method signature.

🔗 Analysis chain

LGTM: Updated fetchIssue method signature

The update to the fetchIssue method signature is a good change. Renaming issueType to issueStatus improves clarity, and removing "ARCHIVED" as a possible value aligns with the PR objective of merging default and archived issue details endpoints.

To ensure this change doesn't break existing code, please verify all calls to fetchIssue across the codebase. Run the following script to find all occurrences:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all calls to fetchIssue method
# Expected result: All calls should use "DEFAULT" or "DRAFT" as the last argument, or omit it entirely

rg -p 'fetchIssue\s*\([^)]*\)' --type ts

Length of output: 857

Comment on lines +485 to +522
Issue.objects.filter(
project_id=self.kwargs.get("project_id")
)
.filter(workspace__slug=self.kwargs.get("slug"))
.select_related("workspace", "project", "state", "parent")
.prefetch_related("assignees", "labels", "issue_module__module")
.annotate(
cycle_id=Case(
When(
issue_cycle__cycle__deleted_at__isnull=True,
then=F("issue_cycle__cycle_id"),
),
default=None,
)
)
.annotate(
link_count=IssueLink.objects.filter(issue=OuterRef("id"))
.order_by()
.annotate(count=Func(F("id"), function="Count"))
.values("count")
)
.annotate(
attachment_count=FileAsset.objects.filter(
issue_id=OuterRef("id"),
entity_type=FileAsset.EntityTypeContext.ISSUE_ATTACHMENT,
)
.order_by()
.annotate(count=Func(F("id"), function="Count"))
.values("count")
)
.annotate(
sub_issues_count=Issue.issue_objects.filter(
parent=OuterRef("id")
)
.order_by()
.annotate(count=Func(F("id"), function="Count"))
.values("count")
)
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor repeated queryset logic into a helper method

The queryset annotations and prefetches in the retrieve method are similar to those in partial_update. To reduce code duplication and enhance maintainability, consider refactoring this logic into a shared helper method or modifying get_queryset() to include the necessary annotations.


⚠️ Potential issue

Use self.get_queryset() to ensure consistent querying

In the retrieve method, you're directly querying Issue.objects instead of using self.get_queryset(). This may bypass filters, annotations, or prefetches defined in get_queryset(), potentially leading to inconsistencies. It's recommended to use self.get_queryset().filter(pk=pk) to maintain consistency across the viewset methods.

Apply this diff to fix the issue:

-def retrieve(self, request, slug, project_id, pk=None):
-    project = Project.objects.get(pk=project_id, workspace__slug=slug)
-
-    issue = (
-        Issue.objects.filter(
-            project_id=self.kwargs.get("project_id")
-        )
-        .filter(workspace__slug=self.kwargs.get("slug"))
-        .select_related("workspace", "project", "state", "parent")
-        .prefetch_related("assignees", "labels", "issue_module__module")
-        .annotate(
-            cycle_id=Case(
-                When(
-                    issue_cycle__cycle__deleted_at__isnull=True,
-                    then=F("issue_cycle__cycle_id"),
-                ),
-                default=None,
-            )
-        )
-        .annotate(
-            link_count=IssueLink.objects.filter(issue=OuterRef("id"))
-            .order_by()
-            .annotate(count=Func(F("id"), function="Count"))
-            .values("count")
-        )
-        .annotate(
-            attachment_count=FileAsset.objects.filter(
-                issue_id=OuterRef("id"),
-                entity_type=FileAsset.EntityTypeContext.ISSUE_ATTACHMENT,
-            )
-            .order_by()
-            .annotate(count=Func(F("id"), function="Count"))
-            .values("count")
-        )
-        .annotate(
-            sub_issues_count=Issue.issue_objects.filter(
-                parent=OuterRef("id")
-            )
-            .order_by()
-            .annotate(count=Func(F("id"), function="Count"))
-            .values("count")
-        )
-        .filter(pk=pk)
-        .annotate(
-            # Annotations...
-        )
-        .prefetch_related(
-            # Prefetches...
-        )
-    ).first()
+    issue = (
+        self.get_queryset()
+        .filter(pk=pk)
+        .annotate(
+            label_ids=Coalesce(
+                ArrayAgg(
+                    "labels__id",
+                    distinct=True,
+                    filter=(
+                        ~Q(labels__id__isnull=True)
+                        & Q(labels__deleted_at__isnull=True)
+                    ),
+                ),
+                Value([], output_field=ArrayField(UUIDField())),
+            ),
+            assignee_ids=Coalesce(
+                ArrayAgg(
+                    "assignees__id",
+                    distinct=True,
+                    filter=~Q(assignees__id__isnull=True)
+                    & Q(assignees__member_project__is_active=True),
+                ),
+                Value([], output_field=ArrayField(UUIDField())),
+            ),
+            module_ids=Coalesce(
+                ArrayAgg(
+                    "issue_module__module_id",
+                    distinct=True,
+                    filter=~Q(issue_module__module_id__isnull=True)
+                    & Q(issue_module__module__archived_at__isnull=True)
+                    & Q(issue_module__module__deleted_at__isnull=True),
+                ),
+                Value([], output_field=ArrayField(UUIDField())),
+            ),
+        )
+        .prefetch_related(
+            Prefetch(
+                "issue_reactions",
+                queryset=IssueReaction.objects.select_related(
+                    "issue", "actor"
+                ),
+            )
+        )
+        .prefetch_related(
+            Prefetch(
+                "issue_link",
+                queryset=IssueLink.objects.select_related("created_by"),
+            )
+        )
+        .annotate(
+            is_subscribed=Exists(
+                IssueSubscriber.objects.filter(
+                    workspace__slug=slug,
+                    project_id=project_id,
+                    issue_id=OuterRef("pk"),
+                    subscriber=request.user,
+                )
+            )
+        )
+        .first()
+    )
📝 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
Issue.objects.filter(
project_id=self.kwargs.get("project_id")
)
.filter(workspace__slug=self.kwargs.get("slug"))
.select_related("workspace", "project", "state", "parent")
.prefetch_related("assignees", "labels", "issue_module__module")
.annotate(
cycle_id=Case(
When(
issue_cycle__cycle__deleted_at__isnull=True,
then=F("issue_cycle__cycle_id"),
),
default=None,
)
)
.annotate(
link_count=IssueLink.objects.filter(issue=OuterRef("id"))
.order_by()
.annotate(count=Func(F("id"), function="Count"))
.values("count")
)
.annotate(
attachment_count=FileAsset.objects.filter(
issue_id=OuterRef("id"),
entity_type=FileAsset.EntityTypeContext.ISSUE_ATTACHMENT,
)
.order_by()
.annotate(count=Func(F("id"), function="Count"))
.values("count")
)
.annotate(
sub_issues_count=Issue.issue_objects.filter(
parent=OuterRef("id")
)
.order_by()
.annotate(count=Func(F("id"), function="Count"))
.values("count")
)
issue = (
self.get_queryset()
.filter(pk=pk)
.annotate(
label_ids=Coalesce(
ArrayAgg(
"labels__id",
distinct=True,
filter=(
~Q(labels__id__isnull=True)
& Q(labels__deleted_at__isnull=True)
),
),
Value([], output_field=ArrayField(UUIDField())),
),
assignee_ids=Coalesce(
ArrayAgg(
"assignees__id",
distinct=True,
filter=~Q(assignees__id__isnull=True)
& Q(assignees__member_project__is_active=True),
),
Value([], output_field=ArrayField(UUIDField())),
),
module_ids=Coalesce(
ArrayAgg(
"issue_module__module_id",
distinct=True,
filter=~Q(issue_module__module_id__isnull=True)
& Q(issue_module__module__archived_at__isnull=True)
& Q(issue_module__module__deleted_at__isnull=True),
),
Value([], output_field=ArrayField(UUIDField())),
),
)
.prefetch_related(
Prefetch(
"issue_reactions",
queryset=IssueReaction.objects.select_related(
"issue", "actor"
),
)
)
.prefetch_related(
Prefetch(
"issue_link",
queryset=IssueLink.objects.select_related("created_by"),
)
)
.annotate(
is_subscribed=Exists(
IssueSubscriber.objects.filter(
workspace__slug=slug,
project_id=project_id,
issue_id=OuterRef("pk"),
subscriber=request.user,
)
)
)
.first()
)

Copy link
Contributor

@rahulramesha rahulramesha left a comment

Choose a reason for hiding this comment

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

@prateekshourya29 , pull request looks good, but can you check with product if an issue is archived should the sub issues also be archived

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants