Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dad7d8f
fix(preview): remove dependency on isSignRemoveInterstitialEnabled
diogostavares Nov 6, 2024
60f54fc
refactor(preview): use signConfig from primary props
diogostavares Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions i18n/en-US.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,8 +356,6 @@ be.contentSidebar.boxSignRequestSignature = Request Signature
be.contentSidebar.boxSignSecurityBlockedTooltip = This action is unavailable due to a security policy.
# One of the dropdown options that opens a Box Sign sign myself experience
be.contentSidebar.boxSignSignMyself = Sign Myself
# label for button that opens a Box Sign signature fulfillment experience
be.contentSidebar.boxSignSignature = Sign
# Tooltip text for when Box Sign is blocked due to an item being watermarked
be.contentSidebar.boxSignWatermarkBlockedTooltip = This action is unavailable, because the file is watermarked.
# title for when editing an existing approval task
Expand Down
4 changes: 4 additions & 0 deletions src/elements/content-sidebar/ContentSidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import type { DetailsSidebarProps } from './DetailsSidebar';
import type { DocGenSidebarProps } from './DocGenSidebar/DocGenSidebar';
import type { MetadataSidebarProps } from './MetadataSidebar';
import type { VersionsSidebarProps } from './versions';
import type { SignSideBarProps } from './SidebarNavSign';
import type { WithLoggerProps } from '../../common/types/logging';
import type { ElementsXhrError, RequestOptions, ErrorContextProps } from '../../common/types/api';
import type { MetadataEditor } from '../../common/types/metadata';
Expand Down Expand Up @@ -87,6 +88,7 @@ type Props = {
sharedLink?: string,
sharedLinkPassword?: string,
theme?: Theme,
signSidebarProps: SignSideBarProps,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
signSidebarProps: SignSideBarProps,
signSidebarProps: SignSidebarProps,

token: Token,
versionsSidebarProps: VersionsSidebarProps,
} & ErrorContextProps &
Expand Down Expand Up @@ -368,6 +370,7 @@ class ContentSidebar extends React.Component<Props, State> {
onVersionHistoryClick,
theme,
versionsSidebarProps,
signSidebarProps,
Comment on lines 371 to +373
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
theme,
versionsSidebarProps,
signSidebarProps,
signSidebarProps,
theme,
versionsSidebarProps,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}: Props = this.props;
const { file, isLoading, metadataEditors }: State = this.state;
const initialPath = defaultView.charAt(0) === '/' ? defaultView : `/${defaultView}`;
Expand Down Expand Up @@ -410,6 +413,7 @@ class ContentSidebar extends React.Component<Props, State> {
onVersionHistoryClick={onVersionHistoryClick}
theme={theme}
versionsSidebarProps={versionsSidebarProps}
signSidebarProps={signSidebarProps}
wrappedComponentRef={ref => {
this.sidebarRef = ref;
}}
Expand Down
4 changes: 4 additions & 0 deletions src/elements/content-sidebar/Sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import type { VersionsSidebarProps } from './versions';
import type { AdditionalSidebarTab } from './flowTypes';
import type { MetadataEditor } from '../../common/types/metadata';
import type { BoxItem, User } from '../../common/types/core';
import type { SignSideBarProps } from './SidebarNavSign';
import type { Errors } from '../common/flowTypes';
// $FlowFixMe TypeScript file
import type { Theme } from '../common/theming';
Expand Down Expand Up @@ -70,6 +71,7 @@ type Props = {
onVersionChange?: Function,
onVersionHistoryClick?: Function,
theme?: Theme,
signSidebarProps: SignSideBarProps,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

versionsSidebarProps: VersionsSidebarProps,
};

Expand Down Expand Up @@ -311,6 +313,7 @@ class Sidebar extends React.Component<Props, State> {
onVersionChange,
theme,
versionsSidebarProps,
signSidebarProps,
}: Props = this.props;
const isOpen = this.isOpen();
const hasBoxAI = SidebarUtils.canHaveBoxAISidebar(this.props);
Expand Down Expand Up @@ -346,6 +349,7 @@ class Sidebar extends React.Component<Props, State> {
hasMetadata={hasMetadata}
hasSkills={hasSkills}
hasDocGen={docGenSidebarProps.isDocGenTemplate}
signSideBarProps={signSidebarProps}
isOpen={isOpen}
onPanelChange={this.handlePanelChange}
Comment on lines 351 to 354
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

want to emphasize typo: signSideBarProps -> signSidebarProps

/>
Expand Down
7 changes: 5 additions & 2 deletions src/elements/content-sidebar/SidebarNav.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
import { useFeatureConfig } from '../common/feature-checking';
import type { NavigateOptions, AdditionalSidebarTab } from './flowTypes';
import './SidebarNav.scss';
import type { SignSideBarProps } from './SidebarNavSign';

type Props = {
additionalTabs?: Array<AdditionalSidebarTab>,
Expand All @@ -49,6 +50,7 @@ type Props = {
isOpen?: boolean,
onNavigate?: (SyntheticEvent<>, NavigateOptions) => void,
onPanelChange?: (name: string, isInitialState: boolean) => void,
signSideBarProps: SignSideBarProps,
};

const SidebarNav = ({
Expand All @@ -66,8 +68,9 @@ const SidebarNav = ({
isOpen,
onNavigate,
onPanelChange = noop,
signSideBarProps,
}: Props) => {
const { enabled: hasBoxSign } = useFeatureConfig('boxSign');
const { enabled: hasBoxSign } = signSideBarProps || {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@diogostavares diogostavares Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const { disabledTooltip: boxAIDisabledTooltip, showOnlyNavButton: showOnlyBoxAINavButton } =
useFeatureConfig('boxai.sidebar');

Expand Down Expand Up @@ -159,7 +162,7 @@ const SidebarNav = ({

{hasBoxSign && (
<div className="bcs-SidebarNav-secondary">
<SidebarNavSign />
<SidebarNavSign {...signSideBarProps} />
</div>
)}

Expand Down
65 changes: 29 additions & 36 deletions src/elements/content-sidebar/SidebarNavSign.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import * as React from 'react';
import { FormattedMessage } from 'react-intl';

// @ts-ignore Module is written in Flow
import { useFeatureConfig } from '../common/feature-checking';
// @ts-ignore Module is written in Flow
import { SIDEBAR_NAV_TARGETS } from '../common/interactionTargets';

Expand All @@ -17,48 +15,43 @@ import { Menu, MenuItem } from '../../components/menu';
import messages from './messages';

import './SidebarNavSign.scss';
// @ts-ignore Module is written in Flow
import type { TargetingApi } from '../../features/targeting/types';

export interface SignSideBarProps {
blockedReason: string;
enabled: boolean;
onClick: () => void;
onClickSignMyself: () => void;
targetingApi: TargetingApi | null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
targetingApi: TargetingApi | null;
targetingApi?: TargetingApi;

}

export function SidebarNavSign() {
export function SidebarNavSign(signSideBarProps: SignSideBarProps) {
const {
blockedReason: boxSignBlockedReason,
onClick: onBoxClickRequestSignature,
onClickSignMyself: onBoxClickSignMyself,
status: boxSignStatus,
targetingApi: boxSignTargetingApi,
isSignRemoveInterstitialEnabled,
} = useFeatureConfig('boxSign');
} = signSideBarProps;

return (
<>
{isSignRemoveInterstitialEnabled ? (
<DropdownMenu isResponsive constrainToWindow isRightAligned>
<SidebarNavSignButton
blockedReason={boxSignBlockedReason}
status={boxSignStatus}
targetingApi={boxSignTargetingApi}
data-resin-target={SIDEBAR_NAV_TARGETS.SIGN}
/>
<Menu>
<MenuItem data-testid="sign-request-signature-button" onClick={onBoxClickRequestSignature}>
<SignMeOthers32 width={16} height={16} className="bcs-SidebarNavSign-icon" />
<FormattedMessage {...messages.boxSignRequestSignature} />
</MenuItem>
<MenuItem data-testid="sign-sign-myself-button" onClick={onBoxClickSignMyself}>
<SignMe32 width={16} height={16} className="bcs-SidebarNavSign-icon" />
<FormattedMessage {...messages.boxSignSignMyself} />
</MenuItem>
</Menu>
</DropdownMenu>
) : (
<SidebarNavSignButton
blockedReason={boxSignBlockedReason}
data-resin-target={SIDEBAR_NAV_TARGETS.SIGN}
onClick={onBoxClickRequestSignature}
status={boxSignStatus}
targetingApi={boxSignTargetingApi}
/>
)}
</>
<DropdownMenu isResponsive constrainToWindow isRightAligned>
<SidebarNavSignButton
blockedReason={boxSignBlockedReason}
targetingApi={boxSignTargetingApi}
data-resin-target={SIDEBAR_NAV_TARGETS.SIGN}
/>
<Menu>
<MenuItem onClick={onBoxClickRequestSignature}>
<SignMeOthers32 width={16} height={16} className="bcs-SidebarNavSign-icon" />
<FormattedMessage {...messages.boxSignRequestSignature} />
</MenuItem>
<MenuItem onClick={onBoxClickSignMyself}>
<SignMe32 width={16} height={16} className="bcs-SidebarNavSign-icon" />
<FormattedMessage {...messages.boxSignSignMyself} />
</MenuItem>
</Menu>
</DropdownMenu>
);
}

Expand Down
6 changes: 3 additions & 3 deletions src/elements/content-sidebar/SidebarNavSignButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ export type Props = PlainButtonProps & {

export const PlaceholderTooltip = ({ children }: { children: React.ReactNode }) => children;

export function SidebarNavSignButton({ blockedReason, intl, status, targetingApi, ...rest }: Props) {
export function SidebarNavSignButton({ blockedReason, intl, targetingApi, ...rest }: Props) {
const isSignDisabled = !!blockedReason;
const isTargeted = targetingApi && targetingApi.canShow;
const isTargeted = targetingApi?.canShow;
const FtuxTooltip = !isSignDisabled && isTargeted ? TargetedClickThroughGuideTooltip : PlaceholderTooltip;
const label = intl.formatMessage(status === 'active' ? messages.boxSignSignature : messages.boxSignRequest);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const label = intl.formatMessage(messages.boxSignRequest);
const buttonClassName = classnames('bcs-SidebarNavSignButton', { 'bdl-is-disabled': isSignDisabled });

let tooltipMessage = label;
Expand Down
12 changes: 4 additions & 8 deletions src/elements/content-sidebar/__tests__/SidebarNav.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,19 +166,15 @@ describe('elements/content-sidebar/SidebarNav', () => {
});

test('should render the Box Sign entry point if its feature is enabled', () => {
const features = {
boxSign: {
const props = {
signSideBarProps: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
signSideBarProps: {
signSidebarProps: {

enabled: true,
onClick: () => {},
},
};
const wrapper = getWrapper({}, 'activity', features);
expect(wrapper.exists(SidebarNavSignButton)).toBe(true);
});
const wrapper = getWrapper(props);

test('should not render the Box Sign entry point if its feature is not enabled', () => {
const wrapper = getWrapper();
expect(wrapper.exists(SidebarNavSignButton)).toBe(false);
expect(wrapper.exists(SidebarNavSignButton)).toBe(true);
});
test('should render docgen tab', () => {
const props = {
Expand Down
93 changes: 33 additions & 60 deletions src/elements/content-sidebar/__tests__/SidebarNavSign.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,80 +1,53 @@
import * as React from 'react';
import { render, fireEvent } from '@testing-library/react';

import { userEvent } from '@testing-library/user-event';
import { render, screen } from '../../../test-utils/testing-library';
import SidebarNavSign from '../SidebarNavSign';
// @ts-ignore Module is written in Flow
import FeatureProvider from '../../common/feature-checking/FeatureProvider';

describe('elements/content-sidebar/SidebarNavSign', () => {
const onClickRequestSignature = jest.fn();
const onClickSignMyself = jest.fn();
Comment on lines 8 to 9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const renderComponent = (props = {}, features = {}) =>
render(
<FeatureProvider features={features}>
<SidebarNavSign {...props} />
</FeatureProvider>,
);
const defaultSignSideBarProps = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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} />);
};

blockedReason: '',
enabled: true,
onClick: onClickRequestSignature,
onClickSignMyself,
targetingApi: null,
};

const renderComponent = () => render(<SidebarNavSign {...defaultSignSideBarProps} />, {});

test.each([true, false])('should render sign button', isRemoveInterstitialEnabled => {
const features = {
boxSign: {
isSignRemoveInterstitialEnabled: isRemoveInterstitialEnabled,
},
};
test('should render sign button', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need async here

renderComponent();

const wrapper = renderComponent({}, features);
expect(wrapper.getByTestId('sign-button')).toBeVisible();
expect(screen.getByRole('button', { name: 'Request Signature' })).toBeInTheDocument();
});

test('should call correct handler when sign button is clicked', () => {
const features = {
boxSign: {
isSignRemoveInterstitialEnabled: false,
onClick: onClickRequestSignature,
},
};
const { getByTestId } = renderComponent({}, features);
test('should open dropdown with 2 menu items when sign button is clicked', async () => {
renderComponent();

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

expect(onClickRequestSignature).toBeCalled();
expect(screen.getByRole('menuitem', { name: 'Request Signature' })).toBeVisible();
expect(screen.getByRole('menuitem', { name: 'Sign Myself' })).toBeVisible();
});

test('should open dropdown with 2 menu items when sign button is clicked', () => {
const features = {
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' }));
Comment on lines -45 to +39
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

expect(onClickRequestSignature).toHaveBeenCalled();
});

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

await userEvent.click(screen.getByRole('button', { name: 'Request Signature' }));
Comment on lines +46 to +48
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

await userEvent.click(screen.getByRole('menuitem', { name: 'Sign Myself' }));

expect(onClickSignMyself).toHaveBeenCalled();
});
});
13 changes: 4 additions & 9 deletions src/elements/content-sidebar/__tests__/SidebarNavSignButton.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,11 @@ import Tooltip from '../../../components/tooltip';
describe('elements/content-sidebar/SidebarNavSignButton', () => {
const getWrapper = (props = {}) => shallow(<SidebarNavSignButton {...props} />).dive();

test.each`
status | label
${undefined} | ${'Request Signature'}
${'random'} | ${'Request Signature'}
${'active'} | ${'Sign'}
`('should render the correct label based on the current signature status', ({ label, status }) => {
const wrapper = getWrapper({ status });
test('should render the correct label', () => {
const wrapper = getWrapper();

expect(wrapper.find(Tooltip).prop('text')).toBe(label);
expect(wrapper.find(PlainButton).prop('aria-label')).toBe(label);
expect(wrapper.find(Tooltip).prop('text')).toBe('Request Signature');
expect(wrapper.find(PlainButton).prop('aria-label')).toBe('Request Signature');
expect(wrapper.exists(BoxSign28)).toBe(true);
});

Expand Down
5 changes: 0 additions & 5 deletions src/elements/content-sidebar/messages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,6 @@ const messages = defineMessages({
defaultMessage: 'Sign Myself',
description: 'One of the dropdown options that opens a Box Sign sign myself experience',
},
boxSignSignature: {
id: 'be.contentSidebar.boxSignSignature',
defaultMessage: 'Sign',
description: 'label for button that opens a Box Sign signature fulfillment experience',
},
boxSignSecurityBlockedTooltip: {
defaultMessage: 'This action is unavailable due to a security policy.',
description: 'Tooltip text for when Box Sign is blocked due to a security policy',
Expand Down