Skip to content
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.

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

[WEB-2848] improvement: enhanced components modularity #6196

Merged
merged 2 commits into from
Dec 13, 2024

Conversation

prateekshourya29
Copy link
Member

@prateekshourya29 prateekshourya29 commented Dec 13, 2024

Description

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Improvement (change that would cause existing functionality to not work as expected)
  • Code refactoring
  • Performance improvements
  • Documentation update

Screenshots and Media (if applicable)

Test Scenarios

References

Summary by CodeRabbit

  • New Features

    • Introduced new filter type for team projects in the filtering components.
    • Added drag-and-drop enhancements in Kanban and Calendar components, allowing for more granular control based on project context.
    • Enhanced permission checks for editing properties based on project and workspace context.
  • Bug Fixes

    • Improved handling of editing permissions and drag-and-drop logic based on project ID across various components.
  • Documentation

    • Updated type definitions to include new properties and methods for enhanced functionality.
  • Style

    • Minor adjustments to the structure and layout of components for better user experience.
  • Chores

    • Reorganized import statements for clarity and maintainability across multiple components.

Copy link
Contributor

coderabbitai bot commented Dec 13, 2024

Walkthrough

The changes in this pull request involve extensive modifications across multiple components within the issue layouts, primarily focusing on enhancing user permissions and editing capabilities based on project context. Key updates include the addition of new properties and methods to handle permissions, drag-and-drop functionality, and the integration of project-specific logic. The Calendar, Kanban, and Spreadsheet components have been updated to include new types and functions that facilitate more granular control over editing and dragging operations, ensuring that user permissions are accurately reflected in the UI.

Changes

File Change Summary
web/core/components/issues/issue-layouts/calendar/base-calendar-root.tsx Updated CalendarStoreType to include EIssuesStoreType.TEAM and EIssuesStoreType.TEAM_VIEW. Added optional method canEditPropertiesBasedOnProject to IBaseCalendarRoot. Modified handleDragAndDrop and useEffect logic for clarity.
web/core/components/issues/issue-layouts/calendar/calendar.tsx Removed useUserPermissions hook and updated Props type to include canEditProperties. Modified handleDragAndDrop function signature.
web/core/components/issues/issue-layouts/calendar/day-tile.tsx Added issueProjectId to handleDragAndDrop function. Updated Props type to include canEditProperties.
web/core/components/issues/issue-layouts/calendar/issue-block-root.tsx Added canEditProperties to Props type. Modified drag logic to incorporate canEditProperties.
web/core/components/issues/issue-layouts/calendar/issue-blocks.tsx Added canEditProperties to Props type.
web/core/components/issues/issue-layouts/calendar/roots/project-root.tsx Modified CalendarLayout to include permission checks using canEditPropertiesBasedOnProject.
web/core/components/issues/issue-layouts/calendar/week-days.tsx Updated handleDragAndDrop function signature to include issueProjectId. Added canEditProperties to Props type.
web/core/components/issues/issue-layouts/filters/applied-filters/filters-list.tsx Added conditional rendering for new filter type team_project.
web/core/components/issues/issue-layouts/filters/header/filters/filters-selection.tsx Introduced FilterTeamProjects for filtering based on team projects.
web/core/components/issues/issue-layouts/kanban/base-kanban-root.tsx Updated KanbanStoreType to include EIssuesStoreType.TEAM and EIssuesStoreType.TEAM_VIEW. Modified renderQuickActions to utilize canEditProperties.
web/core/components/issues/issue-layouts/kanban/block.tsx Added canDragIssuesInCurrentGrouping to IssueBlockProps. Updated drag logic.
web/core/components/issues/issue-layouts/kanban/blocks-list.tsx Added canDragIssuesInCurrentGrouping to IssueBlocksListProps.
web/core/components/issues/issue-layouts/kanban/kanban-group.tsx Introduced DRAG_ALLOWED_GROUPS to manage drag permissions based on grouping.
web/core/components/issues/issue-layouts/kanban/roots/project-root.tsx Updated KanBanLayout to include permission checks with canEditPropertiesBasedOnProject.
web/core/components/issues/issue-layouts/list/base-list-root.tsx Updated ListStoreType to include EIssuesStoreType.TEAM and EIssuesStoreType.TEAM_VIEW. Modified renderQuickActions logic.
web/core/components/issues/issue-layouts/list/block.tsx Introduced derived values for drag permissions. Updated useEffect dependencies.
web/core/components/issues/issue-layouts/list/list-group.tsx Removed useParams hook and simplified drag logic.
web/core/components/issues/issue-layouts/list/roots/project-root.tsx Simplified ListLayout component by removing projectId extraction.
web/core/components/issues/issue-layouts/quick-action-dropdowns/project-issue.tsx Updated permission checks in ProjectIssueQuickActions.
web/core/components/issues/issue-layouts/spreadsheet/base-spreadsheet-root.tsx Updated SpreadsheetStoreType to include EIssuesStoreType.TEAM and EIssuesStoreType.TEAM_VIEW. Modified renderQuickActions logic.
web/core/components/issues/issue-layouts/spreadsheet/roots/project-root.tsx Updated ProjectSpreadsheetLayout to include permission checks with canEditPropertiesBasedOnProject.
web/core/components/issues/peek-overview/issue-detail.tsx Updated isSubmitting type to TNameDescriptionLoader.
web/core/components/issues/peek-overview/view.tsx Updated isSubmitting state type to TNameDescriptionLoader.
web/core/components/issues/title-input.tsx Updated isSubmitting and setIsSubmitting to use TNameDescriptionLoader.
web/core/components/profile/notification/email-notification-form.tsx Minor textual change in the description for "Property changes" notification setting.

Suggested labels

🧹chore, 🛠️refactor, 🌟improvement

Suggested reviewers

  • SatishGandham

🐰 In the garden of code, we hop and play,
New features sprout, brightening the day.
With permissions in check, we dance and glide,
Editing made easy, with joy as our guide.
So let’s celebrate changes, both big and small,
A leap for our project, let’s cherish them all! 🌼✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 97502ca and e5d7f48.

📒 Files selected for processing (1)
  • web/core/components/profile/notification/email-notification-form.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • web/core/components/profile/notification/email-notification-form.tsx

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 generate docstrings to generate docstrings for this PR. (Experiment)
  • @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: 3

🧹 Outside diff range and nitpick comments (14)
web/core/components/issues/issue-layouts/kanban/block.tsx (1)

200-208: Consider extracting error messages to improve maintainability.

The inline error messages could be moved to a constants file for better maintainability and reusability.

+// In constants/error-messages.ts
+export const ISSUE_DRAG_ERROR_MESSAGES = {
+  NO_PERMISSION: "You are not allowed to move this issue",
+  DISABLED_GROUPING: "Drag and drop is disabled for the current grouping",
+};

-message: !canEditIssueProperties
-  ? "You are not allowed to move this issue"
-  : "Drag and drop is disabled for the current grouping",
+message: !canEditIssueProperties
+  ? ISSUE_DRAG_ERROR_MESSAGES.NO_PERMISSION
+  : ISSUE_DRAG_ERROR_MESSAGES.DISABLED_GROUPING,
web/core/components/issues/issue-layouts/kanban/kanban-group.tsx (1)

261-264: LGTM: Well-structured drag control implementation.

The implementation of canDragIssuesInCurrentGrouping correctly handles both single and sub-group scenarios, ensuring drag operations are only allowed for supported groupings.

Remove the redundant double-negation in the sub-group check:

const canDragIssuesInCurrentGrouping =
  !!group_by &&
  DRAG_ALLOWED_GROUPS.includes(group_by) &&
-  (!!sub_group_by ? DRAG_ALLOWED_GROUPS.includes(sub_group_by) : true);
+  (!sub_group_by || DRAG_ALLOWED_GROUPS.includes(sub_group_by));
🧰 Tools
🪛 Biome (1.9.4)

[error] 264-264: Avoid redundant double-negation.

It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation

(lint/complexity/noExtraBooleanCast)

web/core/components/issues/issue-layouts/kanban/roots/project-root.tsx (2)

14-14: Review the use of toString() on workspaceSlug

The workspaceSlug obtained from useParams() is already a string. Using workspaceSlug?.toString() may be unnecessary. Consider removing toString() to simplify the code.


18-24: Extract canEditPropertiesBasedOnProject into a reusable hook

The canEditPropertiesBasedOnProject function is defined similarly in multiple components. To promote code reuse and maintainability, consider extracting this function into a shared utility or custom hook.

web/core/components/issues/issue-layouts/spreadsheet/roots/project-root.tsx (1)

19-25: Extract canEditPropertiesBasedOnProject into a shared utility

The canEditPropertiesBasedOnProject function is repeated across different components. Refactoring this into a shared utility or custom hook would enhance code maintainability and reduce duplication.

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

33-34: Consider null coalescing operator for project_id

The permission check is correctly integrated with drag functionality, but the null check could be more concise.

-  const canDrag = !isDragDisabled && canEditProperties(issue?.project_id ?? undefined);
+  const canDrag = !isDragDisabled && canEditProperties(issue?.project_id ?? null);
web/core/components/issues/issue-layouts/calendar/week-days.tsx (1)

Line range hint 1-98: Consider documenting permission handling

While the implementation is solid, adding JSDoc comments for the permission-related props and functions would improve maintainability.

Add documentation for permission handling:

+ /**
+  * Props for CalendarWeekDays component
+  * @property {Function} canEditProperties - Function to check if properties can be edited based on project permissions
+  * @property {Function} handleDragAndDrop - Handler for drag-and-drop operations with project context
+  */
type Props = {
web/core/components/issues/title-input.tsx (1)

Line range hint 45-54: Consider enhancing error handling for network failures

The update operation should handle network failures gracefully and provide user feedback.

 if (debouncedValue && debouncedValue !== value) {
   if (debouncedValue.trim().length > 0) {
-    issueOperations.update(workspaceSlug, projectId, issueId, { name: debouncedValue }).finally(() => {
-      setIsSubmitting("saved");
-      if (textarea && !textarea.matches(":focus")) {
-        const trimmedTitle = debouncedValue.trim();
-        if (trimmedTitle !== title) setTitle(trimmedTitle);
-      }
-    });
+    issueOperations.update(workspaceSlug, projectId, issueId, { name: debouncedValue })
+      .then(() => {
+        setIsSubmitting("saved");
+        if (textarea && !textarea.matches(":focus")) {
+          const trimmedTitle = debouncedValue.trim();
+          if (trimmedTitle !== title) setTitle(trimmedTitle);
+        }
+      })
+      .catch((error) => {
+        setIsSubmitting("error");
+        setTitle(value || ""); // Revert on error
+        // Consider adding a toast notification here
+      });
web/core/components/issues/peek-overview/issue-detail.tsx (1)

Line range hint 45-53: Consider using a constant for the timeout duration

The timeout duration for submission state should be extracted into a named constant for better maintainability.

+const SUBMISSION_RESET_TIMEOUT = 2000;
+
 useEffect(() => {
   if (isSubmitting === "submitted") {
     setShowAlert(false);
     setTimeout(async () => {
       setIsSubmitting("saved");
-    }, 2000);
+    }, SUBMISSION_RESET_TIMEOUT);
   } else if (isSubmitting === "submitting") {
     setShowAlert(true);
   }
 }, [isSubmitting, setShowAlert, setIsSubmitting]);
web/core/components/issues/issue-layouts/calendar/base-calendar-root.tsx (1)

133-141: Consider memoizing enableInlineEditing access

The canEditProperties callback accesses enableInlineEditing on every call, which could be optimized by destructuring it outside the callback to avoid potential re-renders.

+ const { enableInlineEditing } = issues?.viewFlags || {};
  const canEditProperties = useCallback(
    (projectId: string | undefined) => {
      const isEditingAllowedBasedOnProject =
        canEditPropertiesBasedOnProject && projectId ? canEditPropertiesBasedOnProject(projectId) : isEditingAllowed;
-     return enableInlineEditing && isEditingAllowedBasedOnProject;
+     return !!enableInlineEditing && isEditingAllowedBasedOnProject;
    },
-   [canEditPropertiesBasedOnProject, enableInlineEditing, isEditingAllowed]
+   [canEditPropertiesBasedOnProject, isEditingAllowed]
  );
web/core/components/issues/issue-layouts/quick-action-dropdowns/project-issue.tsx (1)

56-61: Consider extracting permission check logic into a custom hook

The permission check logic could be moved to a custom hook (e.g., useProjectPermissions) to improve reusability and maintain consistency across components.

+// hooks/useProjectPermissions.ts
+export const useProjectPermissions = () => {
+  const { allowPermissions } = useUserPermissions();
+  
+  const isEditingAllowed = (workspaceSlug: string, projectId?: string, readOnly = false) =>
+    allowPermissions(
+      [EUserPermissions.ADMIN, EUserPermissions.MEMBER],
+      EUserPermissionsLevel.PROJECT,
+      workspaceSlug,
+      projectId
+    ) && !readOnly;
+    
+  return { isEditingAllowed };
+};

// In component:
-const isEditingAllowed =
-  allowPermissions(
-    [EUserPermissions.ADMIN, EUserPermissions.MEMBER],
-    EUserPermissionsLevel.PROJECT,
-    workspaceSlug?.toString(),
-    issue.project_id ?? undefined
-  ) && !readOnly;
+const { isEditingAllowed } = useProjectPermissions();
+const canEdit = isEditingAllowed(workspaceSlug?.toString() ?? "", issue.project_id, readOnly);
web/core/components/issues/issue-layouts/filters/header/filters/filters-selection.tsx (1)

212-221: Consider extracting filter sections into separate components

To improve code organization and reduce the component's size, consider extracting each filter section into a separate component.

+// components/issues/filters/FilterSection.tsx
+interface FilterSectionProps {
+  isEnabled: boolean;
+  children: React.ReactNode;
+}
+
+export const FilterSection: React.FC<FilterSectionProps> = ({ isEnabled, children }) => {
+  if (!isEnabled) return null;
+  return <div className="py-2">{children}</div>;
+};

// In FilterSelection:
-        {isFilterEnabled("team_project") && (
-          <div className="py-2">
-            <FilterTeamProjects
-              appliedFilters={filters.team_project ?? null}
-              handleUpdate={(val) => handleFiltersUpdate("team_project", val)}
-              searchQuery={filtersSearchQuery}
-            />
-          </div>
-        )}
+        <FilterSection isEnabled={isFilterEnabled("team_project")}>
+          <FilterTeamProjects
+            appliedFilters={filters.team_project ?? null}
+            handleUpdate={(val) => handleFiltersUpdate("team_project", val)}
+            searchQuery={filtersSearchQuery}
+          />
+        </FilterSection>
web/core/components/issues/issue-layouts/kanban/base-kanban-root.tsx (1)

186-186: Consider adding QuickActions to the dependency array

The renderQuickActions callback's dependency array should include the QuickActions component to prevent potential stale closures.

-  [isCompletedCycle, canEditProperties, removeIssue, updateIssue, removeIssueFromView, archiveIssue, restoreIssue]
+  [QuickActions, isCompletedCycle, canEditProperties, removeIssue, updateIssue, removeIssueFromView, archiveIssue, restoreIssue]
web/core/components/issues/issue-layouts/list/block.tsx (1)

88-92: LGTM! Consider adding type annotations for clarity.

The derived values improve code readability and maintainability by clearly separating permission checks.

Consider adding TypeScript type annotations for better type safety:

-  const issue = issuesMap[issueId];
-  const subIssuesCount = issue?.sub_issues_count ?? 0;
-  const canEditIssueProperties = canEditProperties(issue?.project_id ?? undefined);
-  const isDraggingAllowed = canDrag && canEditIssueProperties;
+  const issue: TIssue | undefined = issuesMap[issueId];
+  const subIssuesCount: number = issue?.sub_issues_count ?? 0;
+  const canEditIssueProperties: boolean = canEditProperties(issue?.project_id ?? undefined);
+  const isDraggingAllowed: boolean = canDrag && canEditIssueProperties;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ab11e83 and 97502ca.

📒 Files selected for processing (24)
  • web/core/components/issues/issue-layouts/calendar/base-calendar-root.tsx (7 hunks)
  • web/core/components/issues/issue-layouts/calendar/calendar.tsx (9 hunks)
  • web/core/components/issues/issue-layouts/calendar/day-tile.tsx (4 hunks)
  • web/core/components/issues/issue-layouts/calendar/issue-block-root.tsx (4 hunks)
  • web/core/components/issues/issue-layouts/calendar/issue-blocks.tsx (3 hunks)
  • web/core/components/issues/issue-layouts/calendar/roots/project-root.tsx (1 hunks)
  • web/core/components/issues/issue-layouts/calendar/week-days.tsx (3 hunks)
  • web/core/components/issues/issue-layouts/filters/applied-filters/filters-list.tsx (1 hunks)
  • web/core/components/issues/issue-layouts/filters/header/filters/filters-selection.tsx (2 hunks)
  • web/core/components/issues/issue-layouts/kanban/base-kanban-root.tsx (2 hunks)
  • web/core/components/issues/issue-layouts/kanban/block.tsx (4 hunks)
  • web/core/components/issues/issue-layouts/kanban/blocks-list.tsx (3 hunks)
  • web/core/components/issues/issue-layouts/kanban/kanban-group.tsx (3 hunks)
  • web/core/components/issues/issue-layouts/kanban/roots/project-root.tsx (1 hunks)
  • web/core/components/issues/issue-layouts/list/base-list-root.tsx (2 hunks)
  • web/core/components/issues/issue-layouts/list/block.tsx (4 hunks)
  • web/core/components/issues/issue-layouts/list/list-group.tsx (1 hunks)
  • web/core/components/issues/issue-layouts/list/roots/project-root.tsx (1 hunks)
  • web/core/components/issues/issue-layouts/quick-action-dropdowns/project-issue.tsx (1 hunks)
  • web/core/components/issues/issue-layouts/spreadsheet/base-spreadsheet-root.tsx (2 hunks)
  • web/core/components/issues/issue-layouts/spreadsheet/roots/project-root.tsx (1 hunks)
  • web/core/components/issues/peek-overview/issue-detail.tsx (2 hunks)
  • web/core/components/issues/peek-overview/view.tsx (2 hunks)
  • web/core/components/issues/title-input.tsx (2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
web/core/components/issues/issue-layouts/kanban/kanban-group.tsx

[error] 264-264: Avoid redundant double-negation.

It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation

(lint/complexity/noExtraBooleanCast)

🔇 Additional comments (30)
web/core/components/issues/issue-layouts/kanban/blocks-list.tsx (1)

19-19: LGTM: Clean prop addition for drag control.

The addition of the canDragIssuesInCurrentGrouping prop is well-implemented, following React best practices and maintaining consistent prop drilling.

Also applies to: 31-31, 62-62

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

38-38: LGTM: Enhanced drag-and-drop control logic.

The addition of canDragIssuesInCurrentGrouping and its integration with the drag allowance logic improves the control over issue movements.

Also applies to: 144-144

web/core/components/issues/issue-layouts/kanban/kanban-group.tsx (1)

296-296: LGTM: Consistent prop passing.

The canDragIssuesInCurrentGrouping prop is correctly passed down to the KanbanIssueBlocksList component.

web/core/components/issues/issue-layouts/kanban/roots/project-root.tsx (1)

18-24: Handle potential undefined workspaceSlug in allowPermissions

Since workspaceSlug could be undefined, ensure that the allowPermissions function can handle undefined values without causing runtime errors. Consider providing a default value or adding null checks within the function.

Run the following script to check how allowPermissions handles undefined workspaceSlug:

web/core/components/issues/issue-layouts/list/roots/project-root.tsx (1)

19-19: Confirm the null check for workspaceSlug

The check if (!workspaceSlug) return null; ensures the component doesn't render without a valid workspaceSlug. This is a good practice to prevent potential runtime errors.

web/core/components/issues/issue-layouts/calendar/roots/project-root.tsx (2)

14-14: Review the necessity of toString() on workspaceSlug

The use of workspaceSlug?.toString() may be redundant if workspaceSlug is already a string. Consider verifying its type and removing toString() if it's unnecessary.


18-25: Refactor canEditPropertiesBasedOnProject into a common hook

This function is identical to those in other components. Extracting it into a shared utility or hook will reduce code duplication and improve maintainability.

web/core/components/issues/issue-layouts/calendar/issue-block-root.tsx (2)

18-18: LGTM: Well-typed permission control

The canEditProperties prop is properly typed as a function that takes an optional project ID, maintaining type safety while improving modularity.


Line range hint 43-53: Verify drag-and-drop behavior with permissions

The drag-and-drop functionality now considers project-level permissions, but we should verify the behavior across different permission scenarios.

✅ Verification successful

Based on the search results, I can now provide a comprehensive verification of the drag-and-drop behavior with permissions:

Permission checks for drag-and-drop are consistently implemented

The codebase shows a robust and consistent implementation of permission checks across different views:

  • Calendar view (current file) correctly uses canEditProperties to check project-level permissions
  • Other views (Kanban, List, Spreadsheet) follow the same pattern where drag permissions are controlled by:
    • Project-level permissions through canEditProperties/canEditPropertiesBasedOnProject
    • Additional view-specific conditions (e.g., completed cycles, grouping restrictions)
  • Permission checks are properly propagated through component hierarchies

The implementation in the calendar view aligns with the established pattern across the codebase, ensuring consistent behavior.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other drag-and-drop implementations to ensure consistent permission handling
ast-grep --pattern 'canDrag = $$$canEditProperties($$$)'

Length of output: 58


Script:

#!/bin/bash
# Search for drag-and-drop related permission checks
rg -A 3 "canDrag.*=" 

# Search for permission-related functions that might be used in drag-and-drop
rg -A 3 "canEdit(Props|Properties|Issue)"

# Search for drag-and-drop hook usage
ast-grep --pattern 'useDrag({
  $$$
})'

Length of output: 62350

web/core/components/issues/issue-layouts/calendar/issue-blocks.tsx (2)

25-25: LGTM: Clean props interface extension

The canEditProperties prop is properly added to the Props interface, maintaining good type safety.


68-68: LGTM: Proper prop propagation

The permission check is correctly passed down to the CalendarIssueBlockRoot component.

web/core/components/issues/issue-layouts/calendar/week-days.tsx (2)

29-32: LGTM: Enhanced drag-and-drop context

The handleDragAndDrop signature now includes project context, improving the modularity of permission handling.


37-37: Verify permission propagation chain

The permission handling is properly implemented, but we should verify the complete chain of permission propagation through the component hierarchy.

Also applies to: 94-94

✅ Verification successful

Permission chain is properly implemented

The permission chain for canEditProperties in the calendar components is correctly implemented and follows a secure pattern:

  1. The permission originates from BaseCalendarRoot which uses useUserPermissions hook to check user permissions
  2. The permission is propagated through props from the root component down to child components
  3. The permission check includes both project-level permissions and inline editing capabilities
  4. The implementation includes proper fallbacks and safety checks
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the permission prop usage across calendar components
ast-grep --pattern 'canEditProperties: $$$'

Length of output: 45


Script:

#!/bin/bash
# Search for canEditProperties usage in the codebase
rg "canEditProperties" -A 3 -B 3

# Also search for any permission-related props in calendar components
rg -g "*calendar*.tsx" "can.*Props|permissions?" -A 3 -B 3

Length of output: 70734


Script:

#!/bin/bash
# Search for the permission chain in calendar components and their parents
rg -g "week-days.tsx" "canEditProperties|permissions?" -A 5 -B 5

# Search for any calendar-related components that use this prop
rg -g "*calendar*.tsx" "canEditProperties" -A 5 -B 5

Length of output: 16010


Script:

#!/bin/bash
# Search for calendar root components to understand the permission chain
rg -g "*calendar*/roots/*.tsx" "canEditProperties|permissions?" -A 5 -B 5

# Search for any calendar-related hooks that might handle permissions
rg -g "*calendar*" "usePermissions|useUserPermissions" -A 5 -B 5

Length of output: 2771

web/core/components/issues/title-input.tsx (1)

18-19: LGTM: Type enhancement improves consistency

The change from string union type to TNameDescriptionLoader aligns with the PR's modularity goals by standardizing loading state types across components.

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

30-31: LGTM: Consistent type usage across components

The interface update to use TNameDescriptionLoader maintains type consistency with other components.

web/core/components/issues/issue-layouts/spreadsheet/base-spreadsheet-root.tsx (2)

27-29: LGTM: Enhanced store type coverage

The addition of team-related store types improves modularity by supporting team-specific views and operations.


105-105: LGTM: Improved permission handling

The switch to canEditProperties for read-only state provides more granular control over editing permissions, and the dependency array is correctly updated to include the new function.

Also applies to: 109-109

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

137-143: LGTM: Good reuse of existing component

The implementation of the team_project filter follows good practices by:

  • Reusing the existing AppliedProjectFilters component
  • Maintaining consistency with other filter implementations
  • Following the same permission handling pattern
web/core/components/issues/issue-layouts/calendar/base-calendar-root.tsx (1)

26-28: LGTM: Store type extension

The addition of TEAM and TEAM_VIEW types maintains consistency with other layout components.

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

29-31: LGTM: Consistent store type extension

The addition of new store types maintains consistency across layout components.

web/core/components/issues/issue-layouts/calendar/day-tile.tsx (1)

49-49: LGTM: Good separation of edit permission logic

The addition of canEditProperties prop improves modularity by separating permission logic from the component implementation.

Also applies to: 70-70, 187-187

web/core/components/issues/issue-layouts/filters/header/filters/filters-selection.tsx (1)

28-28: LGTM: Clean import of new filter component

The import of FilterTeamProjects follows the established pattern and maintains code organization.

web/core/components/issues/issue-layouts/calendar/calendar.tsx (2)

66-66: LGTM: Good modularization of permissions logic

The addition of canEditProperties as a prop improves component modularity by moving permission checks out of the component.


99-99: LGTM: Consistent permission handling across views

The changes ensure consistent handling of quick add and issue creation permissions across different calendar views (month, week, and mobile).

Also applies to: 163-164, 186-187, 211-212, 238-239

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

3-4: LGTM: Improved type safety with TNameDescriptionLoader

Good addition of a specific type for name/description loading states.


55-55: LGTM: Consistent state management

The state type change ensures consistent submission state handling across components.

web/core/components/issues/issue-layouts/kanban/base-kanban-root.tsx (2)

35-37: LGTM: Extended store types for team views

Good addition of TEAM and TEAM_VIEW store types to support team-based issue management.


182-182: LGTM: Consistent permission handling in QuickActions

Good use of canEditProperties for consistent permission checks.

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

166-172: LGTM! Clear and user-friendly error handling.

The error messages now clearly distinguish between permission-based restrictions and functional limitations, improving the user experience.

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

238-238: Verify that removing projectId check doesn't bypass permission controls.

The simplified isDragAllowed check now only depends on group_by being in DRAG_ALLOWED_GROUPS. While this improves modularity, we should ensure that permission checks are properly handled at a higher level.

✅ Verification successful

Based on the code analysis, I can now provide a definitive response about the safety of removing the projectId check from isDragAllowed.

Drag operations are still properly permission-controlled

The removal of the projectId check from isDragAllowed is safe because:

  1. The drag permission is controlled through multiple layers:

    • At the group level, isDragAllowed checks if the grouping type is in DRAG_ALLOWED_GROUPS
    • At the issue level, drag operations require canEditProperties to be true, which checks user permissions
    • The canEditProperties function validates user permissions through allowPermissions([EUserPermissions.ADMIN, EUserPermissions.MEMBER], EUserPermissionsLevel.PROJECT)
  2. The actual drag operation is only enabled when both conditions are met:

    const isDragAllowed = canDragIssuesInCurrentGrouping && !issue?.tempId && canEditIssueProperties;
  3. The permission check is consistently applied across different views (list, kanban, calendar) through the canEditProperties function, which ensures users have the appropriate project-level permissions.

The change improves code organization by separating group-level drag constraints from permission checks, while maintaining the same level of security through the existing permission system.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if permissions are validated elsewhere in the component hierarchy
rg -A 5 "canEditProperties|isDragAllowed" --type typescript

Length of output: 96


Script:

#!/bin/bash
# Search for permission checks and drag-related functionality
rg -A 5 "canEditProperties|isDragAllowed|canDrag|permissions" 

# Also check for the DRAG_ALLOWED_GROUPS constant definition
rg "DRAG_ALLOWED_GROUPS.*=" 

Length of output: 75968


Script:

#!/bin/bash
# Search for DRAG_ALLOWED_GROUPS constant definition and its usage
rg -A 5 "DRAG_ALLOWED_GROUPS\s*=\s*" 

# Search for permission checks in the issue layouts
rg -A 5 "allowPermissions|canEditProperties|userPermissions" web/core/components/issues/issue-layouts/

Length of output: 65793


Script:

#!/bin/bash
# Search for DRAG_ALLOWED_GROUPS constant definition and its usage context
rg -B 5 -A 5 "DRAG_ALLOWED_GROUPS\s*=\s*\[" 

# Search for drag-related permission checks in the codebase
rg -A 5 "isDragAllowed.*=.*\|\|.*group_by" 

Length of output: 90


Script:

#!/bin/bash
# Search for DRAG_ALLOWED_GROUPS and isDragAllowed with more context
rg -B 10 -A 10 "DRAG_ALLOWED_GROUPS|isDragAllowed" web/core/components/issues/

# Search for drag-related permission checks in the issue layouts
rg -B 5 -A 5 "canDrag|draggable|drag.*allowed" web/core/components/issues/issue-layouts/

Length of output: 55183

@prateekshourya29 prateekshourya29 changed the title improvement: enhanced components modularity [WEB-2848] improvement: enhanced components modularity Dec 13, 2024
@SatishGandham SatishGandham merged commit 9234f21 into preview Dec 13, 2024
13 of 14 checks passed
@SatishGandham SatishGandham deleted the improvement-component branch December 13, 2024 08:56
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.

2 participants