[GH-311] Fix issues: Not able to create meeting in threads view and by clicking "Create New Metting" button in a channel

[raghavaggarwal2308]

Summary

Fix the following issues:

• Not able to create a meeting in the threads view.
• Not able to create a meeting by clicking "Create New Meeting" in a channel.

What to test?

• Able to create a meeting in the threads view
• Able to create a new meeting by clicking the "Create New Meeting" button

Steps to reproduce:

• For threads view:

1. Have CRT enabled, and open the Threads view
2. Open a thread that pops into the RHS
3. Click the Zoom button on the right

• For clicking the button:

1. Start a new meeting from the desired user's account.
2. Without ending the meeting started, try to start a new meeting again.
3. Click on the "Create New Meeting" button.

Expected behaviour:

• The meetings should be created successfully.

Environment:

MM version: v7.8.10
Node version: 14.18.0
Go version: 1.19.0

Ticket Link

FIxes #311

[mickmister]

/update-branch

[mattermost-build]

We don't have permissions to update this PR, please contact the submitter to apply the update.

[mickmister]

@raghavaggarwal2308 Did you check the box that allows maintainers to edit the PR?

[mickmister]

The code is a bit hard to read/understand (not the changes in this PR, just the code itself). I suggest as a follow up we introduce typescript into this repo and use objects as the function args for startMeeting etc.

For the purpose of this fix, LGTM 👍

[mickmister]

Looks good for this PR. Unrelated comment below

---

Do we really need a Client.forceStartMeeting? Since that function really just calls Client.startMeeting with force=true

Also, this function signature is pretty hard to read, along with the function call below. Thoughts on changing this to accept an object instead? Would be best if it were typescript as well so we have type safety on the object

type StartMeetingArgs = {
    channelId: string;
    rootId?: string;
    force?: boolean;
    topic?: string;
}

export function startMeeting(args: StartMeetingArgs) {
    return async (dispatch, getState) => {
        try {
            const startFunction = force ? Client.forceStartMeeting : Client.startMeeting;
            const meetingURL = await startFunction(channelId, rootId, true, topic);

We can do this as a follow up thing since that would expand the scope of this fix quite a bit

[raghavaggarwal2308]

@mickmister I agree with your suggestions. Should I create an issue for the same?

[mickmister]

@raghavaggarwal2308 Yes sounds good to me 👍

[raghavaggarwal2308]

@mickmister Created the issue. Please let me know if we need to add anything else.
#319

[mickmister]

Is meetingId ever defined when calling these client functions?

[raghavaggarwal2308]

@mickmister I don't think this will ever be defined.

[mickmister]

@raghavaggarwal2308 If it's never defined when this function is called anywhere in the plugin, then I think we should remove the function parameter in this PR. Do you think it will eventually be used?

[raghavaggarwal2308]

@mickmister I don't think it will be used. Removed it now.

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (6813e92) 19.33% compared to head (1b51d87) 19.33%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #316   +/-   ##
=======================================
  Coverage   19.33%   19.33%           
=======================================
  Files           9        9           
  Lines        1479     1479           
=======================================
  Hits          286      286           
  Misses       1138     1138           
  Partials       55       55           

Files	Coverage Δ
server/http.go	5.18% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mickmister]

Seems like personal is always true as well right? I think these were added as placeholders, but they haven't been implemented for years so I think they are a bit of tech debt at this point. Do you see value in keeping it around? I don't want to add risk to the PR so I trust your judgment

[raghavaggarwal2308]

@mickmister I don't think we need this for now. I have removed it. We can always add it back if we need it in the future.

[AayushChaudhary0001]

Tested and approved.

This PR is tested for the below scenarios :-

• Able to create a meeting having CRT enabled in threads view.
• Able to start a new meeting by clicking the "Create new meeting" button.

Working fine, LGTM.