From 03fc50cd797f3eb06d4844e4e548688da373752f Mon Sep 17 00:00:00 2001 From: ashit-rath Date: Wed, 17 Jan 2024 15:34:50 +0530 Subject: [PATCH] chore: entity name generation refactor to include other entity types (#30316) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description When we create an entity (JS or Query or API), the name is always generated based the lookup of only that particular type. This leads to name clash if a different entity has the same name format. Example: If a js object has the name "Query1" then if a query is being created; the generated name would be "Query1" and it would fail to create as there is a name clash. To avoid this; all entities are considered to generate name. This PR also makes sure that the name generation logic is done at a central place (saga) rather than the creator of the entity. This avoid code duplication. #### PR fixes following issue(s) PR for https://github.com/appsmithorg/appsmith-ee/pull/3306 > if no issue exists, please create an issue and ask the maintainers about this first > > #### Media > A video or a GIF is preferred. when using Loom, don’t embed because it looks like it’s a GIF. instead, just link to the video > > #### Type of change > Please delete options that are not relevant. - Chore (housekeeping or task changes that don't impact user perception) ## Testing > #### How Has This Been Tested? > Please describe the tests that you ran to verify your changes. Also list any relevant details for your test configuration. > Delete anything that is not relevant - [ ] Manual - [ ] JUnit - [ ] Jest - [ ] Cypress > > #### Test Plan > Add Testsmith test cases links that relate to this PR > > #### Issues raised during DP testing > Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR) > > > ## Checklist: #### Dev activity - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] PR is being merged under a feature flag #### QA activity: - [ ] [Speedbreak features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-) have been covered - [ ] Test plan covers all impacted features and [areas of interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-) - [ ] Test plan has been peer reviewed by project stakeholders and other QA members - [ ] Manually tested functionality on DP - [ ] We had an implementation alignment call with stakeholders post QA Round 2 - [ ] Cypress test cases have been added and approved by SDET/manual QA - [ ] Added `Test Plan Approved` label after Cypress tests were reviewed - [ ] Added `Test Plan Approved` label after JUnit tests were reviewed ## Summary by CodeRabbit - **New Features** - Introduced a new action creation process with enhanced naming conventions. - Implemented a new selector to generate entity names based on existing entities. - **Improvements** - Streamlined action creation flow by integrating new helper functions. - **Refactor** - Updated various components to utilize the new entity naming selector. - Consolidated action creation logic across different parts of the application. - **Bug Fixes** - Fixed inconsistencies in action naming during the creation of APIs and Queries. - **Code Cleanup** - Removed unused code and imports related to deprecated naming utilities. --- app/client/src/actions/pluginActionActions.ts | 6 ++ .../src/ce/constants/ReduxActionConstants.tsx | 1 + app/client/src/ce/sagas/helpers.ts | 22 ++++++++ .../src/ce/selectors/entitiesSelector.ts | 26 +++++++++ app/client/src/ee/sagas/helpers.ts | 1 + app/client/src/entities/Action/index.ts | 4 +- .../Editor/APIEditor/CurlImportEditor.tsx | 15 +++-- .../Editor/DatasourceInfo/QueryTemplates.tsx | 3 - .../src/pages/Editor/SaaSEditor/ListView.tsx | 5 -- app/client/src/sagas/ActionSagas.ts | 55 +++++++++++++++++++ app/client/src/sagas/ApiPaneSagas.ts | 17 ------ app/client/src/sagas/QueryPaneSagas.ts | 14 +---- 12 files changed, 125 insertions(+), 44 deletions(-) create mode 100644 app/client/src/ce/sagas/helpers.ts create mode 100644 app/client/src/ee/sagas/helpers.ts diff --git a/app/client/src/actions/pluginActionActions.ts b/app/client/src/actions/pluginActionActions.ts index 063088d2d0..c12ba3082c 100644 --- a/app/client/src/actions/pluginActionActions.ts +++ b/app/client/src/actions/pluginActionActions.ts @@ -17,6 +17,12 @@ import type { ModalInfo } from "reducers/uiReducers/modalActionReducer"; import type { OtlpSpan } from "UITelemetry/generateTraces"; export const createActionRequest = (payload: Partial) => { + return { + type: ReduxActionTypes.CREATE_ACTION_REQUEST, + payload, + }; +}; +export const createActionInit = (payload: Partial) => { return { type: ReduxActionTypes.CREATE_ACTION_INIT, payload, diff --git a/app/client/src/ce/constants/ReduxActionConstants.tsx b/app/client/src/ce/constants/ReduxActionConstants.tsx index 80693215db..9a63a75eab 100644 --- a/app/client/src/ce/constants/ReduxActionConstants.tsx +++ b/app/client/src/ce/constants/ReduxActionConstants.tsx @@ -256,6 +256,7 @@ const ActionTypes = { FETCH_PROPERTY_PANE_CONFIGS_SUCCESS: "FETCH_PROPERTY_PANE_CONFIGS_SUCCESS", FETCH_CONFIGS_INIT: "FETCH_CONFIGS_INIT", ADD_WIDGET_REF: "ADD_WIDGET_REF", + CREATE_ACTION_REQUEST: "CREATE_ACTION_REQUEST", CREATE_ACTION_INIT: "CREATE_ACTION_INIT", CREATE_ACTION_SUCCESS: "CREATE_ACTION_SUCCESS", FETCH_ACTIONS_INIT: "FETCH_ACTIONS_INIT", diff --git a/app/client/src/ce/sagas/helpers.ts b/app/client/src/ce/sagas/helpers.ts new file mode 100644 index 0000000000..79480ca7cc --- /dev/null +++ b/app/client/src/ce/sagas/helpers.ts @@ -0,0 +1,22 @@ +import type { CreateNewActionKeyInterface } from "@appsmith/entities/Engine/actionHelpers"; +import { CreateNewActionKey } from "@appsmith/entities/Engine/actionHelpers"; +import type { Action } from "entities/Action"; + +export interface ResolveParentEntityMetadataReturnType { + parentEntityId?: string; + parentEntityKey?: CreateNewActionKeyInterface; +} + +// This function is extended in EE. Please check the EE implementation before any modification. +export const resolveParentEntityMetadata = ( + action: Partial, +): ResolveParentEntityMetadataReturnType => { + if (action.pageId) { + return { + parentEntityId: action.pageId, + parentEntityKey: CreateNewActionKey.PAGE, + }; + } + + return { parentEntityId: undefined, parentEntityKey: undefined }; +}; diff --git a/app/client/src/ce/selectors/entitiesSelector.ts b/app/client/src/ce/selectors/entitiesSelector.ts index 1b40f1e03e..7f6bdcaad8 100644 --- a/app/client/src/ce/selectors/entitiesSelector.ts +++ b/app/client/src/ce/selectors/entitiesSelector.ts @@ -57,6 +57,8 @@ import { getCurrentWorkflowJSActions, } from "@appsmith/selectors/workflowSelectors"; import { MAX_DATASOURCE_SUGGESTIONS } from "constants/DatasourceEditorConstants"; +import type { CreateNewActionKeyInterface } from "@appsmith/entities/Engine/actionHelpers"; +import { getNextEntityName } from "utils/AppsmithUtils"; export const getEntities = (state: AppState): AppState["entities"] => state.entities; @@ -74,6 +76,12 @@ export enum PluginCategory { Others = "Others", } +export interface NewEntityNameOptions { + prefix: string; + parentEntityId: string; + parentEntityKey: CreateNewActionKeyInterface; +} + export type DatasourceGroupByPluginCategory = Record< PluginCategory, Datasource[] @@ -1432,3 +1440,21 @@ export const getAllJSCollections = createSelector( export const getIsActionConverting = (state: AppState, actionId: string) => { return false; }; + +export const getNewEntityName = createSelector( + getActions, + getJSCollections, + (_state: AppState, options: NewEntityNameOptions) => options, + (actions, jsCollections, options) => { + const { parentEntityId, parentEntityKey, prefix } = options; + + const actionNames = actions + .filter((a) => a.config[parentEntityKey] === parentEntityId) + .map((a) => a.config.name); + const jsActionNames = jsCollections + .filter((a) => a.config[parentEntityKey] === parentEntityId) + .map((a) => a.config.name); + + return getNextEntityName(prefix, actionNames.concat(jsActionNames)); + }, +); diff --git a/app/client/src/ee/sagas/helpers.ts b/app/client/src/ee/sagas/helpers.ts new file mode 100644 index 0000000000..8e05b607d8 --- /dev/null +++ b/app/client/src/ee/sagas/helpers.ts @@ -0,0 +1 @@ +export * from "ce/sagas/helpers"; diff --git a/app/client/src/entities/Action/index.ts b/app/client/src/entities/Action/index.ts index 6e99bcfd1a..3d835d3749 100644 --- a/app/client/src/entities/Action/index.ts +++ b/app/client/src/entities/Action/index.ts @@ -283,11 +283,11 @@ export const SCHEMA_SECTION_ID = "t--api-right-pane-schema"; export interface CreateApiActionDefaultsParams { apiType: string; from?: EventLocation; - newActionName: string; + newActionName?: string; } export interface CreateActionDefaultsParams { datasourceId: string; from?: EventLocation; - newActionName: string; + newActionName?: string; } diff --git a/app/client/src/pages/Editor/APIEditor/CurlImportEditor.tsx b/app/client/src/pages/Editor/APIEditor/CurlImportEditor.tsx index 107165d601..37b553a199 100644 --- a/app/client/src/pages/Editor/APIEditor/CurlImportEditor.tsx +++ b/app/client/src/pages/Editor/APIEditor/CurlImportEditor.tsx @@ -1,9 +1,8 @@ import React, { useMemo } from "react"; import CurlImportForm from "./CurlImportForm"; -import { createNewApiName } from "utils/AppsmithUtils"; import { curlImportSubmitHandler } from "./helpers"; -import { getActions } from "@appsmith/selectors/entitiesSelector"; +import { getNewEntityName } from "@appsmith/selectors/entitiesSelector"; import { getIsImportingCurl } from "selectors/ui"; import { showDebuggerFlag } from "selectors/debuggerSelectors"; import { useSelector } from "react-redux"; @@ -12,19 +11,27 @@ import type { BuilderRouteParams } from "constants/routes"; import CloseEditor from "components/editorComponents/CloseEditor"; import { useFeatureFlag } from "utils/hooks/useFeatureFlag"; import { FEATURE_FLAG } from "@appsmith/entities/FeatureFlag"; +import { CreateNewActionKey } from "@appsmith/entities/Engine/actionHelpers"; +import { DEFAULT_PREFIX } from "sagas/ActionSagas"; type CurlImportEditorProps = RouteComponentProps; function CurlImportEditor(props: CurlImportEditorProps) { - const actions = useSelector(getActions); const { pageId } = props.match.params; + const actionName = useSelector((state) => + getNewEntityName(state, { + prefix: DEFAULT_PREFIX.API, + parentEntityId: pageId, + parentEntityKey: CreateNewActionKey.PAGE, + }), + ); const showDebugger = useSelector(showDebuggerFlag); const isImportingCurl = useSelector(getIsImportingCurl); const initialFormValues = { pageId, - name: createNewApiName(actions, pageId), + name: actionName, }; const isPagesPaneEnabled = useFeatureFlag( FEATURE_FLAG.release_show_new_sidebar_pages_pane_enabled, diff --git a/app/client/src/pages/Editor/DatasourceInfo/QueryTemplates.tsx b/app/client/src/pages/Editor/DatasourceInfo/QueryTemplates.tsx index c358a17c2e..57449d7fb1 100644 --- a/app/client/src/pages/Editor/DatasourceInfo/QueryTemplates.tsx +++ b/app/client/src/pages/Editor/DatasourceInfo/QueryTemplates.tsx @@ -2,7 +2,6 @@ import React, { useCallback, useContext } from "react"; import { useDispatch, useSelector } from "react-redux"; import { createActionRequest } from "actions/pluginActionActions"; import type { AppState } from "@appsmith/reducers"; -import { createNewQueryName } from "utils/AppsmithUtils"; import { getCurrentApplicationId, getCurrentPageId, @@ -81,7 +80,6 @@ export function QueryTemplates(props: QueryTemplatesProps) { ); const createQueryAction = useCallback( (template: QueryTemplate) => { - const newQueryName = createNewQueryName(actions, currentPageId || ""); const queryactionConfiguration: Partial = { actionConfiguration: { body: template.body, @@ -93,7 +91,6 @@ export function QueryTemplates(props: QueryTemplatesProps) { dispatch( createActionRequest({ - name: newQueryName, pageId: currentPageId, pluginId: dataSource?.pluginId, datasource: { diff --git a/app/client/src/pages/Editor/SaaSEditor/ListView.tsx b/app/client/src/pages/Editor/SaaSEditor/ListView.tsx index 6a32f496fd..65adcc2744 100644 --- a/app/client/src/pages/Editor/SaaSEditor/ListView.tsx +++ b/app/client/src/pages/Editor/SaaSEditor/ListView.tsx @@ -12,7 +12,6 @@ import { createDatasourceFromForm } from "actions/datasourceActions"; import type { SaaSAction } from "entities/Action"; import { createActionRequest } from "actions/pluginActionActions"; import type { Datasource } from "entities/Datasource"; -import { createNewApiName } from "utils/AppsmithUtils"; import type { ActionDataState } from "@appsmith/reducers/entityReducers/actionsReducer"; // Design @@ -88,7 +87,6 @@ class ListView extends React.Component { handleCreateNewAPI = (datasource: Datasource) => { const { - actions, location, match: { params: { pageId }, @@ -100,10 +98,7 @@ class ListView extends React.Component { pgId = pageId; } if (pgId) { - const newApiName = createNewApiName(actions, pgId); - this.props.createAction({ - name: newApiName, pageId: pgId, pluginId: datasource.pluginId, datasource: { diff --git a/app/client/src/sagas/ActionSagas.ts b/app/client/src/sagas/ActionSagas.ts index 4f70cfab85..363026151c 100644 --- a/app/client/src/sagas/ActionSagas.ts +++ b/app/client/src/sagas/ActionSagas.ts @@ -31,6 +31,7 @@ import type { import { copyActionError, copyActionSuccess, + createActionInit, createActionSuccess, deleteActionSuccess, fetchActionsForPage, @@ -79,6 +80,7 @@ import { getPlugin, getSettingConfig, getPageActions, + getNewEntityName, } from "@appsmith/selectors/entitiesSelector"; import history from "utils/history"; import { INTEGRATION_TABS } from "constants/routes"; @@ -142,6 +144,12 @@ import { selectFeatureFlagCheck } from "@appsmith/selectors/featureFlagsSelector import { FEATURE_FLAG } from "@appsmith/entities/FeatureFlag"; import { identifyEntityFromPath } from "../navigation/FocusEntity"; import { getActionConfig } from "../pages/Editor/Explorer/Actions/helpers"; +import { resolveParentEntityMetadata } from "@appsmith/sagas/helpers"; + +export const DEFAULT_PREFIX = { + QUERY: "Query", + API: "Api", +} as const; export function* createDefaultActionPayloadWithPluginDefaults( props: CreateActionDefaultsParams, @@ -245,6 +253,52 @@ export function* getPluginActionDefaultValues(pluginId: string) { return initialValues; } +/** + * This saga prepares the action request i.e it helps generating a + * new name of an action. This is to reduce any dependency on name generation + * on the caller of this saga. + */ +export function* createActionRequestSaga( + actionPayload: ReduxAction< + Partial & { eventData: any; pluginId: string } + >, +) { + const payload = { ...actionPayload.payload }; + const pluginId = + actionPayload.payload.pluginId || + actionPayload.payload.datasource?.pluginId; + if (!actionPayload.payload.name) { + const { parentEntityId, parentEntityKey } = resolveParentEntityMetadata( + actionPayload.payload, + ); + + if (!parentEntityId || !parentEntityKey) return; + const plugin: Plugin | undefined = yield select(getPlugin, pluginId || ""); + const isQueryType = + plugin?.type === PluginType.DB || + plugin?.packageName === PluginPackageName.APPSMITH_AI; + + const prefix = isQueryType ? DEFAULT_PREFIX.QUERY : DEFAULT_PREFIX.API; + + if ( + plugin?.type === PluginType.DB || + plugin?.packageName === PluginPackageName.APPSMITH_AI + ) { + DEFAULT_PREFIX.QUERY; + } + + const name: string = yield select(getNewEntityName, { + prefix, + parentEntityId, + parentEntityKey, + }); + + payload.name = name; + } + + yield put(createActionInit(payload)); +} + export function* createActionSaga( actionPayload: ReduxAction< Partial & { eventData: any; pluginId: string } @@ -1128,6 +1182,7 @@ export function* watchActionSagas() { ReduxActionTypes.FETCH_ACTIONS_VIEW_MODE_INIT, fetchActionsForViewModeSaga, ), + takeEvery(ReduxActionTypes.CREATE_ACTION_REQUEST, createActionRequestSaga), takeEvery(ReduxActionTypes.CREATE_ACTION_INIT, createActionSaga), takeLatest(ReduxActionTypes.UPDATE_ACTION_INIT, updateActionSaga), takeLatest(ReduxActionTypes.DELETE_ACTION_INIT, deleteActionSaga), diff --git a/app/client/src/sagas/ApiPaneSagas.ts b/app/client/src/sagas/ApiPaneSagas.ts index bfc8b5f286..629f7e8f33 100644 --- a/app/client/src/sagas/ApiPaneSagas.ts +++ b/app/client/src/sagas/ApiPaneSagas.ts @@ -34,19 +34,13 @@ import history from "utils/history"; import { INTEGRATION_EDITOR_MODES, INTEGRATION_TABS } from "constants/routes"; import { initialize, autofill, change, reset } from "redux-form"; import type { Property } from "api/ActionAPI"; -import { createNewApiName, createNewQueryName } from "utils/AppsmithUtils"; import { getQueryParams } from "utils/URLUtils"; import { getPluginIdOfPackageName } from "sagas/selectors"; import { getAction, - getActions, getDatasourceActionRouteInfo, getPlugin, } from "@appsmith/selectors/entitiesSelector"; -import type { - ActionData, - ActionDataState, -} from "@appsmith/reducers/entityReducers/actionsReducer"; import { createActionRequest, setActionProperty, @@ -724,16 +718,6 @@ function* handleCreateNewApiActionSaga( const { apiType = PluginPackageName.REST_API, from, pageId } = action.payload; if (pageId) { - const actions: ActionDataState = yield select(getActions); - const pageActions = actions.filter( - (a: ActionData) => a.config.pageId === pageId, - ); - let newActionName = createNewApiName(pageActions, pageId); - - // Change the action name to Query if the plugin is Appsmith AI - if (apiType === PluginPackageName.APPSMITH_AI) { - newActionName = createNewQueryName(pageActions, pageId); - } // Note: Do NOT send pluginId on top level here. // It breaks embedded rest datasource flow. @@ -742,7 +726,6 @@ function* handleCreateNewApiActionSaga( { apiType, from, - newActionName, }, ); diff --git a/app/client/src/sagas/QueryPaneSagas.ts b/app/client/src/sagas/QueryPaneSagas.ts index fd90fad027..b4ecf6bdfc 100644 --- a/app/client/src/sagas/QueryPaneSagas.ts +++ b/app/client/src/sagas/QueryPaneSagas.ts @@ -36,7 +36,6 @@ import { getSettingConfig, getPlugins, getGenerateCRUDEnabledPluginMap, - getActions, } from "@appsmith/selectors/entitiesSelector"; import type { Action, QueryAction } from "entities/Action"; import { PluginType } from "entities/Action"; @@ -86,8 +85,6 @@ import { selectFeatureFlags } from "@appsmith/selectors/featureFlagsSelectors"; import { isGACEnabled } from "@appsmith/utils/planHelpers"; import { getHasManageActionPermission } from "@appsmith/utils/BusinessFeatures/permissionPageHelpers"; import type { ChangeQueryPayload } from "actions/queryPaneActions"; -import { createNewApiName, createNewQueryName } from "utils/AppsmithUtils"; -import type { ActionDataState } from "@appsmith/reducers/entityReducers/actionsReducer"; import { getApplicationByIdFromWorkspaces, getCurrentApplicationIdForCreateNewApp, @@ -499,23 +496,14 @@ function* createNewQueryForDatasourceSaga( from: EventLocation; }>, ) { - const { datasourceId, from, pageId } = action.payload; + const { datasourceId, from } = action.payload; if (!datasourceId) return; - const actions: ActionDataState = yield select(getActions); - const datasource: Datasource = yield select(getDatasource, datasourceId); - const plugin: Plugin = yield select(getPlugin, datasource?.pluginId); - const newActionName = - plugin?.type === PluginType.DB - ? createNewQueryName(actions, pageId || "") - : createNewApiName(actions, pageId || ""); - const createActionPayload: Partial = yield call( createDefaultActionPayloadWithPluginDefaults, { datasourceId, from, - newActionName, }, );