MM-67790 Adding missing checks for muted #978
MM-67790 Adding missing checks for muted #978avasconcelos114 wants to merge 5 commits intomasterfrom
Conversation
avasconcelos114
added
2: Dev Review
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded sender-based mute checks across webhook notification flows (PR description mentions, PR comments, PR reviews, issues, assignments). Mute parsing now handles comma-separated, case-insensitive lists and guards empties. Changes
Sequence Diagram(s)sequenceDiagram
participant Sender as "Sender (GitHub bot)"
participant Plugin as "Plugin (webhook handler)"
participant KV as "KV Store (muted-users)"
participant MM as "Mattermost (DM/Post)"
rect rgba(30,144,255,0.5)
Sender->>Plugin: webhook event (PR/comment/review/issue)
end
rect rgba(34,139,34,0.5)
Plugin->>KV: GET muted-users for recipient
KV-->>Plugin: muted list (CSV, case-insensitive)
end
alt recipient has muted sender
Plugin->>Plugin: skip DM/channel fetch and posting (early return)
else not muted
Plugin->>MM: fetch/create DM channel
MM-->>Plugin: channel id
Plugin->>MM: post DM or channel message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/plugin/webhook_test.go (1)
266-336: Consider adding a test case for empty muted users list.The
senderMutedByReceiverfunction now has explicit handling for empty muted usernames (lines 922-924 in webhook.go). Adding a dedicated test case would explicitly verify this behavior.💡 Suggested test case
{ name: "Error fetching muted users", userID: "user1", sender: "sender1", setup: func(mockKVStore *mocks.MockKvStore, mockAPI *plugintest.API) { mockKVStore.EXPECT().Get("user1-muted-users", mock.MatchedBy(func(val any) bool { _, ok := val.(*[]uint8) return ok })).Return(errors.New("store error")).Times(1) mockAPI.On("LogWarn", "Failed to get muted users", "userID", "user1").Times(1) }, assert: func(t *testing.T, muted bool) { assert.False(t, muted, "Expected sender to not be muted due to store error") }, }, + { + name: "Empty muted users list", + userID: "user1", + sender: "sender1", + setup: func(mockKVStore *mocks.MockKvStore, _ *plugintest.API) { + mockKVStore.EXPECT().Get("user1-muted-users", mock.MatchedBy(func(val any) bool { + _, ok := val.(*[]uint8) + return ok + })).Return(nil).Do(func(key string, value any) { + *value.(*[]byte) = []byte("") + }).Times(1) + }, + assert: func(t *testing.T, muted bool) { + assert.False(t, muted, "Expected sender to not be muted when list is empty") + }, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin/webhook_test.go` around lines 266 - 336, Add a new unit test case in server/plugin/webhook_test.go within TestSenderMutedByReceiver to cover the empty muted-users list: in the tests slice add a case (name "Empty muted users list", userID "user1", sender "sender1") whose setup mocks MockKvStore.Get for key "user1-muted-users" to succeed and write an empty byte slice (e.g. *[]byte = []byte("")) into the value parameter, and then assert senderMutedByReceiver("user1","sender1") returns false; reference the senderMutedByReceiver function and the existing mock pattern (mockKVStore.EXPECT().Get(...).Do(...)) so the new case follows the same mocking style and verifies the explicit empty-list handling.
🤖 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/webhook_test.go`:
- Around line 266-336: Add a new unit test case in server/plugin/webhook_test.go
within TestSenderMutedByReceiver to cover the empty muted-users list: in the
tests slice add a case (name "Empty muted users list", userID "user1", sender
"sender1") whose setup mocks MockKvStore.Get for key "user1-muted-users" to
succeed and write an empty byte slice (e.g. *[]byte = []byte("")) into the value
parameter, and then assert senderMutedByReceiver("user1","sender1") returns
false; reference the senderMutedByReceiver function and the existing mock
pattern (mockKVStore.EXPECT().Get(...).Do(...)) so the new case follows the same
mocking style and verifies the explicit empty-list handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3ae59bc7-cf6c-498f-af37-ba16105b8e73
📒 Files selected for processing (2)
server/plugin/webhook.goserver/plugin/webhook_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/plugin/webhook_test.go (1)
306-337: Add a comma-space mute-list case here.The new helper cases cover casing and empty values, but not the common
sender1, sender2format. A regression case for that input would lock down the parser path that currently misses later entries with leading spaces.Example test case
+ { + name: "Sender is muted with whitespace after comma", + userID: "user1", + sender: "sender2", + setup: func(mockKVStore *mocks.MockKvStore, _ *plugintest.API) { + mockKVStore.EXPECT().Get("user1-muted-users", mock.MatchedBy(func(val any) bool { + _, ok := val.(*[]uint8) + return ok + })).Return(nil).Do(func(key string, value any) { + *value.(*[]byte) = []byte("sender1, sender2") + }).Times(1) + }, + assert: func(t *testing.T, muted bool) { + assert.True(t, muted, "Expected sender to be muted with whitespace-separated entries") + }, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin/webhook_test.go` around lines 306 - 337, Add a test case to cover comma-space separated muted lists so the parser trims leading spaces: insert a case in the same table alongside the other tests that uses mockKVStore.EXPECT().Get with key "user1-muted-users" and Do to set *value.(*[]byte) = []byte("sender1, sender2"); set userID to "user1" and sender to "sender2" and assert.True(t, muted, "Expected sender to be muted when list contains 'sender1, sender2'") so the code path exercised by the Get mock and the mute-check logic (the test harness using mockKVStore and the assertion helpers) validates handling of comma + space entries.
🤖 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/plugin/webhook.go`:
- Around line 925-929: The loop that checks mutedUsernames uses strings.Split
and compares strings.ToLower(muted) to senderLower without trimming, so entries
like " sender2" won't match; in the loop over strings.Split(mutedUsernames, ",")
trim each item with strings.TrimSpace (e.g., assign trimmed :=
strings.TrimSpace(muted)), skip if trimmed == "" and compare
strings.ToLower(trimmed) to senderLower (or compute trimmedLower once) to ensure
matches succeed even when stored values include surrounding spaces.
---
Nitpick comments:
In `@server/plugin/webhook_test.go`:
- Around line 306-337: Add a test case to cover comma-space separated muted
lists so the parser trims leading spaces: insert a case in the same table
alongside the other tests that uses mockKVStore.EXPECT().Get with key
"user1-muted-users" and Do to set *value.(*[]byte) = []byte("sender1, sender2");
set userID to "user1" and sender to "sender2" and assert.True(t, muted,
"Expected sender to be muted when list contains 'sender1, sender2'") so the code
path exercised by the Get mock and the mute-check logic (the test harness using
mockKVStore and the assertion helpers) validates handling of comma + space
entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ead70ccd-e295-4c3d-baaa-754041a2af39
📒 Files selected for processing (2)
server/plugin/webhook.goserver/plugin/webhook_test.go
|
@coderabbitai Please re-evaluate the PR and either approve and request changes |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me gather the current state of the code changes to conduct a thorough re-evaluation. 🏁 Script executed: Length of output: 19466 Let me check a few more things before making my final assessment. 🏁 Script executed: Length of output: 7843
Summary of FindingsCorrectness ✅ All major DM notification paths now have mute guards:
The module declares Case-insensitive matching ✅ The refactored Test quality ✅
Minor observation (non-blocking) The Overall the implementation is correct, comprehensive, and well-tested. Approving. [approve] 🧠 Learnings used |
|
@coderabbitai buddy you forgot to actually approve the PR after your last comment |
|
🧠 Learnings used✅ Actions performedComments resolved and changes approved. |
|
@avasconcelos114 is coderabbit not doing the reviews automatically? I see you have to explicitly request it. It should automatically kick off a review when you open the PR and then on each incremental push. Wondering if it's having issues given this error that it shared in the first comment: |
|
@marianunez There are times when it decides enough is enough and it only triggers when I explicitly ask it to (and as seen above it will sometimes "approve" my PRs without actually approving it, so I'll just request it to just so human reviewers won't get the impression that AI reviews are still ongoing) |
|
@marianunez Another thing I noticed about the |
It does have a auto-pause feature where it stops reviewing after 5 commits but that doesn't seem to be the case here? |
|
Yeah I'm a bit puzzled myself, it only really reviewed 2 commits before I had to manually request it. That said I don't particularly mind it, small price to pay for the quality of reviews I've been getting 🙌 |
3 participants
Summary
This PR addresses issues where users still get notified by events from users they have muted, the following events are now covered:
Ticket Link
Fixes #969
Change Impact: 🟡 Medium
Reasoning: The PR applies sender-based mute checks across multiple GitHub webhook handlers and modifies internal notification plumbing (including a changed postIssueNotification signature), which affects the central notification flow for PRs, issues, comments, and reviews. Changes are focused within the notification layer and tests were expanded, but the spread across several handlers increases the blast radius.
Regression Risk: Moderate — mostly additive guard conditions with expanded test coverage for muted-sender scenarios and edge cases (case-insensitivity, empty lists), but the updates touch 5+ event handlers and adjust an internal method signature, leaving room for subtle regressions in notification delivery or DM gating.
QA Recommendation: Medium manual QA. Validate suppression and normal delivery across updated handlers (PR reviews, PR/issue assignments, issue/PR closed-reopened, comment and PR-body mentions) for regular and bot accounts, verify case-insensitive mute matching, and test behavior when mute-list retrieval or parsing fails. Skipping manual QA is not advised due to user-facing notification impact.
Generated by CodeRabbitAI