[MM-62695] Extend property types for CPA (#30201)

* test: Add unit tests for custom profile attributes select options

* feat: Add custom profile attributes model with validation and constants

* refactor: Trim spaces from name and color in custom profile attribute select option constructor

* gofmt

* refactor: Fix typo in custom profile attributes select option function name

* feat: Add IsValid method to validate CustomProfileAttributesSelectOptions

* refactor: Replace map[string]bool with map[string]struct{} for key existence check

* refactor: Rename NewCustomProfileAttributeSelectOption to NewCustomProfileAttributesSelectOption

* feat: Add validation to prevent empty custom profile attribute options

* refactor: Add validation and creation methods for custom profile attributes

* feat: Add index number to validation error messages in custom profile attributes

* fix tests

* add default visibility

* feat: Add comprehensive test cases for custom profile attributes field validation

* fix: Update custom profile attributes map keys to use capitalized names

* feat: Add support for lowercase and title case keys in custom profile attributes map

* test: Add comprehensive test for NewCustomProfileAttributesSelectOptionFromMap

* feat: Add validation for custom profile attributes fields

* refactor: Update CustomProfileAttributesSelectOption constructor to prioritize ID parameter

* test: Add test cases for preserving IDs in custom profile attributes

* feat: Enhance ID validation and trimming in custom profile attributes

* don't do validation in constructor

* test: Add test case for preserving option IDs when patching select field

* improve test

* i18n

* refactor: Modify CustomProfileAttributesSelectOption to use lowercase JSON keys

* fix casing in custom profilte attributes test

* refactor: Use consistent "ValidateCPAField" in error messages for custom profile attributes

* use custom types rather than string

* lint

* fix api test

* refactor: Make color field optional in custom profile attributes

* style

* generic options

* removed unused i18n

* test: Add tests for NewCPAFieldFromPropertyField and CPAFieldToPropertyField

* test: Add test case for property field with empty attributes

* refactor: Cleanup whitespace and remove empty Attrs in custom profile attributes test

* test: Add test case for CPA field with empty attributes

* refactor: Improve custom profile attributes field handling and validation

* refactor: Move validateCustomProfileAttributesField to Validate method on CPAField struct

* use CPAField

* code style

* add validation and tests

* tests

* i18n

* err->appErr

* fix TestDeleteCPAField test

* i18n

* Add SAML and LDAP attr

* rename CustomProfileAttributes in method to CPA

* rename CPASortOrder method

* rearrange consts

* use Len test method

* sanitize and validate

* manage error the same way property field and value do

* fix: Update test error ID for custom profile attributes validation

* test: Update error ID expectations in custom profile attributes tests

* refactor: Convert CPAAttrs.SortOrder from string to int

* json uses float64

* feat: Add length validation for custom profile attribute option name and color

---------

Co-authored-by: Mattermost Build <build@mattermost.com>

server/channels/app/custom_profile_attributes.go

@@ -72,13 +72,13 @@ func (a *App) ListCPAFields() ([]*model.PropertyField, *model.AppError) {
 	}
 
 	sort.Slice(fields, func(i, j int) bool {
-		return model.CustomProfileAttributesPropertySortOrder(fields[i]) < model.CustomProfileAttributesPropertySortOrder(fields[j])
+		return model.CPASortOrder(fields[i]) < model.CPASortOrder(fields[j])
 	})
 
 	return fields, nil
 }
 
-func (a *App) CreateCPAField(field *model.PropertyField) (*model.PropertyField, *model.AppError) {
+func (a *App) CreateCPAField(field *model.CPAField) (*model.PropertyField, *model.AppError) {
 	groupID, err := a.cpaGroupID()
 	if err != nil {
 		return nil, model.NewAppError("CreateCPAField", "app.custom_profile_attributes.cpa_group_id.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
@@ -94,7 +94,12 @@ func (a *App) CreateCPAField(field *model.PropertyField) (*model.PropertyField,
 	}
 
 	field.GroupID = groupID
-	newField, err := a.Srv().propertyService.CreatePropertyField(field)
+
+	if appErr := field.SanitizeAndValidate(); appErr != nil {
+		return nil, appErr
+	}
+
+	newField, err := a.Srv().propertyService.CreatePropertyField(field.ToPropertyField())
 	if err != nil {
 		var appErr *model.AppError
 		switch {
@@ -123,7 +128,16 @@ func (a *App) PatchCPAField(fieldID string, patch *model.PropertyFieldPatch) (*m
 	patch.TargetType = nil
 	existingField.Patch(patch)
 
-	patchedField, err := a.Srv().propertyService.UpdatePropertyField(existingField)
+	cpaField, err := model.NewCPAFieldFromPropertyField(existingField)
+	if err != nil {
+		return nil, model.NewAppError("UpdateCPAField", "app.custom_profile_attributes.property_field_conversion.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
+	}
+
+	if appErr := cpaField.SanitizeAndValidate(); appErr != nil {
+		return nil, appErr
+	}
+
+	patchedField, err := a.Srv().propertyService.UpdatePropertyField(cpaField.ToPropertyField())
 	if err != nil {
 		var nfErr *store.ErrNotFound
 		switch {

server/channels/app/custom_profile_attributes_test.go

@@ -47,22 +47,23 @@ func TestGetCPAField(t *testing.T) {
 	})
 
 	t.Run("should get an existing CPA field", func(t *testing.T) {
-		field := &model.PropertyField{
+		field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
 			GroupID: cpaGroupID,
 			Name:    "Test Field",
 			Type:    model.PropertyFieldTypeText,
-			Attrs:   model.StringInterface{"visibility": "hidden"},
-		}
+			Attrs:   model.StringInterface{model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityHidden},
+		})
+		require.NoError(t, err)
 
-		createdField, err := th.App.CreateCPAField(field)
-		require.Nil(t, err)
+		createdField, appErr := th.App.CreateCPAField(field)
+		require.Nil(t, appErr)
 		require.NotEmpty(t, createdField.ID)
 
-		fetchedField, err := th.App.GetCPAField(createdField.ID)
-		require.Nil(t, err)
+		fetchedField, appErr := th.App.GetCPAField(createdField.ID)
+		require.Nil(t, appErr)
 		require.Equal(t, createdField.ID, fetchedField.ID)
 		require.Equal(t, "Test Field", fetchedField.Name)
-		require.Equal(t, model.StringInterface{"visibility": "hidden"}, fetchedField.Attrs)
+		require.Equal(t, model.CustomProfileAttributesVisibilityHidden, fetchedField.Attrs["visibility"])
 	})
 }
 
@@ -120,19 +121,21 @@ func TestCreateCPAField(t *testing.T) {
 	require.NoError(t, cErr)
 
 	t.Run("should fail if the field is not valid", func(t *testing.T) {
-		field := &model.PropertyField{Name: model.NewId()}
+		field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{Name: model.NewId()})
+		require.NoError(t, err)
 
 		createdField, err := th.App.CreateCPAField(field)
-		require.NotNil(t, err)
+		require.Error(t, err)
 		require.Empty(t, createdField)
 	})
 
 	t.Run("should not be able to create a property field for a different feature", func(t *testing.T) {
-		field := &model.PropertyField{
+		field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
 			GroupID: model.NewId(),
 			Name:    model.NewId(),
 			Type:    model.PropertyFieldTypeText,
-		}
+		})
+		require.NoError(t, err)
 
 		createdField, appErr := th.App.CreateCPAField(field)
 		require.Nil(t, appErr)
@@ -140,18 +143,19 @@ func TestCreateCPAField(t *testing.T) {
 	})
 
 	t.Run("should correctly create a CPA field", func(t *testing.T) {
-		field := &model.PropertyField{
+		field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
 			GroupID: cpaGroupID,
 			Name:    model.NewId(),
 			Type:    model.PropertyFieldTypeText,
-			Attrs:   model.StringInterface{"visibility": "hidden"},
-		}
+			Attrs:   model.StringInterface{model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityHidden},
+		})
+		require.NoError(t, err)
 
-		createdField, err := th.App.CreateCPAField(field)
-		require.Nil(t, err)
+		createdField, appErr := th.App.CreateCPAField(field)
+		require.Nil(t, appErr)
 		require.NotZero(t, createdField.ID)
 		require.Equal(t, cpaGroupID, createdField.GroupID)
-		require.Equal(t, model.StringInterface{"visibility": "hidden"}, createdField.Attrs)
+		require.Equal(t, model.CustomProfileAttributesVisibilityHidden, createdField.Attrs["visibility"])
 
 		fetchedField, gErr := th.App.Srv().propertyService.GetPropertyField("", createdField.ID)
 		require.NoError(t, gErr)
@@ -170,42 +174,48 @@ func TestCreateCPAField(t *testing.T) {
 		t.Run("should not be able to create CPA fields above the limit", func(t *testing.T) {
 			// we create the rest of the fields required to reach the limit
 			for i := 1; i <= CustomProfileAttributesFieldLimit; i++ {
-				field := &model.PropertyField{
+				field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
 					Name: model.NewId(),
 					Type: model.PropertyFieldTypeText,
-				}
-				createdField, err := th.App.CreateCPAField(field)
-				require.Nil(t, err)
+				})
+				require.NoError(t, err)
+
+				createdField, appErr := th.App.CreateCPAField(field)
+				require.Nil(t, appErr)
 				require.NotZero(t, createdField.ID)
 			}
 
 			// then, we create a last one that would exceed the limit
-			field := &model.PropertyField{
-				Name: model.NewId(),
-				Type: model.PropertyFieldTypeText,
+			field := &model.CPAField{
+				PropertyField: model.PropertyField{
+					Name: model.NewId(),
+					Type: model.PropertyFieldTypeText,
+				},
 			}
-			createdField, err := th.App.CreateCPAField(field)
-			require.NotNil(t, err)
-			require.Equal(t, http.StatusUnprocessableEntity, err.StatusCode)
+			createdField, appErr := th.App.CreateCPAField(field)
+			require.NotNil(t, appErr)
+			require.Equal(t, http.StatusUnprocessableEntity, appErr.StatusCode)
 			require.Zero(t, createdField)
 		})
 
 		t.Run("deleted fields should not count for the limit", func(t *testing.T) {
 			// we retrieve the list of fields and check we've reached the limit
-			fields, err := th.App.ListCPAFields()
-			require.Nil(t, err)
+			fields, appErr := th.App.ListCPAFields()
+			require.Nil(t, appErr)
 			require.Len(t, fields, CustomProfileAttributesFieldLimit)
 
 			// then we delete one field
 			require.Nil(t, th.App.DeleteCPAField(fields[0].ID))
 
 			// creating a new one should work now
-			field := &model.PropertyField{
-				Name: model.NewId(),
-				Type: model.PropertyFieldTypeText,
+			field := &model.CPAField{
+				PropertyField: model.PropertyField{
+					Name: model.NewId(),
+					Type: model.PropertyFieldTypeText,
+				},
 			}
-			createdField, err := th.App.CreateCPAField(field)
-			require.Nil(t, err)
+			createdField, appErr := th.App.CreateCPAField(field)
+			require.Nil(t, appErr)
 			require.NotZero(t, createdField.ID)
 		})
 	})
@@ -220,25 +230,27 @@ func TestPatchCPAField(t *testing.T) {
 	cpaGroupID, cErr := th.App.cpaGroupID()
 	require.NoError(t, cErr)
 
-	newField := &model.PropertyField{
+	newField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
 		GroupID: cpaGroupID,
 		Name:    model.NewId(),
 		Type:    model.PropertyFieldTypeText,
-		Attrs:   model.StringInterface{"visibility": "hidden"},
-	}
-	createdField, err := th.App.CreateCPAField(newField)
-	require.Nil(t, err)
+		Attrs:   model.StringInterface{model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityHidden},
+	})
+	require.NoError(t, err)
+
+	createdField, appErr := th.App.CreateCPAField(newField)
+	require.Nil(t, appErr)
 
 	patch := &model.PropertyFieldPatch{
 		Name:       model.NewPointer("Patched name"),
-		Attrs:      model.NewPointer(model.StringInterface{"visibility": "default"}),
+		Attrs:      model.NewPointer(model.StringInterface{model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityWhenSet}),
 		TargetID:   model.NewPointer(model.NewId()),
 		TargetType: model.NewPointer(model.NewId()),
 	}
 
 	t.Run("should fail if the field doesn't exist", func(t *testing.T) {
-		updatedField, err := th.App.PatchCPAField(model.NewId(), patch)
-		require.NotNil(t, err)
+		updatedField, appErr := th.App.PatchCPAField(model.NewId(), patch)
+		require.NotNil(t, appErr)
 		require.Empty(t, updatedField)
 	})
 
@@ -248,6 +260,7 @@ func TestPatchCPAField(t *testing.T) {
 			Name:    model.NewId(),
 			Type:    model.PropertyFieldTypeText,
 		}
+
 		field, err := th.App.Srv().propertyService.CreatePropertyField(newField)
 		require.NoError(t, err)
 
@@ -260,15 +273,88 @@ func TestPatchCPAField(t *testing.T) {
 	t.Run("should correctly patch the CPA property field", func(t *testing.T) {
 		time.Sleep(10 * time.Millisecond) // ensure the UpdateAt is different than CreateAt
 
-		updatedField, err := th.App.PatchCPAField(createdField.ID, patch)
-		require.Nil(t, err)
+		updatedField, appErr := th.App.PatchCPAField(createdField.ID, patch)
+		require.Nil(t, appErr)
 		require.Equal(t, createdField.ID, updatedField.ID)
 		require.Equal(t, "Patched name", updatedField.Name)
-		require.Equal(t, "default", updatedField.Attrs["visibility"])
+		require.Equal(t, model.CustomProfileAttributesVisibilityWhenSet, updatedField.Attrs[model.CustomProfileAttributesPropertyAttrsVisibility])
 		require.Empty(t, updatedField.TargetID, "CPA should not allow to patch the field's target ID")
 		require.Empty(t, updatedField.TargetType, "CPA should not allow to patch the field's target type")
 		require.Greater(t, updatedField.UpdateAt, createdField.UpdateAt)
 	})
+
+	t.Run("should preserve option IDs when patching select field options", func(t *testing.T) {
+		// Create a select field with options
+		selectField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
+			GroupID: cpaGroupID,
+			Name:    "Select Field",
+			Type:    model.PropertyFieldTypeSelect,
+			Attrs: map[string]any{
+				model.PropertyFieldAttributeOptions: []any{
+					map[string]any{
+						"name":  "Option 1",
+						"color": "#111111",
+					},
+					map[string]any{
+						"name":  "Option 2",
+						"color": "#222222",
+					},
+				},
+			},
+		})
+		require.NoError(t, err)
+
+		createdSelectField, appErr := th.App.CreateCPAField(selectField)
+		require.Nil(t, appErr)
+
+		// Get the original option IDs
+		options := createdSelectField.Attrs[model.PropertyFieldAttributeOptions].(model.PropertyOptions[*model.CustomProfileAttributesSelectOption])
+		require.Len(t, options, 2)
+		originalID1 := options[0].ID
+		originalID2 := options[1].ID
+		require.NotEmpty(t, originalID1)
+		require.NotEmpty(t, originalID2)
+
+		// Patch the field with updated option names and colors
+		selectPatch := &model.PropertyFieldPatch{
+			Attrs: model.NewPointer(model.StringInterface{
+				model.PropertyFieldAttributeOptions: []any{
+					map[string]any{
+						"id":    originalID1,
+						"name":  "Updated Option 1",
+						"color": "#333333",
+					},
+					map[string]any{
+						"name":  "New Option 1.5",
+						"color": "#353535",
+					},
+					map[string]any{
+						"id":    originalID2,
+						"name":  "Updated Option 2",
+						"color": "#444444",
+					},
+				},
+			}),
+		}
+
+		updatedSelectField, appErr := th.App.PatchCPAField(createdSelectField.ID, selectPatch)
+		require.Nil(t, appErr)
+
+		updatedOptions := updatedSelectField.Attrs[model.PropertyFieldAttributeOptions].(model.PropertyOptions[*model.CustomProfileAttributesSelectOption])
+		require.Len(t, updatedOptions, 3)
+
+		// Verify the options were updated while preserving IDs
+		require.Equal(t, originalID1, updatedOptions[0].ID)
+		require.Equal(t, "Updated Option 1", updatedOptions[0].Name)
+		require.Equal(t, "#333333", updatedOptions[0].Color)
+		require.Equal(t, originalID2, updatedOptions[2].ID)
+		require.Equal(t, "Updated Option 2", updatedOptions[2].Name)
+		require.Equal(t, "#444444", updatedOptions[2].Color)
+
+		// Check the new option
+		require.Equal(t, "New Option 1.5", updatedOptions[1].Name)
+		require.Equal(t, "#353535", updatedOptions[1].Color)
+	})
 }
 
 func TestDeleteCPAField(t *testing.T) {
@@ -280,15 +366,17 @@ func TestDeleteCPAField(t *testing.T) {
 	cpaGroupID, cErr := th.App.cpaGroupID()
 	require.NoError(t, cErr)
 
-	newField := &model.PropertyField{
+	newField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
 		GroupID: cpaGroupID,
 		Name:    model.NewId(),
 		Type:    model.PropertyFieldTypeText,
-	}
-	createdField, err := th.App.CreateCPAField(newField)
-	require.Nil(t, err)
+	})
+	require.NoError(t, err)
 
-	for i := 0; i < 3; i++ {
+	createdField, appErr := th.App.CreateCPAField(newField)
+	require.Nil(t, appErr)
+
+	for i := range 3 {
 		newValue := &model.PropertyValue{
 			TargetID:   model.NewId(),
 			TargetType: "user",
@@ -302,9 +390,9 @@ func TestDeleteCPAField(t *testing.T) {
 	}
 
 	t.Run("should fail if the field doesn't exist", func(t *testing.T) {
-		err := th.App.DeleteCPAField(model.NewId())
-		require.NotNil(t, err)
-		require.Equal(t, "app.custom_profile_attributes.property_field_not_found.app_error", err.Id)
+		appErr := th.App.DeleteCPAField(model.NewId())
+		require.NotNil(t, appErr)
+		require.Equal(t, "app.custom_profile_attributes.property_field_not_found.app_error", appErr.Id)
 	})
 
 	t.Run("should not allow to delete a field outside of CPA", func(t *testing.T) {
@@ -428,6 +516,63 @@ func TestGetCPAValue(t *testing.T) {
 	})
 }
 
+func TestListCPAValues(t *testing.T) {
+	os.Setenv("MM_FEATUREFLAGS_CUSTOMPROFILEATTRIBUTES", "true")
+	defer os.Unsetenv("MM_FEATUREFLAGS_CUSTOMPROFILEATTRIBUTES")
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	cpaGroupID, cErr := th.App.cpaGroupID()
+	require.NoError(t, cErr)
+
+	userID := model.NewId()
+
+	t.Run("should return empty list when user has no values", func(t *testing.T) {
+		values, appErr := th.App.ListCPAValues(userID)
+		require.Nil(t, appErr)
+		require.Empty(t, values)
+	})
+
+	t.Run("should list all values for a user", func(t *testing.T) {
+		var expectedValues []json.RawMessage
+
+		for i := 1; i <= CustomProfileAttributesFieldLimit; i++ {
+			field := &model.PropertyField{
+				GroupID: cpaGroupID,
+				Name:    fmt.Sprintf("Field %d", i),
+				Type:    model.PropertyFieldTypeText,
+			}
+			_, err := th.App.Srv().propertyService.CreatePropertyField(field)
+			require.NoError(t, err)
+
+			value := &model.PropertyValue{
+				TargetID:   userID,
+				TargetType: "user",
+				GroupID:    cpaGroupID,
+				FieldID:    field.ID,
+				Value:      json.RawMessage(fmt.Sprintf(`"Value %d"`, i)),
+			}
+			_, err = th.App.Srv().propertyService.CreatePropertyValue(value)
+			require.NoError(t, err)
+			expectedValues = append(expectedValues, value.Value)
+		}
+
+		// List values for original user
+		values, appErr := th.App.ListCPAValues(userID)
+		require.Nil(t, appErr)
+		require.Len(t, values, CustomProfileAttributesFieldLimit)
+
+		actualValues := make([]json.RawMessage, len(values))
+		for i, value := range values {
+			require.Equal(t, userID, value.TargetID)
+			require.Equal(t, "user", value.TargetType)
+			require.Equal(t, cpaGroupID, value.GroupID)
+			actualValues[i] = value.Value
+		}
+		require.ElementsMatch(t, expectedValues, actualValues)
+	})
+}
+
 func TestPatchCPAValue(t *testing.T) {
 	os.Setenv("MM_FEATUREFLAGS_CUSTOMPROFILEATTRIBUTES", "true")
 	defer os.Unsetenv("MM_FEATUREFLAGS_CUSTOMPROFILEATTRIBUTES")
@@ -515,60 +660,3 @@ func TestPatchCPAValue(t *testing.T) {
 		require.Equal(t, userID, updatedValue.TargetID)
 	})
 }
-
-func TestListCPAValues(t *testing.T) {
-	os.Setenv("MM_FEATUREFLAGS_CUSTOMPROFILEATTRIBUTES", "true")
-	defer os.Unsetenv("MM_FEATUREFLAGS_CUSTOMPROFILEATTRIBUTES")
-	th := Setup(t).InitBasic()
-	defer th.TearDown()
-
-	cpaGroupID, cErr := th.App.cpaGroupID()
-	require.NoError(t, cErr)
-
-	userID := model.NewId()
-
-	t.Run("should return empty list when user has no values", func(t *testing.T) {
-		values, appErr := th.App.ListCPAValues(userID)
-		require.Nil(t, appErr)
-		require.Empty(t, values)
-	})
-
-	t.Run("should list all values for a user", func(t *testing.T) {
-		var expectedValues []json.RawMessage
-
-		for i := 1; i <= CustomProfileAttributesFieldLimit; i++ {
-			field := &model.PropertyField{
-				GroupID: cpaGroupID,
-				Name:    fmt.Sprintf("Field %d", i),
-				Type:    model.PropertyFieldTypeText,
-			}
-			_, err := th.App.Srv().propertyService.CreatePropertyField(field)
-			require.NoError(t, err)
-
-			value := &model.PropertyValue{
-				TargetID:   userID,
-				TargetType: "user",
-				GroupID:    cpaGroupID,
-				FieldID:    field.ID,
-				Value:      json.RawMessage(fmt.Sprintf(`"Value %d"`, i)),
-			}
-			_, err = th.App.Srv().propertyService.CreatePropertyValue(value)
-			require.NoError(t, err)
-			expectedValues = append(expectedValues, value.Value)
-		}
-
-		// List values for original user
-		values, appErr := th.App.ListCPAValues(userID)
-		require.Nil(t, appErr)
-		require.Len(t, values, CustomProfileAttributesFieldLimit)
-
-		actualValues := make([]json.RawMessage, len(values))
-		for i, value := range values {
-			require.Equal(t, userID, value.TargetID)
-			require.Equal(t, "user", value.TargetType)
-			require.Equal(t, cpaGroupID, value.GroupID)
-			actualValues[i] = value.Value
-		}
-		require.ElementsMatch(t, expectedValues, actualValues)
-	})
-}