Fix Jira bot DM notifications persisting after user disconnect#1286
Merged
nevyangelova merged 2 commits intomasterfrom Mar 19, 2026
Merged
Fix Jira bot DM notifications persisting after user disconnect#1286nevyangelova merged 2 commits intomasterfrom
nevyangelova merged 2 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces channel type validation to prevent subscriptions to direct message and group channels, and implements automatic cleanup of DM subscriptions when users disconnect from the system. Changes
Sequence DiagramsequenceDiagram
participant User as User Disconnect
participant Plugin as Plugin (user.go)
participant Subscribe as Subscribe Module
participant KV as KV Store
User->>Plugin: disconnectUser()
Plugin->>Subscribe: cleanupDMSubscriptionsOnDisconnect(instanceID, mattermostUserID)
Subscribe->>Plugin: GetDirectChannel(mattermostUserID)
Plugin-->>Subscribe: channelID or error
alt DM Channel Found
Subscribe->>KV: GetSubscriptions(instanceID)
KV-->>Subscribe: subscriptions
Subscribe->>Subscribe: filter subscriptions by DM channelID
Subscribe->>Subscribe: removeSubscriptionsForDMChannel()
Subscribe->>KV: SetWithOptions(updated subscriptions)
KV-->>Subscribe: success
else DM Channel Not Found
Subscribe-->>Plugin: no-op (log warning if error)
end
Plugin-->>User: disconnect completed
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested Labels
Suggested Reviewers
🚥 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. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/subscribe.go (1)
1374-1374:⚠️ Potential issue | 🟡 MinorRemove debug print statement.
Line 1374 contains
fmt.Print("/n httpGetSubscriptionTemplates")which appears to be leftover debug code.🧹 Remove debug statement
func (p *Plugin) httpGetSubscriptionTemplates(w http.ResponseWriter, r *http.Request) (int, error) { - fmt.Print("/n httpGetSubscriptionTemplates") mattermostUserID := r.Header.Get("Mattermost-User-Id")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/subscribe.go` at line 1374, Remove the leftover debug print statement fmt.Print("/n httpGetSubscriptionTemplates") from the httpGetSubscriptionTemplates handler; locate the call inside the httpGetSubscriptionTemplates function and delete that fmt.Print line so no debugging output is printed in production.
🧹 Nitpick comments (1)
server/subscribe_test.go (1)
1682-1806: LGTM with a suggestion.The test cases comprehensively cover the key scenarios for
removeSubscriptionsForDMChannel. The verification logic correctly checks that only the expected subscriptions remain after removal.Consider adding an explicit assertion in the "no subscriptions to remove" case to verify that
KVSetWithOptionsis NOT called, making the test intent clearer:💡 Optional: Add explicit no-write assertion
"no subscriptions to remove": { existingSubs: []ChannelSubscription{}, expectedRemainingByID: nil, },After running the test, add:
if tc.expectedRemainingByID == nil { api.AssertNotCalled(t, "KVSetWithOptions", mock.Anything, mock.Anything, mock.Anything) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/subscribe_test.go` around lines 1682 - 1806, Add an explicit assertion that KVSetWithOptions is NOT called when there are no subscriptions to remove: inside TestRemoveSubscriptionsForDMChannel's loop after calling p.removeSubscriptionsForDMChannel (for the case where tc.expectedRemainingByID == nil) call api.AssertNotCalled(t, "KVSetWithOptions", mock.Anything, mock.Anything, mock.Anything) so the test verifies no write happens; locate this change in the TestRemoveSubscriptionsForDMChannel test that sets up api (plugintest.API) and invokes p.removeSubscriptionsForDMChannel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/subscribe.go`:
- Line 1374: Remove the leftover debug print statement fmt.Print("/n
httpGetSubscriptionTemplates") from the httpGetSubscriptionTemplates handler;
locate the call inside the httpGetSubscriptionTemplates function and delete that
fmt.Print line so no debugging output is printed in production.
---
Nitpick comments:
In `@server/subscribe_test.go`:
- Around line 1682-1806: Add an explicit assertion that KVSetWithOptions is NOT
called when there are no subscriptions to remove: inside
TestRemoveSubscriptionsForDMChannel's loop after calling
p.removeSubscriptionsForDMChannel (for the case where tc.expectedRemainingByID
== nil) call api.AssertNotCalled(t, "KVSetWithOptions", mock.Anything,
mock.Anything, mock.Anything) so the test verifies no write happens; locate this
change in the TestRemoveSubscriptionsForDMChannel test that sets up api
(plugintest.API) and invokes p.removeSubscriptionsForDMChannel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 81000ad5-9a63-4cd7-acb5-01bcd011cc75
📒 Files selected for processing (5)
server/http_test.goserver/subscribe.goserver/subscribe_test.goserver/user.goserver/utils_test.go
nevyangelova
added
2: Dev Review
Contributor
avasconcelos114
left a comment
avasconcelos114
left a comment
There was a problem hiding this comment.
Nice work! Just got a couple of comments
Comment on lines
+482
to
+485
| dmChannel, err := p.client.Channel.GetDirect(mattermostUserID, conf.botUserID) | ||
| if err != nil { | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Feels like we may want to log errors here as we are doing for any error that happens in removeSubscriptionsForDMChannel
| }) | ||
| } | ||
|
|
||
| func (p *Plugin) removeSubscriptionsForDMChannel(instanceID types.ID, channelID string) error { |
Contributor
There was a problem hiding this comment.
Considering the function accepts any channelID and removes subscriptions regardless of whether they are DM or not, we might want to rename this for accuracy
Contributor
Author
|
Thanks @avasconcelos114 I addressed the comments 🙏 |
Contributor
Author
|
@ogi-m dev reviews complete |
ogi-m
added
4: Reviews Complete
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
QA Steps
users/jira connect, open bot DM, try to create a subscription and it should get rejected.DM subscriptions cleaned up on disconnect
/jira disconnect/jira subscribe listin bot DM should remove the subscription.Ticket Link
https://mattermost.atlassian.net/browse/MM-67804