-
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 72 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 |
---|---|---|
@@ -0,0 +1,166 @@ | ||
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. | ||
// See LICENSE.txt for license information. | ||
|
||
import {fireEvent} from '@testing-library/react-native'; | ||
import React from 'react'; | ||
|
||
import {Screens} from '@constants'; | ||
import {PostPriorityType} from '@constants/post'; | ||
import NetworkManager from '@managers/network_manager'; | ||
import {openAsBottomSheet} from '@screens/navigation'; | ||
import {renderWithEverything} from '@test/intl-test-helper'; | ||
import TestHelper from '@test/test_helper'; | ||
import {persistentNotificationsConfirmation} from '@utils/post'; | ||
|
||
import DraftInput from './draft_input'; | ||
|
||
import type ServerDataOperator from '@database/operator/server_data_operator'; | ||
import type {Database} from '@nozbe/watermelondb'; | ||
|
||
const SERVER_URL = 'https://appv1.mattermost.com'; | ||
|
||
// this is needed to when using the useServerUrl hook | ||
jest.mock('@context/server', () => ({ | ||
useServerUrl: jest.fn(() => SERVER_URL), | ||
})); | ||
|
||
jest.mock('@screens/navigation', () => ({ | ||
openAsBottomSheet: jest.fn(), | ||
})); | ||
|
||
jest.mock('@utils/post', () => ({ | ||
persistentNotificationsConfirmation: jest.fn(), | ||
})); | ||
|
||
describe('DraftInput', () => { | ||
const baseProps = { | ||
testID: 'draft_input', | ||
channelId: 'channelId', | ||
channelType: 'O' as ChannelType, | ||
currentUserId: 'currentUserId', | ||
postPriority: {priority: ''} as PostPriority, | ||
updatePostPriority: jest.fn(), | ||
persistentNotificationInterval: 0, | ||
persistentNotificationMaxRecipients: 0, | ||
updateCursorPosition: jest.fn(), | ||
cursorPosition: 0, | ||
sendMessage: jest.fn(), | ||
canSend: true, | ||
maxMessageLength: 4000, | ||
files: [], | ||
value: '', | ||
uploadFileError: null, | ||
updateValue: jest.fn(), | ||
addFiles: jest.fn(), | ||
updatePostInputTop: jest.fn(), | ||
setIsFocused: jest.fn(), | ||
scheduledPostsEnabled: true, | ||
}; | ||
|
||
beforeEach(() => { | ||
jest.clearAllMocks(); | ||
}); | ||
|
||
let database: Database; | ||
let operator: ServerDataOperator; | ||
|
||
beforeEach(async () => { | ||
const server = await TestHelper.setupServerDatabase(SERVER_URL); | ||
database = server.database; | ||
operator = server.operator; | ||
}); | ||
|
||
afterEach(async () => { | ||
await TestHelper.tearDown(); | ||
NetworkManager.invalidateClient(SERVER_URL); | ||
}); | ||
|
||
describe('Rendering', () => { | ||
it('renders base components', async () => { | ||
const {getByTestId} = renderWithEverything(<DraftInput {...baseProps}/>, {database}); | ||
expect(getByTestId('draft_input')).toBeTruthy(); | ||
expect(getByTestId('draft_input.post.input')).toBeTruthy(); | ||
expect(getByTestId('draft_input.quick_actions')).toBeTruthy(); | ||
expect(getByTestId('draft_input.send_action.send.button')).toBeTruthy(); | ||
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. Any strong opinion on why to use 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. Updated elsewhere as well. |
||
}); | ||
|
||
it('shows upload error when present', () => { | ||
const props = {...baseProps, uploadFileError: 'Error message'}; | ||
const {getByText} = renderWithEverything(<DraftInput {...props}/>, {database}); | ||
expect(getByText('Error message')).toBeTruthy(); | ||
}); | ||
}); | ||
|
||
describe('Message Actions', () => { | ||
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. There are a couple of details you are not testing. Mainly that you are passing the right action to the popups (i.e. What could be done to test that is the following:
This way you verify that if the bottom sheet (or the verification for priority messages) does what you expect them to do, your code does things right (in other words... you are passing the right functions into the right places). 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('sends message on press', () => { | ||
const {getByTestId} = renderWithEverything(<DraftInput {...baseProps}/>, {database}); | ||
fireEvent.press(getByTestId('draft_input.send_action.send.button')); | ||
expect(baseProps.sendMessage).toHaveBeenCalled(); | ||
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. Should you verify it has been called with no arguments? 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. ^^ Would it be beneficial to use toHaveBeenCalledWith() instead, and confirm sendMessage() is passing the right arguments? 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('opens scheduled post options on long press', async () => { | ||
// make this a re-usable function | ||
await operator.handleConfigs({ | ||
configs: [ | ||
{id: 'ScheduledPosts', value: 'true'}, | ||
], | ||
configsToDelete: [], | ||
prepareRecordsOnly: false, | ||
}); | ||
|
||
const {getByTestId} = renderWithEverything(<DraftInput {...baseProps}/>, {database}); | ||
fireEvent(getByTestId('draft_input.send_action.send.button'), 'longPress'); | ||
expect(openAsBottomSheet).toHaveBeenCalledWith(expect.objectContaining({ | ||
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. Just as sanity check, we could add here also a check to make sure the other function ( 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. |
||
screen: Screens.SCHEDULED_POST_OPTIONS, | ||
})); | ||
}); | ||
|
||
it('handles persistent notifications', async () => { | ||
jest.mocked(persistentNotificationsConfirmation).mockResolvedValueOnce(); | ||
const props = { | ||
...baseProps, | ||
postPriority: { | ||
persistent_notifications: true, | ||
priority: PostPriorityType.URGENT, | ||
} as PostPriority, | ||
value: '@user1 @user2 message', | ||
}; | ||
|
||
const {getByTestId} = renderWithEverything(<DraftInput {...props}/>, {database}); | ||
fireEvent.press(getByTestId('draft_input.send_action.send.button')); | ||
expect(persistentNotificationsConfirmation).toHaveBeenCalled(); | ||
}); | ||
}); | ||
|
||
describe('Input Handling', () => { | ||
it('updates text value', () => { | ||
const {getByTestId} = renderWithEverything(<DraftInput {...baseProps}/>, {database}); | ||
fireEvent.changeText(getByTestId('draft_input.post.input'), 'new message'); | ||
expect(baseProps.updateValue).toHaveBeenCalledWith('new message'); | ||
}); | ||
|
||
it('handles cursor position', () => { | ||
const {getByTestId} = renderWithEverything(<DraftInput {...baseProps}/>, {database}); | ||
fireEvent(getByTestId('draft_input.post.input'), 'selectionChange', { | ||
nativeEvent: {selection: {start: 5, end: 5}}, | ||
}); | ||
expect(baseProps.updateCursorPosition).toHaveBeenCalledWith(5); | ||
}); | ||
|
||
it('updates focus state', () => { | ||
const {getByTestId} = renderWithEverything(<DraftInput {...baseProps}/>, {database}); | ||
fireEvent(getByTestId('draft_input.post.input'), 'focus'); | ||
expect(baseProps.setIsFocused).toHaveBeenCalledWith(true); | ||
}); | ||
}); | ||
|
||
describe('Validation', () => { | ||
it('disables send when canSend is false', () => { | ||
const props = {...baseProps, canSend: false}; | ||
const {getByTestId} = renderWithEverything(<DraftInput {...props}/>, {database}); | ||
const sendButton = getByTestId('draft_input.send_action.send.button.disabled'); | ||
expect(sendButton).toBeTruthy(); | ||
expect(sendButton).toBeDisabled(); | ||
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. Again, just as sanity check, we could double check that both press and long press actions do not call the functions, since the button is disabled. But that may be already covered by the button being marked as disabled... 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 |
---|---|---|
@@ -0,0 +1,60 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`components/post_draft/send_action should match snapshot 1`] = ` | ||
<View | ||
accessibilityState={ | ||
{ | ||
"busy": undefined, | ||
"checked": undefined, | ||
"disabled": false, | ||
"expanded": undefined, | ||
"selected": undefined, | ||
} | ||
} | ||
accessibilityValue={ | ||
{ | ||
"max": undefined, | ||
"min": undefined, | ||
"now": undefined, | ||
"text": undefined, | ||
} | ||
} | ||
accessible={true} | ||
collapsable={false} | ||
focusable={true} | ||
onClick={[Function]} | ||
onResponderGrant={[Function]} | ||
onResponderMove={[Function]} | ||
onResponderRelease={[Function]} | ||
onResponderTerminate={[Function]} | ||
onResponderTerminationRequest={[Function]} | ||
onStartShouldSetResponder={[Function]} | ||
style={ | ||
{ | ||
"justifyContent": "flex-end", | ||
"opacity": 1, | ||
"paddingRight": 8, | ||
} | ||
} | ||
testID="test_id.send.button" | ||
> | ||
<View | ||
style={ | ||
{ | ||
"alignItems": "center", | ||
"backgroundColor": "#1c58d9", | ||
"borderRadius": 4, | ||
"height": 32, | ||
"justifyContent": "center", | ||
"width": 80, | ||
} | ||
} | ||
> | ||
<Icon | ||
color="#ffffff" | ||
name="send" | ||
size={24} | ||
/> | ||
</View> | ||
</View> | ||
`; |
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.