MM-56501 Enable/disable automuting when a user connects/disconnects #479
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #479 +/- ##
==========================================
+ Coverage 41.43% 41.67% +0.23%
==========================================
Files 23 24 +1
Lines 6436 6493 +57
==========================================
+ Hits 2667 2706 +39
- Misses 3536 3551 +15
- Partials 233 236 +3 ☔ View full report in Codecov by Sentry. |
|
@johndavidlugtu, this relates to the comment on the previous PR, but I'm wondering if we want to restrict this functionality only for connected users. Maybe this setting should work for all users regardless of connection status? |
|
If we were to make that change, then in other words, a non-connected user would be able to set their primary platform and have all their linked channels auto-muted. There'd be a fair bit of this logic that would/could be rewritten then, but it would simplify some stuff since the multiple conditions for enabling automuting did make this rather complicated to reason about and make work |
There was a problem hiding this comment.
This looks great, @hmhealey! Just a handful of comments, but nothing that requires a re-review from me.
| @@ -472,6 +472,8 @@ func (a *API) oauthRedirectHandler(w http.ResponseWriter, r *http.Request) { | |||
|
|
|||
| _, _ = w.Write([]byte(fmt.Sprintf("<html><body><h1>%s</h1><p>You can close this window.</p></body></html>", connectionMessage))) | |||
|
|
|||
| _, _ = a.p.updateAutomutingOnUserConnect(mmUserID) | |||
There was a problem hiding this comment.
Can we log LogWarn any error that's returned here to avoid burying it?
There was a problem hiding this comment.
This code is actually a bit misleading since we do log any errors inside updateAutomutingOnUserConnect. I wanted the method to log its own errors to keep the automute logic more self-contained, but I still wanted the method to return errors because that makes testing easier
server/automute_user_connect.go
Outdated
|
|
||
| automuteEnabled, err := p.enableAutomute(userID) | ||
| if err != nil { | ||
| p.API.LogError( |
There was a problem hiding this comment.
We're about to start raising alerts to SRE for errors. Is this something that we think is human actionable? Or might this fail for other reasons that justifying just a LogWarn?
Alternatively, since we already return the error, can we skip the log altogether?
There was a problem hiding this comment.
Yeah, I can change them to LogWarn since they're not really human actionable as you said. (Edit: I'll also change this for the methods added in the previous part)
And returning vs logging is the opposite of my previous comment. I can change that if you think it's a good idea if you think it's better to log these in api.go to be a bit more idiomatic
Good point @lieut-data . Correct in that receiving "unwanted" DM/GM notifications from Mattermost is dependent on the sender's connection status and not the recipient. +1 on opening it up to non-connected users cc: @hmhealey |
- Fix logged variable naming - Change some log levels - Return early from updateAutomutingOnUserDisconnect
This reverts commit 4d6386e.
hmhealey
force-pushed
the
MM-56501_automute-part-2_user-connect-disconnect
branch
from
f23088e
to
9a02045
Compare
1. Make logging variable names consistent 2. Fix not using perPage in all cases when looping
lieut-data
added
the
QA/Deferred
hmhealey
added
4: Reviews Complete
hmhealey
deleted the
MM-56501_automute-part-2_user-connect-disconnect
branch
Summary
Continuing from #477, since automuting is enabled when a user is both connected and has their primary platform set to MS Teams, we need to also potentially enable or disable automuting when a user connects or disconnects from MS Teams.
Ticket Link
https://mattermost.atlassian.net/browse/MM-56501