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.

Sign up for GitHub

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

Jump to bottom

drop -sync, renaming to just msteams #492

Merged
merged 4 commits into from
Feb 20, 2024
Merged

drop -sync, renaming to just msteams #492

merged 4 commits into from
Feb 20, 2024

Conversation

lieut-data
Copy link
Member

Summary

Note that this does NOT change the plugin id. It's not worth the cost to effect a plugin ID change at this time. (Too many dependencies on the KV Store and metrics scraping.)

Ticket Link

Fixes: https://mattermost.atlassian.net/browse/MM-55478

@lieut-data
drop -sync, renaming to just msteams
b8cdea2 
Note that this does **NOT** change the plugin id. It's not worth the
cost to effect a plugin ID change at this time.

Fixes: https://mattermost.atlassian.net/browse/MM-55478
@lieut-data lieut-data added the 2: Dev Review Requires review by a core committer label Feb 2, 2024
@lieut-data lieut-data added this to the v1.7.0 milestone Feb 2, 2024
@johndavidlugtu
Copy link

FYI on this @cwarnermm

johndavidlugtu
johndavidlugtu approved these changes Feb 5, 2024
View reviewed changes
Copy link

@johndavidlugtu johndavidlugtu left a comment

Choose a reason for hiding this comment

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

Thanks @lieut-data

@cwarnermm
Copy link
Member

@lieut-data - Is this PR intended to update the System Console UI text to remove sync terminology as well? Or will that update be addressed as a separate PR? CC @johndavidlugtu

@lieut-data lieut-data modified the milestones: v1.7.0, v1.8.0 Feb 6, 2024
@lieut-data
Copy link
Member Author

@cwarnermm, this should remove the sync terminology everywhere, but I'll double check the system console to be sure.

@amyblais, heads up I'm punting this to v1.8.0 until https://mattermost.atlassian.net/browse/CLD-7049 is resolved.

@cwarnermm cwarnermm mentioned this pull request Feb 6, 2024
Merged
@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2024 •
edited
Loading

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (0c4f8d2) 41.07% compared to head (4fffc1a) 41.07%.

Files Patch % Lines
server/plugin.go 25.00% 3 Missing ⚠️
server/command.go 0.00% 2 Missing ⚠️
server/message_hooks.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #492   +/-   ##
=======================================
  Coverage   41.07%   41.07%           
=======================================
  Files          22       22           
  Lines        6281     6281           
=======================================
  Hits         2580     2580           
  Misses       3481     3481           
  Partials      220      220

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

@lieut-data
Copy link
Member Author

Alas, I thought @johndavidlugtu's review would "count", but seems like I need a second developer review -- @wiggin77, might I get your sign off?

wiggin77
wiggin77 reviewed Feb 20, 2024
View reviewed changes
server/message_hooks.go
@@ -542,7 +542,7 @@ func (p *Plugin) handlePromptForConnection(userID, channelID string) {
}

if time.Until(timestamp) < -time.Hour*time.Duration(promptInterval) {
p.sendBotEphemeralPost(userID, channelID, "Your Mattermost account is not connected to MS Teams so your activity will not be relayed to users on MS Teams. You can connect your account using the `/msteams-sync connect` slash command.")
p.sendBotEphemeralPost(userID, channelID, "Your Mattermost account is not connected to MS Teams so your activity will not be relayed to users on MS Teams. You can connect your account using the `/msteams connect` slash command.")
Copy link
Member

Choose a reason for hiding this comment

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

Are we standardizing on "MS Teams" versus "Microsoft Teams" or "MS-Teams" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question! I'm hoping @calebroseland and @asaadmahmood can help us with all the bot interactions as well as word smithing going forward.

Copy link
Member

Choose a reason for hiding this comment

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

I recommend "Microsoft Teams"

wiggin77
wiggin77 approved these changes Feb 20, 2024
View reviewed changes
Copy link
Member

@wiggin77 wiggin77 left a comment

Choose a reason for hiding this comment

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

LGTM

@lieut-data lieut-data added QA/Deferred Agreement with QA that these changes will be tested post-merge and removed 2: Dev Review Requires review by a core committer labels Feb 20, 2024
@lieut-data lieut-data merged commit b95ab96 into main Feb 20, 2024
7 checks passed
@lieut-data lieut-data deleted the rename-to-msteams branch February 20, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwarnermm cwarnermm cwarnermm left review comments

@johndavidlugtu johndavidlugtu johndavidlugtu approved these changes

@wiggin77 wiggin77 wiggin77 approved these changes

@crspeller crspeller crspeller approved these changes

Assignees
No one assigned
Labels
QA/Deferred Agreement with QA that these changes will be tested post-merge
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.
6 participants
@lieut-data @johndavidlugtu @cwarnermm @codecov-commenter @crspeller @wiggin77