Sort by displayName by default instead of ID (#2087)

api/graphql/schema.graphql

@@ -1051,6 +1051,7 @@ enum ListPoliciesSortFieldsEnum {
 }
 
 enum ListRulesSortFieldsEnum {
+  displayName
   enabled
   id
   lastModified

api/lambda/analysis/models/rules.go

@@ -55,7 +55,7 @@ type ListRulesInput struct {
 	Fields []string `json:"fields" validate:"max=20,dive,required,max=100"`
 
 	// ----- Sorting -----
-	SortBy  string `json:"sortBy" validate:"omitempty,oneof=enabled id lastModified logTypes severity"`
+	SortBy  string `json:"sortBy" validate:"omitempty,oneof=displayName enabled id lastModified logTypes severity"`
 	SortDir string `json:"sortDir" validate:"omitempty,oneof=ascending descending"`
 
 	// ----- Paging -----

go.mod

@@ -29,7 +29,9 @@ require (
 	github.com/valyala/fasttemplate v1.2.1
 	go.uber.org/multierr v1.5.0
 	go.uber.org/zap v1.16.0
+	golang.org/x/net v0.0.0-20201110031124-69a78807bb2b // indirect
 	golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208
+	golang.org/x/text v0.3.4 // indirect
 	golang.org/x/tools v0.0.0-20200914175622-c9b80dc7fda4
 	gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f // indirect
 	gopkg.in/go-playground/assert.v1 v1.2.1 // indirect

go.sum

@@ -114,15 +114,21 @@ golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLL
 golang.org/x/net v0.0.0-20200202094626-16171245cfb2/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=
 golang.org/x/net v0.0.0-20200822124328-c89045814202 h1:VvcQYSHwXgi7W+TpUR6A9g6Up98WAHf3f/ulnJ62IyA=
 golang.org/x/net v0.0.0-20200822124328-c89045814202/go.mod h1:/O7V0waA8r7cgGh81Ro3o1hOxt32SMVPicZroKQ2sZA=
+golang.org/x/net v0.0.0-20201110031124-69a78807bb2b h1:uwuIcX0g4Yl1NC5XAz37xsr2lTtcqevgzYNVt49waME=
+golang.org/x/net v0.0.0-20201110031124-69a78807bb2b/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU=
 golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208 h1:qwRHBd0NqMbJxfbotnDhm2ByMI1Shq4Y6oRJo21SGJA=
 golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 golang.org/x/text v0.3.2 h1:tW2bmiBqwgJj/UpqtC8EpXEZVYOwU0yG4iWbprSVAcs=
 golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk=
+golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
+golang.org/x/text v0.3.4 h1:0YWbFKbhXG/wIiuHDSKpS0Iy7FSA+u45VtBMfQcFTTc=
+golang.org/x/text v0.3.4/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
 golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20190311212946-11955173bddd/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs=
 golang.org/x/tools v0.0.0-20190621195816-6e04913cbbac/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc=

internal/core/analysis_api/handlers/list.go

@@ -29,6 +29,7 @@ import (
 )
 
 const (
+	defaultSortBy   = "displayName"
 	defaultSortDir  = "ascending"
 	defaultPage     = 1
 	defaultPageSize = 25
@@ -122,8 +123,12 @@ func sortItems(items []tableItem, sortBy, sortDir string, compliance map[string]
 	ascending := sortDir != "descending"
 
 	switch sortBy {
+	case "displayName":
+		sortByDisplayName(items, ascending)
 	case "complianceStatus":
 		sortByStatus(items, ascending, compliance)
+	case "id":
+		sortByID(items, ascending)
 	case "enabled":
 		sortByEnabled(items, ascending)
 	case "lastModified":
@@ -133,10 +138,44 @@ func sortItems(items []tableItem, sortBy, sortDir string, compliance map[string]
 	case "severity":
 		sortBySeverity(items, ascending)
 	default:
-		sortByID(items, ascending)
+		// Input validation for the caller already happens in the struct validate tags.
+		// If we reach this code, it means there is a sortBy allowed in the input validation,
+		// but not supported in the backend, which should never happen
+		panic("Unexpected sortBy: " + sortBy)
 	}
 }
 
+func sortByDisplayName(items []tableItem, ascending bool) {
+	sort.Slice(items, func(i, j int) bool {
+		left, right := items[i], items[j]
+
+		var leftName, rightName string
+		leftName, rightName = left.DisplayName, right.DisplayName
+
+		// The frontend shows display name *or* ID (when there is no display name)
+		// So we sort the same way it is shown to the user - displayName if available, otherwise ID
+		if leftName == "" {
+			leftName = left.ID
+		}
+		if rightName == "" {
+			rightName = right.ID
+		}
+
+		if leftName != rightName {
+			if ascending {
+				return strings.ToLower(leftName) < strings.ToLower(rightName)
+			}
+			return strings.ToLower(leftName) > strings.ToLower(rightName)
+		}
+
+		// Same display name: sort by ID
+		if ascending {
+			return strings.ToLower(left.ID) < strings.ToLower(right.ID)
+		}
+		return strings.ToLower(left.ID) > strings.ToLower(right.ID)
+	})
+}
+
 func sortByStatus(items []tableItem, ascending bool, compliance map[string]complianceStatus) {
 	sort.Slice(items, func(i, j int) bool {
 		left, right := items[i], items[j]

internal/core/analysis_api/handlers/list_rules.go

@@ -74,6 +74,9 @@ func stdRuleListInput(input *models.ListRulesInput) {
 	if input.PageSize == 0 {
 		input.PageSize = defaultPageSize
 	}
+	if input.SortBy == "" {
+		input.SortBy = defaultSortBy
+	}
 	if input.SortDir == "" {
 		input.SortDir = defaultSortDir
 	}

internal/core/analysis_api/handlers/list_test.go

@@ -63,3 +63,46 @@ func TestPagePoliciesPageOutOfBounds(t *testing.T) {
 	assert.Equal(t, models.Paging{ThisPage: 10, TotalItems: 4, TotalPages: 4}, paging)
 	assert.Equal(t, truncation, []tableItem{}) // empty list - page out of bounds
 }
+
+func TestPagePoliciesDisplayNameSort(t *testing.T) {
+	items := []tableItem{
+		{ID: "a", DisplayName: "z"},
+		{ID: "h", DisplayName: "b"},
+		{ID: "c", DisplayName: "y"},
+		{ID: "e", DisplayName: "a"},
+		{ID: "g", DisplayName: "b"},
+		{ID: "d", DisplayName: ""},
+	}
+
+	sortByDisplayName(items, true)
+
+	expected := []tableItem{
+		{ID: "e", DisplayName: "a"},
+		{ID: "g", DisplayName: "b"},
+		{ID: "h", DisplayName: "b"},
+		{ID: "d", DisplayName: ""},
+		{ID: "c", DisplayName: "y"},
+		{ID: "a", DisplayName: "z"},
+	}
+	assert.Equal(t, expected, items)
+}
+
+func TestPagePoliciesDisplayNameSortReverse(t *testing.T) {
+	items := []tableItem{
+		{ID: "e", DisplayName: "a"},
+		{ID: "a", DisplayName: "z"},
+		{ID: "c", DisplayName: "y"},
+		{ID: "g", DisplayName: "b"},
+		{ID: "d", DisplayName: "y"},
+	}
+	sortByDisplayName(items, false)
+
+	expected := []tableItem{
+		{ID: "a", DisplayName: "z"},
+		{ID: "d", DisplayName: "y"},
+		{ID: "c", DisplayName: "y"},
+		{ID: "g", DisplayName: "b"},
+		{ID: "e", DisplayName: "a"},
+	}
+	assert.Equal(t, expected, items)
+}

web/__tests__/__mocks__/builders.generated.ts

@@ -903,7 +903,7 @@ export const buildListRulesInput = (overrides: Partial<ListRulesInput> = {}): Li
     logTypes: 'logTypes' in overrides ? overrides.logTypes : ['Drive'],
     severity: 'severity' in overrides ? overrides.severity : SeverityEnum.Low,
     tags: 'tags' in overrides ? overrides.tags : ['channels'],
-    sortBy: 'sortBy' in overrides ? overrides.sortBy : ListRulesSortFieldsEnum.Enabled,
+    sortBy: 'sortBy' in overrides ? overrides.sortBy : ListRulesSortFieldsEnum.DisplayName,
     sortDir: 'sortDir' in overrides ? overrides.sortDir : SortDirEnum.Ascending,
     pageSize: 'pageSize' in overrides ? overrides.pageSize : 19,
     page: 'page' in overrides ? overrides.page : 323,

web/src/pages/ListRules/ListRulesFilters/ListRulesFilters.tsx

@@ -45,31 +45,31 @@ const defaultValues = {
 
 const sortingOpts: SortingOptions = [
   {
-    opt: 'Most Recently Modified',
+    opt: 'Name Ascending',
     resolution: {
-      sortBy: 'lastModified' as ListRulesSortFieldsEnum,
-      sortDir: 'descending' as SortDirEnum,
+      sortBy: 'displayName' as ListRulesSortFieldsEnum,
+      sortDir: 'ascending' as SortDirEnum,
     },
   },
   {
-    opt: 'Oldest Modified',
+    opt: 'Name Descending',
     resolution: {
-      sortBy: 'lastModified' as ListRulesSortFieldsEnum,
-      sortDir: 'ascending' as SortDirEnum,
+      sortBy: 'displayName' as ListRulesSortFieldsEnum,
+      sortDir: 'descending' as SortDirEnum,
     },
   },
   {
-    opt: 'ID Ascending',
+    opt: 'Most Recently Modified',
     resolution: {
-      sortBy: 'id' as ListRulesSortFieldsEnum,
-      sortDir: 'ascending' as SortDirEnum,
+      sortBy: 'lastModified' as ListRulesSortFieldsEnum,
+      sortDir: 'descending' as SortDirEnum,
     },
   },
   {
-    opt: 'ID Descending',
+    opt: 'Oldest Modified',
     resolution: {
-      sortBy: 'id' as ListRulesSortFieldsEnum,
-      sortDir: 'descending' as SortDirEnum,
+      sortBy: 'lastModified' as ListRulesSortFieldsEnum,
+      sortDir: 'ascending' as SortDirEnum,
     },
   },
   {