From db0e371ca239b5286e58ad264c46a8891eafbe9b Mon Sep 17 00:00:00 2001 From: Julien Tant Date: Thu, 13 Feb 2025 19:05:58 -0700 Subject: [PATCH] generic options --- .../channels/app/custom_profile_attributes.go | 20 +- .../app/custom_profile_attributes_test.go | 45 +++-- .../public/model/custom_profile_attributes.go | 66 +------ .../model/custom_profile_attributes_test.go | 185 ------------------ server/public/model/property_field.go | 54 +++++ 5 files changed, 91 insertions(+), 279 deletions(-) diff --git a/server/channels/app/custom_profile_attributes.go b/server/channels/app/custom_profile_attributes.go index 108c0d77794..54de6bed0cc 100644 --- a/server/channels/app/custom_profile_attributes.go +++ b/server/channels/app/custom_profile_attributes.go @@ -285,24 +285,16 @@ func validateCustomProfileAttributesField(field *model.PropertyField) *model.App } case model.PropertyFieldTypeSelect, model.PropertyFieldTypeMultiselect: - if options, ok := field.Attrs[model.CustomProfileAttributesPropertyAttrsOptions]; ok { - var finalOptions model.CustomProfileAttributesSelectOptions - optionsArr, ok := options.([]any) - if !ok { - return model.NewAppError("ValidateCPAField", "app.custom_profile_attributes.not_array_options.app_error", nil, "", http.StatusUnprocessableEntity) - } - for i, option := range optionsArr { - optionMap, ok := option.(map[string]any) - if !ok { - return model.NewAppError("ValidateCPAField", "app.custom_profile_attributes.not_map_option.app_error", map[string]any{"Index": i}, "", http.StatusUnprocessableEntity) - } - option := model.NewCustomProfileAttributesSelectOptionFromMap(optionMap) - finalOptions = append(finalOptions, option) + if options, ok := field.Attrs[model.PropertyFieldAttributeOptions]; ok { + finalOptions, err := model.NewPropertyOptionsFromFieldAttrs[*model.CustomProfileAttributesSelectOption](options) + if err != nil { + return model.NewAppError("ValidateCPAField", "app.custom_profile_attributes.invalid_options.app_error", nil, "", http.StatusUnprocessableEntity).Wrap(err) } + if err := finalOptions.IsValid(); err != nil { return model.NewAppError("ValidateCPAField", "app.custom_profile_attributes.invalid_options.app_error", nil, "", http.StatusUnprocessableEntity).Wrap(err) } - field.Attrs[model.CustomProfileAttributesPropertyAttrsOptions] = finalOptions + field.Attrs[model.PropertyFieldAttributeOptions] = finalOptions } } diff --git a/server/channels/app/custom_profile_attributes_test.go b/server/channels/app/custom_profile_attributes_test.go index 8c65b0edd9a..1c86a4b93d7 100644 --- a/server/channels/app/custom_profile_attributes_test.go +++ b/server/channels/app/custom_profile_attributes_test.go @@ -12,7 +12,6 @@ import ( "time" "github.com/mattermost/mattermost/server/public/model" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -280,7 +279,7 @@ func TestPatchCPAField(t *testing.T) { Name: "Select Field", Type: model.PropertyFieldTypeSelect, Attrs: map[string]any{ - model.CustomProfileAttributesPropertyAttrsOptions: []any{ + model.PropertyFieldAttributeOptions: []any{ map[string]any{ "name": "Option 1", "color": "#111111", @@ -296,7 +295,7 @@ func TestPatchCPAField(t *testing.T) { require.Nil(t, err) // Get the original option IDs - options := createdSelectField.Attrs[model.CustomProfileAttributesPropertyAttrsOptions].(model.CustomProfileAttributesSelectOptions) + options := createdSelectField.Attrs[model.PropertyFieldAttributeOptions].(model.PropertyOptions[*model.CustomProfileAttributesSelectOption]) require.Len(t, options, 2) originalID1 := options[0].ID originalID2 := options[1].ID @@ -306,7 +305,7 @@ func TestPatchCPAField(t *testing.T) { // Patch the field with updated option names and colors selectPatch := &model.PropertyFieldPatch{ Attrs: model.NewPointer(map[string]any{ - model.CustomProfileAttributesPropertyAttrsOptions: []any{ + model.PropertyFieldAttributeOptions: []any{ map[string]any{ "id": originalID1, "name": "Updated Option 1", @@ -328,7 +327,7 @@ func TestPatchCPAField(t *testing.T) { updatedSelectField, err := th.App.PatchCPAField(createdSelectField.ID, selectPatch) require.Nil(t, err) - updatedOptions := updatedSelectField.Attrs[model.CustomProfileAttributesPropertyAttrsOptions].(model.CustomProfileAttributesSelectOptions) + updatedOptions := updatedSelectField.Attrs[model.PropertyFieldAttributeOptions].(model.PropertyOptions[*model.CustomProfileAttributesSelectOption]) require.Len(t, updatedOptions, 3) // Verify the options were updated while preserving IDs @@ -709,7 +708,7 @@ func TestValidateCustomProfileAttributesField(t *testing.T) { field: &model.PropertyField{ Type: model.PropertyFieldTypeSelect, Attrs: model.StringInterface{ - model.CustomProfileAttributesPropertyAttrsOptions: []any{ + model.PropertyFieldAttributeOptions: []any{ map[string]interface{}{ "name": "Option 1", "color": "#123456", @@ -724,7 +723,7 @@ func TestValidateCustomProfileAttributesField(t *testing.T) { expectError: false, expectedAttrs: model.StringInterface{ model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityDefault, - model.CustomProfileAttributesPropertyAttrsOptions: model.CustomProfileAttributesSelectOptions{ + model.PropertyFieldAttributeOptions: model.PropertyOptions[*model.CustomProfileAttributesSelectOption]{ {Name: "Option 1", Color: "#123456"}, {Name: "Option 2", Color: "#654321"}, }, @@ -735,7 +734,7 @@ func TestValidateCustomProfileAttributesField(t *testing.T) { field: &model.PropertyField{ Type: model.PropertyFieldTypeSelect, Attrs: model.StringInterface{ - model.CustomProfileAttributesPropertyAttrsOptions: []any{ + model.PropertyFieldAttributeOptions: []any{ map[string]interface{}{ "name": "Option 1", "color": "opt1", @@ -755,24 +754,24 @@ func TestValidateCustomProfileAttributesField(t *testing.T) { field: &model.PropertyField{ Type: model.PropertyFieldTypeSelect, Attrs: model.StringInterface{ - model.CustomProfileAttributesPropertyAttrsOptions: "not an array", + model.PropertyFieldAttributeOptions: "not an array", }, }, expectError: true, - errorId: "app.custom_profile_attributes.not_array_options.app_error", + errorId: "app.custom_profile_attributes.invalid_options.app_error", }, { name: "invalid select field with invalid option format", field: &model.PropertyField{ Type: model.PropertyFieldTypeSelect, Attrs: model.StringInterface{ - model.CustomProfileAttributesPropertyAttrsOptions: []interface{}{ - "not a map", + model.PropertyFieldAttributeOptions: []interface{}{ + "some string", }, }, }, expectError: true, - errorId: "app.custom_profile_attributes.not_map_option.app_error", + errorId: "app.custom_profile_attributes.invalid_options.app_error", }, { name: "invalid field with unknown visibility", @@ -791,23 +790,27 @@ func TestValidateCustomProfileAttributesField(t *testing.T) { t.Run(tt.name, func(t *testing.T) { err := validateCustomProfileAttributesField(tt.field) if tt.expectError { - assert.NotNil(t, err) - assert.Equal(t, tt.errorId, err.Id) + require.NotNil(t, err) + require.Equal(t, tt.errorId, err.Id) } else { - assert.Nil(t, err) + var ogErr error + if err != nil { + ogErr = err.Unwrap() + } + require.Nilf(t, err, "unexpected error: %v, with original error: %v", err, ogErr) if tt.expectedAttrs != nil { for key, value := range tt.expectedAttrs { - if key == model.CustomProfileAttributesPropertyAttrsOptions { - expectedOptions := value.(model.CustomProfileAttributesSelectOptions) - actualOptions := tt.field.Attrs[model.CustomProfileAttributesPropertyAttrsOptions].(model.CustomProfileAttributesSelectOptions) + if key == model.PropertyFieldAttributeOptions { + expectedOptions := value.(model.PropertyOptions[*model.CustomProfileAttributesSelectOption]) + actualOptions := tt.field.Attrs[model.PropertyFieldAttributeOptions].(model.PropertyOptions[*model.CustomProfileAttributesSelectOption]) // remove IDs from actualOptions to compare for i := range actualOptions { actualOptions[i].ID = "" } - assert.ElementsMatch(t, expectedOptions, actualOptions) + require.ElementsMatch(t, expectedOptions, actualOptions) } else { - assert.Equal(t, value, tt.field.Attrs[key]) + require.Equal(t, value, tt.field.Attrs[key]) } } } diff --git a/server/public/model/custom_profile_attributes.go b/server/public/model/custom_profile_attributes.go index 4719261d97b..e5641c17747 100644 --- a/server/public/model/custom_profile_attributes.go +++ b/server/public/model/custom_profile_attributes.go @@ -5,15 +5,12 @@ package model import ( "errors" - "fmt" - "strings" ) const CustomProfileAttributesPropertyGroupName = "custom_profile_attributes" const ( CustomProfileAttributesPropertyAttrsValueType = "value_type" - CustomProfileAttributesPropertyAttrsOptions = "options" CustomProfileAttributesPropertyAttrsVisibility = "visibility" ) @@ -59,43 +56,16 @@ type CustomProfileAttributesSelectOption struct { Color string `json:"color"` } -func NewCustomProfileAttributesSelectOptionFromMap(m map[string]any) CustomProfileAttributesSelectOption { - id := "" - name := "" - color := "" - - if v, ok := m["id"]; ok { - if vStr, ok := v.(string); ok { - id = vStr - } - } - - if v, ok := m["name"]; ok { - if vStr, ok := v.(string); ok { - name = vStr - } - } - - if v, ok := m["color"]; ok { - if vStr, ok := v.(string); ok { - color = vStr - } - } - - return NewCustomProfileAttributesSelectOption(id, name, color) +func (c CustomProfileAttributesSelectOption) GetID() string { + return c.ID } -func NewCustomProfileAttributesSelectOption(id, name, color string) CustomProfileAttributesSelectOption { - optionID := strings.TrimSpace(id) - if optionID == "" { - optionID = NewId() - } +func (c CustomProfileAttributesSelectOption) GetName() string { + return c.Name +} - return CustomProfileAttributesSelectOption{ - ID: optionID, - Name: strings.TrimSpace(name), - Color: strings.TrimSpace(color), - } +func (c *CustomProfileAttributesSelectOption) SetID(id string) { + c.ID = id } func (c CustomProfileAttributesSelectOption) IsValid() error { @@ -113,25 +83,3 @@ func (c CustomProfileAttributesSelectOption) IsValid() error { return nil } - -type CustomProfileAttributesSelectOptions []CustomProfileAttributesSelectOption - -func (c CustomProfileAttributesSelectOptions) IsValid() error { - if len(c) == 0 { - return errors.New("options list cannot be empty") - } - - seenNames := make(map[string]struct{}) - for i, option := range c { - if err := option.IsValid(); err != nil { - return fmt.Errorf("invalid option at index %d: %w", i, err) - } - - if _, exists := seenNames[option.Name]; exists { - return fmt.Errorf("duplicate option name found at index %d: %s", i, option.Name) - } - seenNames[option.Name] = struct{}{} - } - - return nil -} diff --git a/server/public/model/custom_profile_attributes_test.go b/server/public/model/custom_profile_attributes_test.go index 6f2d9739de9..67ace328766 100644 --- a/server/public/model/custom_profile_attributes_test.go +++ b/server/public/model/custom_profile_attributes_test.go @@ -9,42 +9,6 @@ import ( "github.com/stretchr/testify/assert" ) -func TestNewCustomProfileAttributeSelectOption(t *testing.T) { - t.Run("creates valid option with generated ID", func(t *testing.T) { - option := NewCustomProfileAttributesSelectOption("", "Test Option", "#FF0000") - - assert.NotEmpty(t, option.ID) - assert.True(t, IsValidId(option.ID)) - assert.Equal(t, "Test Option", option.Name) - assert.Equal(t, "#FF0000", option.Color) - }) - - t.Run("trims spaces from name and color", func(t *testing.T) { - option := NewCustomProfileAttributesSelectOption("", " Test Option ", " #FF0000 ") - - assert.Equal(t, "Test Option", option.Name) - assert.Equal(t, "#FF0000", option.Color) - }) - - t.Run("preserves provided ID", func(t *testing.T) { - providedID := NewId() - option := NewCustomProfileAttributesSelectOption(providedID, "Test Option", "#FF0000") - - assert.Equal(t, providedID, option.ID) - assert.Equal(t, "Test Option", option.Name) - assert.Equal(t, "#FF0000", option.Color) - }) - - t.Run("trims spaces from ID", func(t *testing.T) { - validID := NewId() - option := NewCustomProfileAttributesSelectOption(" "+validID+" ", "Test Option", "#FF0000") - - assert.Equal(t, validID, option.ID) - assert.Equal(t, "Test Option", option.Name) - assert.Equal(t, "#FF0000", option.Color) - }) -} - func TestCustomProfileAttributeSelectOptionIsValid(t *testing.T) { tests := []struct { name string @@ -108,152 +72,3 @@ func TestCustomProfileAttributeSelectOptionIsValid(t *testing.T) { }) } } - -func TestNewCustomProfileAttributesSelectOptionFromMap(t *testing.T) { - tests := []struct { - name string - input map[string]any - expected CustomProfileAttributesSelectOption - }{ - { - name: "valid option", - input: map[string]any{ - "name": "Test Option", - "color": "#FF0000", - }, - expected: CustomProfileAttributesSelectOption{ - Name: "Test Option", - Color: "#FF0000", - }, - }, - { - name: "with spaces to trim", - input: map[string]any{ - "name": " Test Option ", - "color": " #FF0000 ", - }, - expected: CustomProfileAttributesSelectOption{ - Name: "Test Option", - Color: "#FF0000", - }, - }, - { - name: "with provided id", - input: map[string]any{ - "id": "existingid123456789012345678", - "name": "Test Option", - "color": "#FF0000", - }, - expected: CustomProfileAttributesSelectOption{ - ID: "existingid123456789012345678", - Name: "Test Option", - Color: "#FF0000", - }, - }, - { - name: "with non-string values", - input: map[string]any{ - "name": 123, - "color": true, - }, - expected: CustomProfileAttributesSelectOption{ - Name: "", - Color: "", - }, - }, - { - name: "empty map", - input: map[string]any{}, - expected: CustomProfileAttributesSelectOption{ - Name: "", - Color: "", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := NewCustomProfileAttributesSelectOptionFromMap(tt.input) - if tt.expected.ID != "" { - // When an ID is expected, verify it matches exactly - assert.Equal(t, tt.expected.ID, result.ID) - } else { - // When no ID is provided, verify generated ID is valid - assert.True(t, IsValidId(result.ID)) - } - assert.Equal(t, tt.expected.Name, result.Name) - assert.Equal(t, tt.expected.Color, result.Color) - }) - } -} - -func TestCustomProfileAttributesSelectOptionsIsValid(t *testing.T) { - tests := []struct { - name string - options CustomProfileAttributesSelectOptions - wantErr string - }{ - { - name: "empty options", - options: CustomProfileAttributesSelectOptions{}, - wantErr: "options list cannot be empty", - }, - { - name: "valid options with and without color", - options: CustomProfileAttributesSelectOptions{ - { - ID: NewId(), - Name: "Option 1", - Color: "#FF0000", - }, - { - ID: NewId(), - Name: "Option 2", - }, - }, - wantErr: "", - }, - { - name: "invalid option", - options: CustomProfileAttributesSelectOptions{ - { - ID: NewId(), - Name: "Option 1", - Color: "#FF0000", - }, - { - ID: "", - Name: "Option 2", - }, - }, - wantErr: "invalid option at index 1: id cannot be empty", - }, - { - name: "duplicate names", - options: CustomProfileAttributesSelectOptions{ - { - ID: NewId(), - Name: "Option 1", - Color: "#FF0000", - }, - { - ID: NewId(), - Name: "Option 1", - Color: "#00FF00", - }, - }, - wantErr: "duplicate option name found at index 1: Option 1", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := tt.options.IsValid() - if tt.wantErr != "" { - assert.EqualError(t, err, tt.wantErr) - } else { - assert.NoError(t, err) - } - }) - } -} diff --git a/server/public/model/property_field.go b/server/public/model/property_field.go index 531b81ceb87..c769049462e 100644 --- a/server/public/model/property_field.go +++ b/server/public/model/property_field.go @@ -4,7 +4,9 @@ package model import ( + "encoding/json" "errors" + "fmt" "net/http" "strings" ) @@ -174,3 +176,55 @@ type PropertyFieldSearchOpts struct { Cursor PropertyFieldSearchCursor PerPage int } + +const PropertyFieldAttributeOptions = "options" + +type PropertyOption interface { + GetID() string + GetName() string + SetID(id string) + IsValid() error +} + +type PropertyOptions[T PropertyOption] []T + +func NewPropertyOptionsFromFieldAttrs[T PropertyOption](optionsArr any) (PropertyOptions[T], error) { + options := PropertyOptions[T]{} + b, err := json.Marshal(optionsArr) + if err != nil { + return nil, fmt.Errorf("failed to marshal options: %w", err) + } + + err = json.Unmarshal(b, &options) + if err != nil { + return nil, fmt.Errorf("failed to unmarshal options: %w", err) + } + + for i := range options { + if options[i].GetID() == "" { + options[i].SetID(NewId()) + } + } + + return options, nil +} + +func (p PropertyOptions[T]) IsValid() error { + if len(p) == 0 { + return errors.New("options list cannot be empty") + } + + seenNames := make(map[string]struct{}) + for i, option := range p { + if err := option.IsValid(); err != nil { + return fmt.Errorf("invalid option at index %d: %w", i, err) + } + + if _, exists := seenNames[option.GetName()]; exists { + return fmt.Errorf("duplicate option name found at index %d: %s", i, option.GetName()) + } + seenNames[option.GetName()] = struct{}{} + } + + return nil +}