From 8046df44d1721eac77629bb2e02d9684a71e1a5c Mon Sep 17 00:00:00 2001 From: Dhruvik Neharia Date: Tue, 26 Sep 2023 07:38:05 +0530 Subject: [PATCH] fix: Fork button text color/hover, and fork=true query param fix (#27336) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description #### PR fixes following issue(s) Fixes #26295 #### 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. - Bug fix (non-breaking change which fixes an issue) - New feature (non-breaking change which adds functionality) - Breaking change (fix or feature that would cause existing functionality to not work as expected) - Chore (housekeeping or task changes that don't impact user perception) - This change requires a documentation update > > > ## 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 --- .../ForkApplicationInDeployedMode_spec.ts | 31 ++++++++++ .../ClientSide/Fork/ForkApplication_spec.ts | 2 +- app/client/cypress/locators/Applications.json | 1 + .../src/pages/AppViewer/PrimaryCTA.test.tsx | 1 + app/client/src/pages/AppViewer/PrimaryCTA.tsx | 52 ++++++++++++++-- .../pages/Applications/ApplicationCard.tsx | 4 +- .../Applications/ForkApplicationModal.tsx | 61 ++++++------------- .../src/pages/Editor/EditorAppName/index.tsx | 4 +- 8 files changed, 107 insertions(+), 49 deletions(-) create mode 100644 app/client/cypress/e2e/Regression/ClientSide/Fork/ForkApplicationInDeployedMode_spec.ts diff --git a/app/client/cypress/e2e/Regression/ClientSide/Fork/ForkApplicationInDeployedMode_spec.ts b/app/client/cypress/e2e/Regression/ClientSide/Fork/ForkApplicationInDeployedMode_spec.ts new file mode 100644 index 0000000000..f7e9552e5d --- /dev/null +++ b/app/client/cypress/e2e/Regression/ClientSide/Fork/ForkApplicationInDeployedMode_spec.ts @@ -0,0 +1,31 @@ +import { + agHelper, + appSettings, + deployMode, + embedSettings, +} from "../../../../support/Objects/ObjectsCore"; +import applicationLocators from "../../../../locators/Applications.json"; + +describe("Fork application in deployed mode", function () { + it("1. Fork modal should open and close", function () { + appSettings.OpenAppSettings(); + appSettings.GoToEmbedSettings(); + embedSettings.ToggleMarkForkable(); + embedSettings.TogglePublicAccess(); + deployMode.DeployApp(); + + cy.url().then((url) => { + const forkableAppUrl = url; + cy.LogOut(); + cy.LogintoApp(Cypress.env("TESTUSERNAME1"), Cypress.env("TESTPASSWORD1")); + cy.visit(forkableAppUrl); + + agHelper.GetNClick(applicationLocators.forkButton); + cy.wait(2000); + agHelper.AssertElementVisibility(applicationLocators.forkModal); + cy.location("search").should("include", "fork=true"); + agHelper.GetNClick(applicationLocators.closeModalPopup); + cy.location("search").should("not.include", "fork=true"); + }); + }); +}); diff --git a/app/client/cypress/e2e/Regression/ClientSide/Fork/ForkApplication_spec.ts b/app/client/cypress/e2e/Regression/ClientSide/Fork/ForkApplication_spec.ts index 740793e61c..495f2361a1 100644 --- a/app/client/cypress/e2e/Regression/ClientSide/Fork/ForkApplication_spec.ts +++ b/app/client/cypress/e2e/Regression/ClientSide/Fork/ForkApplication_spec.ts @@ -117,7 +117,7 @@ describe("Fork application across workspaces", function () { }); }); - it("Mark application as forkable", () => { + it("3. Mark application as forkable", () => { homePage.LogintoApp(Cypress.env("USERNAME"), Cypress.env("PASSWORD")); homePage.CreateNewApplication(); appSettings.OpenAppSettings(); diff --git a/app/client/cypress/locators/Applications.json b/app/client/cypress/locators/Applications.json index 0f22c1f76e..e79a30d28b 100644 --- a/app/client/cypress/locators/Applications.json +++ b/app/client/cypress/locators/Applications.json @@ -1,6 +1,7 @@ { "copyUrl": ".t--copy-url", "forkButton": ".t--fork-app", + "forkModal": ".fork-modal", "shareButton": "button:contains('Share')", "placeholderTxt": "//input[@placeholder='Enter email address(es)']", "placeholderTxtEE": "//input[@placeholder='Enter email address(es) or group(s)']", diff --git a/app/client/src/pages/AppViewer/PrimaryCTA.test.tsx b/app/client/src/pages/AppViewer/PrimaryCTA.test.tsx index a02e52df03..517a42bc63 100644 --- a/app/client/src/pages/AppViewer/PrimaryCTA.test.tsx +++ b/app/client/src/pages/AppViewer/PrimaryCTA.test.tsx @@ -9,6 +9,7 @@ import configureStore from "redux-mock-store"; jest.mock("react-router", () => ({ ...jest.requireActual("react-router"), + useHistory: () => ({ push: jest.fn() }), useLocation: () => ({ pathname: "/app/test-3/page1-63cccd44463c535b9fbc297c", search: "?fork=true", diff --git a/app/client/src/pages/AppViewer/PrimaryCTA.tsx b/app/client/src/pages/AppViewer/PrimaryCTA.tsx index b56d53c1e6..3db8451f65 100644 --- a/app/client/src/pages/AppViewer/PrimaryCTA.tsx +++ b/app/client/src/pages/AppViewer/PrimaryCTA.tsx @@ -1,4 +1,4 @@ -import React, { useMemo, useState } from "react"; +import React, { useEffect, useMemo, useState } from "react"; import { useDispatch, useSelector } from "react-redux"; import Button from "./AppViewerButton"; import { AUTH_LOGIN_URL } from "constants/routes"; @@ -22,7 +22,7 @@ import { getCurrentUser } from "selectors/usersSelectors"; import { ANONYMOUS_USERNAME } from "constants/userConstants"; import ForkApplicationModal from "pages/Applications/ForkApplicationModal"; import { viewerURL } from "RouteBuilder"; -import { useHistory } from "react-router"; +import { useHistory, useLocation } from "react-router"; import { useHref } from "pages/Editor/utils"; import type { NavigationSetting } from "constants/AppConstants"; import { Icon, Tooltip } from "design-system"; @@ -70,6 +70,48 @@ function PrimaryCTA(props: Props) { const [isForkModalOpen, setIsForkModalOpen] = useState(false); const isPreviewMode = useSelector(previewModeSelector); const dispatch = useDispatch(); + const location = useLocation(); + const queryParams = new URLSearchParams(location.search); + + useEffect(() => { + if (queryParams.get("fork") === "true") { + handleForkModalOpen(); + } else { + handleForkModalClose(); + } + }, []); + + const appendOrDeleteForkParam = (appendOrDelete: "append" | "delete") => { + const url = new URL(window.location.href); + + if (appendOrDelete === "append" && !url.searchParams.has("fork")) { + url.searchParams.append("fork", "true"); + history.push(url.toString().slice(url.origin.length)); + } else if (appendOrDelete === "delete" && url.searchParams.has("fork")) { + url.searchParams.delete("fork"); + history.push(url.toString().slice(url.origin.length)); + } + }; + + const handleForkModalOpen = () => { + setIsForkModalOpen(true); + appendOrDeleteForkParam("append"); + }; + + const handleForkModalClose = () => { + setIsForkModalOpen(false); + appendOrDeleteForkParam("delete"); + }; + + useEffect(() => { + // delete "fork" param from url if user is not logged in + if ( + currentApplication?.forkingEnabled && + currentUser?.username === ANONYMOUS_USERNAME + ) { + appendOrDeleteForkParam("delete"); + } + }, [currentApplication?.forkingEnabled, currentUser?.username]); const appViewerURL = useHref(viewerURL, { pageId: currentPageID, @@ -171,15 +213,17 @@ function PrimaryCTA(props: Props) { insideSidebar={insideSidebar} navColorStyle={navColorStyle} onClick={() => { - setIsForkModalOpen(true); + handleForkModalOpen(); }} primaryColor={primaryColor} text={createMessage(FORK_APP)} + varient={ButtonVariantTypes.SECONDARY} /> ); diff --git a/app/client/src/pages/Applications/ApplicationCard.tsx b/app/client/src/pages/Applications/ApplicationCard.tsx index c439dfe018..862259dd0f 100644 --- a/app/client/src/pages/Applications/ApplicationCard.tsx +++ b/app/client/src/pages/Applications/ApplicationCard.tsx @@ -411,8 +411,10 @@ export function ApplicationCard(props: ApplicationCardProps) { { + setForkApplicationModalOpen(false); + }} isModalOpen={isForkApplicationModalopen} - setModalClose={setForkApplicationModalOpen} /> ); diff --git a/app/client/src/pages/Applications/ForkApplicationModal.tsx b/app/client/src/pages/Applications/ForkApplicationModal.tsx index 4f70091bd7..970b22f146 100644 --- a/app/client/src/pages/Applications/ForkApplicationModal.tsx +++ b/app/client/src/pages/Applications/ForkApplicationModal.tsx @@ -17,7 +17,6 @@ import { Option, } from "design-system"; import { ButtonWrapper, SpinnerWrapper } from "./ForkModalStyles"; -import { useLocation } from "react-router"; import { CANCEL, createMessage, @@ -27,7 +26,6 @@ import { FORK_APP_MODAL_SUCCESS_TITLE, } from "@appsmith/constants/messages"; import { getAllApplications } from "@appsmith/actions/applicationActions"; -import history from "utils/history"; type ForkApplicationModalProps = { applicationId: string; @@ -35,12 +33,13 @@ type ForkApplicationModalProps = { // it renders that component trigger?: React.ReactNode; isModalOpen?: boolean; - setModalClose?: (isOpen: boolean) => void; + handleOpen?: () => void; + handleClose?: () => void; isInEditMode?: boolean; }; function ForkApplicationModal(props: ForkApplicationModalProps) { - const { isModalOpen, setModalClose } = props; + const { handleClose, handleOpen, isModalOpen } = props; const [workspace, selectWorkspace] = useState<{ label: string; value: string; @@ -52,20 +51,12 @@ function ForkApplicationModal(props: ForkApplicationModalProps) { ); const isFetchingApplications = useSelector(getIsFetchingApplications); - const location = useLocation(); - const queryParams = new URLSearchParams(location.search); - - useEffect(() => { - if (queryParams.get("fork") === "true") { - handleOpen(); - } - }, []); useEffect(() => { // This effect makes sure that no if // is getting controlled from outside, then we always load workspaces if (isModalOpen) { - handleOpen(); + getApplicationsListAndOpenModal(); return; } }, [isModalOpen]); @@ -73,9 +64,16 @@ function ForkApplicationModal(props: ForkApplicationModalProps) { useEffect(() => { // when we fork from within the appeditor, fork modal remains open // even on the landing page of "Forked" app, this closes it + const url = new URL(window.location.href); const shouldCloseForcibly = - !forkingApplication && isModalOpen && setModalClose; - shouldCloseForcibly && setModalClose(false); + !forkingApplication && + isModalOpen && + handleClose && + !url.searchParams.has("fork"); + + if (shouldCloseForcibly) { + handleClose(); + } }, [forkingApplication]); const forkApplication = () => { @@ -118,41 +116,21 @@ function ForkApplicationModal(props: ForkApplicationModalProps) { ? createMessage(FORK_APP_MODAL_EMPTY_TITLE) : createMessage(FORK_APP_MODAL_SUCCESS_TITLE); - const handleClose = () => { - if (!props.setModalClose) { - const url = new URL(window.location.href); - if (url.searchParams.has("fork")) { - url.searchParams.delete("fork"); - history.push(url.toString().slice(url.origin.length)); - } - } - }; - - const handleOpen = () => { - if (!props.setModalClose) { - const url = new URL(window.location.href); - if (!url.searchParams.has("fork")) { - url.searchParams.append("fork", "true"); - history.push(url.toString().slice(url.origin.length)); - } - } + const getApplicationsListAndOpenModal = () => { !workspaceList.length && dispatch(getAllApplications()); + handleOpen && handleOpen(); }; const handleOnOpenChange = (isOpen: boolean) => { if (isOpen) { - handleOpen(); + getApplicationsListAndOpenModal(); } else { - setModalClose && setModalClose(false); - handleClose(); + handleClose && handleClose(); } }; return ( - + {modalHeading} {isFetchingApplications ? ( @@ -185,8 +163,7 @@ function ForkApplicationModal(props: ForkApplicationModalProps) { isDisabled={forkingApplication} kind="secondary" onClick={() => { - setModalClose && setModalClose(false); - handleClose(); + handleClose && handleClose(); }} size="md" > diff --git a/app/client/src/pages/Editor/EditorAppName/index.tsx b/app/client/src/pages/Editor/EditorAppName/index.tsx index 2307fe14f6..10db95f714 100644 --- a/app/client/src/pages/Editor/EditorAppName/index.tsx +++ b/app/client/src/pages/Editor/EditorAppName/index.tsx @@ -176,9 +176,11 @@ export function EditorAppName(props: EditorAppNameProps) { { + setForkApplicationModalOpen(false); + }} isInEditMode isModalOpen={isForkApplicationModalopen} - setModalClose={setForkApplicationModalOpen} /> ) : null;