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-133] Added a feature of restricting bot messages to private channels and DM/GMs #337

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

ayusht2810
Copy link
Contributor

@ayusht2810 ayusht2810 commented Jan 17, 2024

Summary

  • Added a feature to restrict bot messages to private channels and DM/GMs through plugin config.
  • Added a feature to restrict bot messages to a particular channel via the slash command.
  • Added a feature to list all channel preferences.

Ticket Link

#79

Screenshots

image
image
image
image

What to test?

  • Restricting bot posting meeting links in the channels when changing settings through plugin config or through slash commands.
  • View list of channels with updated preferences.
  • Disabling of settings slash command when posting meeting links is restricted.

@ayusht2810 ayusht2810 self-assigned this Jan 17, 2024
Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: Patch coverage is 41.22807% with 134 lines in your changes missing coverage. Please review.

Project coverage is 16.56%. Comparing base (d73eafa) to head (2549a1e).
Report is 8 commits behind head on master.

Current head 2549a1e differs from pull request most recent head 5ff77f7

Please upload reports for the commit 5ff77f7 to get more accurate results.

Files Patch % Lines
server/command.go 9.90% 100 Missing ⚠️
server/http.go 76.82% 15 Missing and 4 partials ⚠️
server/store.go 57.14% 12 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
- Coverage   18.04%   16.56%   -1.49%     
==========================================
  Files           9        9              
  Lines        1546     1733     +187     
==========================================
+ Hits          279      287       +8     
- Misses       1216     1407     +191     
+ Partials       51       39      -12     

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

@ayusht2810 ayusht2810 added the 2: Dev Review Requires review by a core committer label Jan 18, 2024
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
plugin.json Outdated Show resolved Hide resolved
server/command.go Outdated Show resolved Hide resolved
server/command.go Outdated Show resolved Hide resolved
server/command.go Outdated Show resolved Hide resolved
server/command.go Outdated Show resolved Hide resolved
server/command.go Outdated Show resolved Hide resolved
server/command.go Outdated Show resolved Hide resolved
server/command.go Outdated Show resolved Hide resolved
server/command.go Outdated Show resolved Hide resolved
server/http.go Outdated Show resolved Hide resolved
server/http.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
plugin.json Outdated Show resolved Hide resolved
plugin.json Outdated Show resolved Hide resolved
Copy link
Contributor

@raghavaggarwal2308 raghavaggarwal2308 left a comment

Choose a reason for hiding this comment

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

Can you add testcases for this new feature

server/http.go Outdated Show resolved Hide resolved
@raghavaggarwal2308
Copy link
Contributor

@ayusht2810 Please resolve conflicts

@raghavaggarwal2308
Copy link
Contributor

@mickmister Gentle reminder for review

@ayusht2810
Copy link
Contributor Author

@mickmister Gentle reminder to review the PR

Copy link
Member

@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.

LGTM 👍 just some non-blocking comments for discussion


For the markdown table in the channel-settings list response, we should probably have it say:

Default: Allow meetings in public channels, private channels, and DMs/GMs

or

Default: Allow meetings only in private channels and DMs/GMs

then simply omit any rows from the table that have the "default" value. We could probably just remove that channel from the saved settings map if the user chooses "default". 0/5 on this though

webapp/src/actions/index.js Outdated Show resolved Hide resolved
@@ -253,6 +271,10 @@ func (p *Plugin) runDisconnectCommand(user *model.User) (string, error) {
// runHelpCommand runs command to display help text.
func (p *Plugin) runHelpCommand(user *model.User) (string, error) {
text := starterText + strings.ReplaceAll(helpText+"\n"+settingHelpText, "|", "`")
if p.API.HasPermissionTo(user.Id, model.PermissionManageSystem) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit to use p.client instead of p.API. I wonder if we can make a golint rule for this. It doesn't make a huge difference, but it helps with references to AppError and error to avoid weird concrete vs interface things there. This HasPermissionTo method returns only a boolean so it's not any difference here

Copy link
Contributor

Choose a reason for hiding this comment

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

We can make a seperate for adding golint rule for not using p.API since it will include a lot changes through the code once the new lint rule is implemented

server/command.go Outdated Show resolved Hide resolved
server/command.go Outdated Show resolved Hide resolved
server/command.go Outdated Show resolved Hide resolved
server/http.go Outdated
}

zoomChannelSettingsMapValue := ZoomChannelSettingsMapValue{
ChannelName: submitRequest.CallbackId,
Copy link
Member

Choose a reason for hiding this comment

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

We're using the display name so probably best to put that here. A little unfortunate that this will become stale if the channel's display name changes. Maybe we should use channel.Name instead, which is generally more permanent? The user can change both values, but it's more likely that a display name gets changed. Storing it with the channel id below makes sense. I would expect the channel id to be in the struct as well, but it's obviously not required since the feature currently works without that

Suggested change
ChannelName: submitRequest.CallbackId,
ChannelDisplayName: submitRequest.CallbackId,

server/http.go Outdated Show resolved Hide resolved
server/http.go Outdated
@@ -915,3 +984,45 @@ func (p *Plugin) slackAttachmentToUpdatePMI(currentValue, channelID string) *mod

return &slackAttachment
}

func (p *Plugin) checkChannelPreference(channelID string) (bool, int, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func (p *Plugin) checkChannelPreference(channelID string) (bool, int, error) {
func (p *Plugin) isChannelRestrictedForMeetings(channelID string) (bool, int, error) {

Not sure why we need the http status code to be returned from here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Returning statusCode for any error that arises so we can return the respective error.

Copy link
Member

Choose a reason for hiding this comment

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

Right but the only values returned are http.StatusInternalServerError and http.StatusOK, which can instead be inferred by the caller if an error happened or not

server/http.go Show resolved Hide resolved
server/store.go Outdated Show resolved Hide resolved
@mickmister mickmister added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer labels May 13, 2024
@Kshitij-Katiyar
Copy link
Contributor

LGTM 👍 just some non-blocking comments for discussion

For the markdown table in the channel-settings list response, we should probably have it say:

Default: Allow meetings in public channels, private channels, and DMs/GMs

or

Default: Allow meetings only in private channels and DMs/GMs

then simply omit any rows from the table that have the "default" value. We could probably just remove that channel from the saved settings map if the user chooses "default". 0/5 on this though

@mickmister
the default setting has a dynamic value (i.e. it can be updated in plugin configuration)
Screenshot from 2024-06-06 18-06-05
Didn't get you on this, please explain.

@mickmister
Copy link
Member

@mickmister the default setting has a dynamic value (i.e. it can be updated in plugin configuration) Screenshot from 2024-06-06 18-06-05 Didn't get you on this, please explain.

@Kshitij-Katiyar Yes I was suggesting we display this value in the response in the wording I posted above. Just so when someone wants to inspect the settings on a channel, they can know what the default value is. Then we can omit any channels that are either unassigned or using the default value

@Kshitij-Katiyar
Copy link
Contributor

Default: Allow meetings only in private channels and DMs/GMs

this was the existing channel setting list
channel setting zoom old

And this is the updated channel setting list
channel setting configuraiton new

@mickmister I hope this aligns with your suggestions. Please let us know if anything else is required.

@raghavaggarwal2308
Copy link
Contributor

@fmartingr Gentle reminder for review

@raghavaggarwal2308 raghavaggarwal2308 added this to the v1.9.0 milestone Jul 2, 2024
Copy link

@fmartingr fmartingr left a comment

Choose a reason for hiding this comment

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

Apologies for the delay, just a nit about the help text.

server/command.go Outdated Show resolved Hide resolved
server/http.go Outdated
@@ -915,3 +984,45 @@ func (p *Plugin) slackAttachmentToUpdatePMI(currentValue, channelID string) *mod

return &slackAttachment
}

func (p *Plugin) checkChannelPreference(channelID string) (bool, int, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Right but the only values returned are http.StatusInternalServerError and http.StatusOK, which can instead be inferred by the caller if an error happened or not

server/configuration.go Outdated Show resolved Hide resolved
server/command.go Outdated Show resolved Hide resolved
1. Removed unused params from function
2. Update constant name
3. Remove status code from values
@raghavaggarwal2308
Copy link
Contributor

@mickmister Fixed the review comments added by you

Copy link
Member

@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.

LGTM, just one request to remove the two bin files

Copy link
Member

Choose a reason for hiding this comment

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

This and bin/gotestsum should be deleted I think

Copy link
Contributor

Choose a reason for hiding this comment

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

@mickmister Removed

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.

Feature request: limit posting bot message to private channels only
5 participants