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.

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(uaa-parity): Fix consecutive version events issue #3828

Merged
merged 7 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions i18n/en-US.properties
Original file line number Diff line number Diff line change
Expand Up @@ -846,8 +846,12 @@ be.uploadsProvidedFolderNameInvalidMessage = Provided folder name, {name}, could
be.uploadsRetryButtonTooltip = Retry upload
# Error message shown when account storage limit has been reached
be.uploadsStorageLimitErrorMessage = Account storage limit reached
# Message displayed in the activity feed to represent the range of versions actioned by a single user. {name} is the user who did the action. {actionMessage} is the action. {versions} is a range of versions.
be.versionCollapsed = {name} {actionMessage} v{versions}
# Message displayed in the activity feed for a deleted version. {name} is the user who performed the action. {version_number} is the file version string.
be.versionDeleted = {name} deleted v{version_number}
# Message displayed in the activity feed to represent the range of versions actioned by multiple users. {numberOfCollaborators} is a number. {actionMessage} is the action. {versions} is a range of versions.
be.versionMultipleUsersCollapsed = {numberOfCollaborators} collaborators {actionMessage} v{versions}
# Message displayed in the activity feed to represent the range of versions uploaded by multiple users. {numberOfCollaborators} is a number and {versions} is a range of versions.
be.versionMultipleUsersUploaded = {numberOfCollaborators} collaborators uploaded v{versions}
# Message displayed in the activity feed for a promoted version. {name} is the user who performed the action. {version_promoted} is the originating file version string. {version_number} is the file version string.
Expand Down
12 changes: 12 additions & 0 deletions src/elements/common/messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -857,6 +857,18 @@ const messages = defineMessages({
description:
'Message displayed in the activity feed to represent the range of versions uploaded by a single user. {name} is the user who uploaded. {versions} is a range of versions.',
},
versionMultipleUsersCollapsed: {
id: 'be.versionMultipleUsersCollapsed',
defaultMessage: '{numberOfCollaborators} collaborators {actionMessage} v{versions}',
description:
'Message displayed in the activity feed to represent the range of versions actioned by multiple users. {numberOfCollaborators} is a number. {actionMessage} is the action. {versions} is a range of versions.',
},
versionCollapsed: {
id: 'be.versionCollapsed',
defaultMessage: '{name} {actionMessage} v{versions}',
description:
'Message displayed in the activity feed to represent the range of versions actioned by a single user. {name} is the user who did the action. {actionMessage} is the action. {versions} is a range of versions.',
},
versionUploaded: {
id: 'be.versionUploaded',
defaultMessage: '{name} uploaded v{version_number}',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ class ActivityFeed extends React.Component<ActivityFeedProps, State> {
hasReplies={hasReplies}
hasVersions={hasVersions}
isDisabled={isDisabled}
items={collapseFeedState(feedItems)}
items={shouldUseUAA ? feedItems : collapseFeedState(feedItems)}
mentionSelectorContacts={mentionSelectorContacts}
onAnnotationDelete={onAnnotationDelete}
onAnnotationEdit={onAnnotationEdit}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,48 @@ import messages from '../../../common/messages';
import selectors from '../../../common/selectors/version';
import { ACTIVITY_TARGETS } from '../../../common/interactionTargets';
import type { User, FileVersions } from '../../../../common/types/core';
import { ACTION_TYPE_CREATED, ACTION_TYPE_RESTORED, ACTION_TYPE_TRASHED } from '../../../../constants';
import './Version.scss';

const ACTION_MESSAGE_UPLOAD = 'uploaded';
const ACTION_MESSAGE_RESTORE = 'restored';
const ACTION_MESSAGE_TRASH = 'deleted';

function getMessageForAction(
action: string,
collaborators: { [collaborator_id: string]: User },
collaborators: { [collaborator_id: string]: User } = {},
version_start: number,
version_end: number,
shouldUseUAA?: boolean,
action_by?: User[],
): React.Node {
// We only support collapsing for multiple upload versions
if (action !== 'upload') {
if (
action !== 'upload' &&
action !== ACTION_TYPE_RESTORED &&
action !== ACTION_TYPE_TRASHED &&
action !== ACTION_TYPE_CREATED
Comment on lines +32 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like a switch here with a default to null would read better as well or invert this gate and default to returning null.

) {
return null;
}

let actionMessage = '';
switch (action) {
case ACTION_TYPE_CREATED:
actionMessage = ACTION_MESSAGE_UPLOAD;
break;
case ACTION_TYPE_RESTORED:
actionMessage = ACTION_MESSAGE_RESTORE;
break;
case ACTION_TYPE_TRASHED:
actionMessage = ACTION_MESSAGE_TRASH;
break;
default:
actionMessage = '';
break;
}

const collaboratorIDs = Object.keys(collaborators);
const numberOfCollaborators = collaboratorIDs.length;
const numberOfCollaborators = shouldUseUAA ? action_by?.length : collaboratorIDs.length;

const versionRange: React.Node = (
<span className="bcs-Version-range">
Expand All @@ -36,13 +63,40 @@ function getMessageForAction(
);

if (numberOfCollaborators === 1) {
const collaborator = collaborators[collaboratorIDs[0]];
const collaborator = shouldUseUAA ? action_by?.[0] : collaborators[collaboratorIDs[0]];

if (shouldUseUAA) {
return (
<FormattedMessage
{...messages.versionCollapsed}
values={{
name: <strong>{collaborator?.name}</strong>,
versions: versionRange,
actionMessage,
}}
/>
);
}

return (
<FormattedMessage
{...messages.versionUploadCollapsed}
values={{
name: <strong>{collaborator.name}</strong>,
name: <strong>{collaborator?.name}</strong>,
versions: versionRange,
}}
/>
);
}

if (shouldUseUAA) {
return (
<FormattedMessage
{...messages.versionMultipleUsersCollapsed}
values={{
numberOfCollaborators,
versions: versionRange,
actionMessage,
}}
/>
);
Expand All @@ -60,6 +114,8 @@ function getMessageForAction(
}

type Props = {
action_by?: User[],
action_type?: string,
collaborators: { [collaborator_id: string]: User },
id: string,
intl: IntlShape,
Expand All @@ -71,14 +127,25 @@ type Props = {
};

const CollapsedVersion = (props: Props): React.Node => {
const {
action_by,
action_type = ACTION_TYPE_CREATED,
collaborators,
id,
intl,
onInfo,
shouldUseUAA,
versions,
version_start,
version_end,
} = props;
// $FlowFixMe
const action = selectors.getVersionAction(props);
const { collaborators, id, intl, onInfo, shouldUseUAA, versions, version_start, version_end } = props;
const action = shouldUseUAA ? action_type : selectors.getVersionAction(props);

return (
<ActivityCard className="bcs-Version">
<span className="bcs-Version-message">
{getMessageForAction(action, collaborators, version_start, version_end)}
{getMessageForAction(action, collaborators, version_start, version_end, shouldUseUAA, action_by)}
</span>
{onInfo ? (
<span className="bcs-Version-actions">
Expand Down
Original file line number Diff line number Diff line change
@@ -1,86 +1,87 @@
import * as React from 'react';
import { shallow } from 'enzyme';

import { render, screen } from '@testing-library/react';
import { IntlProvider } from 'react-intl';
import { ACTION_TYPE_CREATED, ACTION_TYPE_RESTORED, ACTION_TYPE_TRASHED } from '../../../../../constants';
import CollapsedVersion from '../CollapsedVersion';
import selectors from '../../../../common/selectors/version';
import { CollapsedVersionBase as CollapsedVersion } from '../CollapsedVersion';

const translationProps = {
intl: { formatMessage: () => {} },
};
jest.mock('react-intl', () => ({
...jest.requireActual('react-intl'),
}));
Comment on lines +8 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

if you import render and screen from test-utils/testing-library, you won't need to mock this or import IntlProvider separately in this file. there should be some examples in this repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests not passing when I do that for some reason . Given this change is a little time-sensitive, I will leave as is.


describe('elements/content-sidebar/ActivityFeed/version/CollapsedVersion', () => {
const render = item => shallow(<CollapsedVersion {...translationProps} {...item} />);

beforeEach(() => {
selectors.getVersionAction = jest.fn().mockReturnValue('upload');
});

test('should correctly render for single collaborator', () => {
const version_start = 1;
const version_end = 10;
const item = {
collaborators: { 1: { name: 'Person one', id: 1 } },
version_start,
version_end,
};
const intl = {
formatMessage: jest.fn().mockImplementation(message => message.defaultMessage),
};

const renderComponent = props =>
render(
<IntlProvider locale="en">
<CollapsedVersion
intl={intl}
collaborators={{ 1: { name: 'Person one', id: 1 } }}
version_start={1}
version_end={10}
versions={[]}
id={0}
{...props}
/>
</IntlProvider>,
);

const wrapper = render(item);
const formattedMessage = wrapper.find('FormattedMessage');
expect(wrapper).toMatchSnapshot();
test('should correctly render for single collaborator', () => {
renderComponent();

const renderedVersionsMessage = shallow(formattedMessage.prop('values').versions);
expect(renderedVersionsMessage).toMatchSnapshot();
expect(screen.getByText('Person one')).toBeInTheDocument();
expect(screen.getByText('uploaded v')).toBeInTheDocument();
expect(screen.getByText('1 - 10')).toBeInTheDocument();
});

test('should correctly render for multiple collaborators', () => {
const version_start = 1;
const version_end = 10;
const item = {
collaborators: {
1: { name: 'Person one', id: 1 },
2: { name: 'Person two', id: 2 },
},
version_start,
version_end,
};
renderComponent({
collaborators: { 1: { name: 'Person one', id: 1 }, 2: { name: 'Person two', id: 2 } },
});

const wrapper = render(item);
const formattedMessage = wrapper.find('FormattedMessage');

expect(wrapper).toMatchSnapshot();

const renderedVersionsMessage = shallow(formattedMessage.prop('values').versions);
expect(renderedVersionsMessage).toMatchSnapshot();
expect(screen.getByText('2 collaborators uploaded v')).toBeInTheDocument();
expect(screen.getByText('1 - 10')).toBeInTheDocument();
});

test('should correctly render info icon if onInfo is passed', () => {
const item = {
renderComponent({
onInfo: () => {},
collaborators: {
1: { name: 'Person one', id: 1 },
2: { name: 'Person two', id: 2 },
},
version_start: 1,
version_end: 10,
};
});

const wrapper = render(item);

expect(wrapper.exists('IconInfo')).toBe(true);
expect(screen.getByLabelText('Get version information')).toBeInTheDocument();
});

test('should not render a message if action is not upload', () => {
selectors.getVersionAction.mockReturnValueOnce('delete');

const item = {
collaborators: { 1: { name: 'Person one', id: 1 } },
version_start: 1,
version_end: 10,
};
renderComponent();

const wrapper = render(item);
const formattedMessage = wrapper.find('FormattedMessage');
expect(screen.queryByText('Person one')).not.toBeInTheDocument();
});

expect(formattedMessage.length).toBe(0);
test.each`
actionType | actionText
${ACTION_TYPE_RESTORED} | ${'restored v'}
${ACTION_TYPE_TRASHED} | ${'deleted v'}
${ACTION_TYPE_CREATED} | ${'uploaded v'}
`('should correctly render when shouldUseUAA is true with actionType $actionType', ({ actionType, actionText }) => {
renderComponent({
shouldUseUAA: true,
action_type: actionType,
action_by: [{ name: 'John Doe', id: 3 }],
version_start: 2,
version_end: 4,
});

expect(screen.getByText('John Doe')).toBeInTheDocument();
expect(screen.getByText(actionText)).toBeInTheDocument();
expect(screen.getByText('2 - 4')).toBeInTheDocument();
});
Comment on lines +69 to 86
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks for converting this!

});

This file was deleted.

Loading