From 8500d834ff0b63c18c1f1ca2c478a9c6d379df1f Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Thu, 8 Dec 2022 14:10:47 -0500 Subject: [PATCH 01/58] Add basic model for true up review data requirements. --- model/true_up_review_profile.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 model/true_up_review_profile.go diff --git a/model/true_up_review_profile.go b/model/true_up_review_profile.go new file mode 100644 index 0000000000..ca4e537492 --- /dev/null +++ b/model/true_up_review_profile.go @@ -0,0 +1,25 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package model + +type TrueUpReviewProfile struct { + ServerId string `json:"server_id"` + ServerVersion string `json:"server_version"` + CustomerName string `json:"customer_name"` + LicenseId string `json:"licnes_id"` + LicnesedSeats int `json:"license_seats"` + LicensePlan string `json:"license_plan"` + ActiveUsers int `json:"active_users"` + AuthenticationFeatures []string `json:"authentication_features"` + Plugins TrueUpReviewPlugins `json:"plugins"` + TotalWebhooks int `json:"webhooks_count"` + TotalPlaybooks int `json:"playbooks_count"` + TotalBoards int `json:"boards_count"` + TotalCalls int `json:"calls_count"` +} + +type TrueUpReviewPlugins struct { + TotalPlugins int `json:"total_plugins"` + PluginNames []string `json:"plugin_names"` +} From 6e6a1ec01c006cfb7f09dd48c0e3ace20c14d826 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Thu, 8 Dec 2022 14:13:23 -0500 Subject: [PATCH 02/58] Add server installation type, ensure customer name is optional. --- model/true_up_review_profile.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/model/true_up_review_profile.go b/model/true_up_review_profile.go index ca4e537492..87f0741390 100644 --- a/model/true_up_review_profile.go +++ b/model/true_up_review_profile.go @@ -6,7 +6,8 @@ package model type TrueUpReviewProfile struct { ServerId string `json:"server_id"` ServerVersion string `json:"server_version"` - CustomerName string `json:"customer_name"` + ServerInstallationType string `json:"server_installation_type"` + CustomerName *string `json:"customer_name"` // Might not be availabe? LicenseId string `json:"licnes_id"` LicnesedSeats int `json:"license_seats"` LicensePlan string `json:"license_plan"` From c0735ae689fe583c94d12f3ba44ef6e27092cc0b Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Thu, 8 Dec 2022 16:11:43 -0500 Subject: [PATCH 03/58] Add route with functionality for building out the true up review profile. --- api4/license.go | 83 +++++++++++++++++++++++++++ model/true_up_review_profile.go | 15 +++-- services/telemetry/telemetry.go | 2 + store/sqlstore/webhook_store.go | 36 ++++++++++++ store/store.go | 3 + store/storetest/mocks/WebhookStore.go | 42 ++++++++++++++ 6 files changed, 175 insertions(+), 6 deletions(-) diff --git a/api4/license.go b/api4/license.go index bc89f7e1e2..749fd71ead 100644 --- a/api4/license.go +++ b/api4/license.go @@ -9,7 +9,9 @@ import ( "fmt" "io" "net/http" + "os" + "github.com/mattermost/mattermost-server/v6/services/telemetry" "github.com/mattermost/mattermost-server/v6/shared/mlog" "github.com/mattermost/mattermost-server/v6/utils" @@ -24,6 +26,7 @@ func (api *API) InitLicense() { api.BaseRoutes.APIRoot.Handle("/license", api.APISessionRequired(removeLicense)).Methods("DELETE") api.BaseRoutes.APIRoot.Handle("/license/renewal", api.APISessionRequired(requestRenewalLink)).Methods("GET") api.BaseRoutes.APIRoot.Handle("/license/client", api.APIHandler(getClientLicense)).Methods("GET") + api.BaseRoutes.APIRoot.Handle("/license/review", api.APIHandler(requestTrueUpReview)).Methods("POST") } func getClientLicense(c *Context, w http.ResponseWriter, r *http.Request) { @@ -296,3 +299,83 @@ func getPrevTrialLicense(c *Context, w http.ResponseWriter, r *http.Request) { w.Write([]byte(model.MapToJSON(clientLicense))) } + +func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { + license := c.App.Channels().License() + if license == nil { + return + } + + userId := c.AppContext.Session().UserId + subscription, err := c.App.Cloud().GetSubscription(userId) + if err != nil { + fmt.Printf("1: %+v", err) + return + } + + reviewProfile := model.TrueUpReviewProfile{} + + // Server Data + reviewProfile.ServerId = c.App.TelemetryId() + reviewProfile.ServerVersion = model.CurrentVersion + reviewProfile.ServerInstallationType = os.Getenv(telemetry.EnvVarInstallType) + + // License Data + reviewProfile.LicenseId = license.Id + reviewProfile.LicensedSeats = subscription.Seats + reviewProfile.LicensePlan = license.SkuName + + activeUserCount, err := c.App.Srv().GetStore().Status().GetTotalActiveUsersCount() + if err != nil { + fmt.Printf("2: %+v", err) + return + } + + // Customer Info & Usage Analytics + reviewProfile.CustomerName = license.Customer.Name + reviewProfile.ActiveUsers = activeUserCount + + // Webhook, call, board, playbook counts + var totalWebHookCount int64 = 0 + incomingWebhookCount, err := c.App.Srv().Store().Webhook().GetIncomingTotal() + if err != nil { + fmt.Printf("3: %+v", err) + return + } + outgoingWebhookCount, err := c.App.Srv().Store().Webhook().GetOutgoingTotal() + if err != nil { + fmt.Printf("4: %+v", err) + return + } + + totalWebHookCount += incomingWebhookCount + totalWebHookCount += outgoingWebhookCount + + reviewProfile.TotalWebhooks = totalWebHookCount + reviewProfile.TotalCalls = 0 + reviewProfile.TotalBoards = 0 + reviewProfile.TotalPlaybooks = 0 + + // Plugin Data + trueUpReviewPlugins := model.TrueUpReviewPlugins{} + if pluginResponse, err := c.App.GetPlugins(); err == nil { + for _, plugin := range pluginResponse.Active { + trueUpReviewPlugins.ActivePluginNames = append(trueUpReviewPlugins.ActivePluginNames, plugin.Name) + trueUpReviewPlugins.TotalActivePlugins += 1 + } + + for _, plugin := range pluginResponse.Inactive { + trueUpReviewPlugins.InactivePluginNames = append(trueUpReviewPlugins.InactivePluginNames, plugin.Name) + trueUpReviewPlugins.TotalInactivePlugins += 1 + } + } + + reviewProfile.Plugins = trueUpReviewPlugins + + json, err := json.Marshal(reviewProfile) + if err != nil { + fmt.Printf("5: %+v", err) + return + } + w.Write(json) +} diff --git a/model/true_up_review_profile.go b/model/true_up_review_profile.go index 87f0741390..67b51a3e88 100644 --- a/model/true_up_review_profile.go +++ b/model/true_up_review_profile.go @@ -7,20 +7,23 @@ type TrueUpReviewProfile struct { ServerId string `json:"server_id"` ServerVersion string `json:"server_version"` ServerInstallationType string `json:"server_installation_type"` - CustomerName *string `json:"customer_name"` // Might not be availabe? LicenseId string `json:"licnes_id"` - LicnesedSeats int `json:"license_seats"` + LicensedSeats int `json:"licensed_seats"` LicensePlan string `json:"license_plan"` - ActiveUsers int `json:"active_users"` + CustomerName string `json:"customer_name"` + ActiveUsers int64 `json:"active_users"` AuthenticationFeatures []string `json:"authentication_features"` Plugins TrueUpReviewPlugins `json:"plugins"` - TotalWebhooks int `json:"webhooks_count"` + TotalWebhooks int64 `json:"webhooks_count"` TotalPlaybooks int `json:"playbooks_count"` TotalBoards int `json:"boards_count"` TotalCalls int `json:"calls_count"` } type TrueUpReviewPlugins struct { - TotalPlugins int `json:"total_plugins"` - PluginNames []string `json:"plugin_names"` + TotalPlugins int `json:"total_plugins"` + TotalActivePlugins int `json:"total_active_plugins"` + TotalInactivePlugins int `json:"total_inactive_plugins"` + ActivePluginNames []string `json:"active_plugin_names"` + InactivePluginNames []string `json:"inactive_plugin_names"` } diff --git a/services/telemetry/telemetry.go b/services/telemetry/telemetry.go index 9db3a36d00..67a0ca1209 100644 --- a/services/telemetry/telemetry.go +++ b/services/telemetry/telemetry.go @@ -81,6 +81,8 @@ const ( TrackLicense = "license" TrackServer = "server" TrackPlugins = "plugins" + + TrackTrueUpReview = "true_up_review" ) type ServerIface interface { diff --git a/store/sqlstore/webhook_store.go b/store/sqlstore/webhook_store.go index d9f08120ac..df10c2f1a0 100644 --- a/store/sqlstore/webhook_store.go +++ b/store/sqlstore/webhook_store.go @@ -400,3 +400,39 @@ func (s SqlWebhookStore) AnalyticsOutgoingCount(teamId string) (int64, error) { } return count, nil } + +func (s SqlWebhookStore) GetIncomingTotal() (int64, error) { + queryBuilder := + s.getQueryBuilder(). + Select("COUNT(*)"). + From("IncomingWebhooks") + + queryString, args, err := queryBuilder.ToSql() + if err != nil { + return 0, errors.Wrap(err, "incoming_webhook_tosql") + } + + var count int64 + if err := s.GetReplicaX().Get(&count, queryString, args...); err != nil { + return 0, errors.Wrap(err, "failed to count total IncomingWebooks") + } + return count, nil +} + +func (s SqlWebhookStore) GetOutgoingTotal() (int64, error) { + queryBuilder := + s.getQueryBuilder(). + Select("COUNT(*)"). + From("OutgoingWebhooks") + + queryString, args, err := queryBuilder.ToSql() + if err != nil { + return 0, errors.Wrap(err, "outgoing_webhook_tosql") + } + + var count int64 + if err := s.GetReplicaX().Get(&count, queryString, args...); err != nil { + return 0, errors.Wrap(err, "failed to count total OutgoingWebhooks") + } + return count, nil +} diff --git a/store/store.go b/store/store.go index 6a2a7d6e1d..3014d7bcf6 100644 --- a/store/store.go +++ b/store/store.go @@ -615,6 +615,9 @@ type WebhookStore interface { AnalyticsOutgoingCount(teamID string) (int64, error) InvalidateWebhookCache(webhook string) ClearCaches() + + GetOutgoingTotal() (int64, error) + GetIncomingTotal() (int64, error) } type CommandStore interface { diff --git a/store/storetest/mocks/WebhookStore.go b/store/storetest/mocks/WebhookStore.go index 5abfc36738..b6ce04cc3d 100644 --- a/store/storetest/mocks/WebhookStore.go +++ b/store/storetest/mocks/WebhookStore.go @@ -227,6 +227,27 @@ func (_m *WebhookStore) GetIncomingListByUser(userID string, offset int, limit i return r0, r1 } +// GetIncomingTotal provides a mock function with given fields: +func (_m *WebhookStore) GetIncomingTotal() (int64, error) { + ret := _m.Called() + + var r0 int64 + if rf, ok := ret.Get(0).(func() int64); ok { + r0 = rf() + } else { + r0 = ret.Get(0).(int64) + } + + var r1 error + if rf, ok := ret.Get(1).(func() error); ok { + r1 = rf() + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // GetOutgoing provides a mock function with given fields: id func (_m *WebhookStore) GetOutgoing(id string) (*model.OutgoingWebhook, error) { ret := _m.Called(id) @@ -388,6 +409,27 @@ func (_m *WebhookStore) GetOutgoingListByUser(userID string, offset int, limit i return r0, r1 } +// GetOutgoingTotal provides a mock function with given fields: +func (_m *WebhookStore) GetOutgoingTotal() (int64, error) { + ret := _m.Called() + + var r0 int64 + if rf, ok := ret.Get(0).(func() int64); ok { + r0 = rf() + } else { + r0 = ret.Get(0).(int64) + } + + var r1 error + if rf, ok := ret.Get(1).(func() error); ok { + r1 = rf() + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // InvalidateWebhookCache provides a mock function with given fields: webhook func (_m *WebhookStore) InvalidateWebhookCache(webhook string) { _m.Called(webhook) From 68c27f7c374c9ea0686d9c6eee8c5cbc27e9444c Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Thu, 8 Dec 2022 16:20:17 -0500 Subject: [PATCH 04/58] Run make store-layers --- store/opentracinglayer/opentracinglayer.go | 36 +++++++++++++++++++ store/retrylayer/retrylayer.go | 42 ++++++++++++++++++++++ store/timerlayer/timerlayer.go | 32 +++++++++++++++++ 3 files changed, 110 insertions(+) diff --git a/store/opentracinglayer/opentracinglayer.go b/store/opentracinglayer/opentracinglayer.go index aa41c28ab5..75e832ccf9 100644 --- a/store/opentracinglayer/opentracinglayer.go +++ b/store/opentracinglayer/opentracinglayer.go @@ -12462,6 +12462,24 @@ func (s *OpenTracingLayerWebhookStore) GetIncomingListByUser(userID string, offs return result, err } +func (s *OpenTracingLayerWebhookStore) GetIncomingTotal() (int64, error) { + origCtx := s.Root.Store.Context() + span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "WebhookStore.GetIncomingTotal") + s.Root.Store.SetContext(newCtx) + defer func() { + s.Root.Store.SetContext(origCtx) + }() + + defer span.Finish() + result, err := s.WebhookStore.GetIncomingTotal() + if err != nil { + span.LogFields(spanlog.Error(err)) + ext.Error.Set(span, true) + } + + return result, err +} + func (s *OpenTracingLayerWebhookStore) GetOutgoing(id string) (*model.OutgoingWebhook, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "WebhookStore.GetOutgoing") @@ -12588,6 +12606,24 @@ func (s *OpenTracingLayerWebhookStore) GetOutgoingListByUser(userID string, offs return result, err } +func (s *OpenTracingLayerWebhookStore) GetOutgoingTotal() (int64, error) { + origCtx := s.Root.Store.Context() + span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "WebhookStore.GetOutgoingTotal") + s.Root.Store.SetContext(newCtx) + defer func() { + s.Root.Store.SetContext(origCtx) + }() + + defer span.Finish() + result, err := s.WebhookStore.GetOutgoingTotal() + if err != nil { + span.LogFields(spanlog.Error(err)) + ext.Error.Set(span, true) + } + + return result, err +} + func (s *OpenTracingLayerWebhookStore) InvalidateWebhookCache(webhook string) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "WebhookStore.InvalidateWebhookCache") diff --git a/store/retrylayer/retrylayer.go b/store/retrylayer/retrylayer.go index 5aca1535cb..426cc15487 100644 --- a/store/retrylayer/retrylayer.go +++ b/store/retrylayer/retrylayer.go @@ -14211,6 +14211,27 @@ func (s *RetryLayerWebhookStore) GetIncomingListByUser(userID string, offset int } +func (s *RetryLayerWebhookStore) GetIncomingTotal() (int64, error) { + + tries := 0 + for { + result, err := s.WebhookStore.GetIncomingTotal() + if err == nil { + return result, nil + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + func (s *RetryLayerWebhookStore) GetOutgoing(id string) (*model.OutgoingWebhook, error) { tries := 0 @@ -14358,6 +14379,27 @@ func (s *RetryLayerWebhookStore) GetOutgoingListByUser(userID string, offset int } +func (s *RetryLayerWebhookStore) GetOutgoingTotal() (int64, error) { + + tries := 0 + for { + result, err := s.WebhookStore.GetOutgoingTotal() + if err == nil { + return result, nil + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + func (s *RetryLayerWebhookStore) InvalidateWebhookCache(webhook string) { s.WebhookStore.InvalidateWebhookCache(webhook) diff --git a/store/timerlayer/timerlayer.go b/store/timerlayer/timerlayer.go index c4c89b935c..cff6c65e91 100644 --- a/store/timerlayer/timerlayer.go +++ b/store/timerlayer/timerlayer.go @@ -11224,6 +11224,22 @@ func (s *TimerLayerWebhookStore) GetIncomingListByUser(userID string, offset int return result, err } +func (s *TimerLayerWebhookStore) GetIncomingTotal() (int64, error) { + start := time.Now() + + result, err := s.WebhookStore.GetIncomingTotal() + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("WebhookStore.GetIncomingTotal", success, elapsed) + } + return result, err +} + func (s *TimerLayerWebhookStore) GetOutgoing(id string) (*model.OutgoingWebhook, error) { start := time.Now() @@ -11336,6 +11352,22 @@ func (s *TimerLayerWebhookStore) GetOutgoingListByUser(userID string, offset int return result, err } +func (s *TimerLayerWebhookStore) GetOutgoingTotal() (int64, error) { + start := time.Now() + + result, err := s.WebhookStore.GetOutgoingTotal() + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("WebhookStore.GetOutgoingTotal", success, elapsed) + } + return result, err +} + func (s *TimerLayerWebhookStore) InvalidateWebhookCache(webhook string) { start := time.Now() From cad414fd4dfd10f16cd41a82d35bf944a2f2616d Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Tue, 13 Dec 2022 13:09:20 -0400 Subject: [PATCH 05/58] Use proper error handling, fix up data structure, attempt to send telemetry data. --- api4/license.go | 59 ++++++++++++++++++++++++++------- model/license.go | 9 +++++ model/true_up_review_profile.go | 1 - 3 files changed, 56 insertions(+), 13 deletions(-) diff --git a/api4/license.go b/api4/license.go index 749fd71ead..0aa6d301b3 100644 --- a/api4/license.go +++ b/api4/license.go @@ -303,13 +303,14 @@ func getPrevTrialLicense(c *Context, w http.ResponseWriter, r *http.Request) { func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { license := c.App.Channels().License() if license == nil { + http.Error(w, "A License is required to perform a true-up review", http.StatusBadRequest) return } userId := c.AppContext.Session().UserId subscription, err := c.App.Cloud().GetSubscription(userId) if err != nil { - fmt.Printf("1: %+v", err) + http.Error(w, err.Error(), http.StatusInternalServerError) return } @@ -325,26 +326,26 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { reviewProfile.LicensedSeats = subscription.Seats reviewProfile.LicensePlan = license.SkuName + // Customer Info & Usage Analytics activeUserCount, err := c.App.Srv().GetStore().Status().GetTotalActiveUsersCount() if err != nil { - fmt.Printf("2: %+v", err) + http.Error(w, err.Error(), http.StatusInternalServerError) return } - // Customer Info & Usage Analytics reviewProfile.CustomerName = license.Customer.Name reviewProfile.ActiveUsers = activeUserCount - // Webhook, call, board, playbook counts + // Webhook, calls, boards, and playbook counts var totalWebHookCount int64 = 0 incomingWebhookCount, err := c.App.Srv().Store().Webhook().GetIncomingTotal() if err != nil { - fmt.Printf("3: %+v", err) + http.Error(w, err.Error(), http.StatusInternalServerError) return } outgoingWebhookCount, err := c.App.Srv().Store().Webhook().GetOutgoingTotal() if err != nil { - fmt.Printf("4: %+v", err) + http.Error(w, err.Error(), http.StatusInternalServerError) return } @@ -352,12 +353,16 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { totalWebHookCount += outgoingWebhookCount reviewProfile.TotalWebhooks = totalWebHookCount - reviewProfile.TotalCalls = 0 - reviewProfile.TotalBoards = 0 - reviewProfile.TotalPlaybooks = 0 + reviewProfile.TotalCalls = 0 // TODO: Maybe from plugin? + reviewProfile.TotalBoards = 0 // TODO: Maybe from plugin? + reviewProfile.TotalPlaybooks = 0 // TODO: Maybe from plugin? // Plugin Data - trueUpReviewPlugins := model.TrueUpReviewPlugins{} + trueUpReviewPlugins := model.TrueUpReviewPlugins{ + ActivePluginNames: []string{}, + InactivePluginNames: []string{}, + } + if pluginResponse, err := c.App.GetPlugins(); err == nil { for _, plugin := range pluginResponse.Active { trueUpReviewPlugins.ActivePluginNames = append(trueUpReviewPlugins.ActivePluginNames, plugin.Name) @@ -369,12 +374,42 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { trueUpReviewPlugins.TotalInactivePlugins += 1 } } - reviewProfile.Plugins = trueUpReviewPlugins + // Authentication Data + mfaUsed := c.App.Config().ServiceSettings.EnforceMultifactorAuthentication + ldapUsed := c.App.Config().LdapSettings.Enable + samlUsed := c.App.Config().SamlSettings.Enable + openIdUsed := c.App.Config().OpenIdSettings.Enable + guessAccessAllowed := c.App.Config().GuestAccountsSettings.Enable + + authFeatures := map[string]*bool{ + model.TrueUpReviewAuthFeaturesMfa: mfaUsed, + model.TueUpReviewAuthFeaturesAdLdap: ldapUsed, + model.TrueUpReviewauthFeaturesSaml: samlUsed, + model.TrueUpReviewAuthFeatureOpenId: openIdUsed, + model.TrueUpReviewAuthFeatureGuestAccess: guessAccessAllowed, + } + + reviewProfile.AuthenticationFeatures = []string{} + for feature, used := range authFeatures { + if used != nil && *used { + reviewProfile.AuthenticationFeatures = append(reviewProfile.AuthenticationFeatures, feature) + } + } + + // Convert true up review profile struct to map + var telemetryProperties map[string]interface{} + marshalled, _ := json.Marshal(reviewProfile) + json.Unmarshal(marshalled, &telemetryProperties) + + // Send telemetry data. + telemetryService := c.App.Srv().GetTelemetryService() + telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, telemetryProperties) + json, err := json.Marshal(reviewProfile) if err != nil { - fmt.Printf("5: %+v", err) + http.Error(w, err.Error(), http.StatusInternalServerError) return } w.Write(json) diff --git a/model/license.go b/model/license.go index 94f0b81da4..ef7d2e4ffd 100644 --- a/model/license.go +++ b/model/license.go @@ -39,6 +39,15 @@ var ( sanctionedTrialDurationUpperBound = 29*(time.Hour*24) + (time.Hour * 23) + (time.Minute * 59) + (time.Second * 59) // 696 hours (29 days) + 23 hours, 59 mins and 59 seconds ) +const ( + TrueUpReviewTelemetryName = "true-up-review-sent" + TrueUpReviewAuthFeaturesMfa = "multi_factor_authentication" + TueUpReviewAuthFeaturesAdLdap = "ad_ldap_sign_in" + TrueUpReviewauthFeaturesSaml = "saml_sign_in" + TrueUpReviewAuthFeatureOpenId = "openid_connect" + TrueUpReviewAuthFeatureGuestAccess = "guest_access" +) + type LicenseRecord struct { Id string `json:"id"` CreateAt int64 `json:"create_at"` diff --git a/model/true_up_review_profile.go b/model/true_up_review_profile.go index 67b51a3e88..4d8f8be7e9 100644 --- a/model/true_up_review_profile.go +++ b/model/true_up_review_profile.go @@ -21,7 +21,6 @@ type TrueUpReviewProfile struct { } type TrueUpReviewPlugins struct { - TotalPlugins int `json:"total_plugins"` TotalActivePlugins int `json:"total_active_plugins"` TotalInactivePlugins int `json:"total_inactive_plugins"` ActivePluginNames []string `json:"active_plugin_names"` From 96ed60a472b3ac46678eead1831151542cf1b010 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Mon, 19 Dec 2022 15:36:15 -0500 Subject: [PATCH 06/58] Remove unused fields, update response. --- api4/license.go | 20 +++++++------------- model/true_up_review_profile.go | 6 ++---- 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/api4/license.go b/api4/license.go index 0aa6d301b3..71de1ad298 100644 --- a/api4/license.go +++ b/api4/license.go @@ -327,7 +327,7 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { reviewProfile.LicensePlan = license.SkuName // Customer Info & Usage Analytics - activeUserCount, err := c.App.Srv().GetStore().Status().GetTotalActiveUsersCount() + activeUserCount, err := c.App.Srv().Store().Status().GetTotalActiveUsersCount() if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -337,7 +337,6 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { reviewProfile.ActiveUsers = activeUserCount // Webhook, calls, boards, and playbook counts - var totalWebHookCount int64 = 0 incomingWebhookCount, err := c.App.Srv().Store().Webhook().GetIncomingTotal() if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) @@ -349,13 +348,8 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { return } - totalWebHookCount += incomingWebhookCount - totalWebHookCount += outgoingWebhookCount - - reviewProfile.TotalWebhooks = totalWebHookCount - reviewProfile.TotalCalls = 0 // TODO: Maybe from plugin? - reviewProfile.TotalBoards = 0 // TODO: Maybe from plugin? - reviewProfile.TotalPlaybooks = 0 // TODO: Maybe from plugin? + reviewProfile.TotalIncomingWebhooks = incomingWebhookCount + reviewProfile.TotalOutgoingWebhooks = outgoingWebhookCount // Plugin Data trueUpReviewPlugins := model.TrueUpReviewPlugins{ @@ -400,17 +394,17 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { // Convert true up review profile struct to map var telemetryProperties map[string]interface{} - marshalled, _ := json.Marshal(reviewProfile) - json.Unmarshal(marshalled, &telemetryProperties) + reviewProfileJson, err := json.Marshal(reviewProfile) + json.Unmarshal(reviewProfileJson, &telemetryProperties) // Send telemetry data. telemetryService := c.App.Srv().GetTelemetryService() telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, telemetryProperties) - json, err := json.Marshal(reviewProfile) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return } - w.Write(json) + + ReturnStatusOK(w) } diff --git a/model/true_up_review_profile.go b/model/true_up_review_profile.go index 4d8f8be7e9..91e8038f85 100644 --- a/model/true_up_review_profile.go +++ b/model/true_up_review_profile.go @@ -14,10 +14,8 @@ type TrueUpReviewProfile struct { ActiveUsers int64 `json:"active_users"` AuthenticationFeatures []string `json:"authentication_features"` Plugins TrueUpReviewPlugins `json:"plugins"` - TotalWebhooks int64 `json:"webhooks_count"` - TotalPlaybooks int `json:"playbooks_count"` - TotalBoards int `json:"boards_count"` - TotalCalls int `json:"calls_count"` + TotalIncomingWebhooks int64 `json:"incoming_webhooks_count"` + TotalOutgoingWebhooks int64 `json:"outgoing_webhooks_count"` } type TrueUpReviewPlugins struct { From 2c092291f5e6d4a1b8c7e519bbf0b0bce0058161 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Mon, 19 Dec 2022 17:11:27 -0500 Subject: [PATCH 07/58] Reorder data extraction, define review profile in-place rather than sectioned for each data domain --- api4/license.go | 54 +++++++++++++++++++++++++------------------- api4/license_test.go | 35 ++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 23 deletions(-) diff --git a/api4/license.go b/api4/license.go index 71de1ad298..6240be1190 100644 --- a/api4/license.go +++ b/api4/license.go @@ -301,31 +301,31 @@ func getPrevTrialLicense(c *Context, w http.ResponseWriter, r *http.Request) { } func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { + // Only admins can request a true up review. + if !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionManageSystem) { + c.SetPermissionError(model.PermissionManageLicenseInformation) + return + } + + if c.App.Cloud() == nil { + c.Err = model.NewAppError("requestRenewalLink", "api.license.upgrade_needed.app_error", nil, "", http.StatusForbidden) + return + } + license := c.App.Channels().License() if license == nil { http.Error(w, "A License is required to perform a true-up review", http.StatusBadRequest) return } + // Subscription Data userId := c.AppContext.Session().UserId subscription, err := c.App.Cloud().GetSubscription(userId) - if err != nil { + if err != nil || subscription == nil { http.Error(w, err.Error(), http.StatusInternalServerError) return } - reviewProfile := model.TrueUpReviewProfile{} - - // Server Data - reviewProfile.ServerId = c.App.TelemetryId() - reviewProfile.ServerVersion = model.CurrentVersion - reviewProfile.ServerInstallationType = os.Getenv(telemetry.EnvVarInstallType) - - // License Data - reviewProfile.LicenseId = license.Id - reviewProfile.LicensedSeats = subscription.Seats - reviewProfile.LicensePlan = license.SkuName - // Customer Info & Usage Analytics activeUserCount, err := c.App.Srv().Store().Status().GetTotalActiveUsersCount() if err != nil { @@ -333,9 +333,6 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { return } - reviewProfile.CustomerName = license.Customer.Name - reviewProfile.ActiveUsers = activeUserCount - // Webhook, calls, boards, and playbook counts incomingWebhookCount, err := c.App.Srv().Store().Webhook().GetIncomingTotal() if err != nil { @@ -348,9 +345,6 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { return } - reviewProfile.TotalIncomingWebhooks = incomingWebhookCount - reviewProfile.TotalOutgoingWebhooks = outgoingWebhookCount - // Plugin Data trueUpReviewPlugins := model.TrueUpReviewPlugins{ ActivePluginNames: []string{}, @@ -368,9 +362,8 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { trueUpReviewPlugins.TotalInactivePlugins += 1 } } - reviewProfile.Plugins = trueUpReviewPlugins - // Authentication Data + // Authentication Features mfaUsed := c.App.Config().ServiceSettings.EnforceMultifactorAuthentication ldapUsed := c.App.Config().LdapSettings.Enable samlUsed := c.App.Config().SamlSettings.Enable @@ -385,13 +378,28 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { model.TrueUpReviewAuthFeatureGuestAccess: guessAccessAllowed, } - reviewProfile.AuthenticationFeatures = []string{} + authFeatureList := []string{} for feature, used := range authFeatures { if used != nil && *used { - reviewProfile.AuthenticationFeatures = append(reviewProfile.AuthenticationFeatures, feature) + authFeatureList = append(authFeatureList, feature) } } + reviewProfile := model.TrueUpReviewProfile{ + ServerId: c.App.TelemetryId(), + ServerVersion: model.CurrentVersion, + ServerInstallationType: os.Getenv(telemetry.EnvVarInstallType), + LicenseId: license.Id, + LicensedSeats: subscription.Seats, + LicensePlan: license.SkuName, + CustomerName: license.Customer.Name, + ActiveUsers: activeUserCount, + TotalIncomingWebhooks: incomingWebhookCount, + TotalOutgoingWebhooks: outgoingWebhookCount, + Plugins: trueUpReviewPlugins, + AuthenticationFeatures: authFeatureList, + } + // Convert true up review profile struct to map var telemetryProperties map[string]interface{} reviewProfileJson, err := json.Marshal(reviewProfile) diff --git a/api4/license_test.go b/api4/license_test.go index d11fe1b150..f76fd4a084 100644 --- a/api4/license_test.go +++ b/api4/license_test.go @@ -330,3 +330,38 @@ func TestRequestRenewalLink(t *testing.T) { require.Equal(t, http.StatusBadRequest, resp.StatusCode) }) } + +func TestRequestTrueUpReview(t *testing.T) { + th := Setup(t) + defer th.TearDown() + + subscription := &model.Subscription{ + ID: "MySubscriptionID", + CustomerID: "MyCustomer", + ProductID: "SomeProductId", + AddOns: []string{}, + StartAt: 1000000000, + EndAt: 2000000000, + CreateAt: 1000000000, + Seats: 10, + IsFreeTrial: "true", + DNS: "some.dns.server", + TrialEndAt: 2000000000, + LastInvoice: &model.Invoice{}, + } + + th.App.Srv().SetLicense(model.NewTestLicense()) + + cloud := mocks.CloudInterface{} + cloud.Mock.On("GetSubscription", mock.Anything).Return(subscription, nil) + + cloudImpl := th.App.Srv().Cloud + th.App.Srv().Cloud = &cloud + defer func() { + th.App.Srv().Cloud = cloudImpl + }() + + resp, err := th.SystemAdminClient.DoAPIPost("/license/review", "") + require.Nil(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) +} From eb0cc1c3a4c82bf2d362cf0fc62bf0cd6d29a404 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Mon, 19 Dec 2022 17:28:26 -0500 Subject: [PATCH 08/58] Add tests. --- api4/license_test.go | 44 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/api4/license_test.go b/api4/license_test.go index f76fd4a084..a9df54c8ea 100644 --- a/api4/license_test.go +++ b/api4/license_test.go @@ -5,6 +5,7 @@ package api4 import ( "encoding/json" + "errors" "net/http" "net/http/httptest" "testing" @@ -352,16 +353,39 @@ func TestRequestTrueUpReview(t *testing.T) { th.App.Srv().SetLicense(model.NewTestLicense()) - cloud := mocks.CloudInterface{} - cloud.Mock.On("GetSubscription", mock.Anything).Return(subscription, nil) + t.Run("returns status 200 when telemetry data sent", func(t *testing.T) { + cloud := mocks.CloudInterface{} + cloud.Mock.On("GetSubscription", mock.Anything).Return(subscription, nil) - cloudImpl := th.App.Srv().Cloud - th.App.Srv().Cloud = &cloud - defer func() { - th.App.Srv().Cloud = cloudImpl - }() + th.App.Srv().Cloud = &cloud + cloudImpl := th.App.Srv().Cloud + defer func() { + th.App.Srv().Cloud = cloudImpl + }() - resp, err := th.SystemAdminClient.DoAPIPost("/license/review", "") - require.Nil(t, err) - require.Equal(t, http.StatusOK, resp.StatusCode) + resp, err := th.SystemAdminClient.DoAPIPost("/license/review", "") + require.Nil(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) + }) + + t.Run("returns 500 when data extraction fails", func(t *testing.T) { + cloud := mocks.CloudInterface{} + cloud.Mock.On("GetSubscription", mock.Anything).Return(nil, errors.New("Could not get subscription")) + + th.App.Srv().Cloud = &cloud + cloudImpl := th.App.Srv().Cloud + defer func() { + th.App.Srv().Cloud = cloudImpl + }() + + resp, err := th.SystemAdminClient.DoAPIPost("/license/review", "") + require.NotNil(t, err) + require.Equal(t, http.StatusInternalServerError, resp.StatusCode) + }) + + t.Run("returns 403 when user does not have permissions", func(t *testing.T) { + resp, err := th.Client.DoAPIPost("/license/review", "") + require.NotNil(t, err) + require.Equal(t, http.StatusForbidden, resp.StatusCode) + }) } From e4593998bc69c6d5de2d1fe4fcb79f5458998a25 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Mon, 19 Dec 2022 17:40:37 -0500 Subject: [PATCH 09/58] Fix error checks in tests. --- api4/license_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api4/license_test.go b/api4/license_test.go index a9df54c8ea..e5560827d3 100644 --- a/api4/license_test.go +++ b/api4/license_test.go @@ -364,7 +364,7 @@ func TestRequestTrueUpReview(t *testing.T) { }() resp, err := th.SystemAdminClient.DoAPIPost("/license/review", "") - require.Nil(t, err) + require.NoError(t, err) require.Equal(t, http.StatusOK, resp.StatusCode) }) @@ -379,13 +379,13 @@ func TestRequestTrueUpReview(t *testing.T) { }() resp, err := th.SystemAdminClient.DoAPIPost("/license/review", "") - require.NotNil(t, err) + require.Error(t, err) require.Equal(t, http.StatusInternalServerError, resp.StatusCode) }) t.Run("returns 403 when user does not have permissions", func(t *testing.T) { resp, err := th.Client.DoAPIPost("/license/review", "") - require.NotNil(t, err) + require.Error(t, err) require.Equal(t, http.StatusForbidden, resp.StatusCode) }) } From 9c4ce9016ff4627341472ad54c894c84203c8d62 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Tue, 20 Dec 2022 09:18:20 -0500 Subject: [PATCH 10/58] Fix error check placement. --- api4/license.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/api4/license.go b/api4/license.go index 6240be1190..9233e3751f 100644 --- a/api4/license.go +++ b/api4/license.go @@ -403,16 +403,15 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { // Convert true up review profile struct to map var telemetryProperties map[string]interface{} reviewProfileJson, err := json.Marshal(reviewProfile) - json.Unmarshal(reviewProfileJson, &telemetryProperties) - - // Send telemetry data. - telemetryService := c.App.Srv().GetTelemetryService() - telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, telemetryProperties) - if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return } + // Send telemetry data. + json.Unmarshal(reviewProfileJson, &telemetryProperties) + telemetryService := c.App.Srv().GetTelemetryService() + telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, telemetryProperties) + ReturnStatusOK(w) } From 68647175047231b10d94ea6aeb30667cbc300587 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Tue, 20 Dec 2022 09:42:01 -0500 Subject: [PATCH 11/58] Add nil checks for cloud in case of air-gapped instances? --- api4/license.go | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/api4/license.go b/api4/license.go index 9233e3751f..d377769bbe 100644 --- a/api4/license.go +++ b/api4/license.go @@ -307,23 +307,21 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { return } - if c.App.Cloud() == nil { - c.Err = model.NewAppError("requestRenewalLink", "api.license.upgrade_needed.app_error", nil, "", http.StatusForbidden) - return - } - license := c.App.Channels().License() if license == nil { http.Error(w, "A License is required to perform a true-up review", http.StatusBadRequest) return } - // Subscription Data - userId := c.AppContext.Session().UserId - subscription, err := c.App.Cloud().GetSubscription(userId) - if err != nil || subscription == nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return + var subscription *model.Subscription + if c.App.Cloud() != nil { + // Subscription Data + userId := c.AppContext.Session().UserId + subscription, err := c.App.Cloud().GetSubscription(userId) + if err != nil || subscription == nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } } // Customer Info & Usage Analytics @@ -385,12 +383,16 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { } } + seats := 0 + if subscription != nil { + seats = subscription.Seats + } reviewProfile := model.TrueUpReviewProfile{ ServerId: c.App.TelemetryId(), ServerVersion: model.CurrentVersion, ServerInstallationType: os.Getenv(telemetry.EnvVarInstallType), LicenseId: license.Id, - LicensedSeats: subscription.Seats, + LicensedSeats: seats, LicensePlan: license.SkuName, CustomerName: license.Customer.Name, ActiveUsers: activeUserCount, From 197262bc7062b2738f1f82bc3655573cfa92fc47 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Tue, 20 Dec 2022 09:43:07 -0500 Subject: [PATCH 12/58] Move seats data extraction closer to subscription definition. --- api4/license.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/api4/license.go b/api4/license.go index d377769bbe..2a86243224 100644 --- a/api4/license.go +++ b/api4/license.go @@ -324,6 +324,11 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { } } + seats := 0 + if subscription != nil { + seats = subscription.Seats + } + // Customer Info & Usage Analytics activeUserCount, err := c.App.Srv().Store().Status().GetTotalActiveUsersCount() if err != nil { @@ -383,10 +388,6 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { } } - seats := 0 - if subscription != nil { - seats = subscription.Seats - } reviewProfile := model.TrueUpReviewProfile{ ServerId: c.App.TelemetryId(), ServerVersion: model.CurrentVersion, From 589ae56c2960301cfffd1c858d5e5e36c0f9a3b7 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Tue, 20 Dec 2022 09:53:20 -0500 Subject: [PATCH 13/58] Remove shadowing declaration of subscription. --- api4/license.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api4/license.go b/api4/license.go index 2a86243224..1badba597c 100644 --- a/api4/license.go +++ b/api4/license.go @@ -314,10 +314,11 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { } var subscription *model.Subscription + var err error if c.App.Cloud() != nil { // Subscription Data userId := c.AppContext.Session().UserId - subscription, err := c.App.Cloud().GetSubscription(userId) + subscription, err = c.App.Cloud().GetSubscription(userId) if err != nil || subscription == nil { http.Error(w, err.Error(), http.StatusInternalServerError) return From 95594c03740a654d3171d1523210d847931b7059 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Tue, 20 Dec 2022 14:22:39 -0500 Subject: [PATCH 14/58] respond with review profile for use in bundle download. --- api4/license.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api4/license.go b/api4/license.go index 1badba597c..fb5358ee24 100644 --- a/api4/license.go +++ b/api4/license.go @@ -417,5 +417,5 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { telemetryService := c.App.Srv().GetTelemetryService() telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, telemetryProperties) - ReturnStatusOK(w) + w.Write([]byte(reviewProfileJson)) } From a64be851c0498fc08d5dacf6832c4a96640651c8 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Tue, 20 Dec 2022 14:43:37 -0500 Subject: [PATCH 15/58] Remove unneeded conversion. --- api4/license.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api4/license.go b/api4/license.go index fb5358ee24..449df3a70a 100644 --- a/api4/license.go +++ b/api4/license.go @@ -417,5 +417,5 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { telemetryService := c.App.Srv().GetTelemetryService() telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, telemetryProperties) - w.Write([]byte(reviewProfileJson)) + w.Write(reviewProfileJson) } From 77d3e3591a961518408569c6a31ca44ad19ebae7 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Wed, 21 Dec 2022 11:54:45 -0500 Subject: [PATCH 16/58] Add minor code review changes. --- api4/license.go | 15 ++++++++------- model/license.go | 6 +++--- model/true_up_review_profile.go | 2 +- services/telemetry/telemetry.go | 2 -- 4 files changed, 12 insertions(+), 13 deletions(-) diff --git a/api4/license.go b/api4/license.go index 449df3a70a..e39ef3051b 100644 --- a/api4/license.go +++ b/api4/license.go @@ -368,16 +368,17 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { } // Authentication Features - mfaUsed := c.App.Config().ServiceSettings.EnforceMultifactorAuthentication - ldapUsed := c.App.Config().LdapSettings.Enable - samlUsed := c.App.Config().SamlSettings.Enable - openIdUsed := c.App.Config().OpenIdSettings.Enable - guessAccessAllowed := c.App.Config().GuestAccountsSettings.Enable + config := c.App.Config() + mfaUsed := config.ServiceSettings.EnforceMultifactorAuthentication + ldapUsed := config.LdapSettings.Enable + samlUsed := config.SamlSettings.Enable + openIdUsed := config.OpenIdSettings.Enable + guessAccessAllowed := config.GuestAccountsSettings.Enable authFeatures := map[string]*bool{ model.TrueUpReviewAuthFeaturesMfa: mfaUsed, - model.TueUpReviewAuthFeaturesAdLdap: ldapUsed, - model.TrueUpReviewauthFeaturesSaml: samlUsed, + model.TrueUpReviewAuthFeaturesADLdap: ldapUsed, + model.TrueUpReviewAuthFeaturesSaml: samlUsed, model.TrueUpReviewAuthFeatureOpenId: openIdUsed, model.TrueUpReviewAuthFeatureGuestAccess: guessAccessAllowed, } diff --git a/model/license.go b/model/license.go index ef7d2e4ffd..b0cb95c809 100644 --- a/model/license.go +++ b/model/license.go @@ -40,10 +40,10 @@ var ( ) const ( - TrueUpReviewTelemetryName = "true-up-review-sent" + TrueUpReviewTelemetryName = "true_up_review_sent" TrueUpReviewAuthFeaturesMfa = "multi_factor_authentication" - TueUpReviewAuthFeaturesAdLdap = "ad_ldap_sign_in" - TrueUpReviewauthFeaturesSaml = "saml_sign_in" + TrueUpReviewAuthFeaturesADLdap = "ad_ldap_sign_in" + TrueUpReviewAuthFeaturesSaml = "saml_sign_in" TrueUpReviewAuthFeatureOpenId = "openid_connect" TrueUpReviewAuthFeatureGuestAccess = "guest_access" ) diff --git a/model/true_up_review_profile.go b/model/true_up_review_profile.go index 91e8038f85..54cfb3f9af 100644 --- a/model/true_up_review_profile.go +++ b/model/true_up_review_profile.go @@ -7,7 +7,7 @@ type TrueUpReviewProfile struct { ServerId string `json:"server_id"` ServerVersion string `json:"server_version"` ServerInstallationType string `json:"server_installation_type"` - LicenseId string `json:"licnes_id"` + LicenseId string `json:"license_id"` LicensedSeats int `json:"licensed_seats"` LicensePlan string `json:"license_plan"` CustomerName string `json:"customer_name"` diff --git a/services/telemetry/telemetry.go b/services/telemetry/telemetry.go index d0e8f91a19..ec88b868c0 100644 --- a/services/telemetry/telemetry.go +++ b/services/telemetry/telemetry.go @@ -82,8 +82,6 @@ const ( TrackLicense = "license" TrackServer = "server" TrackPlugins = "plugins" - - TrackTrueUpReview = "true_up_review" ) type ServerIface interface { From f65aa6144069b871a441e4c08abdae2075e8cf69 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Wed, 21 Dec 2022 12:09:33 -0500 Subject: [PATCH 17/58] remove webhook queries in favor of using existing ones that ignore deleted webhooks, enforce not implemented for cloud users, get seats from license.features.users. --- api4/license.go | 24 ++++--------- store/opentracinglayer/opentracinglayer.go | 36 ------------------- store/retrylayer/retrylayer.go | 42 ---------------------- store/sqlstore/webhook_store.go | 36 ------------------- store/store.go | 3 -- store/storetest/mocks/WebhookStore.go | 42 ---------------------- store/timerlayer/timerlayer.go | 32 ----------------- 7 files changed, 6 insertions(+), 209 deletions(-) diff --git a/api4/license.go b/api4/license.go index e39ef3051b..8d41113e70 100644 --- a/api4/license.go +++ b/api4/license.go @@ -313,21 +313,9 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { return } - var subscription *model.Subscription - var err error - if c.App.Cloud() != nil { - // Subscription Data - userId := c.AppContext.Session().UserId - subscription, err = c.App.Cloud().GetSubscription(userId) - if err != nil || subscription == nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - } - - seats := 0 - if subscription != nil { - seats = subscription.Seats + if c.App.Cloud() == nil { + c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "app.job.true_up_review_not_allowd", nil, "", http.StatusNotImplemented) + return } // Customer Info & Usage Analytics @@ -338,12 +326,12 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { } // Webhook, calls, boards, and playbook counts - incomingWebhookCount, err := c.App.Srv().Store().Webhook().GetIncomingTotal() + incomingWebhookCount, err := c.App.Srv().Store().Webhook().AnalyticsIncomingCount("") if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return } - outgoingWebhookCount, err := c.App.Srv().Store().Webhook().GetOutgoingTotal() + outgoingWebhookCount, err := c.App.Srv().Store().Webhook().AnalyticsOutgoingCount("") if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -395,7 +383,7 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { ServerVersion: model.CurrentVersion, ServerInstallationType: os.Getenv(telemetry.EnvVarInstallType), LicenseId: license.Id, - LicensedSeats: seats, + LicensedSeats: *license.Features.Users, LicensePlan: license.SkuName, CustomerName: license.Customer.Name, ActiveUsers: activeUserCount, diff --git a/store/opentracinglayer/opentracinglayer.go b/store/opentracinglayer/opentracinglayer.go index 546e0664d6..87b443640a 100644 --- a/store/opentracinglayer/opentracinglayer.go +++ b/store/opentracinglayer/opentracinglayer.go @@ -12480,24 +12480,6 @@ func (s *OpenTracingLayerWebhookStore) GetIncomingListByUser(userID string, offs return result, err } -func (s *OpenTracingLayerWebhookStore) GetIncomingTotal() (int64, error) { - origCtx := s.Root.Store.Context() - span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "WebhookStore.GetIncomingTotal") - s.Root.Store.SetContext(newCtx) - defer func() { - s.Root.Store.SetContext(origCtx) - }() - - defer span.Finish() - result, err := s.WebhookStore.GetIncomingTotal() - if err != nil { - span.LogFields(spanlog.Error(err)) - ext.Error.Set(span, true) - } - - return result, err -} - func (s *OpenTracingLayerWebhookStore) GetOutgoing(id string) (*model.OutgoingWebhook, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "WebhookStore.GetOutgoing") @@ -12624,24 +12606,6 @@ func (s *OpenTracingLayerWebhookStore) GetOutgoingListByUser(userID string, offs return result, err } -func (s *OpenTracingLayerWebhookStore) GetOutgoingTotal() (int64, error) { - origCtx := s.Root.Store.Context() - span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "WebhookStore.GetOutgoingTotal") - s.Root.Store.SetContext(newCtx) - defer func() { - s.Root.Store.SetContext(origCtx) - }() - - defer span.Finish() - result, err := s.WebhookStore.GetOutgoingTotal() - if err != nil { - span.LogFields(spanlog.Error(err)) - ext.Error.Set(span, true) - } - - return result, err -} - func (s *OpenTracingLayerWebhookStore) InvalidateWebhookCache(webhook string) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "WebhookStore.InvalidateWebhookCache") diff --git a/store/retrylayer/retrylayer.go b/store/retrylayer/retrylayer.go index e23160cf77..38a3f52dbf 100644 --- a/store/retrylayer/retrylayer.go +++ b/store/retrylayer/retrylayer.go @@ -14232,27 +14232,6 @@ func (s *RetryLayerWebhookStore) GetIncomingListByUser(userID string, offset int } -func (s *RetryLayerWebhookStore) GetIncomingTotal() (int64, error) { - - tries := 0 - for { - result, err := s.WebhookStore.GetIncomingTotal() - if err == nil { - return result, nil - } - if !isRepeatableError(err) { - return result, err - } - tries++ - if tries >= 3 { - err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") - return result, err - } - timepkg.Sleep(100 * timepkg.Millisecond) - } - -} - func (s *RetryLayerWebhookStore) GetOutgoing(id string) (*model.OutgoingWebhook, error) { tries := 0 @@ -14400,27 +14379,6 @@ func (s *RetryLayerWebhookStore) GetOutgoingListByUser(userID string, offset int } -func (s *RetryLayerWebhookStore) GetOutgoingTotal() (int64, error) { - - tries := 0 - for { - result, err := s.WebhookStore.GetOutgoingTotal() - if err == nil { - return result, nil - } - if !isRepeatableError(err) { - return result, err - } - tries++ - if tries >= 3 { - err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") - return result, err - } - timepkg.Sleep(100 * timepkg.Millisecond) - } - -} - func (s *RetryLayerWebhookStore) InvalidateWebhookCache(webhook string) { s.WebhookStore.InvalidateWebhookCache(webhook) diff --git a/store/sqlstore/webhook_store.go b/store/sqlstore/webhook_store.go index df10c2f1a0..d9f08120ac 100644 --- a/store/sqlstore/webhook_store.go +++ b/store/sqlstore/webhook_store.go @@ -400,39 +400,3 @@ func (s SqlWebhookStore) AnalyticsOutgoingCount(teamId string) (int64, error) { } return count, nil } - -func (s SqlWebhookStore) GetIncomingTotal() (int64, error) { - queryBuilder := - s.getQueryBuilder(). - Select("COUNT(*)"). - From("IncomingWebhooks") - - queryString, args, err := queryBuilder.ToSql() - if err != nil { - return 0, errors.Wrap(err, "incoming_webhook_tosql") - } - - var count int64 - if err := s.GetReplicaX().Get(&count, queryString, args...); err != nil { - return 0, errors.Wrap(err, "failed to count total IncomingWebooks") - } - return count, nil -} - -func (s SqlWebhookStore) GetOutgoingTotal() (int64, error) { - queryBuilder := - s.getQueryBuilder(). - Select("COUNT(*)"). - From("OutgoingWebhooks") - - queryString, args, err := queryBuilder.ToSql() - if err != nil { - return 0, errors.Wrap(err, "outgoing_webhook_tosql") - } - - var count int64 - if err := s.GetReplicaX().Get(&count, queryString, args...); err != nil { - return 0, errors.Wrap(err, "failed to count total OutgoingWebhooks") - } - return count, nil -} diff --git a/store/store.go b/store/store.go index b57aeba676..49490afdbb 100644 --- a/store/store.go +++ b/store/store.go @@ -615,9 +615,6 @@ type WebhookStore interface { AnalyticsOutgoingCount(teamID string) (int64, error) InvalidateWebhookCache(webhook string) ClearCaches() - - GetOutgoingTotal() (int64, error) - GetIncomingTotal() (int64, error) } type CommandStore interface { diff --git a/store/storetest/mocks/WebhookStore.go b/store/storetest/mocks/WebhookStore.go index b6ce04cc3d..5abfc36738 100644 --- a/store/storetest/mocks/WebhookStore.go +++ b/store/storetest/mocks/WebhookStore.go @@ -227,27 +227,6 @@ func (_m *WebhookStore) GetIncomingListByUser(userID string, offset int, limit i return r0, r1 } -// GetIncomingTotal provides a mock function with given fields: -func (_m *WebhookStore) GetIncomingTotal() (int64, error) { - ret := _m.Called() - - var r0 int64 - if rf, ok := ret.Get(0).(func() int64); ok { - r0 = rf() - } else { - r0 = ret.Get(0).(int64) - } - - var r1 error - if rf, ok := ret.Get(1).(func() error); ok { - r1 = rf() - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - // GetOutgoing provides a mock function with given fields: id func (_m *WebhookStore) GetOutgoing(id string) (*model.OutgoingWebhook, error) { ret := _m.Called(id) @@ -409,27 +388,6 @@ func (_m *WebhookStore) GetOutgoingListByUser(userID string, offset int, limit i return r0, r1 } -// GetOutgoingTotal provides a mock function with given fields: -func (_m *WebhookStore) GetOutgoingTotal() (int64, error) { - ret := _m.Called() - - var r0 int64 - if rf, ok := ret.Get(0).(func() int64); ok { - r0 = rf() - } else { - r0 = ret.Get(0).(int64) - } - - var r1 error - if rf, ok := ret.Get(1).(func() error); ok { - r1 = rf() - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - // InvalidateWebhookCache provides a mock function with given fields: webhook func (_m *WebhookStore) InvalidateWebhookCache(webhook string) { _m.Called(webhook) diff --git a/store/timerlayer/timerlayer.go b/store/timerlayer/timerlayer.go index 8161ad126a..02be7b110d 100644 --- a/store/timerlayer/timerlayer.go +++ b/store/timerlayer/timerlayer.go @@ -11240,22 +11240,6 @@ func (s *TimerLayerWebhookStore) GetIncomingListByUser(userID string, offset int return result, err } -func (s *TimerLayerWebhookStore) GetIncomingTotal() (int64, error) { - start := time.Now() - - result, err := s.WebhookStore.GetIncomingTotal() - - elapsed := float64(time.Since(start)) / float64(time.Second) - if s.Root.Metrics != nil { - success := "false" - if err == nil { - success = "true" - } - s.Root.Metrics.ObserveStoreMethodDuration("WebhookStore.GetIncomingTotal", success, elapsed) - } - return result, err -} - func (s *TimerLayerWebhookStore) GetOutgoing(id string) (*model.OutgoingWebhook, error) { start := time.Now() @@ -11368,22 +11352,6 @@ func (s *TimerLayerWebhookStore) GetOutgoingListByUser(userID string, offset int return result, err } -func (s *TimerLayerWebhookStore) GetOutgoingTotal() (int64, error) { - start := time.Now() - - result, err := s.WebhookStore.GetOutgoingTotal() - - elapsed := float64(time.Since(start)) / float64(time.Second) - if s.Root.Metrics != nil { - success := "false" - if err == nil { - success = "true" - } - s.Root.Metrics.ObserveStoreMethodDuration("WebhookStore.GetOutgoingTotal", success, elapsed) - } - return result, err -} - func (s *TimerLayerWebhookStore) InvalidateWebhookCache(webhook string) { start := time.Now() From 518ba17920ff4641b2581c9e9a164d0965a7801c Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Wed, 21 Dec 2022 12:21:35 -0500 Subject: [PATCH 18/58] Ensure telemetry data is sent as a flat map, rather than a structured one. --- api4/license.go | 6 ++++++ model/true_up_review_profile.go | 9 +++++++++ 2 files changed, 15 insertions(+) diff --git a/api4/license.go b/api4/license.go index 8d41113e70..143e84bea0 100644 --- a/api4/license.go +++ b/api4/license.go @@ -403,6 +403,12 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { // Send telemetry data. json.Unmarshal(reviewProfileJson, &telemetryProperties) + delete(telemetryProperties, "plugins") + plugins := reviewProfile.Plugins.ToMap() + for pluginName, pluginValue := range plugins { + telemetryProperties[pluginName] = pluginValue + } + telemetryService := c.App.Srv().GetTelemetryService() telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, telemetryProperties) diff --git a/model/true_up_review_profile.go b/model/true_up_review_profile.go index 54cfb3f9af..8c72ffad71 100644 --- a/model/true_up_review_profile.go +++ b/model/true_up_review_profile.go @@ -24,3 +24,12 @@ type TrueUpReviewPlugins struct { ActivePluginNames []string `json:"active_plugin_names"` InactivePluginNames []string `json:"inactive_plugin_names"` } + +func (t *TrueUpReviewPlugins) ToMap() map[string]any { + return map[string]any{ + "total_active_plugins": t.TotalActivePlugins, + "total_inactive_plugins": t.TotalInactivePlugins, + "active_plugin_names": t.ActivePluginNames, + "inactive_plugin_names": t.InactivePluginNames, + } +} From 91bc4b3c80f3fd89448dfea66d5476f6e4d9586c Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Wed, 21 Dec 2022 15:02:24 -0500 Subject: [PATCH 19/58] Fix tests, flip login for cloud check. --- api4/license.go | 2 +- api4/license_test.go | 41 +++++++++++------------------------------ 2 files changed, 12 insertions(+), 31 deletions(-) diff --git a/api4/license.go b/api4/license.go index 143e84bea0..4e20d0a67f 100644 --- a/api4/license.go +++ b/api4/license.go @@ -313,7 +313,7 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { return } - if c.App.Cloud() == nil { + if c.App.Cloud() != nil { c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "app.job.true_up_review_not_allowd", nil, "", http.StatusNotImplemented) return } diff --git a/api4/license_test.go b/api4/license_test.go index e5560827d3..5b9ff528d8 100644 --- a/api4/license_test.go +++ b/api4/license_test.go @@ -5,7 +5,6 @@ package api4 import ( "encoding/json" - "errors" "net/http" "net/http/httptest" "testing" @@ -336,51 +335,25 @@ func TestRequestTrueUpReview(t *testing.T) { th := Setup(t) defer th.TearDown() - subscription := &model.Subscription{ - ID: "MySubscriptionID", - CustomerID: "MyCustomer", - ProductID: "SomeProductId", - AddOns: []string{}, - StartAt: 1000000000, - EndAt: 2000000000, - CreateAt: 1000000000, - Seats: 10, - IsFreeTrial: "true", - DNS: "some.dns.server", - TrialEndAt: 2000000000, - LastInvoice: &model.Invoice{}, - } - th.App.Srv().SetLicense(model.NewTestLicense()) t.Run("returns status 200 when telemetry data sent", func(t *testing.T) { - cloud := mocks.CloudInterface{} - cloud.Mock.On("GetSubscription", mock.Anything).Return(subscription, nil) - - th.App.Srv().Cloud = &cloud - cloudImpl := th.App.Srv().Cloud - defer func() { - th.App.Srv().Cloud = cloudImpl - }() - resp, err := th.SystemAdminClient.DoAPIPost("/license/review", "") require.NoError(t, err) require.Equal(t, http.StatusOK, resp.StatusCode) }) - t.Run("returns 500 when data extraction fails", func(t *testing.T) { + t.Run("returns 501 when ran by cloud user", func(t *testing.T) { cloud := mocks.CloudInterface{} - cloud.Mock.On("GetSubscription", mock.Anything).Return(nil, errors.New("Could not get subscription")) - - th.App.Srv().Cloud = &cloud cloudImpl := th.App.Srv().Cloud + th.App.Srv().Cloud = &cloud defer func() { th.App.Srv().Cloud = cloudImpl }() resp, err := th.SystemAdminClient.DoAPIPost("/license/review", "") require.Error(t, err) - require.Equal(t, http.StatusInternalServerError, resp.StatusCode) + require.Equal(t, http.StatusNotImplemented, resp.StatusCode) }) t.Run("returns 403 when user does not have permissions", func(t *testing.T) { @@ -388,4 +361,12 @@ func TestRequestTrueUpReview(t *testing.T) { require.Error(t, err) require.Equal(t, http.StatusForbidden, resp.StatusCode) }) + + t.Run("returns 400 when license is nil", func(t *testing.T) { + th.App.Srv().SetLicense(nil) + + resp, err := th.SystemAdminClient.DoAPIPost("/license/review", "") + require.Error(t, err) + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + }) } From 88090ea40b539fac0fd543270bc615475e1f13e8 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Thu, 22 Dec 2022 14:59:05 -0500 Subject: [PATCH 20/58] Add true up review status history, add migration for persisting history, add store to create, get, and update true up review history entries, cleanup errors, etc. --- api4/license.go | 78 +++++++++++++++---- api4/license_test.go | 42 +++++++++- db/migrations/migrations.list | 2 + ...101_create_true_up_review_history.down.sql | 1 + ...00101_create_true_up_review_history.up.sql | 5 ++ ...101_create_true_up_review_history.down.sql | 1 + ...00101_create_true_up_review_history.up.sql | 5 ++ i18n/en.json | 24 ++++++ model/true_up_review_profile.go | 12 +++ store/opentracinglayer/opentracinglayer.go | 47 +++++++++++ store/retrylayer/retrylayer.go | 53 +++++++++++++ store/sqlstore/store.go | 6 ++ store/sqlstore/true_up_review_store.go | 77 ++++++++++++++++++ store/store.go | 6 ++ store/storetest/mocks/Store.go | 16 ++++ store/storetest/mocks/TrueUpReviewStore.go | 61 +++++++++++++++ store/storetest/store.go | 4 + store/timerlayer/timerlayer.go | 43 ++++++++++ utils/license.go | 22 ++++++ 19 files changed, 488 insertions(+), 17 deletions(-) create mode 100644 db/migrations/mysql/000101_create_true_up_review_history.down.sql create mode 100644 db/migrations/mysql/000101_create_true_up_review_history.up.sql create mode 100644 db/migrations/postgres/000101_create_true_up_review_history.down.sql create mode 100644 db/migrations/postgres/000101_create_true_up_review_history.up.sql create mode 100644 store/sqlstore/true_up_review_store.go create mode 100644 store/storetest/mocks/TrueUpReviewStore.go diff --git a/api4/license.go b/api4/license.go index 4e20d0a67f..4e1a402ad4 100644 --- a/api4/license.go +++ b/api4/license.go @@ -26,7 +26,8 @@ func (api *API) InitLicense() { api.BaseRoutes.APIRoot.Handle("/license", api.APISessionRequired(removeLicense)).Methods("DELETE") api.BaseRoutes.APIRoot.Handle("/license/renewal", api.APISessionRequired(requestRenewalLink)).Methods("GET") api.BaseRoutes.APIRoot.Handle("/license/client", api.APIHandler(getClientLicense)).Methods("GET") - api.BaseRoutes.APIRoot.Handle("/license/review", api.APIHandler(requestTrueUpReview)).Methods("POST") + api.BaseRoutes.APIRoot.Handle("/license/review", api.APISessionRequired(requestTrueUpReview)).Methods("POST") + api.BaseRoutes.APIRoot.Handle("/license/review/status", api.APISessionRequired(trueUpReviewStatus)).Methods("GET") } func getClientLicense(c *Context, w http.ResponseWriter, r *http.Request) { @@ -309,19 +310,19 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { license := c.App.Channels().License() if license == nil { - http.Error(w, "A License is required to perform a true-up review", http.StatusBadRequest) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.license.required", nil, "", http.StatusNotImplemented) return } if c.App.Cloud() != nil { - c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "app.job.true_up_review_not_allowd", nil, "", http.StatusNotImplemented) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.not.allowed.for.cloud", nil, "", http.StatusNotImplemented) return } // Customer Info & Usage Analytics activeUserCount, err := c.App.Srv().Store().Status().GetTotalActiveUsersCount() if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.user.count.fail", nil, "", http.StatusInternalServerError) return } @@ -329,11 +330,12 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { incomingWebhookCount, err := c.App.Srv().Store().Webhook().AnalyticsIncomingCount("") if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.webhook.in.count.fail", nil, "", http.StatusInternalServerError) return } outgoingWebhookCount, err := c.App.Srv().Store().Webhook().AnalyticsOutgoingCount("") if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.webhook.out.count.fail", nil, "", http.StatusInternalServerError) return } @@ -346,13 +348,13 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { if pluginResponse, err := c.App.GetPlugins(); err == nil { for _, plugin := range pluginResponse.Active { trueUpReviewPlugins.ActivePluginNames = append(trueUpReviewPlugins.ActivePluginNames, plugin.Name) - trueUpReviewPlugins.TotalActivePlugins += 1 } + trueUpReviewPlugins.TotalActivePlugins = len(trueUpReviewPlugins.ActivePluginNames) for _, plugin := range pluginResponse.Inactive { trueUpReviewPlugins.InactivePluginNames = append(trueUpReviewPlugins.InactivePluginNames, plugin.Name) - trueUpReviewPlugins.TotalInactivePlugins += 1 } + trueUpReviewPlugins.TotalInactivePlugins = len(trueUpReviewPlugins.InactivePluginNames) } // Authentication Features @@ -397,20 +399,64 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { var telemetryProperties map[string]interface{} reviewProfileJson, err := json.Marshal(reviewProfile) if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) + c.Err = model.NewAppError("requestTrueUpReview", "api.marshal_error", nil, "", http.StatusInternalServerError) return } - // Send telemetry data. - json.Unmarshal(reviewProfileJson, &telemetryProperties) - delete(telemetryProperties, "plugins") - plugins := reviewProfile.Plugins.ToMap() - for pluginName, pluginValue := range plugins { - telemetryProperties[pluginName] = pluginValue + dueDate := utils.GetNextTrueUpReviewDueDate() + status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(dueDate) + if err != nil { + c.Err = model.NewAppError("trueUpReviewStatus", "api.license.true_up_review.get.fail.app_error", nil, "", http.StatusInternalServerError) + return } - telemetryService := c.App.Srv().GetTelemetryService() - telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, telemetryProperties) + // Do not send true-up review data if the user has already requested one for the quarter. + if !status.Completed { + // Send telemetry data. + json.Unmarshal(reviewProfileJson, &telemetryProperties) + delete(telemetryProperties, "plugins") + plugins := reviewProfile.Plugins.ToMap() + for pluginName, pluginValue := range plugins { + telemetryProperties[pluginName] = pluginValue + } + + telemetryService := c.App.Srv().GetTelemetryService() + telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, telemetryProperties) + } w.Write(reviewProfileJson) } + +func trueUpReviewStatus(c *Context, w http.ResponseWriter, r *http.Request) { + // Only admins can request a true up review. + if !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionManageSystem) { + c.SetPermissionError(model.PermissionManageLicenseInformation) + return + } + + license := c.App.Channels().License() + if license == nil { + c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "api.license.true_up_review.license.required", nil, "", http.StatusNotImplemented) + return + } + + if c.App.Cloud() != nil { + c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "api.license.true_up_review.not.allowed.for.cloud", nil, "", http.StatusNotImplemented) + return + } + + nextDueDate := utils.GetNextTrueUpReviewDueDate() + status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(nextDueDate) + if err != nil { + c.Err = model.NewAppError("trueUpReviewStatus", "api.license.true_up_review.get.fail.app_error", nil, "", http.StatusInternalServerError) + return + } + + json, err := json.Marshal(status) + if err != nil { + c.Err = model.NewAppError("trueUpReviewStatus", "api.marshal_error", nil, "", http.StatusInternalServerError) + return + } + + w.Write(json) +} diff --git a/api4/license_test.go b/api4/license_test.go index 5b9ff528d8..7b3a2e9afd 100644 --- a/api4/license_test.go +++ b/api4/license_test.go @@ -367,6 +367,46 @@ func TestRequestTrueUpReview(t *testing.T) { resp, err := th.SystemAdminClient.DoAPIPost("/license/review", "") require.Error(t, err) - require.Equal(t, http.StatusBadRequest, resp.StatusCode) + require.Equal(t, http.StatusNotImplemented, resp.StatusCode) + }) +} + +func TestTrueUpReviewStatus(t *testing.T) { + th := Setup(t) + + defer th.TearDown() + th.App.Srv().SetLicense(model.NewTestLicense()) + + t.Run("returns 200 when status retrieved", func(t *testing.T) { + resp, err := th.SystemAdminClient.DoAPIGet("/license/review/status", "") + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) + }) + + t.Run("returns 501 when ran by cloud user", func(t *testing.T) { + cloud := mocks.CloudInterface{} + cloudImpl := th.App.Srv().Cloud + th.App.Srv().Cloud = &cloud + defer func() { + th.App.Srv().Cloud = cloudImpl + }() + + resp, err := th.SystemAdminClient.DoAPIGet("/license/review/status", "") + require.Error(t, err) + require.Equal(t, http.StatusNotImplemented, resp.StatusCode) + }) + + t.Run("returns 403 when user does not have permissions", func(t *testing.T) { + resp, err := th.Client.DoAPIGet("/license/review/status", "") + require.Error(t, err) + require.Equal(t, http.StatusForbidden, resp.StatusCode) + }) + + t.Run("returns 400 when license is nil", func(t *testing.T) { + th.App.Srv().SetLicense(nil) + + resp, err := th.SystemAdminClient.DoAPIGet("/license/review/status", "") + require.Error(t, err) + require.Equal(t, http.StatusNotImplemented, resp.StatusCode) }) } diff --git a/db/migrations/migrations.list b/db/migrations/migrations.list index a7fffb38f3..ffaa2e283e 100644 --- a/db/migrations/migrations.list +++ b/db/migrations/migrations.list @@ -400,3 +400,5 @@ db/migrations/postgres/000099_create_drafts.down.sql db/migrations/postgres/000099_create_drafts.up.sql db/migrations/postgres/000100_add_draft_priority_column.down.sql db/migrations/postgres/000100_add_draft_priority_column.up.sql +db/migrations/postgres/000101_create_true_up_review_history.down.sql +db/migrations/postgres/000101_create_true_up_review_history.up.sql diff --git a/db/migrations/mysql/000101_create_true_up_review_history.down.sql b/db/migrations/mysql/000101_create_true_up_review_history.down.sql new file mode 100644 index 0000000000..746a779807 --- /dev/null +++ b/db/migrations/mysql/000101_create_true_up_review_history.down.sql @@ -0,0 +1 @@ +DROP TABLE IF EXISTS trueupreviewhistory; diff --git a/db/migrations/mysql/000101_create_true_up_review_history.up.sql b/db/migrations/mysql/000101_create_true_up_review_history.up.sql new file mode 100644 index 0000000000..1ef65317ab --- /dev/null +++ b/db/migrations/mysql/000101_create_true_up_review_history.up.sql @@ -0,0 +1,5 @@ +CREATE TABLE IF NOT EXISTS trueupreviewhistory ( + duedate VARCHAR(10), + completed boolean, + PRIMARY KEY (duedate) +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; diff --git a/db/migrations/postgres/000101_create_true_up_review_history.down.sql b/db/migrations/postgres/000101_create_true_up_review_history.down.sql new file mode 100644 index 0000000000..746a779807 --- /dev/null +++ b/db/migrations/postgres/000101_create_true_up_review_history.down.sql @@ -0,0 +1 @@ +DROP TABLE IF EXISTS trueupreviewhistory; diff --git a/db/migrations/postgres/000101_create_true_up_review_history.up.sql b/db/migrations/postgres/000101_create_true_up_review_history.up.sql new file mode 100644 index 0000000000..e42a055b92 --- /dev/null +++ b/db/migrations/postgres/000101_create_true_up_review_history.up.sql @@ -0,0 +1,5 @@ +CREATE TABLE IF NOT EXISTS trueupreviewhistory ( + duedate VARCHAR(10), + completed boolean, + PRIMARY KEY (duedate) +); diff --git a/i18n/en.json b/i18n/en.json index 25f8a93b61..a57b63d641 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -2077,6 +2077,30 @@ "id": "api.license.request_trial_license.fail_get_user_count.app_error", "translation": "Unable to get a trial license, please try again or contact with support@mattermost.com. Cannot obtain the number of registered users." }, + { + "id": "api.license.true_up_review.get.fail.app_error", + "translation": "Unable to get true up review records." + }, + { + "id": "api.license.true_up_review.license.required", + "translation": "A license is required to request a true up review." + }, + { + "id": "api.license.true_up_review.not.allowed.for.cloud", + "translation": "A true up review cannot be requested for cloud subscriptions." + }, + { + "id": "api.license.true_up_review.user.count.fail", + "translation": "Unable to get user counts." + }, + { + "id": "api.license.true_up_review.webhook.in.count.fail", + "translation": "Unable to get inbound webhook counts." + }, + { + "id": "api.license.true_up_review.webhook.out.count.fail", + "translation": "Unable to get outbound webhook counts." + }, { "id": "api.license.upgrade_needed.app_error", "translation": "Feature requires an upgrade to Enterprise Edition." diff --git a/model/true_up_review_profile.go b/model/true_up_review_profile.go index 8c72ffad71..f371936d39 100644 --- a/model/true_up_review_profile.go +++ b/model/true_up_review_profile.go @@ -33,3 +33,15 @@ func (t *TrueUpReviewPlugins) ToMap() map[string]any { "inactive_plugin_names": t.InactivePluginNames, } } + +type TrueUpReviewStatus struct { + Completed bool `json:"true_up_review_completed"` + DueDate string `json:"true_up_review_due_date"` +} + +func (t *TrueUpReviewStatus) ToSlice() []interface{} { + return []interface{}{ + t.DueDate, + t.Completed, + } +} diff --git a/store/opentracinglayer/opentracinglayer.go b/store/opentracinglayer/opentracinglayer.go index 87b443640a..1ded368891 100644 --- a/store/opentracinglayer/opentracinglayer.go +++ b/store/opentracinglayer/opentracinglayer.go @@ -55,6 +55,7 @@ type OpenTracingLayer struct { TermsOfServiceStore store.TermsOfServiceStore ThreadStore store.ThreadStore TokenStore store.TokenStore + TrueUpReviewStore store.TrueUpReviewStore UploadSessionStore store.UploadSessionStore UserStore store.UserStore UserAccessTokenStore store.UserAccessTokenStore @@ -206,6 +207,10 @@ func (s *OpenTracingLayer) Token() store.TokenStore { return s.TokenStore } +func (s *OpenTracingLayer) TrueUpReview() store.TrueUpReviewStore { + return s.TrueUpReviewStore +} + func (s *OpenTracingLayer) UploadSession() store.UploadSessionStore { return s.UploadSessionStore } @@ -406,6 +411,11 @@ type OpenTracingLayerTokenStore struct { Root *OpenTracingLayer } +type OpenTracingLayerTrueUpReviewStore struct { + store.TrueUpReviewStore + Root *OpenTracingLayer +} + type OpenTracingLayerUploadSessionStore struct { store.UploadSessionStore Root *OpenTracingLayer @@ -10612,6 +10622,42 @@ func (s *OpenTracingLayerTokenStore) Save(recovery *model.Token) error { return err } +func (s *OpenTracingLayerTrueUpReviewStore) CreateTrueUpReviewStatusRecord(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) { + origCtx := s.Root.Store.Context() + span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "TrueUpReviewStore.CreateTrueUpReviewStatusRecord") + s.Root.Store.SetContext(newCtx) + defer func() { + s.Root.Store.SetContext(origCtx) + }() + + defer span.Finish() + result, err := s.TrueUpReviewStore.CreateTrueUpReviewStatusRecord(reviewStatus) + if err != nil { + span.LogFields(spanlog.Error(err)) + ext.Error.Set(span, true) + } + + return result, err +} + +func (s *OpenTracingLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate string) (*model.TrueUpReviewStatus, error) { + origCtx := s.Root.Store.Context() + span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "TrueUpReviewStore.GetTrueUpReviewStatus") + s.Root.Store.SetContext(newCtx) + defer func() { + s.Root.Store.SetContext(origCtx) + }() + + defer span.Finish() + result, err := s.TrueUpReviewStore.GetTrueUpReviewStatus(dueDate) + if err != nil { + span.LogFields(spanlog.Error(err)) + ext.Error.Set(span, true) + } + + return result, err +} + func (s *OpenTracingLayerUploadSessionStore) Delete(id string) error { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "UploadSessionStore.Delete") @@ -12840,6 +12886,7 @@ func New(childStore store.Store, ctx context.Context) *OpenTracingLayer { newStore.TermsOfServiceStore = &OpenTracingLayerTermsOfServiceStore{TermsOfServiceStore: childStore.TermsOfService(), Root: &newStore} newStore.ThreadStore = &OpenTracingLayerThreadStore{ThreadStore: childStore.Thread(), Root: &newStore} newStore.TokenStore = &OpenTracingLayerTokenStore{TokenStore: childStore.Token(), Root: &newStore} + newStore.TrueUpReviewStore = &OpenTracingLayerTrueUpReviewStore{TrueUpReviewStore: childStore.TrueUpReview(), Root: &newStore} newStore.UploadSessionStore = &OpenTracingLayerUploadSessionStore{UploadSessionStore: childStore.UploadSession(), Root: &newStore} newStore.UserStore = &OpenTracingLayerUserStore{UserStore: childStore.User(), Root: &newStore} newStore.UserAccessTokenStore = &OpenTracingLayerUserAccessTokenStore{UserAccessTokenStore: childStore.UserAccessToken(), Root: &newStore} diff --git a/store/retrylayer/retrylayer.go b/store/retrylayer/retrylayer.go index 38a3f52dbf..b9f1c9f8aa 100644 --- a/store/retrylayer/retrylayer.go +++ b/store/retrylayer/retrylayer.go @@ -58,6 +58,7 @@ type RetryLayer struct { TermsOfServiceStore store.TermsOfServiceStore ThreadStore store.ThreadStore TokenStore store.TokenStore + TrueUpReviewStore store.TrueUpReviewStore UploadSessionStore store.UploadSessionStore UserStore store.UserStore UserAccessTokenStore store.UserAccessTokenStore @@ -209,6 +210,10 @@ func (s *RetryLayer) Token() store.TokenStore { return s.TokenStore } +func (s *RetryLayer) TrueUpReview() store.TrueUpReviewStore { + return s.TrueUpReviewStore +} + func (s *RetryLayer) UploadSession() store.UploadSessionStore { return s.UploadSessionStore } @@ -409,6 +414,11 @@ type RetryLayerTokenStore struct { Root *RetryLayer } +type RetryLayerTrueUpReviewStore struct { + store.TrueUpReviewStore + Root *RetryLayer +} + type RetryLayerUploadSessionStore struct { store.UploadSessionStore Root *RetryLayer @@ -12126,6 +12136,48 @@ func (s *RetryLayerTokenStore) Save(recovery *model.Token) error { } +func (s *RetryLayerTrueUpReviewStore) CreateTrueUpReviewStatusRecord(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) { + + tries := 0 + for { + result, err := s.TrueUpReviewStore.CreateTrueUpReviewStatusRecord(reviewStatus) + if err == nil { + return result, nil + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + +func (s *RetryLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate string) (*model.TrueUpReviewStatus, error) { + + tries := 0 + for { + result, err := s.TrueUpReviewStore.GetTrueUpReviewStatus(dueDate) + if err == nil { + return result, nil + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + func (s *RetryLayerUploadSessionStore) Delete(id string) error { tries := 0 @@ -14630,6 +14682,7 @@ func New(childStore store.Store) *RetryLayer { newStore.TermsOfServiceStore = &RetryLayerTermsOfServiceStore{TermsOfServiceStore: childStore.TermsOfService(), Root: &newStore} newStore.ThreadStore = &RetryLayerThreadStore{ThreadStore: childStore.Thread(), Root: &newStore} newStore.TokenStore = &RetryLayerTokenStore{TokenStore: childStore.Token(), Root: &newStore} + newStore.TrueUpReviewStore = &RetryLayerTrueUpReviewStore{TrueUpReviewStore: childStore.TrueUpReview(), Root: &newStore} newStore.UploadSessionStore = &RetryLayerUploadSessionStore{UploadSessionStore: childStore.UploadSession(), Root: &newStore} newStore.UserStore = &RetryLayerUserStore{UserStore: childStore.User(), Root: &newStore} newStore.UserAccessTokenStore = &RetryLayerUserAccessTokenStore{UserAccessTokenStore: childStore.UserAccessToken(), Root: &newStore} diff --git a/store/sqlstore/store.go b/store/sqlstore/store.go index 4c1bf1f05b..bf32b5984b 100644 --- a/store/sqlstore/store.go +++ b/store/sqlstore/store.go @@ -112,6 +112,7 @@ type SqlStoreStores struct { notifyAdmin store.NotifyAdminStore postPriority store.PostPriorityStore postAcknowledgement store.PostAcknowledgementStore + trueUpReviewStatus store.TrueUpReviewStore } type SqlStore struct { @@ -220,6 +221,7 @@ func New(settings model.SqlSettings, metrics einterfaces.MetricsInterface) *SqlS store.stores.notifyAdmin = newSqlNotifyAdminStore(store) store.stores.postPriority = newSqlPostPriorityStore(store) store.stores.postAcknowledgement = newSqlPostAcknowledgementStore(store) + store.stores.trueUpReviewStatus = newSqlTrueUpReviewStore(store) store.stores.preference.(*SqlPreferenceStore).deleteUnusedFeatures() @@ -973,6 +975,10 @@ func (ss *SqlStore) PostAcknowledgement() store.PostAcknowledgementStore { return ss.stores.postAcknowledgement } +func (ss *SqlStore) TrueUpReview() store.TrueUpReviewStore { + return ss.stores.trueUpReviewStatus +} + func (ss *SqlStore) DropAllTables() { if ss.DriverName() == model.DatabaseDriverPostgres { ss.masterX.Exec(`DO diff --git a/store/sqlstore/true_up_review_store.go b/store/sqlstore/true_up_review_store.go new file mode 100644 index 0000000000..87fd29d93f --- /dev/null +++ b/store/sqlstore/true_up_review_store.go @@ -0,0 +1,77 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package sqlstore + +import ( + "github.com/mattermost/mattermost-server/v6/model" + "github.com/mattermost/mattermost-server/v6/store" + sq "github.com/mattermost/squirrel" + "github.com/pkg/errors" +) + +// SqlLicenseStore encapsulates the database writes and reads for +// model.LicenseRecord objects. +type SqlTrueUpReviewStore struct { + *SqlStore +} + +func newSqlTrueUpReviewStore(sqlStore *SqlStore) store.TrueUpReviewStore { + return &SqlTrueUpReviewStore{sqlStore} +} + +func trueUpReviewStatusColumns() []string { + return []string{ + "DueDate", + "Completed", + } +} + +func (s *SqlTrueUpReviewStore) GetTrueUpReviewStatus(dueDate string) (*model.TrueUpReviewStatus, error) { + query := s.getQueryBuilder(). + Select("*"). + From("TrueUpReviewHistory"). + Where(sq.Eq{"DueDate": dueDate}) + + queryString, args, err := query.ToSql() + if err != nil { + return nil, errors.Wrap(err, "get_trueUpReviewStatusRecord_tosql") + } + var trueUpReviewStatus model.TrueUpReviewStatus + if err := s.GetReplicaX().Get(&trueUpReviewStatus, queryString, args...); err != nil { + trueUpReviewStatus.Completed = false + trueUpReviewStatus.DueDate = dueDate + + // If no record is available, create one so there is a record trail. + return s.CreateTrueUpReviewStatusRecord(&trueUpReviewStatus) + } + + return &trueUpReviewStatus, nil +} + +func (s *SqlTrueUpReviewStore) CreateTrueUpReviewStatusRecord(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) { + builder := s.getQueryBuilder().Insert("TrueUpReviewHistory").Columns(trueUpReviewStatusColumns()...).Values(reviewStatus.ToSlice()...) + query, args, err := builder.ToSql() + if err != nil { + return nil, errors.Wrap(err, "create_trueUpReviewStatusRecord_tosql") + } + + if _, err = s.GetMasterX().Exec(query, args...); err != nil { + return nil, errors.Wrap(err, "fail to create true up review status record") + } + + return reviewStatus, nil +} + +func (s *SqlTrueUpReviewStore) Update(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) { + query := s.getQueryBuilder(). + Update("TrueUpReviewHistory"). + Set("Completed", reviewStatus.Completed). + Where(sq.Eq{"DueDate": reviewStatus.DueDate}) + + if _, err := s.GetMasterX().ExecBuilder(query); err != nil { + return nil, errors.Wrapf(err, "failed to update true up review status with DueDate=%s", reviewStatus.DueDate) + } + + return reviewStatus, nil +} diff --git a/store/store.go b/store/store.go index 49490afdbb..346bfee7ca 100644 --- a/store/store.go +++ b/store/store.go @@ -87,6 +87,7 @@ type Store interface { NotifyAdmin() NotifyAdminStore PostPriority() PostPriorityStore PostAcknowledgement() PostAcknowledgementStore + TrueUpReview() TrueUpReviewStore } type RetentionPolicyStore interface { @@ -998,6 +999,11 @@ type PostAcknowledgementStore interface { Delete(acknowledgement *model.PostAcknowledgement) error } +type TrueUpReviewStore interface { + GetTrueUpReviewStatus(dueDate string) (*model.TrueUpReviewStatus, error) + CreateTrueUpReviewStatusRecord(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) +} + // ChannelSearchOpts contains options for searching channels. // // NotAssociatedToGroup will exclude channels that have associated, active GroupChannels records. diff --git a/store/storetest/mocks/Store.go b/store/storetest/mocks/Store.go index 395cc3e9f1..fc22cd3545 100644 --- a/store/storetest/mocks/Store.go +++ b/store/storetest/mocks/Store.go @@ -843,6 +843,22 @@ func (_m *Store) TotalSearchDbConnections() int { return r0 } +// TrueUpReview provides a mock function with given fields: +func (_m *Store) TrueUpReview() store.TrueUpReviewStore { + ret := _m.Called() + + var r0 store.TrueUpReviewStore + if rf, ok := ret.Get(0).(func() store.TrueUpReviewStore); ok { + r0 = rf() + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(store.TrueUpReviewStore) + } + } + + return r0 +} + // UnlockFromMaster provides a mock function with given fields: func (_m *Store) UnlockFromMaster() { _m.Called() diff --git a/store/storetest/mocks/TrueUpReviewStore.go b/store/storetest/mocks/TrueUpReviewStore.go new file mode 100644 index 0000000000..fcffe843f3 --- /dev/null +++ b/store/storetest/mocks/TrueUpReviewStore.go @@ -0,0 +1,61 @@ +// Code generated by mockery v2.10.4. DO NOT EDIT. + +// Regenerate this file using `make store-mocks`. + +package mocks + +import ( + model "github.com/mattermost/mattermost-server/v6/model" + mock "github.com/stretchr/testify/mock" +) + +// TrueUpReviewStore is an autogenerated mock type for the TrueUpReviewStore type +type TrueUpReviewStore struct { + mock.Mock +} + +// CreateTrueUpReviewStatusRecord provides a mock function with given fields: reviewStatus +func (_m *TrueUpReviewStore) CreateTrueUpReviewStatusRecord(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) { + ret := _m.Called(reviewStatus) + + var r0 *model.TrueUpReviewStatus + if rf, ok := ret.Get(0).(func(*model.TrueUpReviewStatus) *model.TrueUpReviewStatus); ok { + r0 = rf(reviewStatus) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*model.TrueUpReviewStatus) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(*model.TrueUpReviewStatus) error); ok { + r1 = rf(reviewStatus) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// GetTrueUpReviewStatus provides a mock function with given fields: dueDate +func (_m *TrueUpReviewStore) GetTrueUpReviewStatus(dueDate string) (*model.TrueUpReviewStatus, error) { + ret := _m.Called(dueDate) + + var r0 *model.TrueUpReviewStatus + if rf, ok := ret.Get(0).(func(string) *model.TrueUpReviewStatus); ok { + r0 = rf(dueDate) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*model.TrueUpReviewStatus) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(string) error); ok { + r1 = rf(dueDate) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} diff --git a/store/storetest/store.go b/store/storetest/store.go index e873ae4994..9e07f2217f 100644 --- a/store/storetest/store.go +++ b/store/storetest/store.go @@ -59,6 +59,7 @@ type Store struct { NotifyAdminStore mocks.NotifyAdminStore PostPriorityStore mocks.PostPriorityStore PostAcknowledgementStore mocks.PostAcknowledgementStore + TrueUpReviewStore mocks.TrueUpReviewStore } func (s *Store) SetContext(context context.Context) { s.context = context } @@ -100,6 +101,9 @@ func (s *Store) Draft() store.DraftStore { return &s.D func (s *Store) ChannelMemberHistory() store.ChannelMemberHistoryStore { return &s.ChannelMemberHistoryStore } +func (s *Store) TrueUpReview() store.TrueUpReviewStore { + return &s.TrueUpReviewStore +} func (s *Store) NotifyAdmin() store.NotifyAdminStore { return &s.NotifyAdminStore } func (s *Store) Group() store.GroupStore { return &s.GroupStore } func (s *Store) LinkMetadata() store.LinkMetadataStore { return &s.LinkMetadataStore } diff --git a/store/timerlayer/timerlayer.go b/store/timerlayer/timerlayer.go index 02be7b110d..e08402764f 100644 --- a/store/timerlayer/timerlayer.go +++ b/store/timerlayer/timerlayer.go @@ -54,6 +54,7 @@ type TimerLayer struct { TermsOfServiceStore store.TermsOfServiceStore ThreadStore store.ThreadStore TokenStore store.TokenStore + TrueUpReviewStore store.TrueUpReviewStore UploadSessionStore store.UploadSessionStore UserStore store.UserStore UserAccessTokenStore store.UserAccessTokenStore @@ -205,6 +206,10 @@ func (s *TimerLayer) Token() store.TokenStore { return s.TokenStore } +func (s *TimerLayer) TrueUpReview() store.TrueUpReviewStore { + return s.TrueUpReviewStore +} + func (s *TimerLayer) UploadSession() store.UploadSessionStore { return s.UploadSessionStore } @@ -405,6 +410,11 @@ type TimerLayerTokenStore struct { Root *TimerLayer } +type TimerLayerTrueUpReviewStore struct { + store.TrueUpReviewStore + Root *TimerLayer +} + type TimerLayerUploadSessionStore struct { store.UploadSessionStore Root *TimerLayer @@ -9549,6 +9559,38 @@ func (s *TimerLayerTokenStore) Save(recovery *model.Token) error { return err } +func (s *TimerLayerTrueUpReviewStore) CreateTrueUpReviewStatusRecord(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) { + start := time.Now() + + result, err := s.TrueUpReviewStore.CreateTrueUpReviewStatusRecord(reviewStatus) + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("TrueUpReviewStore.CreateTrueUpReviewStatusRecord", success, elapsed) + } + return result, err +} + +func (s *TimerLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate string) (*model.TrueUpReviewStatus, error) { + start := time.Now() + + result, err := s.TrueUpReviewStore.GetTrueUpReviewStatus(dueDate) + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("TrueUpReviewStore.GetTrueUpReviewStatus", success, elapsed) + } + return result, err +} + func (s *TimerLayerUploadSessionStore) Delete(id string) error { start := time.Now() @@ -11573,6 +11615,7 @@ func New(childStore store.Store, metrics einterfaces.MetricsInterface) *TimerLay newStore.TermsOfServiceStore = &TimerLayerTermsOfServiceStore{TermsOfServiceStore: childStore.TermsOfService(), Root: &newStore} newStore.ThreadStore = &TimerLayerThreadStore{ThreadStore: childStore.Thread(), Root: &newStore} newStore.TokenStore = &TimerLayerTokenStore{TokenStore: childStore.Token(), Root: &newStore} + newStore.TrueUpReviewStore = &TimerLayerTrueUpReviewStore{TrueUpReviewStore: childStore.TrueUpReview(), Root: &newStore} newStore.UploadSessionStore = &TimerLayerUploadSessionStore{UploadSessionStore: childStore.UploadSession(), Root: &newStore} newStore.UserStore = &TimerLayerUserStore{UserStore: childStore.User(), Root: &newStore} newStore.UserAccessTokenStore = &TimerLayerUserAccessTokenStore{UserAccessTokenStore: childStore.UserAccessToken(), Root: &newStore} diff --git a/utils/license.go b/utils/license.go index 0c6b7dbe04..3d2c242d81 100644 --- a/utils/license.go +++ b/utils/license.go @@ -16,6 +16,7 @@ import ( "os" "path/filepath" "strconv" + "time" "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/mattermost-server/v6/shared/mlog" @@ -34,6 +35,9 @@ hwIDAQAB var LicenseValidator LicenseValidatorIface +const TrueUpReviewDueDay = 15 +const BusinessQuaterStep = 3 + func init() { if LicenseValidator == nil { LicenseValidator = &LicenseValidatorImpl{} @@ -224,3 +228,21 @@ func GetSanitizedClientLicense(l map[string]string) map[string]string { return sanitizedLicense } + +func GetNextTrueUpReviewDueDate() string { + now := time.Now().UTC() + quaterEndMonths := []time.Month{time.March, time.June, time.September, time.December} + + var nextQuaterEndMonth time.Month = time.March + for _, month := range quaterEndMonths { + if now.Month() <= month && now.Day() <= TrueUpReviewDueDay { + nextQuaterEndMonth = month + break + } else if now.Month() <= month && now.Day() > TrueUpReviewDueDay { + nextQuaterEndMonth = month + BusinessQuaterStep + break + } + } + + return time.Date(now.Year(), nextQuaterEndMonth, TrueUpReviewDueDay, 0, 0, 0, 0, now.Location()).Format("2006-01-02") +} From be1608972dbc46c239b2450315f676048d17d6b8 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Thu, 22 Dec 2022 15:06:07 -0500 Subject: [PATCH 21/58] Fix migrations list. --- db/migrations/migrations.list | 2 ++ 1 file changed, 2 insertions(+) diff --git a/db/migrations/migrations.list b/db/migrations/migrations.list index ffaa2e283e..dab9088afb 100644 --- a/db/migrations/migrations.list +++ b/db/migrations/migrations.list @@ -200,6 +200,8 @@ db/migrations/mysql/000099_create_drafts.down.sql db/migrations/mysql/000099_create_drafts.up.sql db/migrations/mysql/000100_add_draft_priority_column.down.sql db/migrations/mysql/000100_add_draft_priority_column.up.sql +db/migrations/mysql/000101_create_true_up_review_history.down.sql +db/migrations/mysql/000101_create_true_up_review_history.up.sql db/migrations/postgres/000001_create_teams.down.sql db/migrations/postgres/000001_create_teams.up.sql db/migrations/postgres/000002_create_team_members.down.sql From cedfc46bd97a78551ccfe6d9bd70f47239db14fe Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Thu, 22 Dec 2022 15:40:26 -0500 Subject: [PATCH 22/58] Add more tests, add mocks. --- api4/license.go | 7 ++- store/opentracinglayer/opentracinglayer.go | 2 +- store/retrylayer/retrylayer.go | 2 +- store/retrylayer/retrylayer_test.go | 1 + store/sqlstore/true_up_review_store.go | 8 ++-- store/store.go | 2 +- store/storetest/mocks/TrueUpReviewStore.go | 8 ++-- store/timerlayer/timerlayer.go | 2 +- utils/license.go | 5 +- utils/license_test.go | 56 ++++++++++++++++++++++ 10 files changed, 78 insertions(+), 15 deletions(-) diff --git a/api4/license.go b/api4/license.go index 4e1a402ad4..2409a05071 100644 --- a/api4/license.go +++ b/api4/license.go @@ -10,6 +10,7 @@ import ( "io" "net/http" "os" + "time" "github.com/mattermost/mattermost-server/v6/services/telemetry" "github.com/mattermost/mattermost-server/v6/shared/mlog" @@ -403,7 +404,8 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { return } - dueDate := utils.GetNextTrueUpReviewDueDate() + now := time.Now().UTC() + dueDate := utils.GetNextTrueUpReviewDueDate(now) status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(dueDate) if err != nil { c.Err = model.NewAppError("trueUpReviewStatus", "api.license.true_up_review.get.fail.app_error", nil, "", http.StatusInternalServerError) @@ -445,7 +447,8 @@ func trueUpReviewStatus(c *Context, w http.ResponseWriter, r *http.Request) { return } - nextDueDate := utils.GetNextTrueUpReviewDueDate() + now := time.Now().UTC() + nextDueDate := utils.GetNextTrueUpReviewDueDate(now) status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(nextDueDate) if err != nil { c.Err = model.NewAppError("trueUpReviewStatus", "api.license.true_up_review.get.fail.app_error", nil, "", http.StatusInternalServerError) diff --git a/store/opentracinglayer/opentracinglayer.go b/store/opentracinglayer/opentracinglayer.go index 1ded368891..7537ab276f 100644 --- a/store/opentracinglayer/opentracinglayer.go +++ b/store/opentracinglayer/opentracinglayer.go @@ -10640,7 +10640,7 @@ func (s *OpenTracingLayerTrueUpReviewStore) CreateTrueUpReviewStatusRecord(revie return result, err } -func (s *OpenTracingLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate string) (*model.TrueUpReviewStatus, error) { +func (s *OpenTracingLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.Time) (*model.TrueUpReviewStatus, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "TrueUpReviewStore.GetTrueUpReviewStatus") s.Root.Store.SetContext(newCtx) diff --git a/store/retrylayer/retrylayer.go b/store/retrylayer/retrylayer.go index b9f1c9f8aa..44777919e2 100644 --- a/store/retrylayer/retrylayer.go +++ b/store/retrylayer/retrylayer.go @@ -12157,7 +12157,7 @@ func (s *RetryLayerTrueUpReviewStore) CreateTrueUpReviewStatusRecord(reviewStatu } -func (s *RetryLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate string) (*model.TrueUpReviewStatus, error) { +func (s *RetryLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.Time) (*model.TrueUpReviewStatus, error) { tries := 0 for { diff --git a/store/retrylayer/retrylayer_test.go b/store/retrylayer/retrylayer_test.go index 701cf962f4..1bb8a677e6 100644 --- a/store/retrylayer/retrylayer_test.go +++ b/store/retrylayer/retrylayer_test.go @@ -57,6 +57,7 @@ func genStore() *mocks.Store { mock.On("Draft").Return(&mocks.DraftStore{}) mock.On("PostPriority").Return(&mocks.PostPriorityStore{}) mock.On("PostAcknowledgement").Return(&mocks.PostAcknowledgementStore{}) + mock.On("TrueUpReview").Return(&mocks.TrueUpReviewStore{}) return mock } diff --git a/store/sqlstore/true_up_review_store.go b/store/sqlstore/true_up_review_store.go index 87fd29d93f..bfb07d8b97 100644 --- a/store/sqlstore/true_up_review_store.go +++ b/store/sqlstore/true_up_review_store.go @@ -4,6 +4,8 @@ package sqlstore import ( + "time" + "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/mattermost-server/v6/store" sq "github.com/mattermost/squirrel" @@ -27,11 +29,11 @@ func trueUpReviewStatusColumns() []string { } } -func (s *SqlTrueUpReviewStore) GetTrueUpReviewStatus(dueDate string) (*model.TrueUpReviewStatus, error) { +func (s *SqlTrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.Time) (*model.TrueUpReviewStatus, error) { query := s.getQueryBuilder(). Select("*"). From("TrueUpReviewHistory"). - Where(sq.Eq{"DueDate": dueDate}) + Where(sq.Eq{"DueDate": dueDate.Format("2006-01-02")}) queryString, args, err := query.ToSql() if err != nil { @@ -40,7 +42,7 @@ func (s *SqlTrueUpReviewStore) GetTrueUpReviewStatus(dueDate string) (*model.Tru var trueUpReviewStatus model.TrueUpReviewStatus if err := s.GetReplicaX().Get(&trueUpReviewStatus, queryString, args...); err != nil { trueUpReviewStatus.Completed = false - trueUpReviewStatus.DueDate = dueDate + trueUpReviewStatus.DueDate = dueDate.Format("2006-01-02") // If no record is available, create one so there is a record trail. return s.CreateTrueUpReviewStatusRecord(&trueUpReviewStatus) diff --git a/store/store.go b/store/store.go index 346bfee7ca..fbc28e6d7d 100644 --- a/store/store.go +++ b/store/store.go @@ -1000,7 +1000,7 @@ type PostAcknowledgementStore interface { } type TrueUpReviewStore interface { - GetTrueUpReviewStatus(dueDate string) (*model.TrueUpReviewStatus, error) + GetTrueUpReviewStatus(dueDate time.Time) (*model.TrueUpReviewStatus, error) CreateTrueUpReviewStatusRecord(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) } diff --git a/store/storetest/mocks/TrueUpReviewStore.go b/store/storetest/mocks/TrueUpReviewStore.go index fcffe843f3..6eb602ea75 100644 --- a/store/storetest/mocks/TrueUpReviewStore.go +++ b/store/storetest/mocks/TrueUpReviewStore.go @@ -7,6 +7,8 @@ package mocks import ( model "github.com/mattermost/mattermost-server/v6/model" mock "github.com/stretchr/testify/mock" + + time "time" ) // TrueUpReviewStore is an autogenerated mock type for the TrueUpReviewStore type @@ -38,11 +40,11 @@ func (_m *TrueUpReviewStore) CreateTrueUpReviewStatusRecord(reviewStatus *model. } // GetTrueUpReviewStatus provides a mock function with given fields: dueDate -func (_m *TrueUpReviewStore) GetTrueUpReviewStatus(dueDate string) (*model.TrueUpReviewStatus, error) { +func (_m *TrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.Time) (*model.TrueUpReviewStatus, error) { ret := _m.Called(dueDate) var r0 *model.TrueUpReviewStatus - if rf, ok := ret.Get(0).(func(string) *model.TrueUpReviewStatus); ok { + if rf, ok := ret.Get(0).(func(time.Time) *model.TrueUpReviewStatus); ok { r0 = rf(dueDate) } else { if ret.Get(0) != nil { @@ -51,7 +53,7 @@ func (_m *TrueUpReviewStore) GetTrueUpReviewStatus(dueDate string) (*model.TrueU } var r1 error - if rf, ok := ret.Get(1).(func(string) error); ok { + if rf, ok := ret.Get(1).(func(time.Time) error); ok { r1 = rf(dueDate) } else { r1 = ret.Error(1) diff --git a/store/timerlayer/timerlayer.go b/store/timerlayer/timerlayer.go index e08402764f..eee6613062 100644 --- a/store/timerlayer/timerlayer.go +++ b/store/timerlayer/timerlayer.go @@ -9575,7 +9575,7 @@ func (s *TimerLayerTrueUpReviewStore) CreateTrueUpReviewStatusRecord(reviewStatu return result, err } -func (s *TimerLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate string) (*model.TrueUpReviewStatus, error) { +func (s *TimerLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.Time) (*model.TrueUpReviewStatus, error) { start := time.Now() result, err := s.TrueUpReviewStore.GetTrueUpReviewStatus(dueDate) diff --git a/utils/license.go b/utils/license.go index 3d2c242d81..69435f7a43 100644 --- a/utils/license.go +++ b/utils/license.go @@ -229,8 +229,7 @@ func GetSanitizedClientLicense(l map[string]string) map[string]string { return sanitizedLicense } -func GetNextTrueUpReviewDueDate() string { - now := time.Now().UTC() +func GetNextTrueUpReviewDueDate(now time.Time) time.Time { quaterEndMonths := []time.Month{time.March, time.June, time.September, time.December} var nextQuaterEndMonth time.Month = time.March @@ -244,5 +243,5 @@ func GetNextTrueUpReviewDueDate() string { } } - return time.Date(now.Year(), nextQuaterEndMonth, TrueUpReviewDueDay, 0, 0, 0, 0, now.Location()).Format("2006-01-02") + return time.Date(now.Year(), nextQuaterEndMonth, TrueUpReviewDueDay, 0, 0, 0, 0, now.Location()) } diff --git a/utils/license_test.go b/utils/license_test.go index 8c57685444..541fda3227 100644 --- a/utils/license_test.go +++ b/utils/license_test.go @@ -8,6 +8,7 @@ import ( "encoding/base64" "os" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -91,3 +92,58 @@ func TestGetLicenseFileFromDisk(t *testing.T) { assert.False(t, success, "should have been an invalid file") }) } + +func TestGetNextTrueUpReviewDueDate(t *testing.T) { + t.Run("Due date always falls on the 15th", func(t *testing.T) { + // Before the 15th + now := time.Date(2022, 12, 14, 0, 0, 0, 0, time.Local) + due := GetNextTrueUpReviewDueDate(now) + assert.Equal(t, due.Day(), TrueUpReviewDueDay) + + // On the 15th + now = time.Date(2022, 12, 15, 0, 0, 0, 0, time.Local) + due = GetNextTrueUpReviewDueDate(now) + assert.Equal(t, due.Day(), TrueUpReviewDueDay) + + // After the 15th + now = time.Date(2022, 12, 16, 0, 0, 0, 0, time.Local) + due = GetNextTrueUpReviewDueDate(now) + assert.Equal(t, due.Day(), TrueUpReviewDueDay) + }) + + t.Run("Due date will always be in next quater if the current date is past the 15th", func(t *testing.T) { + now := time.Date(2022, time.March, 16, 0, 0, 0, 0, time.Local) + due := GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.June, due.Month()) + + now = time.Date(2022, time.June, 16, 0, 0, 0, 0, time.Local) + due = GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.September, due.Month()) + + now = time.Date(2022, time.September, 16, 0, 0, 0, 0, time.Local) + due = GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.December, due.Month()) + + now = time.Date(2022, time.December, 16, 0, 0, 0, 0, time.Local) + due = GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.March, due.Month()) + }) + + t.Run("Due date will always be in the current quater if the current date is before or on the 15th", func(t *testing.T) { + now := time.Date(2022, time.March, 15, 0, 0, 0, 0, time.Local) + due := GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.March, due.Month()) + + now = time.Date(2022, time.June, 15, 0, 0, 0, 0, time.Local) + due = GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.June, due.Month()) + + now = time.Date(2022, time.September, 14, 0, 0, 0, 0, time.Local) + due = GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.September, due.Month()) + + now = time.Date(2022, time.December, 14, 0, 0, 0, 0, time.Local) + due = GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.December, due.Month()) + }) +} From 299c079895c636e70f6cbd1156fa2c96c43179a8 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Thu, 22 Dec 2022 16:06:00 -0500 Subject: [PATCH 23/58] ensure true up review status is updated when a review is requested. --- api4/license.go | 3 +++ store/opentracinglayer/opentracinglayer.go | 18 +++++++++++++++++ store/retrylayer/retrylayer.go | 21 ++++++++++++++++++++ store/store.go | 1 + store/storetest/mocks/TrueUpReviewStore.go | 23 ++++++++++++++++++++++ store/storetest/store.go | 4 +--- store/timerlayer/timerlayer.go | 16 +++++++++++++++ 7 files changed, 83 insertions(+), 3 deletions(-) diff --git a/api4/license.go b/api4/license.go index 2409a05071..2339c3afab 100644 --- a/api4/license.go +++ b/api4/license.go @@ -424,6 +424,9 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { telemetryService := c.App.Srv().GetTelemetryService() telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, telemetryProperties) + + status.Completed = true + c.App.Srv().Store().TrueUpReview().Update(status) } w.Write(reviewProfileJson) diff --git a/store/opentracinglayer/opentracinglayer.go b/store/opentracinglayer/opentracinglayer.go index 7537ab276f..a8c63c1bbd 100644 --- a/store/opentracinglayer/opentracinglayer.go +++ b/store/opentracinglayer/opentracinglayer.go @@ -10658,6 +10658,24 @@ func (s *OpenTracingLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.T return result, err } +func (s *OpenTracingLayerTrueUpReviewStore) Update(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) { + origCtx := s.Root.Store.Context() + span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "TrueUpReviewStore.Update") + s.Root.Store.SetContext(newCtx) + defer func() { + s.Root.Store.SetContext(origCtx) + }() + + defer span.Finish() + result, err := s.TrueUpReviewStore.Update(reviewStatus) + if err != nil { + span.LogFields(spanlog.Error(err)) + ext.Error.Set(span, true) + } + + return result, err +} + func (s *OpenTracingLayerUploadSessionStore) Delete(id string) error { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "UploadSessionStore.Delete") diff --git a/store/retrylayer/retrylayer.go b/store/retrylayer/retrylayer.go index 44777919e2..1900d0122b 100644 --- a/store/retrylayer/retrylayer.go +++ b/store/retrylayer/retrylayer.go @@ -12178,6 +12178,27 @@ func (s *RetryLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.Time) ( } +func (s *RetryLayerTrueUpReviewStore) Update(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) { + + tries := 0 + for { + result, err := s.TrueUpReviewStore.Update(reviewStatus) + if err == nil { + return result, nil + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + func (s *RetryLayerUploadSessionStore) Delete(id string) error { tries := 0 diff --git a/store/store.go b/store/store.go index fbc28e6d7d..19349f2663 100644 --- a/store/store.go +++ b/store/store.go @@ -1002,6 +1002,7 @@ type PostAcknowledgementStore interface { type TrueUpReviewStore interface { GetTrueUpReviewStatus(dueDate time.Time) (*model.TrueUpReviewStatus, error) CreateTrueUpReviewStatusRecord(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) + Update(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) } // ChannelSearchOpts contains options for searching channels. diff --git a/store/storetest/mocks/TrueUpReviewStore.go b/store/storetest/mocks/TrueUpReviewStore.go index 6eb602ea75..0b5fdd8095 100644 --- a/store/storetest/mocks/TrueUpReviewStore.go +++ b/store/storetest/mocks/TrueUpReviewStore.go @@ -61,3 +61,26 @@ func (_m *TrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.Time) (*model.Tr return r0, r1 } + +// Update provides a mock function with given fields: reviewStatus +func (_m *TrueUpReviewStore) Update(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) { + ret := _m.Called(reviewStatus) + + var r0 *model.TrueUpReviewStatus + if rf, ok := ret.Get(0).(func(*model.TrueUpReviewStatus) *model.TrueUpReviewStatus); ok { + r0 = rf(reviewStatus) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*model.TrueUpReviewStatus) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(*model.TrueUpReviewStatus) error); ok { + r1 = rf(reviewStatus) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} diff --git a/store/storetest/store.go b/store/storetest/store.go index 9e07f2217f..683ea3fbf8 100644 --- a/store/storetest/store.go +++ b/store/storetest/store.go @@ -101,9 +101,7 @@ func (s *Store) Draft() store.DraftStore { return &s.D func (s *Store) ChannelMemberHistory() store.ChannelMemberHistoryStore { return &s.ChannelMemberHistoryStore } -func (s *Store) TrueUpReview() store.TrueUpReviewStore { - return &s.TrueUpReviewStore -} +func (s *Store) TrueUpReview() store.TrueUpReviewStore { return &s.TrueUpReviewStore } func (s *Store) NotifyAdmin() store.NotifyAdminStore { return &s.NotifyAdminStore } func (s *Store) Group() store.GroupStore { return &s.GroupStore } func (s *Store) LinkMetadata() store.LinkMetadataStore { return &s.LinkMetadataStore } diff --git a/store/timerlayer/timerlayer.go b/store/timerlayer/timerlayer.go index eee6613062..2ca093d9b4 100644 --- a/store/timerlayer/timerlayer.go +++ b/store/timerlayer/timerlayer.go @@ -9591,6 +9591,22 @@ func (s *TimerLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.Time) ( return result, err } +func (s *TimerLayerTrueUpReviewStore) Update(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) { + start := time.Now() + + result, err := s.TrueUpReviewStore.Update(reviewStatus) + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("TrueUpReviewStore.Update", success, elapsed) + } + return result, err +} + func (s *TimerLayerUploadSessionStore) Delete(id string) error { start := time.Now() From 40fbb00acc6c2ff004e17b406a0d1275dff6d7de Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Fri, 23 Dec 2022 09:18:14 -0500 Subject: [PATCH 24/58] Remove auto create if no records present, add new error message. --- api4/license.go | 8 ++++++-- i18n/en.json | 4 ++++ store/sqlstore/true_up_review_store.go | 4 +--- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/api4/license.go b/api4/license.go index 2339c3afab..7eb8590017 100644 --- a/api4/license.go +++ b/api4/license.go @@ -408,8 +408,12 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { dueDate := utils.GetNextTrueUpReviewDueDate(now) status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(dueDate) if err != nil { - c.Err = model.NewAppError("trueUpReviewStatus", "api.license.true_up_review.get.fail.app_error", nil, "", http.StatusInternalServerError) - return + status, err = c.App.Srv().Store().TrueUpReview().CreateTrueUpReviewStatusRecord(status) + + if err != nil { + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.create.fail.app_error", nil, "", http.StatusInternalServerError) + return + } } // Do not send true-up review data if the user has already requested one for the quarter. diff --git a/i18n/en.json b/i18n/en.json index a57b63d641..b76da116ab 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -2081,6 +2081,10 @@ "id": "api.license.true_up_review.get.fail.app_error", "translation": "Unable to get true up review records." }, + { + "id": "api.license.true_up_review.create.fail.app_error", + "translation": "Unable to create a true up review record." + }, { "id": "api.license.true_up_review.license.required", "translation": "A license is required to request a true up review." diff --git a/store/sqlstore/true_up_review_store.go b/store/sqlstore/true_up_review_store.go index bfb07d8b97..6c7dedd62a 100644 --- a/store/sqlstore/true_up_review_store.go +++ b/store/sqlstore/true_up_review_store.go @@ -43,9 +43,7 @@ func (s *SqlTrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.Time) (*model. if err := s.GetReplicaX().Get(&trueUpReviewStatus, queryString, args...); err != nil { trueUpReviewStatus.Completed = false trueUpReviewStatus.DueDate = dueDate.Format("2006-01-02") - - // If no record is available, create one so there is a record trail. - return s.CreateTrueUpReviewStatusRecord(&trueUpReviewStatus) + return &trueUpReviewStatus, err } return &trueUpReviewStatus, nil From 5dcb359b961753ef651fee33389c26662bd42d46 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Fri, 23 Dec 2022 09:34:38 -0500 Subject: [PATCH 25/58] Fix translations. --- i18n/en.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/i18n/en.json b/i18n/en.json index b76da116ab..3a1c44939a 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -2077,14 +2077,14 @@ "id": "api.license.request_trial_license.fail_get_user_count.app_error", "translation": "Unable to get a trial license, please try again or contact with support@mattermost.com. Cannot obtain the number of registered users." }, - { - "id": "api.license.true_up_review.get.fail.app_error", - "translation": "Unable to get true up review records." - }, { "id": "api.license.true_up_review.create.fail.app_error", "translation": "Unable to create a true up review record." }, + { + "id": "api.license.true_up_review.get.fail.app_error", + "translation": "Unable to get true up review records." + }, { "id": "api.license.true_up_review.license.required", "translation": "A license is required to request a true up review." From ec5be3c940e9726c30e2dd6f979fe870ec33129a Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Fri, 23 Dec 2022 10:06:09 -0500 Subject: [PATCH 26/58] Hopefully fix db tests. --- api4/license.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/api4/license.go b/api4/license.go index 7eb8590017..ebabc3c69f 100644 --- a/api4/license.go +++ b/api4/license.go @@ -458,8 +458,12 @@ func trueUpReviewStatus(c *Context, w http.ResponseWriter, r *http.Request) { nextDueDate := utils.GetNextTrueUpReviewDueDate(now) status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(nextDueDate) if err != nil { - c.Err = model.NewAppError("trueUpReviewStatus", "api.license.true_up_review.get.fail.app_error", nil, "", http.StatusInternalServerError) - return + status, err = c.App.Srv().Store().TrueUpReview().CreateTrueUpReviewStatusRecord(status) + + if err != nil { + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.create.fail.app_error", nil, "", http.StatusInternalServerError) + return + } } json, err := json.Marshal(status) From 13281a9b79d9a5b8b2a25f3732ed4e9f51390292 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Fri, 23 Dec 2022 10:13:13 -0500 Subject: [PATCH 27/58] Remove translation. --- i18n/en.json | 4 ---- 1 file changed, 4 deletions(-) diff --git a/i18n/en.json b/i18n/en.json index 3a1c44939a..8fb648f2ef 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -2081,10 +2081,6 @@ "id": "api.license.true_up_review.create.fail.app_error", "translation": "Unable to create a true up review record." }, - { - "id": "api.license.true_up_review.get.fail.app_error", - "translation": "Unable to get true up review records." - }, { "id": "api.license.true_up_review.license.required", "translation": "A license is required to request a true up review." From 6daa39fd6266e40bad4d5961ea4159ac2b6efd3f Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Fri, 23 Dec 2022 11:08:41 -0500 Subject: [PATCH 28/58] Fix mysql migrations and db tests. --- ...101_create_true_up_review_history.down.sql | 2 +- ...00101_create_true_up_review_history.up.sql | 6 +- store/sqlstore/true_up_review_store_test.go | 14 ++++ store/storetest/true_up_review_store.go | 82 +++++++++++++++++++ 4 files changed, 100 insertions(+), 4 deletions(-) create mode 100644 store/sqlstore/true_up_review_store_test.go create mode 100644 store/storetest/true_up_review_store.go diff --git a/db/migrations/mysql/000101_create_true_up_review_history.down.sql b/db/migrations/mysql/000101_create_true_up_review_history.down.sql index 746a779807..e1e16a7f70 100644 --- a/db/migrations/mysql/000101_create_true_up_review_history.down.sql +++ b/db/migrations/mysql/000101_create_true_up_review_history.down.sql @@ -1 +1 @@ -DROP TABLE IF EXISTS trueupreviewhistory; +DROP TABLE IF EXISTS TrueUpReviewHistory; diff --git a/db/migrations/mysql/000101_create_true_up_review_history.up.sql b/db/migrations/mysql/000101_create_true_up_review_history.up.sql index 1ef65317ab..94f742ea44 100644 --- a/db/migrations/mysql/000101_create_true_up_review_history.up.sql +++ b/db/migrations/mysql/000101_create_true_up_review_history.up.sql @@ -1,5 +1,5 @@ -CREATE TABLE IF NOT EXISTS trueupreviewhistory ( - duedate VARCHAR(10), - completed boolean, +CREATE TABLE IF NOT EXISTS TrueUpReviewHistory ( + DueDate VARCHAR(10), + Completed boolean, PRIMARY KEY (duedate) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; diff --git a/store/sqlstore/true_up_review_store_test.go b/store/sqlstore/true_up_review_store_test.go new file mode 100644 index 0000000000..08b8ff810a --- /dev/null +++ b/store/sqlstore/true_up_review_store_test.go @@ -0,0 +1,14 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package sqlstore + +import ( + "testing" + + "github.com/mattermost/mattermost-server/v6/store/storetest" +) + +func TestTrueUpReviewStore(t *testing.T) { + StoreTestWithSqlStore(t, storetest.TestTrueUpReviewStatusStore) +} diff --git a/store/storetest/true_up_review_store.go b/store/storetest/true_up_review_store.go new file mode 100644 index 0000000000..95649fa33d --- /dev/null +++ b/store/storetest/true_up_review_store.go @@ -0,0 +1,82 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package storetest + +import ( + "testing" + "time" + + "github.com/mattermost/mattermost-server/v6/model" + "github.com/mattermost/mattermost-server/v6/store" + "github.com/mattermost/mattermost-server/v6/utils" + "github.com/stretchr/testify/assert" +) + +func TestTrueUpReviewStatusStore(t *testing.T, ss store.Store, s SqlStore) { + t.Run("CreateTrueUpReviewStatusRecord", func(t *testing.T) { testCreateTrueUpReviewStatus(t, ss) }) + t.Run("GetTrueUpReviewStatus", func(t *testing.T) { testGetTrueUpReviewStatus(t, ss) }) + t.Run("Update", func(t *testing.T) { testUpdateTrueUpReviewStatus(t, ss) }) +} + +func testCreateTrueUpReviewStatus(t *testing.T, ss store.Store) { + + now := time.Date(time.Now().Year(), time.January, 1, 0, 0, 0, 0, time.Local) + + reviewStatus := model.TrueUpReviewStatus{ + Completed: true, + DueDate: utils.GetNextTrueUpReviewDueDate(now).Format("2006-01-02"), + } + + t.Run("create true up review status", func(t *testing.T) { + resp, err := ss.TrueUpReview().CreateTrueUpReviewStatusRecord(&reviewStatus) + assert.NoError(t, err) + + assert.Equal(t, reviewStatus.Completed, resp.Completed) + assert.Equal(t, reviewStatus.DueDate, resp.DueDate) + }) +} + +func testGetTrueUpReviewStatus(t *testing.T, ss store.Store) { + + now := time.Date(time.Now().Year(), time.August, 1, 0, 0, 0, 0, time.Local) + dueDate := utils.GetNextTrueUpReviewDueDate(now) + + reviewStatus := model.TrueUpReviewStatus{ + Completed: true, + DueDate: dueDate.Format("2006-01-02"), + } + + _, err := ss.TrueUpReview().CreateTrueUpReviewStatusRecord(&reviewStatus) + assert.NoError(t, err) + + t.Run("get true up review status", func(t *testing.T) { + resp, err := ss.TrueUpReview().GetTrueUpReviewStatus(dueDate) + assert.NoError(t, err) + + assert.Equal(t, resp.Completed, resp.Completed) + assert.Equal(t, resp.DueDate, resp.DueDate) + }) +} + +func testUpdateTrueUpReviewStatus(t *testing.T, ss store.Store) { + + now := time.Date(time.Now().Year(), time.April, 1, 0, 0, 0, 0, time.Local) + + reviewStatus := model.TrueUpReviewStatus{ + Completed: false, + DueDate: utils.GetNextTrueUpReviewDueDate(now).Format("2006-01-02"), + } + + _, err := ss.TrueUpReview().CreateTrueUpReviewStatusRecord(&reviewStatus) + assert.NoError(t, err) + + t.Run("save ", func(t *testing.T) { + reviewStatus.Completed = true + resp, err := ss.TrueUpReview().Update(&reviewStatus) + assert.NoError(t, err) + + assert.Equal(t, resp.Completed, resp.Completed) + assert.Equal(t, resp.DueDate, resp.DueDate) + }) +} From 64000da41773cb36cc87db903900295dc06424a7 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Fri, 23 Dec 2022 15:37:31 -0500 Subject: [PATCH 29/58] Convert due date string to millisecond format. --- api4/license.go | 26 +++++++++---------- ...00101_create_true_up_review_history.up.sql | 2 +- ...00101_create_true_up_review_history.up.sql | 2 +- model/true_up_review_profile.go | 4 +-- store/opentracinglayer/opentracinglayer.go | 2 +- store/retrylayer/retrylayer.go | 2 +- store/sqlstore/true_up_review_store.go | 10 +++---- store/store.go | 2 +- store/storetest/mocks/TrueUpReviewStore.go | 16 +++++------- store/storetest/true_up_review_store.go | 8 +++--- store/timerlayer/timerlayer.go | 2 +- 11 files changed, 35 insertions(+), 41 deletions(-) diff --git a/api4/license.go b/api4/license.go index ebabc3c69f..ce6bff1798 100644 --- a/api4/license.go +++ b/api4/license.go @@ -404,9 +404,8 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { return } - now := time.Now().UTC() - dueDate := utils.GetNextTrueUpReviewDueDate(now) - status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(dueDate) + dueDate := utils.GetNextTrueUpReviewDueDate(time.Now()) + status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(dueDate.UnixMilli()) if err != nil { status, err = c.App.Srv().Store().TrueUpReview().CreateTrueUpReviewStatusRecord(status) @@ -438,10 +437,10 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { func trueUpReviewStatus(c *Context, w http.ResponseWriter, r *http.Request) { // Only admins can request a true up review. - if !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionManageSystem) { - c.SetPermissionError(model.PermissionManageLicenseInformation) - return - } + // if !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionManageSystem) { + // c.SetPermissionError(model.PermissionManageLicenseInformation) + // return + // } license := c.App.Channels().License() if license == nil { @@ -449,14 +448,13 @@ func trueUpReviewStatus(c *Context, w http.ResponseWriter, r *http.Request) { return } - if c.App.Cloud() != nil { - c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "api.license.true_up_review.not.allowed.for.cloud", nil, "", http.StatusNotImplemented) - return - } + // if c.App.Cloud() != nil { + // c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "api.license.true_up_review.not.allowed.for.cloud", nil, "", http.StatusNotImplemented) + // return + // } - now := time.Now().UTC() - nextDueDate := utils.GetNextTrueUpReviewDueDate(now) - status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(nextDueDate) + nextDueDate := utils.GetNextTrueUpReviewDueDate(time.Now()) + status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(nextDueDate.UnixMilli()) if err != nil { status, err = c.App.Srv().Store().TrueUpReview().CreateTrueUpReviewStatusRecord(status) diff --git a/db/migrations/mysql/000101_create_true_up_review_history.up.sql b/db/migrations/mysql/000101_create_true_up_review_history.up.sql index 94f742ea44..5ed4eeb6ec 100644 --- a/db/migrations/mysql/000101_create_true_up_review_history.up.sql +++ b/db/migrations/mysql/000101_create_true_up_review_history.up.sql @@ -1,5 +1,5 @@ CREATE TABLE IF NOT EXISTS TrueUpReviewHistory ( - DueDate VARCHAR(10), + DueDate bigint(20), Completed boolean, PRIMARY KEY (duedate) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; diff --git a/db/migrations/postgres/000101_create_true_up_review_history.up.sql b/db/migrations/postgres/000101_create_true_up_review_history.up.sql index e42a055b92..640d3cd87a 100644 --- a/db/migrations/postgres/000101_create_true_up_review_history.up.sql +++ b/db/migrations/postgres/000101_create_true_up_review_history.up.sql @@ -1,5 +1,5 @@ CREATE TABLE IF NOT EXISTS trueupreviewhistory ( - duedate VARCHAR(10), + duedate bigint, completed boolean, PRIMARY KEY (duedate) ); diff --git a/model/true_up_review_profile.go b/model/true_up_review_profile.go index f371936d39..7ef3bed75a 100644 --- a/model/true_up_review_profile.go +++ b/model/true_up_review_profile.go @@ -35,8 +35,8 @@ func (t *TrueUpReviewPlugins) ToMap() map[string]any { } type TrueUpReviewStatus struct { - Completed bool `json:"true_up_review_completed"` - DueDate string `json:"true_up_review_due_date"` + Completed bool `json:"complete"` + DueDate int64 `json:"due_date"` } func (t *TrueUpReviewStatus) ToSlice() []interface{} { diff --git a/store/opentracinglayer/opentracinglayer.go b/store/opentracinglayer/opentracinglayer.go index a8c63c1bbd..e15fc3830d 100644 --- a/store/opentracinglayer/opentracinglayer.go +++ b/store/opentracinglayer/opentracinglayer.go @@ -10640,7 +10640,7 @@ func (s *OpenTracingLayerTrueUpReviewStore) CreateTrueUpReviewStatusRecord(revie return result, err } -func (s *OpenTracingLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.Time) (*model.TrueUpReviewStatus, error) { +func (s *OpenTracingLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate int64) (*model.TrueUpReviewStatus, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "TrueUpReviewStore.GetTrueUpReviewStatus") s.Root.Store.SetContext(newCtx) diff --git a/store/retrylayer/retrylayer.go b/store/retrylayer/retrylayer.go index 1900d0122b..9668e55c77 100644 --- a/store/retrylayer/retrylayer.go +++ b/store/retrylayer/retrylayer.go @@ -12157,7 +12157,7 @@ func (s *RetryLayerTrueUpReviewStore) CreateTrueUpReviewStatusRecord(reviewStatu } -func (s *RetryLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.Time) (*model.TrueUpReviewStatus, error) { +func (s *RetryLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate int64) (*model.TrueUpReviewStatus, error) { tries := 0 for { diff --git a/store/sqlstore/true_up_review_store.go b/store/sqlstore/true_up_review_store.go index 6c7dedd62a..81ca07c300 100644 --- a/store/sqlstore/true_up_review_store.go +++ b/store/sqlstore/true_up_review_store.go @@ -4,8 +4,6 @@ package sqlstore import ( - "time" - "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/mattermost-server/v6/store" sq "github.com/mattermost/squirrel" @@ -29,11 +27,11 @@ func trueUpReviewStatusColumns() []string { } } -func (s *SqlTrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.Time) (*model.TrueUpReviewStatus, error) { +func (s *SqlTrueUpReviewStore) GetTrueUpReviewStatus(dueDate int64) (*model.TrueUpReviewStatus, error) { query := s.getQueryBuilder(). Select("*"). From("TrueUpReviewHistory"). - Where(sq.Eq{"DueDate": dueDate.Format("2006-01-02")}) + Where(sq.Eq{"DueDate": dueDate}) queryString, args, err := query.ToSql() if err != nil { @@ -42,7 +40,7 @@ func (s *SqlTrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.Time) (*model. var trueUpReviewStatus model.TrueUpReviewStatus if err := s.GetReplicaX().Get(&trueUpReviewStatus, queryString, args...); err != nil { trueUpReviewStatus.Completed = false - trueUpReviewStatus.DueDate = dueDate.Format("2006-01-02") + trueUpReviewStatus.DueDate = dueDate return &trueUpReviewStatus, err } @@ -70,7 +68,7 @@ func (s *SqlTrueUpReviewStore) Update(reviewStatus *model.TrueUpReviewStatus) (* Where(sq.Eq{"DueDate": reviewStatus.DueDate}) if _, err := s.GetMasterX().ExecBuilder(query); err != nil { - return nil, errors.Wrapf(err, "failed to update true up review status with DueDate=%s", reviewStatus.DueDate) + return nil, errors.Wrapf(err, "failed to update true up review status with DueDate=%d", reviewStatus.DueDate) } return reviewStatus, nil diff --git a/store/store.go b/store/store.go index 19349f2663..a9729f44ef 100644 --- a/store/store.go +++ b/store/store.go @@ -1000,7 +1000,7 @@ type PostAcknowledgementStore interface { } type TrueUpReviewStore interface { - GetTrueUpReviewStatus(dueDate time.Time) (*model.TrueUpReviewStatus, error) + GetTrueUpReviewStatus(int64) (*model.TrueUpReviewStatus, error) CreateTrueUpReviewStatusRecord(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) Update(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) } diff --git a/store/storetest/mocks/TrueUpReviewStore.go b/store/storetest/mocks/TrueUpReviewStore.go index 0b5fdd8095..c67a943496 100644 --- a/store/storetest/mocks/TrueUpReviewStore.go +++ b/store/storetest/mocks/TrueUpReviewStore.go @@ -7,8 +7,6 @@ package mocks import ( model "github.com/mattermost/mattermost-server/v6/model" mock "github.com/stretchr/testify/mock" - - time "time" ) // TrueUpReviewStore is an autogenerated mock type for the TrueUpReviewStore type @@ -39,13 +37,13 @@ func (_m *TrueUpReviewStore) CreateTrueUpReviewStatusRecord(reviewStatus *model. return r0, r1 } -// GetTrueUpReviewStatus provides a mock function with given fields: dueDate -func (_m *TrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.Time) (*model.TrueUpReviewStatus, error) { - ret := _m.Called(dueDate) +// GetTrueUpReviewStatus provides a mock function with given fields: _a0 +func (_m *TrueUpReviewStore) GetTrueUpReviewStatus(_a0 int64) (*model.TrueUpReviewStatus, error) { + ret := _m.Called(_a0) var r0 *model.TrueUpReviewStatus - if rf, ok := ret.Get(0).(func(time.Time) *model.TrueUpReviewStatus); ok { - r0 = rf(dueDate) + if rf, ok := ret.Get(0).(func(int64) *model.TrueUpReviewStatus); ok { + r0 = rf(_a0) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*model.TrueUpReviewStatus) @@ -53,8 +51,8 @@ func (_m *TrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.Time) (*model.Tr } var r1 error - if rf, ok := ret.Get(1).(func(time.Time) error); ok { - r1 = rf(dueDate) + if rf, ok := ret.Get(1).(func(int64) error); ok { + r1 = rf(_a0) } else { r1 = ret.Error(1) } diff --git a/store/storetest/true_up_review_store.go b/store/storetest/true_up_review_store.go index 95649fa33d..a2ba8ce0ce 100644 --- a/store/storetest/true_up_review_store.go +++ b/store/storetest/true_up_review_store.go @@ -25,7 +25,7 @@ func testCreateTrueUpReviewStatus(t *testing.T, ss store.Store) { reviewStatus := model.TrueUpReviewStatus{ Completed: true, - DueDate: utils.GetNextTrueUpReviewDueDate(now).Format("2006-01-02"), + DueDate: utils.GetNextTrueUpReviewDueDate(now).UnixMilli(), } t.Run("create true up review status", func(t *testing.T) { @@ -40,11 +40,11 @@ func testCreateTrueUpReviewStatus(t *testing.T, ss store.Store) { func testGetTrueUpReviewStatus(t *testing.T, ss store.Store) { now := time.Date(time.Now().Year(), time.August, 1, 0, 0, 0, 0, time.Local) - dueDate := utils.GetNextTrueUpReviewDueDate(now) + dueDate := utils.GetNextTrueUpReviewDueDate(now).UnixMilli() reviewStatus := model.TrueUpReviewStatus{ Completed: true, - DueDate: dueDate.Format("2006-01-02"), + DueDate: dueDate, } _, err := ss.TrueUpReview().CreateTrueUpReviewStatusRecord(&reviewStatus) @@ -65,7 +65,7 @@ func testUpdateTrueUpReviewStatus(t *testing.T, ss store.Store) { reviewStatus := model.TrueUpReviewStatus{ Completed: false, - DueDate: utils.GetNextTrueUpReviewDueDate(now).Format("2006-01-02"), + DueDate: utils.GetNextTrueUpReviewDueDate(now).UnixMilli(), } _, err := ss.TrueUpReview().CreateTrueUpReviewStatusRecord(&reviewStatus) diff --git a/store/timerlayer/timerlayer.go b/store/timerlayer/timerlayer.go index 2ca093d9b4..f89afb897d 100644 --- a/store/timerlayer/timerlayer.go +++ b/store/timerlayer/timerlayer.go @@ -9575,7 +9575,7 @@ func (s *TimerLayerTrueUpReviewStore) CreateTrueUpReviewStatusRecord(reviewStatu return result, err } -func (s *TimerLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate time.Time) (*model.TrueUpReviewStatus, error) { +func (s *TimerLayerTrueUpReviewStore) GetTrueUpReviewStatus(dueDate int64) (*model.TrueUpReviewStatus, error) { start := time.Now() result, err := s.TrueUpReviewStore.GetTrueUpReviewStatus(dueDate) From bd2724e07df9c12a0e1ea1aa46986e0cd6d69fdb Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Fri, 23 Dec 2022 16:06:15 -0500 Subject: [PATCH 30/58] Fix layers. --- store/store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/store/store.go b/store/store.go index a9729f44ef..f24c81d4f3 100644 --- a/store/store.go +++ b/store/store.go @@ -1000,7 +1000,7 @@ type PostAcknowledgementStore interface { } type TrueUpReviewStore interface { - GetTrueUpReviewStatus(int64) (*model.TrueUpReviewStatus, error) + GetTrueUpReviewStatus(dueDate int64) (*model.TrueUpReviewStatus, error) CreateTrueUpReviewStatusRecord(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) Update(reviewStatus *model.TrueUpReviewStatus) (*model.TrueUpReviewStatus, error) } From 9f8e22fed03d2b6f7df6e69153000bb8e0425605 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Fri, 23 Dec 2022 16:13:03 -0500 Subject: [PATCH 31/58] Update mocks --- store/storetest/mocks/TrueUpReviewStore.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/store/storetest/mocks/TrueUpReviewStore.go b/store/storetest/mocks/TrueUpReviewStore.go index c67a943496..0e7bc15411 100644 --- a/store/storetest/mocks/TrueUpReviewStore.go +++ b/store/storetest/mocks/TrueUpReviewStore.go @@ -37,13 +37,13 @@ func (_m *TrueUpReviewStore) CreateTrueUpReviewStatusRecord(reviewStatus *model. return r0, r1 } -// GetTrueUpReviewStatus provides a mock function with given fields: _a0 -func (_m *TrueUpReviewStore) GetTrueUpReviewStatus(_a0 int64) (*model.TrueUpReviewStatus, error) { - ret := _m.Called(_a0) +// GetTrueUpReviewStatus provides a mock function with given fields: dueDate +func (_m *TrueUpReviewStore) GetTrueUpReviewStatus(dueDate int64) (*model.TrueUpReviewStatus, error) { + ret := _m.Called(dueDate) var r0 *model.TrueUpReviewStatus if rf, ok := ret.Get(0).(func(int64) *model.TrueUpReviewStatus); ok { - r0 = rf(_a0) + r0 = rf(dueDate) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*model.TrueUpReviewStatus) @@ -52,7 +52,7 @@ func (_m *TrueUpReviewStore) GetTrueUpReviewStatus(_a0 int64) (*model.TrueUpRevi var r1 error if rf, ok := ret.Get(1).(func(int64) error); ok { - r1 = rf(_a0) + r1 = rf(dueDate) } else { r1 = ret.Error(1) } From d5a5686986834cfba9b1a4a262302a831413cd01 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Fri, 23 Dec 2022 16:48:19 -0500 Subject: [PATCH 32/58] Add admin and cloud checks back. --- api4/license.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/api4/license.go b/api4/license.go index ce6bff1798..506d44b0a0 100644 --- a/api4/license.go +++ b/api4/license.go @@ -437,10 +437,10 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { func trueUpReviewStatus(c *Context, w http.ResponseWriter, r *http.Request) { // Only admins can request a true up review. - // if !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionManageSystem) { - // c.SetPermissionError(model.PermissionManageLicenseInformation) - // return - // } + if !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionManageSystem) { + c.SetPermissionError(model.PermissionManageLicenseInformation) + return + } license := c.App.Channels().License() if license == nil { @@ -448,10 +448,10 @@ func trueUpReviewStatus(c *Context, w http.ResponseWriter, r *http.Request) { return } - // if c.App.Cloud() != nil { - // c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "api.license.true_up_review.not.allowed.for.cloud", nil, "", http.StatusNotImplemented) - // return - // } + if c.App.Cloud() != nil { + c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "api.license.true_up_review.not.allowed.for.cloud", nil, "", http.StatusNotImplemented) + return + } nextDueDate := utils.GetNextTrueUpReviewDueDate(time.Now()) status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(nextDueDate.UnixMilli()) From c633a1d2fa80fef4743638e203c5e4eda8944b46 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Tue, 27 Dec 2022 16:03:04 -0500 Subject: [PATCH 33/58] Code review comments. --- .../000101_create_true_up_review_history.up.sql | 2 +- model/true_up_review_profile.go | 4 ++-- store/sqlstore/store.go | 6 +++--- utils/license.go | 16 ++++++++-------- utils/license_test.go | 4 ++-- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/db/migrations/mysql/000101_create_true_up_review_history.up.sql b/db/migrations/mysql/000101_create_true_up_review_history.up.sql index 5ed4eeb6ec..5b25ffdb80 100644 --- a/db/migrations/mysql/000101_create_true_up_review_history.up.sql +++ b/db/migrations/mysql/000101_create_true_up_review_history.up.sql @@ -1,5 +1,5 @@ CREATE TABLE IF NOT EXISTS TrueUpReviewHistory ( DueDate bigint(20), Completed boolean, - PRIMARY KEY (duedate) + PRIMARY KEY (DueDate) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; diff --git a/model/true_up_review_profile.go b/model/true_up_review_profile.go index 7ef3bed75a..d396488ae4 100644 --- a/model/true_up_review_profile.go +++ b/model/true_up_review_profile.go @@ -25,8 +25,8 @@ type TrueUpReviewPlugins struct { InactivePluginNames []string `json:"inactive_plugin_names"` } -func (t *TrueUpReviewPlugins) ToMap() map[string]any { - return map[string]any{ +func (t *TrueUpReviewPlugins) ToMap() map[string]interface{} { + return map[string]interface{}{ "total_active_plugins": t.TotalActivePlugins, "total_inactive_plugins": t.TotalInactivePlugins, "active_plugin_names": t.ActivePluginNames, diff --git a/store/sqlstore/store.go b/store/sqlstore/store.go index 9a1ceceb2d..0337a0d735 100644 --- a/store/sqlstore/store.go +++ b/store/sqlstore/store.go @@ -112,7 +112,7 @@ type SqlStoreStores struct { notifyAdmin store.NotifyAdminStore postPriority store.PostPriorityStore postAcknowledgement store.PostAcknowledgementStore - trueUpReviewStatus store.TrueUpReviewStore + trueUpReview store.TrueUpReviewStore } type SqlStore struct { @@ -221,7 +221,7 @@ func New(settings model.SqlSettings, metrics einterfaces.MetricsInterface) *SqlS store.stores.notifyAdmin = newSqlNotifyAdminStore(store) store.stores.postPriority = newSqlPostPriorityStore(store) store.stores.postAcknowledgement = newSqlPostAcknowledgementStore(store) - store.stores.trueUpReviewStatus = newSqlTrueUpReviewStore(store) + store.stores.trueUpReview = newSqlTrueUpReviewStore(store) store.stores.preference.(*SqlPreferenceStore).deleteUnusedFeatures() @@ -978,7 +978,7 @@ func (ss *SqlStore) PostAcknowledgement() store.PostAcknowledgementStore { } func (ss *SqlStore) TrueUpReview() store.TrueUpReviewStore { - return ss.stores.trueUpReviewStatus + return ss.stores.trueUpReview } func (ss *SqlStore) DropAllTables() { diff --git a/utils/license.go b/utils/license.go index 69435f7a43..cca1978558 100644 --- a/utils/license.go +++ b/utils/license.go @@ -35,8 +35,8 @@ hwIDAQAB var LicenseValidator LicenseValidatorIface -const TrueUpReviewDueDay = 15 -const BusinessQuaterStep = 3 +const trueUpReviewDueDay = 15 +const businessQuarterStep = 3 func init() { if LicenseValidator == nil { @@ -232,16 +232,16 @@ func GetSanitizedClientLicense(l map[string]string) map[string]string { func GetNextTrueUpReviewDueDate(now time.Time) time.Time { quaterEndMonths := []time.Month{time.March, time.June, time.September, time.December} - var nextQuaterEndMonth time.Month = time.March + var nextQuarterEndMonth time.Month = time.March for _, month := range quaterEndMonths { - if now.Month() <= month && now.Day() <= TrueUpReviewDueDay { - nextQuaterEndMonth = month + if now.Month() <= month && now.Day() <= trueUpReviewDueDay { + nextQuarterEndMonth = month break - } else if now.Month() <= month && now.Day() > TrueUpReviewDueDay { - nextQuaterEndMonth = month + BusinessQuaterStep + } else if now.Month() <= month && now.Day() > trueUpReviewDueDay { + nextQuarterEndMonth = month + businessQuarterStep break } } - return time.Date(now.Year(), nextQuaterEndMonth, TrueUpReviewDueDay, 0, 0, 0, 0, now.Location()) + return time.Date(now.Year(), nextQuarterEndMonth, trueUpReviewDueDay, 0, 0, 0, 0, now.Location()) } diff --git a/utils/license_test.go b/utils/license_test.go index 541fda3227..159ebcdedd 100644 --- a/utils/license_test.go +++ b/utils/license_test.go @@ -111,7 +111,7 @@ func TestGetNextTrueUpReviewDueDate(t *testing.T) { assert.Equal(t, due.Day(), TrueUpReviewDueDay) }) - t.Run("Due date will always be in next quater if the current date is past the 15th", func(t *testing.T) { + t.Run("Due date will always be in next quarter if the current date is past the 15th", func(t *testing.T) { now := time.Date(2022, time.March, 16, 0, 0, 0, 0, time.Local) due := GetNextTrueUpReviewDueDate(now) assert.Equal(t, time.June, due.Month()) @@ -129,7 +129,7 @@ func TestGetNextTrueUpReviewDueDate(t *testing.T) { assert.Equal(t, time.March, due.Month()) }) - t.Run("Due date will always be in the current quater if the current date is before or on the 15th", func(t *testing.T) { + t.Run("Due date will always be in the current quarter if the current date is before or on the 15th", func(t *testing.T) { now := time.Date(2022, time.March, 15, 0, 0, 0, 0, time.Local) due := GetNextTrueUpReviewDueDate(now) assert.Equal(t, time.March, due.Month()) From 9c71784d0d8892faf34bc239e76ebbda2991b552 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Tue, 27 Dec 2022 16:54:46 -0500 Subject: [PATCH 34/58] Code review comments. --- api4/license.go | 45 +++++++++++++++++++++++++-------- api4/license_test.go | 18 +++++-------- model/true_up_review_profile.go | 6 +++-- utils/license_test.go | 6 ++--- 4 files changed, 47 insertions(+), 28 deletions(-) diff --git a/api4/license.go b/api4/license.go index 506d44b0a0..c8af641ea4 100644 --- a/api4/license.go +++ b/api4/license.go @@ -6,14 +6,17 @@ package api4 import ( "bytes" "encoding/json" + "errors" "fmt" "io" "net/http" "os" + "strings" "time" "github.com/mattermost/mattermost-server/v6/services/telemetry" "github.com/mattermost/mattermost-server/v6/shared/mlog" + "github.com/mattermost/mattermost-server/v6/store" "github.com/mattermost/mattermost-server/v6/utils" "github.com/mattermost/mattermost-server/v6/audit" @@ -315,15 +318,15 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { return } - if c.App.Cloud() != nil { - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.not.allowed.for.cloud", nil, "", http.StatusNotImplemented) + if license.IsCloud() { + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.not_allowed_for_cloud", nil, "", http.StatusNotImplemented) return } // Customer Info & Usage Analytics activeUserCount, err := c.App.Srv().Store().Status().GetTotalActiveUsersCount() if err != nil { - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.user.count.fail", nil, "", http.StatusInternalServerError) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.user_count_fail", nil, "", http.StatusInternalServerError) return } @@ -331,12 +334,12 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { incomingWebhookCount, err := c.App.Srv().Store().Webhook().AnalyticsIncomingCount("") if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.webhook.in.count.fail", nil, "", http.StatusInternalServerError) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.webhook_in_count_fail", nil, "", http.StatusInternalServerError) return } outgoingWebhookCount, err := c.App.Srv().Store().Webhook().AnalyticsOutgoingCount("") if err != nil { - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.webhook.out.count.fail", nil, "", http.StatusInternalServerError) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.webhook_out_count_fail", nil, "", http.StatusInternalServerError) return } @@ -407,8 +410,16 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { dueDate := utils.GetNextTrueUpReviewDueDate(time.Now()) status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(dueDate.UnixMilli()) if err != nil { - status, err = c.App.Srv().Store().TrueUpReview().CreateTrueUpReviewStatusRecord(status) + var nfErr *store.ErrNotFound + switch { + case errors.As(err, &nfErr): + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.status_not_found", nil, "", http.StatusNotFound).Wrap(err) + default: + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.get_status_error", nil, "", http.StatusInternalServerError).Wrap(err) + return + } + status, err = c.App.Srv().Store().TrueUpReview().CreateTrueUpReviewStatusRecord(status) if err != nil { c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.create.fail.app_error", nil, "", http.StatusInternalServerError) return @@ -422,9 +433,12 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { delete(telemetryProperties, "plugins") plugins := reviewProfile.Plugins.ToMap() for pluginName, pluginValue := range plugins { - telemetryProperties[pluginName] = pluginValue + telemetryProperties["plugin_"+pluginName] = pluginValue } + delete(telemetryProperties, "authentication_features") + telemetryProperties["authentication_features"] = strings.Join(reviewProfile.AuthenticationFeatures, ",") + telemetryService := c.App.Srv().GetTelemetryService() telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, telemetryProperties) @@ -444,22 +458,31 @@ func trueUpReviewStatus(c *Context, w http.ResponseWriter, r *http.Request) { license := c.App.Channels().License() if license == nil { - c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "api.license.true_up_review.license.required", nil, "", http.StatusNotImplemented) + c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "api.license.true_up_review.license_required", nil, "", http.StatusNotImplemented) return } - if c.App.Cloud() != nil { - c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "api.license.true_up_review.not.allowed.for.cloud", nil, "", http.StatusNotImplemented) + if license.IsCloud() { + c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "api.license.true_up_review.not_allowed_for_cloud", nil, "", http.StatusNotImplemented) return } nextDueDate := utils.GetNextTrueUpReviewDueDate(time.Now()) status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(nextDueDate.UnixMilli()) if err != nil { + var nfErr *store.ErrNotFound + switch { + case errors.As(err, &nfErr): + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.status_not_found", nil, "", http.StatusNotFound).Wrap(err) + default: + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.get_status_error", nil, "", http.StatusInternalServerError).Wrap(err) + return + } + status, err = c.App.Srv().Store().TrueUpReview().CreateTrueUpReviewStatusRecord(status) if err != nil { - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.create.fail.app_error", nil, "", http.StatusInternalServerError) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.create_error", nil, "", http.StatusInternalServerError) return } } diff --git a/api4/license_test.go b/api4/license_test.go index 7b3a2e9afd..b8eb703c8d 100644 --- a/api4/license_test.go +++ b/api4/license_test.go @@ -344,16 +344,13 @@ func TestRequestTrueUpReview(t *testing.T) { }) t.Run("returns 501 when ran by cloud user", func(t *testing.T) { - cloud := mocks.CloudInterface{} - cloudImpl := th.App.Srv().Cloud - th.App.Srv().Cloud = &cloud - defer func() { - th.App.Srv().Cloud = cloudImpl - }() + th.App.Srv().SetLicense(model.NewTestLicense("cloud")) resp, err := th.SystemAdminClient.DoAPIPost("/license/review", "") require.Error(t, err) require.Equal(t, http.StatusNotImplemented, resp.StatusCode) + + th.App.Srv().SetLicense(model.NewTestLicense()) }) t.Run("returns 403 when user does not have permissions", func(t *testing.T) { @@ -384,16 +381,13 @@ func TestTrueUpReviewStatus(t *testing.T) { }) t.Run("returns 501 when ran by cloud user", func(t *testing.T) { - cloud := mocks.CloudInterface{} - cloudImpl := th.App.Srv().Cloud - th.App.Srv().Cloud = &cloud - defer func() { - th.App.Srv().Cloud = cloudImpl - }() + th.App.Srv().SetLicense(model.NewTestLicense("cloud")) resp, err := th.SystemAdminClient.DoAPIGet("/license/review/status", "") require.Error(t, err) require.Equal(t, http.StatusNotImplemented, resp.StatusCode) + + th.App.Srv().SetLicense(model.NewTestLicense()) }) t.Run("returns 403 when user does not have permissions", func(t *testing.T) { diff --git a/model/true_up_review_profile.go b/model/true_up_review_profile.go index d396488ae4..74b62fe2e1 100644 --- a/model/true_up_review_profile.go +++ b/model/true_up_review_profile.go @@ -3,6 +3,8 @@ package model +import "strings" + type TrueUpReviewProfile struct { ServerId string `json:"server_id"` ServerVersion string `json:"server_version"` @@ -29,8 +31,8 @@ func (t *TrueUpReviewPlugins) ToMap() map[string]interface{} { return map[string]interface{}{ "total_active_plugins": t.TotalActivePlugins, "total_inactive_plugins": t.TotalInactivePlugins, - "active_plugin_names": t.ActivePluginNames, - "inactive_plugin_names": t.InactivePluginNames, + "active_plugin_names": strings.Join(t.ActivePluginNames, ","), + "inactive_plugin_names": strings.Join(t.InactivePluginNames, ","), } } diff --git a/utils/license_test.go b/utils/license_test.go index 159ebcdedd..b7e963fe69 100644 --- a/utils/license_test.go +++ b/utils/license_test.go @@ -98,17 +98,17 @@ func TestGetNextTrueUpReviewDueDate(t *testing.T) { // Before the 15th now := time.Date(2022, 12, 14, 0, 0, 0, 0, time.Local) due := GetNextTrueUpReviewDueDate(now) - assert.Equal(t, due.Day(), TrueUpReviewDueDay) + assert.Equal(t, due.Day(), trueUpReviewDueDay) // On the 15th now = time.Date(2022, 12, 15, 0, 0, 0, 0, time.Local) due = GetNextTrueUpReviewDueDate(now) - assert.Equal(t, due.Day(), TrueUpReviewDueDay) + assert.Equal(t, due.Day(), trueUpReviewDueDay) // After the 15th now = time.Date(2022, 12, 16, 0, 0, 0, 0, time.Local) due = GetNextTrueUpReviewDueDate(now) - assert.Equal(t, due.Day(), TrueUpReviewDueDay) + assert.Equal(t, due.Day(), trueUpReviewDueDay) }) t.Run("Due date will always be in next quarter if the current date is past the 15th", func(t *testing.T) { From 7048d83ca5480f4cbc85f3fbbb7f83baae9af7ee Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Tue, 27 Dec 2022 16:59:16 -0500 Subject: [PATCH 35/58] Add test to ensure GetNextTrueUpREviewDueDate will return a year in the future if necessary. --- utils/license_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/utils/license_test.go b/utils/license_test.go index b7e963fe69..7e031200a4 100644 --- a/utils/license_test.go +++ b/utils/license_test.go @@ -146,4 +146,11 @@ func TestGetNextTrueUpReviewDueDate(t *testing.T) { due = GetNextTrueUpReviewDueDate(now) assert.Equal(t, time.December, due.Month()) }) + + t.Run("Due date will be in the next year if the next quarter is not within the current year", func(t *testing.T) { + now := time.Date(2022, time.December, 18, 0, 0, 0, 0, time.Local) + due := GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.March, due.Month()) + assert.Equal(t, 2023, due.Year()) + }) } From 896974844a97bd74c190edcbabb8fcd0967c611b Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Tue, 27 Dec 2022 17:07:59 -0500 Subject: [PATCH 36/58] i18n --- i18n/en.json | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/i18n/en.json b/i18n/en.json index 94b78b2f23..c94c39a4a0 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -2081,25 +2081,41 @@ "id": "api.license.true_up_review.create.fail.app_error", "translation": "Unable to create a true up review record." }, + { + "id": "api.license.true_up_review.create_error", + "translation": "" + }, + { + "id": "api.license.true_up_review.get_status_error", + "translation": "" + }, { "id": "api.license.true_up_review.license.required", "translation": "A license is required to request a true up review." }, { - "id": "api.license.true_up_review.not.allowed.for.cloud", - "translation": "A true up review cannot be requested for cloud subscriptions." + "id": "api.license.true_up_review.license_required", + "translation": "" }, { - "id": "api.license.true_up_review.user.count.fail", - "translation": "Unable to get user counts." + "id": "api.license.true_up_review.not_allowed_for_cloud", + "translation": "" }, { - "id": "api.license.true_up_review.webhook.in.count.fail", - "translation": "Unable to get inbound webhook counts." + "id": "api.license.true_up_review.status_not_found", + "translation": "" }, { - "id": "api.license.true_up_review.webhook.out.count.fail", - "translation": "Unable to get outbound webhook counts." + "id": "api.license.true_up_review.user_count_fail", + "translation": "" + }, + { + "id": "api.license.true_up_review.webhook_in_count_fail", + "translation": "" + }, + { + "id": "api.license.true_up_review.webhook_out_count_fail", + "translation": "" }, { "id": "api.license.upgrade_needed.app_error", From ba1c4befa8451b2403f91b109dbf310ef81787ae Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Tue, 27 Dec 2022 17:20:22 -0500 Subject: [PATCH 37/58] Fix empty translations, add details to errors. --- api4/license.go | 22 +++++++++++----------- i18n/en.json | 20 ++++++++------------ 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/api4/license.go b/api4/license.go index c8af641ea4..ffa1dc16ba 100644 --- a/api4/license.go +++ b/api4/license.go @@ -326,7 +326,7 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { // Customer Info & Usage Analytics activeUserCount, err := c.App.Srv().Store().Status().GetTotalActiveUsersCount() if err != nil { - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.user_count_fail", nil, "", http.StatusInternalServerError) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.user_count_fail", nil, "Could not get the total active users count", http.StatusInternalServerError) return } @@ -334,12 +334,12 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { incomingWebhookCount, err := c.App.Srv().Store().Webhook().AnalyticsIncomingCount("") if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.webhook_in_count_fail", nil, "", http.StatusInternalServerError) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.webhook_in_count_fail", nil, "Could not get the total incoming webhook count", http.StatusInternalServerError) return } outgoingWebhookCount, err := c.App.Srv().Store().Webhook().AnalyticsOutgoingCount("") if err != nil { - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.webhook_out_count_fail", nil, "", http.StatusInternalServerError) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.webhook_out_count_fail", nil, "Could not get the total outgoing webhook count", http.StatusInternalServerError) return } @@ -413,15 +413,15 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { var nfErr *store.ErrNotFound switch { case errors.As(err, &nfErr): - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.status_not_found", nil, "", http.StatusNotFound).Wrap(err) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.status_not_found", nil, "Could not find any true up status records", http.StatusNotFound).Wrap(err) default: - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.get_status_error", nil, "", http.StatusInternalServerError).Wrap(err) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.get_status_error", nil, "Could not get true up status records", http.StatusInternalServerError).Wrap(err) return } status, err = c.App.Srv().Store().TrueUpReview().CreateTrueUpReviewStatusRecord(status) if err != nil { - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.create.fail.app_error", nil, "", http.StatusInternalServerError) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.create_error", nil, "Could not create true up status record", http.StatusInternalServerError) return } } @@ -458,12 +458,12 @@ func trueUpReviewStatus(c *Context, w http.ResponseWriter, r *http.Request) { license := c.App.Channels().License() if license == nil { - c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "api.license.true_up_review.license_required", nil, "", http.StatusNotImplemented) + c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "api.license.true_up_review.license_required", nil, "True up review requires a license", http.StatusNotImplemented) return } if license.IsCloud() { - c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "api.license.true_up_review.not_allowed_for_cloud", nil, "", http.StatusNotImplemented) + c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "api.license.true_up_review.not_allowed_for_cloud", nil, "True up review is not allowed for cloud instances", http.StatusNotImplemented) return } @@ -473,16 +473,16 @@ func trueUpReviewStatus(c *Context, w http.ResponseWriter, r *http.Request) { var nfErr *store.ErrNotFound switch { case errors.As(err, &nfErr): - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.status_not_found", nil, "", http.StatusNotFound).Wrap(err) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.status_not_found", nil, "Could not find any true up status records", http.StatusNotFound).Wrap(err) default: - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.get_status_error", nil, "", http.StatusInternalServerError).Wrap(err) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.get_status_error", nil, "Could not get true up status records", http.StatusInternalServerError).Wrap(err) return } status, err = c.App.Srv().Store().TrueUpReview().CreateTrueUpReviewStatusRecord(status) if err != nil { - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.create_error", nil, "", http.StatusInternalServerError) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.create_error", nil, "Could not create true up status record", http.StatusInternalServerError) return } } diff --git a/i18n/en.json b/i18n/en.json index c94c39a4a0..90e97fd899 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -2077,17 +2077,13 @@ "id": "api.license.request_trial_license.fail_get_user_count.app_error", "translation": "Unable to get a trial license, please try again or contact with support@mattermost.com. Cannot obtain the number of registered users." }, - { - "id": "api.license.true_up_review.create.fail.app_error", - "translation": "Unable to create a true up review record." - }, { "id": "api.license.true_up_review.create_error", - "translation": "" + "translation": "Could not create true up status record" }, { "id": "api.license.true_up_review.get_status_error", - "translation": "" + "translation": "Could not get true up status records" }, { "id": "api.license.true_up_review.license.required", @@ -2095,27 +2091,27 @@ }, { "id": "api.license.true_up_review.license_required", - "translation": "" + "translation": "True up review requires a license" }, { "id": "api.license.true_up_review.not_allowed_for_cloud", - "translation": "" + "translation": "True up review is not allowed for cloud instances" }, { "id": "api.license.true_up_review.status_not_found", - "translation": "" + "translation": "Could not find any true up status records" }, { "id": "api.license.true_up_review.user_count_fail", - "translation": "" + "translation": "Could not get the total active users count" }, { "id": "api.license.true_up_review.webhook_in_count_fail", - "translation": "" + "translation": "Could not get the total incoming webhook count" }, { "id": "api.license.true_up_review.webhook_out_count_fail", - "translation": "" + "translation": "Could not get the total outgoing webhook count" }, { "id": "api.license.upgrade_needed.app_error", From f42f7bdaf82238048aa8812a01fed8fcd5aa1ab6 Mon Sep 17 00:00:00 2001 From: Conor Macpherson <116016004+ConorMacpherson@users.noreply.github.com> Date: Wed, 28 Dec 2022 09:27:23 -0500 Subject: [PATCH 38/58] Update api4/license.go typo Co-authored-by: Allan Guwatudde --- api4/license.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api4/license.go b/api4/license.go index ffa1dc16ba..4e4464b669 100644 --- a/api4/license.go +++ b/api4/license.go @@ -367,7 +367,7 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { ldapUsed := config.LdapSettings.Enable samlUsed := config.SamlSettings.Enable openIdUsed := config.OpenIdSettings.Enable - guessAccessAllowed := config.GuestAccountsSettings.Enable + guestAccessAllowed := config.GuestAccountsSettings.Enable authFeatures := map[string]*bool{ model.TrueUpReviewAuthFeaturesMfa: mfaUsed, From 3c0375d18e208e7ba6a31fbf3774de0920f5736c Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Wed, 28 Dec 2022 09:56:58 -0500 Subject: [PATCH 39/58] Fix error type checks, change log levels, update comments. --- api4/license.go | 17 +++++++++++------ store/sqlstore/true_up_review_store.go | 9 ++++++--- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/api4/license.go b/api4/license.go index ffa1dc16ba..0f77b2dcb4 100644 --- a/api4/license.go +++ b/api4/license.go @@ -407,18 +407,21 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { return } - dueDate := utils.GetNextTrueUpReviewDueDate(time.Now()) - status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(dueDate.UnixMilli()) + dueDate := utils.GetNextTrueUpReviewDueDate(time.Now()).UnixMilli() + status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(dueDate) if err != nil { + // Check error. Continue if the status was just not found. var nfErr *store.ErrNotFound switch { case errors.As(err, &nfErr): - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.status_not_found", nil, "Could not find any true up status records", http.StatusNotFound).Wrap(err) + c.Logger.Warn("Could not find true up review status") default: c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.get_status_error", nil, "Could not get true up status records", http.StatusInternalServerError).Wrap(err) return } + // No status was found, so create a new one. + status = &model.TrueUpReviewStatus{DueDate: dueDate, Completed: false} status, err = c.App.Srv().Store().TrueUpReview().CreateTrueUpReviewStatusRecord(status) if err != nil { c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.create_error", nil, "Could not create true up status record", http.StatusInternalServerError) @@ -428,7 +431,7 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { // Do not send true-up review data if the user has already requested one for the quarter. if !status.Completed { - // Send telemetry data. + // "Flatten" telemetry data. json.Unmarshal(reviewProfileJson, &telemetryProperties) delete(telemetryProperties, "plugins") plugins := reviewProfile.Plugins.ToMap() @@ -439,9 +442,11 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { delete(telemetryProperties, "authentication_features") telemetryProperties["authentication_features"] = strings.Join(reviewProfile.AuthenticationFeatures, ",") + // Send telemetry data telemetryService := c.App.Srv().GetTelemetryService() telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, telemetryProperties) + // Update the review status to reflect the completion. status.Completed = true c.App.Srv().Store().TrueUpReview().Update(status) } @@ -473,14 +478,14 @@ func trueUpReviewStatus(c *Context, w http.ResponseWriter, r *http.Request) { var nfErr *store.ErrNotFound switch { case errors.As(err, &nfErr): - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.status_not_found", nil, "Could not find any true up status records", http.StatusNotFound).Wrap(err) + c.Logger.Warn("Could not find true up review status") default: c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.get_status_error", nil, "Could not get true up status records", http.StatusInternalServerError).Wrap(err) return } + status = &model.TrueUpReviewStatus{DueDate: nextDueDate.UnixMilli(), Completed: false} status, err = c.App.Srv().Store().TrueUpReview().CreateTrueUpReviewStatusRecord(status) - if err != nil { c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.create_error", nil, "Could not create true up status record", http.StatusInternalServerError) return diff --git a/store/sqlstore/true_up_review_store.go b/store/sqlstore/true_up_review_store.go index 81ca07c300..45cf955849 100644 --- a/store/sqlstore/true_up_review_store.go +++ b/store/sqlstore/true_up_review_store.go @@ -4,6 +4,9 @@ package sqlstore import ( + "database/sql" + "strconv" + "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/mattermost-server/v6/store" sq "github.com/mattermost/squirrel" @@ -39,9 +42,9 @@ func (s *SqlTrueUpReviewStore) GetTrueUpReviewStatus(dueDate int64) (*model.True } var trueUpReviewStatus model.TrueUpReviewStatus if err := s.GetReplicaX().Get(&trueUpReviewStatus, queryString, args...); err != nil { - trueUpReviewStatus.Completed = false - trueUpReviewStatus.DueDate = dueDate - return &trueUpReviewStatus, err + if err == sql.ErrNoRows { + return nil, store.NewErrNotFound("TrueUpReviewStatus", strconv.FormatInt(dueDate, 10)) + } } return &trueUpReviewStatus, nil From a839646757b372e8e7073d9ca06287f48c1db303 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Wed, 28 Dec 2022 10:01:20 -0500 Subject: [PATCH 40/58] Fix typo, ensure we don't throw away errors that are not store.NotFound. --- api4/license.go | 2 +- store/sqlstore/true_up_review_store.go | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/api4/license.go b/api4/license.go index 5581daa906..47f484cbb1 100644 --- a/api4/license.go +++ b/api4/license.go @@ -374,7 +374,7 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { model.TrueUpReviewAuthFeaturesADLdap: ldapUsed, model.TrueUpReviewAuthFeaturesSaml: samlUsed, model.TrueUpReviewAuthFeatureOpenId: openIdUsed, - model.TrueUpReviewAuthFeatureGuestAccess: guessAccessAllowed, + model.TrueUpReviewAuthFeatureGuestAccess: guestAccessAllowed, } authFeatureList := []string{} diff --git a/store/sqlstore/true_up_review_store.go b/store/sqlstore/true_up_review_store.go index 45cf955849..23688ce5c1 100644 --- a/store/sqlstore/true_up_review_store.go +++ b/store/sqlstore/true_up_review_store.go @@ -45,6 +45,8 @@ func (s *SqlTrueUpReviewStore) GetTrueUpReviewStatus(dueDate int64) (*model.True if err == sql.ErrNoRows { return nil, store.NewErrNotFound("TrueUpReviewStatus", strconv.FormatInt(dueDate, 10)) } + + return nil, err } return &trueUpReviewStatus, nil From 46c28e42574348eb12105db7e0336a697a87c68f Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Wed, 28 Dec 2022 10:07:24 -0500 Subject: [PATCH 41/58] i18n --- i18n/en.json | 4 ---- 1 file changed, 4 deletions(-) diff --git a/i18n/en.json b/i18n/en.json index 90e97fd899..a4cde3d1c8 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -2097,10 +2097,6 @@ "id": "api.license.true_up_review.not_allowed_for_cloud", "translation": "True up review is not allowed for cloud instances" }, - { - "id": "api.license.true_up_review.status_not_found", - "translation": "Could not find any true up status records" - }, { "id": "api.license.true_up_review.user_count_fail", "translation": "Could not get the total active users count" From 6685268085783bb826016cf31983dd3719eb65f4 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Thu, 29 Dec 2022 14:58:05 -0500 Subject: [PATCH 42/58] Add cws availability check. --- api4/cloud.go | 12 ++++++++++++ einterfaces/cloud.go | 2 ++ einterfaces/mocks/CloudInterface.go | 14 ++++++++++++++ i18n/en.json | 4 ++++ model/client4.go | 11 +++++++++++ 5 files changed, 43 insertions(+) diff --git a/api4/cloud.go b/api4/cloud.go index d517291d48..b9beec397b 100644 --- a/api4/cloud.go +++ b/api4/cloud.go @@ -52,6 +52,9 @@ func (api *API) InitCloud() { // POST /api/v4/cloud/webhook api.BaseRoutes.Cloud.Handle("/webhook", api.CloudAPIKeyRequired(handleCWSWebhook)).Methods("POST") + + // GET /api/v4/cloud/cws-health-check + api.BaseRoutes.Cloud.Handle("/check-cws-connection", api.APIHandler(handleCheckCWSConnection)).Methods("GET") } func getSubscription(c *Context, w http.ResponseWriter, r *http.Request) { @@ -746,3 +749,12 @@ func handleCWSWebhook(c *Context, w http.ResponseWriter, r *http.Request) { ReturnStatusOK(w) } + +func handleCheckCWSConnection(c *Context, w http.ResponseWriter, r *http.Request) { + if err := c.App.Cloud().CheckCWSConnection(); err != nil { + c.Err = model.NewAppError("Api4.handleCWSHealthCheck", "api.server.cws.health_check.app_error", nil, "CWS Server is not available.", http.StatusInternalServerError) + return + } + + ReturnStatusOK(w) +} diff --git a/einterfaces/cloud.go b/einterfaces/cloud.go index 0c70c02cf3..dadca484ff 100644 --- a/einterfaces/cloud.go +++ b/einterfaces/cloud.go @@ -45,4 +45,6 @@ type CloudInterface interface { CreateOrUpdateSubscriptionHistoryEvent(userID string, userCount int) (*model.SubscriptionHistory, error) HandleLicenseChange() error + + CheckCWSConnection() error } diff --git a/einterfaces/mocks/CloudInterface.go b/einterfaces/mocks/CloudInterface.go index a447399cd8..b0ef53c14f 100644 --- a/einterfaces/mocks/CloudInterface.go +++ b/einterfaces/mocks/CloudInterface.go @@ -60,6 +60,20 @@ func (_m *CloudInterface) ChangeSubscription(userID string, subscriptionID strin return r0, r1 } +// CheckCWSConnection provides a mock function with given fields: +func (_m *CloudInterface) CheckCWSConnection() error { + ret := _m.Called() + + var r0 error + if rf, ok := ret.Get(0).(func() error); ok { + r0 = rf() + } else { + r0 = ret.Error(0) + } + + return r0 +} + // ConfirmCustomerPayment provides a mock function with given fields: userID, confirmRequest func (_m *CloudInterface) ConfirmCustomerPayment(userID string, confirmRequest *model.ConfirmPaymentMethodRequest) error { ret := _m.Called(userID, confirmRequest) diff --git a/i18n/en.json b/i18n/en.json index a4cde3d1c8..93716f8a4b 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -2575,6 +2575,10 @@ "id": "api.scheme.patch_scheme.license.error", "translation": "Your license does not support update permissions schemes" }, + { + "id": "api.server.cws.health_check.app_error", + "translation": "CWS Server is not available." + }, { "id": "api.server.hosted_signup_unavailable.error", "translation": "Portal unavailable for self-hosted signup." diff --git a/model/client4.go b/model/client4.go index 298334d71a..85699eac6c 100644 --- a/model/client4.go +++ b/model/client4.go @@ -8700,6 +8700,17 @@ func (c *Client4) AddUserToGroupSyncables(userID string) (*Response, error) { return BuildResponse(r), nil } +func (c *Client4) CheckCWSConnection() (*Response, error) { + r, err := c.DoAPIGet(c.cloudRoute()+"/healthz", "") + + if err != nil { + return BuildResponse(r), err + } + defer closeBody(r) + + return BuildResponse(r), nil +} + // Worktemplates sections func (c *Client4) worktemplatesRoute() string { From 4e54a40a219c2d27026b81ebe0e4236313be5b67 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Thu, 29 Dec 2022 15:07:06 -0500 Subject: [PATCH 43/58] Add cws availability check. --- api4/cloud.go | 2 +- einterfaces/cloud.go | 2 +- einterfaces/mocks/CloudInterface.go | 10 +++++----- model/client4.go | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/api4/cloud.go b/api4/cloud.go index b9beec397b..e9493a32d4 100644 --- a/api4/cloud.go +++ b/api4/cloud.go @@ -751,7 +751,7 @@ func handleCWSWebhook(c *Context, w http.ResponseWriter, r *http.Request) { } func handleCheckCWSConnection(c *Context, w http.ResponseWriter, r *http.Request) { - if err := c.App.Cloud().CheckCWSConnection(); err != nil { + if err := c.App.Cloud().CheckCWSConnection(c.AppContext.Session().UserId); err != nil { c.Err = model.NewAppError("Api4.handleCWSHealthCheck", "api.server.cws.health_check.app_error", nil, "CWS Server is not available.", http.StatusInternalServerError) return } diff --git a/einterfaces/cloud.go b/einterfaces/cloud.go index dadca484ff..bfaabae67e 100644 --- a/einterfaces/cloud.go +++ b/einterfaces/cloud.go @@ -46,5 +46,5 @@ type CloudInterface interface { CreateOrUpdateSubscriptionHistoryEvent(userID string, userCount int) (*model.SubscriptionHistory, error) HandleLicenseChange() error - CheckCWSConnection() error + CheckCWSConnection(userId string) error } diff --git a/einterfaces/mocks/CloudInterface.go b/einterfaces/mocks/CloudInterface.go index b0ef53c14f..4b19465161 100644 --- a/einterfaces/mocks/CloudInterface.go +++ b/einterfaces/mocks/CloudInterface.go @@ -60,13 +60,13 @@ func (_m *CloudInterface) ChangeSubscription(userID string, subscriptionID strin return r0, r1 } -// CheckCWSConnection provides a mock function with given fields: -func (_m *CloudInterface) CheckCWSConnection() error { - ret := _m.Called() +// CheckCWSConnection provides a mock function with given fields: userId +func (_m *CloudInterface) CheckCWSConnection(userId string) error { + ret := _m.Called(userId) var r0 error - if rf, ok := ret.Get(0).(func() error); ok { - r0 = rf() + if rf, ok := ret.Get(0).(func(string) error); ok { + r0 = rf(userId) } else { r0 = ret.Error(0) } diff --git a/model/client4.go b/model/client4.go index 85699eac6c..64bf163312 100644 --- a/model/client4.go +++ b/model/client4.go @@ -8700,7 +8700,7 @@ func (c *Client4) AddUserToGroupSyncables(userID string) (*Response, error) { return BuildResponse(r), nil } -func (c *Client4) CheckCWSConnection() (*Response, error) { +func (c *Client4) CheckCWSConnection(userId string) (*Response, error) { r, err := c.DoAPIGet(c.cloudRoute()+"/healthz", "") if err != nil { From 408d752b5cf5a6d6171e76f73d6b087339354524 Mon Sep 17 00:00:00 2001 From: Nathaniel Allred Date: Thu, 29 Dec 2022 14:27:45 -0600 Subject: [PATCH 44/58] dry up some code. Move other code to app layer (#21961) * dry up some code. Move other code to app layer * check err --- api4/license.go | 163 ++++++--------------------- app/app_iface.go | 1 + app/opentracing/opentracing_layer.go | 22 ++++ app/true_up.go | 119 +++++++++++++++++++ 4 files changed, 176 insertions(+), 129 deletions(-) create mode 100644 app/true_up.go diff --git a/api4/license.go b/api4/license.go index 47f484cbb1..471caeefd6 100644 --- a/api4/license.go +++ b/api4/license.go @@ -10,11 +10,8 @@ import ( "fmt" "io" "net/http" - "os" - "strings" "time" - "github.com/mattermost/mattermost-server/v6/services/telemetry" "github.com/mattermost/mattermost-server/v6/shared/mlog" "github.com/mattermost/mattermost-server/v6/store" "github.com/mattermost/mattermost-server/v6/utils" @@ -305,6 +302,29 @@ func getPrevTrialLicense(c *Context, w http.ResponseWriter, r *http.Request) { w.Write([]byte(model.MapToJSON(clientLicense))) } +func getOrCreateTrueUpReviewStatus(c *Context) (*model.TrueUpReviewStatus, bool) { + nextDueDate := utils.GetNextTrueUpReviewDueDate(time.Now()) + status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(nextDueDate.UnixMilli()) + if err != nil { + var nfErr *store.ErrNotFound + switch { + case errors.As(err, &nfErr): + c.Logger.Warn("Could not find true up review status") + default: + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.get_status_error", nil, "Could not get true up status records", http.StatusInternalServerError).Wrap(err) + return nil, false + } + + status, err = c.App.Srv().Store().TrueUpReview().CreateTrueUpReviewStatusRecord(&model.TrueUpReviewStatus{DueDate: nextDueDate.UnixMilli(), Completed: false}) + if err != nil { + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.create_error", nil, "Could not create true up status record", http.StatusInternalServerError) + return nil, false + } + } + + return status, true +} + func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { // Only admins can request a true up review. if !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionManageSystem) { @@ -323,135 +343,35 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { return } - // Customer Info & Usage Analytics - activeUserCount, err := c.App.Srv().Store().Status().GetTotalActiveUsersCount() + profileMap, err := c.App.GetTrueUpProfile() if err != nil { - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.user_count_fail", nil, "Could not get the total active users count", http.StatusInternalServerError) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.get_status_error", nil, "", http.StatusInternalServerError) return } - // Webhook, calls, boards, and playbook counts - incomingWebhookCount, err := c.App.Srv().Store().Webhook().AnalyticsIncomingCount("") + profileMapJson, err := json.Marshal(profileMap) if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.webhook_in_count_fail", nil, "Could not get the total incoming webhook count", http.StatusInternalServerError) - return - } - outgoingWebhookCount, err := c.App.Srv().Store().Webhook().AnalyticsOutgoingCount("") - if err != nil { - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.webhook_out_count_fail", nil, "Could not get the total outgoing webhook count", http.StatusInternalServerError) + c.SetJSONEncodingError(err) return } - // Plugin Data - trueUpReviewPlugins := model.TrueUpReviewPlugins{ - ActivePluginNames: []string{}, - InactivePluginNames: []string{}, - } - - if pluginResponse, err := c.App.GetPlugins(); err == nil { - for _, plugin := range pluginResponse.Active { - trueUpReviewPlugins.ActivePluginNames = append(trueUpReviewPlugins.ActivePluginNames, plugin.Name) - } - trueUpReviewPlugins.TotalActivePlugins = len(trueUpReviewPlugins.ActivePluginNames) - - for _, plugin := range pluginResponse.Inactive { - trueUpReviewPlugins.InactivePluginNames = append(trueUpReviewPlugins.InactivePluginNames, plugin.Name) - } - trueUpReviewPlugins.TotalInactivePlugins = len(trueUpReviewPlugins.InactivePluginNames) - } - - // Authentication Features - config := c.App.Config() - mfaUsed := config.ServiceSettings.EnforceMultifactorAuthentication - ldapUsed := config.LdapSettings.Enable - samlUsed := config.SamlSettings.Enable - openIdUsed := config.OpenIdSettings.Enable - guestAccessAllowed := config.GuestAccountsSettings.Enable - - authFeatures := map[string]*bool{ - model.TrueUpReviewAuthFeaturesMfa: mfaUsed, - model.TrueUpReviewAuthFeaturesADLdap: ldapUsed, - model.TrueUpReviewAuthFeaturesSaml: samlUsed, - model.TrueUpReviewAuthFeatureOpenId: openIdUsed, - model.TrueUpReviewAuthFeatureGuestAccess: guestAccessAllowed, - } - - authFeatureList := []string{} - for feature, used := range authFeatures { - if used != nil && *used { - authFeatureList = append(authFeatureList, feature) - } - } - - reviewProfile := model.TrueUpReviewProfile{ - ServerId: c.App.TelemetryId(), - ServerVersion: model.CurrentVersion, - ServerInstallationType: os.Getenv(telemetry.EnvVarInstallType), - LicenseId: license.Id, - LicensedSeats: *license.Features.Users, - LicensePlan: license.SkuName, - CustomerName: license.Customer.Name, - ActiveUsers: activeUserCount, - TotalIncomingWebhooks: incomingWebhookCount, - TotalOutgoingWebhooks: outgoingWebhookCount, - Plugins: trueUpReviewPlugins, - AuthenticationFeatures: authFeatureList, - } - - // Convert true up review profile struct to map - var telemetryProperties map[string]interface{} - reviewProfileJson, err := json.Marshal(reviewProfile) - if err != nil { - c.Err = model.NewAppError("requestTrueUpReview", "api.marshal_error", nil, "", http.StatusInternalServerError) + status, ok := getOrCreateTrueUpReviewStatus(c) + if !ok { return } - dueDate := utils.GetNextTrueUpReviewDueDate(time.Now()).UnixMilli() - status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(dueDate) - if err != nil { - // Check error. Continue if the status was just not found. - var nfErr *store.ErrNotFound - switch { - case errors.As(err, &nfErr): - c.Logger.Warn("Could not find true up review status") - default: - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.get_status_error", nil, "Could not get true up status records", http.StatusInternalServerError).Wrap(err) - return - } - - // No status was found, so create a new one. - status = &model.TrueUpReviewStatus{DueDate: dueDate, Completed: false} - status, err = c.App.Srv().Store().TrueUpReview().CreateTrueUpReviewStatusRecord(status) - if err != nil { - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.create_error", nil, "Could not create true up status record", http.StatusInternalServerError) - return - } - } - // Do not send true-up review data if the user has already requested one for the quarter. if !status.Completed { - // "Flatten" telemetry data. - json.Unmarshal(reviewProfileJson, &telemetryProperties) - delete(telemetryProperties, "plugins") - plugins := reviewProfile.Plugins.ToMap() - for pluginName, pluginValue := range plugins { - telemetryProperties["plugin_"+pluginName] = pluginValue - } - - delete(telemetryProperties, "authentication_features") - telemetryProperties["authentication_features"] = strings.Join(reviewProfile.AuthenticationFeatures, ",") - // Send telemetry data telemetryService := c.App.Srv().GetTelemetryService() - telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, telemetryProperties) + telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, profileMap) // Update the review status to reflect the completion. status.Completed = true c.App.Srv().Store().TrueUpReview().Update(status) } - w.Write(reviewProfileJson) + w.Write(profileMapJson) } func trueUpReviewStatus(c *Context, w http.ResponseWriter, r *http.Request) { @@ -472,24 +392,9 @@ func trueUpReviewStatus(c *Context, w http.ResponseWriter, r *http.Request) { return } - nextDueDate := utils.GetNextTrueUpReviewDueDate(time.Now()) - status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(nextDueDate.UnixMilli()) - if err != nil { - var nfErr *store.ErrNotFound - switch { - case errors.As(err, &nfErr): - c.Logger.Warn("Could not find true up review status") - default: - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.get_status_error", nil, "Could not get true up status records", http.StatusInternalServerError).Wrap(err) - return - } - - status = &model.TrueUpReviewStatus{DueDate: nextDueDate.UnixMilli(), Completed: false} - status, err = c.App.Srv().Store().TrueUpReview().CreateTrueUpReviewStatusRecord(status) - if err != nil { - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.create_error", nil, "Could not create true up status record", http.StatusInternalServerError) - return - } + status, ok := getOrCreateTrueUpReviewStatus(c) + if !ok { + return } json, err := json.Marshal(status) diff --git a/app/app_iface.go b/app/app_iface.go index f10ca2d8b9..4b197e2dd3 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -811,6 +811,7 @@ type AppIface interface { GetTopReactionsForUserSince(userID string, teamID string, opts *model.InsightsOpts) (*model.TopReactionList, *model.AppError) GetTopThreadsForTeamSince(c request.CTX, teamID, userID string, opts *model.InsightsOpts) (*model.TopThreadList, *model.AppError) GetTopThreadsForUserSince(c request.CTX, teamID, userID string, opts *model.InsightsOpts) (*model.TopThreadList, *model.AppError) + GetTrueUpProfile() (map[string]any, error) GetUploadSession(c request.CTX, uploadId string) (*model.UploadSession, *model.AppError) GetUploadSessionsForUser(userID string) ([]*model.UploadSession, *model.AppError) GetUser(userID string) (*model.User, *model.AppError) diff --git a/app/opentracing/opentracing_layer.go b/app/opentracing/opentracing_layer.go index 367d98ec39..220f461921 100644 --- a/app/opentracing/opentracing_layer.go +++ b/app/opentracing/opentracing_layer.go @@ -10328,6 +10328,28 @@ func (a *OpenTracingAppLayer) GetTotalUsersStats(viewRestrictions *model.ViewUse return resultVar0, resultVar1 } +func (a *OpenTracingAppLayer) GetTrueUpProfile() (map[string]any, error) { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.GetTrueUpProfile") + + a.ctx = newCtx + a.app.Srv().Store().SetContext(newCtx) + defer func() { + a.app.Srv().Store().SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + resultVar0, resultVar1 := a.app.GetTrueUpProfile() + + if resultVar1 != nil { + span.LogFields(spanlog.Error(resultVar1)) + ext.Error.Set(span, true) + } + + return resultVar0, resultVar1 +} + func (a *OpenTracingAppLayer) GetUploadSession(c request.CTX, uploadId string) (*model.UploadSession, *model.AppError) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.GetUploadSession") diff --git a/app/true_up.go b/app/true_up.go new file mode 100644 index 0000000000..790d7a8353 --- /dev/null +++ b/app/true_up.go @@ -0,0 +1,119 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package app + +import ( + "encoding/json" + "net/http" + "os" + "strings" + + "github.com/mattermost/mattermost-server/v6/model" + "github.com/mattermost/mattermost-server/v6/services/telemetry" +) + +func (a *App) getTrueUpProfile() (*model.TrueUpReviewProfile, error) { + + license := a.Channels().License() + // Customer Info & Usage Analytics + activeUserCount, err := a.Srv().Store().Status().GetTotalActiveUsersCount() + if err != nil { + return nil, model.NewAppError("requestTrueUpReview", "api.license.true_up_review.user_count_fail", nil, "Could not get the total active users count", http.StatusInternalServerError) + } + + // Webhook, calls, boards, and playbook counts + incomingWebhookCount, err := a.Srv().Store().Webhook().AnalyticsIncomingCount("") + if err != nil { + return nil, model.NewAppError("requestTrueUpReview", "api.license.true_up_review.webhook_in_count_fail", nil, "Could not get the total incoming webhook count", http.StatusInternalServerError) + } + outgoingWebhookCount, err := a.Srv().Store().Webhook().AnalyticsOutgoingCount("") + if err != nil { + return nil, model.NewAppError("requestTrueUpReview", "api.license.true_up_review.webhook_out_count_fail", nil, "Could not get the total outgoing webhook count", http.StatusInternalServerError) + } + + // Plugin Data + trueUpReviewPlugins := model.TrueUpReviewPlugins{ + ActivePluginNames: []string{}, + InactivePluginNames: []string{}, + } + + if pluginResponse, err := a.GetPlugins(); err == nil { + for _, plugin := range pluginResponse.Active { + trueUpReviewPlugins.ActivePluginNames = append(trueUpReviewPlugins.ActivePluginNames, plugin.Name) + } + trueUpReviewPlugins.TotalActivePlugins = len(trueUpReviewPlugins.ActivePluginNames) + + for _, plugin := range pluginResponse.Inactive { + trueUpReviewPlugins.InactivePluginNames = append(trueUpReviewPlugins.InactivePluginNames, plugin.Name) + } + trueUpReviewPlugins.TotalInactivePlugins = len(trueUpReviewPlugins.InactivePluginNames) + } + + // Authentication Features + config := a.Config() + mfaUsed := config.ServiceSettings.EnforceMultifactorAuthentication + ldapUsed := config.LdapSettings.Enable + samlUsed := config.SamlSettings.Enable + openIdUsed := config.OpenIdSettings.Enable + guestAccessAllowed := config.GuestAccountsSettings.Enable + + authFeatures := map[string]*bool{ + model.TrueUpReviewAuthFeaturesMfa: mfaUsed, + model.TrueUpReviewAuthFeaturesADLdap: ldapUsed, + model.TrueUpReviewAuthFeaturesSaml: samlUsed, + model.TrueUpReviewAuthFeatureOpenId: openIdUsed, + model.TrueUpReviewAuthFeatureGuestAccess: guestAccessAllowed, + } + + authFeatureList := []string{} + for feature, used := range authFeatures { + if used != nil && *used { + authFeatureList = append(authFeatureList, feature) + } + } + + reviewProfile := model.TrueUpReviewProfile{ + ServerId: a.TelemetryId(), + ServerVersion: model.CurrentVersion, + ServerInstallationType: os.Getenv(telemetry.EnvVarInstallType), + LicenseId: license.Id, + LicensedSeats: *license.Features.Users, + LicensePlan: license.SkuName, + CustomerName: license.Customer.Name, + ActiveUsers: activeUserCount, + TotalIncomingWebhooks: incomingWebhookCount, + TotalOutgoingWebhooks: outgoingWebhookCount, + Plugins: trueUpReviewPlugins, + AuthenticationFeatures: authFeatureList, + } + + return &reviewProfile, nil + +} + +func (a *App) GetTrueUpProfile() (map[string]any, error) { + profile, err := a.getTrueUpProfile() + + if err != nil { + return nil, err + } + + profileJson, err := json.Marshal(profile) + if err != nil { + return nil, err + } + telemetryProperties := map[string]any{} + + json.Unmarshal(profileJson, &telemetryProperties) + delete(telemetryProperties, "plugins") + plugins := profile.Plugins.ToMap() + for pluginName, pluginValue := range plugins { + telemetryProperties["plugin_"+pluginName] = pluginValue + } + + delete(telemetryProperties, "authentication_features") + telemetryProperties["authentication_features"] = strings.Join(profile.AuthenticationFeatures, ",") + + return telemetryProperties, nil +} From 49c5ca69dfaff8d13b1455b9985f07986727f3ed Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Thu, 29 Dec 2022 17:11:50 -0500 Subject: [PATCH 45/58] Add comment to re-run checks. --- api4/license.go | 1 + 1 file changed, 1 insertion(+) diff --git a/api4/license.go b/api4/license.go index 471caeefd6..fa83ddf61e 100644 --- a/api4/license.go +++ b/api4/license.go @@ -381,6 +381,7 @@ func trueUpReviewStatus(c *Context, w http.ResponseWriter, r *http.Request) { return } + // Check for license license := c.App.Channels().License() if license == nil { c.Err = model.NewAppError("cloudTrueUpReviewNotAllowed", "api.license.true_up_review.license_required", nil, "True up review requires a license", http.StatusNotImplemented) From c30439743cafba7b5d168a41698a32dfa9942b1d Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Thu, 29 Dec 2022 19:47:58 -0500 Subject: [PATCH 46/58] Ensure content is base64 encoded, add check for telemetry enabled. --- api4/license.go | 17 ++++++++++++++--- model/true_up_review_profile.go | 5 +++-- services/telemetry/telemetry.go | 4 ++-- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/api4/license.go b/api4/license.go index fa83ddf61e..4bf6ed741c 100644 --- a/api4/license.go +++ b/api4/license.go @@ -5,6 +5,7 @@ package api4 import ( "bytes" + b64 "encoding/base64" "encoding/json" "errors" "fmt" @@ -322,6 +323,8 @@ func getOrCreateTrueUpReviewStatus(c *Context) (*model.TrueUpReviewStatus, bool) } } + telemetryService := c.App.Srv().GetTelemetryService() + status.TelemetryEnabled = telemetryService.TelemetryEnabled() return status, true } @@ -361,9 +364,10 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { } // Do not send true-up review data if the user has already requested one for the quarter. - if !status.Completed { + // And only send a true-up review via as a one-time telemetry request if telemetry is disabled. + telemetryService := c.App.Srv().GetTelemetryService() + if !status.Completed && !telemetryService.TelemetryEnabled() { // Send telemetry data - telemetryService := c.App.Srv().GetTelemetryService() telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, profileMap) // Update the review status to reflect the completion. @@ -371,7 +375,14 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { c.App.Srv().Store().TrueUpReview().Update(status) } - w.Write(profileMapJson) + // Encode to string rather than byte[] otherwise json.Marshal will encode it further. + encodedData := b64.StdEncoding.EncodeToString(profileMapJson) + responseContent := struct { + Content string `json:"content"` + }{Content: encodedData} + response, _ := json.Marshal(responseContent) + + w.Write(response) } func trueUpReviewStatus(c *Context, w http.ResponseWriter, r *http.Request) { diff --git a/model/true_up_review_profile.go b/model/true_up_review_profile.go index 74b62fe2e1..250032dcf2 100644 --- a/model/true_up_review_profile.go +++ b/model/true_up_review_profile.go @@ -37,8 +37,9 @@ func (t *TrueUpReviewPlugins) ToMap() map[string]interface{} { } type TrueUpReviewStatus struct { - Completed bool `json:"complete"` - DueDate int64 `json:"due_date"` + Completed bool `json:"complete"` + DueDate int64 `json:"due_date"` + TelemetryEnabled bool `json:"telemetry_enabled"` } func (t *TrueUpReviewStatus) ToSlice() []interface{} { diff --git a/services/telemetry/telemetry.go b/services/telemetry/telemetry.go index ec88b868c0..5f319b03a5 100644 --- a/services/telemetry/telemetry.go +++ b/services/telemetry/telemetry.go @@ -149,13 +149,13 @@ func (ts *TelemetryService) getRudderConfig() RudderConfig { } } -func (ts *TelemetryService) telemetryEnabled() bool { +func (ts *TelemetryService) TelemetryEnabled() bool { return *ts.srv.Config().LogSettings.EnableDiagnostics && ts.srv.IsLeader() } func (ts *TelemetryService) sendDailyTelemetry(override bool) { config := ts.getRudderConfig() - if ts.telemetryEnabled() && ((config.DataplaneURL != "" && config.RudderKey != "") || override) { + if ts.TelemetryEnabled() && ((config.DataplaneURL != "" && config.RudderKey != "") || override) { ts.initRudder(config.DataplaneURL, config.RudderKey) ts.trackActivity() ts.trackConfig() From b3b39e4f15b1222427bc2cf555623e532dcc6929 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Fri, 30 Dec 2022 13:11:17 -0500 Subject: [PATCH 47/58] Move get or create true up review status into the app interface, adds a function to execute checks for being within true up review widnow (and tests), adds a job for sending true up telemetry. --- api4/license.go | 39 ++++------------------ app/app_iface.go | 1 + app/opentracing/opentracing_layer.go | 22 +++++++++++++ app/server.go | 7 ++++ app/true_up.go | 27 +++++++++++++++ jobs/license_true_up/scheduler.go | 25 ++++++++++++++ jobs/license_true_up/worker.go | 47 ++++++++++++++++++++++++++ model/job.go | 1 + utils/license.go | 12 +++++++ utils/license_test.go | 49 ++++++++++++++++++++++++++++ 10 files changed, 197 insertions(+), 33 deletions(-) create mode 100644 jobs/license_true_up/scheduler.go create mode 100644 jobs/license_true_up/worker.go diff --git a/api4/license.go b/api4/license.go index 4bf6ed741c..56c4f50d38 100644 --- a/api4/license.go +++ b/api4/license.go @@ -7,14 +7,11 @@ import ( "bytes" b64 "encoding/base64" "encoding/json" - "errors" "fmt" "io" "net/http" - "time" "github.com/mattermost/mattermost-server/v6/shared/mlog" - "github.com/mattermost/mattermost-server/v6/store" "github.com/mattermost/mattermost-server/v6/utils" "github.com/mattermost/mattermost-server/v6/audit" @@ -303,31 +300,6 @@ func getPrevTrialLicense(c *Context, w http.ResponseWriter, r *http.Request) { w.Write([]byte(model.MapToJSON(clientLicense))) } -func getOrCreateTrueUpReviewStatus(c *Context) (*model.TrueUpReviewStatus, bool) { - nextDueDate := utils.GetNextTrueUpReviewDueDate(time.Now()) - status, err := c.App.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(nextDueDate.UnixMilli()) - if err != nil { - var nfErr *store.ErrNotFound - switch { - case errors.As(err, &nfErr): - c.Logger.Warn("Could not find true up review status") - default: - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.get_status_error", nil, "Could not get true up status records", http.StatusInternalServerError).Wrap(err) - return nil, false - } - - status, err = c.App.Srv().Store().TrueUpReview().CreateTrueUpReviewStatusRecord(&model.TrueUpReviewStatus{DueDate: nextDueDate.UnixMilli(), Completed: false}) - if err != nil { - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.create_error", nil, "Could not create true up status record", http.StatusInternalServerError) - return nil, false - } - } - - telemetryService := c.App.Srv().GetTelemetryService() - status.TelemetryEnabled = telemetryService.TelemetryEnabled() - return status, true -} - func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { // Only admins can request a true up review. if !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionManageSystem) { @@ -358,8 +330,9 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { return } - status, ok := getOrCreateTrueUpReviewStatus(c) - if !ok { + status, appErr := c.App.GetOrCreateTrueUpReviewStatus() + if err != nil { + c.Err = appErr return } @@ -404,9 +377,9 @@ func trueUpReviewStatus(c *Context, w http.ResponseWriter, r *http.Request) { return } - status, ok := getOrCreateTrueUpReviewStatus(c) - if !ok { - return + status, appErr := c.App.GetOrCreateTrueUpReviewStatus() + if appErr != nil { + c.Err = appErr } json, err := json.Marshal(status) diff --git a/app/app_iface.go b/app/app_iface.go index 4b197e2dd3..09f544c605 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -699,6 +699,7 @@ type AppIface interface { GetOnboarding() (*model.System, *model.AppError) GetOpenGraphMetadata(requestURL string) ([]byte, error) GetOrCreateDirectChannel(c request.CTX, userID, otherUserID string, channelOptions ...model.ChannelOption) (*model.Channel, *model.AppError) + GetOrCreateTrueUpReviewStatus() (*model.TrueUpReviewStatus, *model.AppError) GetOutgoingWebhook(hookID string) (*model.OutgoingWebhook, *model.AppError) GetOutgoingWebhooksForChannelPageByUser(channelID string, userID string, page, perPage int) ([]*model.OutgoingWebhook, *model.AppError) GetOutgoingWebhooksForTeamPage(teamID string, page, perPage int) ([]*model.OutgoingWebhook, *model.AppError) diff --git a/app/opentracing/opentracing_layer.go b/app/opentracing/opentracing_layer.go index 220f461921..4d9d37de6e 100644 --- a/app/opentracing/opentracing_layer.go +++ b/app/opentracing/opentracing_layer.go @@ -7584,6 +7584,28 @@ func (a *OpenTracingAppLayer) GetOrCreateDirectChannel(c request.CTX, userID str return resultVar0, resultVar1 } +func (a *OpenTracingAppLayer) GetOrCreateTrueUpReviewStatus() (*model.TrueUpReviewStatus, *model.AppError) { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.GetOrCreateTrueUpReviewStatus") + + a.ctx = newCtx + a.app.Srv().Store().SetContext(newCtx) + defer func() { + a.app.Srv().Store().SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + resultVar0, resultVar1 := a.app.GetOrCreateTrueUpReviewStatus() + + if resultVar1 != nil { + span.LogFields(spanlog.Error(resultVar1)) + ext.Error.Set(span, true) + } + + return resultVar0, resultVar1 +} + func (a *OpenTracingAppLayer) GetOutgoingWebhook(hookID string) (*model.OutgoingWebhook, *model.AppError) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.GetOutgoingWebhook") diff --git a/app/server.go b/app/server.go index fbd433a6db..2c6507c245 100644 --- a/app/server.go +++ b/app/server.go @@ -45,6 +45,7 @@ import ( "github.com/mattermost/mattermost-server/v6/jobs/import_process" "github.com/mattermost/mattermost-server/v6/jobs/last_accessible_file" "github.com/mattermost/mattermost-server/v6/jobs/last_accessible_post" + "github.com/mattermost/mattermost-server/v6/jobs/license_true_up" "github.com/mattermost/mattermost-server/v6/jobs/migrations" "github.com/mattermost/mattermost-server/v6/jobs/notify_admin" "github.com/mattermost/mattermost-server/v6/jobs/product_notices" @@ -1529,6 +1530,12 @@ func (s *Server) initJobs() { notify_admin.MakeScheduler(s.Jobs, s.License(), model.JobTypeTrialNotifyAdmin), ) + s.Jobs.RegisterJobType( + model.JobTypeLicenseTrueUpReview, + license_true_up.MakeWorker(s.Jobs, s.License(), New(ServerConnector(s.Channels())), s.telemetryService), + license_true_up.MakeScheduler(s.Jobs, s.License(), s.telemetryService), + ) + s.platform.Jobs = s.Jobs } diff --git a/app/true_up.go b/app/true_up.go index 790d7a8353..7240fc4e90 100644 --- a/app/true_up.go +++ b/app/true_up.go @@ -5,12 +5,16 @@ package app import ( "encoding/json" + "errors" "net/http" "os" "strings" + "time" "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/mattermost-server/v6/services/telemetry" + "github.com/mattermost/mattermost-server/v6/store" + "github.com/mattermost/mattermost-server/v6/utils" ) func (a *App) getTrueUpProfile() (*model.TrueUpReviewProfile, error) { @@ -117,3 +121,26 @@ func (a *App) GetTrueUpProfile() (map[string]any, error) { return telemetryProperties, nil } + +func (a *App) GetOrCreateTrueUpReviewStatus() (*model.TrueUpReviewStatus, *model.AppError) { + nextDueDate := utils.GetNextTrueUpReviewDueDate(time.Now()) + status, err := a.Srv().Store().TrueUpReview().GetTrueUpReviewStatus(nextDueDate.UnixMilli()) + if err != nil { + var nfErr *store.ErrNotFound + switch { + case errors.As(err, &nfErr): + a.Log().Warn("Could not find true up review status") + default: + return nil, model.NewAppError("requestTrueUpReview", "api.license.true_up_review.get_status_error", nil, "Could not get true up status records", http.StatusInternalServerError).Wrap(err) + } + + status, err = a.Srv().Store().TrueUpReview().CreateTrueUpReviewStatusRecord(&model.TrueUpReviewStatus{DueDate: nextDueDate.UnixMilli(), Completed: false}) + if err != nil { + return nil, model.NewAppError("requestTrueUpReview", "api.license.true_up_review.create_error", nil, "Could not create true up status record", http.StatusInternalServerError) + } + } + + telemetryService := a.Srv().GetTelemetryService() + status.TelemetryEnabled = telemetryService.TelemetryEnabled() + return status, nil +} diff --git a/jobs/license_true_up/scheduler.go b/jobs/license_true_up/scheduler.go new file mode 100644 index 0000000000..e5b7dae3ad --- /dev/null +++ b/jobs/license_true_up/scheduler.go @@ -0,0 +1,25 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package license_true_up + +import ( + "strconv" + "time" + + "github.com/mattermost/mattermost-server/v6/jobs" + "github.com/mattermost/mattermost-server/v6/model" + "github.com/mattermost/mattermost-server/v6/services/telemetry" + "github.com/mattermost/mattermost-server/v6/shared/mlog" +) + +const scheduleFrequency = time.Hour * 24 + +func MakeScheduler(jobServer *jobs.JobServer, license *model.License, telemetryService *telemetry.TelemetryService) model.Scheduler { + isEnabled := func(cfg *model.Config) bool { + enabled := license != nil && !*license.Features.Cloud && !license.IsTrialLicense() && telemetryService.TelemetryEnabled() + mlog.Debug("Scheduler: isEnabled: "+strconv.FormatBool(enabled), mlog.String("scheduler", model.JobTypeLicenseTrueUpReview)) + return enabled + } + return jobs.NewPeriodicScheduler(jobServer, model.JobTypeLicenseTrueUpReview, scheduleFrequency, isEnabled) +} diff --git a/jobs/license_true_up/worker.go b/jobs/license_true_up/worker.go new file mode 100644 index 0000000000..a9dc4a6e8e --- /dev/null +++ b/jobs/license_true_up/worker.go @@ -0,0 +1,47 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package license_true_up + +import ( + "time" + + "github.com/mattermost/mattermost-server/v6/jobs" + "github.com/mattermost/mattermost-server/v6/model" + "github.com/mattermost/mattermost-server/v6/services/telemetry" + "github.com/mattermost/mattermost-server/v6/utils" +) + +const ( + JobName = "LicenseTrueUpReview" +) + +type AppIface interface { + GetTrueUpProfile() (map[string]any, error) +} + +func MakeWorker(jobServer *jobs.JobServer, license *model.License, app AppIface, telemetryService *telemetry.TelemetryService) model.Worker { + isEnabled := func(_ *model.Config) bool { + return license != nil && !*license.Features.Cloud && !license.IsTrialLicense() && telemetryService.TelemetryEnabled() + } + + execute := func(job *model.Job) error { + defer jobServer.HandleJobPanic(job) + + // Ensure we are within the due date + dueDate := utils.GetNextTrueUpReviewDueDate(time.Now()) + if !utils.IsTrueUpReviewDueDateWithinTheNextTwoWeeks(time.Now(), dueDate) { + return nil + } + profile, err := app.GetTrueUpProfile() + if err != nil { + return err + } + + telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, profile) + + return nil + } + worker := jobs.NewSimpleWorker(JobName, jobServer, execute, isEnabled) + return worker +} diff --git a/model/job.go b/model/job.go index c63d260d85..b90892e4e1 100644 --- a/model/job.go +++ b/model/job.go @@ -31,6 +31,7 @@ const ( JobTypeLastAccessibleFile = "last_accessible_file" JobTypeUpgradeNotifyAdmin = "upgrade_notify_admin" JobTypeTrialNotifyAdmin = "trial_notify_admin" + JobTypeLicenseTrueUpReview = "license_true_up_review" JobStatusPending = "pending" JobStatusInProgress = "in_progress" diff --git a/utils/license.go b/utils/license.go index cca1978558..eb49b3e165 100644 --- a/utils/license.go +++ b/utils/license.go @@ -37,6 +37,8 @@ var LicenseValidator LicenseValidatorIface const trueUpReviewDueDay = 15 const businessQuarterStep = 3 +const day = time.Hour * 24 +const week = day * 7 func init() { if LicenseValidator == nil { @@ -245,3 +247,13 @@ func GetNextTrueUpReviewDueDate(now time.Time) time.Time { return time.Date(now.Year(), nextQuarterEndMonth, trueUpReviewDueDay, 0, 0, 0, 0, now.Location()) } + +func IsTrueUpReviewDueDateWithinTheNextTwoWeeks(now time.Time, dueDate time.Time) bool { + dueDateWindow := dueDate.Add(-(week * 2)) + + if now.Before(dueDateWindow) || now.After(dueDate) { + return false + } + + return true +} diff --git a/utils/license_test.go b/utils/license_test.go index 7e031200a4..57c57a6a3c 100644 --- a/utils/license_test.go +++ b/utils/license_test.go @@ -154,3 +154,52 @@ func TestGetNextTrueUpReviewDueDate(t *testing.T) { assert.Equal(t, 2023, due.Year()) }) } + +func TestIsTrueUpReviewDueDateWithinTheNextTwoWeeks(t *testing.T) { + t.Run("Ensure a date within two weeks before the due date returns true", func(t *testing.T) { + // 1 Day before the due date + now := time.Date(2022, time.December, 14, 0, 0, 0, 0, time.Local) + // Due date is December 15th, 2022 + due := GetNextTrueUpReviewDueDate(now) + + res := IsTrueUpReviewDueDateWithinTheNextTwoWeeks(now, due) + assert.True(t, res) + }) + + t.Run("Ensure a date that is more than two weeks before the due date returns false", func(t *testing.T) { + // 15 Days before the due date + now := time.Date(2022, time.November, 30, 0, 0, 0, 0, time.Local) + // Due date is December 15th, 2022 + due := GetNextTrueUpReviewDueDate(now) + + res := IsTrueUpReviewDueDateWithinTheNextTwoWeeks(now, due) + assert.False(t, res) + }) + + t.Run("Ensure a date that past the due date returns false", func(t *testing.T) { + now := time.Date(2022, time.December, 16, 0, 0, 0, 0, time.Local) + + // Due date is December 15th, 2022 + dueNow := time.Date(2022, time.December, 15, 0, 0, 0, 0, time.Local) + due := GetNextTrueUpReviewDueDate(dueNow) + + res := IsTrueUpReviewDueDateWithinTheNextTwoWeeks(now, due) + assert.False(t, res) + }) + + t.Run("Ensure a date that is on the due date returns true", func(t *testing.T) { + now := time.Date(2022, time.December, 15, 0, 0, 0, 0, time.Local) + due := GetNextTrueUpReviewDueDate(now) + + res := IsTrueUpReviewDueDateWithinTheNextTwoWeeks(now, due) + assert.True(t, res) + }) + + t.Run("Ensure a date that is on the first day of the due date window returns true", func(t *testing.T) { + now := time.Date(2022, time.December, 1, 0, 0, 0, 0, time.Local) + due := GetNextTrueUpReviewDueDate(now) + + res := IsTrueUpReviewDueDateWithinTheNextTwoWeeks(now, due) + assert.True(t, res) + }) +} From 84fe5514fc7f33ea2aa1d3c1455332f6a43dc802 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Fri, 30 Dec 2022 15:04:30 -0500 Subject: [PATCH 48/58] One last cleanup, i18n, etc. --- api4/license.go | 2 +- app/true_up.go | 4 ++++ i18n/en.json | 4 ---- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/api4/license.go b/api4/license.go index 56c4f50d38..9fab5cc38c 100644 --- a/api4/license.go +++ b/api4/license.go @@ -309,7 +309,7 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { license := c.App.Channels().License() if license == nil { - c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.license.required", nil, "", http.StatusNotImplemented) + c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.license_required", nil, "", http.StatusNotImplemented) return } diff --git a/app/true_up.go b/app/true_up.go index 7240fc4e90..b80ed77333 100644 --- a/app/true_up.go +++ b/app/true_up.go @@ -20,6 +20,10 @@ import ( func (a *App) getTrueUpProfile() (*model.TrueUpReviewProfile, error) { license := a.Channels().License() + if license == nil { + return nil, model.NewAppError("requestTrueUpReview", "api.license.true_up_review.license_required", nil, "Could not get the total active users count", http.StatusInternalServerError) + } + // Customer Info & Usage Analytics activeUserCount, err := a.Srv().Store().Status().GetTotalActiveUsersCount() if err != nil { diff --git a/i18n/en.json b/i18n/en.json index 93716f8a4b..bf696f48a5 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -2085,10 +2085,6 @@ "id": "api.license.true_up_review.get_status_error", "translation": "Could not get true up status records" }, - { - "id": "api.license.true_up_review.license.required", - "translation": "A license is required to request a true up review." - }, { "id": "api.license.true_up_review.license_required", "translation": "True up review requires a license" From e4fe76d01f71784f31b60f18061a470008a15018 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Tue, 3 Jan 2023 11:47:05 -0500 Subject: [PATCH 49/58] Only pull active plugins from the mattermost marketplace. --- app/true_up.go | 44 ++++++++++++++++++++++++--------- model/true_up_review_profile.go | 12 +++------ services/telemetry/telemetry.go | 4 +-- 3 files changed, 38 insertions(+), 22 deletions(-) diff --git a/app/true_up.go b/app/true_up.go index b80ed77333..b782e956ea 100644 --- a/app/true_up.go +++ b/app/true_up.go @@ -17,6 +17,34 @@ import ( "github.com/mattermost/mattermost-server/v6/utils" ) +func pluginActivated(pluginStates map[string]*model.PluginState, pluginId string) bool { + state, ok := pluginStates[pluginId] + if !ok { + return false + } + return state.Enable +} + +func (a *App) getMarketplacePlugins() ([]string, error) { + ts := a.Srv().telemetryService + config := a.Srv().Config() + + marketplacePlugins, err := ts.GetAllMarketplacePlugins(model.PluginSettingsDefaultMarketplaceURL) + if err != nil { + return nil, err + } + + activePlugins := []string{} + for _, p := range marketplacePlugins { + id := p.Manifest.Id + if pluginActivated(config.PluginSettings.PluginStates, id) { + activePlugins = append(activePlugins, id) + } + } + + return activePlugins, nil +} + func (a *App) getTrueUpProfile() (*model.TrueUpReviewProfile, error) { license := a.Channels().License() @@ -42,20 +70,12 @@ func (a *App) getTrueUpProfile() (*model.TrueUpReviewProfile, error) { // Plugin Data trueUpReviewPlugins := model.TrueUpReviewPlugins{ - ActivePluginNames: []string{}, - InactivePluginNames: []string{}, + PluginNames: []string{}, } - if pluginResponse, err := a.GetPlugins(); err == nil { - for _, plugin := range pluginResponse.Active { - trueUpReviewPlugins.ActivePluginNames = append(trueUpReviewPlugins.ActivePluginNames, plugin.Name) - } - trueUpReviewPlugins.TotalActivePlugins = len(trueUpReviewPlugins.ActivePluginNames) - - for _, plugin := range pluginResponse.Inactive { - trueUpReviewPlugins.InactivePluginNames = append(trueUpReviewPlugins.InactivePluginNames, plugin.Name) - } - trueUpReviewPlugins.TotalInactivePlugins = len(trueUpReviewPlugins.InactivePluginNames) + if plugins, err := a.getMarketplacePlugins(); err == nil { + trueUpReviewPlugins.PluginNames = plugins + trueUpReviewPlugins.TotalPlugins = len(plugins) } // Authentication Features diff --git a/model/true_up_review_profile.go b/model/true_up_review_profile.go index 250032dcf2..bb6b6d9dd6 100644 --- a/model/true_up_review_profile.go +++ b/model/true_up_review_profile.go @@ -21,18 +21,14 @@ type TrueUpReviewProfile struct { } type TrueUpReviewPlugins struct { - TotalActivePlugins int `json:"total_active_plugins"` - TotalInactivePlugins int `json:"total_inactive_plugins"` - ActivePluginNames []string `json:"active_plugin_names"` - InactivePluginNames []string `json:"inactive_plugin_names"` + TotalPlugins int `json:"total_plugins"` + PluginNames []string `json:"plugin_names"` } func (t *TrueUpReviewPlugins) ToMap() map[string]interface{} { return map[string]interface{}{ - "total_active_plugins": t.TotalActivePlugins, - "total_inactive_plugins": t.TotalInactivePlugins, - "active_plugin_names": strings.Join(t.ActivePluginNames, ","), - "inactive_plugin_names": strings.Join(t.InactivePluginNames, ","), + "total_plugins": t.TotalPlugins, + "plugin_names": strings.Join(t.PluginNames, ","), } } diff --git a/services/telemetry/telemetry.go b/services/telemetry/telemetry.go index 5f319b03a5..a426fbd80b 100644 --- a/services/telemetry/telemetry.go +++ b/services/telemetry/telemetry.go @@ -1418,7 +1418,7 @@ func (ts *TelemetryService) trackPluginConfig(cfg *model.Config, marketplaceURL "focalboard", } - marketplacePlugins, err := ts.getAllMarketplaceplugins(marketplaceURL) + marketplacePlugins, err := ts.GetAllMarketplacePlugins(marketplaceURL) if err != nil { mlog.Info("Failed to fetch marketplace plugins for telemetry. Using predefined list.", mlog.Err(err)) @@ -1456,7 +1456,7 @@ func (ts *TelemetryService) trackPluginConfig(cfg *model.Config, marketplaceURL ts.SendTelemetry(TrackConfigPlugin, pluginConfigData) } -func (ts *TelemetryService) getAllMarketplaceplugins(marketplaceURL string) ([]*model.BaseMarketplacePlugin, error) { +func (ts *TelemetryService) GetAllMarketplacePlugins(marketplaceURL string) ([]*model.BaseMarketplacePlugin, error) { marketplaceClient, err := marketplace.NewClient( marketplaceURL, ts.srv.HTTPService(), From e31a6a47b02c455880623d65515cadabd578552d Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Tue, 3 Jan 2023 13:35:31 -0500 Subject: [PATCH 50/58] Only check if diagnostics are disabled, remove telemetry enabled in true up review status response in favor of using front-end configs. --- api4/license.go | 6 +++--- app/true_up.go | 2 -- model/true_up_review_profile.go | 5 ++--- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/api4/license.go b/api4/license.go index 9fab5cc38c..880447ed58 100644 --- a/api4/license.go +++ b/api4/license.go @@ -338,10 +338,10 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { // Do not send true-up review data if the user has already requested one for the quarter. // And only send a true-up review via as a one-time telemetry request if telemetry is disabled. - telemetryService := c.App.Srv().GetTelemetryService() - if !status.Completed && !telemetryService.TelemetryEnabled() { + telemetryEnabled := c.App.Config().LogSettings.EnableDiagnostics + if !status.Completed && telemetryEnabled != nil && !*telemetryEnabled { // Send telemetry data - telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, profileMap) + c.App.Srv().GetTelemetryService().SendTelemetry(model.TrueUpReviewTelemetryName, profileMap) // Update the review status to reflect the completion. status.Completed = true diff --git a/app/true_up.go b/app/true_up.go index b782e956ea..3e3c779f31 100644 --- a/app/true_up.go +++ b/app/true_up.go @@ -164,7 +164,5 @@ func (a *App) GetOrCreateTrueUpReviewStatus() (*model.TrueUpReviewStatus, *model } } - telemetryService := a.Srv().GetTelemetryService() - status.TelemetryEnabled = telemetryService.TelemetryEnabled() return status, nil } diff --git a/model/true_up_review_profile.go b/model/true_up_review_profile.go index bb6b6d9dd6..8e4d186b75 100644 --- a/model/true_up_review_profile.go +++ b/model/true_up_review_profile.go @@ -33,9 +33,8 @@ func (t *TrueUpReviewPlugins) ToMap() map[string]interface{} { } type TrueUpReviewStatus struct { - Completed bool `json:"complete"` - DueDate int64 `json:"due_date"` - TelemetryEnabled bool `json:"telemetry_enabled"` + Completed bool `json:"complete"` + DueDate int64 `json:"due_date"` } func (t *TrueUpReviewStatus) ToSlice() []interface{} { From a8fc0ddf176bcdbf809cb6b70662da004f9a4820 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Wed, 4 Jan 2023 13:31:16 -0500 Subject: [PATCH 51/58] Remove true up review job, remove unneeded export of telemetryEnabled function. --- jobs/license_true_up/scheduler.go | 25 ---------------- jobs/license_true_up/worker.go | 47 ------------------------------- services/telemetry/telemetry.go | 4 +-- 3 files changed, 2 insertions(+), 74 deletions(-) delete mode 100644 jobs/license_true_up/scheduler.go delete mode 100644 jobs/license_true_up/worker.go diff --git a/jobs/license_true_up/scheduler.go b/jobs/license_true_up/scheduler.go deleted file mode 100644 index e5b7dae3ad..0000000000 --- a/jobs/license_true_up/scheduler.go +++ /dev/null @@ -1,25 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -package license_true_up - -import ( - "strconv" - "time" - - "github.com/mattermost/mattermost-server/v6/jobs" - "github.com/mattermost/mattermost-server/v6/model" - "github.com/mattermost/mattermost-server/v6/services/telemetry" - "github.com/mattermost/mattermost-server/v6/shared/mlog" -) - -const scheduleFrequency = time.Hour * 24 - -func MakeScheduler(jobServer *jobs.JobServer, license *model.License, telemetryService *telemetry.TelemetryService) model.Scheduler { - isEnabled := func(cfg *model.Config) bool { - enabled := license != nil && !*license.Features.Cloud && !license.IsTrialLicense() && telemetryService.TelemetryEnabled() - mlog.Debug("Scheduler: isEnabled: "+strconv.FormatBool(enabled), mlog.String("scheduler", model.JobTypeLicenseTrueUpReview)) - return enabled - } - return jobs.NewPeriodicScheduler(jobServer, model.JobTypeLicenseTrueUpReview, scheduleFrequency, isEnabled) -} diff --git a/jobs/license_true_up/worker.go b/jobs/license_true_up/worker.go deleted file mode 100644 index a9dc4a6e8e..0000000000 --- a/jobs/license_true_up/worker.go +++ /dev/null @@ -1,47 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -package license_true_up - -import ( - "time" - - "github.com/mattermost/mattermost-server/v6/jobs" - "github.com/mattermost/mattermost-server/v6/model" - "github.com/mattermost/mattermost-server/v6/services/telemetry" - "github.com/mattermost/mattermost-server/v6/utils" -) - -const ( - JobName = "LicenseTrueUpReview" -) - -type AppIface interface { - GetTrueUpProfile() (map[string]any, error) -} - -func MakeWorker(jobServer *jobs.JobServer, license *model.License, app AppIface, telemetryService *telemetry.TelemetryService) model.Worker { - isEnabled := func(_ *model.Config) bool { - return license != nil && !*license.Features.Cloud && !license.IsTrialLicense() && telemetryService.TelemetryEnabled() - } - - execute := func(job *model.Job) error { - defer jobServer.HandleJobPanic(job) - - // Ensure we are within the due date - dueDate := utils.GetNextTrueUpReviewDueDate(time.Now()) - if !utils.IsTrueUpReviewDueDateWithinTheNextTwoWeeks(time.Now(), dueDate) { - return nil - } - profile, err := app.GetTrueUpProfile() - if err != nil { - return err - } - - telemetryService.SendTelemetry(model.TrueUpReviewTelemetryName, profile) - - return nil - } - worker := jobs.NewSimpleWorker(JobName, jobServer, execute, isEnabled) - return worker -} diff --git a/services/telemetry/telemetry.go b/services/telemetry/telemetry.go index a426fbd80b..3783b2bd46 100644 --- a/services/telemetry/telemetry.go +++ b/services/telemetry/telemetry.go @@ -149,13 +149,13 @@ func (ts *TelemetryService) getRudderConfig() RudderConfig { } } -func (ts *TelemetryService) TelemetryEnabled() bool { +func (ts *TelemetryService) telemetryEnabled() bool { return *ts.srv.Config().LogSettings.EnableDiagnostics && ts.srv.IsLeader() } func (ts *TelemetryService) sendDailyTelemetry(override bool) { config := ts.getRudderConfig() - if ts.TelemetryEnabled() && ((config.DataplaneURL != "" && config.RudderKey != "") || override) { + if ts.telemetryEnabled() && ((config.DataplaneURL != "" && config.RudderKey != "") || override) { ts.initRudder(config.DataplaneURL, config.RudderKey) ts.trackActivity() ts.trackConfig() From bdd7cb4638749bface22925167f22a0bf4f8c3be Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Wed, 4 Jan 2023 13:45:21 -0500 Subject: [PATCH 52/58] Remove job reigstration and name. --- app/server.go | 7 ------- model/job.go | 1 - 2 files changed, 8 deletions(-) diff --git a/app/server.go b/app/server.go index 834af8570a..dc66353ee4 100644 --- a/app/server.go +++ b/app/server.go @@ -45,7 +45,6 @@ import ( "github.com/mattermost/mattermost-server/v6/jobs/import_process" "github.com/mattermost/mattermost-server/v6/jobs/last_accessible_file" "github.com/mattermost/mattermost-server/v6/jobs/last_accessible_post" - "github.com/mattermost/mattermost-server/v6/jobs/license_true_up" "github.com/mattermost/mattermost-server/v6/jobs/migrations" "github.com/mattermost/mattermost-server/v6/jobs/notify_admin" "github.com/mattermost/mattermost-server/v6/jobs/product_notices" @@ -1530,12 +1529,6 @@ func (s *Server) initJobs() { notify_admin.MakeScheduler(s.Jobs, s.License(), model.JobTypeTrialNotifyAdmin), ) - s.Jobs.RegisterJobType( - model.JobTypeLicenseTrueUpReview, - license_true_up.MakeWorker(s.Jobs, s.License(), New(ServerConnector(s.Channels())), s.telemetryService), - license_true_up.MakeScheduler(s.Jobs, s.License(), s.telemetryService), - ) - s.platform.Jobs = s.Jobs } diff --git a/model/job.go b/model/job.go index b90892e4e1..c63d260d85 100644 --- a/model/job.go +++ b/model/job.go @@ -31,7 +31,6 @@ const ( JobTypeLastAccessibleFile = "last_accessible_file" JobTypeUpgradeNotifyAdmin = "upgrade_notify_admin" JobTypeTrialNotifyAdmin = "trial_notify_admin" - JobTypeLicenseTrueUpReview = "license_true_up_review" JobStatusPending = "pending" JobStatusInProgress = "in_progress" From 8d604f715a8d0221ff0f564b4fd8bb1a0a3f8407 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Thu, 5 Jan 2023 15:31:58 -0500 Subject: [PATCH 53/58] Change logic for due dates. Previosuly they were 15 days after the first days of the quarters last month. Now, due dates are 15 days after the last day of the quarters last month. --- utils/license.go | 33 ----------- utils/license_test.go | 112 ------------------------------------- utils/true_up.go | 85 ++++++++++++++++++++++++++++ utils/true_up_test.go | 125 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 210 insertions(+), 145 deletions(-) create mode 100644 utils/true_up.go create mode 100644 utils/true_up_test.go diff --git a/utils/license.go b/utils/license.go index eb49b3e165..0c6b7dbe04 100644 --- a/utils/license.go +++ b/utils/license.go @@ -16,7 +16,6 @@ import ( "os" "path/filepath" "strconv" - "time" "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/mattermost-server/v6/shared/mlog" @@ -35,11 +34,6 @@ hwIDAQAB var LicenseValidator LicenseValidatorIface -const trueUpReviewDueDay = 15 -const businessQuarterStep = 3 -const day = time.Hour * 24 -const week = day * 7 - func init() { if LicenseValidator == nil { LicenseValidator = &LicenseValidatorImpl{} @@ -230,30 +224,3 @@ func GetSanitizedClientLicense(l map[string]string) map[string]string { return sanitizedLicense } - -func GetNextTrueUpReviewDueDate(now time.Time) time.Time { - quaterEndMonths := []time.Month{time.March, time.June, time.September, time.December} - - var nextQuarterEndMonth time.Month = time.March - for _, month := range quaterEndMonths { - if now.Month() <= month && now.Day() <= trueUpReviewDueDay { - nextQuarterEndMonth = month - break - } else if now.Month() <= month && now.Day() > trueUpReviewDueDay { - nextQuarterEndMonth = month + businessQuarterStep - break - } - } - - return time.Date(now.Year(), nextQuarterEndMonth, trueUpReviewDueDay, 0, 0, 0, 0, now.Location()) -} - -func IsTrueUpReviewDueDateWithinTheNextTwoWeeks(now time.Time, dueDate time.Time) bool { - dueDateWindow := dueDate.Add(-(week * 2)) - - if now.Before(dueDateWindow) || now.After(dueDate) { - return false - } - - return true -} diff --git a/utils/license_test.go b/utils/license_test.go index 57c57a6a3c..8c57685444 100644 --- a/utils/license_test.go +++ b/utils/license_test.go @@ -8,7 +8,6 @@ import ( "encoding/base64" "os" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -92,114 +91,3 @@ func TestGetLicenseFileFromDisk(t *testing.T) { assert.False(t, success, "should have been an invalid file") }) } - -func TestGetNextTrueUpReviewDueDate(t *testing.T) { - t.Run("Due date always falls on the 15th", func(t *testing.T) { - // Before the 15th - now := time.Date(2022, 12, 14, 0, 0, 0, 0, time.Local) - due := GetNextTrueUpReviewDueDate(now) - assert.Equal(t, due.Day(), trueUpReviewDueDay) - - // On the 15th - now = time.Date(2022, 12, 15, 0, 0, 0, 0, time.Local) - due = GetNextTrueUpReviewDueDate(now) - assert.Equal(t, due.Day(), trueUpReviewDueDay) - - // After the 15th - now = time.Date(2022, 12, 16, 0, 0, 0, 0, time.Local) - due = GetNextTrueUpReviewDueDate(now) - assert.Equal(t, due.Day(), trueUpReviewDueDay) - }) - - t.Run("Due date will always be in next quarter if the current date is past the 15th", func(t *testing.T) { - now := time.Date(2022, time.March, 16, 0, 0, 0, 0, time.Local) - due := GetNextTrueUpReviewDueDate(now) - assert.Equal(t, time.June, due.Month()) - - now = time.Date(2022, time.June, 16, 0, 0, 0, 0, time.Local) - due = GetNextTrueUpReviewDueDate(now) - assert.Equal(t, time.September, due.Month()) - - now = time.Date(2022, time.September, 16, 0, 0, 0, 0, time.Local) - due = GetNextTrueUpReviewDueDate(now) - assert.Equal(t, time.December, due.Month()) - - now = time.Date(2022, time.December, 16, 0, 0, 0, 0, time.Local) - due = GetNextTrueUpReviewDueDate(now) - assert.Equal(t, time.March, due.Month()) - }) - - t.Run("Due date will always be in the current quarter if the current date is before or on the 15th", func(t *testing.T) { - now := time.Date(2022, time.March, 15, 0, 0, 0, 0, time.Local) - due := GetNextTrueUpReviewDueDate(now) - assert.Equal(t, time.March, due.Month()) - - now = time.Date(2022, time.June, 15, 0, 0, 0, 0, time.Local) - due = GetNextTrueUpReviewDueDate(now) - assert.Equal(t, time.June, due.Month()) - - now = time.Date(2022, time.September, 14, 0, 0, 0, 0, time.Local) - due = GetNextTrueUpReviewDueDate(now) - assert.Equal(t, time.September, due.Month()) - - now = time.Date(2022, time.December, 14, 0, 0, 0, 0, time.Local) - due = GetNextTrueUpReviewDueDate(now) - assert.Equal(t, time.December, due.Month()) - }) - - t.Run("Due date will be in the next year if the next quarter is not within the current year", func(t *testing.T) { - now := time.Date(2022, time.December, 18, 0, 0, 0, 0, time.Local) - due := GetNextTrueUpReviewDueDate(now) - assert.Equal(t, time.March, due.Month()) - assert.Equal(t, 2023, due.Year()) - }) -} - -func TestIsTrueUpReviewDueDateWithinTheNextTwoWeeks(t *testing.T) { - t.Run("Ensure a date within two weeks before the due date returns true", func(t *testing.T) { - // 1 Day before the due date - now := time.Date(2022, time.December, 14, 0, 0, 0, 0, time.Local) - // Due date is December 15th, 2022 - due := GetNextTrueUpReviewDueDate(now) - - res := IsTrueUpReviewDueDateWithinTheNextTwoWeeks(now, due) - assert.True(t, res) - }) - - t.Run("Ensure a date that is more than two weeks before the due date returns false", func(t *testing.T) { - // 15 Days before the due date - now := time.Date(2022, time.November, 30, 0, 0, 0, 0, time.Local) - // Due date is December 15th, 2022 - due := GetNextTrueUpReviewDueDate(now) - - res := IsTrueUpReviewDueDateWithinTheNextTwoWeeks(now, due) - assert.False(t, res) - }) - - t.Run("Ensure a date that past the due date returns false", func(t *testing.T) { - now := time.Date(2022, time.December, 16, 0, 0, 0, 0, time.Local) - - // Due date is December 15th, 2022 - dueNow := time.Date(2022, time.December, 15, 0, 0, 0, 0, time.Local) - due := GetNextTrueUpReviewDueDate(dueNow) - - res := IsTrueUpReviewDueDateWithinTheNextTwoWeeks(now, due) - assert.False(t, res) - }) - - t.Run("Ensure a date that is on the due date returns true", func(t *testing.T) { - now := time.Date(2022, time.December, 15, 0, 0, 0, 0, time.Local) - due := GetNextTrueUpReviewDueDate(now) - - res := IsTrueUpReviewDueDateWithinTheNextTwoWeeks(now, due) - assert.True(t, res) - }) - - t.Run("Ensure a date that is on the first day of the due date window returns true", func(t *testing.T) { - now := time.Date(2022, time.December, 1, 0, 0, 0, 0, time.Local) - due := GetNextTrueUpReviewDueDate(now) - - res := IsTrueUpReviewDueDateWithinTheNextTwoWeeks(now, due) - assert.True(t, res) - }) -} diff --git a/utils/true_up.go b/utils/true_up.go new file mode 100644 index 0000000000..22f19a7fc8 --- /dev/null +++ b/utils/true_up.go @@ -0,0 +1,85 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package utils + +import ( + "fmt" + "time" +) + +const trueUpReviewDueDay = 15 +const day = time.Hour * 24 +const week = day * 7 + +type DueDateWindow struct { + Start time.Time + End time.Time +} + +func GetNextTrueUpReviewDueDate(now time.Time) time.Time { + nowYear := now.Year() + nowMonth := now.Month() + nowDay := now.Day() + finalQuarterYear := nowYear + if nowMonth >= time.October && nowMonth <= time.December { + finalQuarterYear = nowYear + 1 + } + trueUpSubmissionWindows := []DueDateWindow{ + { + Start: time.Date(now.Year(), time.January, 16, 0, 0, 0, 0, now.Location()), + End: time.Date(now.Year(), time.April, 15, 0, 0, 0, 0, now.Location()), + }, + { + Start: time.Date(now.Year(), time.April, 16, 0, 0, 0, 0, now.Location()), + End: time.Date(now.Year(), time.July, 15, 0, 0, 0, 0, now.Location()), + }, + { + Start: time.Date(now.Year(), time.July, 16, 0, 0, 0, 0, now.Location()), + End: time.Date(now.Year(), time.October, 15, 0, 0, 0, 0, now.Location()), + }, + { + Start: time.Date(now.Year(), time.October, 16, 0, 0, 0, 0, now.Location()), + End: time.Date(finalQuarterYear, time.January, 15, 0, 0, 0, 0, now.Location()), + }, + } + + for _, window := range trueUpSubmissionWindows { + withinWindow := false + // Our due dates "wrap" around, so we'll need to check the months different. Since January = 1 and December = 12, the checks + // for the current month being greater or equal to the start month and less than or equal to the end month will not work. + if window.End.Month() == time.January { + withinWindow = (nowMonth != time.January && nowMonth >= window.Start.Month()) || nowMonth == window.End.Month() + } else { + withinWindow = nowMonth >= window.Start.Month() && nowMonth <= window.End.Month() + fmt.Printf("now month: %s, window start month: %s, window end month: %s\n", now.Format("Jan"), window.Start.Format("Jan"), window.End.Format("Jan")) + } + + // Only check the days if the current month is equal to the start or end months. + // The dates of the middle month(s) don't matter so much. + isFirstMonth := nowMonth == window.Start.Month() + if isFirstMonth { + withinWindow = withinWindow && nowDay >= window.Start.Day() + } + isFinalMonth := nowMonth == window.End.Month() + if isFinalMonth { + withinWindow = withinWindow && nowDay <= window.End.Day() + } + + if withinWindow { + return window.End + } + } + + return trueUpSubmissionWindows[0].End +} + +func IsTrueUpReviewDueDateWithinTheNext30Days(now time.Time, dueDate time.Time) bool { + dueDateWindow := dueDate.Add(-day * 30) + + if now.Before(dueDateWindow) || now.After(dueDate) { + return false + } + + return true +} diff --git a/utils/true_up_test.go b/utils/true_up_test.go new file mode 100644 index 0000000000..76517ac728 --- /dev/null +++ b/utils/true_up_test.go @@ -0,0 +1,125 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package utils + +import ( + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestGetNextTrueUpReviewDueDate(t *testing.T) { + t.Run("Due date always falls on the 15th", func(t *testing.T) { + // Before the 15th + now := time.Date(2022, 12, 14, 0, 0, 0, 0, time.Local) + due := GetNextTrueUpReviewDueDate(now) + assert.Equal(t, due.Day(), trueUpReviewDueDay) + + // On the 15th + now = time.Date(2022, 12, 15, 0, 0, 0, 0, time.Local) + due = GetNextTrueUpReviewDueDate(now) + assert.Equal(t, due.Day(), trueUpReviewDueDay) + + // After the 15th + now = time.Date(2022, 12, 16, 0, 0, 0, 0, time.Local) + due = GetNextTrueUpReviewDueDate(now) + assert.Equal(t, due.Day(), trueUpReviewDueDay) + }) + + t.Run("Due date will always be in next quarter if the current date is past the 15th", func(t *testing.T) { + now := time.Date(2022, time.March, 16, 0, 0, 0, 0, time.Local) + due := GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.April, due.Month()) + + now = time.Date(2022, time.June, 16, 0, 0, 0, 0, time.Local) + due = GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.July, due.Month()) + + now = time.Date(2022, time.September, 16, 0, 0, 0, 0, time.Local) + due = GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.October, due.Month()) + + now = time.Date(2022, time.December, 16, 0, 0, 0, 0, time.Local) + due = GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.January, due.Month()) + }) + + t.Run("Due date will always be in the current quarter if the current date is before or on the 15th", func(t *testing.T) { + now := time.Date(2022, time.April, 15, 0, 0, 0, 0, time.Local) + due := GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.April, due.Month()) + + now = time.Date(2022, time.July, 15, 0, 0, 0, 0, time.Local) + due = GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.July, due.Month()) + + now = time.Date(2022, time.October, 14, 0, 0, 0, 0, time.Local) + due = GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.October, due.Month()) + + now = time.Date(2022, time.January, 14, 0, 0, 0, 0, time.Local) + due = GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.January, due.Month()) + }) + + t.Run("Due date will be in the next year if the next quarter is not within the current year", func(t *testing.T) { + now := time.Date(2022, time.October, 21, 0, 0, 0, 0, time.Local) + due := GetNextTrueUpReviewDueDate(now) + assert.Equal(t, time.January, due.Month()) + assert.Equal(t, 2023, due.Year()) + }) +} + +func TestIsTrueUpReviewDueDateWithinTheNext15Days(t *testing.T) { + t.Run("Ensure a date within 30 days before the due date returns true", func(t *testing.T) { + // 1 Day before the due date + now := time.Date(2022, time.March, 16, 0, 0, 0, 0, time.Local) + // Due date is December 15th, 2022 + due := GetNextTrueUpReviewDueDate(now) + + res := IsTrueUpReviewDueDateWithinTheNext30Days(now, due) + assert.True(t, res) + }) + + t.Run("Ensure a date that is more than two weeks before the due date returns false", func(t *testing.T) { + // 15 Days before the due date + now := time.Date(2022, time.October, 16, 0, 0, 0, 0, time.Local) + // Due date is December 15th, 2022 + due := GetNextTrueUpReviewDueDate(now) + + res := IsTrueUpReviewDueDateWithinTheNext30Days(now, due) + assert.False(t, res) + }) + + t.Run("Ensure a date that is past the due date returns false", func(t *testing.T) { + now := time.Date(2022, time.April, 15, 0, 0, 0, 0, time.Local) + + // Due date is April 16th, 2022 + dueNow := time.Date(2022, time.April, 16, 0, 0, 0, 0, time.Local) + due := GetNextTrueUpReviewDueDate(dueNow) + + res := IsTrueUpReviewDueDateWithinTheNext30Days(now, due) + assert.False(t, res) + }) + + t.Run("Ensure a date that is on the due date returns true", func(t *testing.T) { + now := time.Date(2022, time.January, 15, 0, 0, 0, 0, time.Local) + due := GetNextTrueUpReviewDueDate(now) + fmt.Printf("\n\ndue date: %s\n\n", due.Format("2006-Jan-02")) + fmt.Printf("\n\nnow: %s\n\n", now.Format("2006-Jan-02")) + + res := IsTrueUpReviewDueDateWithinTheNext30Days(now, due) + assert.True(t, res) + }) + + t.Run("Ensure a date that is on the first day of the due date window returns true", func(t *testing.T) { + now := time.Date(2022, time.December, 16, 0, 0, 0, 0, time.Local) + due := GetNextTrueUpReviewDueDate(now) + + res := IsTrueUpReviewDueDateWithinTheNext30Days(now, due) + assert.True(t, res) + }) +} From 72f012d6a0295d8a7c2aa7f0de9bfe9226cec1ae Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Thu, 5 Jan 2023 17:06:54 -0500 Subject: [PATCH 54/58] remove unused variable. --- utils/true_up.go | 1 - 1 file changed, 1 deletion(-) diff --git a/utils/true_up.go b/utils/true_up.go index 22f19a7fc8..fc753bcd74 100644 --- a/utils/true_up.go +++ b/utils/true_up.go @@ -10,7 +10,6 @@ import ( const trueUpReviewDueDay = 15 const day = time.Hour * 24 -const week = day * 7 type DueDateWindow struct { Start time.Time From 4f9f331184036edf7cc6c342a22ad3c052879031 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Fri, 6 Jan 2023 09:31:19 -0500 Subject: [PATCH 55/58] Update comments, use correct ordering of args for assertions. --- utils/true_up.go | 2 +- utils/true_up_test.go | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/utils/true_up.go b/utils/true_up.go index fc753bcd74..fa88dd1b9b 100644 --- a/utils/true_up.go +++ b/utils/true_up.go @@ -45,7 +45,7 @@ func GetNextTrueUpReviewDueDate(now time.Time) time.Time { for _, window := range trueUpSubmissionWindows { withinWindow := false - // Our due dates "wrap" around, so we'll need to check the months different. Since January = 1 and December = 12, the checks + // Our due dates "wrap" around (i.e. can go into the next year), so we'll need to check the months different. Since January = 1 and December = 12, the checks // for the current month being greater or equal to the start month and less than or equal to the end month will not work. if window.End.Month() == time.January { withinWindow = (nowMonth != time.January && nowMonth >= window.Start.Month()) || nowMonth == window.End.Month() diff --git a/utils/true_up_test.go b/utils/true_up_test.go index 76517ac728..3ba0085649 100644 --- a/utils/true_up_test.go +++ b/utils/true_up_test.go @@ -14,19 +14,19 @@ import ( func TestGetNextTrueUpReviewDueDate(t *testing.T) { t.Run("Due date always falls on the 15th", func(t *testing.T) { // Before the 15th - now := time.Date(2022, 12, 14, 0, 0, 0, 0, time.Local) + now := time.Date(2022, time.March, 14, 0, 0, 0, 0, time.Local) due := GetNextTrueUpReviewDueDate(now) - assert.Equal(t, due.Day(), trueUpReviewDueDay) + assert.Equal(t, trueUpReviewDueDay, due.Day()) // On the 15th - now = time.Date(2022, 12, 15, 0, 0, 0, 0, time.Local) + now = time.Date(2022, time.December, 15, 0, 0, 0, 0, time.Local) due = GetNextTrueUpReviewDueDate(now) - assert.Equal(t, due.Day(), trueUpReviewDueDay) + assert.Equal(t, trueUpReviewDueDay, due.Day()) // After the 15th - now = time.Date(2022, 12, 16, 0, 0, 0, 0, time.Local) + now = time.Date(2022, time.September, 16, 0, 0, 0, 0, time.Local) due = GetNextTrueUpReviewDueDate(now) - assert.Equal(t, due.Day(), trueUpReviewDueDay) + assert.Equal(t, trueUpReviewDueDay, due.Day()) }) t.Run("Due date will always be in next quarter if the current date is past the 15th", func(t *testing.T) { From 337c80f2bf26b1cc925e95a0024a0696ea5201ed Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Fri, 6 Jan 2023 11:08:51 -0500 Subject: [PATCH 56/58] Create true up review earlier, move completed check to be earlier. --- api4/license.go | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/api4/license.go b/api4/license.go index 880447ed58..69537ba9e5 100644 --- a/api4/license.go +++ b/api4/license.go @@ -318,6 +318,18 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { return } + status, appErr := c.App.GetOrCreateTrueUpReviewStatus() + if appErr != nil { + c.Err = appErr + return + } + + // If a true up review has already been submitted for the current due date, complete the request + // with no errors. + if status.Completed { + ReturnStatusOK(w) + } + profileMap, err := c.App.GetTrueUpProfile() if err != nil { c.Err = model.NewAppError("requestTrueUpReview", "api.license.true_up_review.get_status_error", nil, "", http.StatusInternalServerError) @@ -330,16 +342,10 @@ func requestTrueUpReview(c *Context, w http.ResponseWriter, r *http.Request) { return } - status, appErr := c.App.GetOrCreateTrueUpReviewStatus() - if err != nil { - c.Err = appErr - return - } - // Do not send true-up review data if the user has already requested one for the quarter. // And only send a true-up review via as a one-time telemetry request if telemetry is disabled. telemetryEnabled := c.App.Config().LogSettings.EnableDiagnostics - if !status.Completed && telemetryEnabled != nil && !*telemetryEnabled { + if telemetryEnabled != nil && !*telemetryEnabled { // Send telemetry data c.App.Srv().GetTelemetryService().SendTelemetry(model.TrueUpReviewTelemetryName, profileMap) From b237c21e2834800619993062e31dcb172b9df02f Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Fri, 6 Jan 2023 12:54:21 -0500 Subject: [PATCH 57/58] remove printf to re-run checks. --- utils/true_up.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/utils/true_up.go b/utils/true_up.go index fa88dd1b9b..1be96d86f8 100644 --- a/utils/true_up.go +++ b/utils/true_up.go @@ -4,7 +4,6 @@ package utils import ( - "fmt" "time" ) @@ -51,7 +50,6 @@ func GetNextTrueUpReviewDueDate(now time.Time) time.Time { withinWindow = (nowMonth != time.January && nowMonth >= window.Start.Month()) || nowMonth == window.End.Month() } else { withinWindow = nowMonth >= window.Start.Month() && nowMonth <= window.End.Month() - fmt.Printf("now month: %s, window start month: %s, window end month: %s\n", now.Format("Jan"), window.Start.Format("Jan"), window.End.Format("Jan")) } // Only check the days if the current month is equal to the start or end months. From e5444269f5d935d8bf88bfeaf627abd1c5430dd2 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Fri, 6 Jan 2023 15:41:06 -0500 Subject: [PATCH 58/58] Remove plugin prefix. --- app/true_up.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/true_up.go b/app/true_up.go index 3e3c779f31..67e1ed01f8 100644 --- a/app/true_up.go +++ b/app/true_up.go @@ -136,8 +136,8 @@ func (a *App) GetTrueUpProfile() (map[string]any, error) { json.Unmarshal(profileJson, &telemetryProperties) delete(telemetryProperties, "plugins") plugins := profile.Plugins.ToMap() - for pluginName, pluginValue := range plugins { - telemetryProperties["plugin_"+pluginName] = pluginValue + for key, pluginValue := range plugins { + telemetryProperties[key] = pluginValue } delete(telemetryProperties, "authentication_features")