Skip to content

Commit 7d691df

Browse files
booc0mtacomergify[bot]
booc0mtaco
authored and
mergify[bot]
committed
feat(tasks): handle new error message with title and description (#1564)
* refactor(tasks): change tasks update sequence * fix(tasks): Handle race where task should update in sidebar * feat(tasks): handle new error message with title and description * refactor(tasks): address PR comments and support save flow * fix(tasks): address comments from CR * test(tasks): rebase on master and include snapshot updates
1 parent 1e4446e commit 7d691df

File tree

9 files changed

+539
-47
lines changed

9 files changed

+539
-47
lines changed

‎i18n/en-US.properties‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,8 +94,14 @@ be.contentSidebar.activityFeed.task.taskEditMenuItem = Modify task
9494
be.contentSidebar.activityFeed.taskForm.taskAnyCheckboxLabel = Only one assignee is required to complete this task
9595
# Text in tooltip explaining completion rule for an any assignee task.
9696
be.contentSidebar.activityFeed.taskForm.taskAnyInfoTooltip = By default, all assignees are required to take action before a task is complete. Selecting this option will require only one assignee to complete this task.
97+
# Warning message showing that, while the task was updated, not all assignees (1+) were removed
98+
be.contentSidebar.activityFeed.taskForm.taskApprovalAssigneeRemovalWarningMessage = Unable to remove assignee(s) because the task is now approved.
9799
# Title shown above error message when a task creation fails
98100
be.contentSidebar.activityFeed.taskForm.taskCreateErrorTitle = Error
101+
# Title shown above warning message when a task create/edit partially fails
102+
be.contentSidebar.activityFeed.taskForm.taskEditWarningTitle = Task Updated with Errors
103+
# Warning message showing that, while the task was updated, not all assignees (1+) were removed
104+
be.contentSidebar.activityFeed.taskForm.taskGeneralAssigneeRemovalWarningMessage = Unable to remove assignee(s) because the task is now completed.
99105
# Error message when a task edit fails
100106
be.contentSidebar.activityFeed.taskForm.taskUpdateErrorMessage = An error occurred while modifying this task. Please try again.
101107
# label for cancel button in create task popup

‎src/api/Feed.js‎

Lines changed: 37 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -535,7 +535,7 @@ class Feed extends Base {
535535
* @param {Function} errorCallback - the function which will be called on error
536536
* @return {void}
537537
*/
538-
updateTaskNew = (
538+
updateTaskNew = async (
539539
file: BoxItem,
540540
task: TaskUpdatePayload,
541541
successCallback: () => void = noop,
@@ -545,21 +545,36 @@ class Feed extends Base {
545545
throw getBadItemError();
546546
}
547547

548+
let updatedWithoutError = true;
549+
let removeAssigneeError;
550+
548551
this.file = file;
549552
this.errorCallback = errorCallback;
550553
this.tasksNewAPI = new TasksNewAPI(this.options);
551554
this.updateFeedItem({ isPending: true }, task.id);
552555

553-
Promise.all(task.addedAssignees.map(assignee => this.createTaskCollaborator(file, task, assignee)))
554-
.then(() => {
555-
return Promise.all(
556-
task.removedAssignees.map(assignee => this.deleteTaskCollaborator(file, task, assignee)),
557-
);
558-
})
559-
.then(() => {
556+
try {
557+
await Promise.all(task.addedAssignees.map(assignee => this.createTaskCollaborator(file, task, assignee)));
558+
await new Promise((resolve, reject) => {
560559
this.tasksNewAPI.updateTask({
561560
file,
562561
task,
562+
successCallback: resolve,
563+
errorCallback: reject,
564+
});
565+
});
566+
567+
await Promise.all(
568+
task.removedAssignees.map(assignee => this.deleteTaskCollaborator(file, task, assignee)),
569+
).catch((e: ElementsXhrError) => {
570+
updatedWithoutError = false;
571+
removeAssigneeError = e;
572+
});
573+
574+
await new Promise((resolve, reject) => {
575+
this.tasksNewAPI.getTask({
576+
file,
577+
id: task.id,
563578
successCallback: (taskData: Task) => {
564579
this.updateFeedItem(
565580
{
@@ -569,20 +584,28 @@ class Feed extends Base {
569584
task.id,
570585
);
571586

572-
if (!this.isDestroyed()) {
573-
successCallback();
587+
if (!updatedWithoutError) {
588+
this.feedErrorCallback(false, removeAssigneeError, ERROR_CODE_UPDATE_TASK);
574589
}
590+
591+
resolve();
575592
},
576593
errorCallback: (e: ElementsXhrError) => {
577594
this.updateFeedItem({ isPending: false }, task.id);
578595
this.feedErrorCallback(false, e, ERROR_CODE_UPDATE_TASK);
596+
reject();
579597
},
580598
});
581-
})
582-
.catch((e: ElementsXhrError) => {
583-
this.updateFeedItem({ isPending: false }, task.id);
584-
this.feedErrorCallback(false, e, ERROR_CODE_UPDATE_TASK);
585599
});
600+
601+
// everything succeeded, so call the passed in success callback
602+
if (!this.isDestroyed() && updatedWithoutError) {
603+
successCallback();
604+
}
605+
} catch (e) {
606+
this.updateFeedItem({ isPending: false }, task.id);
607+
this.feedErrorCallback(false, e, ERROR_CODE_UPDATE_TASK);
608+
}
586609
};
587610

588611
/**

‎src/api/__tests__/Feed-test.js‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -711,13 +711,18 @@ describe('api/Feed', () => {
711711
feed.updateFeedItem = jest.fn();
712712
});
713713

714-
test('should throw if no file id', () => {
715-
expect(() => feed.updateTaskNew({})).toThrow(fileError);
714+
test('should throw if no file id', async () => {
715+
expect.assertions(1);
716+
const updatedTask = feed.updateTaskNew({});
717+
await expect(updatedTask).rejects.toEqual(Error(fileError));
716718
});
717719

718720
test('should call the error handling when unable to create new task collaborator', async () => {
719721
const mockErrorCallback = jest.fn();
722+
const mockSuccessCallback = jest.fn();
723+
720724
feed.createTaskCollaborator = jest.fn().mockRejectedValue(new Error('forced rejection'));
725+
feed.deleteTaskCollaborator = jest.fn().mockResolvedValue();
721726

722727
const task = {
723728
id: '1',
@@ -745,17 +750,20 @@ describe('api/Feed', () => {
745750
],
746751
};
747752

748-
feed.updateTaskNew(file, task, jest.fn(), mockErrorCallback);
753+
feed.updateTaskNew(file, task, mockSuccessCallback, mockErrorCallback);
749754

750755
await new Promise(r => setTimeout(r, 0));
751756

752757
expect(feed.tasksNewAPI.updateTask).not.toBeCalled();
758+
expect(feed.tasksNewAPI.getTask).not.toBeCalled();
759+
expect(feed.deleteTaskCollaborator).not.toBeCalled();
753760
expect(feed.updateFeedItem).toBeCalled();
754761
expect(mockErrorCallback).toBeCalled();
755762
});
756763

757764
test('should call the error handling when unable to delete existing task collaborator', async () => {
758765
const mockErrorCallback = jest.fn();
766+
const mockSuccessCallback = jest.fn();
759767
feed.deleteTaskCollaborator = jest.fn().mockRejectedValue(new Error('forced rejection'));
760768

761769
const task = {
@@ -784,11 +792,12 @@ describe('api/Feed', () => {
784792
],
785793
};
786794

787-
feed.updateTaskNew(file, task, jest.fn(), mockErrorCallback);
795+
feed.updateTaskNew(file, task, mockSuccessCallback, mockErrorCallback);
788796

789797
await new Promise(r => setTimeout(r, 0));
790798

791-
expect(feed.tasksNewAPI.updateTask).not.toBeCalled();
799+
expect(feed.tasksNewAPI.updateTask).toBeCalled();
800+
expect(feed.tasksNewAPI.getTask).toBeCalled();
792801
expect(feed.updateFeedItem).toBeCalled();
793802
expect(mockErrorCallback).toBeCalled();
794803
});
@@ -808,7 +817,8 @@ describe('api/Feed', () => {
808817
await new Promise(r => setTimeout(r, 0));
809818

810819
expect(feed.tasksNewAPI.updateTask).toBeCalled();
811-
expect(feed.updateFeedItem).toBeCalled();
820+
expect(feed.tasksNewAPI.getTask).toBeCalled();
821+
expect(feed.updateFeedItem).toBeCalledTimes(2);
812822
expect(successCallback).toBeCalled();
813823
});
814824

‎src/elements/content-sidebar/activity-feed/task-form/TaskError.js‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
/**
2+
* @flow
3+
* @file Component for in-modal error messages for tasks
4+
*/
5+
6+
import * as React from 'react';
7+
import { FormattedMessage } from 'react-intl';
8+
import getProp from 'lodash/get';
9+
10+
import messages from './messages';
11+
import apiMessages from '../../../../api/messages';
12+
import { TASK_EDIT_MODE_EDIT } from '../../../../constants';
13+
import InlineNotice from '../../../../components/inline-notice/InlineNotice';
14+
15+
import type { TaskType, TaskEditMode } from '../../../../common/types/tasks';
16+
17+
type Props = {
18+
editMode?: TaskEditMode,
19+
error?: { status: number },
20+
taskType: TaskType,
21+
};
22+
23+
const TaskError = ({ editMode, error, taskType }: Props) => {
24+
const isEditMode = editMode === TASK_EDIT_MODE_EDIT;
25+
const isForbiddenErrorOnEdit = getProp(error, 'status') === 403 && isEditMode;
26+
const errorTitle = isForbiddenErrorOnEdit ? messages.taskEditWarningTitle : messages.taskCreateErrorTitle;
27+
let errorMessage = isEditMode ? messages.taskUpdateErrorMessage : apiMessages.taskCreateErrorMessage;
28+
29+
if (!error) {
30+
return null;
31+
}
32+
33+
// error message changes when a forbidden operation occurs while editing a task
34+
if (isForbiddenErrorOnEdit) {
35+
switch (taskType) {
36+
case 'GENERAL':
37+
errorMessage = messages.taskGeneralAssigneeRemovalWarningMessage;
38+
break;
39+
case 'APPROVAL':
40+
errorMessage = messages.taskApprovalAssigneeRemovalWarningMessage;
41+
break;
42+
default:
43+
return null;
44+
}
45+
}
46+
47+
return (
48+
<InlineNotice type="error" title={<FormattedMessage {...errorTitle} />}>
49+
<FormattedMessage {...errorMessage} />
50+
</InlineNotice>
51+
);
52+
};
53+
54+
export default TaskError;

‎src/elements/content-sidebar/activity-feed/task-form/TaskForm.js‎

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,11 @@
55

66
import * as React from 'react';
77
import noop from 'lodash/noop';
8+
import getProp from 'lodash/get';
89
import classNames from 'classnames';
910
import { FormattedMessage, injectIntl } from 'react-intl';
1011
import commonMessages from '../../../../common/messages';
1112
import messages from './messages';
12-
import apiMessages from '../../../../api/messages';
1313
import commentFormMessages from '../comment-form/messages';
1414
import Form from '../../../../components/form-elements/form/Form';
1515
import ContactDatalistItem from '../../../../components/contact-datalist-item/ContactDatalistItem';
@@ -20,7 +20,6 @@ import PillSelectorDropdown from '../../../../components/pill-selector-dropdown/
2020
import Button from '../../../../components/button/Button';
2121
import { FeatureFlag } from '../../../common/feature-checking';
2222
import PrimaryButton from '../../../../components/primary-button/PrimaryButton';
23-
import InlineError from '../../../../components/inline-error/InlineError';
2423
import {
2524
TASK_COMPLETION_RULE_ANY,
2625
TASK_COMPLETION_RULE_ALL,
@@ -35,11 +34,12 @@ import type {
3534
TaskEditMode,
3635
TaskUpdatePayload,
3736
} from '../../../../common/types/tasks';
37+
import TaskError from './TaskError';
3838

3939
import './TaskForm.scss';
4040

4141
type TaskFormProps = {|
42-
error?: any,
42+
error?: { status: number }, // TODO: update to ElementsXhrError once API supports it
4343
isDisabled?: boolean,
4444
onCancel: () => any,
4545
onSubmitError: (e: ElementsXhrError) => any,
@@ -313,7 +313,7 @@ class TaskForm extends React.Component<Props, State> {
313313
};
314314

315315
render() {
316-
const { approverSelectorContacts, className, error, isDisabled, intl, editMode } = this.props;
316+
const { approverSelectorContacts, className, error, isDisabled, intl, editMode, taskType } = this.props;
317317
const { dueDate, approvers, message, formValidityState, isLoading, completionRule } = this.state;
318318
const inputContainerClassNames = classNames('bcs-task-input-container', 'bcs-task-input-is-open', className);
319319
const isCreateEditMode = editMode === TASK_EDIT_MODE_CREATE;
@@ -332,23 +332,15 @@ class TaskForm extends React.Component<Props, State> {
332332
const submitButtonMessage = isCreateEditMode
333333
? messages.tasksAddTaskFormSubmitLabel
334334
: messages.tasksEditTaskFormSubmitLabel;
335-
336-
const taskErrorMessage = isCreateEditMode
337-
? apiMessages.taskCreateErrorMessage
338-
: messages.taskUpdateErrorMessage;
339-
340335
const shouldShowCompletionRule = approvers.length > 0;
341336
const isCompletionRuleCheckboxDisabled = approvers.length <= 1;
342337
const isCompletionRuleCheckboxChecked = completionRule === TASK_COMPLETION_RULE_ANY;
338+
const isForbiddenErrorOnEdit = isLoading || (getProp(error, 'status') === 403 && !isCreateEditMode);
343339

344340
return (
345341
<div className={inputContainerClassNames} data-resin-component="taskform">
346342
<div className="bcs-task-input-form-container">
347-
{error ? (
348-
<InlineError title={<FormattedMessage {...messages.taskCreateErrorTitle} />}>
349-
<FormattedMessage {...taskErrorMessage} />
350-
</InlineError>
351-
) : null}
343+
<TaskError editMode={editMode} error={error} taskType={taskType} />
352344
<Form
353345
formValidityState={formValidityState}
354346
onInvalidSubmit={this.handleInvalidSubmit}
@@ -357,7 +349,7 @@ class TaskForm extends React.Component<Props, State> {
357349
<PillSelectorDropdown
358350
className={pillSelectorOverlayClasses}
359351
error={this.getErrorByFieldname('taskAssignees')}
360-
disabled={isLoading}
352+
disabled={isForbiddenErrorOnEdit}
361353
inputProps={{ 'data-testid': 'task-form-assignee-input' }}
362354
isRequired
363355
label={<FormattedMessage {...messages.tasksAddTaskFormSelectAssigneesLabel} />}
@@ -386,7 +378,7 @@ class TaskForm extends React.Component<Props, State> {
386378
<Checkbox
387379
data-testid="task-form-completion-rule-checkbox"
388380
isChecked={isCompletionRuleCheckboxChecked}
389-
isDisabled={isCompletionRuleCheckboxDisabled}
381+
isDisabled={isCompletionRuleCheckboxDisabled || isForbiddenErrorOnEdit}
390382
label={<FormattedMessage {...messages.taskAnyCheckboxLabel} />}
391383
tooltip={intl.formatMessage(messages.taskAnyInfoTooltip)}
392384
name="completionRule"
@@ -398,7 +390,7 @@ class TaskForm extends React.Component<Props, State> {
398390
<TextArea
399391
className="bcs-task-name-input"
400392
data-testid="task-form-name-input"
401-
disabled={isDisabled || isLoading}
393+
disabled={isDisabled || isForbiddenErrorOnEdit}
402394
error={this.getErrorByFieldname('taskName')}
403395
isRequired
404396
label={<FormattedMessage {...messages.tasksAddTaskFormMessageLabel} />}
@@ -415,7 +407,7 @@ class TaskForm extends React.Component<Props, State> {
415407
[INTERACTION_TARGET]: ACTIVITY_TARGETS.TASK_DATE_PICKER,
416408
'data-testid': 'task-form-date-input',
417409
}}
418-
isDisabled={isLoading}
410+
isDisabled={isForbiddenErrorOnEdit}
419411
isRequired={false}
420412
label={<FormattedMessage {...messages.tasksAddTaskFormDueDateLabel} />}
421413
minDate={new Date()}
@@ -440,7 +432,7 @@ class TaskForm extends React.Component<Props, State> {
440432
className="bcs-task-input-submit-btn"
441433
data-resin-target={ACTIVITY_TARGETS.APPROVAL_FORM_POST}
442434
data-testid="task-form-submit-button"
443-
isDisabled={isLoading}
435+
isDisabled={isForbiddenErrorOnEdit}
444436
isLoading={isLoading}
445437
{...this.addResinInfo()}
446438
>

‎src/elements/content-sidebar/activity-feed/task-form/__tests__/TaskError-test.js‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import * as React from 'react';
2+
import { mount } from 'enzyme';
3+
4+
import TaskError from '../TaskError';
5+
6+
describe('components/content-sidebar/activity-feed/task-form/TaskError', () => {
7+
test.each([
8+
['GENERAL', 'EDIT', undefined, 0],
9+
['GENERAL', 'EDIT', { status: 403 }, 1],
10+
['GENERAL', 'EDIT', { status: 404 }, 1],
11+
['GENERAL', 'CREATE', undefined, 0],
12+
['GENERAL', 'CREATE', { status: 403 }, 1],
13+
['GENERAL', 'CREATE', { status: 404 }, 1],
14+
['APPROVAL', 'EDIT', undefined, 0],
15+
['APPROVAL', 'EDIT', { status: 403 }, 1],
16+
['APPROVAL', 'EDIT', { status: 404 }, 1],
17+
['APPROVAL', 'CREATE', undefined, 0],
18+
['APPROVAL', 'CREATE', { status: 403 }, 1],
19+
['APPROVAL', 'CREATE', { status: 404 }, 1],
20+
])(
21+
'when type is %s and edit mode is %s, with error obj %o, we show the proper inline error',
22+
(taskType, editMode, error) => {
23+
const wrapper = mount(<TaskError taskType={taskType} editMode={editMode} error={error} />);
24+
expect(wrapper).toMatchSnapshot();
25+
},
26+
);
27+
});

‎src/elements/content-sidebar/activity-feed/task-form/__tests__/TaskForm-test.js‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -191,14 +191,6 @@ describe('components/ContentSidebar/ActivityFeed/task-form/TaskForm', () => {
191191
expect(wrapper.find('PillSelectorDropdown').hasClass('scrollable'));
192192
});
193193

194-
test('should show inline error when error prop is passed', () => {
195-
const wrapper = render({
196-
createTask: jest.fn(),
197-
error: 'error',
198-
});
199-
expect(wrapper.find('.inline-alert').length).toBe(1);
200-
});
201-
202194
describe('handleDueDateChange()', () => {
203195
test('should set the approval date to be one millisecond before midnight of the next day', async () => {
204196
// Midnight on December 3rd GMT

0 commit comments

Comments
 (0)