-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Create scheduled post api integration #8519
Changes from 42 commits
b7d0ba6
cb9a62b
b2b144d
7a13704
52bfcdd
08c9a53
5ea68e1
2a2d101
37aa32b
d62bb3c
4be9065
96dcab2
7dc66b5
27344f1
34f7ced
e5527d0
4e893a1
d34305d
d6ab9d1
2edf74a
976af46
e832f05
82c8fb0
f08a7e6
1498cb9
8a63e96
d443026
5ade7cd
bfaf507
06dbcdf
bdc6113
cd62280
7699011
69efb06
5141c7d
c06a248
7ad7a15
637d5d0
2d6eb3b
4c1e54f
90b28aa
40c3f8d
b57c3c3
dd5bb8c
f3a09f0
d322d8d
e8d9f46
0c5cd53
a69e820
08d125a
16158b4
48db248
7350939
fe617c9
8ebfd42
2f91aa9
20a1dd8
c3bb8a4
2edc2b8
d280d27
ed7c9ad
37a0acc
7ba4d9c
2e4e447
8595da3
688ef6d
d64f8e0
eac1e87
35e7460
e1fbd58
bbf2cba
be5ba6b
a907328
b34fef9
67c03c1
2c89ccb
8086888
db90702
df72fba
d4cf535
b940f71
5a2cf9c
9da20ba
5de1382
fccde48
a9ffd89
cb2e36e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. | ||
// See LICENSE.txt for license information. | ||
|
||
import DatabaseManager from '@database/manager'; | ||
import NetworkManager from '@managers/network_manager'; | ||
|
||
import {createScheduledPost} from './scheduled_post'; | ||
|
||
import type ServerDataOperator from '@database/operator/server_data_operator'; | ||
|
||
const serverUrl = 'baseHandler.test.com'; | ||
let operator: ServerDataOperator; | ||
|
||
const user1 = {id: 'userid1', username: 'user1', email: '[email protected]', roles: ''} as UserProfile; | ||
const scheduledPost = { | ||
id: 'scheduledPostId1', | ||
root_id: '', | ||
update_at: 0, | ||
channel_id: 'channelid1', | ||
message: 'Test message', | ||
scheduled_at: Date.now() + 10000, | ||
} as ScheduledPost; | ||
|
||
const throwFunc = () => { | ||
throw Error('error'); | ||
}; | ||
|
||
const mockClient = { | ||
createScheduledPost: jest.fn(() => ({...scheduledPost})), | ||
}; | ||
|
||
beforeAll(() => { | ||
// eslint-disable-next-line | ||
// @ts-ignore | ||
NetworkManager.getClient = () => mockClient; | ||
}); | ||
|
||
beforeEach(async () => { | ||
await DatabaseManager.init([serverUrl]); | ||
operator = DatabaseManager.serverDatabases[serverUrl]!.operator; | ||
}); | ||
|
||
afterEach(async () => { | ||
await DatabaseManager.destroyServerDatabase(serverUrl); | ||
}); | ||
|
||
describe('scheduled_post', () => { | ||
it('createScheduledPost - handle not found database', async () => { | ||
const result = await createScheduledPost('foo', scheduledPost); | ||
expect(result).toBeDefined(); | ||
expect(result.error).toBeDefined(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kinda redundant. If you have result.error, you know result is defined. But it would be more beneficial to know the error is what you expect. Like 'database not found' or something. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
}); | ||
|
||
it('createScheduledPost - base case', async () => { | ||
await operator.handleUsers({users: [user1], prepareRecordsOnly: false}); | ||
const result = await createScheduledPost(serverUrl, scheduledPost); | ||
expect(result).toBeDefined(); | ||
expect(result.error).toBeUndefined(); | ||
expect(result.data).toBe(true); | ||
expect(result.response).toBeDefined(); | ||
if (result.response) { | ||
expect(result.response.id).toBe(scheduledPost.id); | ||
} | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above, I think you can skip result toBeDefined(). The check for result.response is unnecessary if you have already expect result.response toBeDefined() right? I barely see any need for if-else statements in test. You're not creating conditions in your test, you're trying to paint a story of what you're expecting. In this instance: expect(result.response.id).toBe(scheduledPost.id); negates the need to check if it's defined, or that result.response is true/exists. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
it('createScheduledPost - request error', async () => { | ||
mockClient.createScheduledPost.mockImplementationOnce(jest.fn(throwFunc)); | ||
const result = await createScheduledPost(serverUrl, scheduledPost); | ||
expect(result).toBeDefined(); | ||
expect(result.error).toBeDefined(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: We could check that on error, we are also logging the error and calling logout if necessary. We would just have to mock the logDebug and the forceLogoutIfNecessary and making sure they are called. You could also make sure the error logged / returned is the one returned by the client function. 0/5 if we have to be so meticulous though There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like it meticulous. Say somebody remove the forceLogoutIfNecessary by accident, that meticulousness will flag and bring a discussion if that removal is necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,26 +2,28 @@ | |
// See LICENSE.txt for license information. | ||
|
||
import {forceLogoutIfNecessary} from '@actions/remote/session'; | ||
import DatabaseManager from '@database/manager'; | ||
import NetworkManager from '@managers/network_manager'; | ||
import websocketManager from '@managers/websocket_manager'; | ||
import {getFullErrorMessage} from '@utils/errors'; | ||
import {logError} from '@utils/log'; | ||
|
||
export async function createScheduledPost(serverUrl: string, schedulePost: ScheduledPost, connectionId?: string, fetchOnly = false) { | ||
import type {CreateResponse} from '@hooks/handle_send_message'; | ||
|
||
export async function createScheduledPost(serverUrl: string, schedulePost: ScheduledPost, connectionId?: string): Promise<CreateResponse> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Not a request for changes, just a knowledge share) In this particular case, I don't foresee any case where we want to create a schedule post and not update the local database, so it should be just fine not having the parameter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had fetchOnly but since this doesn't make aAPI calls, the linter complained about an ununsed param and I removed it. I will add it when needed. |
||
const operator = DatabaseManager.serverDatabases[serverUrl]?.operator; | ||
if (!operator) { | ||
return {error: `${serverUrl} database not found`}; | ||
} | ||
|
||
try { | ||
const client = NetworkManager.getClient(serverUrl); | ||
const ws = websocketManager.getClient(serverUrl); | ||
|
||
const created = await client.createScheduledPost(schedulePost, ws?.getConnectionId()); | ||
const response = await client.createScheduledPost(schedulePost, connectionId); | ||
|
||
if (!fetchOnly) { | ||
// TODO - to be implemented later once DB tables are ready | ||
// await operator.handleScheduledPost({scheduledPosts: [created], prepareRecordsOnly: false}); | ||
} | ||
return {scheduledPost: created}; | ||
// TODO - record scheduled post in database here | ||
return {data: true, response}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still think that data should be the response directly, not a boolean. See for example There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would tend to agree with you @larkox, but apparently this is an else for createPost(), which also returns {data: true/false}. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It has to be data: boolean and response separately as |
||
} catch (error) { | ||
logError('error on createScheduledPost', getFullErrorMessage(error)); | ||
forceLogoutIfNecessary(serverUrl, error); | ||
return {error}; | ||
return {error: getFullErrorMessage(error)}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import {View} from 'react-native'; | |
import CompassIcon from '@components/compass_icon'; | ||
import TouchableWithFeedback from '@components/touchable_with_feedback'; | ||
import {useTheme} from '@context/theme'; | ||
import {usePreventDoubleTap} from '@hooks/utils'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is for this entire file. And I'm still iffy on the standard. What I understand is if it's an index.tsx file, then this is an observable file. Like database observable file. If it's a presentation file, it would be named send_action.tsx.
When I search for SendAction, I was expecting this particular file to be showing in my search result, to make developer's experience a bit easier. I'm still new with the mobile repo, and the conventions, but we need to establish it properly. Open for discussion @larkox . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its an existing where I made a two line changes. The issues described above are existing, not new. A separate ticket can be filed for these unrelated changes. |
||
import {changeOpacity, makeStyleSheetFromTheme} from '@utils/theme'; | ||
|
||
type Props = { | ||
|
@@ -55,10 +56,12 @@ function SendButton({ | |
|
||
const buttonColor = disabled ? changeOpacity(theme.buttonColor, 0.5) : theme.buttonColor; | ||
|
||
const sendMessageWithDoubleTapPrevention = usePreventDoubleTap(sendMessage); | ||
|
||
return ( | ||
<TouchableWithFeedback | ||
testID={sendButtonTestID} | ||
onPress={sendMessage} | ||
onPress={sendMessageWithDoubleTapPrevention} | ||
style={style.sendButtonContainer} | ||
type={'opacity'} | ||
disabled={disabled} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import {getChannelTimezones} from '@actions/remote/channel'; | |
import {executeCommand, handleGotoLocation} from '@actions/remote/command'; | ||
import {createPost} from '@actions/remote/post'; | ||
import {handleReactionToLatestPost} from '@actions/remote/reactions'; | ||
import {createScheduledPost} from '@actions/remote/scheduled_post'; | ||
import {setStatus} from '@actions/remote/user'; | ||
import {handleCallsSlashCommand} from '@calls/actions'; | ||
import {Events, Screens} from '@constants'; | ||
|
@@ -18,11 +19,17 @@ import DraftUploadManager from '@managers/draft_upload_manager'; | |
import * as DraftUtils from '@utils/draft'; | ||
import {isReactionMatch} from '@utils/emoji/helpers'; | ||
import {getFullErrorMessage} from '@utils/errors'; | ||
import {preventDoubleTap} from '@utils/tap'; | ||
import {scheduledPostFromPost} from '@utils/post'; | ||
import {confirmOutOfOfficeDisabled} from '@utils/user'; | ||
|
||
import type CustomEmojiModel from '@typings/database/models/servers/custom_emoji'; | ||
|
||
export type CreateResponse = { | ||
data?: boolean; | ||
error?: unknown; | ||
response?: Post | ScheduledPost; | ||
} | ||
|
||
type Props = { | ||
value: string; | ||
channelId: string; | ||
|
@@ -86,7 +93,7 @@ export const useHandleSendMessage = ({ | |
setSendingMessage(false); | ||
}, [serverUrl, rootId, clearDraft]); | ||
|
||
const doSubmitMessage = useCallback(() => { | ||
const doSubmitMessage = useCallback(async (schedulingInfo?: SchedulingInfo): Promise<CreateResponse> => { | ||
const postFiles = files.filter((f) => !f.failed); | ||
const post = { | ||
user_id: currentUserId, | ||
|
@@ -105,20 +112,31 @@ export const useHandleSendMessage = ({ | |
}; | ||
} | ||
|
||
createPost(serverUrl, post, postFiles); | ||
let response: CreateResponse; | ||
if (schedulingInfo) { | ||
response = await createScheduledPost(serverUrl, scheduledPostFromPost(post, schedulingInfo, postPriority)); | ||
} else { | ||
response = await createPost(serverUrl, post, postFiles); | ||
} | ||
|
||
clearDraft(); | ||
setSendingMessage(false); | ||
DeviceEventEmitter.emit(Events.POST_LIST_SCROLL_TO_BOTTOM, rootId ? Screens.THREAD : Screens.CHANNEL); | ||
|
||
return response; | ||
}, [files, currentUserId, channelId, rootId, value, postPriority, serverUrl, clearDraft]); | ||
|
||
const showSendToAllOrChannelOrHereAlert = useCallback((calculatedMembersCount: number, atHere: boolean) => { | ||
const showSendToAllOrChannelOrHereAlert = useCallback((calculatedMembersCount: number, atHere: boolean, schedulingInfo?: SchedulingInfo) => { | ||
const notifyAllMessage = DraftUtils.buildChannelWideMentionMessage(intl, calculatedMembersCount, channelTimezoneCount, atHere); | ||
const cancel = () => { | ||
setSendingMessage(false); | ||
}; | ||
|
||
DraftUtils.alertChannelWideMention(intl, notifyAllMessage, doSubmitMessage, cancel); | ||
// Creating a wrapper function to pass the schedulingInfo to the doSubmitMessage function as the accepted | ||
// function signature causes conflict.\ | ||
// TODO for later - change alert message if this is a scheduled post | ||
const doSubmitMessageScheduledPostWrapper = () => doSubmitMessage(schedulingInfo); | ||
DraftUtils.alertChannelWideMention(intl, notifyAllMessage, doSubmitMessageScheduledPostWrapper, cancel); | ||
}, [intl, channelTimezoneCount, doSubmitMessage]); | ||
|
||
const sendCommand = useCallback(async () => { | ||
|
@@ -167,31 +185,34 @@ export const useHandleSendMessage = ({ | |
} | ||
}, [value, userIsOutOfOffice, serverUrl, intl, channelId, rootId, clearDraft, channelType, currentUserId]); | ||
|
||
const sendMessage = useCallback(() => { | ||
const sendMessage = useCallback(async (schedulingInfo?: SchedulingInfo) => { | ||
const notificationsToChannel = enableConfirmNotificationsToChannel && useChannelMentions; | ||
const toAllOrChannel = DraftUtils.textContainsAtAllAtChannel(value); | ||
const toHere = DraftUtils.textContainsAtHere(value); | ||
|
||
if (value.indexOf('/') === 0) { | ||
if (value.indexOf('/') === 0 && !schedulingInfo) { | ||
// Don't execute slash command when scheduling message | ||
sendCommand(); | ||
} else if (notificationsToChannel && membersCount > NOTIFY_ALL_MEMBERS && (toAllOrChannel || toHere)) { | ||
showSendToAllOrChannelOrHereAlert(membersCount, toHere && !toAllOrChannel); | ||
showSendToAllOrChannelOrHereAlert(membersCount, toHere && !toAllOrChannel, schedulingInfo); | ||
} else { | ||
doSubmitMessage(); | ||
return doSubmitMessage(schedulingInfo); | ||
} | ||
|
||
return Promise.resolve(); | ||
}, [enableConfirmNotificationsToChannel, useChannelMentions, value, membersCount, sendCommand, showSendToAllOrChannelOrHereAlert, doSubmitMessage]); | ||
|
||
const handleSendMessage = useCallback(preventDoubleTap(() => { | ||
const handleSendMessage = useCallback(async (schedulingInfo?: SchedulingInfo) => { | ||
if (!canSend) { | ||
return; | ||
return Promise.resolve(); | ||
} | ||
|
||
setSendingMessage(true); | ||
|
||
const match = isReactionMatch(value, customEmojis); | ||
if (match && !files.length) { | ||
handleReaction(match.emoji, match.add); | ||
return; | ||
return Promise.resolve(); | ||
} | ||
|
||
const hasFailedAttachments = files.some((f) => f.failed); | ||
|
@@ -201,14 +222,16 @@ export const useHandleSendMessage = ({ | |
}; | ||
const accept = () => { | ||
// Files are filtered on doSubmitMessage | ||
sendMessage(); | ||
sendMessage(schedulingInfo); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably want to do things (among others, wait for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm unable to test this out right now, but I've filed a ticket (https://mattermost.atlassian.net/browse/MM-62756) for handle this. |
||
}; | ||
|
||
DraftUtils.alertAttachmentFail(intl, accept, cancel); | ||
} else { | ||
sendMessage(); | ||
return sendMessage(schedulingInfo); | ||
} | ||
}), [canSend, value, handleReaction, files, sendMessage, customEmojis]); | ||
|
||
return Promise.resolve(); | ||
}, [canSend, value, customEmojis, files, handleReaction, intl, sendMessage]); | ||
|
||
useEffect(() => { | ||
getChannelTimezones(serverUrl, channelId).then(({channelTimezones}) => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, removed and used ts-expect-error, otherwise we'd need both.