Skip to content
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

Websocket extension #8585

Open
wants to merge 8 commits into
base: feature_schedule_posts
Choose a base branch
from

Conversation

harshilsharma63
Copy link
Member

Summary

Adds websocket handling for scheduled post events

Ticket Link

Fixes https://mattermost.atlassian.net/browse/MM-63111

Checklist

  • Added or updated unit tests (required for all new features)

Device Information

ios emulator

Release Note

None

Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a couple nitpicks.

Comment on lines 22 to 23
const ws = websocketManager.getClient(serverUrl);
const connectionId = ws?.getConnectionId();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0/5 nitpick

Suggested change
const ws = websocketManager.getClient(serverUrl);
const connectionId = ws?.getConnectionId();
const connectionId = websocketManager.getClient(serverUrl)?.getConnectionId();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

export async function handleCreateOrUpdateScheduledPost(serverUrl: string, msg: WebSocketMessage<ScheduledPostWebsocketEventPayload>, prepareRecordsOnly = false) {
try {
const scheduledPost: ScheduledPost = JSON.parse(msg.data.scheduled_post);
return handleScheduledPosts(serverUrl, ActionType.SCHEDULED_POSTS.CREATE_OR_UPDATED_SCHEDULED_POST, [scheduledPost], prepareRecordsOnly);
const scheduledPost: ScheduledPost[] = msg?.data?.scheduledPost ? [JSON.parse(msg.data.scheduledPost)] : [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think msg is defined in this context. Not so sure about data.

Suggested change
const scheduledPost: ScheduledPost[] = msg?.data?.scheduledPost ? [JSON.parse(msg.data.scheduledPost)] : [];
const scheduledPost: ScheduledPost[] = msg.data?.scheduledPost ? [JSON.parse(msg.data.scheduledPost)] : [];

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -51,48 +35,63 @@ afterEach(async () => {
});

describe('handleCreateOrUpdateSchedulePost', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nicer to have the tests check if the posts were really created / deleted in the database, but 0/5

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 88 to 90
// default: {
// getClient: jest.fn(() => mockWebSocketClient),
// },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unneeded comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, done.

@amyblais amyblais added this to the v2.27.0 milestone Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants