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

[MM-343] Updated meeting flow for new users. #344

Merged
merged 9 commits into from
Apr 15, 2024

Conversation

raghavaggarwal2308
Copy link
Contributor

@raghavaggarwal2308 raghavaggarwal2308 commented Mar 4, 2024

Summary

  • New users will get a post to select the default meeting ID when they try to start a meeting for the first time.

Screenshots

post
updated_post

What to test?

  • New users are getting a post to select the default meeting ID when they try to start a meeting for the first time.
Steps to test:
  1. Create a new user on MM.
  2. Connect their zoom account.
  3. Start a zoom meeting.

Ticket Link

Fixes #343
Fixes #341

- User will get a post to select the default meeting ID when they try to start a meeting for the first time.
Copy link

codecov bot commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 0% with 38 lines in your changes are missing coverage. Please review.

Project coverage is 18.04%. Comparing base (009e61e) to head (b66daca).

Files Patch % Lines
server/http.go 0.00% 38 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #344      +/-   ##
==========================================
- Coverage   18.47%   18.04%   -0.44%     
==========================================
  Files           9        9              
  Lines        1510     1546      +36     
==========================================
  Hits          279      279              
- Misses       1180     1216      +36     
  Partials       51       51              

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

@mickmister mickmister requested a review from asaadmahmood March 5, 2024 13:29
@mickmister
Copy link
Contributor

@raghavaggarwal2308 Can you please post some more screenshots to show the other possible states that we show the user?

@asaadmahmood
Copy link
Contributor

@raghavaggarwal2308 Do we automatically create the zoom meeting and take the user to the zoom meeting page after they select an option?

@raghavaggarwal2308
Copy link
Contributor Author

raghavaggarwal2308 commented Mar 6, 2024

@raghavaggarwal2308 Do we automatically create the zoom meeting and take the user to the zoom meeting page after they select an option?

@asaadmahmood Added logic to automatically redirect the user to the meeting page and yes, we automatically create the zoom meeting.

@raghavaggarwal2308
Copy link
Contributor Author

raghavaggarwal2308 commented Mar 6, 2024

@raghavaggarwal2308 Can you please post some more screenshots to show the other possible states that we show the user?

@mickmister
For new users:
new_users.webm

For existing users:
existing_users.webm

Copy link
Contributor

@asaadmahmood asaadmahmood left a comment

Choose a reason for hiding this comment

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

LGTM

@mickmister
Copy link
Contributor

It seems off to me to have the zoom settings message shown during the connection process, as this makes it emphasized twice in this flow:

image

Thoughts on only showing this on meeting creation, and not account connection? @asaadmahmood

Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

In general LGTM. I do want the UX question above verified before merging though

webapp/src/index.js Outdated Show resolved Hide resolved
server/http.go Outdated Show resolved Hide resolved
server/http.go Outdated Show resolved Hide resolved
@raghavaggarwal2308
Copy link
Contributor Author

@mickmister Fixed the review comments mentioned by you

@AayushChaudhary0001
Copy link
Contributor

The PR above has been tested for the following scenario:-

  • Every time new user gets a post to select which meeting ID before starting a meeting.

This PR is working fine for the above condition, LGTM. Approved.

@mickmister
Copy link
Contributor

I'm still thinking we should only show the zoom settings message on meeting creation, and not account connection. @asaadmahmood What do you think #344 (comment)?

@ayusht2810
Copy link
Contributor

@mickmister @asaadmahmood Gentle reminder for the updates on this PR.

@asaadmahmood
Copy link
Contributor

@mickmister @ayusht2810 Yes, I think we should only show it on meeting creation.

@ayusht2810
Copy link
Contributor

@mickmister @asaadmahmood updated the statement. Please re-review

@asaadmahmood
Copy link
Contributor

@ayusht2810 Can you show a video.

@ayusht2810
Copy link
Contributor

@asaadmahmood Demo video for the above fix screen-capture.webm

@asaadmahmood
Copy link
Contributor

LGTM

@raghavaggarwal2308
Copy link
Contributor Author

@mickmister Can we merge this PR and move forward with release v1.7.1?

@ayusht2810
Copy link
Contributor

@hanzei Can we merge this PR as it is Dev and QA approved for the release 1.7.1?

@mickmister mickmister merged commit 3206b7c into mattermost:master Apr 15, 2024
13 checks passed
@ayusht2810 ayusht2810 mentioned this pull request Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants