From 72217ca11b8ede4fe11ca1c5ea84af46e0f0c95f Mon Sep 17 00:00:00 2001 From: Michael Kochell <6913320+mickmister@users.noreply.github.com> Date: Thu, 21 May 2020 04:46:18 -0400 Subject: [PATCH 1/5] Remove daily summary index --- server/mscalendar/daily_summary.go | 90 ++++++++++---------- server/mscalendar/user.go | 5 -- server/store/daily_summary_store.go | 114 -------------------------- server/store/mock_store/mock_store.go | 72 ---------------- server/store/setting_store.go | 18 ++-- server/store/store.go | 4 - server/store/user_store.go | 8 ++ 7 files changed, 61 insertions(+), 250 deletions(-) delete mode 100644 server/store/daily_summary_store.go diff --git a/server/mscalendar/daily_summary.go b/server/mscalendar/daily_summary.go index 55a2ad0b..2e935a59 100644 --- a/server/mscalendar/daily_summary.go +++ b/server/mscalendar/daily_summary.go @@ -30,21 +30,24 @@ type DailySummary interface { } func (m *mscalendar) GetDailySummarySettingsForUser(user *User) (*store.DailySummaryUserSettings, error) { - dsumIndex, err := m.Store.LoadDailySummaryIndex() + err := m.Filter(withUserExpanded(user)) if err != nil { return nil, err } - for _, dsum := range dsumIndex { - if dsum.MattermostUserID == user.MattermostUserID { - return dsum, nil - } + dsum := user.Settings.DailySummary + if dsum == nil { + return nil, errors.New("No daily summary settings found") } - - return nil, errors.New("No daily summary settings found") + return dsum, nil } func (m *mscalendar) SetDailySummaryPostTime(user *User, timeStr string) (*store.DailySummaryUserSettings, error) { + err := m.Filter(withUserExpanded(user)) + if err != nil { + return nil, err + } + t, err := time.Parse(time.Kitchen, timeStr) if err != nil { return nil, errors.New("Invalid time value: " + timeStr) @@ -59,18 +62,16 @@ func (m *mscalendar) SetDailySummaryPostTime(user *User, timeStr string) (*store return nil, err } - dsum, err := m.Store.LoadDailySummaryUserSettings(user.MattermostUserID) - if err != nil { - return nil, err - } + dsum := user.Settings.DailySummary if dsum == nil { - dsum = &store.DailySummaryUserSettings{MattermostUserID: user.MattermostUserID} + dsum = &store.DailySummaryUserSettings{} + user.Settings.DailySummary = dsum } dsum.PostTime = timeStr dsum.Timezone = timezone - err = m.Store.StoreDailySummaryUserSettings(dsum) + err = m.Store.StoreUser(user.User) if err != nil { return nil, err } @@ -78,24 +79,19 @@ func (m *mscalendar) SetDailySummaryPostTime(user *User, timeStr string) (*store } func (m *mscalendar) SetDailySummaryEnabled(user *User, enable bool) (*store.DailySummaryUserSettings, error) { - err := m.Filter( - withClient, - withUserExpanded(user), - ) + err := m.Filter(withUserExpanded(user)) if err != nil { return nil, err } - dsum, err := m.Store.LoadDailySummaryUserSettings(user.MattermostUserID) - if err != nil { - return nil, err - } + dsum := user.Settings.DailySummary if dsum == nil { - dsum = &store.DailySummaryUserSettings{MattermostUserID: user.MattermostUserID} + dsum = &store.DailySummaryUserSettings{} + user.Settings.DailySummary = dsum } dsum.Enable = enable - err = m.Store.StoreDailySummaryUserSettings(dsum) + err = m.Store.StoreUser(user.User) if err != nil { return nil, err } @@ -103,11 +99,11 @@ func (m *mscalendar) SetDailySummaryEnabled(user *User, enable bool) (*store.Dai } func (m *mscalendar) ProcessAllDailySummary(now time.Time) error { - dsumIndex, err := m.Store.LoadDailySummaryIndex() + userIndex, err := m.Store.LoadUserIndex() if err != nil { return err } - if len(dsumIndex) == 0 { + if len(userIndex) == 0 { return nil } @@ -117,10 +113,18 @@ func (m *mscalendar) ProcessAllDailySummary(now time.Time) error { } requests := []*remote.ViewCalendarParams{} - for _, dsum := range dsumIndex { + byRemoteID := map[string]*store.User{} + for _, user := range userIndex { + storeUser, err := m.Store.LoadUser(user.MattermostUserID) + if err != nil { + m.Logger.Warnf("Error loading user %s for daily summary. err=%v", user.MattermostUserID, err) + continue + } + + dsum := storeUser.Settings.DailySummary shouldPost, shouldPostErr := shouldPostDailySummary(dsum, now) if shouldPostErr != nil { - m.Logger.Warnf("Error posting daily summary for user %s. err=%v", dsum.MattermostUserID, shouldPostErr) + m.Logger.Warnf("Error posting daily summary for user %s. err=%v", user.MattermostUserID, shouldPostErr) continue } if !shouldPost { @@ -129,7 +133,7 @@ func (m *mscalendar) ProcessAllDailySummary(now time.Time) error { start, end := getTodayHoursForTimezone(now, dsum.Timezone) req := &remote.ViewCalendarParams{ - RemoteUserID: dsum.RemoteID, + RemoteUserID: storeUser.Remote.ID, StartTime: start, EndTime: end, } @@ -141,36 +145,26 @@ func (m *mscalendar) ProcessAllDailySummary(now time.Time) error { return err } - mappedPostTimes := map[string]string{} - byRemoteID := dsumIndex.ByRemoteID() for _, res := range responses { - dsum := byRemoteID[res.RemoteUserID] + user := byRemoteID[res.RemoteUserID] if res.Error != nil { - m.Logger.Warnf("Error rendering user %s calendar. err=%s %s", dsum.MattermostUserID, res.Error.Code, res.Error.Message) + m.Logger.Warnf("Error rendering user %s calendar. err=%s %s", user.MattermostUserID, res.Error.Code, res.Error.Message) } + dsum := user.Settings.DailySummary postStr, err := views.RenderCalendarView(res.Events, dsum.Timezone) if err != nil { - m.Logger.Warnf("Error rendering user %s calendar. err=%v", dsum.MattermostUserID, err) + m.Logger.Warnf("Error rendering user %s calendar. err=%v", user.MattermostUserID, err) } - m.Poster.DM(dsum.MattermostUserID, postStr) - mappedPostTimes[dsum.MattermostUserID] = time.Now().Format(time.RFC3339) - } - - modErr := m.Store.ModifyDailySummaryIndex(func(dsumIndex store.DailySummaryIndex) (store.DailySummaryIndex, error) { - for _, setting := range dsumIndex { - postTime, ok := mappedPostTimes[setting.MattermostUserID] - if ok { - setting.LastPostTime = postTime - } + m.Poster.DM(user.MattermostUserID, postStr) + dsum.LastPostTime = time.Now().Format(time.RFC3339) + err = m.Store.StoreUser(user) + if err != nil { + m.Logger.Warnf("Error storing daily summary LastPostTime for user %s. err=%v", user.MattermostUserID, err) } - return dsumIndex, nil - }) - if modErr != nil { - return modErr } - m.Logger.Infof("Processed daily summary for %d users", len(mappedPostTimes)) + m.Logger.Infof("Processed daily summary for %d users", len(responses)) return nil } diff --git a/server/mscalendar/user.go b/server/mscalendar/user.go index 1b868f26..cd194760 100644 --- a/server/mscalendar/user.go +++ b/server/mscalendar/user.go @@ -152,11 +152,6 @@ func (m *mscalendar) DisconnectUser(mattermostUserID string) error { return err } - err = m.Store.DeleteDailySummaryUserSettings(mattermostUserID) - if err != nil { - return err - } - return nil } diff --git a/server/store/daily_summary_store.go b/server/store/daily_summary_store.go deleted file mode 100644 index d1eac5fd..00000000 --- a/server/store/daily_summary_store.go +++ /dev/null @@ -1,114 +0,0 @@ -// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved. -// See License for license information. - -package store - -import ( - "encoding/json" - - "github.com/mattermost/mattermost-plugin-mscalendar/server/utils/kvstore" -) - -type DailySummaryStore interface { - LoadDailySummaryIndex() (DailySummaryIndex, error) - LoadDailySummaryUserSettings(mattermostUserID string) (*DailySummaryUserSettings, error) - StoreDailySummaryUserSettings(dsum *DailySummaryUserSettings) error - DeleteDailySummaryUserSettings(mattermostUserID string) error - ModifyDailySummaryIndex(modify func(dsumIndex DailySummaryIndex) (DailySummaryIndex, error)) error -} - -type DailySummaryUserSettings struct { - MattermostUserID string `json:"mm_id"` - RemoteID string `json:"remote_id"` - Enable bool `json:"enable"` - PostTime string `json:"post_time"` // Kitchen format, i.e. 8:30AM - Timezone string `json:"tz"` // Timezone in MSCal when PostTime is set/updated - LastPostTime string `json:"last_post_time"` -} - -type DailySummaryIndex []*DailySummaryUserSettings - -func (s *pluginStore) LoadDailySummaryIndex() (DailySummaryIndex, error) { - dsumIndex := DailySummaryIndex{} - err := kvstore.LoadJSON(s.dailySummaryKV, "", &dsumIndex) - if err != nil && err.Error() != "not found" { - return nil, err - } - return dsumIndex, nil -} - -func (s *pluginStore) LoadDailySummaryUserSettings(mattermostUserID string) (*DailySummaryUserSettings, error) { - index, err := s.LoadDailySummaryIndex() - if err != nil { - return nil, err - } - if index == nil { - return nil, nil - } - - for _, dsum := range index { - if dsum.MattermostUserID == mattermostUserID { - return dsum, nil - } - } - return nil, nil -} - -func (s *pluginStore) StoreDailySummaryUserSettings(toStore *DailySummaryUserSettings) error { - return s.ModifyDailySummaryIndex(func(dsumIndex DailySummaryIndex) (DailySummaryIndex, error) { - for i, dsum := range dsumIndex { - if dsum.MattermostUserID == toStore.MattermostUserID { - result := append(dsumIndex[:i], toStore) - return append(result, dsumIndex[i+1:]...), nil - } - } - - return append(dsumIndex, toStore), nil - }) -} - -func (s *pluginStore) DeleteDailySummaryUserSettings(mattermostUserID string) error { - return s.ModifyDailySummaryIndex(func(dsumIndex DailySummaryIndex) (DailySummaryIndex, error) { - for i, u := range dsumIndex { - if u.MattermostUserID == mattermostUserID { - return append(dsumIndex[:i], dsumIndex[i+1:]...), nil - } - } - return dsumIndex, nil - }) -} - -func (s *pluginStore) ModifyDailySummaryIndex(modify func(dsumIndex DailySummaryIndex) (DailySummaryIndex, error)) error { - return kvstore.AtomicModify(s.dailySummaryKV, "", func(initialBytes []byte, storeErr error) ([]byte, error) { - if storeErr != nil && storeErr != ErrNotFound { - return nil, storeErr - } - - storedSettings := DailySummaryIndex{} - if len(initialBytes) > 0 { - err := json.Unmarshal(initialBytes, &storedSettings) - if err != nil { - return nil, err - } - } - - updated, err := modify(storedSettings) - if err != nil { - return nil, err - } - b, err := json.Marshal(updated) - if err != nil { - return nil, err - } - - return b, nil - }) -} - -func (index DailySummaryIndex) ByRemoteID() map[string]*DailySummaryUserSettings { - result := map[string]*DailySummaryUserSettings{} - for _, u := range index { - result[u.RemoteID] = u - } - return result -} diff --git a/server/store/mock_store/mock_store.go b/server/store/mock_store/mock_store.go index 7a9fdea5..16996145 100644 --- a/server/store/mock_store/mock_store.go +++ b/server/store/mock_store/mock_store.go @@ -47,20 +47,6 @@ func (mr *MockStoreMockRecorder) DeleteCurrentStep(arg0 interface{}) *gomock.Cal return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteCurrentStep", reflect.TypeOf((*MockStore)(nil).DeleteCurrentStep), arg0) } -// DeleteDailySummaryUserSettings mocks base method -func (m *MockStore) DeleteDailySummaryUserSettings(arg0 string) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteDailySummaryUserSettings", arg0) - ret0, _ := ret[0].(error) - return ret0 -} - -// DeleteDailySummaryUserSettings indicates an expected call of DeleteDailySummaryUserSettings -func (mr *MockStoreMockRecorder) DeleteDailySummaryUserSettings(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteDailySummaryUserSettings", reflect.TypeOf((*MockStore)(nil).DeleteDailySummaryUserSettings), arg0) -} - // DeletePanelPostID mocks base method func (m *MockStore) DeletePanelPostID(arg0 string) error { m.ctrl.T.Helper() @@ -206,36 +192,6 @@ func (mr *MockStoreMockRecorder) GetSetting(arg0, arg1 interface{}) *gomock.Call return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetSetting", reflect.TypeOf((*MockStore)(nil).GetSetting), arg0, arg1) } -// LoadDailySummaryIndex mocks base method -func (m *MockStore) LoadDailySummaryIndex() (store.DailySummaryIndex, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "LoadDailySummaryIndex") - ret0, _ := ret[0].(store.DailySummaryIndex) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// LoadDailySummaryIndex indicates an expected call of LoadDailySummaryIndex -func (mr *MockStoreMockRecorder) LoadDailySummaryIndex() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LoadDailySummaryIndex", reflect.TypeOf((*MockStore)(nil).LoadDailySummaryIndex)) -} - -// LoadDailySummaryUserSettings mocks base method -func (m *MockStore) LoadDailySummaryUserSettings(arg0 string) (*store.DailySummaryUserSettings, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "LoadDailySummaryUserSettings", arg0) - ret0, _ := ret[0].(*store.DailySummaryUserSettings) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// LoadDailySummaryUserSettings indicates an expected call of LoadDailySummaryUserSettings -func (mr *MockStoreMockRecorder) LoadDailySummaryUserSettings(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LoadDailySummaryUserSettings", reflect.TypeOf((*MockStore)(nil).LoadDailySummaryUserSettings), arg0) -} - // LoadMattermostUserID mocks base method func (m *MockStore) LoadMattermostUserID(arg0 string) (string, error) { m.ctrl.T.Helper() @@ -341,20 +297,6 @@ func (mr *MockStoreMockRecorder) LoadUserWelcomePost(arg0 interface{}) *gomock.C return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LoadUserWelcomePost", reflect.TypeOf((*MockStore)(nil).LoadUserWelcomePost), arg0) } -// ModifyDailySummaryIndex mocks base method -func (m *MockStore) ModifyDailySummaryIndex(arg0 func(store.DailySummaryIndex) (store.DailySummaryIndex, error)) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ModifyDailySummaryIndex", arg0) - ret0, _ := ret[0].(error) - return ret0 -} - -// ModifyDailySummaryIndex indicates an expected call of ModifyDailySummaryIndex -func (mr *MockStoreMockRecorder) ModifyDailySummaryIndex(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ModifyDailySummaryIndex", reflect.TypeOf((*MockStore)(nil).ModifyDailySummaryIndex), arg0) -} - // ModifyUserIndex mocks base method func (m *MockStore) ModifyUserIndex(arg0 func(store.UserIndex) (store.UserIndex, error)) error { m.ctrl.T.Helper() @@ -453,20 +395,6 @@ func (mr *MockStoreMockRecorder) SetSetting(arg0, arg1, arg2 interface{}) *gomoc return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetSetting", reflect.TypeOf((*MockStore)(nil).SetSetting), arg0, arg1, arg2) } -// StoreDailySummaryUserSettings mocks base method -func (m *MockStore) StoreDailySummaryUserSettings(arg0 *store.DailySummaryUserSettings) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "StoreDailySummaryUserSettings", arg0) - ret0, _ := ret[0].(error) - return ret0 -} - -// StoreDailySummaryUserSettings indicates an expected call of StoreDailySummaryUserSettings -func (mr *MockStoreMockRecorder) StoreDailySummaryUserSettings(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "StoreDailySummaryUserSettings", reflect.TypeOf((*MockStore)(nil).StoreDailySummaryUserSettings), arg0) -} - // StoreOAuth2State mocks base method func (m *MockStore) StoreOAuth2State(arg0 string) error { m.ctrl.T.Helper() diff --git a/server/store/setting_store.go b/server/store/setting_store.go index e269f863..d6de758b 100644 --- a/server/store/setting_store.go +++ b/server/store/setting_store.go @@ -74,17 +74,22 @@ func (s *pluginStore) GetSetting(userID, settingID string) (interface{}, error) case ReceiveRemindersSettingID: return user.Settings.ReceiveReminders, nil case DailySummarySettingID: - return s.LoadDailySummaryUserSettings(userID) + dsum L= user.Settings.DailySummary + if dsum == nil { + return nil, errors.New("Daily summary settings not found") + } + return dsum, nil default: return nil, fmt.Errorf("setting %s not found", settingID) } } func (s *pluginStore) updateDailySummarySettingForUser(userID string, value interface{}) error { - dsum, err := s.LoadDailySummaryUserSettings(userID) + user, err := s.LoadUser(userID) if err != nil { return err } + dsum := user.Settings.DailySummary stringValue := value.(string) splittedValue := strings.Split(stringValue, " ") @@ -97,10 +102,9 @@ func (s *pluginStore) updateDailySummarySettingForUser(userID string, value inte } dsum = &DailySummaryUserSettings{ - MattermostUserID: userID, - PostTime: timeStr, - Timezone: timezone, - Enable: splittedValue[0] == "true", + PostTime: timeStr, + Timezone: timezone, + Enable: splittedValue[0] == "true", } } else { switch splittedValue[0] { @@ -114,7 +118,7 @@ func (s *pluginStore) updateDailySummarySettingForUser(userID string, value inte } } - err = s.StoreDailySummaryUserSettings(dsum) + err = s.StoreUser(user) if err != nil { return err } diff --git a/server/store/store.go b/server/store/store.go index 4a474645..25900d5a 100644 --- a/server/store/store.go +++ b/server/store/store.go @@ -23,7 +23,6 @@ const ( EventKeyPrefix = "ev_" WelcomeKeyPrefix = "welcome_" SettingsPanelPrefix = "settings_panel_" - DailySummaryKeyPrefix = "dsum_" ) const OAuth2KeyExpiration = 15 * time.Minute @@ -39,7 +38,6 @@ type Store interface { flow.FlowStore settingspanel.SettingStore settingspanel.PanelStore - DailySummaryStore } type pluginStore struct { @@ -52,7 +50,6 @@ type pluginStore struct { eventKV kvstore.KVStore welcomeIndexKV kvstore.KVStore settingsPanelKV kvstore.KVStore - dailySummaryKV kvstore.KVStore Logger bot.Logger } @@ -65,7 +62,6 @@ func NewPluginStore(api plugin.API, logger bot.Logger) Store { mattermostUserIDKV: kvstore.NewHashedKeyStore(basicKV, MattermostUserIDKeyPrefix), subscriptionKV: kvstore.NewHashedKeyStore(basicKV, SubscriptionKeyPrefix), eventKV: kvstore.NewHashedKeyStore(basicKV, EventKeyPrefix), - dailySummaryKV: kvstore.NewHashedKeyStore(basicKV, DailySummaryKeyPrefix), oauth2KV: kvstore.NewHashedKeyStore(kvstore.NewOneTimePluginStore(api, OAuth2KeyExpiration), OAuth2KeyPrefix), welcomeIndexKV: kvstore.NewHashedKeyStore(basicKV, WelcomeKeyPrefix), settingsPanelKV: kvstore.NewHashedKeyStore(basicKV, SettingsPanelPrefix), diff --git a/server/store/user_store.go b/server/store/user_store.go index 18ab8623..651157a6 100644 --- a/server/store/user_store.go +++ b/server/store/user_store.go @@ -49,6 +49,14 @@ type Settings struct { GetConfirmation bool ReceiveReminders bool ReceiveNotificationsDuringMeeting bool + DailySummary *DailySummaryUserSettings +} + +type DailySummaryUserSettings struct { + Enable bool `json:"enable"` + PostTime string `json:"post_time"` // Kitchen format, i.e. 8:30AM + Timezone string `json:"tz"` // Timezone in MSCal when PostTime is set/updated + LastPostTime string `json:"last_post_time"` } type WelcomeFlowStatus struct { From 3433380d52f9f1282bb8a98fc234f32649818d95 Mon Sep 17 00:00:00 2001 From: Michael Kochell <6913320+mickmister@users.noreply.github.com> Date: Fri, 22 May 2020 03:32:17 -0400 Subject: [PATCH 2/5] fix tests --- server/mscalendar/daily_summary.go | 1 + server/mscalendar/daily_summary_test.go | 103 ++++++++++++++++-------- server/store/setting_store.go | 3 +- 3 files changed, 71 insertions(+), 36 deletions(-) diff --git a/server/mscalendar/daily_summary.go b/server/mscalendar/daily_summary.go index 2e935a59..64098ea1 100644 --- a/server/mscalendar/daily_summary.go +++ b/server/mscalendar/daily_summary.go @@ -120,6 +120,7 @@ func (m *mscalendar) ProcessAllDailySummary(now time.Time) error { m.Logger.Warnf("Error loading user %s for daily summary. err=%v", user.MattermostUserID, err) continue } + byRemoteID[storeUser.Remote.ID] = storeUser dsum := storeUser.Settings.DailySummary shouldPost, shouldPostErr := shouldPostDailySummary(dsum, now) diff --git a/server/mscalendar/daily_summary_test.go b/server/mscalendar/daily_summary_test.go index c39c8753..b29e5974 100644 --- a/server/mscalendar/daily_summary_test.go +++ b/server/mscalendar/daily_summary_test.go @@ -27,7 +27,7 @@ func TestProcessAllDailySummary(t *testing.T) { err: "index store error", runAssertions: func(deps *Dependencies, client remote.Client) { s := deps.Store.(*mock_store.MockStore) - s.EXPECT().LoadDailySummaryIndex().Return(store.DailySummaryIndex{}, errors.New("index store error")) + s.EXPECT().LoadUserIndex().Return(nil, errors.New("index store error")) }, }, { @@ -35,7 +35,7 @@ func TestProcessAllDailySummary(t *testing.T) { err: "", runAssertions: func(deps *Dependencies, client remote.Client) { s := deps.Store.(*mock_store.MockStore) - s.EXPECT().LoadDailySummaryIndex().Return(store.DailySummaryIndex{}, nil) + s.EXPECT().LoadUserIndex().Return(store.UserIndex{}, nil) }, }, { @@ -43,16 +43,23 @@ func TestProcessAllDailySummary(t *testing.T) { err: "error fetching events", runAssertions: func(deps *Dependencies, client remote.Client) { s := deps.Store.(*mock_store.MockStore) - s.EXPECT().LoadDailySummaryIndex().Return(store.DailySummaryIndex{ - &store.DailySummaryUserSettings{ - MattermostUserID: "user1_mm_id", - RemoteID: "user1_remote_id", - Enable: true, - PostTime: "9:00AM", - Timezone: "Eastern Standard Time", - LastPostTime: "", + s.EXPECT().LoadUserIndex().Return(store.UserIndex{{ + MattermostUserID: "user1_mm_id", + RemoteID: "user1_remote_id", + }}, nil) + + s.EXPECT().LoadUser("user1_mm_id").Return(&store.User{ + MattermostUserID: "user1_mm_id", + Remote: &remote.User{ID: "user1_remote_id"}, + Settings: store.Settings{ + DailySummary: &store.DailySummaryUserSettings{ + Enable: true, + PostTime: "9:00AM", + Timezone: "Eastern Standard Time", + LastPostTime: "", + }, }, - }, nil).Times(1) + }, nil) mockClient := client.(*mock_remote.MockClient) mockRemote := deps.Remote.(*mock_remote.MockRemote) @@ -66,32 +73,55 @@ func TestProcessAllDailySummary(t *testing.T) { err: "", runAssertions: func(deps *Dependencies, client remote.Client) { s := deps.Store.(*mock_store.MockStore) - s.EXPECT().LoadDailySummaryIndex().Return(store.DailySummaryIndex{ - &store.DailySummaryUserSettings{ - MattermostUserID: "user1_mm_id", - RemoteID: "user1_remote_id", - Enable: true, - PostTime: "9:00AM", - Timezone: "Eastern Standard Time", - LastPostTime: "", + s.EXPECT().LoadUserIndex().Return(store.UserIndex{{ + MattermostUserID: "user1_mm_id", + RemoteID: "user1_remote_id", + }, { + MattermostUserID: "user2_mm_id", + RemoteID: "user2_remote_id", + }, { + MattermostUserID: "user3_mm_id", + RemoteID: "user3_remote_id", + }}, nil) + + s.EXPECT().LoadUser("user1_mm_id").Return(&store.User{ + MattermostUserID: "user1_mm_id", + Remote: &remote.User{ID: "user1_remote_id"}, + Settings: store.Settings{ + DailySummary: &store.DailySummaryUserSettings{ + Enable: true, + PostTime: "9:00AM", + Timezone: "Eastern Standard Time", + LastPostTime: "", + }, }, - &store.DailySummaryUserSettings{ - MattermostUserID: "user2_mm_id", - RemoteID: "user2_remote_id", - Enable: true, - PostTime: "6:00AM", - Timezone: "Pacific Standard Time", - LastPostTime: "", + }, nil) + + s.EXPECT().LoadUser("user2_mm_id").Return(&store.User{ + MattermostUserID: "user2_mm_id", + Remote: &remote.User{ID: "user2_remote_id"}, + Settings: store.Settings{ + DailySummary: &store.DailySummaryUserSettings{ + Enable: true, + PostTime: "6:00AM", + Timezone: "Pacific Standard Time", + LastPostTime: "", + }, }, - &store.DailySummaryUserSettings{ - MattermostUserID: "user3_mm_id", - RemoteID: "user3_remote_id", - Enable: true, - PostTime: "10:00AM", // should not receive summary - Timezone: "Pacific Standard Time", - LastPostTime: "", + }, nil) + + s.EXPECT().LoadUser("user3_mm_id").Return(&store.User{ + MattermostUserID: "user3_mm_id", + Remote: &remote.User{ID: "user3_remote_id"}, + Settings: store.Settings{ + DailySummary: &store.DailySummaryUserSettings{ + Enable: true, + PostTime: "10:00AM", // should not receive summary + Timezone: "Pacific Standard Time", + LastPostTime: "", + }, }, - }, nil).Times(1) + }, nil) mockClient := client.(*mock_remote.MockClient) loc, err := time.LoadLocation("MST") @@ -122,7 +152,10 @@ Wednesday February 12 | 9:00AM - 11:00AM | [The subject]() |`).Return("postID2", nil).Times(1), ) - s.EXPECT().ModifyDailySummaryIndex(gomock.Any()).Return(nil) + s.EXPECT().StoreUser(gomock.Any()).Times(2).DoAndReturn(func(u *store.User) error { + require.NotEmpty(t, u.Settings.DailySummary.LastPostTime) + return nil + }) mockLogger := deps.Logger.(*mock_bot.MockLogger) mockLogger.EXPECT().Infof("Processed daily summary for %d users", 2) diff --git a/server/store/setting_store.go b/server/store/setting_store.go index d6de758b..3062f576 100644 --- a/server/store/setting_store.go +++ b/server/store/setting_store.go @@ -1,6 +1,7 @@ package store import ( + "errors" "fmt" "strings" ) @@ -74,7 +75,7 @@ func (s *pluginStore) GetSetting(userID, settingID string) (interface{}, error) case ReceiveRemindersSettingID: return user.Settings.ReceiveReminders, nil case DailySummarySettingID: - dsum L= user.Settings.DailySummary + dsum := user.Settings.DailySummary if dsum == nil { return nil, errors.New("Daily summary settings not found") } From 5cc0442eeef77ec738f3251166a3106eccf7c3b8 Mon Sep 17 00:00:00 2001 From: Michael Kochell <6913320+mickmister@users.noreply.github.com> Date: Fri, 22 May 2020 04:05:32 -0400 Subject: [PATCH 3/5] Fix variable shadow --- server/mscalendar/daily_summary.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/mscalendar/daily_summary.go b/server/mscalendar/daily_summary.go index 64098ea1..8350b354 100644 --- a/server/mscalendar/daily_summary.go +++ b/server/mscalendar/daily_summary.go @@ -115,9 +115,9 @@ func (m *mscalendar) ProcessAllDailySummary(now time.Time) error { requests := []*remote.ViewCalendarParams{} byRemoteID := map[string]*store.User{} for _, user := range userIndex { - storeUser, err := m.Store.LoadUser(user.MattermostUserID) - if err != nil { - m.Logger.Warnf("Error loading user %s for daily summary. err=%v", user.MattermostUserID, err) + storeUser, storeErr := m.Store.LoadUser(user.MattermostUserID) + if storeErr != nil { + m.Logger.Warnf("Error loading user %s for daily summary. err=%v", user.MattermostUserID, storeErr) continue } byRemoteID[storeUser.Remote.ID] = storeUser From 2a04a43e915af2dd4eea34f1f6843ae934184a28 Mon Sep 17 00:00:00 2001 From: Michael Kochell <6913320+mickmister@users.noreply.github.com> Date: Tue, 26 May 2020 17:51:53 -0400 Subject: [PATCH 4/5] ensure daily summary settings exist after connect --- server/mscalendar/daily_summary.go | 15 +--------- server/mscalendar/oauth2.go | 6 ++++ server/mscalendar/user.go | 21 ++++++++++++++ server/store/setting_store.go | 46 +++++++----------------------- 4 files changed, 38 insertions(+), 50 deletions(-) diff --git a/server/mscalendar/daily_summary.go b/server/mscalendar/daily_summary.go index 8350b354..4fdf2d3b 100644 --- a/server/mscalendar/daily_summary.go +++ b/server/mscalendar/daily_summary.go @@ -35,11 +35,7 @@ func (m *mscalendar) GetDailySummarySettingsForUser(user *User) (*store.DailySum return nil, err } - dsum := user.Settings.DailySummary - if dsum == nil { - return nil, errors.New("No daily summary settings found") - } - return dsum, nil + return user.Settings.DailySummary, nil } func (m *mscalendar) SetDailySummaryPostTime(user *User, timeStr string) (*store.DailySummaryUserSettings, error) { @@ -63,11 +59,6 @@ func (m *mscalendar) SetDailySummaryPostTime(user *User, timeStr string) (*store } dsum := user.Settings.DailySummary - if dsum == nil { - dsum = &store.DailySummaryUserSettings{} - user.Settings.DailySummary = dsum - } - dsum.PostTime = timeStr dsum.Timezone = timezone @@ -85,10 +76,6 @@ func (m *mscalendar) SetDailySummaryEnabled(user *User, enable bool) (*store.Dai } dsum := user.Settings.DailySummary - if dsum == nil { - dsum = &store.DailySummaryUserSettings{} - user.Settings.DailySummary = dsum - } dsum.Enable = enable err = m.Store.StoreUser(user.User) diff --git a/server/mscalendar/oauth2.go b/server/mscalendar/oauth2.go index a2df8627..a86c0d84 100644 --- a/server/mscalendar/oauth2.go +++ b/server/mscalendar/oauth2.go @@ -108,6 +108,12 @@ func (app *oauth2App) CompleteOAuth2(authedUserID, code, state string) error { return err } + mscal := New(app.Env, mattermostUserID) + err = mscal.InitUser(mattermostUserID) + if err != nil { + return err + } + app.Welcomer.AfterSuccessfullyConnect(mattermostUserID, me.Mail) return nil diff --git a/server/mscalendar/user.go b/server/mscalendar/user.go index cd194760..a4bdd8f7 100644 --- a/server/mscalendar/user.go +++ b/server/mscalendar/user.go @@ -21,6 +21,7 @@ type Users interface { GetRemoteUser(mattermostUserID string) (*remote.User, error) IsAuthorizedAdmin(mattermostUserID string) (bool, error) GetUserSettings(user *User) (*store.Settings, error) + InitUser(mattermostUserID string) error } type User struct { @@ -189,3 +190,23 @@ func (m *mscalendar) GetUserSettings(user *User) (*store.Settings, error) { return &user.Settings, nil } + +func (m *mscalendar) InitUser(mattermostUserID string) error { + user, err := m.Store.LoadUser(mattermostUserID) + if err != nil { + return err + } + + timezone, err := m.GetTimezoneByID(mattermostUserID) + if err != nil { + return err + } + + user.Settings.DailySummary = &store.DailySummaryUserSettings{ + PostTime: "8:00AM", + Timezone: timezone, + Enable: false, + } + + return m.Store.StoreUser(user) +} diff --git a/server/store/setting_store.go b/server/store/setting_store.go index 3062f576..0505dd52 100644 --- a/server/store/setting_store.go +++ b/server/store/setting_store.go @@ -1,7 +1,6 @@ package store import ( - "errors" "fmt" "strings" ) @@ -46,7 +45,7 @@ func (s *pluginStore) SetSetting(userID, settingID string, value interface{}) er } user.Settings.ReceiveReminders = storableValue case DailySummarySettingID: - s.updateDailySummarySettingForUser(userID, value) + s.updateDailySummarySettingForUser(user, value) default: return fmt.Errorf("setting %s not found", settingID) } @@ -76,52 +75,27 @@ func (s *pluginStore) GetSetting(userID, settingID string) (interface{}, error) return user.Settings.ReceiveReminders, nil case DailySummarySettingID: dsum := user.Settings.DailySummary - if dsum == nil { - return nil, errors.New("Daily summary settings not found") - } return dsum, nil default: return nil, fmt.Errorf("setting %s not found", settingID) } } -func (s *pluginStore) updateDailySummarySettingForUser(userID string, value interface{}) error { - user, err := s.LoadUser(userID) - if err != nil { - return err - } +func (s *pluginStore) updateDailySummarySettingForUser(user *User, value interface{}) error { dsum := user.Settings.DailySummary stringValue := value.(string) splittedValue := strings.Split(stringValue, " ") timezone := strings.Join(splittedValue[1:], " ") - if dsum == nil { - timeStr := splittedValue[0] - if splittedValue[0] == "true" || splittedValue[0] == "false" { - timeStr = "8:00AM" - } - - dsum = &DailySummaryUserSettings{ - PostTime: timeStr, - Timezone: timezone, - Enable: splittedValue[0] == "true", - } - } else { - switch splittedValue[0] { - case "true": - dsum.Enable = true - case "false": - dsum.Enable = false - default: - dsum.PostTime = splittedValue[0] - dsum.Timezone = timezone - } - } - - err = s.StoreUser(user) - if err != nil { - return err + switch splittedValue[0] { + case "true": + dsum.Enable = true + case "false": + dsum.Enable = false + default: + dsum.PostTime = splittedValue[0] + dsum.Timezone = timezone } return nil From d7e54eae7f5039adb26e0463d273b625415f8c3f Mon Sep 17 00:00:00 2001 From: Michael Kochell <6913320+mickmister@users.noreply.github.com> Date: Tue, 26 May 2020 19:00:21 -0400 Subject: [PATCH 5/5] fetch timezone after connect --- server/mscalendar/oauth2.go | 13 +++++++++---- server/mscalendar/oauth2_test.go | 9 ++++++++- server/mscalendar/user.go | 21 --------------------- 3 files changed, 17 insertions(+), 26 deletions(-) diff --git a/server/mscalendar/oauth2.go b/server/mscalendar/oauth2.go index a86c0d84..18192ef0 100644 --- a/server/mscalendar/oauth2.go +++ b/server/mscalendar/oauth2.go @@ -98,18 +98,23 @@ func (app *oauth2App) CompleteOAuth2(authedUserID, code, state string) error { OAuth2Token: tok, } - err = app.Store.StoreUser(u) + mailboxSettings, err := client.GetMailboxSettings(me.ID) if err != nil { return err } - err = app.Store.StoreUserInIndex(u) + u.Settings.DailySummary = &store.DailySummaryUserSettings{ + PostTime: "8:00AM", + Timezone: mailboxSettings.TimeZone, + Enable: false, + } + + err = app.Store.StoreUser(u) if err != nil { return err } - mscal := New(app.Env, mattermostUserID) - err = mscal.InitUser(mattermostUserID) + err = app.Store.StoreUserInIndex(u) if err != nil { return err } diff --git a/server/mscalendar/oauth2_test.go b/server/mscalendar/oauth2_test.go index 240193d5..ffe8efc1 100644 --- a/server/mscalendar/oauth2_test.go +++ b/server/mscalendar/oauth2_test.go @@ -337,8 +337,15 @@ func statusOKGraphAPIResponder() { }` meResponder := httpmock.NewStringResponder(http.StatusOK, meResponse) - httpmock.RegisterResponder("GET", meRequestURL, meResponder) + + mailSettingsURL := "https://graph.microsoft.com/v1.0/users/user-remote-id/mailboxSettings" + mailSettingsResponse := `{ + "timeZone": "Pacific Standard Time" + }` + mailSettingsResponder := httpmock.NewStringResponder(http.StatusOK, mailSettingsResponse) + + httpmock.RegisterResponder("GET", mailSettingsURL, mailSettingsResponder) } func newHTTPRequest(mattermostUserID, rawQuery string) *http.Request { diff --git a/server/mscalendar/user.go b/server/mscalendar/user.go index a4bdd8f7..cd194760 100644 --- a/server/mscalendar/user.go +++ b/server/mscalendar/user.go @@ -21,7 +21,6 @@ type Users interface { GetRemoteUser(mattermostUserID string) (*remote.User, error) IsAuthorizedAdmin(mattermostUserID string) (bool, error) GetUserSettings(user *User) (*store.Settings, error) - InitUser(mattermostUserID string) error } type User struct { @@ -190,23 +189,3 @@ func (m *mscalendar) GetUserSettings(user *User) (*store.Settings, error) { return &user.Settings, nil } - -func (m *mscalendar) InitUser(mattermostUserID string) error { - user, err := m.Store.LoadUser(mattermostUserID) - if err != nil { - return err - } - - timezone, err := m.GetTimezoneByID(mattermostUserID) - if err != nil { - return err - } - - user.Settings.DailySummary = &store.DailySummaryUserSettings{ - PostTime: "8:00AM", - Timezone: timezone, - Enable: false, - } - - return m.Store.StoreUser(user) -}