From 9c71784d0d8892faf34bc239e76ebbda2991b552 Mon Sep 17 00:00:00 2001 From: Conor Macpherson Date: Tue, 27 Dec 2022 16:54:46 -0500 Subject: [PATCH] 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) {