-
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
Scheduled post Actions #8603
base: list-scheduled-post
Are you sure you want to change the base?
Scheduled post Actions #8603
Conversation
ad1c32e
to
10b0d66
Compare
de2b148
to
3a6f367
Compare
cd6e159
to
2bd8672
Compare
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.
LGTM!
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.
Some comments to address, but in general it looks good.
deleteScheduledPostConfirmation(propsWithoutSwipeable); | ||
|
||
// Get the cancel button callback | ||
const cancelButton = (Alert.alert as jest.Mock).mock.calls[0][2][0]; |
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.
Use jest.mocked(Alert.alert).mock...
instead of casting it to jest.Mock. That way you keep type safety.
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.
By doing this it was giving a linter error Object can be undefined.
.
app/utils/scheduled_post/index.ts
Outdated
if (swipeable?.current) { | ||
swipeable.current.close(); | ||
} |
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.
0/5 Just for simplicity:
if (swipeable?.current) { | |
swipeable.current.close(); | |
} | |
swipeable.current?.close(); |
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.
Updated the code.
swipeable?: React.RefObject<SwipeableMethods>; | ||
}) { | ||
const deleteScheduledPostOnConfirm = async () => { | ||
await deleteScheduledPost(serverUrl, scheduledPostId); |
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 we want to do anything if this fails?
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.
This is handled in this PR: #8634
swipeable?: React.RefObject<SwipeableMethods>; | ||
}) { | ||
const deleteScheduledPostOnConfirm = async () => { | ||
await deleteScheduledPost(serverUrl, scheduledPostId); |
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.
Also, if this is the whole body, I don't think we need an async function. If I am not mistaken, the alert button will just call this function and forget. It will not wait for the promise to be resolved. So, if we keep it like this, there is no need for it to be a async function, or to be awaiting.
const baseProps: { | ||
intl: IntlShape; | ||
serverUrl: string; | ||
scheduledPostId: string; | ||
swipeable?: React.RefObject<SwipeableMethods>; | ||
} = { |
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.
You can use something like Parameters<T>[0]
so you don't have to rewrite the type.
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.
Done.
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.
Why not name this file index.test.tsx
if it is about the index file?
it('should match snapshot', () => { | ||
const wrapper = renderWithEverything( | ||
<SendHandler {...baseProps}/>, {database}, | ||
); | ||
expect(wrapper.toJSON()).toBeTruthy(); | ||
}); |
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.
This is not matching any snapshot, and not sure how useful it is.
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.
Yup, that correct, better to remove this from the test.
await waitFor(() => { | ||
expect(wrapper.toJSON()).toBeTruthy(); | ||
}); |
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.
Not sure what is this for.
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.
Not necessary, removed it.
const sendDraftButton = wrapper.getByTestId('send_draft_button'); | ||
expect(sendDraftButton).toBeTruthy(); |
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.
Not sure how this translate to "should pass correct props".
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.
Updated the test.
expect(draftInput).toBeTruthy(); | ||
|
||
await waitFor(() => { | ||
expect(wrapper.toJSON()).toBeTruthy(); | ||
}); |
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.
Not sure how this translates to should handle send message action
.
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.
Updated the test.
68bf2da
to
63d31e5
Compare
2bd8672
to
5517875
Compare
Summary
Ticket Link
Checklist
E2E iOS tests for PR
.Device Information
This PR was tested on:
Screenshots
Release Note