Skip to content

[WEB-2724] fix: custom properties issue while moving to project #6090

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
Nov 26, 2024
Merged

[WEB-2724] fix: custom properties issue while moving to project #6090

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3b8d91
fixed custom properties adding issue
vamsikrishnamathala Nov 25, 2024
64c4194
added error handling to function
vamsikrishnamathala Nov 25, 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
28 changes: 19 additions & 9 deletions web/core/components/issues/issue-modal/form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ export const IssueFormRoot: FC<IssueFormProps> = observer((props) => {

// store hooks
const { getProjectById } = useProject();
const { getIssueTypeIdOnProjectChange, getActiveAdditionalPropertiesLength, handlePropertyValuesValidation } =
const { getIssueTypeIdOnProjectChange, getActiveAdditionalPropertiesLength, handlePropertyValuesValidation, handleCreateUpdatePropertyValues } =
useIssueModal();
const { isMobile } = usePlatformOS();
const { moveIssue } = useWorkspaceDraftIssues();
Expand Down Expand Up @@ -236,6 +236,23 @@ export const IssueFormRoot: FC<IssueFormProps> = observer((props) => {
});
};

const handleMoveToProjects = async () => {
if( !data?.id ||!data?.project_id || !data) return
await handleCreateUpdatePropertyValues({
issueId: data.id,
issueTypeId: data.type_id,
projectId: data.project_id,
workspaceSlug: workspaceSlug.toString(),
isDraft: true
})

moveIssue(workspaceSlug.toString(), data.id, {
...data,
...getValues(),
} as TWorkspaceDraftIssue);

}
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 25, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling and user feedback

While the function correctly handles the core operations, it needs better error handling and user feedback:

  1. Error handling for both operations
  2. Loading state management
  3. Success/error notifications

Consider this implementation:

  const handleMoveToProjects = async () => {
    if (!data?.id || !data?.project_id || !data) return;
+   try {
+     setIsMoving(true);  // Add this state
      await handleCreateUpdatePropertyValues({
        issueId: data.id,
        issueTypeId: data.type_id,
        projectId: data.project_id,
        workspaceSlug: workspaceSlug.toString(),
        isDraft: true
      });

      await moveIssue(workspaceSlug.toString(), data.id, {
        ...data,
        ...getValues(),
      } as TWorkspaceDraftIssue);
+     setToast({
+       type: TOAST_TYPE.SUCCESS,
+       title: "Success!",
+       message: "Issue successfully moved to project.",
+     });
+   } catch (error) {
+     setToast({
+       type: TOAST_TYPE.ERROR,
+       title: "Error!",
+       message: "Failed to move issue to project. Please try again.",
+     });
+   } finally {
+     setIsMoving(false);  // Add this state
+   }
  }

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 25, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling and type safety in handleMoveToProjects

While the function implements the core functionality, it could benefit from several improvements:

  1. The early return condition should be more explicit
  2. Add error handling for async operations
  3. Add loading state management
  4. Improve type safety for workspaceSlug

Consider this implementation:

-  const handleMoveToProjects = async () => {
-    if( !data?.id ||!data?.project_id || !data) return
-    await handleCreateUpdatePropertyValues({
-      issueId: data.id,
-      issueTypeId: data.type_id,
-      projectId: data.project_id,
-      workspaceSlug: workspaceSlug.toString(),
-      isDraft: true
-    })
-
-    moveIssue(workspaceSlug.toString(), data.id, {
-      ...data,
-      ...getValues(),
-    } as TWorkspaceDraftIssue);
-  }
+  const handleMoveToProjects = async () => {
+    try {
+      // Early return with explicit conditions
+      if (!data?.id || !data?.project_id || !workspaceSlug) {
+        setToast({
+          type: TOAST_TYPE.ERROR,
+          title: "Error",
+          message: "Missing required data for moving to project",
+        });
+        return;
+      }
+
+      // Set loading state
+      setValue("isSubmitting", true);
+
+      // Update property values
+      await handleCreateUpdatePropertyValues({
+        issueId: data.id,
+        issueTypeId: data.type_id,
+        projectId: data.project_id,
+        workspaceSlug: workspaceSlug.toString(),
+        isDraft: true
+      });
+
+      // Move the issue
+      await moveIssue(workspaceSlug.toString(), data.id, {
+        ...data,
+        ...getValues(),
+      } as TWorkspaceDraftIssue);
+
+      setToast({
+        type: TOAST_TYPE.SUCCESS,
+        title: "Success",
+        message: "Issue moved to project successfully",
+      });
+    } catch (error) {
+      setToast({
+        type: TOAST_TYPE.ERROR,
+        title: "Error",
+        message: "Failed to move issue to project",
+      });
+      console.error("Error moving issue to project:", error);
+    } finally {
+      setValue("isSubmitting", false);
+    }
+  };

Committable suggestion skipped: line range outside the PR's diff.


const condition =
(watch("name") && watch("name") !== "") || (watch("description_html") && watch("description_html") !== "<p></p>");

Expand Down Expand Up @@ -493,14 +510,7 @@ export const IssueFormRoot: FC<IssueFormProps> = observer((props) => {
type="button"
size="sm"
loading={isSubmitting}
onClick={() => {
if (data?.id && data) {
moveIssue(workspaceSlug.toString(), data?.id, {
...data,
...getValues(),
} as TWorkspaceDraftIssue);
}
}}
onClick={handleMoveToProjects}
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 25, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Update button to reflect loading state

The button should display the loading state during the move operation.

-  onClick={handleMoveToProjects}
+  onClick={handleMoveToProjects}
+  loading={isMoving}  // Add this prop after adding the state
+  disabled={isMoving} // Prevent multiple clicks

Committable suggestion skipped: line range outside the PR's diff.

>
Add to project
</Button>
Expand Down