Create new meeting instead of using PMI #147

larkox · 2020-07-17T14:41:06Z

Summary

Create new meetings instead of using the user PMI at the meeting creation

Ticket Link

codecov · 2020-07-20T09:18:18Z

Codecov Report

Merging #147 (660ddf8) into master (bb036f8) will not change coverage.
The diff coverage is 0.00%.

@@          Coverage Diff           @@
##           master    #147   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           8       8           
  Lines         860     882   +22     
======================================
- Misses        860     882   +22

Impacted Files	Coverage Δ
server/command.go	`0.00% <0.00%> (ø)`
server/http.go	`0.00% <0.00%> (ø)`
server/plugin.go	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb036f8...660ddf8. Read the comment docs.

mickmister

Awesome improvement @larkox! Just a few requests

mickmister · 2020-11-16T16:38:05Z

server/http.go

+	meeting, err := client.CreateMeeting(user.Email)
+	if err != nil {
+		p.API.LogWarn("Error creating the meeting", "err", err)
+		return
+	}


Does the Mattermost user's email have to match the Zoom user's email? If so, can we communicate that to the user here?

No, there is no need on OAuth. I will make some changes to properly handle this.

mickmister · 2020-11-16T16:40:04Z

server/zoom/jwt.go

+	var ret Meeting
+	meetingRequest := CreateMeetingRequest{
+		Topic: "Meeting created on Mattermost",
+		Type:  1,


Can we have a constant for this, and also a NewCreateMeetingRequest function?

The constant will be done. Regarding the function, I am not sure how useful will be. We may eventually add more different kinds of meetings, and this approach seems good for the time being. Also, this definition is only repeated on the jwt and oauth code, but the jwt code will be removed for the next release.

mickmister · 2020-11-16T16:40:39Z

server/zoom/meeting.go

+	// Recurrence struct {
+	// 	Type           int    `json:"type"`
+	// 	RepeatInterval int    `json:"repeat_interval"`
+	// 	WeeklyDays     string `json:"weekly_days,omitempty"`
+	// 	MonthlyDay     int    `json:"monthly_day,omitempty"`
+	// 	MonthlyWeekDay int    `json:"monthly_week_day,omitempty"`
+	// 	EndTimes       int    `json:"end_times,omitempty"`
+	// 	EndDateTime    int    `json:"end_date_time,omitempty"`
+	// } `json:"recurrence,omitempty"`


Any reason we would want to have this commented out, rather than uncomment or remove this?

I would like to keep it as reference. Right now removing the comments is not an option because it will send the "recurrence" field in the payload, and will create errors on zoom side.

Can you do *Recurrence with omitempty?

Indeed I can and I will. 😅

mickmister · 2020-11-16T16:42:03Z

server/zoom/jwt.go

+func (c *JWTClient) CreateMeeting(userEmail string) (*Meeting, error) {
+	var ret Meeting
+	meetingRequest := CreateMeetingRequest{
+		Topic: "Meeting created on Mattermost",


Can we let the user specify a meeting topic via /zoom start Let's have some fun!

That can be done, but I think it would be a great HW ticket, if you are willing to write it down.

mickmister · 2020-11-16T16:42:47Z

server/zoom/oauth.go

+	meetingRequest := CreateMeetingRequest{
+		Topic: "Meeting created on Mattermost",
+		Type:  1,
+	}


Same comments mentioned in jwt.go

… for meeting types.

mickmister

LGTM, one non-blocking request to prefix MeetingType constants with the data type

mickmister · 2020-11-16T18:02:42Z

server/zoom/meeting.go

+	Instant MeetingType = 1
+	// Scheduled meeting
+	Scheduled MeetingType = 2
+	// RecurringWithNoFixedTime meeting
+	RecurringWithNoFixedTime MeetingType = 3
+	// RecurringWithFixedTime meeting
+	RecurringWithFixedTime MeetingType = 8


Can we have these prefixed with MeetingType? In other parts of the code, reading Instant is not easy to know that it is a meeting type

hanzei

LGTM

hanzei · 2020-11-17T13:47:17Z

Does this also fix #199?

larkox · 2020-11-17T13:50:10Z

@hanzei Yes and no. A follow-up to this PR is this one #181 . After that one, is merged, the problem of #199 will appear again if the user has set in Mattermost to use the PMI.

hanzei · 2020-11-17T13:54:49Z

Got it. Then let's keep #199 open.

larkox · 2020-12-01T09:51:16Z

Waiting for #184 to be merged and cut

dipak-demansol

@Hanze i when i deploy this PR code then first time when i use /zoom start command then still for the first time i can see that the PMI number is same, and some time the issue was not regenerated, after testing this again and again in the server & in the Private window, i did not found the issue so its LGTM

dipak-demansol · 2021-11-30T10:47:54Z

Should i remove the do not merge tag? @hanzei

larkox · 2021-12-01T11:31:43Z

I merged master into this, and should be ready to merge. I leave up to you if some smoke test is needed in case the merge went wrong.

dipak-demansol · 2021-12-03T07:34:59Z

@Hanze i when i deploy this PR code then first time when i use /zoom start command then still for the first time i can see that the PMI number is same, and some time the issue was not regenerated, after testing this again and again in the server & in the Private window, i did not found the issue so its LGTM

@hanzei pls see this video, i was talking about this issue

at.the.First.time.the.while.creating.the.meeting.link.should.be.the.random.mp4

hanzei · 2021-12-03T07:57:26Z

@larkox ☝️

dipak-demansol

@larkox pls see the video, i reported a issue.

larkox · 2021-12-16T16:47:12Z

@dipak-demansol I was missing that particular case. It should be solved now.
I also removed a few references to PMI from some minor places.

Let me know if you find any other issue.

dipak-demansol

LGTM, Tested with the below Scenario.

Manually Disconnected The zoom account and connected again and tested that PMI number is different or not.
after 1 hour automatically the zoom account disconnect then connected zoom account and tested the same thing.
many times Created the zoom meeting and every time its generating the different PMI number.

This reverts commit 65af542.

Create new meeting instead of using PMI

b69eaf6

larkox added 2: Dev Review Requires review by a core committer Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) 3: QA Review Requires review by a QA tester labels Jul 17, 2020

larkox added this to the v2.0.0 milestone Jul 17, 2020

Fix test and remove unused debug line

78b11e7

larkox mentioned this pull request Aug 11, 2020

Decide whether to use PMI or not on instant meetings #154

Closed

larkox added 2 commits November 16, 2020 13:54

Merge branch 'master' into CreateMeeting

621279b

Fix test

b867c90

larkox requested review from mickmister and hanzei and removed request for mickmister November 16, 2020 13:24

mickmister suggested changes Nov 16, 2020

View reviewed changes

Add some missing comments, fix the user email issue and add constants…

7540819

… for meeting types.

larkox requested a review from mickmister November 16, 2020 17:54

Uncomment recurrence

ac3d5c9

mickmister mentioned this pull request Nov 16, 2020

Support naming the zoom meeting via /zoom start #200

Closed

mickmister approved these changes Nov 16, 2020

View reviewed changes

Make constants names more verbose

0984644

hanzei approved these changes Nov 17, 2020

View reviewed changes

andrew-the-drawer mentioned this pull request Nov 21, 2020

GH-154: PMI setting command for meeting creation #181

Closed

hanzei removed the 2: Dev Review Requires review by a core committer label Dec 1, 2020

larkox removed the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Dec 15, 2020

Merge branch 'master' into CreateMeeting

066ed9b

hanzei added the Do Not Merge Should not be merged until this label is removed label Nov 30, 2021

dipak-demansol approved these changes Nov 30, 2021

View reviewed changes

dipak-demansol added QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester labels Nov 30, 2021

dipak-demansol added 4: Reviews Complete All reviewers have approved the pull request and removed Do Not Merge Should not be merged until this label is removed QA Review Done PR has been approved by QA labels Nov 30, 2021

Merge branch 'master' into CreateMeeting

66b7dd5

hanzei requested a review from dipak-demansol December 1, 2021 12:33

hanzei added 3: QA Review Requires review by a QA tester and removed 4: Reviews Complete All reviewers have approved the pull request Awaiting Submitter Action Blocked on the author labels Dec 1, 2021

dipak-demansol suggested changes Dec 6, 2021

View reviewed changes

hanzei added the Awaiting Submitter Action Blocked on the author label Dec 7, 2021

larkox added 2 commits December 16, 2021 17:38

Merge branch 'master' into CreateMeeting

04466a7

Use new meeting ID on first connect and minor fixes

660ddf8

larkox requested a review from dipak-demansol December 16, 2021 16:46

hanzei removed the Awaiting Submitter Action Blocked on the author label Dec 17, 2021

dipak-demansol approved these changes Dec 20, 2021

View reviewed changes

hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Dec 20, 2021

hanzei merged commit 65af542 into mattermost:master Dec 20, 2021

mickmister added a commit that referenced this pull request Jul 28, 2022

Revert "Create new meeting instead of using PMI (#147)"

20d8720

This reverts commit 65af542.

This was referenced Nov 14, 2023

Bump version v2.0.0 (closed, renamed to v1.7.0) #321

Closed

Bump version v1.7.0. #322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create new meeting instead of using PMI #147

Create new meeting instead of using PMI #147

larkox commented Jul 17, 2020

codecov bot commented Jul 20, 2020 •

edited

Loading

mickmister left a comment

mickmister Nov 16, 2020

larkox Nov 16, 2020

mickmister Nov 16, 2020

larkox Nov 16, 2020

mickmister Nov 16, 2020

larkox Nov 16, 2020

mickmister Nov 16, 2020

larkox Nov 16, 2020

mickmister Nov 16, 2020

larkox Nov 16, 2020

mickmister Nov 16, 2020

mickmister left a comment

mickmister Nov 16, 2020

hanzei left a comment

hanzei commented Nov 17, 2020

larkox commented Nov 17, 2020

hanzei commented Nov 17, 2020

larkox commented Dec 1, 2020

dipak-demansol left a comment •

edited

Loading

dipak-demansol commented Nov 30, 2021

larkox commented Dec 1, 2021

dipak-demansol commented Dec 3, 2021

hanzei commented Dec 3, 2021

dipak-demansol left a comment

larkox commented Dec 16, 2021

dipak-demansol left a comment

Create new meeting instead of using PMI #147

Create new meeting instead of using PMI #147

Conversation

larkox commented Jul 17, 2020

Summary

Ticket Link

codecov bot commented Jul 20, 2020 • edited Loading

Codecov Report

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanzei left a comment

Choose a reason for hiding this comment

hanzei commented Nov 17, 2020

larkox commented Nov 17, 2020

hanzei commented Nov 17, 2020

larkox commented Dec 1, 2020

dipak-demansol left a comment • edited Loading

Choose a reason for hiding this comment

dipak-demansol commented Nov 30, 2021

larkox commented Dec 1, 2021

dipak-demansol commented Dec 3, 2021

hanzei commented Dec 3, 2021

dipak-demansol left a comment

Choose a reason for hiding this comment

larkox commented Dec 16, 2021

dipak-demansol left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 20, 2020 •

edited

Loading

dipak-demansol left a comment •

edited

Loading