-
Notifications
You must be signed in to change notification settings - Fork 4.5k
chore: refactor query duplication flow #36915
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
Changes from all commits
8e24f09
2558f69
7a25592
0aa8f37
1278d2b
8a4eef5
65c94e3
0e6a414
5d94282
991b26a
9662d00
56ad42b
84b5cda
098b318
67eb34d
fe4c2ee
5fe13ea
6a18c58
7f4a43e
fe98984
04dfbfb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| export { useActionSettingsConfig } from "ee/PluginActionEditor/hooks/useActionSettingsConfig"; | ||
| export { useHandleDeleteClick } from "ee/PluginActionEditor/hooks/useHandleDeleteClick"; | ||
| export { useHandleDuplicateClick } from "./useHandleDuplicateClick"; | ||
| export { useHandleRunClick } from "ee/PluginActionEditor/hooks/useHandleRunClick"; | ||
| export { useBlockExecution } from "ee/PluginActionEditor/hooks/useBlockExecution"; | ||
| export { useAnalyticsOnRunClick } from "ee/PluginActionEditor/hooks/useAnalyticsOnRunClick"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import { copyActionRequest } from "actions/pluginActionActions"; | ||
| import { usePluginActionContext } from "PluginActionEditor/PluginActionContext"; | ||
| import { useCallback } from "react"; | ||
| import { useDispatch } from "react-redux"; | ||
|
|
||
| function useHandleDuplicateClick() { | ||
| const { action } = usePluginActionContext(); | ||
| const dispatch = useDispatch(); | ||
|
|
||
| const handleDuplicateClick = useCallback( | ||
| (destinationEntityId: string) => { | ||
| dispatch( | ||
| copyActionRequest({ | ||
| id: action.id, | ||
| destinationEntityId, | ||
| name: action.name, | ||
| }), | ||
| ); | ||
| }, | ||
| [action.id, action.name, dispatch], | ||
| ); | ||
|
|
||
| return { handleDuplicateClick }; | ||
| } | ||
|
|
||
| export { useHandleDuplicateClick }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ export const Copy = () => { | |
| dispatch( | ||
| copyActionRequest({ | ||
| id: action.id, | ||
| destinationPageId: pageId, | ||
| destinationEntityId: pageId, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Parameter renaming needs to be completed across the codebase The verification reveals inconsistent parameter naming. While
Please update all occurrences to maintain consistency with the new 🔗 Analysis chainVerify consistent parameter naming across the codebase. The change from Let's verify the consistent usage across the codebase: The previous commands failed due to unsupported file type flags. Let's try without them and get a broader context of the changes. The new commands will help us:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for any remaining instances of destinationPageId
rg "destinationPageId" --type ts --type tsx
# Search for new parameter usage
rg "destinationEntityId.*copyActionRequest" --type ts --type tsx
Length of output: 166 Script: #!/bin/bash
# Search for any remaining instances of destinationPageId without file type flags
rg "destinationPageId"
# Search for new parameter usage
rg "destinationEntityId.*copyActionRequest"
# Let's also check for the action handler implementation
rg "copyActionRequest" -A 5
Length of output: 9829 |
||
| name: action.name, | ||
| }), | ||
| ), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,23 @@ export interface ResolveParentEntityMetadataReturnType { | |
| parentEntityKey?: CreateNewActionKeyInterface; | ||
| } | ||
|
|
||
| // This function is extended in EE. Please check the EE implementation before any modification. | ||
| export interface GenerateDestinationIdInfoReturnType { | ||
| pageId?: string; | ||
| } | ||
|
Comment on lines
+13
to
+16
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Interface implementation needs to be moved to EE directory The interface is currently defined in CE but imported from EE (
The current setup could lead to import resolution issues and doesn't align with the codebase's CE/EE separation pattern. 🔗 Analysis chainLGTM! Verify EE implementation compatibility. The interface is well-defined and follows TypeScript best practices. Let's verify the EE implementation: The previous search in the Let's check for EE implementations since the comment indicates this function is extended in EE. The previous search only found the CE implementation, so we need to look for actual extensions or usages. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check for any EE-specific extensions of this interface
rg -A 5 "interface GenerateDestinationIdInfoReturnType" "ee/"
Length of output: 105 Script: #!/bin/bash
# Let's try searching in the entire codebase for EE-specific extensions
rg -g '!node_modules' -g '!dist' -A 5 "interface GenerateDestinationIdInfoReturnType"
# Also search for any files that might contain EE implementations
fd -g '*.ts' -g '*.js' -E 'node_modules' -E 'dist' -x rg -l "GenerateDestinationIdInfoReturnType"
Length of output: 718 Script: #!/bin/bash
# Let's search for any EE-specific implementations or extensions
rg -g '!node_modules' -g '!dist' -A 10 "generateDestinationIdInfoForQueryDuplication" --type ts
# Also check for any imports of this interface
rg -g '!node_modules' -g '!dist' "import.*GenerateDestinationIdInfoReturnType" --type ts
Length of output: 2599 |
||
|
|
||
| // This function is extended in EE. Please check the EE implementation before any modification. | ||
| export function generateDestinationIdInfoForQueryDuplication( | ||
| destinationEntityId: string, | ||
| parentEntityKey: CreateNewActionKeyInterface, | ||
| ): GenerateDestinationIdInfoReturnType { | ||
| if (parentEntityKey === CreateNewActionKey.PAGE) { | ||
| return { pageId: destinationEntityId }; | ||
| } | ||
|
|
||
| return {}; | ||
| } | ||
|
|
||
| // This function is extended in EE. Please check the EE implementation before any modification. | ||
| export const resolveParentEntityMetadata = ( | ||
| action: Partial<Action>, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| import React from "react"; | ||
| import { fireEvent, render, waitFor } from "@testing-library/react"; | ||
| import "@testing-library/jest-dom/extend-expect"; | ||
| import { Provider } from "react-redux"; | ||
| import { lightTheme } from "selectors/themeSelectors"; | ||
| import { ThemeProvider } from "styled-components"; | ||
| import configureStore from "redux-mock-store"; | ||
| import { PluginType } from "entities/Action"; | ||
| import { | ||
| ActionEntityContextMenu, | ||
| type EntityContextMenuProps, | ||
| } from "./ActionEntityContextMenu"; | ||
| import { FilesContextProvider } from "../Files/FilesContextProvider"; | ||
| import { ActionParentEntityType } from "ee/entities/Engine/actionHelpers"; | ||
| import { act } from "react-dom/test-utils"; | ||
| import { | ||
| CONTEXT_COPY, | ||
| CONTEXT_DELETE, | ||
| CONTEXT_MOVE, | ||
| CONTEXT_RENAME, | ||
| CONTEXT_SHOW_BINDING, | ||
| createMessage, | ||
| } from "ee/constants/messages"; | ||
| import { | ||
| ReduxActionTypes, | ||
| type ReduxAction, | ||
| } from "ee/constants/ReduxActionConstants"; | ||
|
|
||
| const mockStore = configureStore([]); | ||
|
|
||
| const page1Id = "605c435a91dea93f0eaf91ba"; | ||
| const page2Id = "605c435a91dea93f0eaf91bc"; | ||
| const basePage2Id = "605c435a91dea93f0eaf91bc"; | ||
| const defaultState = { | ||
| ui: { | ||
| selectedWorkspace: { | ||
| workspace: {}, | ||
| }, | ||
| workspaces: { | ||
| packagesList: [], | ||
| }, | ||
| users: { | ||
| featureFlag: { | ||
| data: {}, | ||
| }, | ||
| }, | ||
| }, | ||
| entities: { | ||
| actions: [], | ||
| pageList: { | ||
| pages: [ | ||
| { | ||
| pageId: page1Id, | ||
| basePageId: page2Id, | ||
| pageName: "Page1", | ||
| isDefault: true, | ||
| slug: "page-1", | ||
| }, | ||
| { | ||
| pageId: page2Id, | ||
| basePageId: basePage2Id, | ||
| pageName: "Page2", | ||
| isDefault: false, | ||
| slug: "page-2", | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| const defaultProps: EntityContextMenuProps = { | ||
| id: "test-action-id", | ||
| name: "test-action", | ||
| canManageAction: true, | ||
| canDeleteAction: true, | ||
| pluginType: PluginType.DB, | ||
| }; | ||
|
|
||
| const defaultContext = { | ||
| editorId: "test-editor-id", | ||
| canCreateActions: true, | ||
| parentEntityId: "test-parent-entity-id", | ||
| parentEntityType: ActionParentEntityType.PAGE, | ||
| }; | ||
|
|
||
| describe("ActionEntityContextMenu", () => { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| let store: any; | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it("renders context menu with correct options for application editor", async () => { | ||
| store = mockStore(defaultState); | ||
| const { findByText, getByRole } = render( | ||
| <Provider store={store}> | ||
| <ThemeProvider theme={lightTheme}> | ||
| <FilesContextProvider {...defaultContext}> | ||
| <ActionEntityContextMenu {...defaultProps} /> | ||
| </FilesContextProvider> | ||
| </ThemeProvider> | ||
| </Provider>, | ||
| ); | ||
| const triggerToOpenMenu = getByRole("button"); | ||
|
|
||
| act(() => { | ||
| fireEvent.click(triggerToOpenMenu); | ||
| }); | ||
|
|
||
| await waitFor(() => { | ||
| expect(triggerToOpenMenu.parentNode).toHaveAttribute( | ||
| "aria-expanded", | ||
| "true", | ||
| ); | ||
| }); | ||
|
|
||
| // In context of pages, the copy to page option will be shown | ||
| const copyQueryToPageOptions = await findByText( | ||
| createMessage(CONTEXT_COPY), | ||
| ); | ||
|
|
||
| expect(await findByText(createMessage(CONTEXT_RENAME))).toBeInTheDocument(); | ||
| expect(await findByText(createMessage(CONTEXT_DELETE))).toBeInTheDocument(); | ||
| expect(await findByText(createMessage(CONTEXT_MOVE))).toBeInTheDocument(); | ||
| expect( | ||
| await findByText(createMessage(CONTEXT_SHOW_BINDING)), | ||
| ).toBeInTheDocument(); | ||
| expect(copyQueryToPageOptions).toBeInTheDocument(); | ||
|
|
||
| // Now we click on the copy to page option | ||
| act(() => { | ||
| fireEvent.click(copyQueryToPageOptions); | ||
| }); | ||
|
|
||
| // Now a menu with the list of pages will show up | ||
| const copyQueryToPageSubOptionPage1 = await findByText("Page1"); | ||
|
|
||
| expect(copyQueryToPageSubOptionPage1).toBeInTheDocument(); | ||
| expect(await findByText("Page2")).toBeInTheDocument(); | ||
|
|
||
| // Clicking on the page will trigger the correct action | ||
| act(() => { | ||
| fireEvent.click(copyQueryToPageSubOptionPage1); | ||
| }); | ||
|
|
||
| let actions: Array< | ||
| ReduxAction<{ | ||
| payload: { | ||
| id: string; | ||
| destinationEntityId: string; | ||
| name: string; | ||
| }; | ||
| }> | ||
| > = []; | ||
|
|
||
| await waitFor(() => { | ||
| actions = store.getActions(); | ||
| }); | ||
|
|
||
| expect(actions.length).toBe(1); | ||
| expect(actions[0].type).toBe(ReduxActionTypes.COPY_ACTION_INIT); | ||
| expect(actions[0].payload).toEqual({ | ||
| destinationEntityId: page1Id, | ||
| id: "test-action-id", | ||
| name: "test-action", | ||
| }); | ||
| }); | ||
| // TODO: add tests for all options rendered in the context menu | ||
|
Comment on lines
+94
to
+169
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Expand test coverage for all context menu actions. The current test only verifies the copy action. Consider adding test cases for:
Would you like me to help generate the additional test cases? |
||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ import { | |
| CONTEXT_NO_PAGE, | ||
| CONTEXT_SHOW_BINDING, | ||
| createMessage, | ||
| CONTEXT_DUPLICATE, | ||
| } from "ee/constants/messages"; | ||
| import { builderURL } from "ee/RouteBuilder"; | ||
|
|
||
|
|
@@ -33,8 +34,9 @@ import { useConvertToModuleOptions } from "ee/pages/Editor/Explorer/hooks"; | |
| import { MODULE_TYPE } from "ee/constants/ModuleConstants"; | ||
| import { PluginType } from "entities/Action"; | ||
| import { convertToBaseParentEntityIdSelector } from "selectors/pageListSelectors"; | ||
| import { ActionParentEntityType } from "ee/entities/Engine/actionHelpers"; | ||
|
|
||
| interface EntityContextMenuProps { | ||
| export interface EntityContextMenuProps { | ||
| id: string; | ||
| name: string; | ||
| className?: string; | ||
|
|
@@ -45,20 +47,20 @@ interface EntityContextMenuProps { | |
| export function ActionEntityContextMenu(props: EntityContextMenuProps) { | ||
| // Import the context | ||
| const context = useContext(FilesContext); | ||
| const { menuItems, parentEntityId } = context; | ||
| const { menuItems, parentEntityId, parentEntityType } = context; | ||
| const baseParentEntityId = useSelector((state) => | ||
| convertToBaseParentEntityIdSelector(state, parentEntityId), | ||
| ); | ||
|
|
||
| const { canDeleteAction, canManageAction } = props; | ||
| const dispatch = useDispatch(); | ||
| const [confirmDelete, setConfirmDelete] = useState(false); | ||
| const copyActionToPage = useCallback( | ||
| (actionId: string, actionName: string, pageId: string) => | ||
| const copyAction = useCallback( | ||
| (actionId: string, actionName: string, destinationEntityId: string) => | ||
| dispatch( | ||
| copyActionRequest({ | ||
| id: actionId, | ||
| destinationPageId: pageId, | ||
| destinationEntityId, | ||
| name: actionName, | ||
| }), | ||
| ), | ||
|
|
@@ -129,14 +131,26 @@ export function ActionEntityContextMenu(props: EntityContextMenuProps) { | |
| menuItems.includes(ActionEntityContextMenuItemsEnum.COPY) && | ||
| canManageAction && { | ||
| value: "copy", | ||
| onSelect: noop, | ||
| label: createMessage(CONTEXT_COPY), | ||
| children: menuPages.map((page) => { | ||
| return { | ||
| ...page, | ||
| onSelect: () => copyActionToPage(props.id, props.name, page.id), | ||
| }; | ||
| }), | ||
| onSelect: | ||
| parentEntityType === ActionParentEntityType.PAGE | ||
| ? noop | ||
| : () => { | ||
| copyAction(props.id, props.name, parentEntityId); | ||
| }, | ||
| label: createMessage( | ||
| parentEntityType === ActionParentEntityType.PAGE | ||
| ? CONTEXT_COPY | ||
| : CONTEXT_DUPLICATE, | ||
| ), | ||
| children: | ||
| parentEntityType === ActionParentEntityType.PAGE && | ||
| menuPages.length > 0 && | ||
| menuPages.map((page) => { | ||
| return { | ||
| ...page, | ||
| onSelect: () => copyAction(props.id, props.name, page.id), | ||
| }; | ||
| }), | ||
|
Comment on lines
+134
to
+153
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider extracting copy action logic into separate functions. The copy action configuration is complex and would benefit from being broken down into smaller, more testable functions. Consider this refactoring: const getCopyActionLabel = (parentEntityType: ActionParentEntityType) =>
parentEntityType === ActionParentEntityType.PAGE
? createMessage(CONTEXT_COPY)
: createMessage(CONTEXT_DUPLICATE);
const getCopyActionHandler = (
parentEntityType: ActionParentEntityType,
id: string,
name: string,
parentEntityId: string,
) =>
parentEntityType === ActionParentEntityType.PAGE
? noop
: () => copyAction(id, name, parentEntityId);
const getCopyActionChildren = (
parentEntityType: ActionParentEntityType,
menuPages: Array<any>,
id: string,
name: string,
) =>
parentEntityType === ActionParentEntityType.PAGE && menuPages.length > 0
? menuPages.map((page) => ({
...page,
onSelect: () => copyAction(id, name, page.id),
}))
: undefined;
// In optionsTree:
{
value: "copy",
onSelect: getCopyActionHandler(parentEntityType, props.id, props.name, parentEntityId),
label: getCopyActionLabel(parentEntityType),
children: getCopyActionChildren(parentEntityType, menuPages, props.id, props.name),
} |
||
| }, | ||
| menuItems.includes(ActionEntityContextMenuItemsEnum.MOVE) && | ||
| canManageAction && { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,7 +63,7 @@ export function MoreActionsMenu(props: EntityContextMenuProps) { | |
| dispatch( | ||
| copyActionRequest({ | ||
| id: actionId, | ||
| destinationPageId: pageId, | ||
| destinationEntityId: pageId, | ||
| name: actionName, | ||
| }), | ||
|
Comment on lines
+66
to
68
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider unifying the parameter naming between copy and move operations. The copy operation uses Consider updating the move operation to match: moveActionRequest({
id: actionId,
- destinationPageId,
+ destinationEntityId: destinationPageId,
originalPageId: propPageId ?? "",
name: actionName,
}),Also applies to: 73-80 |
||
| ), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ayushpahwa any unit test case coverage needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a hook which is passing on the action as per the input. I have added test for the rendering of the action menu which was the actual change and also covers the functionality of this hook. This test can be enhanced to introduce coverage of other options also but since they are already covered in cypress, I have just left a TODO comment for now for the same.