From 24654309a21c1b681e42788a266e61f565a23c66 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 7 Oct 2026 14:08:44 +0000 Subject: [PATCH 1/3] Sync 'On a vacation' custom status to GitHub Busy status Opt-in per user via /github settings vacation-sync on|off (default off). Co-authored-by: Devin Binnie --- server/plugin/command.go | 41 ++++ server/plugin/graphql/status.go | 43 +++++ server/plugin/plugin.go | 17 +- server/plugin/template.go | 3 +- server/plugin/vacation_sync.go | 130 +++++++++++++ server/plugin/vacation_sync_test.go | 277 ++++++++++++++++++++++++++++ 6 files changed, 509 insertions(+), 2 deletions(-) create mode 100644 server/plugin/graphql/status.go create mode 100644 server/plugin/vacation_sync.go create mode 100644 server/plugin/vacation_sync_test.go diff --git a/server/plugin/command.go b/server/plugin/command.go index 601cb268b..8bcbaef8d 100644 --- a/server/plugin/command.go +++ b/server/plugin/command.go @@ -802,10 +802,40 @@ func (p *Plugin) handleSettings(_ *plugin.Context, _ *model.CommandArgs, paramet default: return "Invalid value. Accepted values are: \"on\" or \"off\" or \"on-change\" ." } + case settingVacationSync: + switch settingValue { + case settingOn: + userInfo.Settings.SyncVacationStatus = true + case settingOff: + userInfo.Settings.SyncVacationStatus = false + default: + return "Invalid value. Accepted values are: \"on\" or \"off\"." + } default: return "Unknown setting " + setting } + if setting == settingVacationSync { + key := vacationSyncKeyPrefix + userInfo.UserID + if userInfo.Settings.SyncVacationStatus { + if _, err := p.store.Set(key, vacationSyncState{}); err != nil { + p.client.Log.Warn("Failed to store vacation sync state", "userID", userInfo.UserID, "error", err.Error()) + return "Failed to store settings" + } + } else { + var state vacationSyncState + if err := p.store.Get(key, &state); err != nil { + p.client.Log.Warn("Failed to get vacation sync state", "userID", userInfo.UserID, "error", err.Error()) + } + if state.Applied { + if err := p.graphQLConnect(userInfo).ClearStatus(context.Background()); err != nil { + p.client.Log.Warn("Failed to clear GitHub busy status", "userID", userInfo.UserID, "error", err.Error()) + } + } + p.deleteVacationSyncState(key) + } + } + if setting == settingNotifications { if userInfo.Settings.Notifications { err := p.storeGitHubToUserIDMapping(userInfo.GitHubUsername, userInfo.UserID) @@ -1301,6 +1331,17 @@ func getAutocompleteData(config *Configuration) *model.AutocompleteData { remainderNotifications.AddStaticListArgument("", true, settingValue) settings.AddCommand(remainderNotifications) + settingVacationSyncData := model.NewAutocompleteData("vacation-sync", "", "Set your GitHub status to Busy while your Mattermost status is \"On a vacation\"") + settingValue = []model.AutocompleteListItem{{ + HelpText: "Turn vacation sync on", + Item: "on", + }, { + HelpText: "Turn vacation sync off", + Item: "off", + }} + settingVacationSyncData.AddStaticListArgument("", true, settingValue) + settings.AddCommand(settingVacationSyncData) + github.AddCommand(settings) setup := model.NewAutocompleteData("setup", "[command]", "Available commands: oauth, webhook, announcement") diff --git a/server/plugin/graphql/status.go b/server/plugin/graphql/status.go new file mode 100644 index 000000000..180eec7f8 --- /dev/null +++ b/server/plugin/graphql/status.go @@ -0,0 +1,43 @@ +// Copyright (c) 2018-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package graphql + +import ( + "context" + + "github.com/pkg/errors" + "github.com/shurcooL/githubv4" +) + +type changeUserStatusMutation struct { + ChangeUserStatus struct { + Status struct { + Message githubv4.String + } + } `graphql:"changeUserStatus(input: $input)"` +} + +// SetBusyStatus sets the user's GitHub status to Busy, which excludes them from automatic PR assignment. +func (c *Client) SetBusyStatus(ctx context.Context, message, emoji string) error { + return c.changeUserStatus(ctx, githubv4.ChangeUserStatusInput{ + LimitedAvailability: githubv4.NewBoolean(true), + Message: githubv4.NewString(githubv4.String(message)), + Emoji: githubv4.NewString(githubv4.String(emoji)), + }) +} + +// ClearStatus removes the user's GitHub status, including the Busy indicator. +func (c *Client) ClearStatus(ctx context.Context) error { + return c.changeUserStatus(ctx, githubv4.ChangeUserStatusInput{ + LimitedAvailability: githubv4.NewBoolean(false), + }) +} + +func (c *Client) changeUserStatus(ctx context.Context, input githubv4.ChangeUserStatusInput) error { + var m changeUserStatusMutation + if err := c.client.Mutate(ctx, &m, input, nil); err != nil { + return errors.Wrap(err, "error in changing user status") + } + return nil +} diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go index 079bfd172..818eb0a3c 100644 --- a/server/plugin/plugin.go +++ b/server/plugin/plugin.go @@ -51,6 +51,7 @@ const ( settingButtonsTeam = "team" settingNotifications = "notifications" settingReminders = "reminders" + settingVacationSync = "vacation-sync" settingOn = "on" settingOff = "off" settingOnChange = "on-change" @@ -106,6 +107,8 @@ type Plugin struct { slaDigestCancel context.CancelFunc + vacationSyncJob *cluster.Job + emojiMap map[string]string } @@ -300,6 +303,12 @@ func (p *Plugin) OnActivate() error { } }() + vacationSyncJob, err := cluster.Schedule(p.API, vacationSyncJobKey, cluster.MakeWaitForInterval(vacationSyncInterval), p.syncVacationStatuses) + if err != nil { + return errors.Wrap(err, "failed to schedule vacation status sync") + } + p.vacationSyncJob = vacationSyncJob + ctx, cancel := context.WithCancel(context.Background()) p.slaDigestCancel = cancel go p.runSLADigestScheduler(ctx) @@ -311,6 +320,11 @@ func (p *Plugin) OnDeactivate() error { if p.slaDigestCancel != nil { p.slaDigestCancel() } + if p.vacationSyncJob != nil { + if err := p.vacationSyncJob.Close(); err != nil { + p.client.Log.Warn("Failed to close vacation status sync job", "error", err.Error()) + } + } p.webhookBroker.Close() p.oauthBroker.Close() return nil @@ -544,7 +558,7 @@ func (p *Plugin) getOAuthConfig(privateAllowed bool) (*oauth2.Config, error) { // means that asks scope for private repositories repo = github.ScopeRepo } - scopes := []string{string(repo), string(github.ScopeNotifications), string(github.ScopeReadOrg), string(github.ScopeAdminOrgHook)} + scopes := []string{string(repo), string(github.ScopeNotifications), string(github.ScopeReadOrg), string(github.ScopeAdminOrgHook), string(github.ScopeUser)} if config.UsePreregisteredApplication { p.client.Log.Debug("Using Chimera Proxy OAuth configuration") @@ -655,6 +669,7 @@ type UserSettings struct { DailyReminder bool `json:"daily_reminder"` DailyReminderOnChange bool `json:"daily_reminder_on_change"` Notifications bool `json:"notifications"` + SyncVacationStatus bool `json:"sync_vacation_status"` } func (p *Plugin) storeGitHubUserInfo(info *GitHubUserInfo, encryptionKey string) error { diff --git a/server/plugin/template.go b/server/plugin/template.go index 407bd527e..4c1b3b199 100644 --- a/server/plugin/template.go +++ b/server/plugin/template.go @@ -513,8 +513,9 @@ Reviewers: {{range $i, $el := .RequestedReviewers -}} {{- if $i}}, {{end}}{{temp "* `/github subscriptions delete owner[/repo]` - Unsubscribe the current channel from a repository\n" + "* `/github me` - Display the connected GitHub account\n" + "* `/github settings [setting] [value]` - Update your user settings\n" + - " * `setting` can be `notifications` or `reminders`\n" + + " * `setting` can be `notifications`, `reminders` or `vacation-sync`\n" + " * `value` can be `on` or `off`\n" + + " * `vacation-sync` sets your GitHub status to Busy (so you are not auto-assigned PRs) while your Mattermost custom status is \"On a vacation\". Off by default.\n" + "* `/github setup` - Setup your Github plugin\n" + "* `/github mute` - Managed muted GitHub users. You'll not receive notifications for comments in your PRs and issues from those users.\n" + " * `/github mute list` - list your muted GitHub users\n" + diff --git a/server/plugin/vacation_sync.go b/server/plugin/vacation_sync.go new file mode 100644 index 000000000..8570bd9c9 --- /dev/null +++ b/server/plugin/vacation_sync.go @@ -0,0 +1,130 @@ +// Copyright (c) 2018-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package plugin + +import ( + "context" + "strings" + "time" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/pluginapi" +) + +const ( + vacationSyncJobKey = "github_vacation_sync" + vacationSyncKeyPrefix = "vacation_sync_" + vacationSyncInterval = time.Minute + vacationSyncUserTimeout = 30 * time.Second + vacationCustomStatusIcon = "palm_tree" + vacationGitHubEmoji = ":palm_tree:" + vacationGitHubMessage = "On a vacation" +) + +// vacationSyncState is stored per opted-in user under vacationSyncKeyPrefix+userID. +type vacationSyncState struct { + // Applied is true while the plugin has set the user's GitHub status to Busy. + Applied bool `json:"applied"` + // Notified is true once the user was told that the sync failed, to avoid repeating the DM every run. + Notified bool `json:"notified"` +} + +func isOnVacation(user *model.User) bool { + cs := user.GetCustomStatus() + if cs == nil || cs.Emoji != vacationCustomStatusIcon { + return false + } + return cs.ExpiresAt.IsZero() || cs.ExpiresAt.After(time.Now()) +} + +func (p *Plugin) syncVacationStatuses() { + var userIDs []string + for page := 0; ; page++ { + keys, err := p.store.ListKeys(page, keysPerPage, pluginapi.WithPrefix(vacationSyncKeyPrefix)) + if err != nil { + p.client.Log.Warn("Failed to list users for vacation status sync", "page", page, "error", err.Error()) + break + } + for _, key := range keys { + userIDs = append(userIDs, strings.TrimPrefix(key, vacationSyncKeyPrefix)) + } + if len(keys) < keysPerPage { + break + } + } + + for _, userID := range userIDs { + ctx, cancel := context.WithTimeout(context.Background(), vacationSyncUserTimeout) + p.syncUserVacationStatus(ctx, userID) + cancel() + } +} + +func (p *Plugin) syncUserVacationStatus(ctx context.Context, userID string) { + key := vacationSyncKeyPrefix + userID + + info, apiErr := p.getGitHubUserInfo(userID) + if apiErr != nil { + if apiErr.ID == apiErrorIDNotConnected { + p.deleteVacationSyncState(key) + } + return + } + if info.Settings == nil || !info.Settings.SyncVacationStatus { + p.deleteVacationSyncState(key) + return + } + + var state vacationSyncState + if err := p.store.Get(key, &state); err != nil { + p.client.Log.Warn("Failed to get vacation sync state", "userID", userID, "error", err.Error()) + return + } + + user, err := p.client.User.Get(userID) + if err != nil { + p.client.Log.Warn("Failed to get user for vacation status sync", "userID", userID, "error", err.Error()) + return + } + + next := state + onVacation := isOnVacation(user) + switch { + case onVacation && !state.Applied: + err = p.graphQLConnect(info).SetBusyStatus(ctx, vacationGitHubMessage, vacationGitHubEmoji) + if err != nil { + p.client.Log.Warn("Failed to set GitHub status to busy", "userID", userID, "error", err.Error()) + if !state.Notified { + p.CreateBotDMPost(userID, + "Failed to set your GitHub status to Busy while you are on vacation. Your GitHub connection may be missing the `user` permission. Please reconnect your account using `/github disconnect` followed by `/github connect`.", + "custom_git_vacation_sync") + next.Notified = true + } + } else { + next.Applied = true + next.Notified = false + } + case !onVacation && state.Applied: + if err = p.graphQLConnect(info).ClearStatus(ctx); err != nil { + p.client.Log.Warn("Failed to clear GitHub busy status", "userID", userID, "error", err.Error()) + } else { + next.Applied = false + } + case !onVacation: + next.Notified = false + } + + if next == state { + return + } + if _, err = p.store.Set(key, next); err != nil { + p.client.Log.Warn("Failed to store vacation sync state", "userID", userID, "error", err.Error()) + } +} + +func (p *Plugin) deleteVacationSyncState(key string) { + if err := p.store.Delete(key); err != nil { + p.client.Log.Warn("Failed to delete vacation sync state", "key", key, "error", err.Error()) + } +} diff --git a/server/plugin/vacation_sync_test.go b/server/plugin/vacation_sync_test.go new file mode 100644 index 000000000..b64f2299c --- /dev/null +++ b/server/plugin/vacation_sync_test.go @@ -0,0 +1,277 @@ +// Copyright (c) 2018-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package plugin + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" + + "github.com/mattermost/mattermost-plugin-github/server/mocks" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/plugin/plugintest" +) + +func vacationUser(t *testing.T, cs *model.CustomStatus) *model.User { + t.Helper() + user := &model.User{Id: MockUserID, Props: model.StringMap{}} + if cs != nil { + require.NoError(t, user.SetCustomStatus(cs)) + } + return user +} + +func TestIsOnVacation(t *testing.T) { + tests := []struct { + name string + status *model.CustomStatus + expected bool + }{ + {name: "no custom status", status: nil, expected: false}, + {name: "other custom status", status: &model.CustomStatus{Emoji: "calendar", Text: "In a meeting"}, expected: false}, + {name: "vacation without expiry", status: &model.CustomStatus{Emoji: "palm_tree", Text: "On a vacation"}, expected: true}, + {name: "vacation not yet expired", status: &model.CustomStatus{Emoji: "palm_tree", ExpiresAt: time.Now().Add(time.Hour)}, expected: true}, + {name: "vacation expired", status: &model.CustomStatus{Emoji: "palm_tree", ExpiresAt: time.Now().Add(-time.Hour)}, expected: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expected, isOnVacation(vacationUser(t, tc.status))) + }) + } +} + +type vacationSyncTestEnv struct { + p *Plugin + store *mocks.MockKvStore + api *plugintest.API + requests chan map[string]any + info *GitHubUserInfo + key string +} + +func setupVacationSyncTest(t *testing.T, graphQLStatus int, syncEnabled bool) *vacationSyncTestEnv { + t.Helper() + + requests := make(chan map[string]any, 10) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + require.NoError(t, err) + var payload map[string]any + require.NoError(t, json.Unmarshal(body, &payload)) + requests <- payload + w.WriteHeader(graphQLStatus) + _, err = w.Write([]byte(`{"data":{"changeUserStatus":{"status":{"message":"ok"}}}}`)) + require.NoError(t, err) + })) + t.Cleanup(server.Close) + + ctrl := gomock.NewController(t) + store := mocks.NewMockKvStore(ctrl) + api := &plugintest.API{} + p := getPluginTest(api, store) + + encryptionKey := "dummyEncryptKey1" + p.setConfiguration(&Configuration{EncryptionKey: encryptionKey, EnterpriseBaseURL: server.URL}) + encryptedToken, err := encrypt([]byte(encryptionKey), MockAccessToken) + require.NoError(t, err) + + stored, err := json.Marshal(&GitHubUserInfo{ + UserID: MockUserID, + GitHubUsername: MockUsername, + Token: &oauth2.Token{AccessToken: encryptedToken}, + Settings: &UserSettings{SyncVacationStatus: syncEnabled}, + }) + require.NoError(t, err) + store.EXPECT().Get(MockUserID+githubTokenKey, gomock.Any()).DoAndReturn(func(_ string, out any) error { + return json.Unmarshal(stored, out) + }).AnyTimes() + + return &vacationSyncTestEnv{ + p: p, + store: store, + api: api, + requests: requests, + key: vacationSyncKeyPrefix + MockUserID, + } +} + +func (e *vacationSyncTestEnv) expectState(t *testing.T, state vacationSyncState) { + t.Helper() + e.store.EXPECT().Get(e.key, gomock.Any()).DoAndReturn(func(_ string, out any) error { + b, err := json.Marshal(state) + require.NoError(t, err) + return json.Unmarshal(b, out) + }) +} + +func (e *vacationSyncTestEnv) expectUser(user *model.User) { + e.api.On("GetUser", MockUserID).Return(user, nil) +} + +func receiveMutationInput(t *testing.T, requests chan map[string]any) map[string]any { + t.Helper() + select { + case payload := <-requests: + variables, ok := payload["variables"].(map[string]any) + require.True(t, ok) + input, ok := variables["input"].(map[string]any) + require.True(t, ok) + return input + case <-time.After(time.Second): + require.FailNow(t, "expected a GraphQL request") + return nil + } +} + +func TestSyncUserVacationStatus(t *testing.T) { + vacationStatus := &model.CustomStatus{Emoji: "palm_tree", Text: "On a vacation"} + + t.Run("sets Busy when user goes on vacation", func(t *testing.T) { + env := setupVacationSyncTest(t, http.StatusOK, true) + env.expectState(t, vacationSyncState{}) + env.expectUser(vacationUser(t, vacationStatus)) + env.store.EXPECT().Set(env.key, vacationSyncState{Applied: true}).Return(true, nil) + + env.p.syncUserVacationStatus(context.Background(), MockUserID) + + input := receiveMutationInput(t, env.requests) + assert.Equal(t, true, input["limitedAvailability"]) + assert.Equal(t, "On a vacation", input["message"]) + assert.Equal(t, ":palm_tree:", input["emoji"]) + }) + + t.Run("clears status when user returns from vacation", func(t *testing.T) { + env := setupVacationSyncTest(t, http.StatusOK, true) + env.expectState(t, vacationSyncState{Applied: true}) + env.expectUser(vacationUser(t, nil)) + env.store.EXPECT().Set(env.key, vacationSyncState{}).Return(true, nil) + + env.p.syncUserVacationStatus(context.Background(), MockUserID) + + input := receiveMutationInput(t, env.requests) + assert.Equal(t, false, input["limitedAvailability"]) + assert.NotContains(t, input, "message") + }) + + t.Run("does nothing when already applied and still on vacation", func(t *testing.T) { + env := setupVacationSyncTest(t, http.StatusOK, true) + env.expectState(t, vacationSyncState{Applied: true}) + env.expectUser(vacationUser(t, vacationStatus)) + + env.p.syncUserVacationStatus(context.Background(), MockUserID) + + assert.Empty(t, env.requests) + }) + + t.Run("does not touch GitHub status when never on vacation", func(t *testing.T) { + env := setupVacationSyncTest(t, http.StatusOK, true) + env.expectState(t, vacationSyncState{}) + env.expectUser(vacationUser(t, nil)) + + env.p.syncUserVacationStatus(context.Background(), MockUserID) + + assert.Empty(t, env.requests) + }) + + t.Run("removes marker when setting is disabled", func(t *testing.T) { + env := setupVacationSyncTest(t, http.StatusOK, false) + env.store.EXPECT().Delete(env.key).Return(nil) + + env.p.syncUserVacationStatus(context.Background(), MockUserID) + + assert.Empty(t, env.requests) + }) + + t.Run("notifies the user once when GitHub rejects the update", func(t *testing.T) { + env := setupVacationSyncTest(t, http.StatusForbidden, true) + env.expectState(t, vacationSyncState{}) + env.expectUser(vacationUser(t, vacationStatus)) + env.store.EXPECT().Set(env.key, vacationSyncState{Notified: true}).Return(true, nil) + env.api.On("LogWarn", "Failed to set GitHub status to busy", "userID", MockUserID, "error", mock.Anything).Once() + env.api.On("GetDirectChannel", MockUserID, MockBotID).Return(&model.Channel{Id: MockChannelID}, nil) + env.api.On("CreatePost", mock.MatchedBy(func(post *model.Post) bool { + return post.ChannelId == MockChannelID && post.UserId == MockBotID + })).Return(&model.Post{}, nil) + + env.p.syncUserVacationStatus(context.Background(), MockUserID) + + receiveMutationInput(t, env.requests) + env.api.AssertExpectations(t) + }) + + t.Run("does not notify again after a previous failure", func(t *testing.T) { + env := setupVacationSyncTest(t, http.StatusForbidden, true) + env.expectState(t, vacationSyncState{Notified: true}) + env.expectUser(vacationUser(t, vacationStatus)) + env.api.On("LogWarn", "Failed to set GitHub status to busy", "userID", MockUserID, "error", mock.Anything).Once() + + env.p.syncUserVacationStatus(context.Background(), MockUserID) + + receiveMutationInput(t, env.requests) + env.api.AssertNotCalled(t, "CreatePost", mock.Anything) + }) +} + +func TestHandleSettingsVacationSync(t *testing.T) { + t.Run("on stores the sync marker", func(t *testing.T) { + env := setupVacationSyncTest(t, http.StatusOK, false) + info := &GitHubUserInfo{UserID: MockUserID, GitHubUsername: MockUsername, Token: &oauth2.Token{AccessToken: MockAccessToken}, Settings: &UserSettings{}} + env.store.EXPECT().Set(env.key, vacationSyncState{}).Return(true, nil) + env.store.EXPECT().Set(MockUserID+githubTokenKey, gomock.Any()).Return(true, nil) + + result := env.p.handleSettings(nil, nil, []string{settingVacationSync, settingOn}, info) + + assert.Equal(t, "Settings updated.", result) + assert.True(t, info.Settings.SyncVacationStatus) + }) + + t.Run("off while Busy is applied clears GitHub status and the marker", func(t *testing.T) { + env := setupVacationSyncTest(t, http.StatusOK, true) + info := &GitHubUserInfo{UserID: MockUserID, GitHubUsername: MockUsername, Token: &oauth2.Token{AccessToken: MockAccessToken}, Settings: &UserSettings{SyncVacationStatus: true}} + env.expectState(t, vacationSyncState{Applied: true}) + env.store.EXPECT().Delete(env.key).Return(nil) + env.store.EXPECT().Set(MockUserID+githubTokenKey, gomock.Any()).Return(true, nil) + + result := env.p.handleSettings(nil, nil, []string{settingVacationSync, settingOff}, info) + + assert.Equal(t, "Settings updated.", result) + assert.False(t, info.Settings.SyncVacationStatus) + input := receiveMutationInput(t, env.requests) + assert.Equal(t, false, input["limitedAvailability"]) + }) + + t.Run("off without an applied status only removes the marker", func(t *testing.T) { + env := setupVacationSyncTest(t, http.StatusOK, true) + info := &GitHubUserInfo{UserID: MockUserID, GitHubUsername: MockUsername, Token: &oauth2.Token{AccessToken: MockAccessToken}, Settings: &UserSettings{SyncVacationStatus: true}} + env.expectState(t, vacationSyncState{}) + env.store.EXPECT().Delete(env.key).Return(nil) + env.store.EXPECT().Set(MockUserID+githubTokenKey, gomock.Any()).Return(true, nil) + + result := env.p.handleSettings(nil, nil, []string{settingVacationSync, settingOff}, info) + + assert.Equal(t, "Settings updated.", result) + assert.Empty(t, env.requests) + }) + + t.Run("invalid value", func(t *testing.T) { + env := setupVacationSyncTest(t, http.StatusOK, false) + info := &GitHubUserInfo{UserID: MockUserID, Settings: &UserSettings{}} + + result := env.p.handleSettings(nil, nil, []string{settingVacationSync, "maybe"}, info) + + assert.Equal(t, "Invalid value. Accepted values are: \"on\" or \"off\".", result) + }) +} From 7c677f72c7f48c559ca0771b8fbc493dd1da4beb Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 7 Oct 2026 14:12:32 +0000 Subject: [PATCH 2/3] Drive vacation sync from PreferencesHaveChanged instead of a polling job Co-authored-by: Devin Binnie --- server/plugin/command.go | 22 ++--- server/plugin/plugin.go | 13 --- server/plugin/vacation_sync.go | 129 +++++++++++----------------- server/plugin/vacation_sync_test.go | 117 +++++++++++++++---------- 4 files changed, 125 insertions(+), 156 deletions(-) diff --git a/server/plugin/command.go b/server/plugin/command.go index 8bcbaef8d..a40eb3843 100644 --- a/server/plugin/command.go +++ b/server/plugin/command.go @@ -816,24 +816,16 @@ func (p *Plugin) handleSettings(_ *plugin.Context, _ *model.CommandArgs, paramet } if setting == settingVacationSync { - key := vacationSyncKeyPrefix + userInfo.UserID + onVacation := false if userInfo.Settings.SyncVacationStatus { - if _, err := p.store.Set(key, vacationSyncState{}); err != nil { - p.client.Log.Warn("Failed to store vacation sync state", "userID", userInfo.UserID, "error", err.Error()) - return "Failed to store settings" - } - } else { - var state vacationSyncState - if err := p.store.Get(key, &state); err != nil { - p.client.Log.Warn("Failed to get vacation sync state", "userID", userInfo.UserID, "error", err.Error()) - } - if state.Applied { - if err := p.graphQLConnect(userInfo).ClearStatus(context.Background()); err != nil { - p.client.Log.Warn("Failed to clear GitHub busy status", "userID", userInfo.UserID, "error", err.Error()) - } + user, err := p.client.User.Get(userInfo.UserID) + if err != nil { + p.client.Log.Warn("Failed to get user for vacation status sync", "userID", userInfo.UserID, "error", err.Error()) + } else { + onVacation = isOnVacation(user.GetCustomStatus()) } - p.deleteVacationSyncState(key) } + p.applyVacationStatus(context.Background(), userInfo, onVacation) } if setting == settingNotifications { diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go index 818eb0a3c..0d20980e0 100644 --- a/server/plugin/plugin.go +++ b/server/plugin/plugin.go @@ -107,8 +107,6 @@ type Plugin struct { slaDigestCancel context.CancelFunc - vacationSyncJob *cluster.Job - emojiMap map[string]string } @@ -303,12 +301,6 @@ func (p *Plugin) OnActivate() error { } }() - vacationSyncJob, err := cluster.Schedule(p.API, vacationSyncJobKey, cluster.MakeWaitForInterval(vacationSyncInterval), p.syncVacationStatuses) - if err != nil { - return errors.Wrap(err, "failed to schedule vacation status sync") - } - p.vacationSyncJob = vacationSyncJob - ctx, cancel := context.WithCancel(context.Background()) p.slaDigestCancel = cancel go p.runSLADigestScheduler(ctx) @@ -320,11 +312,6 @@ func (p *Plugin) OnDeactivate() error { if p.slaDigestCancel != nil { p.slaDigestCancel() } - if p.vacationSyncJob != nil { - if err := p.vacationSyncJob.Close(); err != nil { - p.client.Log.Warn("Failed to close vacation status sync job", "error", err.Error()) - } - } p.webhookBroker.Close() p.oauthBroker.Close() return nil diff --git a/server/plugin/vacation_sync.go b/server/plugin/vacation_sync.go index 8570bd9c9..a21504dcb 100644 --- a/server/plugin/vacation_sync.go +++ b/server/plugin/vacation_sync.go @@ -5,126 +5,93 @@ package plugin import ( "context" - "strings" + "encoding/json" "time" "github.com/mattermost/mattermost/server/public/model" - "github.com/mattermost/mattermost/server/public/pluginapi" + "github.com/mattermost/mattermost/server/public/plugin" ) const ( - vacationSyncJobKey = "github_vacation_sync" vacationSyncKeyPrefix = "vacation_sync_" - vacationSyncInterval = time.Minute - vacationSyncUserTimeout = 30 * time.Second + vacationSyncTimeout = 30 * time.Second vacationCustomStatusIcon = "palm_tree" vacationGitHubEmoji = ":palm_tree:" vacationGitHubMessage = "On a vacation" ) -// vacationSyncState is stored per opted-in user under vacationSyncKeyPrefix+userID. +// vacationSyncState is stored per user so a GitHub status the plugin did not set is never cleared. type vacationSyncState struct { - // Applied is true while the plugin has set the user's GitHub status to Busy. Applied bool `json:"applied"` - // Notified is true once the user was told that the sync failed, to avoid repeating the DM every run. - Notified bool `json:"notified"` } -func isOnVacation(user *model.User) bool { - cs := user.GetCustomStatus() - if cs == nil || cs.Emoji != vacationCustomStatusIcon { - return false - } - return cs.ExpiresAt.IsZero() || cs.ExpiresAt.After(time.Now()) -} +// PreferencesHaveChanged reacts to custom status changes: the server saves the "recent custom +// statuses" preference whenever a user sets a custom status, and the newest one is first. +func (p *Plugin) PreferencesHaveChanged(_ *plugin.Context, preferences []model.Preference) { + for _, preference := range preferences { + if preference.Category != model.PreferenceCategoryCustomStatus || preference.Name != model.PreferenceNameRecentCustomStatuses { + continue + } -func (p *Plugin) syncVacationStatuses() { - var userIDs []string - for page := 0; ; page++ { - keys, err := p.store.ListKeys(page, keysPerPage, pluginapi.WithPrefix(vacationSyncKeyPrefix)) - if err != nil { - p.client.Log.Warn("Failed to list users for vacation status sync", "page", page, "error", err.Error()) - break + var recent model.RecentCustomStatuses + if err := json.Unmarshal([]byte(preference.Value), &recent); err != nil { + p.client.Log.Warn("Failed to parse recent custom statuses", "userID", preference.UserId, "error", err.Error()) + continue } - for _, key := range keys { - userIDs = append(userIDs, strings.TrimPrefix(key, vacationSyncKeyPrefix)) + if len(recent) == 0 { + continue } - if len(keys) < keysPerPage { - break + + info, apiErr := p.getGitHubUserInfo(preference.UserId) + if apiErr != nil || info.Settings == nil || !info.Settings.SyncVacationStatus { + continue } - } - for _, userID := range userIDs { - ctx, cancel := context.WithTimeout(context.Background(), vacationSyncUserTimeout) - p.syncUserVacationStatus(ctx, userID) + ctx, cancel := context.WithTimeout(context.Background(), vacationSyncTimeout) + p.applyVacationStatus(ctx, info, isOnVacation(&recent[0])) cancel() } } -func (p *Plugin) syncUserVacationStatus(ctx context.Context, userID string) { - key := vacationSyncKeyPrefix + userID - - info, apiErr := p.getGitHubUserInfo(userID) - if apiErr != nil { - if apiErr.ID == apiErrorIDNotConnected { - p.deleteVacationSyncState(key) - } - return - } - if info.Settings == nil || !info.Settings.SyncVacationStatus { - p.deleteVacationSyncState(key) - return +func isOnVacation(cs *model.CustomStatus) bool { + if cs == nil || cs.Emoji != vacationCustomStatusIcon { + return false } + return cs.ExpiresAt.IsZero() || cs.ExpiresAt.After(time.Now()) +} + +// applyVacationStatus sets the user's GitHub status to Busy when they go on vacation and clears it +// when they come back, but only if the plugin was the one that set it. +func (p *Plugin) applyVacationStatus(ctx context.Context, info *GitHubUserInfo, onVacation bool) { + key := vacationSyncKeyPrefix + info.UserID var state vacationSyncState if err := p.store.Get(key, &state); err != nil { - p.client.Log.Warn("Failed to get vacation sync state", "userID", userID, "error", err.Error()) - return - } - - user, err := p.client.User.Get(userID) - if err != nil { - p.client.Log.Warn("Failed to get user for vacation status sync", "userID", userID, "error", err.Error()) + p.client.Log.Warn("Failed to get vacation sync state", "userID", info.UserID, "error", err.Error()) return } - next := state - onVacation := isOnVacation(user) switch { case onVacation && !state.Applied: - err = p.graphQLConnect(info).SetBusyStatus(ctx, vacationGitHubMessage, vacationGitHubEmoji) - if err != nil { - p.client.Log.Warn("Failed to set GitHub status to busy", "userID", userID, "error", err.Error()) - if !state.Notified { - p.CreateBotDMPost(userID, - "Failed to set your GitHub status to Busy while you are on vacation. Your GitHub connection may be missing the `user` permission. Please reconnect your account using `/github disconnect` followed by `/github connect`.", - "custom_git_vacation_sync") - next.Notified = true - } - } else { - next.Applied = true - next.Notified = false + if err := p.graphQLConnect(info).SetBusyStatus(ctx, vacationGitHubMessage, vacationGitHubEmoji); err != nil { + p.client.Log.Warn("Failed to set GitHub status to busy", "userID", info.UserID, "error", err.Error()) + p.CreateBotDMPost(info.UserID, + "Failed to set your GitHub status to Busy while you are on vacation. Your GitHub connection may be missing the `user` permission. Please reconnect your account using `/github disconnect` followed by `/github connect`.", + "custom_git_vacation_sync") + return } + state.Applied = true case !onVacation && state.Applied: - if err = p.graphQLConnect(info).ClearStatus(ctx); err != nil { - p.client.Log.Warn("Failed to clear GitHub busy status", "userID", userID, "error", err.Error()) - } else { - next.Applied = false + if err := p.graphQLConnect(info).ClearStatus(ctx); err != nil { + p.client.Log.Warn("Failed to clear GitHub busy status", "userID", info.UserID, "error", err.Error()) + return } - case !onVacation: - next.Notified = false - } - - if next == state { + state.Applied = false + default: return } - if _, err = p.store.Set(key, next); err != nil { - p.client.Log.Warn("Failed to store vacation sync state", "userID", userID, "error", err.Error()) - } -} -func (p *Plugin) deleteVacationSyncState(key string) { - if err := p.store.Delete(key); err != nil { - p.client.Log.Warn("Failed to delete vacation sync state", "key", key, "error", err.Error()) + if _, err := p.store.Set(key, state); err != nil { + p.client.Log.Warn("Failed to store vacation sync state", "userID", info.UserID, "error", err.Error()) } } diff --git a/server/plugin/vacation_sync_test.go b/server/plugin/vacation_sync_test.go index b64f2299c..134183ecd 100644 --- a/server/plugin/vacation_sync_test.go +++ b/server/plugin/vacation_sync_test.go @@ -4,7 +4,6 @@ package plugin import ( - "context" "encoding/json" "io" "net/http" @@ -48,7 +47,7 @@ func TestIsOnVacation(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.expected, isOnVacation(vacationUser(t, tc.status))) + assert.Equal(t, tc.expected, isOnVacation(tc.status)) }) } } @@ -136,16 +135,28 @@ func receiveMutationInput(t *testing.T, requests chan map[string]any) map[string } } -func TestSyncUserVacationStatus(t *testing.T) { - vacationStatus := &model.CustomStatus{Emoji: "palm_tree", Text: "On a vacation"} +func recentStatusPreference(t *testing.T, statuses ...model.CustomStatus) model.Preference { + t.Helper() + value, err := json.Marshal(statuses) + require.NoError(t, err) + return model.Preference{ + UserId: MockUserID, + Category: model.PreferenceCategoryCustomStatus, + Name: model.PreferenceNameRecentCustomStatuses, + Value: string(value), + } +} + +func TestPreferencesHaveChanged(t *testing.T) { + vacation := model.CustomStatus{Emoji: "palm_tree", Text: "On a vacation"} + meeting := model.CustomStatus{Emoji: "calendar", Text: "In a meeting"} - t.Run("sets Busy when user goes on vacation", func(t *testing.T) { + t.Run("sets Busy when the newest status is vacation", func(t *testing.T) { env := setupVacationSyncTest(t, http.StatusOK, true) env.expectState(t, vacationSyncState{}) - env.expectUser(vacationUser(t, vacationStatus)) env.store.EXPECT().Set(env.key, vacationSyncState{Applied: true}).Return(true, nil) - env.p.syncUserVacationStatus(context.Background(), MockUserID) + env.p.PreferencesHaveChanged(nil, []model.Preference{recentStatusPreference(t, vacation, meeting)}) input := receiveMutationInput(t, env.requests) assert.Equal(t, true, input["limitedAvailability"]) @@ -153,96 +164,110 @@ func TestSyncUserVacationStatus(t *testing.T) { assert.Equal(t, ":palm_tree:", input["emoji"]) }) - t.Run("clears status when user returns from vacation", func(t *testing.T) { + t.Run("clears Busy when a different status is set after vacation", func(t *testing.T) { env := setupVacationSyncTest(t, http.StatusOK, true) env.expectState(t, vacationSyncState{Applied: true}) - env.expectUser(vacationUser(t, nil)) env.store.EXPECT().Set(env.key, vacationSyncState{}).Return(true, nil) - env.p.syncUserVacationStatus(context.Background(), MockUserID) + env.p.PreferencesHaveChanged(nil, []model.Preference{recentStatusPreference(t, meeting, vacation)}) input := receiveMutationInput(t, env.requests) assert.Equal(t, false, input["limitedAvailability"]) assert.NotContains(t, input, "message") }) - t.Run("does nothing when already applied and still on vacation", func(t *testing.T) { + t.Run("does not touch GitHub when Busy was not applied by the plugin", func(t *testing.T) { env := setupVacationSyncTest(t, http.StatusOK, true) - env.expectState(t, vacationSyncState{Applied: true}) - env.expectUser(vacationUser(t, vacationStatus)) + env.expectState(t, vacationSyncState{}) - env.p.syncUserVacationStatus(context.Background(), MockUserID) + env.p.PreferencesHaveChanged(nil, []model.Preference{recentStatusPreference(t, meeting)}) assert.Empty(t, env.requests) }) - t.Run("does not touch GitHub status when never on vacation", func(t *testing.T) { + t.Run("does nothing when already applied and vacation is set again", func(t *testing.T) { env := setupVacationSyncTest(t, http.StatusOK, true) - env.expectState(t, vacationSyncState{}) - env.expectUser(vacationUser(t, nil)) + env.expectState(t, vacationSyncState{Applied: true}) - env.p.syncUserVacationStatus(context.Background(), MockUserID) + env.p.PreferencesHaveChanged(nil, []model.Preference{recentStatusPreference(t, vacation)}) assert.Empty(t, env.requests) }) - t.Run("removes marker when setting is disabled", func(t *testing.T) { + t.Run("ignores users who have not enabled the setting", func(t *testing.T) { env := setupVacationSyncTest(t, http.StatusOK, false) - env.store.EXPECT().Delete(env.key).Return(nil) - env.p.syncUserVacationStatus(context.Background(), MockUserID) + env.p.PreferencesHaveChanged(nil, []model.Preference{recentStatusPreference(t, vacation)}) assert.Empty(t, env.requests) }) - t.Run("notifies the user once when GitHub rejects the update", func(t *testing.T) { + t.Run("ignores unrelated preferences", func(t *testing.T) { + env := setupVacationSyncTest(t, http.StatusOK, true) + + env.p.PreferencesHaveChanged(nil, []model.Preference{ + {UserId: MockUserID, Category: model.PreferenceCategoryTheme, Name: model.PreferenceNameRecentCustomStatuses, Value: "[]"}, + {UserId: MockUserID, Category: model.PreferenceCategoryCustomStatus, Name: "other", Value: "[]"}, + }) + + assert.Empty(t, env.requests) + }) + + t.Run("notifies the user when GitHub rejects the update", func(t *testing.T) { env := setupVacationSyncTest(t, http.StatusForbidden, true) env.expectState(t, vacationSyncState{}) - env.expectUser(vacationUser(t, vacationStatus)) - env.store.EXPECT().Set(env.key, vacationSyncState{Notified: true}).Return(true, nil) env.api.On("LogWarn", "Failed to set GitHub status to busy", "userID", MockUserID, "error", mock.Anything).Once() env.api.On("GetDirectChannel", MockUserID, MockBotID).Return(&model.Channel{Id: MockChannelID}, nil) env.api.On("CreatePost", mock.MatchedBy(func(post *model.Post) bool { return post.ChannelId == MockChannelID && post.UserId == MockBotID })).Return(&model.Post{}, nil) - env.p.syncUserVacationStatus(context.Background(), MockUserID) + env.p.PreferencesHaveChanged(nil, []model.Preference{recentStatusPreference(t, vacation)}) receiveMutationInput(t, env.requests) env.api.AssertExpectations(t) }) +} - t.Run("does not notify again after a previous failure", func(t *testing.T) { - env := setupVacationSyncTest(t, http.StatusForbidden, true) - env.expectState(t, vacationSyncState{Notified: true}) - env.expectUser(vacationUser(t, vacationStatus)) - env.api.On("LogWarn", "Failed to set GitHub status to busy", "userID", MockUserID, "error", mock.Anything).Once() +func TestHandleSettingsVacationSync(t *testing.T) { + newInfo := func(enabled bool) *GitHubUserInfo { + return &GitHubUserInfo{UserID: MockUserID, GitHubUsername: MockUsername, Token: &oauth2.Token{AccessToken: MockAccessToken}, Settings: &UserSettings{SyncVacationStatus: enabled}} + } - env.p.syncUserVacationStatus(context.Background(), MockUserID) + t.Run("on while already on vacation sets Busy", func(t *testing.T) { + env := setupVacationSyncTest(t, http.StatusOK, false) + info := newInfo(false) + env.expectUser(vacationUser(t, &model.CustomStatus{Emoji: "palm_tree", Text: "On a vacation"})) + env.expectState(t, vacationSyncState{}) + env.store.EXPECT().Set(env.key, vacationSyncState{Applied: true}).Return(true, nil) + env.store.EXPECT().Set(MockUserID+githubTokenKey, gomock.Any()).Return(true, nil) - receiveMutationInput(t, env.requests) - env.api.AssertNotCalled(t, "CreatePost", mock.Anything) + result := env.p.handleSettings(nil, nil, []string{settingVacationSync, settingOn}, info) + + assert.Equal(t, "Settings updated.", result) + assert.True(t, info.Settings.SyncVacationStatus) + input := receiveMutationInput(t, env.requests) + assert.Equal(t, true, input["limitedAvailability"]) }) -} -func TestHandleSettingsVacationSync(t *testing.T) { - t.Run("on stores the sync marker", func(t *testing.T) { + t.Run("on while not on vacation does not touch GitHub", func(t *testing.T) { env := setupVacationSyncTest(t, http.StatusOK, false) - info := &GitHubUserInfo{UserID: MockUserID, GitHubUsername: MockUsername, Token: &oauth2.Token{AccessToken: MockAccessToken}, Settings: &UserSettings{}} - env.store.EXPECT().Set(env.key, vacationSyncState{}).Return(true, nil) + info := newInfo(false) + env.expectUser(vacationUser(t, nil)) + env.expectState(t, vacationSyncState{}) env.store.EXPECT().Set(MockUserID+githubTokenKey, gomock.Any()).Return(true, nil) result := env.p.handleSettings(nil, nil, []string{settingVacationSync, settingOn}, info) assert.Equal(t, "Settings updated.", result) - assert.True(t, info.Settings.SyncVacationStatus) + assert.Empty(t, env.requests) }) - t.Run("off while Busy is applied clears GitHub status and the marker", func(t *testing.T) { + t.Run("off while Busy is applied clears the GitHub status", func(t *testing.T) { env := setupVacationSyncTest(t, http.StatusOK, true) - info := &GitHubUserInfo{UserID: MockUserID, GitHubUsername: MockUsername, Token: &oauth2.Token{AccessToken: MockAccessToken}, Settings: &UserSettings{SyncVacationStatus: true}} + info := newInfo(true) env.expectState(t, vacationSyncState{Applied: true}) - env.store.EXPECT().Delete(env.key).Return(nil) + env.store.EXPECT().Set(env.key, vacationSyncState{}).Return(true, nil) env.store.EXPECT().Set(MockUserID+githubTokenKey, gomock.Any()).Return(true, nil) result := env.p.handleSettings(nil, nil, []string{settingVacationSync, settingOff}, info) @@ -253,11 +278,10 @@ func TestHandleSettingsVacationSync(t *testing.T) { assert.Equal(t, false, input["limitedAvailability"]) }) - t.Run("off without an applied status only removes the marker", func(t *testing.T) { + t.Run("off without an applied status does not touch GitHub", func(t *testing.T) { env := setupVacationSyncTest(t, http.StatusOK, true) - info := &GitHubUserInfo{UserID: MockUserID, GitHubUsername: MockUsername, Token: &oauth2.Token{AccessToken: MockAccessToken}, Settings: &UserSettings{SyncVacationStatus: true}} + info := newInfo(true) env.expectState(t, vacationSyncState{}) - env.store.EXPECT().Delete(env.key).Return(nil) env.store.EXPECT().Set(MockUserID+githubTokenKey, gomock.Any()).Return(true, nil) result := env.p.handleSettings(nil, nil, []string{settingVacationSync, settingOff}, info) @@ -268,9 +292,8 @@ func TestHandleSettingsVacationSync(t *testing.T) { t.Run("invalid value", func(t *testing.T) { env := setupVacationSyncTest(t, http.StatusOK, false) - info := &GitHubUserInfo{UserID: MockUserID, Settings: &UserSettings{}} - result := env.p.handleSettings(nil, nil, []string{settingVacationSync, "maybe"}, info) + result := env.p.handleSettings(nil, nil, []string{settingVacationSync, "maybe"}, newInfo(false)) assert.Equal(t, "Invalid value. Accepted values are: \"on\" or \"off\".", result) }) From b836a3fd942ad119e5a312ca5d3d80b6e04c9643 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 7 Oct 2026 14:18:28 +0000 Subject: [PATCH 3/3] Remove unused test env field Co-authored-by: Devin Binnie --- server/plugin/vacation_sync_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/server/plugin/vacation_sync_test.go b/server/plugin/vacation_sync_test.go index 134183ecd..255b1630a 100644 --- a/server/plugin/vacation_sync_test.go +++ b/server/plugin/vacation_sync_test.go @@ -57,7 +57,6 @@ type vacationSyncTestEnv struct { store *mocks.MockKvStore api *plugintest.API requests chan map[string]any - info *GitHubUserInfo key string }