From 32bcd94e544f8ed02eed20e7cb2afb9df459917b Mon Sep 17 00:00:00 2001 From: Parthvi12 <80334441+Parthvi12@users.noreply.github.com> Date: Wed, 18 May 2022 10:15:32 +0530 Subject: [PATCH 1/6] Add tests for git discard changes (#13866) --- .../GitDiscardChange/DiscardChanges_spec.js | 228 ++++++++++++++++++ .../cypress/locators/gitSyncLocators.js | 1 + app/client/cypress/support/gitSync.js | 34 +++ app/client/cypress/support/widgetCommands.js | 11 +- 4 files changed, 273 insertions(+), 1 deletion(-) create mode 100644 app/client/cypress/integration/Smoke_TestSuite/ClientSideTests/GitDiscardChange/DiscardChanges_spec.js diff --git a/app/client/cypress/integration/Smoke_TestSuite/ClientSideTests/GitDiscardChange/DiscardChanges_spec.js b/app/client/cypress/integration/Smoke_TestSuite/ClientSideTests/GitDiscardChange/DiscardChanges_spec.js new file mode 100644 index 0000000000..b5cae1240c --- /dev/null +++ b/app/client/cypress/integration/Smoke_TestSuite/ClientSideTests/GitDiscardChange/DiscardChanges_spec.js @@ -0,0 +1,228 @@ +const datasource = require("../../../../locators/DatasourcesEditor.json"); +const queryLocators = require("../../../../locators/QueryEditor.json"); +const dynamicInputLocators = require("../../../../locators/DynamicInput.json"); +const explorer = require("../../../../locators/explorerlocators.json"); + +describe("Git discard changes:", function() { + let datasourceName; + let repoName; + const query1 = "get_users"; + const query2 = "get_allusers"; + const jsObject = "JSObject1"; + const page2 = "Page_2"; + const page3 = "Page_3"; + + it("1. Create an app with Query1 and JSObject1, connect it to git", () => { + // Create new postgres datasource + cy.NavigateToDatasourceEditor(); + cy.get(datasource.PostgreSQL).click(); + + cy.getPluginFormsAndCreateDatasource(); + + cy.fillPostgresDatasourceForm(); + + cy.testSaveDatasource(); + + cy.get("@createDatasource").then((httpResponse) => { + datasourceName = httpResponse.response.body.data.name; + + cy.get(datasource.datasourceCard) + .contains(datasourceName) + .scrollIntoView() + .should("be.visible") + .closest(datasource.datasourceCard) + .within(() => { + cy.get(datasource.createQuerty).click(); + }); + }); + // Create new postgres query + cy.get(queryLocators.queryNameField).type(`${query1}`); + cy.get(queryLocators.switch) + .last() + .click({ force: true }); + cy.get(queryLocators.templateMenu).click(); + cy.get(queryLocators.query).click({ force: true }); + cy.get(".CodeMirror textarea") + .first() + .focus() + .type("SELECT * FROM users ORDER BY id LIMIT 10;", { + force: true, + parseSpecialCharSequences: false, + }); + cy.WaitAutoSave(); + cy.runQuery(); + + cy.CheckAndUnfoldEntityItem("PAGES"); + cy.wait(1000); + cy.get(".t--entity-item:contains(Page1)") + .first() + .click(); + cy.wait("@getPage"); + // bind input widget to postgres query on page1 + cy.get(explorer.addWidget).click(); + cy.dragAndDropToCanvas("inputwidgetv2", { x: 300, y: 300 }); + cy.get(".t--widget-inputwidgetv2").should("exist"); + cy.get(dynamicInputLocators.input) + .eq(1) + .click({ force: true }) + .type(`{{${query1}.data[0].name}}`, { + parseSpecialCharSequences: false, + }); + cy.wait(2000); + cy.CheckAndUnfoldEntityItem("PAGES"); + cy.Createpage(page2); + cy.wait(1000); + cy.get(`.t--entity-item:contains(${page2})`) + .first() + .click(); + cy.wait("@getPage"); + cy.createJSObject('return "Success";'); + cy.get(explorer.addWidget).click(); + // bind input widget to JSObject response on page2 + cy.dragAndDropToCanvas("inputwidgetv2", { x: 300, y: 300 }); + cy.get(".t--widget-inputwidgetv2").should("exist"); + cy.get(dynamicInputLocators.input) + .eq(1) + .click({ force: true }) + .type("{{JSObject1.myFun1()}}", { parseSpecialCharSequences: false }); + cy.get("#switcher--explorer").click({ force: true }); + // connect app to git + cy.generateUUID().then((uid) => { + repoName = uid; + + cy.createTestGithubRepo(repoName); + cy.connectToGitRepo(repoName); + }); + }); + + it("2. Add new datasource query, discard changes, verify query is deleted", () => { + cy.get(`.t--entity-item:contains("Page1")`) + .first() + .click(); + cy.wait("@getPage"); + // create new postgres query + cy.NavigateToQueryEditor(); + cy.NavigateToActiveTab(); + cy.get(datasource.datasourceCard) + .contains(datasourceName) + .scrollIntoView() + .should("be.visible") + .closest(datasource.datasourceCard) + .within(() => { + cy.get(datasource.createQuerty).click(); + }); + cy.get(queryLocators.queryNameField).type(`${query2}`); + cy.get(queryLocators.switch) + .last() + .click({ force: true }); + cy.get(queryLocators.templateMenu).click(); + cy.get(queryLocators.query).click({ force: true }); + cy.get(".CodeMirror textarea") + .first() + .focus() + .type("SELECT * FROM users;", { + force: true, + parseSpecialCharSequences: false, + }); + cy.WaitAutoSave(); + cy.runQuery(); + // navoigate to Page1 + cy.get(`.t--entity-item:contains(Page1)`) + .first() + .click(); + cy.wait("@getPage"); + // discard changes + cy.gitDiscardChanges(); + cy.CheckAndUnfoldEntityItem("QUERIES/JS"); + // verify query2 is not present + cy.get(`.t--entity-name:contains(${query2})`).should("not.exist"); + }); + + it("3. Add new JSObject , discard changes verify JSObject is deleted", () => { + cy.createJSObject('return "Success";'); + cy.CheckAndUnfoldEntityItem("QUERIES/JS"); + // verify jsObject is not duplicated + cy.get(`.t--entity-name:contains(${jsObject})`).should("have.length", 1); + cy.gitDiscardChanges(); + cy.CheckAndUnfoldEntityItem("QUERIES/JS"); + // verify jsObject2 is deleted after discarding changes + cy.get(`.t--entity-name:contains(${jsObject})`).should("not.exist"); + }); + + it("4. Delete page2 and trigger discard flow, page2 should be available again", () => { + cy.Deletepage(page2); + // verify page is deleted + cy.CheckAndUnfoldEntityItem("PAGES"); + cy.get(`.t--entity-name:contains(${page2})`).should("not.exist"); + cy.gitDiscardChanges(); + // verify page2 is recovered back + cy.get(`.t--entity-name:contains(${page2})`).should("be.visible"); + cy.get(`.t--entity-item:contains(${page2})`) + .first() + .click(); + cy.wait("@getPage"); + // verify data binding on page2 + cy.get(".bp3-input").should("have.value", "Success"); + }); + + it("5. Delete Query1 and trigger discard flow, Query1 will be recovered", () => { + // navigate to Page1 + cy.CheckAndUnfoldEntityItem("PAGES"); + cy.get(`.t--entity-item:contains("Page1")`) + .first() + .click(); + cy.wait("@getPage"); + // delete query1 + cy.deleteQueryOrJS(query1); + // verify Query1 is deleted + cy.get(`.t--entity-name:contains(${query1})`).should("not.exist"); + // discard changes + cy.gitDiscardChanges(); + //verify query1 is recovered + cy.get(`.t--entity-name:contains(${query1})`).should("be.visible"); + + cy.get(".bp3-input").should("have.value", "Test user 7"); + }); + + it("6. Delete JSObject1 and trigger discard flow, JSObject1 should be active again", () => { + // navigate to page2 + cy.CheckAndUnfoldEntityItem("PAGES"); + cy.get(`.t--entity-item:contains(${page2})`) + .first() + .click(); + cy.wait("@getPage"); + // delete jsObject1 + cy.CheckAndUnfoldEntityItem("QUERIES/JS"); + cy.get(`.t--entity-item:contains(${jsObject})`).within(() => { + cy.get(".t--context-menu").click({ force: true }); + }); + cy.selectAction("Delete"); + cy.selectAction("Are you sure?"); + cy.get(`.t--entity-name:contains(${jsObject})`).should("not.exist"); + // discard changes + cy.gitDiscardChanges(); + //verify JSObject is recovered + cy.get(`.t--entity-name:contains(${jsObject})`).should("be.visible"); + cy.get(".bp3-input").should("have.value", "Success"); + }); + + it("7. Add new page i.e page3, go to page2 & discard changes, verify page3 is removed", () => { + // create new page page3 and move to page1 + cy.Createpage(page3); + cy.get(`.t--entity-item:contains(${page2})`) + .first() + .click(); + // discard changes + cy.gitDiscardChanges(); + // verify page3 is removed + cy.CheckAndUnfoldEntityItem("PAGES"); + cy.get(`.t--entity-name:contains("${page3}")`).should("not.exist"); + }); + + it("8. Add new page i.e page3, discard changes should give error resource not found", () => { + cy.Createpage(page3); + cy.gitDiscardChanges(false); + cy.go("back"); + cy.reload(); + }); +}); diff --git a/app/client/cypress/locators/gitSyncLocators.js b/app/client/cypress/locators/gitSyncLocators.js index 9f0b480c63..91964ee618 100644 --- a/app/client/cypress/locators/gitSyncLocators.js +++ b/app/client/cypress/locators/gitSyncLocators.js @@ -51,4 +51,5 @@ export default { gitPullCount: ".t--bottom-bar-pull .count", gitConnectionContainer: "[data-test=t--git-connection-container]", gitRemoteURLContainer: "[data-test=t--remote-url-container]", + discardChanges: ".t--discard-button", }; diff --git a/app/client/cypress/support/gitSync.js b/app/client/cypress/support/gitSync.js index 95f605264f..c2772250fa 100644 --- a/app/client/cypress/support/gitSync.js +++ b/app/client/cypress/support/gitSync.js @@ -363,3 +363,37 @@ Cypress.Commands.add( }); }, ); + +Cypress.Commands.add("gitDiscardChanges", (assertResourceFound = true) => { + cy.get(gitSyncLocators.bottomBarCommitButton).click(); + //cy.intercept("GET", "/api/v1/git/status/*").as("gitStatus"); + // cy.wait("@gitStatus").should( + // "have.nested.property", + // "response.body.responseMeta.status", + // 200, + // ); + cy.get(gitSyncLocators.discardChanges) + .children() + .should("have.text", "Discard changes"); + + cy.get(gitSyncLocators.discardChanges).click(); + cy.contains(Cypress.env("MESSAGES").DISCARD_CHANGES_WARNING()); + + cy.get(gitSyncLocators.discardChanges) + .children() + .should("have.text", "Are you sure?"); + cy.get(gitSyncLocators.discardChanges).click(); + cy.contains(Cypress.env("MESSAGES").DISCARDING_AND_PULLING_CHANGES()); + if (assertResourceFound) { + cy.wait("@applications").should( + "have.nested.property", + "response.body.responseMeta.status", + 200, + ); + cy.validateToastMessage("Discarded changes successfully."); + } else { + cy.get(".bold-text").should(($x) => { + expect($x).contain("Page not found"); + }); + } +}); diff --git a/app/client/cypress/support/widgetCommands.js b/app/client/cypress/support/widgetCommands.js index 532c0a3565..1d93f05f5b 100644 --- a/app/client/cypress/support/widgetCommands.js +++ b/app/client/cypress/support/widgetCommands.js @@ -1109,7 +1109,16 @@ Cypress.Commands.add("clearPropertyValue", (value) => { // eslint-disable-next-line cypress/no-unnecessary-waiting cy.wait(1000); }); - +Cypress.Commands.add("deleteQueryOrJS", (Action) => { + cy.CheckAndUnfoldEntityItem("QUERIES/JS"); + cy.get(`.t--entity-item:contains(${Action})`).within(() => { + cy.get(".t--context-menu").click({ force: true }); + }); + cy.selectAction("Delete"); + cy.selectAction("Are you sure?"); + cy.wait("@deleteAction"); + cy.get("@deleteAction").should("have.property", "status", 200); +}); Cypress.Commands.add( "validateNSelectDropdown", (ddTitle, currentValue, newValue) => { From 94b93a657ba5152f69189addb936287b1a3c9362 Mon Sep 17 00:00:00 2001 From: Bhavin K <58818598+techbhavin@users.noreply.github.com> Date: Wed, 18 May 2022 10:16:25 +0530 Subject: [PATCH 2/6] fix: remove cell background from empty rows (#13257) --- .../Table_EmptyRow_Color_spec.js | 52 +++++++++++++++++++ app/client/cypress/support/widgetCommands.js | 4 +- .../TableWidget/component/TableUtilities.tsx | 24 ++++++--- 3 files changed, 72 insertions(+), 8 deletions(-) create mode 100644 app/client/cypress/integration/Smoke_TestSuite/ClientSideTests/DisplayWidgets/Table_EmptyRow_Color_spec.js diff --git a/app/client/cypress/integration/Smoke_TestSuite/ClientSideTests/DisplayWidgets/Table_EmptyRow_Color_spec.js b/app/client/cypress/integration/Smoke_TestSuite/ClientSideTests/DisplayWidgets/Table_EmptyRow_Color_spec.js new file mode 100644 index 0000000000..fa8a8f8652 --- /dev/null +++ b/app/client/cypress/integration/Smoke_TestSuite/ClientSideTests/DisplayWidgets/Table_EmptyRow_Color_spec.js @@ -0,0 +1,52 @@ +const widgetsPage = require("../../../../locators/Widgets.json"); +const dsl = require("../../../../fixtures/tableNewDsl.json"); +const commonlocators = require("../../../../locators/commonlocators.json"); + +describe("Table Widget empty row color validation", function() { + before(() => { + cy.addDsl(dsl); + }); + + it("1. Validate cell background of columns", function() { + // Open property pane + cy.openPropertyPane("tablewidget"); + // give general color to all table row + cy.selectColor("cellbackgroundcolor", -17); + + cy.editColumn("id"); + // Click on cell background color + cy.selectColor("cellbackground", -27); + cy.wait("@updateLayout"); + cy.get(commonlocators.editPropBackButton).click({ force: true }); + + cy.editColumn("email"); + cy.selectColor("cellbackground", -33); + cy.wait("@updateLayout"); + cy.get(commonlocators.editPropBackButton).click({ force: true }); + + // Verify the cell background color of first column + cy.readTabledataValidateCSS( + "1", + "0", + "background-color", + "rgb(99, 102, 241)", + ); + // Verify the cell background color of second column + cy.readTabledataValidateCSS( + "1", + "1", + "background-color", + "rgb(30, 58, 138)", + ); + }); + it("2. Validate empty row background", function() { + // first cell of first row should be transparent + cy.get( + ".t--widget-tablewidget .tbody div[data-cy='empty-row-0-cell-0']", + ).should("have.css", "background-color", "rgb(99, 102, 241)"); + // second cell of first row should be transparent + cy.get( + ".t--widget-tablewidget .tbody div[data-cy='empty-row-0-cell-1']", + ).should("have.css", "background-color", "rgb(30, 58, 138)"); + }); +}); diff --git a/app/client/cypress/support/widgetCommands.js b/app/client/cypress/support/widgetCommands.js index 1d93f05f5b..8b322c0d84 100644 --- a/app/client/cypress/support/widgetCommands.js +++ b/app/client/cypress/support/widgetCommands.js @@ -424,7 +424,7 @@ Cypress.Commands.add("updateCodeInput", ($selector, value) => { }); }); -Cypress.Commands.add("selectColor", (GivenProperty) => { +Cypress.Commands.add("selectColor", (GivenProperty, colorOffset = -15) => { // Property pane of the widget is opened, and click given property. cy.get( ".t--property-control-" + GivenProperty + " .bp3-input-group input", @@ -433,7 +433,7 @@ Cypress.Commands.add("selectColor", (GivenProperty) => { }); cy.get(widgetsPage.colorPickerV2Color) - .eq(-15) + .eq(colorOffset) .then(($elem) => { cy.get($elem).click({ force: true }); }); diff --git a/app/client/src/widgets/TableWidget/component/TableUtilities.tsx b/app/client/src/widgets/TableWidget/component/TableUtilities.tsx index 34cb1e0a21..1f341de23b 100644 --- a/app/client/src/widgets/TableWidget/component/TableUtilities.tsx +++ b/app/client/src/widgets/TableWidget/component/TableUtilities.tsx @@ -24,7 +24,7 @@ import { TableStyles, MenuItems, } from "./Constants"; -import { isString, isEmpty, findIndex, isNil, isNaN } from "lodash"; +import { isString, isEmpty, findIndex, isNil, isNaN, get, set } from "lodash"; import PopoverVideo from "widgets/VideoWidget/component/PopoverVideo"; import AutoToolTipComponent from "widgets/TableWidget/component/AutoToolTipComponent"; import { ControlIcons } from "icons/ControlIcons"; @@ -532,11 +532,19 @@ export const renderEmptyRows = ( renderCheckBoxCell(false, accentColor, borderRadius)} {row.cells.map((cell: any, cellIndex: number) => { const cellProps = cell.getCellProps(); - if (columns[0]?.columnProperties?.cellBackground) { - cellProps.style.background = - columns[0].columnProperties.cellBackground; - } - return
; + set( + cellProps, + "style.backgroundColor", + get(cell, "column.columnProperties.cellBackground"), + ); + return ( +
+ ); })}
); @@ -568,6 +576,10 @@ export const renderEmptyRows = ( width: column.width + "px", boxSizing: "border-box", flex: `${column.width} 0 auto`, + backgroundColor: get( + column, + "columnProperties.cellBackground", + ), }} /> ); From 1396141af47f40abdb432a04949ed5caf0e0a469 Mon Sep 17 00:00:00 2001 From: Ankita Kinger Date: Wed, 18 May 2022 10:23:08 +0530 Subject: [PATCH 3/6] chore: optimize naming conventions for variables and functions (#13858) * optimize naming conventions for variables and functions * update import statement * minor css change --- .../src/ce/pages/AdminSettings/LeftPane.tsx | 4 +-- .../src/ce/pages/AdminSettings/Main.tsx | 2 +- .../config/authentication/index.tsx | 28 +++++++++---------- .../form/fields/DropdownWrapper.tsx | 4 ++- .../form/fields/SelectField.tsx | 4 ++- .../src/pages/Settings/FormGroup/Dropdown.tsx | 2 +- .../src/pages/Settings/SettingsForm.tsx | 4 +-- app/client/src/pages/Settings/components.ts | 4 +-- 8 files changed, 28 insertions(+), 24 deletions(-) diff --git a/app/client/src/ce/pages/AdminSettings/LeftPane.tsx b/app/client/src/ce/pages/AdminSettings/LeftPane.tsx index dae1d94640..9446fea0d2 100644 --- a/app/client/src/ce/pages/AdminSettings/LeftPane.tsx +++ b/app/client/src/ce/pages/AdminSettings/LeftPane.tsx @@ -55,7 +55,7 @@ export const StyledLink = styled(Link)<{ $active: boolean }>` } `; -export function useSettingsCategory() { +export function getSettingsCategory() { return Array.from(AdminConfig.categories); } @@ -109,7 +109,7 @@ export function Categories({ } export default function LeftPane() { - const categories = useSettingsCategory(); + const categories = getSettingsCategory(); const { category, subCategory } = useParams() as any; return ( diff --git a/app/client/src/ce/pages/AdminSettings/Main.tsx b/app/client/src/ce/pages/AdminSettings/Main.tsx index 2f3e59207a..b5591b667b 100644 --- a/app/client/src/ce/pages/AdminSettings/Main.tsx +++ b/app/client/src/ce/pages/AdminSettings/Main.tsx @@ -11,7 +11,7 @@ const Main = () => { const wrapperCategory = AdminConfig.wrapperCategories[subCategory ?? category]; - if (!!wrapperCategory && !!wrapperCategory.component) { + if (!!wrapperCategory?.component) { const { component: WrapperCategoryComponent } = wrapperCategory; return ; } else if ( diff --git a/app/client/src/ce/pages/AdminSettings/config/authentication/index.tsx b/app/client/src/ce/pages/AdminSettings/config/authentication/index.tsx index 6549643031..cab5a2c906 100644 --- a/app/client/src/ce/pages/AdminSettings/config/authentication/index.tsx +++ b/app/client/src/ce/pages/AdminSettings/config/authentication/index.tsx @@ -25,7 +25,7 @@ const { enableGoogleOAuth, } = getAppsmithConfigs(); -const Form_Auth: AdminConfigType = { +const FormAuth: AdminConfigType = { type: SettingCategories.FORM_AUTH, controlType: SettingTypes.GROUP, title: "Form Login", @@ -65,7 +65,7 @@ const Form_Auth: AdminConfigType = { ], }; -const Google_Auth: AdminConfigType = { +const GoogleAuth: AdminConfigType = { type: SettingCategories.GOOGLE_AUTH, controlType: SettingTypes.GROUP, title: "Google Authentication", @@ -111,7 +111,7 @@ const Google_Auth: AdminConfigType = { ], }; -const Github_Auth: AdminConfigType = { +const GithubAuth: AdminConfigType = { type: SettingCategories.GITHUB_AUTH, controlType: SettingTypes.GROUP, title: "Github Authentication", @@ -149,7 +149,7 @@ const Github_Auth: AdminConfigType = { ], }; -export const Form_Auth_Callout: AuthMethodType = { +export const FormAuthCallout: AuthMethodType = { id: "APPSMITH_FORM_LOGIN_AUTH", category: SettingCategories.FORM_AUTH, label: "Form Login", @@ -159,7 +159,7 @@ export const Form_Auth_Callout: AuthMethodType = { isConnected: !disableLoginForm, }; -export const Google_Auth_Callout: AuthMethodType = { +export const GoogleAuthCallout: AuthMethodType = { id: "APPSMITH_GOOGLE_AUTH", category: SettingCategories.GOOGLE_AUTH, label: "Google", @@ -170,7 +170,7 @@ export const Google_Auth_Callout: AuthMethodType = { isConnected: enableGoogleOAuth, }; -export const Github_Auth_Callout: AuthMethodType = { +export const GithubAuthCallout: AuthMethodType = { id: "APPSMITH_GITHUB_AUTH", category: SettingCategories.GITHUB_AUTH, label: "Github", @@ -181,7 +181,7 @@ export const Github_Auth_Callout: AuthMethodType = { isConnected: enableGithubOAuth, }; -export const Saml_Auth_Callout: AuthMethodType = { +export const SamlAuthCallout: AuthMethodType = { id: "APPSMITH_SAML_AUTH", category: "saml", label: "SAML 2.0", @@ -191,7 +191,7 @@ export const Saml_Auth_Callout: AuthMethodType = { type: "OTHER", }; -export const Oidc_Auth_Callout: AuthMethodType = { +export const OidcAuthCallout: AuthMethodType = { id: "APPSMITH_OIDC_AUTH", category: "oidc", label: "OIDC", @@ -202,11 +202,11 @@ export const Oidc_Auth_Callout: AuthMethodType = { }; const AuthMethods = [ - Oidc_Auth_Callout, - Saml_Auth_Callout, - Google_Auth_Callout, - Github_Auth_Callout, - Form_Auth_Callout, + OidcAuthCallout, + SamlAuthCallout, + GoogleAuthCallout, + GithubAuthCallout, + FormAuthCallout, ]; function AuthMain() { @@ -218,6 +218,6 @@ export const config: AdminConfigType = { controlType: SettingTypes.PAGE, title: "Authentication", canSave: false, - children: [Form_Auth, Google_Auth, Github_Auth], + children: [FormAuth, GoogleAuth, GithubAuth], component: AuthMain, }; diff --git a/app/client/src/components/editorComponents/form/fields/DropdownWrapper.tsx b/app/client/src/components/editorComponents/form/fields/DropdownWrapper.tsx index 704db3bfdc..2ff5d34a24 100644 --- a/app/client/src/components/editorComponents/form/fields/DropdownWrapper.tsx +++ b/app/client/src/components/editorComponents/form/fields/DropdownWrapper.tsx @@ -7,7 +7,8 @@ type DropdownWrapperProps = { value?: string; onChange?: (value?: string) => void; }; - options: Array<{ id: string; value: string; label: string }>; + options: Array<{ id: string; value: string; label?: string }>; + fillOptions?: boolean; }; function DropdownWrapper(props: DropdownWrapperProps) { @@ -28,6 +29,7 @@ function DropdownWrapper(props: DropdownWrapperProps) { return ( ; + options: Array<{ id: string; value: string; label?: string }>; size?: "large" | "small"; outline?: boolean; + fillOptions?: boolean; }; export function SelectField(props: SelectFieldProps) { return ( ` + margin-bottom: ${(props) => props.margin ?? `16px`}; `; export const SettingsHeader = styled.h2` From 33c78e93ef4b4d365e396df5c6f978730aee2792 Mon Sep 17 00:00:00 2001 From: Nayan Date: Wed, 18 May 2022 11:08:13 +0600 Subject: [PATCH 4/6] feat: Update organization slug (#13791) When organization name is updated, the slug is not updated. This generates a outdated URL when user clicks on a organization name from the left panel. This PR changes the following behaviors The Organization slug will be no more unique Link to organization applications will be based on organization id instead of slug Organization slug will be updated whenever there is a change in organization name All the existing organization slugs will be updated --- app/client/src/pages/Applications/index.tsx | 14 ++-- app/client/src/sagas/OrgSagas.ts | 4 +- .../appsmith/server/domains/Organization.java | 8 -- .../server/migrations/DatabaseChangelog.java | 28 ++----- .../server/migrations/DatabaseChangelog2.java | 27 +++++++ .../ce/CustomOrganizationRepositoryCE.java | 2 - .../CustomOrganizationRepositoryCEImpl.java | 25 ------- .../services/ce/OrganizationServiceCE.java | 4 - .../ce/OrganizationServiceCEImpl.java | 33 +++------ .../server/services/ce/UserServiceCEImpl.java | 2 +- .../services/OrganizationServiceTest.java | 74 +++++++++++++------ .../server/services/UserServiceTest.java | 10 +-- .../UserServiceWithDisabledSignupTest.java | 23 +----- 13 files changed, 111 insertions(+), 143 deletions(-) diff --git a/app/client/src/pages/Applications/index.tsx b/app/client/src/pages/Applications/index.tsx index 2cd13f0bb3..ad36911bae 100644 --- a/app/client/src/pages/Applications/index.tsx +++ b/app/client/src/pages/Applications/index.tsx @@ -357,9 +357,9 @@ function OrgMenuItem({ isFetchingApplications, org, selected }: any) { isFetchingApplications ? BlueprintClasses.SKELETON : "" } ellipsize={20} - href={`${window.location.pathname}#${org.organization.slug}`} + href={`${window.location.pathname}#${org.organization.id}`} icon="workspace" - key={org.organization.slug} + key={org.organization.id} ref={menuRef} selected={selected} text={org.organization.name} @@ -419,9 +419,9 @@ function LeftPane() { userOrgs.map((org: any) => ( ))} @@ -652,7 +652,7 @@ function ApplicationsSection(props: any) { {(currentUser || isFetchingApplications) && OrgMenuTarget({ orgName: organization.name, - orgSlug: organization.slug, + orgSlug: organization.id, })} {hasManageOrgPermissions && ( { setOrgToOpenMenu(null); }} @@ -753,7 +753,7 @@ function ApplicationsSection(props: any) { className="t--options-icon" name="context-menu" onClick={() => { - setOrgToOpenMenu(organization.slug); + setOrgToOpenMenu(organization.id); }} size={IconSize.XXXL} /> diff --git a/app/client/src/sagas/OrgSagas.ts b/app/client/src/sagas/OrgSagas.ts index 3d3cb068d8..d0c5276140 100644 --- a/app/client/src/sagas/OrgSagas.ts +++ b/app/client/src/sagas/OrgSagas.ts @@ -258,8 +258,8 @@ export function* createOrgSaga( } // get created org in focus - const slug = response.data.slug; - history.push(`${window.location.pathname}#${slug}`); + const orgId = response.data.id; + history.push(`${window.location.pathname}#${orgId}`); } catch (error) { yield call(reject, { _error: error.message }); yield put({ diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Organization.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Organization.java index e01ba2be85..7101399bbf 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Organization.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Organization.java @@ -42,14 +42,6 @@ public class Organization extends BaseDomain { @JsonIgnore private String logoAssetId; - public String makeSlug() { - return toSlug(name); - } - - public static String toSlug(String text) { - return text == null ? null : text.replaceAll("[^\\w\\d]+", "-").toLowerCase(); - } - public String getLogoUrl() { return Url.ASSET_URL + "/" + logoAssetId; } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog.java index d9ce643c7e..56b0dbd79f 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog.java @@ -69,7 +69,6 @@ import com.appsmith.server.dtos.OrganizationPluginStatus; import com.appsmith.server.dtos.PageDTO; import com.appsmith.server.helpers.GitDeployKeyGenerator; import com.appsmith.server.helpers.TextUtils; -import com.appsmith.server.services.OrganizationService; import com.fasterxml.jackson.databind.ObjectMapper; import com.github.cloudyrock.mongock.ChangeLog; import com.github.cloudyrock.mongock.ChangeSet; @@ -311,23 +310,11 @@ public class DatabaseChangelog { dropIndexIfExists(mongoTemplate, Organization.class, "name"); } - @ChangeSet(order = "003", id = "add-org-slugs", author = "") - public void addOrgSlugs(MongockTemplate mongoTemplate, OrganizationService organizationService) { - // For all existing organizations, add a slug field, which should be unique. - // We are blocking here for adding a slug to each existing organization. This is bad and slow. Do NOT copy this - // code fragment into the services' control flow. This is a single migration code and is expected to run once in - // lifetime of a deployment. - for (Organization organization : mongoTemplate.findAll(Organization.class)) { - if (organization.getSlug() == null) { - organizationService.getNextUniqueSlug(organization.makeSlug()) - .doOnSuccess(slug -> { - organization.setSlug(slug); - mongoTemplate.save(organization); - }) - .block(); - } - } - } + /* + * @ChangeSet(order = "003", id = "add-org-slugs", author = "") + * This migration has been removed as it's no more required. A newer version of this migration has been added + * in the @ChangeLog(order = "002") with id="update-organization-slugs" + */ /** * We are creating indexes manually because Spring's index resolver creates indexes on fields as well. @@ -368,11 +355,6 @@ public class DatabaseChangelog { makeIndex("email").unique() ); - ensureIndexes(mongoTemplate, Organization.class, - createdAtIndex, - makeIndex("slug").unique() - ); - ensureIndexes(mongoTemplate, Page.class, createdAtIndex, makeIndex("applicationId", "name").unique().named("application_page_compound_index") diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java index 033c427186..93feb9cb79 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java @@ -10,14 +10,17 @@ import com.appsmith.server.domains.Application; import com.appsmith.server.domains.ApplicationPage; import com.appsmith.server.domains.NewAction; import com.appsmith.server.domains.NewPage; +import com.appsmith.server.domains.Organization; import com.appsmith.server.domains.Plugin; import com.appsmith.server.domains.QApplication; import com.appsmith.server.domains.QNewAction; import com.appsmith.server.domains.QNewPage; +import com.appsmith.server.domains.QOrganization; import com.appsmith.server.domains.QPlugin; import com.appsmith.server.dtos.ActionDTO; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; +import com.appsmith.server.helpers.TextUtils; import com.github.cloudyrock.mongock.ChangeLog; import com.github.cloudyrock.mongock.ChangeSet; import com.github.cloudyrock.mongock.driver.mongodb.springdata.v3.decorator.impl.MongockTemplate; @@ -746,4 +749,28 @@ public class DatabaseChangelog2 { ); } + /** + * We'll remove the uniqe index on organization slugs. We'll also regenerate the slugs for all organizations as + * most of them are outdated + * @param mongockTemplate MongockTemplate instance + */ + @ChangeSet(order = "008", id = "update-organization-slugs", author = "") + public void updateOrganizationSlugs(MongockTemplate mongockTemplate) { + dropIndexIfExists(mongockTemplate, Organization.class, "slug"); + + // update organizations + final Query getAllOrganizationsQuery = query(where("deletedAt").is(null)); + getAllOrganizationsQuery.fields() + .include(fieldName(QOrganization.organization.name)); + + List organizations = mongockTemplate.find(getAllOrganizationsQuery, Organization.class); + + for (Organization organization : organizations) { + mongockTemplate.updateFirst( + query(where(fieldName(QOrganization.organization.id)).is(organization.getId())), + new Update().set(fieldName(QOrganization.organization.slug), TextUtils.makeSlug(organization.getName())), + Organization.class + ); + } + } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomOrganizationRepositoryCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomOrganizationRepositoryCE.java index 8c96718c7e..0f125725dc 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomOrganizationRepositoryCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomOrganizationRepositoryCE.java @@ -15,8 +15,6 @@ public interface CustomOrganizationRepositoryCE extends AppsmithRepository findByIdsIn(Set orgIds, AclPermission aclPermission, Sort sort); - Mono nextSlugNumber(String slugPrefix); - Mono updateUserRoleNames(String userId, String userName); Flux findAllOrganizations(); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomOrganizationRepositoryCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomOrganizationRepositoryCEImpl.java index e2b36d19e7..233373c2ba 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomOrganizationRepositoryCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomOrganizationRepositoryCEImpl.java @@ -16,10 +16,8 @@ import reactor.core.publisher.Mono; import java.util.List; import java.util.Set; -import java.util.stream.Collectors; import static org.springframework.data.mongodb.core.query.Criteria.where; -import static org.springframework.data.mongodb.core.query.Query.query; @Slf4j public class CustomOrganizationRepositoryCEImpl extends BaseAppsmithRepositoryImpl @@ -43,29 +41,6 @@ public class CustomOrganizationRepositoryCEImpl extends BaseAppsmithRepositoryIm return queryAll(List.of(orgIdsCriteria), aclPermission, sort); } - @Override - public Mono nextSlugNumber(String slugPrefix) { - final String slugField = fieldName(QOrganization.organization.slug); - final Query slugPrefixQuery = query(where(slugField).regex("^" + slugPrefix + "\\d*$")); - slugPrefixQuery.fields().include(slugField); - return mongoOperations - .find(slugPrefixQuery, Organization.class) - .map(Organization::getSlug) - .collect(Collectors.toSet()) - .map(slugs -> { - if (slugs.isEmpty() || !slugs.contains(slugPrefix)) { - return 0L; - } - - long number = 1L; - while (slugs.contains(slugPrefix + number)) { - ++number; - } - - return number; - }); - } - @Override public Mono updateUserRoleNames(String userId, String userName) { return mongoOperations diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCE.java index 67af1ac865..d5355a3e11 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCE.java @@ -17,10 +17,6 @@ public interface OrganizationServiceCE extends CrudService Mono create(Organization organization); - Mono getBySlug(String slug); - - Mono getNextUniqueSlug(String initialSlug); - Mono createDefault(Organization organization, User user); Mono create(Organization organization, User user); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java index 3f5d803bb8..72f1e7bd93 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java @@ -14,6 +14,7 @@ import com.appsmith.server.domains.UserRole; import com.appsmith.server.dtos.OrganizationPluginStatus; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; +import com.appsmith.server.helpers.TextUtils; import com.appsmith.server.repositories.ApplicationRepository; import com.appsmith.server.repositories.AssetRepository; import com.appsmith.server.repositories.OrganizationRepository; @@ -103,17 +104,6 @@ public class OrganizationServiceCEImpl extends BaseService getBySlug(String slug) { - return repository.findBySlug(slug); - } - - @Override - public Mono getNextUniqueSlug(String initialSlug) { - return repository.nextSlugNumber(initialSlug) - .map(number -> initialSlug + (number == 0 ? "" : number)); - } - /** * Creates the given organization as a default organization for the given user. That is, the organization's name * is changed to "[username]'s apps" and then created. The current value of the organization name @@ -160,19 +150,9 @@ public class OrganizationServiceCEImpl extends BaseService setSlugMono; - if (organization.getName() == null) { - setSlugMono = Mono.just(organization); - } else { - setSlugMono = getNextUniqueSlug(organization.makeSlug()) - .map(slug -> { - organization.setSlug(slug); - return organization; - }); - } + organization.setSlug(TextUtils.makeSlug(organization.getName())); - return setSlugMono - .flatMap(this::validateObject) + return validateObject(organization) // Install all the default plugins when the org is created /* TODO: This is a hack. We should ideally use the pluginService.installPlugin() function. Not using it right now because of circular dependency b/w organizationService and pluginService @@ -226,6 +206,10 @@ public class OrganizationServiceCEImpl extends BaseService { AppsmithBeanUtils.copyNewFieldValuesIntoOldObject(resource, existingOrganization); @@ -248,6 +232,9 @@ public class OrganizationServiceCEImpl extends BaseService save(Organization organization) { + if(StringUtils.hasLength(organization.getName())) { + organization.setSlug(TextUtils.makeSlug(organization.getName())); + } return repository.save(organization); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java index a6d2ad0c6c..b38b9f1891 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java @@ -909,7 +909,7 @@ public class UserServiceCEImpl extends BaseService if (isNewUser) { params.put("inviteUrl", inviteUrl); } else { - params.put("inviteUrl", inviteUrl + "/applications#" + organization.getSlug()); + params.put("inviteUrl", inviteUrl + "/applications#" + organization.getId()); } return params; } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/OrganizationServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/OrganizationServiceTest.java index ad35ad57d3..a1b780e027 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/OrganizationServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/OrganizationServiceTest.java @@ -16,6 +16,7 @@ import com.appsmith.server.domains.UserRole; import com.appsmith.server.dtos.InviteUsersDTO; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; +import com.appsmith.server.helpers.TextUtils; import com.appsmith.server.repositories.AssetRepository; import com.appsmith.server.repositories.DatasourceRepository; import com.appsmith.server.repositories.OrganizationRepository; @@ -49,6 +50,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.UUID; import java.util.stream.Collectors; import static com.appsmith.server.acl.AclPermission.EXECUTE_DATASOURCES; @@ -196,7 +198,7 @@ public class OrganizationServiceTest { assertThat(organization1.getName()).isEqualTo("Test Name"); assertThat(organization1.getPolicies()).isNotEmpty(); assertThat(organization1.getPolicies()).containsAll(Set.of(manageOrgAppPolicy, manageOrgPolicy)); - assertThat(organization1.getSlug() != null); + assertThat(organization1.getSlug()).isEqualTo(TextUtils.makeSlug(organization.getName())); assertThat(organization1.getEmail()).isEqualTo("api_user"); assertThat(organization1.getIsAutoGeneratedOrganization()).isNull(); }) @@ -315,25 +317,7 @@ public class OrganizationServiceTest { @Test @WithUserDetails(value = "api_user") - public void uniqueSlugs() { - Organization organization = new Organization(); - organization.setName("Slug org"); - organization.setDomain("example.com"); - organization.setWebsite("https://example.com"); - - Mono uniqueSlug = organizationService.create(organization) - .flatMap(org -> organizationService.getNextUniqueSlug(org.getSlug())); - - StepVerifier.create(uniqueSlug) - .assertNext(slug -> { - assertThat(slug).isEqualTo("slug-org1"); - }) - .verifyComplete(); - } - - @Test - @WithUserDetails(value = "api_user") - public void createDuplicateNameOrganization() { + public void create_WhenNameIsDuplicated_CreatedWithDuplicateName() { Organization firstOrg = new Organization(); firstOrg.setName("Really good org"); firstOrg.setDomain("example.com"); @@ -349,8 +333,11 @@ public class OrganizationServiceTest { StepVerifier.create(Mono.zip(firstOrgCreation, secondOrgCreation)) .assertNext(orgsTuple -> { + assertThat(orgsTuple.getT1().getName()).isEqualTo("Really good org"); assertThat(orgsTuple.getT1().getSlug()).isEqualTo("really-good-org"); - assertThat(orgsTuple.getT2().getSlug()).isEqualTo("really-good-org1"); + + assertThat(orgsTuple.getT2().getName()).isEqualTo("Really good org"); + assertThat(orgsTuple.getT2().getSlug()).isEqualTo("really-good-org"); }) .verifyComplete(); } @@ -1183,4 +1170,49 @@ public class OrganizationServiceTest { .create(deleteOrgMono) .verifyComplete(); } + + @Test + @WithUserDetails(value = "api_user") + public void save_WhenNameIsPresent_SlugGenerated() { + String uniqueString = UUID.randomUUID().toString(); // to make sure name is not conflicted with other tests + Organization organization = new Organization(); + organization.setName("My Organization " + uniqueString); + + String finalName = "Renamed Organization " + uniqueString; + + Mono organizationMono = organizationService.create(organization) + .flatMap(savedOrg -> { + savedOrg.setName(finalName); + return organizationService.save(savedOrg); + }); + + StepVerifier.create(organizationMono) + .assertNext(savedOrganization -> { + assertThat(savedOrganization.getSlug()).isEqualTo(TextUtils.makeSlug(finalName)); + }) + .verifyComplete(); + } + + @Test + @WithUserDetails(value = "api_user") + public void update_WhenNameIsNotPresent_SlugIsNotGenerated() { + String uniqueString = UUID.randomUUID().toString(); // to make sure name is not conflicted with other tests + String initialName = "My Organization " + uniqueString; + Organization organization = new Organization(); + organization.setName(initialName); + + Mono organizationMono = organizationService.create(organization) + .flatMap(savedOrg -> { + Organization orgDto = new Organization(); + orgDto.setWebsite("https://appsmith.com"); + return organizationService.update(savedOrg.getId(), orgDto); + }); + + StepVerifier.create(organizationMono) + .assertNext(savedOrganization -> { + // slug should be unchanged + assertThat(savedOrganization.getSlug()).isEqualTo(TextUtils.makeSlug(initialName)); + }) + .verifyComplete(); + } } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java index f32b1956ae..4320818d5d 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java @@ -51,6 +51,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.UUID; import static com.appsmith.server.acl.AclPermission.MANAGE_APPLICATIONS; import static com.appsmith.server.acl.AclPermission.MANAGE_USERS; @@ -94,15 +95,12 @@ public class UserServiceTest { Mono userMono; - Mono organizationMono; - @Autowired UserSignup userSignup; @Before public void setup() { userMono = userService.findByEmail("usertest@usertest.com"); - organizationMono = organizationService.getBySlug("spring-test-organization"); } //Test if email params are updating correctly @@ -110,13 +108,13 @@ public class UserServiceTest { public void checkEmailParamsForExistingUser() { Organization organization = new Organization(); organization.setName("UserServiceTest Update Org"); - organization.setSlug("userservicetest-update-org"); + organization.setId(UUID.randomUUID().toString()); User inviter = new User(); inviter.setName("inviterUserToApplication"); String inviteUrl = "http://localhost:8080"; - String expectedUrl = inviteUrl + "/applications#userservicetest-update-org"; + String expectedUrl = inviteUrl + "/applications#" + organization.getId(); Map params = userService.getEmailParams(organization, inviter, inviteUrl, false); assertEquals(expectedUrl, params.get("inviteUrl")); @@ -127,8 +125,8 @@ public class UserServiceTest { @Test public void checkEmailParamsForNewUser() { Organization organization = new Organization(); + organization.setId(UUID.randomUUID().toString()); organization.setName("UserServiceTest Update Org"); - organization.setSlug("userservicetest-update-org"); User inviter = new User(); inviter.setName("inviterUserToApplication"); diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceWithDisabledSignupTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceWithDisabledSignupTest.java index 93664264a0..af38c8eed4 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceWithDisabledSignupTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceWithDisabledSignupTest.java @@ -2,24 +2,20 @@ package com.appsmith.server.services; import com.appsmith.external.models.Policy; import com.appsmith.server.acl.AppsmithRole; -import com.appsmith.server.configurations.CommonConfig; import com.appsmith.server.configurations.WithMockAppsmithUser; import com.appsmith.server.domains.Application; import com.appsmith.server.domains.InviteUser; import com.appsmith.server.domains.LoginSource; -import com.appsmith.server.domains.Organization; import com.appsmith.server.domains.User; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.repositories.UserRepository; -import com.appsmith.server.solutions.UserSignup; import lombok.extern.slf4j.Slf4j; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; -import org.springframework.security.crypto.password.PasswordEncoder; import org.springframework.security.test.context.support.WithUserDetails; import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.junit4.SpringRunner; @@ -42,32 +38,17 @@ public class UserServiceWithDisabledSignupTest { @Autowired UserService userService; - @Autowired - OrganizationService organizationService; - @Autowired ApplicationService applicationService; @Autowired UserRepository userRepository; - @Autowired - PasswordEncoder passwordEncoder; - - @Autowired - CommonConfig commonConfig; - Mono userMono; - Mono organizationMono; - - @Autowired - UserSignup userSignup; - @Before public void setup() { userMono = userService.findByEmail("usertest@usertest.com"); - organizationMono = organizationService.getBySlug("spring-test-organization"); } @Test @@ -232,8 +213,8 @@ public class UserServiceWithDisabledSignupTest { StepVerifier.create(userMono) .assertNext(user -> { - assertThat(user.getEmail().equals(newUser.getEmail())); - assertThat(user.getSource().equals(LoginSource.FORM)); + assertThat(user.getEmail()).isEqualTo(newUser.getEmail()); + assertThat(user.getSource()).isEqualTo(LoginSource.FORM); assertThat(user.getIsEnabled()).isTrue(); }) .verifyComplete(); From 8dc9c0b017cdc4d5ae48c36b634217f5e104521a Mon Sep 17 00:00:00 2001 From: Satish Gandham Date: Wed, 18 May 2022 10:51:53 +0530 Subject: [PATCH 5/6] feat: Optimize create uneval tree via memoization (#13476) * - Memoize the expeinsive parts of createUnevalTree * - Split the generateDataTreeWidget and memoize the expensize part - Remove stary console statement in MultiSelectWidget * - Some debugging code, needs to be removed. * Debugging in progress * Still debugging * Still debugging * Working?? * Jest tests passing, and fast enough * remove unused imports * One failing jest test, testing luck with cypress * Remcoe console statment from jest test * - Sync with release. - Comment failing jest test * Disable a test * - Clean up * Final cleanup and documentation * - Make function and variable names accurate and easy to understand * Update code comments Co-authored-by: Satish Gandham --- app/client/package.json | 1 + .../src/entities/DataTree/dataTreeFactory.ts | 24 +++++ .../entities/DataTree/dataTreeWidget.test.ts | 1 - .../src/entities/DataTree/dataTreeWidget.ts | 97 ++++++++++++++----- app/client/src/entities/Widget/utils.ts | 14 ++- .../MultiSelectWidget/component/index.tsx | 1 - app/client/yarn.lock | 5 + 7 files changed, 114 insertions(+), 29 deletions(-) diff --git a/app/client/package.json b/app/client/package.json index fcbf51b9e7..c2a34709c5 100644 --- a/app/client/package.json +++ b/app/client/package.json @@ -83,6 +83,7 @@ "mammoth": "^1.4.19", "marked": "^2.0.0", "memoize-one": "^5.2.1", + "micro-memoize": "^4.0.10", "moment": "^2.24.0", "moment-timezone": "^0.5.27", "nanoid": "^2.0.4", diff --git a/app/client/src/entities/DataTree/dataTreeFactory.ts b/app/client/src/entities/DataTree/dataTreeFactory.ts index ed77cb5145..c2fda4897a 100644 --- a/app/client/src/entities/DataTree/dataTreeFactory.ts +++ b/app/client/src/entities/DataTree/dataTreeFactory.ts @@ -23,6 +23,7 @@ import { } from "entities/DataTree/actionTriggers"; import { AppTheme } from "entities/AppTheming"; import { PluginId } from "api/PluginApi"; +import log from "loglevel"; export type ActionDispatcher = ( ...args: any[] @@ -164,6 +165,9 @@ export class DataTreeFactory { widgetsMeta, }: DataTreeSeed): DataTree { const dataTree: DataTree = {}; + const start = performance.now(); + const startActions = performance.now(); + actions.forEach((action) => { const editorConfig = editorConfigs[action.config.pluginId]; const dependencyConfig = pluginDependencyConfig[action.config.pluginId]; @@ -173,15 +177,24 @@ export class DataTreeFactory { dependencyConfig, ); }); + const endActions = performance.now(); + + const startJsActions = performance.now(); + jsActions.forEach((js) => { dataTree[js.config.name] = generateDataTreeJSAction(js); }); + const endJsActions = performance.now(); + + const startWidgets = performance.now(); + Object.values(widgets).forEach((widget) => { dataTree[widget.widgetName] = generateDataTreeWidget( widget, widgetsMeta[widget.widgetId], ); }); + const endWidgets = performance.now(); dataTree.pageList = pageList; dataTree.appsmith = { @@ -192,6 +205,17 @@ export class DataTreeFactory { theme, } as DataTreeAppsmith; (dataTree.appsmith as DataTreeAppsmith).ENTITY_TYPE = ENTITY_TYPE.APPSMITH; + const end = performance.now(); + + const out = { + total: end - start, + widgets: endWidgets - startWidgets, + actions: endActions - startActions, + jsActions: endJsActions - startJsActions, + }; + + log.debug("### Create unevalTree timing", out); + return dataTree; } } diff --git a/app/client/src/entities/DataTree/dataTreeWidget.test.ts b/app/client/src/entities/DataTree/dataTreeWidget.test.ts index a4fb85e407..f935ebadc4 100644 --- a/app/client/src/entities/DataTree/dataTreeWidget.test.ts +++ b/app/client/src/entities/DataTree/dataTreeWidget.test.ts @@ -289,7 +289,6 @@ describe("generateDataTreeWidget", () => { privateWidgets: {}, deepObj: { level1: { - value: 10, metaValue: 10, }, }, diff --git a/app/client/src/entities/DataTree/dataTreeWidget.ts b/app/client/src/entities/DataTree/dataTreeWidget.ts index 93e24c86dc..4e0b31f20c 100644 --- a/app/client/src/entities/DataTree/dataTreeWidget.ts +++ b/app/client/src/entities/DataTree/dataTreeWidget.ts @@ -1,22 +1,28 @@ -import WidgetFactory from "utils/WidgetFactory"; import { getAllPathsFromPropertyConfig } from "entities/Widget/utils"; -import { getEntityDynamicBindingPathList } from "utils/DynamicBindingUtils"; import _ from "lodash"; - +import memoize from "micro-memoize"; import { FlattenedWidgetProps } from "reducers/entityReducers/canvasWidgetsReducer"; -import { setOverridingProperty } from "./utils"; +import { getEntityDynamicBindingPathList } from "utils/DynamicBindingUtils"; +import WidgetFactory from "utils/WidgetFactory"; import { - OverridingPropertyPaths, - PropertyOverrideDependency, - OverridingPropertyType, DataTreeWidget, ENTITY_TYPE, + OverridingPropertyPaths, + OverridingPropertyType, + PropertyOverrideDependency, } from "./dataTreeFactory"; +import { setOverridingProperty } from "./utils"; -export const generateDataTreeWidget = ( +// We are splitting generateDataTreeWidget into two parts to memoize better as the widget doesn't change very often. +// Widget changes only when dynamicBindingPathList changes. +// Only meta properties change very often, for example typing in an input or selecting a table row. +const generateDataTreeWidgetWithoutMeta = ( widget: FlattenedWidgetProps, - widgetMetaProps: Record = {}, -): DataTreeWidget => { +): { + dataTreeWidgetWithoutMetaProps: DataTreeWidget; + overridingMetaPropsMap: Record; + defaultMetaProps: Record; +} => { const derivedProps: any = {}; const blockedDerivedProps: Record = {}; const unInitializedDefaultProps: Record = {}; @@ -94,16 +100,6 @@ export const generateDataTreeWidget = ( }, ); - const overridingMetaProps: Record = {}; - - // overridingMetaProps has all meta property value either from metaReducer or default set by widget whose dependent property also has default property. - Object.entries(defaultMetaProps).forEach(([key, value]) => { - if (overridingMetaPropsMap[key]) { - overridingMetaProps[key] = - key in widgetMetaProps ? widgetMetaProps[key] : value; - } - }); - const { bindingPaths, reactivePaths, @@ -134,12 +130,12 @@ export const generateDataTreeWidget = ( * * Therefore spread is replaced with "merge" which merges objects recursively. */ - return _.merge( + const dataTreeWidgetWithoutMetaProps = _.merge( {}, widget, unInitializedDefaultProps, defaultMetaProps, - widgetMetaProps, + // widgetMetaProps, derivedProps, { defaultProps, @@ -149,7 +145,7 @@ export const generateDataTreeWidget = ( ...widget.logBlackList, ...blockedDerivedProps, }, - meta: _.merge(overridingMetaProps, widgetMetaProps), + // meta: _.merge(overridingMetaProps, widgetMetaProps), propertyOverrideDependency, overridingPropertyPaths, bindingPaths, @@ -162,4 +158,59 @@ export const generateDataTreeWidget = ( }, }, ); + return { + dataTreeWidgetWithoutMetaProps, + overridingMetaPropsMap, + defaultMetaProps, + }; +}; + +// @todo set the max size dynamically based on number of widgets. (widgets.length) +// Remove the debug statements in July 2022 +const generateDataTreeWidgetWithoutMetaMemoized = memoize( + generateDataTreeWidgetWithoutMeta, + { + maxSize: 1000, + // onCacheHit: (cache, options) => { + // console.log("####### cache was hit: ", cache.keys.length); + // }, + // onCacheAdd: (cache, options) => { + // console.log( + // "####### cache was missed ", + // cache.keys.length, + // cache.keys[0][0].widgetName, + // ); + // }, + }, +); + +export const generateDataTreeWidget = ( + widget: FlattenedWidgetProps, + widgetMetaProps: Record = {}, +) => { + const { + dataTreeWidgetWithoutMetaProps: dataTreeWidget, + defaultMetaProps, + overridingMetaPropsMap, + } = generateDataTreeWidgetWithoutMetaMemoized(widget); + const overridingMetaProps: Record = {}; + + // overridingMetaProps has all meta property value either from metaReducer or default set by widget whose dependent property also has default property. + Object.entries(defaultMetaProps).forEach(([key, value]) => { + if (overridingMetaPropsMap[key]) { + overridingMetaProps[key] = + key in widgetMetaProps ? widgetMetaProps[key] : value; + } + }); + + const meta = _.merge(overridingMetaProps, widgetMetaProps); + + Object.entries(widgetMetaProps).forEach(([key, value]) => { + // Since meta properties are always updated as a whole, we are replacing instead of merging. + // Merging mutates the memoized value, avoid merging meta values + dataTreeWidget[key] = value; + }); + + dataTreeWidget["meta"] = meta; + return dataTreeWidget; }; diff --git a/app/client/src/entities/Widget/utils.ts b/app/client/src/entities/Widget/utils.ts index f7696aaf32..b5b924341a 100644 --- a/app/client/src/entities/Widget/utils.ts +++ b/app/client/src/entities/Widget/utils.ts @@ -1,11 +1,12 @@ -import { WidgetProps } from "widgets/BaseWidget"; import { PropertyPaneConfig, ValidationConfig, } from "constants/PropertyControlConstants"; -import { get, isObject, isUndefined, omitBy } from "lodash"; -import { FlattenedWidgetProps } from "reducers/entityReducers/canvasWidgetsReducer"; import { EvaluationSubstitutionType } from "entities/DataTree/dataTreeFactory"; +import { get, isObject, isUndefined, omitBy } from "lodash"; +import memoize from "micro-memoize"; +import { FlattenedWidgetProps } from "reducers/entityReducers/canvasWidgetsReducer"; +import { WidgetProps } from "widgets/BaseWidget"; /** * @typedef {Object} Paths @@ -163,7 +164,7 @@ const childHasPanelConfig = ( return { reactivePaths, triggerPaths, validationPaths, bindingPaths }; }; -export const getAllPathsFromPropertyConfig = ( +const getAllPathsFromPropertyConfigWithoutMemo = ( widget: WidgetProps, widgetConfig: readonly PropertyPaneConfig[], defaultProperties: Record, @@ -276,6 +277,11 @@ export const getAllPathsFromPropertyConfig = ( return { reactivePaths, triggerPaths, validationPaths, bindingPaths }; }; +export const getAllPathsFromPropertyConfig = memoize( + getAllPathsFromPropertyConfigWithoutMemo, + { maxSize: 1000 }, +); + /** * this function gets the next available row for pasting widgets * NOTE: this function excludes modal widget when calculating next available row diff --git a/app/client/src/widgets/MultiSelectWidget/component/index.tsx b/app/client/src/widgets/MultiSelectWidget/component/index.tsx index 1f0bd17707..9f8548b4dd 100644 --- a/app/client/src/widgets/MultiSelectWidget/component/index.tsx +++ b/app/client/src/widgets/MultiSelectWidget/component/index.tsx @@ -164,7 +164,6 @@ function MultiSelectComponent({ }, []); const id = _.uniqueId(); - console.log("dropDownWidth", dropDownWidth); return ( Date: Wed, 18 May 2022 12:03:09 +0530 Subject: [PATCH 6/6] Added selected state for query/js add button (#13897) Adds selected state to Query/JS add button in explorer when submenu is open. --- app/client/src/pages/Editor/Explorer/Entity/AddButton.tsx | 8 ++++++++ app/client/src/pages/Editor/Explorer/Files/Submenu.tsx | 5 ++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/app/client/src/pages/Editor/Explorer/Entity/AddButton.tsx b/app/client/src/pages/Editor/Explorer/Entity/AddButton.tsx index 09ec10883d..c39cc5d633 100644 --- a/app/client/src/pages/Editor/Explorer/Entity/AddButton.tsx +++ b/app/client/src/pages/Editor/Explorer/Entity/AddButton.tsx @@ -15,6 +15,14 @@ const Wrapper = styled(EntityTogglesWrapper)` } } } + &.selected { + background: ${Colors.SHARK2} !important; + svg { + path { + fill: ${Colors.WHITE} !important; + } + } + } `; const PlusIcon = ControlIcons.INCREASE_CONTROL_V2; diff --git a/app/client/src/pages/Editor/Explorer/Files/Submenu.tsx b/app/client/src/pages/Editor/Explorer/Files/Submenu.tsx index 74abce51d3..e699705fc6 100644 --- a/app/client/src/pages/Editor/Explorer/Files/Submenu.tsx +++ b/app/client/src/pages/Editor/Explorer/Files/Submenu.tsx @@ -236,7 +236,10 @@ export default function ExplorerSubMenu({ hoverOpenDelay={TOOLTIP_HOVER_ON_DELAY} position={Position.RIGHT} > - setShow(true)} /> + setShow(true)} + /> );