Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GCAL] Enable notifications and reminders when a superuser token is not supported #272

Merged
merged 41 commits into from
Aug 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
4d3a2bb
nil control
fmartingr Jul 25, 2023
fe6fe75
use subscriptions default ttl
fmartingr Jul 25, 2023
7634f2c
return specific error when no superuser token can be used
fmartingr Jul 25, 2023
9dc21a2
notification formatting logic to other file
fmartingr Jul 25, 2023
a436769
Client.GetEventsBetweenDates
fmartingr Jul 25, 2023
18767a3
typo: import
fmartingr Jul 25, 2023
8de4223
shorter command name :)
fmartingr Jul 25, 2023
af611a9
availability without super user token
fmartingr Jul 25, 2023
24c2c64
delete store subs only if we successfuly do on remote
fmartingr Jul 25, 2023
0ecad3f
comment
fmartingr Jul 25, 2023
165948c
ignore missing subs
fmartingr Jul 25, 2023
2e6c571
update sync method
fmartingr Jul 25, 2023
cc88b2e
allow switching acting user on the fly
fmartingr Jul 25, 2023
0def44a
daily summary without super user token
fmartingr Jul 25, 2023
c5f0e1c
formatted imports
fmartingr Jul 25, 2023
0b7bce2
calendar -> calendarEvents
fmartingr Jul 25, 2023
100497a
processAllDailySummaryByUser
fmartingr Jul 25, 2023
d90fb5e
revert silenced error under notifications
fmartingr Jul 25, 2023
4345953
msgraph GetEventsBetweenDates
fmartingr Jul 25, 2023
57b30f9
Update server/mscalendar/daily_summary.go
fmartingr Jul 26, 2023
92de736
[GCAL] Create event logic (#269)
fmartingr Jul 25, 2023
3e3f43d
Client.GetEventsBetweenDates
fmartingr Jul 25, 2023
5c05a74
fix: store last post time
fmartingr Jul 26, 2023
5dbc701
refactored availability logic
fmartingr Jul 26, 2023
19abc70
availability test
fmartingr Jul 26, 2023
c3336e9
refactored daily summary logic
fmartingr Jul 26, 2023
e4f73fd
test: availability
fmartingr Jul 26, 2023
9951416
daily summary tests (wip)
fmartingr Jul 26, 2023
a92d545
daily summary test
fmartingr Jul 27, 2023
c090410
fix: merge duplications
fmartingr Jul 27, 2023
ecca739
slack attachment for notifications
fmartingr Jul 27, 2023
6f2b1d7
lint and test
fmartingr Jul 27, 2023
351730c
goimports
fmartingr Jul 27, 2023
7c3b63d
Update server/mscalendar/availability.go
fmartingr Jul 31, 2023
d8745f6
remove logges
fmartingr Jul 31, 2023
58e7b9f
nullify client when changing acting user
fmartingr Jul 31, 2023
9aac20f
move withActiveUser filter to fenced code outside of general method
fmartingr Jul 31, 2023
c9378ce
tests: handling more scenarios
fmartingr Jul 31, 2023
b139ac5
Fixed test after bugfix
fmartingr Jul 31, 2023
08b039e
fixed test assertions
fmartingr Jul 31, 2023
b240b85
allow engine copy to perform safe state mutations
fmartingr Jul 31, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion server/command/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ func (c *Command) Handle() (string, bool, error) {
handler = c.requireConnectedUser(c.findMeetings)
case "showcals":
handler = c.requireConnectedUser(c.showCalendars)
case "availability":
case "avail":
fmartingr marked this conversation as resolved.
Show resolved Hide resolved
handler = c.requireConnectedUser(c.requireAdminUser(c.debugAvailability))
case "settings":
handler = c.requireConnectedUser(c.settings)
Expand Down
2 changes: 1 addition & 1 deletion server/command/view_calendar.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
"github.com/mattermost/mattermost-plugin-mscalendar/server/mscalendar/views"
)

func (c *Command) viewCalendar(parameters ...string) (string, bool, error) {
func (c *Command) viewCalendar(_ ...string) (string, bool, error) {
tz, err := c.MSCalendar.GetTimezone(c.user())
if err != nil {
return "Error: No timezone found", false, err
Expand Down
121 changes: 100 additions & 21 deletions server/mscalendar/availability.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/mattermost/mattermost-plugin-mscalendar/server/remote"
"github.com/mattermost/mattermost-plugin-mscalendar/server/store"
"github.com/mattermost/mattermost-plugin-mscalendar/server/utils"
"github.com/mattermost/mattermost-plugin-mscalendar/server/utils/bot"
)

const (
Expand All @@ -28,6 +29,10 @@ const (
logTruncateLimit = 5
)

var (
errNoUsersNeedToBeSynced = errors.New("no users need to be synced")
)

type StatusSyncJobSummary struct {
NumberOfUsersFailedStatusChanged int
NumberOfUsersStatusChanged int
Expand All @@ -46,15 +51,17 @@ func (m *mscalendar) Sync(mattermostUserID string) (string, *StatusSyncJobSummar
return "", nil, err
}

return m.syncUsers(store.UserIndex{user})
}
userIndex := store.UserIndex{user}

func (m *mscalendar) SyncAll() (string, *StatusSyncJobSummary, error) {
err := m.Filter(withSuperuserClient)
if err != nil {
err = m.Filter(withSuperuserClient)
if err != nil && !errors.Is(err, remote.ErrSuperUserClientNotSupported) {
return "", &StatusSyncJobSummary{}, errors.Wrap(err, "not able to filter the super user client")
}

return m.syncUsers(userIndex, errors.Is(err, remote.ErrSuperUserClientNotSupported))
}

func (m *mscalendar) SyncAll() (string, *StatusSyncJobSummary, error) {
userIndex, err := m.Store.LoadUserIndex()
if err != nil {
if err.Error() == "not found" {
Expand All @@ -63,18 +70,29 @@ func (m *mscalendar) SyncAll() (string, *StatusSyncJobSummary, error) {
return "", &StatusSyncJobSummary{}, errors.Wrap(err, "not able to load the users from user index")
}

return m.syncUsers(userIndex)
}
err = m.Filter(withSuperuserClient)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible this could have side effects since this mutates state, and thus affects functions like syncUsers. From a quick look, it seems that it won't have issues though.

What's weird is that the only client used in that path is in GetCalendarViews, which is setting its own user-level client for batch requests. Not sure how that is working this way:

func (m *mscalendar) GetCalendarViews(users []*store.User) ([]*remote.ViewCalendarResponse, error) {
err := m.Filter(withClient)

if err != nil && !errors.Is(err, remote.ErrSuperUserClientNotSupported) {
return "", &StatusSyncJobSummary{}, errors.Wrap(err, "not able to filter the super user client")
}

func (m *mscalendar) syncUsers(userIndex store.UserIndex) (string, *StatusSyncJobSummary, error) {
syncJobSummary := &StatusSyncJobSummary{}
if len(userIndex) == 0 {
return "No connected users found", syncJobSummary, nil
result, jobSummary, err := m.syncUsers(userIndex, errors.Is(err, remote.ErrSuperUserClientNotSupported))
if result != "" && err != nil {
return result, jobSummary, nil
}
syncJobSummary.NumberOfUsersProcessed = len(userIndex)

return result, jobSummary, err
}

// retrieveUsersToSync retrieves the users and their calendar data to sync up and send notifications
// The parameter fetchIndividually determines if the calendar data should be fetched while we loop the
// users (using individual credentials) or on a batch after the loop.
func (m *mscalendar) retrieveUsersToSync(userIndex store.UserIndex, syncJobSummary *StatusSyncJobSummary, fetchIndividually bool) ([]*store.User, []*remote.ViewCalendarResponse, error) {
start := time.Now().UTC()
end := time.Now().UTC().Add(calendarViewTimeWindowSize)

numberOfLogs := 0
users := []*store.User{}
calendarViews := []*remote.ViewCalendarResponse{}
for _, u := range userIndex {
// TODO fetch users from kvstore in batches, and process in batches instead of all at once
user, err := m.Store.LoadUser(u.MattermostUserID)
Expand All @@ -90,20 +108,63 @@ func (m *mscalendar) syncUsers(userIndex store.UserIndex) (string, *StatusSyncJo
// In case of error in loading, skip this user and continue with the next user
continue
}
if user.Settings.UpdateStatus || user.Settings.ReceiveReminders {
users = append(users, user)

// If user does not have the proper features enabled, just go to the next one
if !(user.Settings.UpdateStatus || user.Settings.ReceiveReminders) {
continue
}

users = append(users, user)

if fetchIndividually {
engine, err := m.FilterCopy(withActingUser(user.MattermostUserID))
if err != nil {
m.Logger.Warnf("Not able to enable active user %s from user index. err=%v", user.MattermostUserID, err)
continue
}

calendarUser := newUserFromStoredUser(user)
calendarEvents, err := engine.GetCalendarEvents(calendarUser, start, end)
if err != nil {
syncJobSummary.NumberOfUsersFailedStatusChanged++
m.Logger.With(bot.LogContext{
"user": u.MattermostUserID,
"err": err,
}).Errorf("error getting calendar events")
continue
}
calendarViews = append(calendarViews, calendarEvents)
}
}
if len(users) == 0 {
return "No users need to be synced", syncJobSummary, nil
return users, calendarViews, errNoUsersNeedToBeSynced
}

calendarViews, err := m.GetCalendarViews(users)
if err != nil {
return "", syncJobSummary, errors.Wrap(err, "not able to get calendar views for connected users")
if !fetchIndividually {
var err error
calendarViews, err = m.GetCalendarViews(users)
if err != nil {
return users, calendarViews, errors.Wrap(err, "not able to get calendar views for connected users")
}
}

if len(calendarViews) == 0 {
return "No calendar views found", syncJobSummary, nil
return users, calendarViews, fmt.Errorf("no calendar views found")
}

return users, calendarViews, nil
}

func (m *mscalendar) syncUsers(userIndex store.UserIndex, fetchIndividually bool) (string, *StatusSyncJobSummary, error) {
syncJobSummary := &StatusSyncJobSummary{}
if len(userIndex) == 0 {
return "No connected users found", syncJobSummary, nil
}
syncJobSummary.NumberOfUsersProcessed = len(userIndex)

users, calendarViews, err := m.retrieveUsersToSync(userIndex, syncJobSummary, fetchIndividually)
if err != nil {
return err.Error(), syncJobSummary, errors.Wrapf(err, "error retrieving users to sync (individually=%v)", fetchIndividually)
}

m.deliverReminders(users, calendarViews)
Expand Down Expand Up @@ -385,6 +446,23 @@ func (m *mscalendar) setStatusOrAskUser(user *store.User, currentStatus *model.S
return nil
}

func (m *mscalendar) GetCalendarEvents(user *User, start, end time.Time) (*remote.ViewCalendarResponse, error) {
err := m.Filter(withClient)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see an opportunity to make this code less error-prone? It concerns me that it's easy to make an error based on where this call is placed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the intention when I removed the withActingUser from this method. Now we can call this one without problems from other places since the new withActingUser only gets called from the fenced code in the "main" calls.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but what can we do to avoid needing to have withActingUser in the fenced code? Just want to make sure we don't run into something similar later, especially if it's something not caught by a unit test

if err != nil {
return nil, errors.Wrap(err, "errror withClient")
}

events, err := m.client.GetEventsBetweenDates(user.Remote.ID, start, end)
if err != nil {
return nil, errors.Wrapf(err, "error getting events for user %s", user.MattermostUserID)
}

return &remote.ViewCalendarResponse{
RemoteUserID: user.Remote.ID,
Events: events,
}, nil
}

func (m *mscalendar) GetCalendarViews(users []*store.User) ([]*remote.ViewCalendarResponse, error) {
err := m.Filter(withClient)
if err != nil {
Expand Down Expand Up @@ -427,12 +505,13 @@ func (m *mscalendar) notifyUpcomingEvents(mattermostUserID string, events []*rem
}
}

message, err := views.RenderUpcomingEvent(event, timezone)
_, attachment, err := views.RenderUpcomingEventAsAttachment(event, timezone)
if err != nil {
m.Logger.Warnf("notifyUpcomingEvent error rendering schedule item. err=%v", err)
continue
}
_, err = m.Poster.DM(mattermostUserID, message)

_, err = m.Poster.DMWithAttachments(mattermostUserID, attachment)
if err != nil {
m.Logger.Warnf("notifyUpcomingEvents error creating DM. err=%v", err)
continue
Expand Down
Loading