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-8]: Added topic for starting meeting #109

Merged
merged 5 commits into from
Aug 16, 2024

Conversation

Kshitij-Katiyar
Copy link
Contributor

@Kshitij-Katiyar Kshitij-Katiyar commented Jul 30, 2024

Summary

Updated slash command to start a meeting to support adding topic to a meeting

Existing command: /mstmeetings start

Updated command: /mstmeetings start DEMO_MEETING_TOPIC

Screenshot

image

Ticket Link

Fixes #8

@Kshitij-Katiyar Kshitij-Katiyar requested review from raghavaggarwal2308 and removed request for raghavaggarwal2308 July 30, 2024 12:57
@Kshitij-Katiyar Kshitij-Katiyar requested review from wiggin77 and raghavaggarwal2308 and removed request for wiggin77 July 30, 2024 16:22
@raghavaggarwal2308 raghavaggarwal2308 changed the title [MM-8]: added topic for starting meeting [MM-8]: Added topic for starting meeting Aug 9, 2024
@raghavaggarwal2308 raghavaggarwal2308 added this to the v2.2.0 milestone Aug 9, 2024
@raghavaggarwal2308 raghavaggarwal2308 added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Aug 9, 2024
@wiggin77 wiggin77 removed the 2: Dev Review Requires review by a core committer label Aug 10, 2024
@AayushChaudhary0001
Copy link

While testing this PR I have found 2 issues.

  1. Multiple arguments allowed with spacing
  2. No limit on length of the meeting topic

I am attaching screenshots supporting above statements, please take a look and see if we can fix them. Option 1 would be an enhancement(as MM considers each word as an argument), but option 2 should be fixed as it would be considered as a bug.

For point 1:-
image

For point 2:-
image

@Kshitij-Katiyar
Copy link
Contributor Author

Kshitij-Katiyar commented Aug 14, 2024

@AayushChaudhary0001 I looked into your comment, and here is what I found:

  • We want to support spaces in the meeting topic, so we are allowing multiple arguments. Another approach could be to take a single argument after the command, which can be the meeting topic enclosed in double quotes. But I think what we have is better.
  • I tested the length limit of the topic on the MS Teams meeting side and found that there is no restriction on the meeting topic length from their side, so we are also not imposing any limitation on the topic's length.

Copy link

@AayushChaudhary0001 AayushChaudhary0001 left a comment

Choose a reason for hiding this comment

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

Sure, this works well and rest all the features are working fine, approved.

@raghavaggarwal2308 raghavaggarwal2308 merged commit 3882bf8 into mattermost:master Aug 16, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: QA Review Requires review by a QA tester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add subject to meetings
4 participants