diff --git a/calendar/api/post_action.go b/calendar/api/post_action.go index 237c7690..ce326f0e 100644 --- a/calendar/api/post_action.go +++ b/calendar/api/post_action.go @@ -262,7 +262,6 @@ func getEventInfo(ctx map[string]interface{}) (string, error) { return views.RenderEventWillStartLine(subject, weblink, startTime), nil } -// REVIEW: mscalendar http status logic func isAcceptedError(err error) bool { return strings.Contains(err.Error(), "202 Accepted") } diff --git a/calendar/command/create_event.go b/calendar/command/create_event.go index c8a798d9..211d0272 100644 --- a/calendar/command/create_event.go +++ b/calendar/command/create_event.go @@ -13,8 +13,6 @@ import ( "github.com/mattermost/mattermost-plugin-mscalendar/calendar/utils" ) -// REVIEW: No autocomplete for this command - func getCreateEventFlagSet() *flag.FlagSet { flagSet := flag.NewFlagSet("create", flag.ContinueOnError) flagSet.Bool("help", false, "show help") diff --git a/calendar/command/find_meeting_times.go b/calendar/command/find_meeting_times.go index 0ea9c81a..abafd19b 100644 --- a/calendar/command/find_meeting_times.go +++ b/calendar/command/find_meeting_times.go @@ -21,7 +21,6 @@ func (c *Command) findMeetings(parameters ...string) (string, bool, error) { return "", false, fmt.Errorf("error in parameter %s", parameter) } t, email := s[0], s[1] - // REVIEW: very small struct being used to fetch meeting times. FindMeetingTimesParameters is a large struct, but only attendees being filled here attendee := remote.Attendee{ Type: t, EmailAddress: &remote.EmailAddress{ diff --git a/calendar/config/const.go b/calendar/config/const.go index b3427aef..61444dd5 100644 --- a/calendar/config/const.go +++ b/calendar/config/const.go @@ -3,8 +3,6 @@ package config -// REVIEW: need an interface for returning bot info -// probably good to have a struct to capture the data clump const ( BotDescription = "Created by the %s Plugin." diff --git a/calendar/engine/availability.go b/calendar/engine/availability.go index 2e0927b0..692adc69 100644 --- a/calendar/engine/availability.go +++ b/calendar/engine/availability.go @@ -23,7 +23,6 @@ const ( StatusSyncJobInterval = 5 * time.Minute upcomingEventNotificationTime = 10 * time.Minute - // REVIEW: This should be documented how this works. A dev has to read code to understand how the timing of these jobs and close proximity calendar events work upcomingEventNotificationWindow = (StatusSyncJobInterval * 11) / 10 // 110% of the interval logTruncateMsg = "We've truncated the logs due to too many messages" logTruncateLimit = 5 @@ -496,7 +495,6 @@ func (m *mscalendar) GetCalendarViews(users []*store.User) ([]*remote.ViewCalend }) } - // REVIEW: gcal batching requirement. maybe don't do batching, and instead use a channel to stream results back to here more concurrently return m.client.DoBatchViewCalendarRequests(params) } diff --git a/calendar/engine/calendar.go b/calendar/engine/calendar.go index 2596531a..ba097633 100644 --- a/calendar/engine/calendar.go +++ b/calendar/engine/calendar.go @@ -94,7 +94,6 @@ func (m *mscalendar) CreateEvent(user *User, event *remote.Event, mattermostUser return m.client.CreateEvent(user.Remote.ID, event) } -// REVIEW: remove all delete calendar references. dead code/feature func (m *mscalendar) DeleteCalendar(user *User, calendarID string) error { err := m.Filter( withClient, @@ -116,7 +115,6 @@ func (m *mscalendar) FindMeetingTimes(user *User, meetingParams *remote.FindMeet return nil, err } - // REVIEW: need to figure out exact usages of this return value, and shorten the scope into a smaller struct, to make it so there are no missed expectations of present data between providers return m.client.FindMeetingTimes(user.Remote.ID, meetingParams) } @@ -129,6 +127,5 @@ func (m *mscalendar) GetCalendars(user *User) ([]*remote.Calendar, error) { return nil, err } - // REVIEW: same here. need to figure out exact usages of this return value, and shorten the scope into a smaller struct, to make it so there are no missed expectations of present data between providers return m.client.GetCalendars(user.Remote.ID) } diff --git a/calendar/engine/client.go b/calendar/engine/client.go index f3c56d6c..47794596 100644 --- a/calendar/engine/client.go +++ b/calendar/engine/client.go @@ -23,7 +23,6 @@ func (m *mscalendar) MakeClient() (remote.Client, error) { return m.Remote.MakeClient(context.Background(), m.actingUser.OAuth2Token), nil } -// REVIEW: google service account? maybe not needed. this is only used for the status sync batch requests func (m *mscalendar) MakeSuperuserClient() (remote.Client, error) { return m.Remote.MakeSuperuserClient(context.Background()) } diff --git a/calendar/engine/daily_summary.go b/calendar/engine/daily_summary.go index 47969d40..a2e75f49 100644 --- a/calendar/engine/daily_summary.go +++ b/calendar/engine/daily_summary.go @@ -204,7 +204,6 @@ func (m *mscalendar) ProcessAllDailySummary(now time.Time) error { m.Poster.DM(user.MattermostUserID, postStr) - // REVIEW: Seems kind of pointless to track a passive event like this m.Dependencies.Tracker.TrackDailySummarySent(user.MattermostUserID) dsum.LastPostTime = time.Now().Format(time.RFC3339) err = m.Store.StoreUser(user) diff --git a/calendar/engine/event_responder.go b/calendar/engine/event_responder.go index 085a21b2..8fc4aaf3 100644 --- a/calendar/engine/event_responder.go +++ b/calendar/engine/event_responder.go @@ -14,7 +14,6 @@ type EventResponder interface { RespondToEvent(user *User, eventID, response string) error } -// REVIEW: See if this is still necessary. I believe RespondToEvent handles all cases already func (m *mscalendar) AcceptEvent(user *User, eventID string) error { err := m.Filter( withClient, diff --git a/calendar/engine/notification.go b/calendar/engine/notification.go index 4eb9ac94..f5b50fe5 100644 --- a/calendar/engine/notification.go +++ b/calendar/engine/notification.go @@ -138,7 +138,6 @@ func (processor *notificationProcessor) processNotification(n *remote.Notificati client := processor.Remote.MakeClient(context.Background(), creator.OAuth2Token) - // REVIEW: depends on lifecycle of subscriptions. its always false for gcal. set to true in msgraph client here https://github.com/mattermost/mattermost-plugin-mscalendar/blob/9ed5ee6e2141e7e6f32a5a80d7a20ab0881c8586/server/remote/msgraph/handle_webhook.go#L77-L80 if n.RecommendRenew { var renewed *remote.Subscription renewed, err = client.RenewSubscription(processor.Config.GetNotificationURL(), sub.Remote.CreatorID, n.Subscription) @@ -161,7 +160,6 @@ func (processor *notificationProcessor) processNotification(n *remote.Notificati }).Debugf("webhook notification: renewed user subscription.") } - // REVIEW: this seems to be implemented for gcal's case already if n.IsBare { n, err = client.GetNotificationData(n) if err != nil { diff --git a/calendar/engine/notification_format.go b/calendar/engine/notification_format.go index eec5f182..50244427 100644 --- a/calendar/engine/notification_format.go +++ b/calendar/engine/notification_format.go @@ -191,7 +191,6 @@ func eventToFields(e *remote.Event, timezone string) fields.Fields { case e.IsAllDay: dur = "all-day" - // REVIEW: would be good to extract some of this stuff out into separate functions. different file too default: switch hours { case 0: @@ -220,7 +219,6 @@ func eventToFields(e *remote.Event, timezone string) fields.Fields { attendees = append(attendees, fields.NewStringValue("None")) } - // REVIEW: some good stuff here. gotta make sure they are all filled in for gcal's case ff := fields.Fields{ FieldSubject: fields.NewStringValue(views.EnsureSubject(e.Subject)), FieldBodyPreview: fields.NewStringValue(valueOrNotDefined(e.BodyPreview)), diff --git a/calendar/engine/oauth2.go b/calendar/engine/oauth2.go index a56242eb..e0cf52e1 100644 --- a/calendar/engine/oauth2.go +++ b/calendar/engine/oauth2.go @@ -111,7 +111,7 @@ func (app *oauth2App) CompleteOAuth2(authedUserID, code, state string) error { } u.Settings.DailySummary = &store.DailySummaryUserSettings{ - PostTime: "8:00AM", // REVIEW: we shouls support military time for user inputs elsewhere + PostTime: "8:00AM", Timezone: mailboxSettings.TimeZone, Enable: false, } diff --git a/calendar/engine/settings_notifications.go b/calendar/engine/settings_notifications.go index 2835c881..ca0bfd4f 100644 --- a/calendar/engine/settings_notifications.go +++ b/calendar/engine/settings_notifications.go @@ -35,7 +35,6 @@ func (s *notificationSetting) Set(userID string, value interface{}) error { cal := s.getCal(userID) if boolValue { - // REVIEW: notification subscription logic in notification settings. this seems a bit weird. what does this function/block have to do specifically with notifications? _, err := cal.LoadMyEventSubscription() if err != nil { _, err := cal.CreateMyEventSubscription() diff --git a/calendar/engine/subscription.go b/calendar/engine/subscription.go index 4314e163..22d9a1c0 100644 --- a/calendar/engine/subscription.go +++ b/calendar/engine/subscription.go @@ -21,7 +21,6 @@ type Subscriptions interface { LoadMyEventSubscription() (*store.Subscription, error) } -// REVIEW: depends on the overlap of subscription logic between providers, but lots of logic about supscription lifecycle in this file func (m *mscalendar) CreateMyEventSubscription() (*store.Subscription, error) { err := m.Filter(withClient) if err != nil { diff --git a/calendar/engine/user.go b/calendar/engine/user.go index dcd50c11..fd01ea69 100644 --- a/calendar/engine/user.go +++ b/calendar/engine/user.go @@ -87,7 +87,6 @@ func (m *mscalendar) ExpandMattermostUser(user *User) error { return nil } -// REVIEW: the timezone is the only thing used from the mailbox settings func (m *mscalendar) GetTimezone(user *User) (string, error) { err := m.Filter( withClient, @@ -159,7 +158,6 @@ func (m *mscalendar) DisconnectUser(mattermostUserID string) error { eventSubscriptionID := storedUser.Settings.EventSubscriptionID if eventSubscriptionID != "" { - // REVIEW: deleting local notification subscription during disconnect sub, errLoad := m.Store.LoadSubscription(eventSubscriptionID) if errLoad != nil { return errors.Wrap(errLoad, "error loading subscription") diff --git a/calendar/jobs/renew_job.go b/calendar/jobs/renew_job.go index 480061a7..42e2e31c 100644 --- a/calendar/jobs/renew_job.go +++ b/calendar/jobs/renew_job.go @@ -31,7 +31,6 @@ func runRenewJob(env engine.Env) { for _, u := range uindex { asUser := engine.New(env, u.MattermostUserID) - // REVIEW: logging here is probably overkill env.Logger.Debugf("Renewing for user: %s", u.MattermostUserID) _, err = asUser.RenewMyEventSubscription() if err != nil { diff --git a/calendar/jobs/status_sync_job.go b/calendar/jobs/status_sync_job.go index 464f2693..f64ab966 100644 --- a/calendar/jobs/status_sync_job.go +++ b/calendar/jobs/status_sync_job.go @@ -26,6 +26,5 @@ func runSyncJob(env engine.Env) { env.Logger.Errorf("Error during user status sync job. err=%v", err) } - // REVIEW: This could be made easier to read env.Logger.Debugf("User status sync job finished.\nSummary\nNumber of users processed:- %d\nNumber of users had their status changed:- %d\nNumber of users had errors:- %d", syncJobSummary.NumberOfUsersProcessed, syncJobSummary.NumberOfUsersStatusChanged, syncJobSummary.NumberOfUsersFailedStatusChanged) } diff --git a/calendar/plugin/plugin.go b/calendar/plugin/plugin.go index ca65b697..112f3efc 100644 --- a/calendar/plugin/plugin.go +++ b/calendar/plugin/plugin.go @@ -172,7 +172,6 @@ func (p *Plugin) OnConfigurationChange() (err error) { e.bot = e.bot.WithConfig(stored.Config) e.Dependencies.Remote = remote.Makers[config.Provider.Name](e.Config, e.bot) - // REVIEW: need to make this provider agnostic terminology mscalendarBot := engine.NewMSCalendarBot(e.bot, e.Env, pluginURL) e.Dependencies.Logger = e.bot diff --git a/calendar/remote/event.go b/calendar/remote/event.go index a07812db..fe524726 100644 --- a/calendar/remote/event.go +++ b/calendar/remote/event.go @@ -10,7 +10,6 @@ const ( EventResponseStatusDeclined = "declined" ) -// REVIEW: we should vet exactly what fields are used from the remote package, and get rid of any "dead fields" from these structs type Event struct { Start *DateTime `json:"start,omitempty"` Location *Location `json:"location,omitempty"` diff --git a/calendar/store/setting_store.go b/calendar/store/setting_store.go index 5870902a..5f2c3850 100644 --- a/calendar/store/setting_store.go +++ b/calendar/store/setting_store.go @@ -88,7 +88,6 @@ func DefaultDailySummaryUserSettings() *DailySummaryUserSettings { return &DailySummaryUserSettings{ PostTime: "8:00AM", - // REVIEW: hardcoding timezone seems bad. I think we replace it with the user's timezone right afterwards though Timezone: "Eastern Standard Time", Enable: false, }