Refactor calendar engine into its own package

server/api/post_action.go

@@ -262,6 +262,7 @@ 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")
 }

server/command/create_event.go

@@ -13,6 +13,8 @@ import (
 	"github.com/mattermost/mattermost-plugin-mscalendar/server/utils"
 )
 
+// REVIEW: No autocomplete for this command
+
 func getCreateEventFlagSet() *flag.FlagSet {
 	flagSet := flag.NewFlagSet("create", flag.ContinueOnError)
 	flagSet.Bool("help", false, "show help")

server/command/find_meeting_times.go

@@ -18,6 +18,7 @@ func (c *Command) findMeetings(parameters ...string) (string, bool, error) {
 	for a := range parameters {
 		s := strings.Split(parameters[a], ":")
 		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{

server/command/info.go

@@ -9,6 +9,7 @@ import (
 	"github.com/mattermost/mattermost-plugin-mscalendar/server/config"
 )
 
+// REVIEW: hardcoded "microsoft"
 func (c *Command) info(parameters ...string) (string, bool, error) {
 	resp := fmt.Sprintf("Mattermost Microsoft Calendar plugin version: %s, "+
 		"[%s](https://github.com/mattermost/%s/commit/%s), built %s\n",

server/config/const.go

@@ -3,6 +3,8 @@
 
 package config
 
+// REVIEW: need an interface for returning bot info
+// probably good to have a struct to capture the data clump
 const (
 	BotUserName    = "gcal"
 	BotDisplayName = "Google Calendar"

server/jobs/renew_job.go

@@ -30,6 +30,8 @@ func runRenewJob(env mscalendar.Env) {
 
 	for _, u := range uindex {
 		asUser := mscalendar.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 {

server/jobs/status_sync_job.go

@@ -28,5 +28,6 @@ func runSyncJob(env mscalendar.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)
 }

server/mscalendar/availability.go

@@ -18,9 +18,11 @@ import (
 )
 
 const (
-	calendarViewTimeWindowSize      = 10 * time.Minute
-	StatusSyncJobInterval           = 5 * time.Minute
-	upcomingEventNotificationTime   = 10 * time.Minute
+	calendarViewTimeWindowSize    = 10 * time.Minute
+	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
@@ -401,6 +403,7 @@ 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)
 }
 

server/mscalendar/calendar.go

@@ -84,6 +84,7 @@ 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,
@@ -105,6 +106,7 @@ 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)
 }
 
@@ -117,5 +119,6 @@ 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)
 }

server/mscalendar/client.go

@@ -23,6 +23,7 @@ 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())
 }

server/mscalendar/daily_summary.go

@@ -160,6 +160,8 @@ 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)

server/mscalendar/event_responder.go

@@ -14,6 +14,7 @@ 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,

server/mscalendar/notification.go

@@ -142,6 +142,7 @@ 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.SubscriptionID)
@@ -164,6 +165,7 @@ 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 {
@@ -398,6 +400,7 @@ 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:
@@ -426,6 +429,7 @@ 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)),

server/mscalendar/oauth2.go

@@ -103,7 +103,7 @@ func (app *oauth2App) CompleteOAuth2(authedUserID, code, state string) error {
 	}
 
 	u.Settings.DailySummary = &store.DailySummaryUserSettings{
-		PostTime: "8:00AM",
+		PostTime: "8:00AM", // REVIEW: we shouls support military time for user inputs elsewhere
 		Timezone: mailboxSettings.TimeZone,
 		Enable:   false,
 	}

server/mscalendar/settings_notifications.go

@@ -35,6 +35,7 @@ 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()

server/mscalendar/subscription.go

@@ -21,6 +21,7 @@ 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 {

server/mscalendar/user.go

@@ -79,6 +79,7 @@ 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,
@@ -131,11 +132,13 @@ func (m *mscalendar) DisconnectUser(mattermostUserID string) error {
 
 	eventSubscriptionID := storedUser.Settings.EventSubscriptionID
 	if eventSubscriptionID != "" {
+		// REVIEW: deleting local notification subscription during disconnect
 		err = m.Store.DeleteUserSubscription(storedUser, eventSubscriptionID)
 		if err != nil && err != store.ErrNotFound {
 			return errors.WithMessagef(err, "failed to delete subscription %s", eventSubscriptionID)
 		}
 
+		// REVIEW: deleting remote notification subscription during disconnect
 		err = m.client.DeleteSubscription(eventSubscriptionID)
 		if err != nil {
 			m.Logger.Warnf("failed to delete remote subscription %s. err=%v", eventSubscriptionID, err)

server/mscalendar/welcomer.go

@@ -31,6 +31,7 @@ type mscBot struct {
 }
 
 const (
+	// REVIEW: "microsoft" referenced here
 	WelcomeMessage = `Welcome to the Microsoft Calendar plugin.
 	[Click here to link your account.](%s/oauth2/connect)`
 )
@@ -122,6 +123,7 @@ func (bot *mscBot) newConnectAttachment() *model.SlackAttachment {
 
 func (bot *mscBot) newConnectedAttachment(userLogin string) *model.SlackAttachment {
 	title := "Connect"
+	// REVIEW: "microsoft" referenced here
 	text := ":tada: Congratulations! Your microsoft account (*" + userLogin + "*) has been connected to Mattermost."
 	return &model.SlackAttachment{
 		Title:    title,

server/plugin/plugin.go

@@ -175,6 +175,7 @@ func (p *Plugin) OnConfigurationChange() (err error) {
 		e.bot = e.bot.WithConfig(stored.Config)
 		e.Dependencies.Remote = remote.Makers[gcal.Kind](e.Config, e.bot)
 
+		// REVIEW: need to make this provider agnostic terminology
 		mscalendarBot := mscalendar.NewMSCalendarBot(e.bot, e.Env, pluginURL)
 
 		e.Dependencies.Logger = e.bot

server/remote/event.go

@@ -3,6 +3,7 @@
 
 package remote
 
+// 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"`

server/remote/gcal/get_default_calendar_view.go

@@ -169,6 +169,7 @@ type calendarViewBatchResponse struct {
 }
 
 func (c *client) DoBatchViewCalendarRequests(allParams []*remote.ViewCalendarParams) ([]*remote.ViewCalendarResponse, error) {
+	// REVIEW: we can just not use googles batch api if necessary
 	if true {
 		return nil, errors.New("gcal DoBatchViewCalendarRequests not implemented")
 	}

server/remote/gcal/get_mailbox_settings.go

@@ -4,13 +4,31 @@
 package gcal
 
 import (
+	"context"
+
+	"github.com/pkg/errors"
+	"google.golang.org/api/calendar/v3"
+	"google.golang.org/api/option"
+
 	"github.com/mattermost/mattermost-plugin-mscalendar/server/remote"
 )
 
 func (c *client) GetMailboxSettings(remoteUserID string) (*remote.MailboxSettings, error) {
-	// GCAL TODO
+	// REVIEW: Implemented timezone but need to verify if it is correct
+
+	service, err := calendar.NewService(context.Background(), option.WithHTTPClient(c.httpClient))
+	if err != nil {
+		return nil, errors.Wrap(err, "gcal GetMailboxSettings, error creating service")
+	}
+
+	setting, err := service.Settings.Get("timezone").Do()
+	if err != nil {
+		return nil, errors.Wrap(err, "gcal GetMailboxSettings, error getting timezone setting")
+	}
+
+	// probably should get rid of `MailBoxSettings` and just return the timezone as a string. or fill out the rest of the struct, which is not necessarily possible or useful. "WorkingHours" is the only other field in the struct
 	out := &remote.MailboxSettings{
-		TimeZone: "Eastern Standard Time",
+		TimeZone: setting.Value,
 	}
 	return out, nil
 }

server/store/setting_store.go

@@ -85,6 +85,8 @@ func (s *pluginStore) GetSetting(userID, settingID string) (interface{}, error)
 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,
 	}

server/utils/oauth2connect/oauth2_complete.go

@@ -28,6 +28,7 @@ func (oa *oa) oauth2Complete(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// REVIEW "microsoft" is hardcoded
 	html := `
 		<!DOCTYPE html>
 		<html>