From 8cd84b0e7b657be1b66eacfd7105a4a1be6e06f9 Mon Sep 17 00:00:00 2001 From: Kerry Date: Wed, 17 May 2023 19:52:44 +1200 Subject: [PATCH] Use semantic headings in user settings - integrations and account deletion (#10837) * allow testids in settings sections * use semantic headings in LabsUserSettingsTab * put back margin var * use SettingsTab wrapper * use semantic headings for deactivate acc section * use semantic heading in manage integratios * i18n * explicit cast to boolean * Update src/components/views/settings/shared/SettingsSubsection.tsx Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * test manage integration settings * test deactivate account section display * remove debug * fix cypress test --------- Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- .../general-user-settings-tab.spec.ts | 22 ++-- .../settings/_SetIntegrationManager.pcss | 8 -- .../views/settings/SetIntegrationManager.tsx | 18 ++- .../tabs/user/GeneralUserSettingsTab.tsx | 43 ++++--- src/i18n/strings/en_EN.json | 2 +- .../tabs/user/GeneralUserSettingsTab-test.tsx | 87 +++++++++++++- .../GeneralUserSettingsTab-test.tsx.snap | 106 ++++++++++++++++++ 7 files changed, 232 insertions(+), 54 deletions(-) create mode 100644 test/components/views/settings/tabs/user/__snapshots__/GeneralUserSettingsTab-test.tsx.snap diff --git a/cypress/e2e/settings/general-user-settings-tab.spec.ts b/cypress/e2e/settings/general-user-settings-tab.spec.ts index 96cf9ad184..b78fe390d9 100644 --- a/cypress/e2e/settings/general-user-settings-tab.spec.ts +++ b/cypress/e2e/settings/general-user-settings-tab.spec.ts @@ -43,7 +43,7 @@ describe("General user settings tab", () => { // Exclude userId from snapshots const percyCSS = ".mx_ProfileSettings_profile_controls_userId { visibility: hidden !important; }"; - cy.get(".mx_SettingsTab.mx_GeneralUserSettingsTab").percySnapshotElement("User settings tab - General", { + cy.findByTestId("mx_GeneralUserSettingsTab").percySnapshotElement("User settings tab - General", { percyCSS, // Emulate TabbedView's actual min and max widths // 580: '.mx_UserSettingsDialog .mx_TabbedView' min-width @@ -51,7 +51,7 @@ describe("General user settings tab", () => { widths: [580, 796], }); - cy.get(".mx_SettingsTab.mx_GeneralUserSettingsTab").within(() => { + cy.findByTestId("mx_GeneralUserSettingsTab").within(() => { // Assert that the top heading is rendered cy.findByTestId("general").should("have.text", "General").should("be.visible"); @@ -156,16 +156,10 @@ describe("General user settings tab", () => { // Make sure integration manager's toggle switch is enabled cy.get(".mx_ToggleSwitch_enabled").should("be.visible"); - // Assert space between "Manage integrations" and the integration server address is set to 4px; - cy.get(".mx_SetIntegrationManager_heading_manager").should("have.css", "column-gap", "4px"); - - cy.get(".mx_SetIntegrationManager_heading_manager").within(() => { - cy.get(".mx_SettingsTab_heading").should("have.text", "Manage integrations"); - - // Assert the headings' inline end margin values are set to zero in favor of the column-gap declaration - cy.get(".mx_SettingsTab_heading").should("have.css", "margin-inline-end", "0px"); - cy.get(".mx_SettingsTab_subheading").should("have.css", "margin-inline-end", "0px"); - }); + cy.get(".mx_SetIntegrationManager_heading_manager").should( + "have.text", + "Manage integrations(scalar.vector.im)", + ); }); // Assert the account deactivation button is displayed @@ -178,7 +172,7 @@ describe("General user settings tab", () => { }); it("should support adding and removing a profile picture", () => { - cy.get(".mx_SettingsTab.mx_GeneralUserSettingsTab .mx_ProfileSettings").within(() => { + cy.get(".mx_SettingsTab .mx_ProfileSettings").within(() => { // Upload a picture cy.get(".mx_ProfileSettings_avatarUpload").selectFile("cypress/fixtures/riot.png", { force: true }); @@ -225,7 +219,7 @@ describe("General user settings tab", () => { }); it("should support changing a display name", () => { - cy.get(".mx_SettingsTab.mx_GeneralUserSettingsTab .mx_ProfileSettings").within(() => { + cy.get(".mx_SettingsTab .mx_ProfileSettings").within(() => { // Change the diaplay name to USER_NAME_NEW cy.findByRole("textbox", { name: "Display Name" }).type(`{selectAll}{del}${USER_NAME_NEW}{enter}`); }); diff --git a/res/css/views/settings/_SetIntegrationManager.pcss b/res/css/views/settings/_SetIntegrationManager.pcss index 30feddb043..db511edab4 100644 --- a/res/css/views/settings/_SetIntegrationManager.pcss +++ b/res/css/views/settings/_SetIntegrationManager.pcss @@ -17,20 +17,12 @@ limitations under the License. .mx_SetIntegrationManager { .mx_SettingsFlag { align-items: center; - margin-top: var(--SettingsTab_heading_nth_child-margin-top); .mx_SetIntegrationManager_heading_manager { display: flex; align-items: center; flex-wrap: wrap; column-gap: $spacing-4; - - .mx_SettingsTab_heading, - .mx_SettingsTab_subheading { - margin-top: 0; - margin-bottom: 0; - margin-inline-end: 0; /* Cancel the default right (inline-end) margin */ - } } .mx_ToggleSwitch { diff --git a/src/components/views/settings/SetIntegrationManager.tsx b/src/components/views/settings/SetIntegrationManager.tsx index c2a498d53d..2f0dcaf78c 100644 --- a/src/components/views/settings/SetIntegrationManager.tsx +++ b/src/components/views/settings/SetIntegrationManager.tsx @@ -23,6 +23,8 @@ import { IntegrationManagerInstance } from "../../../integrations/IntegrationMan import SettingsStore from "../../../settings/SettingsStore"; import { SettingLevel } from "../../../settings/SettingLevel"; import ToggleSwitch from "../elements/ToggleSwitch"; +import Heading from "../typography/Heading"; +import { SettingsSubsectionText } from "./shared/SettingsSubsection"; interface IProps {} @@ -70,11 +72,15 @@ export default class SetIntegrationManager extends React.Component + ); } diff --git a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx index d0bbb86b51..5b4ea04d1a 100644 --- a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx +++ b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx @@ -54,6 +54,9 @@ import SetIdServer from "../../SetIdServer"; import SetIntegrationManager from "../../SetIntegrationManager"; import ToggleSwitch from "../../../elements/ToggleSwitch"; import { IS_MAC } from "../../../../../Keyboard"; +import SettingsTab from "../SettingsTab"; +import { SettingsSection } from "../../shared/SettingsSection"; +import SettingsSubsection from "../../shared/SettingsSubsection"; interface IProps { closeSettingsFn: () => void; @@ -492,27 +495,24 @@ export default class GeneralUserSettingsTab extends React.Component - {_t("Account management")} - - {_t("Deactivating your account is a permanent action — be careful!")} - - - {_t("Deactivate Account")} - - + + + + {_t("Deactivate Account")} + + + ); } private renderIntegrationManagerSection(): ReactNode { if (!SettingsStore.getValue(UIFeature.Widgets)) return null; - return ( -
- {/* has its own heading as it includes the current integration manager */} - -
- ); + return ; } public render(): React.ReactNode { @@ -531,12 +531,7 @@ export default class GeneralUserSettingsTab extends React.Component -
{_t("Deactivate account")}
- {this.renderManagementSection()} - - ); + accountManagementSection = this.renderManagementSection(); } let discoverySection; @@ -552,7 +547,7 @@ export default class GeneralUserSettingsTab extends React.Component +
{_t("General")}
@@ -561,9 +556,9 @@ export default class GeneralUserSettingsTab extends React.Component + ); } } diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 52e01c0303..eb1ef2fcb1 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1568,10 +1568,10 @@ "Language and region": "Language and region", "Spell check": "Spell check", "Agree to the identity server (%(serverName)s) Terms of Service to allow yourself to be discoverable by email address or phone number.": "Agree to the identity server (%(serverName)s) Terms of Service to allow yourself to be discoverable by email address or phone number.", + "Deactivate account": "Deactivate account", "Account management": "Account management", "Deactivating your account is a permanent action — be careful!": "Deactivating your account is a permanent action — be careful!", "Deactivate Account": "Deactivate Account", - "Deactivate account": "Deactivate account", "Discovery": "Discovery", "%(brand)s version:": "%(brand)s version:", "Olm version:": "Olm version:", diff --git a/test/components/views/settings/tabs/user/GeneralUserSettingsTab-test.tsx b/test/components/views/settings/tabs/user/GeneralUserSettingsTab-test.tsx index 863f1fbe55..b5a1c2f084 100644 --- a/test/components/views/settings/tabs/user/GeneralUserSettingsTab-test.tsx +++ b/test/components/views/settings/tabs/user/GeneralUserSettingsTab-test.tsx @@ -10,9 +10,11 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */ -import { render } from "@testing-library/react"; + +import { fireEvent, render, screen, within } from "@testing-library/react"; import React from "react"; import { M_AUTHENTICATION } from "matrix-js-sdk/src/matrix"; +import { logger } from "matrix-js-sdk/src/logger"; import GeneralUserSettingsTab from "../../../../../../src/components/views/settings/tabs/user/GeneralUserSettingsTab"; import MatrixClientContext from "../../../../../../src/contexts/MatrixClientContext"; @@ -24,6 +26,8 @@ import { mockPlatformPeg, flushPromises, } from "../../../../../test-utils"; +import { UIFeature } from "../../../../../../src/settings/UIFeature"; +import { SettingLevel } from "../../../../../../src/settings/SettingLevel"; describe("", () => { const defaultProps = { @@ -49,6 +53,8 @@ describe("", () => { mockPlatformPeg(); jest.clearAllMocks(); clientWellKnownSpy.mockReturnValue({}); + jest.spyOn(SettingsStore, "getValue").mockRestore(); + jest.spyOn(logger, "error").mockRestore(); }); it("does not show account management link when not available", () => { @@ -74,4 +80,83 @@ describe("", () => { expect(getByTestId("external-account-management-outer").textContent).toMatch(/.*id\.server\.org/); expect(getByTestId("external-account-management-link").getAttribute("href")).toMatch(accountManagementLink); }); + + describe("Manage integrations", () => { + it("should not render manage integrations section when widgets feature is disabled", () => { + jest.spyOn(SettingsStore, "getValue").mockImplementation( + (settingName) => settingName !== UIFeature.Widgets, + ); + render(getComponent()); + + expect(screen.queryByTestId("mx_SetIntegrationManager")).not.toBeInTheDocument(); + expect(SettingsStore.getValue).toHaveBeenCalledWith(UIFeature.Widgets); + }); + it("should render manage integrations sections", () => { + jest.spyOn(SettingsStore, "getValue").mockImplementation( + (settingName) => settingName === UIFeature.Widgets, + ); + + render(getComponent()); + + expect(screen.getByTestId("mx_SetIntegrationManager")).toMatchSnapshot(); + }); + it("should update integrations provisioning on toggle", () => { + jest.spyOn(SettingsStore, "getValue").mockImplementation( + (settingName) => settingName === UIFeature.Widgets, + ); + jest.spyOn(SettingsStore, "setValue").mockResolvedValue(undefined); + + render(getComponent()); + + const integrationSection = screen.getByTestId("mx_SetIntegrationManager"); + fireEvent.click(within(integrationSection).getByRole("switch")); + + expect(SettingsStore.setValue).toHaveBeenCalledWith( + "integrationProvisioning", + null, + SettingLevel.ACCOUNT, + true, + ); + expect(within(integrationSection).getByRole("switch")).toBeChecked(); + }); + it("handles error when updating setting fails", async () => { + jest.spyOn(SettingsStore, "getValue").mockImplementation( + (settingName) => settingName === UIFeature.Widgets, + ); + jest.spyOn(logger, "error").mockImplementation(() => {}); + + jest.spyOn(SettingsStore, "setValue").mockRejectedValue("oups"); + + render(getComponent()); + + const integrationSection = screen.getByTestId("mx_SetIntegrationManager"); + fireEvent.click(within(integrationSection).getByRole("switch")); + + await flushPromises(); + + expect(logger.error).toHaveBeenCalledWith("Error changing integration manager provisioning"); + expect(logger.error).toHaveBeenCalledWith("oups"); + expect(within(integrationSection).getByRole("switch")).not.toBeChecked(); + }); + }); + + describe("deactive account", () => { + it("should not render section when account deactivation feature is disabled", () => { + jest.spyOn(SettingsStore, "getValue").mockImplementation( + (settingName) => settingName !== UIFeature.Deactivate, + ); + render(getComponent()); + + expect(screen.queryByText("Deactivate account")).not.toBeInTheDocument(); + expect(SettingsStore.getValue).toHaveBeenCalledWith(UIFeature.Deactivate); + }); + it("should render section when account deactivation feature is enabled", () => { + jest.spyOn(SettingsStore, "getValue").mockImplementation( + (settingName) => settingName === UIFeature.Deactivate, + ); + render(getComponent()); + + expect(screen.getByText("Deactivate account").parentElement!).toMatchSnapshot(); + }); + }); }); diff --git a/test/components/views/settings/tabs/user/__snapshots__/GeneralUserSettingsTab-test.tsx.snap b/test/components/views/settings/tabs/user/__snapshots__/GeneralUserSettingsTab-test.tsx.snap new file mode 100644 index 0000000000..20b487cdaf --- /dev/null +++ b/test/components/views/settings/tabs/user/__snapshots__/GeneralUserSettingsTab-test.tsx.snap @@ -0,0 +1,106 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[` Manage integrations should render manage integrations sections 1`] = ` +