remove "REVIEW:" comments

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")
 }

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")

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{

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."
 

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)
 }
 

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)
 }

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())
 }

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)

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,

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 {

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)),

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,
 	}

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()

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 {

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")

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 {

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)
 }

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

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"`

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,
 	}