fix(content-sidebar): remove dependency on isSignRemoveInterstitialEnabled #3734

diogostavares · 2024-11-04T15:18:04Z

The isSignRemoveInterstitialEnabled feature flag was recently cleaned up on the application side, causing a production defect due to a dependency in this components. This PR removes the dependency in BUIE components to align with the application.

Also uses sign config in the primary props instead of feature props

src/elements/content-sidebar/__tests__/SidebarNavSign.test.tsx

GonchuB

One question and one small followup, if possible (can be another PR)

src/elements/content-sidebar/__tests__/SidebarNavSign.test.tsx

src/elements/content-sidebar/SidebarNavSign.tsx

src/elements/content-sidebar/__tests__/SidebarNavSign.test.tsx

diogostavares · 2025-01-14T10:50:01Z

src/elements/content-sidebar/SidebarNavSignButton.tsx

    const FtuxTooltip = !isSignDisabled && isTargeted ? TargetedClickThroughGuideTooltip : PlaceholderTooltip;
-    const label = intl.formatMessage(status === 'active' ? messages.boxSignSignature : messages.boxSignRequest);


removed this check because status property is not being passed anymore from EUA

greg-in-a-box · 2025-01-16T17:24:00Z

src/elements/content-sidebar/Sidebar.js

@@ -66,6 +67,7 @@ type Props = {
    onVersionChange?: Function,
    onVersionHistoryClick?: Function,
    versionsSidebarProps: VersionsSidebarProps,
+    signSidebarProps: SignSideBarProps,


looks like this is alphabetize, can we alphabetize this prop too

greg-in-a-box · 2025-01-16T17:27:10Z

src/elements/content-sidebar/SidebarNav.js

 }: Props) => {
-    const { enabled: hasBoxSign } = useFeatureConfig('boxSign');
+    const { enabled: hasBoxSign } = signSideBarProps || {};


is there a permission via the user that enables this feature too ?

Yes, there is a user permission controlled by the admin that enables/disables signing. This is reflected in the enabled property sent by EUA

greg-in-a-box

approved but this feature has remain undocumented, normally all props at the root level is usable by 3rd customers sine this feature is only enabled by a flag and additional unknown permissions internally, we cant guarantee the 3rd party customers will be able to use it.

tjuanitas

some minor things I think worth cleaning up

tjuanitas · 2025-02-03T21:51:03Z

src/elements/content-sidebar/Sidebar.js

                                hasDocGen={docGenSidebarProps.isDocGenTemplate}
+                                signSideBarProps={signSidebarProps}
                                isOpen={isOpen}
                                onPanelChange={this.handlePanelChange}


nit:

Suggested change

hasDocGen={docGenSidebarProps.isDocGenTemplate}

signSideBarProps={signSidebarProps}

isOpen={isOpen}

onPanelChange={this.handlePanelChange}

hasDocGen={docGenSidebarProps.isDocGenTemplate}

isOpen={isOpen}

onPanelChange={this.handlePanelChange}

signSidebarProps={signSidebarProps}

want to emphasize typo: signSideBarProps -> signSidebarProps

tjuanitas · 2025-02-03T21:53:05Z

src/elements/content-sidebar/__tests__/SidebarNavSign.test.tsx

-                <SidebarNavSign {...props} />
-            </FeatureProvider>,
-        );
+    const defaultSignSideBarProps = {


nit:
the pattern we usually use for this it to have the renderComponent function take a props parameter that can override the default props. this allows for testing different flows in the component:

const renderComponent = (props = {}) => { const defaultProps = { ... }; render(<SidebarNavSign {...defaultProps} {...props} />); };

tjuanitas · 2025-02-03T21:58:04Z

src/elements/content-sidebar/__tests__/SidebarNavSign.test.tsx

-                isSignRemoveInterstitialEnabled: isRemoveInterstitialEnabled,
-            },
-        };
+    test('should render sign button', async () => {


you don't need async here

tjuanitas · 2025-02-03T21:58:45Z

src/elements/content-sidebar/__tests__/SidebarNavSign.test.tsx

+        renderComponent();
+
+        await userEvent.click(screen.getByRole('button', { name: 'Request Signature' }));


can we check that onClickSignMyself hasn't been called yet before the user click?

tjuanitas · 2025-02-03T21:59:10Z

src/elements/content-sidebar/__tests__/SidebarNavSign.test.tsx

-            boxSign: {
-                isSignRemoveInterstitialEnabled: true,
-            },
-        };
-        const { getByTestId } = renderComponent({}, features);
-        fireEvent.click(getByTestId('sign-button'));
-        expect(getByTestId('sign-request-signature-button')).toBeVisible();
-        expect(getByTestId('sign-sign-myself-button')).toBeVisible();
-    });
+    test('should call correct handler when request signature option is clicked', async () => {
+        renderComponent();

-    test('should call correct handler when request signature option is clicked', () => {
-        const features = {
-            boxSign: {
-                isSignRemoveInterstitialEnabled: true,
-                onClick: onClickRequestSignature,
-            },
-        };
-        const { getByTestId } = renderComponent({}, features);
-        fireEvent.click(getByTestId('sign-button'));
-        fireEvent.click(getByTestId('sign-request-signature-button'));
-        expect(onClickRequestSignature).toBeCalled();
+        await userEvent.click(screen.getByRole('button', { name: 'Request Signature' }));


can we check that onClickRequestSignature hasn't been called yet before the user click?

tjuanitas · 2025-02-03T22:01:35Z

src/elements/content-sidebar/__tests__/SidebarNavSign.test.tsx

    const onClickRequestSignature = jest.fn();
    const onClickSignMyself = jest.fn();


nit: usually we create these locally in the individual tests so that they're not reused across tests. otherwise, it's easy to overlook that if the mocks are not cleared properly, then their implementations and call counts would be carried over between tests.

tjuanitas · 2025-02-03T22:02:34Z

src/elements/content-sidebar/__tests__/SidebarNav.test.js

-        const features = {
-            boxSign: {
+        const props = {
+            signSideBarProps: {


nit

Suggested change

signSideBarProps: {

signSidebarProps: {

tjuanitas · 2025-02-03T22:03:41Z

src/elements/content-sidebar/ContentSidebar.js

@@ -87,6 +88,7 @@ type Props = {
    sharedLink?: string,
    sharedLinkPassword?: string,
    theme?: Theme,
+    signSidebarProps: SignSideBarProps,


Suggested change

signSidebarProps: SignSideBarProps,

signSidebarProps: SignSidebarProps,

tjuanitas · 2025-02-03T22:04:01Z

src/elements/content-sidebar/ContentSidebar.js

            theme,
            versionsSidebarProps,
+            signSidebarProps,


Suggested change

theme,

versionsSidebarProps,

signSidebarProps,

signSidebarProps,

theme,

versionsSidebarProps,

some other locations need some alphabetizing as well. e.g. (line 416 in this file, line 74, 316, 352 in Sidebar.js, etc.)

tjuanitas · 2025-02-03T22:08:16Z

src/elements/content-sidebar/SidebarNavSign.tsx

+    enabled: boolean;
+    onClick: () => void;
+    onClickSignMyself: () => void;
+    targetingApi: TargetingApi | null;


targetingApi is an optional prop in SidebarNavSignButton. can this line align with that?

Suggested change

targetingApi: TargetingApi | null;

targetingApi?: TargetingApi;

diogostavares requested a review from a team as a code owner November 4, 2024 15:18

mergify bot mentioned this pull request Nov 4, 2024

fix(preview): remove dependency on isSignRemoveInterstitialEnabled feature flag #3733

Closed

greg-in-a-box changed the title ~~fix(preview): remove dependency on isSignRemoveInterstitialEnabled~~ fix(content-sidbar): remove dependency on isSignRemoveInterstitialEnabled Nov 4, 2024

greg-in-a-box changed the title ~~fix(content-sidbar): remove dependency on isSignRemoveInterstitialEnabled~~ fix(content-sidebar): remove dependency on isSignRemoveInterstitialEnabled Nov 4, 2024

JChan106 previously approved these changes Nov 4, 2024

View reviewed changes

diogostavares added ready-to-merge and removed ready-to-merge labels Nov 4, 2024

GonchuB reviewed Nov 5, 2024

View reviewed changes

src/elements/content-sidebar/__tests__/SidebarNavSign.test.tsx Outdated Show resolved Hide resolved

GonchuB previously approved these changes Nov 5, 2024

View reviewed changes

diogostavares dismissed JChan106’s stale review via 1020007 November 5, 2024 11:29

diogostavares force-pushed the remove-removeInterstitial-ff-dependency branch from aea92d0 to 1020007 Compare November 5, 2024 11:29

diogostavares requested review from JChan106 and GonchuB November 5, 2024 11:29

diogostavares dismissed GonchuB’s stale review via 1020007 November 5, 2024 17:50

JChan106 previously approved these changes Nov 5, 2024

View reviewed changes

greg-in-a-box requested changes Nov 5, 2024

View reviewed changes

diogostavares dismissed JChan106’s stale review via bdba5eb November 6, 2024 11:51

diogostavares force-pushed the remove-removeInterstitial-ff-dependency branch from 1020007 to bdba5eb Compare November 6, 2024 11:51

diogostavares requested review from greg-in-a-box and JChan106 November 6, 2024 11:59

diogostavares force-pushed the remove-removeInterstitial-ff-dependency branch from bdba5eb to 3b5d387 Compare November 6, 2024 12:08

greg-in-a-box requested changes Nov 6, 2024

View reviewed changes

src/elements/content-sidebar/__tests__/SidebarNavSign.test.tsx Outdated Show resolved Hide resolved

src/elements/content-sidebar/__tests__/SidebarNavSign.test.tsx Outdated Show resolved Hide resolved

diogostavares force-pushed the remove-removeInterstitial-ff-dependency branch from 3b5d387 to a982541 Compare November 6, 2024 16:47

diogostavares requested a review from greg-in-a-box November 6, 2024 16:51

diogostavares force-pushed the remove-removeInterstitial-ff-dependency branch 2 times, most recently from f4855fe to 8fd8f2e Compare November 6, 2024 17:22

diogostavares force-pushed the remove-removeInterstitial-ff-dependency branch from 8fd8f2e to ba55f34 Compare January 10, 2025 09:55

diogostavares requested a review from a team as a code owner January 10, 2025 09:55

diogostavares force-pushed the remove-removeInterstitial-ff-dependency branch 2 times, most recently from b0d2d93 to 8a40819 Compare January 14, 2025 10:48

diogostavares commented Jan 14, 2025

View reviewed changes

diogostavares force-pushed the remove-removeInterstitial-ff-dependency branch 3 times, most recently from cbf2ac3 to f54b7ad Compare January 14, 2025 15:10

greg-in-a-box requested changes Jan 16, 2025

View reviewed changes

diogostavares force-pushed the remove-removeInterstitial-ff-dependency branch from f54b7ad to 8aed68f Compare January 17, 2025 11:23

diogostavares requested a review from greg-in-a-box January 17, 2025 11:36

diogostavares added 2 commits January 30, 2025 15:05

fix(preview): remove dependency on isSignRemoveInterstitialEnabled

dad7d8f

refactor(preview): use signConfig from primary props

60f54fc

diogostavares force-pushed the remove-removeInterstitial-ff-dependency branch from 8aed68f to 60f54fc Compare January 30, 2025 14:08

greg-in-a-box approved these changes Feb 3, 2025

View reviewed changes

tjuanitas reviewed Feb 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(content-sidebar): remove dependency on isSignRemoveInterstitialEnabled #3734

fix(content-sidebar): remove dependency on isSignRemoveInterstitialEnabled #3734

diogostavares commented Nov 4, 2024 •

edited by greg-in-a-box

Loading

GonchuB left a comment •

edited

Loading

diogostavares Jan 14, 2025

greg-in-a-box Jan 16, 2025

diogostavares Jan 17, 2025

greg-in-a-box Jan 16, 2025

diogostavares Jan 17, 2025 •

edited

Loading

greg-in-a-box left a comment

tjuanitas left a comment

tjuanitas Feb 3, 2025

tjuanitas Feb 3, 2025

tjuanitas Feb 3, 2025

tjuanitas Feb 3, 2025

tjuanitas Feb 3, 2025

tjuanitas Feb 3, 2025

tjuanitas Feb 3, 2025

tjuanitas Feb 3, 2025

tjuanitas Feb 3, 2025

tjuanitas Feb 3, 2025

tjuanitas Feb 3, 2025

tjuanitas Feb 3, 2025

		const FtuxTooltip = !isSignDisabled && isTargeted ? TargetedClickThroughGuideTooltip : PlaceholderTooltip;
		const label = intl.formatMessage(status === 'active' ? messages.boxSignSignature : messages.boxSignRequest);

		renderComponent();

		await userEvent.click(screen.getByRole('button', { name: 'Request Signature' }));

		const onClickRequestSignature = jest.fn();
		const onClickSignMyself = jest.fn();

	signSidebarProps: SignSideBarProps,
	signSidebarProps: SignSidebarProps,

	targetingApi: TargetingApi \| null;
	targetingApi?: TargetingApi;

fix(content-sidebar): remove dependency on isSignRemoveInterstitialEnabled #3734

Are you sure you want to change the base?

fix(content-sidebar): remove dependency on isSignRemoveInterstitialEnabled #3734

Conversation

diogostavares commented Nov 4, 2024 • edited by greg-in-a-box Loading

GonchuB left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diogostavares Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

greg-in-a-box left a comment

Choose a reason for hiding this comment

tjuanitas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diogostavares commented Nov 4, 2024 •

edited by greg-in-a-box

Loading

GonchuB left a comment •

edited

Loading

diogostavares Jan 17, 2025 •

edited

Loading