Code review comments.

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

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

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

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