Skip to content

Commit 308f450

Browse files
[MM-66690][MM-66691] Checklist improvements (#9294) (#9302)
* fix no response + handle error * address comment review * add a screen to allow user to name the checklist * handle local update errors * can save as a memoized item * change touchableopacity into a button * fix typo * memoize canSave (cherry picked from commit 93b749e) Co-authored-by: Guillermo Vayá <[email protected]>
1 parent a9b7e08 commit 308f450

File tree

18 files changed

+394
-102
lines changed

18 files changed

+394
-102
lines changed

app/products/playbooks/actions/remote/checklist.test.ts

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
setDueDate as localSetDueDate,
1212
renameChecklist as localRenameChecklist,
1313
} from '@playbooks/actions/local/checklist';
14+
import {handlePlaybookRuns} from '@playbooks/actions/local/run';
1415

1516
import {
1617
updateChecklistItem,
@@ -42,9 +43,11 @@ const mockClient = {
4243
setDueDate: jest.fn(),
4344
renameChecklist: jest.fn(),
4445
addChecklistItem: jest.fn(),
46+
fetchPlaybookRun: jest.fn(),
4547
};
4648

4749
jest.mock('@playbooks/actions/local/checklist');
50+
jest.mock('@playbooks/actions/local/run');
4851

4952
const throwFunc = () => {
5053
throw Error('error');
@@ -353,7 +356,7 @@ describe('checklist', () => {
353356

354357
it('should handle local DB update failure', async () => {
355358
mockClient.renameChecklist.mockResolvedValueOnce({});
356-
(localRenameChecklist as jest.Mock).mockRejectedValueOnce(new Error('DB error'));
359+
jest.mocked(localRenameChecklist).mockResolvedValueOnce({error: 'DB error'});
357360

358361
const result = await renameChecklist(serverUrl, playbookRunId, checklistNumber, checklistId, newTitle);
359362
expect(result).toBeDefined();
@@ -364,6 +367,7 @@ describe('checklist', () => {
364367

365368
it('should rename checklist successfully', async () => {
366369
mockClient.renameChecklist.mockResolvedValueOnce({});
370+
jest.mocked(localRenameChecklist).mockResolvedValueOnce({data: true});
367371

368372
const result = await renameChecklist(serverUrl, playbookRunId, checklistNumber, checklistId, newTitle);
369373
expect(result).toBeDefined();
@@ -375,6 +379,7 @@ describe('checklist', () => {
375379

376380
it('should rename checklist with empty title', async () => {
377381
mockClient.renameChecklist.mockResolvedValueOnce({});
382+
jest.mocked(localRenameChecklist).mockResolvedValueOnce({data: true});
378383
const emptyTitle = '';
379384

380385
const result = await renameChecklist(serverUrl, playbookRunId, checklistNumber, checklistId, emptyTitle);
@@ -394,37 +399,47 @@ describe('checklist', () => {
394399

395400
const result = await addChecklistItem(serverUrl, playbookRunId, checklistNumber, title);
396401
expect(result).toBeDefined();
397-
expect(result.error).toBeDefined();
402+
expect('error' in result && result.error).toBeDefined();
398403
});
399404

400405
it('should handle API exception', async () => {
401406
mockClient.addChecklistItem.mockImplementationOnce(throwFunc);
402407

403408
const result = await addChecklistItem(serverUrl, playbookRunId, checklistNumber, title);
404409
expect(result).toBeDefined();
405-
expect(result.error).toBeDefined();
410+
expect('error' in result && result.error).toBeDefined();
406411
expect(mockClient.addChecklistItem).toHaveBeenCalledWith(playbookRunId, checklistNumber, title);
407412
});
408413

409414
it('should add checklist item successfully', async () => {
410-
mockClient.addChecklistItem.mockResolvedValueOnce({});
415+
const mockRun = {id: playbookRunId, checklists: []};
416+
mockClient.addChecklistItem.mockResolvedValueOnce(undefined);
417+
mockClient.fetchPlaybookRun.mockResolvedValueOnce(mockRun);
418+
(handlePlaybookRuns as jest.Mock).mockResolvedValueOnce({data: true});
411419

412420
const result = await addChecklistItem(serverUrl, playbookRunId, checklistNumber, title);
413421
expect(result).toBeDefined();
414-
expect(result.error).toBeUndefined();
415-
expect(result.data).toBe(true);
422+
expect('error' in result ? result.error : undefined).toBeUndefined();
423+
expect('data' in result && result.data).toBe(true);
416424
expect(mockClient.addChecklistItem).toHaveBeenCalledWith(playbookRunId, checklistNumber, title);
425+
expect(mockClient.fetchPlaybookRun).toHaveBeenCalledWith(playbookRunId);
426+
expect(handlePlaybookRuns).toHaveBeenCalledWith(serverUrl, [mockRun], false, true);
417427
});
418428

419429
it('should add checklist item with empty title', async () => {
420-
mockClient.addChecklistItem.mockResolvedValueOnce({});
430+
const mockRun = {id: playbookRunId, checklists: []};
421431
const emptyTitle = '';
432+
mockClient.addChecklistItem.mockResolvedValueOnce(undefined);
433+
mockClient.fetchPlaybookRun.mockResolvedValueOnce(mockRun);
434+
(handlePlaybookRuns as jest.Mock).mockResolvedValueOnce({data: true});
422435

423436
const result = await addChecklistItem(serverUrl, playbookRunId, checklistNumber, emptyTitle);
424437
expect(result).toBeDefined();
425-
expect(result.error).toBeUndefined();
426-
expect(result.data).toBe(true);
438+
expect('error' in result ? result.error : undefined).toBeUndefined();
439+
expect('data' in result && result.data).toBe(true);
427440
expect(mockClient.addChecklistItem).toHaveBeenCalledWith(playbookRunId, checklistNumber, emptyTitle);
441+
expect(mockClient.fetchPlaybookRun).toHaveBeenCalledWith(playbookRunId);
442+
expect(handlePlaybookRuns).toHaveBeenCalledWith(serverUrl, [mockRun], false, true);
428443
});
429444
});
430445
});

app/products/playbooks/actions/remote/checklist.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
setDueDate as localSetDueDate,
1212
renameChecklist as localRenameChecklist,
1313
} from '@playbooks/actions/local/checklist';
14+
import {handlePlaybookRuns} from '@playbooks/actions/local/run';
1415
import {getFullErrorMessage} from '@utils/errors';
1516
import {logDebug} from '@utils/log';
1617

@@ -172,8 +173,8 @@ export const renameChecklist = async (
172173
await client.renameChecklist(playbookRunId, checklistNumber, newTitle);
173174

174175
// Update local database
175-
await localRenameChecklist(serverUrl, checklistId, newTitle);
176-
return {data: true};
176+
const result = await localRenameChecklist(serverUrl, checklistId, newTitle);
177+
return result.error ? result : {data: true};
177178
} catch (error) {
178179
logDebug('error on renameChecklist', getFullErrorMessage(error));
179180
forceLogoutIfNecessary(serverUrl, error);
@@ -190,7 +191,11 @@ export const addChecklistItem = async (
190191
try {
191192
const client = NetworkManager.getClient(serverUrl);
192193
await client.addChecklistItem(playbookRunId, checklistNumber, title);
193-
return {data: true};
194+
195+
// Fetch and sync the entire run to get the created item with server-generated ID
196+
const run = await client.fetchPlaybookRun(playbookRunId);
197+
const result = await handlePlaybookRuns(serverUrl, [run], false, true);
198+
return result.error ? result : {data: true};
194199
} catch (error) {
195200
logDebug('error on addChecklistItem', getFullErrorMessage(error));
196201
forceLogoutIfNecessary(serverUrl, error);

app/products/playbooks/actions/remote/runs.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -753,6 +753,17 @@ describe('renamePlaybookRun', () => {
753753
expect(localRenamePlaybookRun).not.toHaveBeenCalled();
754754
});
755755

756+
it('should handle local DB update failure', async () => {
757+
mockClient.patchPlaybookRun.mockResolvedValueOnce(undefined);
758+
jest.mocked(localRenamePlaybookRun).mockResolvedValueOnce({error: 'DB error'});
759+
760+
const result = await renamePlaybookRun(serverUrl, playbookRunId, newName);
761+
expect(result).toBeDefined();
762+
expect(result.error).toBeDefined();
763+
expect(mockClient.patchPlaybookRun).toHaveBeenCalledWith(playbookRunId, {name: newName});
764+
expect(localRenamePlaybookRun).toHaveBeenCalledWith(serverUrl, playbookRunId, newName);
765+
});
766+
756767
it('should rename playbook run successfully', async () => {
757768
mockClient.patchPlaybookRun.mockResolvedValueOnce(undefined);
758769

app/products/playbooks/actions/remote/runs.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,9 @@ export const renamePlaybookRun = async (serverUrl: string, playbookRunId: string
141141
const client = NetworkManager.getClient(serverUrl);
142142
await client.patchPlaybookRun(playbookRunId, {name: newName});
143143

144-
await localRenamePlaybookRun(serverUrl, playbookRunId, newName);
145-
return {data: true};
144+
// Update local database
145+
const result = await localRenamePlaybookRun(serverUrl, playbookRunId, newName);
146+
return result.error ? result : {data: true};
146147
} catch (error) {
147148
logDebug('error on renamePlaybookRun', getFullErrorMessage(error));
148149
forceLogoutIfNecessary(serverUrl, error);

app/products/playbooks/constants/screens.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ export const PLAYBOOK_SELECT_USER = 'PlaybookSelectUser';
1313
export const PLAYBOOKS_SELECT_DATE = 'PlaybooksSelectDate';
1414
export const PLAYBOOKS_SELECT_PLAYBOOK = 'PlaybooksSelectPlaybook';
1515
export const PLAYBOOKS_START_A_RUN = 'PlaybooksStartARun';
16+
export const PLAYBOOKS_CREATE_QUICK_CHECKLIST = 'PlaybooksCreateQuickChecklist';
1617

1718
export default {
1819
PLAYBOOKS_RUNS,
@@ -27,4 +28,5 @@ export default {
2728
PLAYBOOKS_SELECT_DATE,
2829
PLAYBOOKS_SELECT_PLAYBOOK,
2930
PLAYBOOKS_START_A_RUN,
31+
PLAYBOOKS_CREATE_QUICK_CHECKLIST,
3032
} as const;
Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,182 @@
1+
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
2+
// See LICENSE.txt for license information.
3+
4+
import React, {useState, useCallback, useEffect} from 'react';
5+
import {useIntl, type IntlShape} from 'react-intl';
6+
import {Keyboard, ScrollView} from 'react-native';
7+
import {SafeAreaView} from 'react-native-safe-area-context';
8+
9+
import CompassIcon from '@components/compass_icon';
10+
import FloatingTextInput from '@components/floating_input/floating_text_input_label';
11+
import {useTheme} from '@context/theme';
12+
import useAndroidHardwareBackHandler from '@hooks/android_back_handler';
13+
import useNavButtonPressed from '@hooks/navigation_button_pressed';
14+
import {usePreventDoubleTap} from '@hooks/utils';
15+
import {createPlaybookRun, fetchPlaybookRunsForChannel} from '@playbooks/actions/remote/runs';
16+
import {goToPlaybookRun} from '@playbooks/screens/navigation';
17+
import {popTopScreen, setButtons} from '@screens/navigation';
18+
import {getFullErrorMessage} from '@utils/errors';
19+
import {logError} from '@utils/log';
20+
import {showPlaybookErrorSnackbar} from '@utils/snack_bar';
21+
import {makeStyleSheetFromTheme} from '@utils/theme';
22+
23+
import type {AvailableScreens} from '@typings/screens/navigation';
24+
import type {OptionsTopBarButton} from 'react-native-navigation';
25+
26+
type Props = {
27+
componentId: AvailableScreens;
28+
channelId: string;
29+
channelName: string;
30+
currentUserId: string;
31+
currentTeamId: string;
32+
serverUrl: string;
33+
}
34+
35+
const getStyleSheet = makeStyleSheetFromTheme((theme: Theme) => {
36+
return {
37+
container: {
38+
flex: 1,
39+
backgroundColor: theme.centerChannelBg,
40+
},
41+
content: {
42+
flex: 1,
43+
paddingHorizontal: 20,
44+
paddingVertical: 24,
45+
},
46+
contentContainer: {
47+
gap: 16,
48+
},
49+
};
50+
});
51+
52+
const CLOSE_BUTTON_ID = 'close-create-quick-checklist';
53+
const CREATE_BUTTON_ID = 'create-quick-checklist';
54+
55+
async function makeLeftButton(theme: Theme): Promise<OptionsTopBarButton> {
56+
return {
57+
id: CLOSE_BUTTON_ID,
58+
icon: await CompassIcon.getImageSource('close', 24, theme.sidebarHeaderTextColor),
59+
testID: 'create_quick_checklist.close.button',
60+
};
61+
}
62+
63+
function makeRightButton(theme: Theme, intl: IntlShape, enabled: boolean): OptionsTopBarButton {
64+
return {
65+
color: theme.sidebarHeaderTextColor,
66+
id: CREATE_BUTTON_ID,
67+
text: intl.formatMessage({id: 'mobile.create_channel', defaultMessage: 'Create'}),
68+
showAsAction: 'always',
69+
testID: 'create_quick_checklist.create.button',
70+
enabled,
71+
};
72+
}
73+
74+
function CreateQuickChecklist({
75+
componentId,
76+
channelId,
77+
channelName,
78+
currentUserId,
79+
currentTeamId,
80+
serverUrl,
81+
}: Props) {
82+
const theme = useTheme();
83+
const intl = useIntl();
84+
const styles = getStyleSheet(theme);
85+
86+
const [checklistName, setChecklistName] = useState(`${channelName} Checklist`);
87+
const [description, setDescription] = useState('');
88+
const canSave = Boolean(checklistName.trim());
89+
90+
const handleCreate = usePreventDoubleTap(useCallback(async () => {
91+
const res = await createPlaybookRun(
92+
serverUrl,
93+
'', // empty playbook_id for standalone checklist
94+
currentUserId,
95+
currentTeamId,
96+
checklistName.trim(),
97+
description.trim(),
98+
channelId,
99+
);
100+
101+
if (res.error || !res.data) {
102+
logError('error on createPlaybookRun', getFullErrorMessage(res.error));
103+
showPlaybookErrorSnackbar();
104+
return;
105+
}
106+
107+
// Pop the create screen first, then navigate to the run
108+
// This ensures the back button goes back to the channel, not the create screen
109+
await popTopScreen(componentId);
110+
await fetchPlaybookRunsForChannel(serverUrl, channelId);
111+
await goToPlaybookRun(intl, res.data.id);
112+
}, [serverUrl, currentUserId, currentTeamId, checklistName, description, channelId, componentId, intl]));
113+
114+
useEffect(() => {
115+
async function asyncWrapper() {
116+
const leftButton = await makeLeftButton(theme);
117+
const rightButton = makeRightButton(theme, intl, canSave);
118+
119+
setButtons(
120+
componentId,
121+
{
122+
leftButtons: [leftButton],
123+
rightButtons: [rightButton],
124+
},
125+
);
126+
}
127+
128+
asyncWrapper();
129+
}, [componentId, theme, intl, canSave]);
130+
131+
const close = useCallback(() => {
132+
Keyboard.dismiss();
133+
popTopScreen(componentId);
134+
}, [componentId]);
135+
136+
useNavButtonPressed(CREATE_BUTTON_ID, componentId, handleCreate, [handleCreate]);
137+
useNavButtonPressed(CLOSE_BUTTON_ID, componentId, close, [close]);
138+
useAndroidHardwareBackHandler(componentId, close);
139+
140+
return (
141+
<SafeAreaView style={styles.container}>
142+
<ScrollView
143+
style={styles.content}
144+
contentContainerStyle={styles.contentContainer}
145+
>
146+
<FloatingTextInput
147+
label={intl.formatMessage({
148+
id: 'playbooks.start_run.run_name_label',
149+
defaultMessage: 'Name',
150+
})}
151+
placeholder={intl.formatMessage({
152+
id: 'playbooks.start_run.run_name_placeholder',
153+
defaultMessage: 'Add a name',
154+
})}
155+
value={checklistName}
156+
onChangeText={setChecklistName}
157+
theme={theme}
158+
testID='create_quick_checklist.name_input'
159+
error={checklistName.trim() ? undefined : intl.formatMessage({
160+
id: 'playbooks.start_run.run_name_error',
161+
defaultMessage: 'Please add a name',
162+
})}
163+
/>
164+
<FloatingTextInput
165+
label={intl.formatMessage({
166+
id: 'playbooks.start_checklist.description_label',
167+
defaultMessage: 'Description (optional)',
168+
})}
169+
value={description}
170+
onChangeText={setDescription}
171+
multiline={true}
172+
multilineInputHeight={100}
173+
theme={theme}
174+
testID='create_quick_checklist.description_input'
175+
/>
176+
</ScrollView>
177+
</SafeAreaView>
178+
);
179+
}
180+
181+
export default CreateQuickChecklist;
182+
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
2+
// See LICENSE.txt for license information.
3+
4+
import CreateQuickChecklist from './create_quick_checklist';
5+
6+
export default CreateQuickChecklist;
7+

app/products/playbooks/screens/index.test.tsx

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {withServerDatabase} from '@database/components';
77
import Screens from '@playbooks/constants/screens';
88
import {render} from '@test/intl-test-helper';
99

10+
import CreateQuickChecklist from './create_quick_checklist';
1011
import EditCommand from './edit_command';
1112
import ParticipantPlaybooks from './participant_playbooks';
1213
import PlaybookRun from './playbook_run';
@@ -65,6 +66,12 @@ jest.mock('@playbooks/screens/start_a_run', () => ({
6566
}));
6667
jest.mocked(StartARun).mockImplementation((props) => <Text {...props}>{Screens.PLAYBOOKS_START_A_RUN}</Text>);
6768

69+
jest.mock('@playbooks/screens/create_quick_checklist', () => ({
70+
__esModule: true,
71+
default: jest.fn(),
72+
}));
73+
jest.mocked(CreateQuickChecklist).mockImplementation((props) => <Text {...props}>{Screens.PLAYBOOKS_CREATE_QUICK_CHECKLIST}</Text>);
74+
6875
jest.mock('@playbooks/screens/select_playbook', () => ({
6976
__esModule: true,
7077
default: jest.fn(),

app/products/playbooks/screens/index.tsx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ export function loadPlaybooksScreen(screenName: string | number) {
3030
return withServerDatabase(require('@playbooks/screens/select_playbook').default);
3131
case Screens.PLAYBOOKS_START_A_RUN:
3232
return withServerDatabase(require('@playbooks/screens/start_a_run').default);
33+
case Screens.PLAYBOOKS_CREATE_QUICK_CHECKLIST:
34+
return withServerDatabase(require('@playbooks/screens/create_quick_checklist').default);
3335
default:
3436
return undefined;
3537
}

0 commit comments

Comments
 (0)