MM-67601 Added DM notification when subscriptions are blocked by group locking#645
MM-67601 Added DM notification when subscriptions are blocked by group locking#645avasconcelos114 merged 6 commits intomasterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDetects changes to the configured GitLab group namespace; when it changes between non-empty values, triggers notifications to creators whose subscriptions become disallowed by sending DMs. Also adds a log line when webhook events are skipped for projects outside the allowed group and related tests. Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration System
participant Plugin as Plugin (notifier)
participant KV as KV Store (subscriptions)
participant Channel as Channel Service
participant Bot as Bot/DM System
Config->>Config: detect GitlabGroup change (previous ≠ new)
Config->>Plugin: notifyUsersOfDisallowedSubscriptions()
Plugin->>KV: KVGet(all subscriptions)
KV-->>Plugin: subscriptions list
loop per creator with disallowed repos
Plugin->>Channel: GetChannel / GetDirectChannel (channel id)
Channel-->>Plugin: channel metadata (name, type)
Plugin->>Plugin: formatGroupLockChangeMessage(repos by channel)
Plugin->>Bot: CreateBotDMPost(user_id, message, post_type)
Bot-->>Plugin: success / error (logged with user_id & post_type)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'modernize', run 'golangci-lint help linters' to see the list of supported linters Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/plugin_test.go (1)
165-194: Tighten the matcher so allowed repos can’t slip through.The current negative check (
"* dev-tool\n") won’t fail if the message includes backticks. Matching on the exact formatted token will make this test more robust.Suggested tweak
- api.On("CreatePost", mock.MatchedBy(func(post *model.Post) bool { - msg := post.Message - // DM must list only disallowed repos grouped by channel (other-group, dev-tool-foo), not the allowed one (dev-tool) - return strings.Contains(msg, "##### ~") && strings.Contains(msg, "other-group") && - strings.Contains(msg, "dev-tool-foo") && !strings.Contains(msg, "* dev-tool\n") - })).Return(&model.Post{}, nil).Once() + api.On("CreatePost", mock.MatchedBy(func(post *model.Post) bool { + msg := post.Message + // DM must list only disallowed repos grouped by channel (other-group, dev-tool-foo), not the allowed one (dev-tool) + return strings.Contains(msg, "##### ~") && + strings.Contains(msg, "`other-group`") && + strings.Contains(msg, "`dev-tool-foo`") && + !strings.Contains(msg, "`dev-tool`") + })).Return(&model.Post{}, nil).Once()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin_test.go` around lines 165 - 194, The matcher in the CreatePost mock is too loose and can miss allowed repos if they appear with backticks; update the negative check inside the mock for CreatePost (used when p.notifyUsersOfDisallowedSubscriptions is exercised) to assert the exact formatted token for the allowed repository (e.g., check that the message does not contain "* `dev-tool`\n") instead of "* dev-tool\n" so allowed repos cannot slip through even when backticked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/configuration.go`:
- Around line 199-207: The code currently treats the first config load as a
change and calls notifyUsersOfDisallowedSubscriptions(); to fix this, only
trigger notifications when there was a prior initialized configuration and the
bot identity is available. Modify the logic around
p.getConfiguration()/previousGitlabGroup and the notification condition so you
first check that the prior p.configuration is non-nil (i.e., the plugin was
already initialized) and that the bot user id is set (non-empty, e.g.,
p.getConfiguration().BotUserID != ""), then compare trimmed GitlabGroup values
and call p.notifyUsersOfDisallowedSubscriptions() only if previous config
existed and BotUserID is set and newGitlabGroup != "" && newGitlabGroup !=
previousGitlabGroup; keep using p.setConfiguration(configuration,
serverConfiguration) as before.
In `@server/plugin.go`:
- Around line 735-739: The code mapping channelNames currently only excludes
direct messages by checking ch.Type != model.ChannelTypeDirect, but group DMs
should also be excluded; update the condition in the block that assigns
channelNames (the logic using ch.Type, ch.Name and channelNames[channelID]) to
skip both model.ChannelTypeDirect and model.ChannelTypeGroup so group message
channels are not formatted as "~channel" links.
---
Nitpick comments:
In `@server/plugin_test.go`:
- Around line 165-194: The matcher in the CreatePost mock is too loose and can
miss allowed repos if they appear with backticks; update the negative check
inside the mock for CreatePost (used when p.notifyUsersOfDisallowedSubscriptions
is exercised) to assert the exact formatted token for the allowed repository
(e.g., check that the message does not contain "* `dev-tool`\n") instead of "*
dev-tool\n" so allowed repos cannot slip through even when backticked.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/plugin.go (1)
769-778: Stabilize repo ordering in DM output.Repo lists are built from map iteration, so ordering can vary run-to-run. Sorting per channel keeps messages deterministic (and avoids flaky tests).
♻️ Suggested tweak
for _, chID := range chIDs { + repos := reposByChannel[chID] + sort.Strings(repos) if channelNames[chID] == "" { lines = append(lines, "##### Others (DM & Group channels)") } else { lines = append(lines, "##### "+channelNames[chID]) } - for _, repoPath := range reposByChannel[chID] { + for _, repoPath := range repos { lines = append(lines, "* `"+repoPath+"`") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin.go` around lines 769 - 778, Repo lists are produced by iterating reposByChannel[chID] which is a map-backed slice and yields non-deterministic order; inside the loop over chIDs in the block that builds lines (around the for _, chID := range chIDs loop referencing channelNames and reposByChannel), sort the slice for that channel (e.g., with sort.Strings(reposByChannel[chID]) or a local copy that you sort) before iterating to append "* `repoPath`" entries, and add an import for the standard sort package if missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/plugin.go`:
- Around line 769-778: Repo lists are produced by iterating reposByChannel[chID]
which is a map-backed slice and yields non-deterministic order; inside the loop
over chIDs in the block that builds lines (around the for _, chID := range chIDs
loop referencing channelNames and reposByChannel), sort the slice for that
channel (e.g., with sort.Strings(reposByChannel[chID]) or a local copy that you
sort) before iterating to append "* `repoPath`" entries, and add an import for
the standard sort package if missing.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/plugin.go (1)
726-733: Avoid misclassifying channels when metadata lookup fails.
IfChannel.Getfails, the channel falls back to the DM/group bucket in the message, which can be misleading for deleted or inaccessible channels. Consider setting a placeholder label so it’s not grouped as “Others”.♻️ Suggested tweak
ch, err := p.client.Channel.Get(channelID) if err != nil { p.client.Log.Warn("Failed to get channel when notifying users of disallowed subscriptions", "channelID", channelID, "err", err.Error()) + channelNames[channelID] = "(unknown channel)" continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin.go` around lines 726 - 733, When Channel.Get fails inside the loop populating channelNames, don’t leave the map without an entry (which causes the message to fall into the DM/group "Others" bucket); instead assign a clear placeholder label into the channelNames map for that channelID (e.g., "Unavailable/Deleted channel" or similar, optionally including the channelID), and keep the existing p.client.Log.Warn call (or augment it) so the placeholder and failure are explicit; update the code around channelNames and p.client.Channel.Get to ensure every channelID has a mapped name even on error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/plugin.go`:
- Around line 726-733: When Channel.Get fails inside the loop populating
channelNames, don’t leave the map without an entry (which causes the message to
fall into the DM/group "Others" bucket); instead assign a clear placeholder
label into the channelNames map for that channelID (e.g., "Unavailable/Deleted
channel" or similar, optionally including the channelID), and keep the existing
p.client.Log.Warn call (or augment it) so the placeholder and failure are
explicit; update the code around channelNames and p.client.Channel.Get to ensure
every channelID has a mapped name even on error.
@NARSimoes any ideas why CodeRabbit it's returning this error ? It seems that it's trying to run that linter |
| if err != nil || subs == nil { | ||
| if err != nil { | ||
| p.client.Log.Warn("notifyUsersOfDisallowedSubscriptions: failed to get subscriptions", "err", err.Error()) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
nit: just to avoid extra nested levels for readability:
| if err != nil || subs == nil { | |
| if err != nil { | |
| p.client.Log.Warn("notifyUsersOfDisallowedSubscriptions: failed to get subscriptions", "err", err.Error()) | |
| } | |
| return | |
| } | |
| if err != nil{ | |
| p.client.Log.Warn("notifyUsersOfDisallowedSubscriptions: failed to get subscriptions", "err", err.Error()) | |
| return | |
| } | |
| if subs == nil { | |
| return | |
| } |
nevyangelova
left a comment
nevyangelova
left a comment
There was a problem hiding this comment.
Thanks @avasconcelos114 just a couple of minor changes!
| configuration.sanitize() | ||
|
|
||
| serverConfiguration := p.client.Configuration.GetConfig() | ||
| p.configurationLock.RLock() |
There was a problem hiding this comment.
Nit: there's a TOCTOU race where hadConfig is read under RLock then getConfiguration() gets a separate RLock. Another goroutine could call setConfiguration in between. Fold both reads into a single critical section maybe?
There was a problem hiding this comment.
Ohh interesting, I'll look into that and see what I can do
Thanks!
| var lines []string | ||
| for _, chID := range chIDs { | ||
| if channelNames[chID] == "" { | ||
| lines = append(lines, "##### Others (DM & Group channels)") |
There was a problem hiding this comment.
If a user has disallowed subscriptions in multiple DM or group channels the ##### Others (DM & Group channels) header gets emitted once per channel. Use a flag to emit it only once.
There was a problem hiding this comment.
Huge find! I've been testing with only a single DM so I completely missed this, thanks for catching this! 🙌
- Removed nesting in early returns for notify function - Addressed TOCTOU race issues in config change events - Preventing "Others" header from printing multiple times in message by adding a flag
4 participants
Summary
This PR adds a DM notification that is triggered in case a configuration change causes existing subscriptions to be disrupted
Ticket Link
Fixes https://mattermost.atlassian.net/browse/MM-67601
Summary by CodeRabbit
New Features
Improvements
Tests