Create scheduled post api integration #8519

harshilsharma63 · 2025-01-24T08:15:30Z

Summary

Integrated create scheduled post API

Ticket Link

Part of https://mattermost.atlassian.net/browse/MM-62279

Checklist

Added or updated unit tests (required for all new features)
Has UI changes
Includes text changes and localization file updates
Have tested against the 5 core themes to ensure consistency between them.
Have run E2E tests by adding label E2E iOS tests for PR.

Device Information

iOS emulator

Screenshots

Recording.at.2025-01-24.13.56.28.Cropped.in.2025-01-24.13.57.29.mp4

Release Note

None

…_api_integration

…integration

larkox

Some comments on the tests. I haven't looked at them all yet, so I will keep adding comments when I get back to this.

Nevertheless, nothing blocking.

larkox · 2025-01-31T10:39:18Z

app/actions/remote/scheduled_post.ts

-        }
-        return {scheduledPost: created};
+        // TODO - record scheduled post in database here
+        return {data: true, response};


I still think that data should be the response directly, not a boolean. See for example fetchThread.

I would tend to agree with you @larkox, but apparently this is an else for createPost(), which also returns {data: true/false}.

It has to be data: boolean and response separately as doSubmitMessage call either createPost or createScheduledPost and both need to return the same structure. Create Post returns data: boolean, and so for that reason, so does createScheduledPost have to and updating createPost is beyond the scope of this feature.

larkox · 2025-01-31T10:42:57Z

app/actions/remote/scheduled_post.ts

-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> {


(Not a request for changes, just a knowledge share)
In general for remote actions we add another optional parameter fetchOnly, default to false. This is so we can decide not to update the local state when we get certain information. This may be done for several reasons, like fetching data that is not going to be kept up to date (nor by websocket nor syncing logic), so we don't want it in the database. This could be, for example, browse channels results.

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.

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.

larkox · 2025-01-31T10:45:33Z

app/actions/remote/scheduled_post.test.ts

+        expect(result).toBeDefined();
+        expect(result.error).toBeDefined();


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

I like it meticulous. Say somebody remove the forceLogoutIfNecessary by accident, that meticulousness will flag and bring a discussion if that removal is necessary.

larkox · 2025-01-31T10:52:17Z

app/components/post_draft/draft_input/draft_input.test.tsx

+            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();


Any strong opinion on why to use toBeTruthy and not toBeVisible? In theory toBeVisible will check a few more interesting things, like that it has a size, but 0/5.

Done. Updated elsewhere as well.

larkox · 2025-01-31T10:53:27Z

app/components/post_draft/draft_input/draft_input.test.tsx

+        it('sends message on press', () => {
+            const {getByTestId} = renderWithEverything(<DraftInput {...baseProps}/>, {database});
+            fireEvent.press(getByTestId('draft_input.send_action.send.button'));
+            expect(baseProps.sendMessage).toHaveBeenCalled();


Should you verify it has been called with no arguments?

^^

Would it be beneficial to use toHaveBeenCalledWith() instead, and confirm sendMessage() is passing the right arguments?

larkox · 2025-01-31T11:04:04Z

app/components/post_draft/draft_input/draft_input.test.tsx

+        });
+    });
+
+    describe('Message Actions', () => {


There are a couple of details you are not testing. Mainly that you are passing the right action to the popups (i.e. openAsBottomSheet and persistentNotificationsConfirmation).

What could be done to test that is the following:

Mock the implementation of openAsBottomSheet (and similar for persistentNotificationsConfirmation) like this:

jest.mocked(openAsBottomSheet).mockImplementation(({props: {onSchedule}}) => onSchedule(SCHEDULING_INFO))

Verify send message is called

Verify that if you had post priority, you still get the warning

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).

larkox · 2025-01-31T11:05:59Z

app/components/post_draft/draft_input/draft_input.test.tsx

+
+            const {getByTestId} = renderWithEverything(<DraftInput {...baseProps}/>, {database});
+            fireEvent(getByTestId('draft_input.send_action.send.button'), 'longPress');
+            expect(openAsBottomSheet).toHaveBeenCalledWith(expect.objectContaining({


Just as sanity check, we could add here also a check to make sure the other function (sendMessage if I am not mistaken) is not called.

larkox · 2025-01-31T11:08:38Z

app/components/post_draft/draft_input/draft_input.test.tsx

+            const {getByTestId} = renderWithEverything(<DraftInput {...props}/>, {database});
+            const sendButton = getByTestId('draft_input.send_action.send.button.disabled');
+            expect(sendButton).toBeTruthy();
+            expect(sendButton).toBeDisabled();


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...

rahimrahman

To be continued...

rahimrahman · 2025-01-31T17:43:52Z

app/components/post_draft/send_action/send_action.test.tsx

+        const {toJSON} = renderWithIntl(
+            <SendAction {...baseProps}/>,
+        );
+        expect(toJSON()).toMatchSnapshot();


I would love to not have this. But if you insist, please tell me in test words that the send button is there, or some sort of text like "Send"?

Done, replaced by specific tests.

rahimrahman · 2025-01-31T17:48:57Z

app/components/post_draft/send_action/index.tsx

@@ -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';


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.

If the file is for send_action, why not call the component SendAction? It it's SendButton component, why not make the file name send_button.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 .

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.

rahimrahman · 2025-01-31T17:51:13Z

app/actions/remote/scheduled_post.ts

-        }
-        return {scheduledPost: created};
+        // TODO - record scheduled post in database here
+        return {data: true, response};


I would tend to agree with you @larkox, but apparently this is an else for createPost(), which also returns {data: true/false}.

rahimrahman · 2025-01-31T17:51:48Z

app/actions/remote/scheduled_post.test.ts

+    // eslint-disable-next-line
+    // @ts-ignore


Do you need both?

No, removed and used ts-expect-error, otherwise we'd need both.

rahimrahman · 2025-01-31T17:53:06Z

app/actions/remote/scheduled_post.test.ts

+        expect(result).toBeDefined();
+        expect(result.error).toBeDefined();


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.

rahimrahman · 2025-01-31T17:56:08Z

app/actions/remote/scheduled_post.test.ts

+        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);
+        }
+    });


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.

rahimrahman · 2025-01-31T18:01:06Z

app/components/post_draft/draft_input/draft_input.test.tsx

+        it('sends message on press', () => {
+            const {getByTestId} = renderWithEverything(<DraftInput {...baseProps}/>, {database});
+            fireEvent.press(getByTestId('draft_input.send_action.send.button'));
+            expect(baseProps.sendMessage).toHaveBeenCalled();


^^

Would it be beneficial to use toHaveBeenCalledWith() instead, and confirm sendMessage() is passing the right arguments?

…t tests

harshilsharma63 and others added 24 commits January 16, 2025 16:35

WIP

b7d0ba6

Separate component for core and custom options

cb9a62b

WIP

b2b144d

WIP

7a13704

WIP

52bfcdd

Merge branch 'feature_schedule_posts' into show_scheduled_post_options

08c9a53

unified options into single component

5ea68e1

Scheduled post menu ready

2a2d101

Merge branch 'feature_schedule_posts' into show_scheduled_post_options

37aa32b

Displayed date time picker by default

d62bb3c

WIP

4be9065

Lint fix

96dcab2

i18n fix

7dc66b5

removed unused screen

27344f1

API call sent successfully

34f7ced

Merge branch 'feature_schedule_posts' into show_scheduled_post_options

e5527d0

Added user time format preference

4e893a1

review fixex and improvements

d34305d

Merge branch 'show_scheduled_post_options' into create_scheduled_post…

d6ab9d1

…_api_integration

Handled loading sttae

2edf74a

Handled loading sttae

976af46

Handled mentions and files and priority

e832f05

Cleanup

82c8fb0

lint fix

f08a7e6

mm-cloud-bot added the release-note label Jan 24, 2025

harshilsharma63 requested review from rahimrahman and devinbinnie January 24, 2025 08:28

mm-cloud-bot added release-note-none and removed release-note labels Jan 24, 2025

i18n fix

1498cb9

harshilsharma63 added 13 commits January 31, 2025 11:48

Added scheuidled post picker tests

d280d27

test: Add empty test file for scheduled post footer

ed7c9ad

test: Add comprehensive tests for ScheduledPostFooter component

37a0acc

feat: Add animatedFooterPosition prop to ScheduledPostFooter test

7ba4d9c

test: Add tablet rendering test for ScheduledPostFooter

2e4e447

test: Update ScheduledPostFooter test mocks and remove unused imports

8595da3

test: Add mocks for BottomSheet context in scheduled post footer tests

688ef6d

Added scheudled post footer test

d64f8e0

test: Add comprehensive tests for handle_send_message hook

eac1e87

Enhanced handle send message tests

35e7460

cleanup

e1fbd58

Added missing snapshots

bbf2cba

Merge branch 'feature_schedule_posts' into create_scheduled_post_api_…

be5ba6b

…integration

amyblais added this to the v2.26.0 milestone Jan 31, 2025

larkox approved these changes Jan 31, 2025

View reviewed changes

rahimrahman reviewed Jan 31, 2025

View reviewed changes

harshilsharma63 added 14 commits February 3, 2025 09:51

test: Add error logging and force logout validation for scheduled pos…

a907328

…t tests

Updated test

b34fef9

used toBeVisible instead of toBeTruthy

67c03c1

test: Update test assertions to use toHaveBeenCalledWith

2c89ccb

test: Add assertions to prevent sendMessage when send button is disabled

8086888

test: Replace snapshot tests with explicit UI element checks

db90702

test improvements

df72fba

Deleted unused snapshots

d4cf535

Updated snaopshot

b940f71

feat: Add close button ID for scheduled post options bottom sheet

5a2cf9c

test: Add mock scheduling flow to scheduled post options test

9da20ba

Fixed incorrect test

5de1382

scheudled post test enhancement

fccde48

minimised a test

a9ffd89

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create scheduled post api integration #8519

Create scheduled post api integration #8519

harshilsharma63 commented Jan 24, 2025 •

edited

Loading

larkox left a comment

larkox Jan 31, 2025

rahimrahman Jan 31, 2025

harshilsharma63 Feb 3, 2025

larkox Jan 31, 2025

harshilsharma63 Feb 3, 2025

larkox Jan 31, 2025

rahimrahman Jan 31, 2025

harshilsharma63 Feb 3, 2025

larkox Jan 31, 2025

harshilsharma63 Feb 3, 2025

larkox Jan 31, 2025

rahimrahman Jan 31, 2025

harshilsharma63 Feb 3, 2025

larkox Jan 31, 2025

larkox Jan 31, 2025

harshilsharma63 Feb 3, 2025

larkox Jan 31, 2025

harshilsharma63 Feb 3, 2025

rahimrahman left a comment

rahimrahman Jan 31, 2025

harshilsharma63 Feb 3, 2025

rahimrahman Jan 31, 2025

harshilsharma63 Feb 3, 2025

rahimrahman Jan 31, 2025

rahimrahman Jan 31, 2025

harshilsharma63 Feb 3, 2025

rahimrahman Jan 31, 2025

harshilsharma63 Feb 3, 2025

rahimrahman Jan 31, 2025

harshilsharma63 Feb 3, 2025

rahimrahman Jan 31, 2025

		expect(result).toBeDefined();
		expect(result.error).toBeDefined();

Create scheduled post api integration #8519

Are you sure you want to change the base?

Create scheduled post api integration #8519

Conversation

harshilsharma63 commented Jan 24, 2025 • edited Loading

Summary

Ticket Link

Checklist

Device Information

Screenshots

Release Note

larkox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahimrahman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshilsharma63 commented Jan 24, 2025 •

edited

Loading