Open raghavaggarwal2308 opened 8 months ago
Attention: Patch coverage is 1.43541%
with 206 lines
in your changes missing coverage. Please review.
Project coverage is 16.41%. Comparing base (
d73eafa
) to head (f22abf2
). Report is 8 commits behind head on master.:exclamation: Current head f22abf2 differs from pull request most recent head 395ca62
Please upload reports for the commit 395ca62 to get more accurate results.
Files | Patch % | Lines |
---|---|---|
server/http.go | 0.00% | 104 Missing :warning: |
server/webhook.go | 0.00% | 80 Missing :warning: |
server/store.go | 0.00% | 14 Missing :warning: |
server/command.go | 27.27% | 8 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@matthewbirtch What are your thoughts on this feature? I'm thinking we should make this a configurable user setting, though not sure if it should be opt-in or opt-out
@mickmister I think it's a cool idea - I agree that we should provide a way to opt out of this in the zoom plugin settings. "For meetings I host, notify me when participants join before me".
I like the "X participants waiting for the host", but I'm not sure we need the "Host is already in the meeting" added to the Zoom meeting post. Seems unnecessary to me.
@mickmister Just for clarification:
/zoom setting
command) to disable the DM notification. (Default value : true)@mickmister Just for clarification:
- We need to add a per-user setting (like in the
/zoom setting
command) to disable the DM notification. (Default value : true)- We need to remove "Host is already in the meeting" displayed on the meeting post.
@mickmister Gentle reminder for this
@raghavaggarwal2308 Yes sounds good :+1: Can we also change "participants(s)" to adapt as "participant" and "participants"?
@mickmister Just for clarification:
- We need to add a per-user setting (like in the
/zoom setting
command) to disable the DM notification. (Default value : true)- We need to remove "Host is already in the meeting" displayed on the meeting post.
@mickmister @matthewbirtch Updated the PR with these changes
Also there are some merge conflicts to resolve here
what's the best way for me to test the ux here @mickmister?
Thanks @matthewbirtch
@raghavaggarwal2308 Are you able to have this running on a cloud server for testing?
@mickmister Fixed the review comments added by you. Also, I have setup the zoom plugin on a cloud server. Please refer this
@raghavaggarwal2308 I've got things working properly, but a few notes from my testing:
**** has joined the meeting before you.
Is this some sort of fallback? Unfortunately I can't see to reproduce this so I don't have more info.
1 participant is waiting for the host to join
(don't use participant(s)
)X participants are waiting for the host to join
(don't use participant(s)
)The host has joined the meeting
@matthewbirtch Few replies/clarification from above comment:
The post should update whenever a user joins the meeting, not sure why its not updating for you.
The host get notified everytime a user joined the meeting before him.
The host has joined the meeting
: We added the logic to show a similar message when we created the PR but it was remove later because of this comment. Please let us know if we should add it back.
- Also, @mickmister had a suggesting about multiple DMs here
I'm concerned about notifying for every single participant that joins - especially for large meetings. I like the idea @mickmister had of updating the original DM message with the participants that have joined. That would be a way to avoid multiple notifications. That would need to max out after more 3 participants join. For example,
Jim and Alice have joined the meeting before you
Jim, Alice, and Frank have joined the meeting before you
Jim, Alice, Frank, and X more have joined the meeting before you
.
The host has joined the meeting
: We added the logic to show a similar message when we created the PR but it was remove later because of this comment. Please let us know if we should add it back.
Thanks @raghavaggarwal2308 - I completely forgot I made that comment. I'm aligned to the original comment, no need to bring it back. Could you remove this from the PR description?
I'm thinking we'll want a clear way for users to opt-out of this feature. Also note that this was a suggestion from a previous PM and not direct ask from a customer. @raghavaggarwal2308 What do you think about the remaining work and potential maintenance costs of the feature?
I'm thinking we'll want a clear way for users to opt-out of this feature. Also note that this was a suggestion from a previous PM and not direct ask from a customer. @raghavaggarwal2308 What do you think about the remaining work and potential maintenance costs of the feature?
@mickmister It will take some time to explore and apply the suggestion given by @matthewbirtch . Also, once the PR is completed and merger, I think there might be some suggestions from users.
@wiggin77 Do you think we should continue our work on this PR?
Summary
Screenshots
Custom post in channel:
Slack attachment in channel: DM message:
What to test?
Note: Check for custom posts and Slack attachments (Slack attachments if displayed in the mobile app or when the plugin is disabled).
Steps to reproduce:
Note: Ensure you have added the "Participant was waiting for host to join", "Participant joined meeting before host" and "Participant/Host joined meeting" webhook events on your Zoom app.
Ticket Link
Fixes #109