Error handling for API call fails for Scheduled post #8634
Conversation
d6f6e0e to
2b60d2c
Compare
d53d2d5 to
2b68e34
Compare
| @@ -847,6 +847,7 @@ | |||
| "msg_typing.areTyping": "{users} and {last} are typing...", | |||
| "msg_typing.isTyping": "{user} is typing...", | |||
| "native.ios.notifications.not_verified": "We could not verify this notification with the server", | |||
| "network_connection.not_connected": "No internet connection", | |||
There was a problem hiding this comment.
This is not factually correct. You can have internet connection but not being in the same VPN as the server, or the server may be down.
@abhijit-singh @cwarnermm @enahum We should come up with a messaging that we are comfortable with, and use it in all similar places, and somehow document it so we know for sure what is the messaging we want to give.
My proposal here is:
"Cannot reach the server"
There was a problem hiding this comment.
100000% agree!! In fact I added some commets about this in the PR
| if (res?.error) { | ||
| showSnackBar({ | ||
| barType: SNACK_BAR_TYPE.DELETE_SCHEDULED_POST_ERROR, | ||
| customMessage: (res.error as Error).message, |
There was a problem hiding this comment.
We have an tuil function to deal with this: getErrorMessage. Use that instead.
| @@ -108,6 +116,27 @@ describe('deleteScheduledPostConfirmation', () => { | |||
| // Should not throw when swipeable.current is null | |||
| expect(() => cancelButton.onPress()).not.toThrow(); | |||
| }); | |||
|
|
|||
| it('shows error snackbar when deleteScheduledPost fails', async () => { | |||
| const showSnackBar = require('@utils/snack_bar').showSnackBar; | |||
There was a problem hiding this comment.
Why do we use require here instead of a normal import?
| it('shows error snackbar when deleteScheduledPost fails', async () => { | ||
| const showSnackBar = require('@utils/snack_bar').showSnackBar; | ||
| const errorMessage = 'Failed to delete scheduled post'; | ||
| (deleteScheduledPost as jest.Mock).mockResolvedValueOnce({ |
There was a problem hiding this comment.
Use jest.mocked instead of as jest.Mock.
| deleteScheduledPostConfirmation(baseProps); | ||
|
|
||
| // Get the confirm button callback | ||
| const confirmButton = (Alert.alert as jest.Mock).mock.calls[0][2][1]; |
There was a problem hiding this comment.
Use jest.mocked instead of as jest.Mock.
| // We need to directly mock the fireEvent.press to make it work | ||
| const originalFireEvent = fireEvent.press; | ||
| fireEvent.press = jest.fn((element) => { | ||
| const result = originalFireEvent(element); | ||
|
|
||
| // If this is the send button, trigger the sendMessageWithAlert mock | ||
| if (element.props.testID === 'send_draft_button') { | ||
| setTimeout(() => { | ||
| sendMessageWithAlert({} as Parameters<typeof sendMessageWithAlert>[0]); | ||
| }, 0); | ||
| } | ||
|
|
||
| return result; | ||
| }); |
There was a problem hiding this comment.
This feels really wrong. We should find a way to do this without mocking the fire event. If you want, schedule some time with me and we can look at how to handle this.
| await waitFor(() => expect(sendMessageWithAlert).toHaveBeenCalled()); | ||
|
|
||
| // Use a simple timeout instead of waitFor | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); |
There was a problem hiding this comment.
You can use TestHelper wait for this.
| fireEvent.press(sendDraftButton); | ||
| await waitFor(() => expect(sendMessageWithAlert).toHaveBeenCalled()); | ||
|
|
||
| // Use a simple timeout instead of waitFor |
There was a problem hiding this comment.
(For my own knowledge) Why waitFor doesn't work here?
| return Promise.resolve(); | ||
| }); | ||
| // Reset the mock first | ||
| (sendMessageWithAlert as jest.Mock).mockReset(); |
There was a problem hiding this comment.
jest.mocked instead of as jest.mock
| // Manually call clearDraft to verify it works | ||
| props.clearDraft(); | ||
| expect(mockClearDraft).toHaveBeenCalled(); |
There was a problem hiding this comment.
This is testing nothing. If you call it of course it has been called.
| await act(async () => { | ||
| fireEvent.press(sendButton); | ||
| }); | ||
|
|
||
| await waitFor(() => { | ||
| expect(sendMessageWithAlert).toHaveBeenCalledWith(expect.objectContaining({ | ||
| channelName: 'test-channel', | ||
| title: expect.any(String), | ||
| intl: expect.any(Object), | ||
| sendMessageHandler: expect.any(Function), | ||
| })); | ||
| }); |
There was a problem hiding this comment.
As far as I know, this is the proper way of doing it
| if (websocketState !== 'connected') { | ||
| showSnackBar({ | ||
| barType: SNACK_BAR_TYPE.CONNECTION_ERROR, | ||
| customMessage: intl.formatMessage({id: 'network_connection.not_connected', defaultMessage: 'No internet connection'}), |
There was a problem hiding this comment.
Really bad error message, this should definitely be changed, device may have internet but the connection to the websocket fails for whatever reason. Also, why rely on the WS connection state here?
| if (websocketState !== 'connected') { | ||
| showSnackBar({ | ||
| barType: SNACK_BAR_TYPE.CONNECTION_ERROR, | ||
| customMessage: intl.formatMessage({id: 'network_connection.not_connected', defaultMessage: 'No internet connection'}), |
There was a problem hiding this comment.
Same comment and question as before
| import {useTheme} from '@context/theme'; | ||
| import {useHandleSendMessage} from '@hooks/handle_send_message'; | ||
| import {usePersistentNotificationProps} from '@hooks/persistent_notification_props'; | ||
| import websocket_manager from '@managers/websocket_manager'; |
| if (websocketState !== 'connected') { | ||
| showSnackBar({ | ||
| barType: SNACK_BAR_TYPE.CONNECTION_ERROR, | ||
| customMessage: intl.formatMessage({id: 'network_connection.not_connected', defaultMessage: 'No internet connection'}), |
| @@ -847,6 +847,7 @@ | |||
| "msg_typing.areTyping": "{users} and {last} are typing...", | |||
| "msg_typing.isTyping": "{user} is typing...", | |||
| "native.ios.notifications.not_verified": "We could not verify this notification with the server", | |||
| "network_connection.not_connected": "No internet connection", | |||
There was a problem hiding this comment.
100000% agree!! In fact I added some commets about this in the PR
Summary
Ticket Link
Checklist
E2E iOS tests for PR.Device Information
This PR was tested on:
Screenshots
Release Note