From e51ea025db39349b7f36ff868eebb69d4f20dbdf Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Tue, 27 May 2025 13:38:05 +0200 Subject: [PATCH] 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 --- .../channels/app/custom_profile_attributes.go | 31 ++- .../app/custom_profile_attributes_test.go | 128 ++++++++++ .../channels/app/properties/property_field.go | 2 +- .../channels/app/properties/property_value.go | 4 + .../channels/store/retrylayer/retrylayer.go | 4 +- .../store/sqlstore/property_value_store.go | 6 +- server/channels/store/store.go | 2 +- .../storetest/mocks/PropertyValueStore.go | 10 +- .../store/storetest/property_value_store.go | 239 +++++++++++++++--- .../channels/store/timerlayer/timerlayer.go | 4 +- .../src/actions/websocket_actions.jsx | 20 +- .../src/actions/websocket_actions.test.jsx | 177 +++++++++++++ .../src/action_types/users.ts | 1 + .../src/reducers/entities/users.test.ts | 97 +++++++ .../src/reducers/entities/users.ts | 20 ++ 15 files changed, 684 insertions(+), 61 deletions(-) diff --git a/server/channels/app/custom_profile_attributes.go b/server/channels/app/custom_profile_attributes.go index 194ef82629..d4387014b0 100644 --- 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 diff --git a/server/channels/app/custom_profile_attributes_test.go b/server/channels/app/custom_profile_attributes_test.go index 0f25bdf283..f43024130e 100644 --- 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) { diff --git a/server/channels/app/properties/property_field.go b/server/channels/app/properties/property_field.go index 388b3478df..fed4d067c6 100644 --- 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 } diff --git a/server/channels/app/properties/property_value.go b/server/channels/app/properties/property_value.go index 895be79ae9..0ffc28d578 100644 --- 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) +} diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index 40616eaf76..2c67daa4b3 100644 --- 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 } diff --git a/server/channels/store/sqlstore/property_value_store.go b/server/channels/store/sqlstore/property_value_store.go index ecd1d436a5..2551ab50a6 100644 --- 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") } diff --git a/server/channels/store/store.go b/server/channels/store/store.go index 6c7b397664..321972d675 100644 --- 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 } diff --git a/server/channels/store/storetest/mocks/PropertyValueStore.go b/server/channels/store/storetest/mocks/PropertyValueStore.go index 1157073db2..f8d280093b 100644 --- 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) } diff --git a/server/channels/store/storetest/property_value_store.go b/server/channels/store/storetest/property_value_store.go index 54860258e9..405898bf4d 100644 --- 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) { diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index ab59be6405..098aa67dfc 100644 --- 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 { diff --git a/webapp/channels/src/actions/websocket_actions.jsx b/webapp/channels/src/actions/websocket_actions.jsx index 307f1b9233..0641a4836f 100644 --- a/webapp/channels/src/actions/websocket_actions.jsx +++ b/webapp/channels/src/actions/websocket_actions.jsx @@ -1952,9 +1952,23 @@ export function handleCustomAttributesCreated(msg) { } export function handleCustomAttributesUpdated(msg) { - return { - type: GeneralTypes.CUSTOM_PROFILE_ATTRIBUTE_FIELD_PATCHED, - data: msg.data.field, + return (dispatch) => { + const {field, delete_values: deleteValues} = msg.data; + + // Check if server indicates values should be deleted + if (deleteValues) { + // Clear values for the field when type changes + dispatch({ + type: UserTypes.CLEAR_CPA_VALUES, + data: {fieldId: field.id}, + }); + } + + // Update the field + dispatch({ + type: GeneralTypes.CUSTOM_PROFILE_ATTRIBUTE_FIELD_PATCHED, + data: field, + }); }; } diff --git a/webapp/channels/src/actions/websocket_actions.test.jsx b/webapp/channels/src/actions/websocket_actions.test.jsx index d231d17488..91d2de4935 100644 --- a/webapp/channels/src/actions/websocket_actions.test.jsx +++ b/webapp/channels/src/actions/websocket_actions.test.jsx @@ -1461,4 +1461,181 @@ describe('handleCustomAttributeCRUD', () => { expect(cpaFields.length).toEqual(1); expect(cpaFields.filter(({id}) => id === field2.id)[0]).toBeTruthy(); }); + + describe('handleCustomAttributesUpdated', () => { + test('should update the CustomAttributeField in the state', () => { + const testStore = realConfigureStore(makeInitialState()); + + // First create a field + testStore.dispatch(handleCustomAttributesCreated({ + event: SocketEvents.CPA_FIELD_CREATED, + data: { + field: field1, + }, + })); + + let cpaFields = getCustomProfileAttributes(testStore.getState()); + expect(cpaFields).toBeTruthy(); + expect(cpaFields.length).toEqual(1); + expect(cpaFields.filter(({id}) => id === field1.id)[0].name).toEqual(field1.name); + + // Update the field + const updatedField = {...field1, name: 'Updated Field Name'}; + testStore.dispatch(handleCustomAttributesUpdated({ + event: SocketEvents.CPA_FIELD_UPDATED, + data: { + field: updatedField, + }, + })); + + cpaFields = getCustomProfileAttributes(testStore.getState()); + expect(cpaFields).toBeTruthy(); + expect(cpaFields.length).toEqual(1); + expect(cpaFields.filter(({id}) => id === field1.id)[0].name).toEqual('Updated Field Name'); + }); + + test('should clear values when delete_values is true', () => { + // Set up a state with a user that has a custom profile attribute value + const testStore = realConfigureStore({ + entities: { + general: {}, + users: { + profiles: { + user1: { + id: 'user1', + custom_profile_attributes: { + [field1.id]: 'some value', + }, + }, + }, + }, + }, + }); + + // First create a field + testStore.dispatch(handleCustomAttributesCreated({ + event: SocketEvents.CPA_FIELD_CREATED, + data: { + field: field1, + }, + })); + + // Update the field with delete_values flag + const updatedField = {...field1, type: 'select'}; + testStore.dispatch(handleCustomAttributesUpdated({ + event: SocketEvents.CPA_FIELD_UPDATED, + data: { + field: updatedField, + delete_values: true, + }, + })); + + // Check that the field was updated + const cpaFields = getCustomProfileAttributes(testStore.getState()); + expect(cpaFields).toBeTruthy(); + expect(cpaFields.length).toEqual(1); + expect(cpaFields.filter(({id}) => id === field1.id)[0].type).toEqual('select'); + + // Check that the user's values for this field were cleared + const user = testStore.getState().entities.users.profiles.user1; + expect(user.custom_profile_attributes?.[field1.id]).toBeFalsy(); + }); + + test('should not clear values when delete_values is false', () => { + // Set up a state with a user that has a custom profile attribute value + const testStore = realConfigureStore({ + entities: { + general: {}, + users: { + profiles: { + user1: { + id: 'user1', + custom_profile_attributes: { + [field1.id]: 'some value', + }, + }, + }, + }, + }, + }); + + // First create a field + testStore.dispatch(handleCustomAttributesCreated({ + event: SocketEvents.CPA_FIELD_CREATED, + data: { + field: field1, + }, + })); + + // Update the field but with delete_values flag set to false + const updatedField = {...field1, name: 'Updated Field Name', type: 'text'}; + testStore.dispatch(handleCustomAttributesUpdated({ + event: SocketEvents.CPA_FIELD_UPDATED, + data: { + field: updatedField, + delete_values: false, + }, + })); + + // Check that the field was updated + const cpaFields = getCustomProfileAttributes(testStore.getState()); + expect(cpaFields).toBeTruthy(); + expect(cpaFields.length).toEqual(1); + expect(cpaFields.filter(({id}) => id === field1.id)[0].name).toEqual('Updated Field Name'); + + // Check that the user's values for this field were NOT cleared + const user = testStore.getState().entities.users.profiles.user1; + expect(user.custom_profile_attributes).toBeTruthy(); + expect(user.custom_profile_attributes[field1.id]).toEqual('some value'); + }); + + test('should not clear values when delete_values is not specified', () => { + // Set up a state with a user that has a custom profile attribute value + const testStore = realConfigureStore({ + entities: { + general: {}, + users: { + profiles: { + user1: { + id: 'user1', + custom_profile_attributes: { + [field1.id]: 'some value', + }, + }, + }, + }, + }, + }); + + // First create a field + testStore.dispatch(handleCustomAttributesCreated({ + event: SocketEvents.CPA_FIELD_CREATED, + data: { + field: field1, + }, + })); + + // Update the field without specifying delete_values + const updatedField = {...field1, type: 'number'}; + testStore.dispatch(handleCustomAttributesUpdated({ + event: SocketEvents.CPA_FIELD_UPDATED, + data: { + field: updatedField, + + // delete_values not specified + }, + })); + + // Check that the field was updated + const cpaFields = getCustomProfileAttributes(testStore.getState()); + expect(cpaFields).toBeTruthy(); + expect(cpaFields.length).toEqual(1); + expect(cpaFields.filter(({id}) => id === field1.id)[0].type).toEqual('number'); + + // Check that the user's values for this field were NOT cleared + const user = testStore.getState().entities.users.profiles.user1; + expect(user.custom_profile_attributes).toBeTruthy(); + expect(user.custom_profile_attributes[field1.id]).toEqual('some value'); + }); + }); }); diff --git a/webapp/channels/src/packages/mattermost-redux/src/action_types/users.ts b/webapp/channels/src/packages/mattermost-redux/src/action_types/users.ts index f4c946163f..9395b55430 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/action_types/users.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/action_types/users.ts @@ -74,4 +74,5 @@ export default keyMirror({ RECEIVED_FILTERED_USER_STATS: null, PROFILE_NO_LONGER_VISIBLE: null, LOGIN: null, + CLEAR_CPA_VALUES: null, }); diff --git a/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/users.test.ts b/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/users.test.ts index 231a73b754..11d24d3855 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/users.test.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/users.test.ts @@ -1086,6 +1086,103 @@ describe('Reducers.users', () => { expect(updatedProfiles.first_user_id.custom_profile_attributes!.field1).toEqual('updatedValue'); }); + + test('UserTypes.CLEAR_CPA_VALUES, should clear the custom profile attribute value for specified field ID', () => { + const user1 = TestHelper.getUserMock({ + id: 'user1', + custom_profile_attributes: { + field1: 'value1', + field2: 'value2', + }, + }); + + const user2 = TestHelper.getUserMock({ + id: 'user2', + custom_profile_attributes: { + field1: 'user2_value1', + field3: 'user2_value3', + }, + }); + + const user3 = TestHelper.getUserMock({ + id: 'user3', + + // No custom profile attributes + }); + + const state = deepFreezeAndThrowOnMutation({ + profiles: {user1, user2, user3}, + }) as UsersState; + + const action = { + type: UserTypes.CLEAR_CPA_VALUES, + data: { + fieldId: 'field1', + }, + }; + + const newState = reducer(state, action); + + // Check user1 (had field1, should be removed) + expect(newState.profiles.user1.custom_profile_attributes).toEqual({ + field2: 'value2', + }); + + // Check user2 (had field1, should be removed) + expect(newState.profiles.user2.custom_profile_attributes).toEqual({ + field3: 'user2_value3', + }); + + // Check user3 (had no attributes, should remain the same) + expect(newState.profiles.user3.custom_profile_attributes).toBeUndefined(); + }); + + test('UserTypes.CLEAR_CPA_VALUES, should return the same state if no profiles have the specified field', () => { + const user1 = TestHelper.getUserMock({ + id: 'user1', + custom_profile_attributes: { + field1: 'value1', + field2: 'value2', + }, + }); + + const state = deepFreezeAndThrowOnMutation({ + profiles: {user1}, + }) as UsersState; + + const action = { + type: UserTypes.CLEAR_CPA_VALUES, + data: { + fieldId: 'non_existent_field', + }, + }; + + const newState = reducer(state, action); + + // State should have changed (new object), but values should remain the same + expect(newState).not.toBe(state); + expect(newState.profiles.user1).toBe(user1); + expect(newState.profiles.user1.custom_profile_attributes).toEqual({ + field1: 'value1', + field2: 'value2', + }); + }); + + test('UserTypes.CLEAR_CPA_VALUES, should handle empty profiles state', () => { + const state = deepFreezeAndThrowOnMutation({ + profiles: {}, + }) as UsersState; + + const action = { + type: UserTypes.CLEAR_CPA_VALUES, + data: { + fieldId: 'field1', + }, + }; + + const newState = reducer(state as UsersState, action); + expect(newState.profiles).toEqual({}); + }); }); test('PROFILE_NO_LONGER_VISIBLE should remove references to users from state', () => { diff --git a/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/users.ts b/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/users.ts index 717db8030b..8a196d01a8 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/users.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/users.ts @@ -236,6 +236,26 @@ function profiles(state: UsersState['profiles'] = {}, action: MMReduxAction) { const profileAttributes = {...existingProfile.custom_profile_attributes, ...customAttributeValues}; return receiveUserProfile(state, {...existingProfile, custom_profile_attributes: profileAttributes}); } + case UserTypes.CLEAR_CPA_VALUES: { + const {fieldId} = action.data; + + return Object.values(state).reduce>((nextState, profile) => { + // Only modify profiles that have this field value + if (profile.custom_profile_attributes && profile.custom_profile_attributes[fieldId] !== undefined) { + const newAttributes = {...profile.custom_profile_attributes}; + delete newAttributes[fieldId]; + + nextState[profile.id] = { + ...profile, + custom_profile_attributes: newAttributes, + }; + } else { + nextState[profile.id] = profile; + } + + return nextState; + }, {}); + } case UserTypes.RECEIVED_PROFILES_LIST: { const users: UserProfile[] = action.data;