-
Notifications
You must be signed in to change notification settings - Fork 69
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
GH-200: Pass topic to zoom start #209
Conversation
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 8 8
Lines 854 858 +4
======================================
- Misses 854 858 +4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! LGTM. Thanks for contributing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a great feature! Thanks for this contribution @kott! Also great job on extracting out the command parsing 👍
Functionally looks good to me, though wondering your thoughts on one suggestion 🙂
server/command.go
Outdated
@@ -107,8 +122,10 @@ func (p *Plugin) runStartCommand(args *model.CommandArgs, user *model.User) (str | |||
return "Error checking previous messages", nil | |||
} | |||
|
|||
_, _, topic := p.parseCommand(args.Command) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a bit off how there are two calls the parseCommand
, ignoring certain return values in each call. What are your thoughts on having runStartCommand
accept a topic
string parameter to avoid this function call here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good point. Just pushed a change.
…mattermost-plugin-zoom into mattermostGH-200-add-meeting-topic * 'mattermostGH-200-add-meeting-topic' of github.com:kott/mattermost-plugin-zoom: Update plugin.json (mattermost#208)
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
@DHaussermann gentle ping, I think this is waiting on your review? |
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
Hi @DHaussermann @larkox - any news on this review? I'm helping to review stale PRs and wonder if this one can be closed if not required / isn't a priority? Thanks! |
@dipak-demansol Could you please review this PR? (Medium priority) |
sure |
@hanzei i tested this, its working but i found a minor issue, after discussing with @DHaussermann i will add the Review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The functionality for this is looking good!
One request would be, is it possible to also update the slash command auto complete functionality for start with help text such as topic (optional) This would help users discover the new functionality.
@kott Would you be interested in adding auto complete for this feature? If not, don't worry. We can merge this PR as it is. |
@hanzei I'd be happy do it but maybe as a separate PR? |
@kott Sound good to me 👍 Merging as it is |
Summary
With this change we will be able to pass a meeting topic to the Zoom start command, e.g.
/zoom start <meeting name>
. Now the chat message containing the meeting's information will also include the topic, rather than justzoom meeting
.Ticket Link
Fixes #200