Added rest client and local/remote/WebSocket handling for Scheduled Post #8526

Rajat-Dabade · 2025-01-28T12:36:43Z

Summary

This PR handles the rest clients, local/remote/WebSocket handling of scheduled posts.

Ticket Link

https://mattermost.atlassian.net/browse/MM-62763

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.

Release Note

NONE

rahimrahman · 2025-01-28T15:48:45Z

@Rajat-Dabade it would be beneficial to write unit test as we need to hit 80% of code coverage by the end of January.

app/client/rest/base.ts

app/actions/websocket/event.ts

app/actions/local/scheduled_post.ts

Rajat-Dabade · 2025-02-03T08:32:01Z

@rahimrahman I have added a test, can you please review this PR? Thank you.

larkox

There is a couple of things I would like moved around, but apart of that it looks great.

larkox · 2025-02-03T17:51:02Z

app/actions/local/scheduled_post.test.ts

+    await DatabaseManager.destroyServerDatabase(serverUrl);
+});
+
+describe('handleScheduledPosts', () => {


I would prefer to have a describe for each function.

larkox · 2025-02-03T17:55:19Z

app/actions/local/scheduled_post.ts

+    }
+}
+
+export async function handleCreateOrUpdateScheduledPost(serverUrl: string, msg: WebSocketMessage<ScheduledPostWebsocketEventPayload>, prepareRecordsOnly = false) {


This feels off. If this handles the websocket message, it should be in the websocket folder. If it is a generic local function (to be used by the websocket and the remote actions, for example), it should be agnostic to the websocket message (and for sure, not do the parsing).

0/5 on where this should be.

I initially referred to channelBookmarks, but after reviewing other files, I found your point valid. I've now moved it to the WebSocket folder.

larkox · 2025-02-03T18:06:33Z

app/actions/local/scheduled_post.test.ts

+        const mockScheduledPost = {
+            ...scheduledPosts[0],
+            toApi: jest.fn().mockResolvedValue(scheduledPosts[0]), // Mock `toApi`
+        };
+        jest.spyOn(database, 'get').mockImplementation(() => ({
+            query: jest.fn().mockReturnValue({
+                fetch: jest.fn().mockResolvedValue([mockScheduledPost]),
+            }),
+        }) as any);


Instead of mocking all this, you can just add the element to the database, and see if it gets deleted.

larkox · 2025-02-03T18:19:36Z

app/actions/remote/scheduled_post.test.ts

+    await DatabaseManager.destroyServerDatabase(serverUrl);
+});
+
+describe('fetch and delete scheduled posts', () => {


Use one describe for each function. That way the name of the test also can be simplified without so much repetition.

Updated the file.

rahimrahman

Excellent work adding your tests.

There's a few patterns that I thought was unnecessary and repeated multiple times.

You can check your coverage and add the output as markdown as a screenshot so you know you coverage is above 80%. I think there's a guidance of that somewhere (I'll look for it)

rahimrahman · 2025-02-04T04:30:16Z

app/actions/remote/scheduled_post.test.ts

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


Should you need to check result.toBeDefined(), when (result.error).toBeTrurthy()?

Would it be beneficial to know if what result.error.toEqual(something) instead of toBeTruthy()?

rahimrahman · 2025-02-04T04:32:22Z

app/actions/remote/scheduled_post.test.ts

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


Same as above and in other test cases in this file. Knowing that the result.xyz is equal to something pretty much (result).toBeDefined().

rahimrahman · 2025-02-04T04:43:57Z

app/actions/websocket/scheduled_post.test.ts

+describe('handleCreateOrUpdateSchedulePost', () => {
+    it('handleCreateOrUpdateScheduledPost - handle not found database', async () => {
+        const {error} = await handleCreateOrUpdateScheduledPost('foo', {} as WebSocketMessage) as {error: unknown};
+        expect(error).toBeTruthy();


Does the error message says "Database not found?"

Yup, updated the test case.

rahimrahman · 2025-02-04T04:58:06Z

app/actions/websocket/scheduled_post.test.ts

+        expect(deletedRecord).toBeTruthy();
+        expect(deletedRecord.models).toBeTruthy();


Are these beneficial as models.length is asserted to 1?

No, as such, removed it and updated the test.

rahimrahman · 2025-02-04T04:59:13Z

app/actions/websocket/scheduled_post.test.ts

+        expect(models).toBeTruthy();
+        expect(models?.length).toBe(1);


Might be more beneficials to know what the models that came back to equal to or a few elements matches the scheduled_post.

rahimrahman · 2025-02-04T04:59:38Z

app/client/rest/scheduled_post.test.ts

+
+        await client.getScheduledPostsForTeam(teamId, includeDirectChannels, groupLabel);
+
+        expect(client.doFetch).toHaveBeenCalledWith(expectedUrl, expectedOptions);


rahimrahman · 2025-02-04T05:03:48Z

app/actions/remote/scheduled_post.test.ts

+
+    it('fetch Schedule post - handle scheduled post enabled', async () => {
+        mockedGetConfigValue.mockResolvedValueOnce('true');
+        jest.spyOn(operator, 'handleScheduledPosts').mockResolvedValueOnce([]);


Should you mock this or should you check that handleScheduledPosts() .toHaveBeenCalledWith() properly?

rahimrahman · 2025-02-04T05:05:56Z

app/actions/local/scheduled_post.ts

+import DatabaseManager from '@database/manager';
+import {logError} from '@utils/log';
+
+export async function handleScheduledPosts(serverUrl: string, actionType: string, scheduledPosts: ScheduledPost[], prepareRecordsOnly = false) {


Missing test?

rahimrahman · 2025-02-04T05:07:50Z

app/actions/remote/scheduled_post.test.ts

+    });
+
+    it('delete Schedule post - handle scheduled post enabled', async () => {
+        jest.spyOn(operator, 'handleScheduledPosts').mockResolvedValueOnce([]);


Same as above, should we mock or check that it was calledWith() properly?

rahimrahman · 2025-02-04T05:12:58Z

app/actions/remote/scheduled_post.test.ts

+const mockClient = {
+    getScheduledPostsForTeam: jest.fn(() => Promise.resolve({...scheduledPostsResponse})),
+    deleteScheduledPost: jest.fn((scheduledPostId) => {
+        return Promise.resolve(scheduledPostsResponse.bar.find((post) => post.id === scheduledPostId));


Curious: when you send deleteScheduledPost, you get the post that you deleted back?

Yup, I know this is of no use, but the handler is written in this way (as we batch all the records for create/update/delete actions together and return Promise of Scheduled posts). It causes no harm but comes with benefits to test which record was actually deleted.

…ch request

Rajat-Dabade · 2025-02-04T19:33:08Z

local/scheduled_post.test.ts

websocket/scheduled_post.test.ts

remote/scheduled_post.test.ts

client/rest/scheduled_post.test.ts

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

Rajat-Dabade requested review from larkox, enahum and harshilsharma63 January 28, 2025 12:37

Rajat-Dabade self-assigned this Jan 28, 2025

Rajat-Dabade added the 2: Dev Review Requires review by a core commiter label Jan 28, 2025

rahimrahman reviewed Jan 28, 2025

View reviewed changes

harshilsharma63 requested a review from rahimrahman January 29, 2025 09:28

harshilsharma63 approved these changes Jan 29, 2025

View reviewed changes

Rajat-Dabade force-pushed the rest-client-schedule-post branch from b3ba535 to aeecc3d Compare February 3, 2025 04:11

Rajat-Dabade force-pushed the rest-client-schedule-post branch 2 times, most recently from aaba87d to 98ef776 Compare February 3, 2025 16:44

larkox requested changes Feb 3, 2025

View reviewed changes

Rajat-Dabade requested a review from larkox February 3, 2025 20:09

Rajat-Dabade force-pushed the rest-client-schedule-post branch from 6bece54 to 9d62952 Compare February 3, 2025 21:12

rahimrahman reviewed Feb 4, 2025

View reviewed changes

amyblais modified the milestone: v2.26.0 Feb 4, 2025

Base automatically changed from migration-scheduled-post to feature_schedule_posts February 4, 2025 14:43

Rajat-Dabade force-pushed the rest-client-schedule-post branch from 9d62952 to e045d63 Compare February 4, 2025 15:36

Rajat-Dabade added 8 commits February 4, 2025 21:10

added handler for receiving all the scheduled post

7624f77

Added migration script for adding schedule post table

e454eb8

reverted back unnecessary changes

d488d35

Addressed review comments

6887d36

Added test for handleScheduledPost

be15784

removed undefined properties

07aaffa

reverted unnecessary changes

5415706

Updated logic and test case for fetching records according to teams

63b02e8

Rajat-Dabade and others added 18 commits February 4, 2025 21:18

remove unnecessary changes

8fe0903

Updated logic for handling scheduled post

a48060a

Update test case

5833787

Updated logic when review all the scheduled post

dca78c1

ci fixes

381aad6

Schedule post rest client, local/remote/websocket handling

514d9a5

review fixes

4be85b6

Added test for local client

f9f90eb

Added test case for handle delete scheduled post

62d4449

Added test for remote client

58ac21e

Added test for websocket schedule post events

473c01c

Added test for scheduled_post rest client

c731fe4

Added include direct channel post flag for handling schedule post fet…

f6f6a0d

…ch request

Updated test case

ef1269e

Addressed review comments

f006100

instead of mocking adding it to database

7276f1f

Updated test

d5a5b58

import fixes

3b3c02a

Rajat-Dabade force-pushed the rest-client-schedule-post branch from e045d63 to 3b3c02a Compare February 4, 2025 15:52

Rajat-Dabade added 2 commits February 5, 2025 00:51

addressed review comments

e6dd711

minor

a670053

Rajat-Dabade requested a review from rahimrahman February 4, 2025 19:37

larkox approved these changes Feb 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added rest client and local/remote/WebSocket handling for Scheduled Post #8526

Added rest client and local/remote/WebSocket handling for Scheduled Post #8526

Rajat-Dabade commented Jan 28, 2025

rahimrahman commented Jan 28, 2025

Rajat-Dabade commented Feb 3, 2025

larkox left a comment

larkox Feb 3, 2025

Rajat-Dabade Feb 3, 2025

larkox Feb 3, 2025

Rajat-Dabade Feb 3, 2025

larkox Feb 3, 2025

Rajat-Dabade Feb 3, 2025

larkox Feb 3, 2025

Rajat-Dabade Feb 3, 2025

rahimrahman left a comment

rahimrahman Feb 4, 2025

Rajat-Dabade Feb 4, 2025

rahimrahman Feb 4, 2025

Rajat-Dabade Feb 4, 2025

rahimrahman Feb 4, 2025

Rajat-Dabade Feb 4, 2025

rahimrahman Feb 4, 2025

Rajat-Dabade Feb 4, 2025

rahimrahman Feb 4, 2025

Rajat-Dabade Feb 4, 2025

rahimrahman Feb 4, 2025

rahimrahman Feb 4, 2025

Rajat-Dabade Feb 4, 2025

rahimrahman Feb 4, 2025

Rajat-Dabade Feb 4, 2025

rahimrahman Feb 4, 2025

Rajat-Dabade Feb 4, 2025

rahimrahman Feb 4, 2025

Rajat-Dabade Feb 4, 2025

Rajat-Dabade commented Feb 4, 2025

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

		expect(deletedRecord).toBeTruthy();
		expect(deletedRecord.models).toBeTruthy();


		await client.getScheduledPostsForTeam(teamId, includeDirectChannels, groupLabel);

		expect(client.doFetch).toHaveBeenCalledWith(expectedUrl, expectedOptions);

Added rest client and local/remote/WebSocket handling for Scheduled Post #8526

Are you sure you want to change the base?

Added rest client and local/remote/WebSocket handling for Scheduled Post #8526

Conversation

Rajat-Dabade commented Jan 28, 2025

Summary

Ticket Link

Checklist

Release Note

rahimrahman commented Jan 28, 2025

Rajat-Dabade commented Feb 3, 2025

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

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

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

Rajat-Dabade commented Feb 4, 2025