Deletes CPA values on CPA field type change (#31122)

* Deletes CPA values on CPA field type change * Fix error method name reference * Cleans the state when a CPA field's type is updated * Fix types * Fix linter * Webapp no longer makes a decision on the change and server sents a flag in the WS message * Fix linter --------- Co-authored-by: Miguel de la Cruz <miguel@ctrlz.es>
2025-05-27 13:38:05 +02:00
--- a/server/channels/app/custom_profile_attributes.go
+++ b/server/channels/app/custom_profile_attributes.go
@@ -10,6 +10,7 @@ import (
 	"sort"

 	"github.com/mattermost/mattermost/server/public/model"
+	"github.com/mattermost/mattermost/server/public/shared/mlog"
 	"github.com/mattermost/mattermost/server/v8/channels/store"
 	"github.com/pkg/errors"
 )
@@ -123,6 +124,11 @@ func (a *App) PatchCPAField(fieldID string, patch *model.PropertyFieldPatch) (*m
 		return nil, appErr
 	}

+	shouldDeleteValues := false
+	if patch.Type != nil && *patch.Type != existingField.Type {
+		shouldDeleteValues = true
+	}
+
 	// custom profile attributes doesn't use targets
 	patch.TargetID = nil
 	patch.TargetType = nil
@@ -130,28 +136,41 @@ func (a *App) PatchCPAField(fieldID string, patch *model.PropertyFieldPatch) (*m

 	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)
+		return nil, model.NewAppError("PatchCPAField", "app.custom_profile_attributes.property_field_conversion.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}

 	if appErr := cpaField.SanitizeAndValidate(); appErr != nil {
 		return nil, appErr
 	}

-	// we've already ensured that the field exists for the CPA group,
-	// we don't need to specify the groupID for the update
-	patchedField, err := a.Srv().propertyService.UpdatePropertyField("", cpaField.ToPropertyField())
+	groupID, err := a.CpaGroupID()
+	if err != nil {
+		return nil, model.NewAppError("PatchCPAField", "app.custom_profile_attributes.cpa_group_id.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
+	}
+
+	patchedField, err := a.Srv().propertyService.UpdatePropertyField(groupID, cpaField.ToPropertyField())
 	if err != nil {
 		var nfErr *store.ErrNotFound
 		switch {
 		case errors.As(err, &nfErr):
-			return nil, model.NewAppError("UpdateCPAField", "app.custom_profile_attributes.property_field_not_found.app_error", nil, "", http.StatusNotFound).Wrap(err)
+			return nil, model.NewAppError("PatchCPAField", "app.custom_profile_attributes.property_field_not_found.app_error", nil, "", http.StatusNotFound).Wrap(err)
 		default:
-			return nil, model.NewAppError("UpdateCPAField", "app.custom_profile_attributes.property_field_update.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
+			return nil, model.NewAppError("PatchCPAField", "app.custom_profile_attributes.property_field_update.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
+		}
+	}
+
+	if shouldDeleteValues {
+		if dErr := a.Srv().propertyService.DeletePropertyValuesForField(groupID, patchedField.ID); dErr != nil {
+			a.Log().Error("Error deleting property values when updating field",
+				mlog.String("fieldID", patchedField.ID),
+				mlog.Err(dErr),
+			)
 		}
 	}

 	message := model.NewWebSocketEvent(model.WebsocketEventCPAFieldUpdated, "", "", "", nil, "")
 	message.Add("field", patchedField)
+	message.Add("delete_values", shouldDeleteValues)
 	a.Publish(message)

 	return patchedField, nil
--- a/server/channels/app/custom_profile_attributes_test.go
+++ b/server/channels/app/custom_profile_attributes_test.go
@@ -409,6 +409,134 @@ func TestPatchCPAField(t *testing.T) {
 		require.Equal(t, "New Option 1.5", updatedOptions[1].Name)
 		require.Equal(t, "#353535", updatedOptions[1].Color)
 	})
+
+	t.Run("Should not delete the values of a field after patching it if the type has not changed", func(t *testing.T) {
+		// Create a select field with options
+		field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
+			GroupID: cpaGroupID,
+			Name:    "Select Field with values",
+			Type:    model.PropertyFieldTypeSelect,
+			Attrs: model.StringInterface{
+				model.PropertyFieldAttributeOptions: []any{
+					map[string]any{
+						"name":  "Option 1",
+						"color": "#FF5733",
+					},
+					map[string]any{
+						"name":  "Option 2",
+						"color": "#33FF57",
+					},
+				},
+			},
+		})
+		require.NoError(t, err)
+		createdField, appErr := th.App.CreateCPAField(field)
+		require.Nil(t, appErr)
+
+		// Get the option IDs by converting back to CPA field
+		cpaField, err := model.NewCPAFieldFromPropertyField(createdField)
+		require.NoError(t, err)
+
+		options := cpaField.Attrs.Options
+		require.Len(t, options, 2)
+		optionID := options[0].ID
+		require.NotEmpty(t, optionID)
+
+		// Create values for this field using the first option
+		userID := model.NewId()
+		value, appErr := th.App.PatchCPAValue(userID, createdField.ID, json.RawMessage(fmt.Sprintf(`"%s"`, optionID)), false)
+		require.Nil(t, appErr)
+		require.NotNil(t, value)
+
+		// Patch the field without changing type (just update name and add a new option)
+		patch := &model.PropertyFieldPatch{
+			Name: model.NewPointer("Updated select field name"),
+			Attrs: model.NewPointer(model.StringInterface{
+				model.PropertyFieldAttributeOptions: []any{
+					map[string]any{
+						"id":    optionID, // Keep the same ID for the first option
+						"name":  "Updated Option 1",
+						"color": "#FF5733",
+					},
+					map[string]any{
+						"name":  "Option 2",
+						"color": "#33FF57",
+					},
+					map[string]any{
+						"name":  "Option 3",
+						"color": "#5733FF",
+					},
+				},
+			}),
+		}
+		updatedField, appErr := th.App.PatchCPAField(createdField.ID, patch)
+		require.Nil(t, appErr)
+		require.Equal(t, "Updated select field name", updatedField.Name)
+		require.Equal(t, model.PropertyFieldTypeSelect, updatedField.Type)
+
+		// Verify values still exist
+		values, appErr := th.App.ListCPAValues(userID)
+		require.Nil(t, appErr)
+		require.Len(t, values, 1)
+		require.Equal(t, json.RawMessage(fmt.Sprintf(`"%s"`, optionID)), values[0].Value)
+	})
+
+	t.Run("Should delete the values of a field after patching it if the type has changed", func(t *testing.T) {
+		// Create a select field with options
+		field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
+			GroupID: cpaGroupID,
+			Name:    "Select Field with type change",
+			Type:    model.PropertyFieldTypeSelect,
+			Attrs: model.StringInterface{
+				model.PropertyFieldAttributeOptions: []any{
+					map[string]any{
+						"name":  "Option A",
+						"color": "#FF5733",
+					},
+					map[string]any{
+						"name":  "Option B",
+						"color": "#33FF57",
+					},
+				},
+			},
+		})
+		require.NoError(t, err)
+		createdField, appErr := th.App.CreateCPAField(field)
+		require.Nil(t, appErr)
+
+		// Get the option IDs by converting back to CPA field
+		cpaField, err := model.NewCPAFieldFromPropertyField(createdField)
+		require.NoError(t, err)
+
+		options := cpaField.Attrs.Options
+		require.Len(t, options, 2)
+		optionID := options[0].ID
+		require.NotEmpty(t, optionID)
+
+		// Create values for this field
+		userID := model.NewId()
+		value, appErr := th.App.PatchCPAValue(userID, createdField.ID, json.RawMessage(fmt.Sprintf(`"%s"`, optionID)), false)
+		require.Nil(t, appErr)
+		require.NotNil(t, value)
+
+		// Verify value exists before type change
+		values, appErr := th.App.ListCPAValues(userID)
+		require.Nil(t, appErr)
+		require.Len(t, values, 1)
+
+		// Patch the field and change type from select to text
+		patch := &model.PropertyFieldPatch{
+			Type: model.NewPointer(model.PropertyFieldTypeText),
+		}
+		updatedField, appErr := th.App.PatchCPAField(createdField.ID, patch)
+		require.Nil(t, appErr)
+		require.Equal(t, model.PropertyFieldTypeText, updatedField.Type)
+
+		// Verify values have been deleted
+		values, appErr = th.App.ListCPAValues(userID)
+		require.Nil(t, appErr)
+		require.Empty(t, values)
+	})
 }

 func TestDeleteCPAField(t *testing.T) {
--- a/server/channels/app/properties/property_field.go
+++ b/server/channels/app/properties/property_field.go
@@ -59,7 +59,7 @@ func (ps *PropertyService) DeletePropertyField(groupID, id string) error {
 		}
 	}

-	if err := ps.valueStore.DeleteForField(id); err != nil {
+	if err := ps.valueStore.DeleteForField(groupID, id); err != nil {
 		return err
 	}

--- a/server/channels/app/properties/property_value.go
+++ b/server/channels/app/properties/property_value.go
@@ -60,3 +60,7 @@ func (ps *PropertyService) DeletePropertyValue(groupID, id string) error {
 func (ps *PropertyService) DeletePropertyValuesForTarget(groupID string, targetType string, targetID string) error {
 	return ps.valueStore.DeleteForTarget(groupID, targetType, targetID)
 }
+
+func (ps *PropertyService) DeletePropertyValuesForField(groupID, fieldID string) error {
+	return ps.valueStore.DeleteForField(groupID, fieldID)
+}
--- a/server/channels/store/retrylayer/retrylayer.go
+++ b/server/channels/store/retrylayer/retrylayer.go
@@ -9452,11 +9452,11 @@ func (s *RetryLayerPropertyValueStore) Delete(groupID string, id string) error {

 }

-func (s *RetryLayerPropertyValueStore) DeleteForField(id string) error {
+func (s *RetryLayerPropertyValueStore) DeleteForField(groupID string, fieldID string) error {

 	tries := 0
 	for {
-		err := s.PropertyValueStore.DeleteForField(id)
+		err := s.PropertyValueStore.DeleteForField(groupID, fieldID)
 		if err == nil {
 			return nil
 		}
--- a/server/channels/store/sqlstore/property_value_store.go
+++ b/server/channels/store/sqlstore/property_value_store.go
@@ -327,12 +327,16 @@ func (s *SqlPropertyValueStore) Delete(groupID string, id string) error {
 	return nil
 }

-func (s *SqlPropertyValueStore) DeleteForField(fieldID string) error {
+func (s *SqlPropertyValueStore) DeleteForField(groupID, fieldID string) error {
 	builder := s.getQueryBuilder().
 		Update("PropertyValues").
 		Set("DeleteAt", model.GetMillis()).
 		Where(sq.Eq{"FieldID": fieldID})

+	if groupID != "" {
+		builder = builder.Where(sq.Eq{"GroupID": groupID})
+	}
+
 	if _, err := s.GetMaster().ExecBuilder(builder); err != nil {
 		return errors.Wrap(err, "property_value_delete_for_field_exec")
 	}
--- a/server/channels/store/store.go
+++ b/server/channels/store/store.go
@@ -1108,7 +1108,7 @@ type PropertyValueStore interface {
 	Update(groupID string, values []*model.PropertyValue) ([]*model.PropertyValue, error)
 	Upsert(values []*model.PropertyValue) ([]*model.PropertyValue, error)
 	Delete(groupID string, id string) error
-	DeleteForField(id string) error
+	DeleteForField(groupID, fieldID string) error
 	DeleteForTarget(groupID string, targetType string, targetID string) error
 }

--- a/server/channels/store/storetest/mocks/PropertyValueStore.go
+++ b/server/channels/store/storetest/mocks/PropertyValueStore.go
@@ -62,17 +62,17 @@ func (_m *PropertyValueStore) Delete(groupID string, id string) error {
 	return r0
 }

-// DeleteForField provides a mock function with given fields: id
-func (_m *PropertyValueStore) DeleteForField(id string) error {
-	ret := _m.Called(id)
+// DeleteForField provides a mock function with given fields: groupID, fieldID
+func (_m *PropertyValueStore) DeleteForField(groupID string, fieldID string) error {
+	ret := _m.Called(groupID, fieldID)

 	if len(ret) == 0 {
 		panic("no return value specified for DeleteForField")
 	}

 	var r0 error
-	if rf, ok := ret.Get(0).(func(string) error); ok {
-		r0 = rf(id)
+	if rf, ok := ret.Get(0).(func(string, string) error); ok {
+		r0 = rf(groupID, fieldID)
 	} else {
 		r0 = ret.Error(0)
 	}
--- a/server/channels/store/storetest/property_value_store.go
+++ b/server/channels/store/storetest/property_value_store.go
@@ -933,54 +933,213 @@ func testCreatePropertyValueWithArray(t *testing.T, _ request.CTX, ss store.Stor
 }

 func testDeleteForField(t *testing.T, _ request.CTX, ss store.Store) {
-	fieldID := model.NewId()
-	groupID := model.NewId()
+	t.Run("should delete values with matching fieldID and groupID", func(t *testing.T) {
+		fieldID := model.NewId()
+		groupID := model.NewId()

-	// Create test values
-	value1 := &model.PropertyValue{
-		TargetID:   model.NewId(),
-		TargetType: "test_type",
-		GroupID:    groupID,
-		FieldID:    fieldID,
-		Value:      json.RawMessage(`"value 1"`),
-	}
+		// Create test values
+		value1 := &model.PropertyValue{
+			TargetID:   model.NewId(),
+			TargetType: "test_type",
+			GroupID:    groupID,
+			FieldID:    fieldID,
+			Value:      json.RawMessage(`"value 1"`),
+		}

-	value2 := &model.PropertyValue{
-		TargetID:   model.NewId(),
-		TargetType: "test_type",
-		GroupID:    groupID,
-		FieldID:    fieldID,
-		Value:      json.RawMessage(`"value 2"`),
-	}
+		value2 := &model.PropertyValue{
+			TargetID:   model.NewId(),
+			TargetType: "test_type",
+			GroupID:    groupID,
+			FieldID:    fieldID,
+			Value:      json.RawMessage(`"value 2"`),
+		}

-	value3 := &model.PropertyValue{
-		TargetID:   model.NewId(),
-		TargetType: "test_type",
-		GroupID:    groupID,
-		FieldID:    model.NewId(), // Different field ID
-		Value:      json.RawMessage(`"value 3"`),
-	}
+		value3 := &model.PropertyValue{
+			TargetID:   model.NewId(),
+			TargetType: "test_type",
+			GroupID:    groupID,
+			FieldID:    model.NewId(), // Different field ID
+			Value:      json.RawMessage(`"value 3"`),
+		}

-	for _, value := range []*model.PropertyValue{value1, value2, value3} {
-		_, err := ss.PropertyValue().Create(value)
+		for _, value := range []*model.PropertyValue{value1, value2, value3} {
+			_, err := ss.PropertyValue().Create(value)
+			require.NoError(t, err)
+		}
+
+		// Delete values for the field
+		err := ss.PropertyValue().DeleteForField(groupID, fieldID)
 		require.NoError(t, err)
-	}

-	// Delete values for the field
-	err := ss.PropertyValue().DeleteForField(fieldID)
-	require.NoError(t, err)
+		// Verify values were soft-deleted
+		deletedValues, err := ss.PropertyValue().GetMany(groupID, []string{value1.ID, value2.ID})
+		require.NoError(t, err)
+		require.Len(t, deletedValues, 2)
+		require.NotZero(t, deletedValues[0].DeleteAt)
+		require.NotZero(t, deletedValues[1].DeleteAt)

-	// Verify values were soft-deleted
-	deletedValues, err := ss.PropertyValue().GetMany(groupID, []string{value1.ID, value2.ID})
-	require.NoError(t, err)
-	require.Len(t, deletedValues, 2)
-	require.NotZero(t, deletedValues[0].DeleteAt)
-	require.NotZero(t, deletedValues[1].DeleteAt)
+		// Verify value with different field ID was not deleted
+		nonDeletedValue, err := ss.PropertyValue().Get(groupID, value3.ID)
+		require.NoError(t, err)
+		require.Zero(t, nonDeletedValue.DeleteAt)
+	})

-	// Verify value with different field ID was not deleted
-	nonDeletedValue, err := ss.PropertyValue().Get(groupID, value3.ID)
-	require.NoError(t, err)
-	require.Zero(t, nonDeletedValue.DeleteAt)
+	t.Run("should not delete values with non-matching groupID", func(t *testing.T) {
+		fieldID := model.NewId()
+		groupID1 := model.NewId()
+		groupID2 := model.NewId()
+
+		// Create values with same fieldID but different groupIDs
+		value1 := &model.PropertyValue{
+			TargetID:   model.NewId(),
+			TargetType: "test_type",
+			GroupID:    groupID1,
+			FieldID:    fieldID,
+			Value:      json.RawMessage(`"group1 value"`),
+		}
+
+		value2 := &model.PropertyValue{
+			TargetID:   model.NewId(),
+			TargetType: "test_type",
+			GroupID:    groupID2,
+			FieldID:    fieldID,
+			Value:      json.RawMessage(`"group2 value"`),
+		}
+
+		for _, value := range []*model.PropertyValue{value1, value2} {
+			_, err := ss.PropertyValue().Create(value)
+			require.NoError(t, err)
+		}
+
+		// Delete values for the field but only in groupID1
+		err := ss.PropertyValue().DeleteForField(groupID1, fieldID)
+		require.NoError(t, err)
+
+		// Verify value1 was deleted
+		deletedValue, err := ss.PropertyValue().Get(groupID1, value1.ID)
+		require.NoError(t, err)
+		require.NotZero(t, deletedValue.DeleteAt)
+
+		// Verify value2 was not deleted (different group)
+		nonDeletedValue, err := ss.PropertyValue().Get(groupID2, value2.ID)
+		require.NoError(t, err)
+		require.Zero(t, nonDeletedValue.DeleteAt)
+	})
+
+	t.Run("should delete values with empty groupID", func(t *testing.T) {
+		fieldID := model.NewId()
+		groupID1 := model.NewId()
+		groupID2 := model.NewId()
+
+		// Create values with same fieldID but different groupIDs
+		value1 := &model.PropertyValue{
+			TargetID:   model.NewId(),
+			TargetType: "test_type",
+			GroupID:    groupID1,
+			FieldID:    fieldID,
+			Value:      json.RawMessage(`"group1 value 2"`),
+		}
+
+		value2 := &model.PropertyValue{
+			TargetID:   model.NewId(),
+			TargetType: "test_type",
+			GroupID:    groupID2,
+			FieldID:    fieldID,
+			Value:      json.RawMessage(`"group2 value 2"`),
+		}
+
+		for _, value := range []*model.PropertyValue{value1, value2} {
+			_, err := ss.PropertyValue().Create(value)
+			require.NoError(t, err)
+		}
+
+		// Delete values for the field across all groups (empty groupID)
+		err := ss.PropertyValue().DeleteForField("", fieldID)
+		require.NoError(t, err)
+
+		// Verify both values were deleted
+		opts := model.PropertyValueSearchOpts{
+			FieldID:        fieldID,
+			IncludeDeleted: true,
+			PerPage:        10,
+		}
+
+		values, err := ss.PropertyValue().SearchPropertyValues(opts)
+		require.NoError(t, err)
+
+		// Find our two values within the results
+		var foundValues []*model.PropertyValue
+		for _, v := range values {
+			if v.ID == value1.ID || v.ID == value2.ID {
+				foundValues = append(foundValues, v)
+			}
+		}
+
+		require.Len(t, foundValues, 2)
+		for _, v := range foundValues {
+			require.NotZero(t, v.DeleteAt, "Value should be deleted")
+		}
+	})
+
+	t.Run("should work with multiple calls targeting different groups", func(t *testing.T) {
+		fieldID := model.NewId()
+		groupID1 := model.NewId()
+		groupID2 := model.NewId()
+		groupID3 := model.NewId()
+
+		// Create values with same fieldID across multiple groups
+		value1 := &model.PropertyValue{
+			TargetID:   model.NewId(),
+			TargetType: "test_type",
+			GroupID:    groupID1,
+			FieldID:    fieldID,
+			Value:      json.RawMessage(`"multi-group test 1"`),
+		}
+
+		value2 := &model.PropertyValue{
+			TargetID:   model.NewId(),
+			TargetType: "test_type",
+			GroupID:    groupID2,
+			FieldID:    fieldID,
+			Value:      json.RawMessage(`"multi-group test 2"`),
+		}
+
+		value3 := &model.PropertyValue{
+			TargetID:   model.NewId(),
+			TargetType: "test_type",
+			GroupID:    groupID3,
+			FieldID:    fieldID,
+			Value:      json.RawMessage(`"multi-group test 3"`),
+		}
+
+		for _, value := range []*model.PropertyValue{value1, value2, value3} {
+			_, err := ss.PropertyValue().Create(value)
+			require.NoError(t, err)
+		}
+
+		// Delete values for the field but only in groupID1
+		err := ss.PropertyValue().DeleteForField(groupID1, fieldID)
+		require.NoError(t, err)
+
+		// Delete values for the field but only in groupID2
+		err = ss.PropertyValue().DeleteForField(groupID2, fieldID)
+		require.NoError(t, err)
+
+		// Verify value1 was deleted
+		value1Result, err := ss.PropertyValue().Get(groupID1, value1.ID)
+		require.NoError(t, err)
+		require.NotZero(t, value1Result.DeleteAt)
+
+		// Verify value2 was deleted
+		value2Result, err := ss.PropertyValue().Get(groupID2, value2.ID)
+		require.NoError(t, err)
+		require.NotZero(t, value2Result.DeleteAt)
+
+		// Verify value3 was not deleted
+		value3Result, err := ss.PropertyValue().Get(groupID3, value3.ID)
+		require.NoError(t, err)
+		require.Zero(t, value3Result.DeleteAt)
+	})
 }

 func testDeleteForTarget(t *testing.T, _ request.CTX, ss store.Store) {
--- a/server/channels/store/timerlayer/timerlayer.go
+++ b/server/channels/store/timerlayer/timerlayer.go
@@ -7493,10 +7493,10 @@ func (s *TimerLayerPropertyValueStore) Delete(groupID string, id string) error {
 	return err
 }

-func (s *TimerLayerPropertyValueStore) DeleteForField(id string) error {
+func (s *TimerLayerPropertyValueStore) DeleteForField(groupID string, fieldID string) error {
 	start := time.Now()

-	err := s.PropertyValueStore.DeleteForField(id)
+	err := s.PropertyValueStore.DeleteForField(groupID, fieldID)

 	elapsed := float64(time.Since(start)) / float64(time.Second)
 	if s.Root.Metrics != nil {