mattermost / mattermost-plugin-zoom

Zoom plugin for Mattermost :electric_plug:
Apache License 2.0
107 stars 68 forks source link

[GH-304] Update logic to post meeting link in thread when a thread is open in RHS #323

Closed raghavaggarwal2308 closed 9 months ago

raghavaggarwal2308 commented 9 months ago

Summary

Update logic to post meeting link in the thread when a thread is open in RHS

Screenshot

image

What to test?

Steps to reproduce:
  1. Connect your Zoom account.
  2. Enable Collapsed Reply Thread(CRT) from system console.
  3. Create a thread in a channel and open it in RHS.
  4. Click on the zoom button in the app bar/channel header.
Environment

MM version: v9.2.2 Node version: 14.18.0 Go version: 1.20.11

Ticket Link

Fixes #304

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (7f468cb) 19.33% compared to head (1ed4165) 19.33%.

:exclamation: Current head 1ed4165 differs from pull request most recent head 292b290. Consider uploading reports for the commit 292b290 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #323 +/- ## ======================================= Coverage 19.33% 19.33% ======================================= Files 9 9 Lines 1479 1479 ======================================= Hits 286 286 Misses 1138 1138 Partials 55 55 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mickmister commented 9 months ago

@raghavaggarwal2308 I think this may be an unwanted change in functionality. We fixed the "threads view" issue https://github.com/mattermost/mattermost-plugin-zoom/issues/300 in a previous PR, but I'm not sure the issue linked here https://github.com/mattermost/mattermost-plugin-zoom/issues/304 is a bug. The plugin has always worked that way, and we are now changing how it works which could cause some confusion and unexpected behavior for users.

@matthewbirtch I'd like to get your thoughts on this. If an RHS thread is open while in the center channel view (not "Threads" view), and we click the Zoom icon in the Apps Bar, should the post for the meeting be created in the center channel or in the open thread? Up until this PR, it has always posted into the center channel, but this PR changes the functionality to post in the RHS.

matthewbirtch commented 9 months ago

@matthewbirtch I'd like to get your thoughts on this. If an RHS thread is open while in the center channel view (not "Threads" view), and we click the Zoom icon in the Apps Bar, should the post for the meeting be created in the center channel or in the open thread? Up until this PR, it has always posted into the center channel, but this PR changes the functionality to post in the RHS.

Posting the zoom link within the thread makes sense in the Threads inbox view, so we're good there. But I see your point about whether we should post the zoom link to the RHS thread while in the channel view. I was thinking that maybe we could infer where you intend to post the link based on whether the thread reply input or the channel input is focused (and default to the channel if neither is focused).

I'd like to bring @asaadmahmood in to this though. Asaad, could you help answer this and come together on a solution with @mickmister?

mickmister commented 9 months ago

I was thinking that maybe we could infer where you intend to post the link based on whether the thread reply input or the channel input is focused (and default to the channel if neither is focused).

@matthewbirtch 2/5 This is outside of the scope of what plugins are responsible for. And if we did implement that, it would be a hacky/brittle solution relying on css selectors to "find" the textbox and inspect focus.

The main thing I'm concerned about, is that this change in a way causes a regression, since we are changing the behavior of the plugin here. As a user, I "know" it will post in the center channel, and I trust that the plugin will continue operating that way. Having the functionality change on me will cause meetings to be posted in the wrong channel/thread when I click the button. I'm 2/5 on keeping it how it is to maintain trust of existing users of the plugin.

matthewbirtch commented 9 months ago

@mickmister I think that's fine to always post it to the center channel - except in the context of the threads inbox view. Is that possible to still post to the active thread when you're in that context?

mickmister commented 9 months ago

@matthewbirtch Yes there's a separate fix for that https://github.com/mattermost/mattermost-plugin-zoom/pull/325

@raghavaggarwal2308 @avas27JTG I'm thinking we should revert this PR (that we are commenting on here) that changes the behavior of when the center channel is visible

matthewbirtch commented 9 months ago

Ok, makes sense to me @mickmister

asaadmahmood commented 9 months ago

@matthewbirtch @mickmister So if a conversation is going on in an RHS thread from a different channel (UX Design), while the center channel is bugs.

There is no way for me to create a call for the channel in the RHS?

mickmister commented 9 months ago

There is no way for me to create a call for the channel in the RHS?

@asaadmahmood You can run /zoom start in the RHS thread but I don't think that's widely used.

For the Apps Bar icon use case, maybe we need to ask the user where they want the meeting to be posted, but then we don't get the benefits of the "one-click" nature of the feature.