From 83ec1d9f7d31dc5ed5836a6e5b3803f88e2afe29 Mon Sep 17 00:00:00 2001 From: Harshil Sharma <18575143+harshilsharma63@users.noreply.github.com> Date: Thu, 13 Feb 2025 12:11:53 +0530 Subject: [PATCH 1/7] added window title for scheduled post tab and fixed draft alignment (#30179) * added window title for scheduled post tab and fixed draft alignment * i18n fix --- .../src/components/drafts/drafts.scss | 5 ++ .../unreads_status_handler/index.ts | 1 + .../unreads_status_handler.test.tsx | 47 +++++++++++++++++++ .../unreads_status_handler.tsx | 11 +++++ webapp/channels/src/i18n/en.json | 1 + 5 files changed, 65 insertions(+) diff --git a/webapp/channels/src/components/drafts/drafts.scss b/webapp/channels/src/components/drafts/drafts.scss index 07c8c6ff3eb..c867f6b8fdc 100644 --- a/webapp/channels/src/components/drafts/drafts.scss +++ b/webapp/channels/src/components/drafts/drafts.scss @@ -14,6 +14,7 @@ &__main { position: absolute; + top: 56px; display: flex; overflow: auto; width: 100%; @@ -68,6 +69,10 @@ position: relative; height: 100%; } + + .DraftList.Drafts__main { + top: 0; + } } } diff --git a/webapp/channels/src/components/unreads_status_handler/index.ts b/webapp/channels/src/components/unreads_status_handler/index.ts index 5a1243928d6..c5a3417ae75 100644 --- a/webapp/channels/src/components/unreads_status_handler/index.ts +++ b/webapp/channels/src/components/unreads_status_handler/index.ts @@ -31,6 +31,7 @@ function mapStateToProps(state: GlobalState, {location: {pathname}}: Props) { unreadStatus: getUnreadStatus(state), inGlobalThreads: matchPath(pathname, {path: '/:team/threads/:threadIdentifier?'}) != null, inDrafts: matchPath(pathname, {path: '/:team/drafts'}) != null, + inScheduledPosts: matchPath(pathname, {path: '/:team/scheduled_posts'}) != null, }; } diff --git a/webapp/channels/src/components/unreads_status_handler/unreads_status_handler.test.tsx b/webapp/channels/src/components/unreads_status_handler/unreads_status_handler.test.tsx index 25f5109a4a6..69f7cca602b 100644 --- a/webapp/channels/src/components/unreads_status_handler/unreads_status_handler.test.tsx +++ b/webapp/channels/src/components/unreads_status_handler/unreads_status_handler.test.tsx @@ -47,6 +47,7 @@ describe('components/UnreadsStatusHandler', () => { currentTeammate: null, inGlobalThreads: false, inDrafts: false, + inScheduledPosts: false, }; test('set correctly the title when needed', () => { @@ -83,6 +84,38 @@ describe('components/UnreadsStatusHandler', () => { currentTeammate: {} as Props['currentTeammate']}); instance.updateTitle(); expect(document.title).toBe('Mattermost - Join a team'); + + wrapper.setProps({ + inDrafts: false, + inScheduledPosts: true, + unreadStatus: 0, + }); + instance.updateTitle(); + expect(document.title).toBe('Scheduled - Test team display name'); + + wrapper.setProps({ + inDrafts: false, + inScheduledPosts: true, + unreadStatus: 10, + }); + instance.updateTitle(); + expect(document.title).toBe('(10) Scheduled - Test team display name'); + + wrapper.setProps({ + inDrafts: true, + inScheduledPosts: false, + unreadStatus: 0, + }); + instance.updateTitle(); + expect(document.title).toBe('Drafts - Test team display name'); + + wrapper.setProps({ + inDrafts: true, + inScheduledPosts: false, + unreadStatus: 10, + }); + instance.updateTitle(); + expect(document.title).toBe('(10) Drafts - Test team display name'); }); test('should set correct title on mentions on safari', () => { @@ -149,4 +182,18 @@ describe('components/UnreadsStatusHandler', () => { expect(document.title).toBe('Drafts - Test team display name'); }); + + test('should display correct title when in scheduled posts tab', () => { + const wrapper = shallowWithIntl( + , + ) as unknown as ShallowWrapper; + wrapper.instance().updateTitle(); + + expect(document.title).toBe('Scheduled - Test team display name'); + }); }); diff --git a/webapp/channels/src/components/unreads_status_handler/unreads_status_handler.tsx b/webapp/channels/src/components/unreads_status_handler/unreads_status_handler.tsx index 12b09a099b9..cd3897e50ea 100644 --- a/webapp/channels/src/components/unreads_status_handler/unreads_status_handler.tsx +++ b/webapp/channels/src/components/unreads_status_handler/unreads_status_handler.tsx @@ -46,6 +46,7 @@ type Props = { currentTeammate: Channel | null; inGlobalThreads: boolean; inDrafts: boolean; + inScheduledPosts: boolean; }; export class UnreadsStatusHandlerClass extends React.PureComponent { @@ -90,6 +91,7 @@ export class UnreadsStatusHandlerClass extends React.PureComponent { unreadStatus, inGlobalThreads, inDrafts, + inScheduledPosts, } = this.props; const {formatMessage} = this.props.intl; @@ -126,6 +128,15 @@ export class UnreadsStatusHandlerClass extends React.PureComponent { displayName: currentTeam.display_name, siteName: currentSiteName, }); + } else if (currentTeam && inScheduledPosts) { + document.title = formatMessage({ + id: 'scheduledPosts.title', + defaultMessage: '{prefix}Scheduled - {displayName} {siteName}', + }, { + prefix: `${mentionTitle}${unreadTitle}`, + displayName: currentTeam.display_name, + siteName: currentSiteName, + }); } else { document.title = formatMessage({id: 'sidebar.team_select', defaultMessage: '{siteName} - Join a team'}, {siteName: currentSiteName || 'Mattermost'}); } diff --git a/webapp/channels/src/i18n/en.json b/webapp/channels/src/i18n/en.json index b1eb312626e..9eebb9f0739 100644 --- a/webapp/channels/src/i18n/en.json +++ b/webapp/channels/src/i18n/en.json @@ -4934,6 +4934,7 @@ "scheduled_posts.row_title_channel.placeholder": "In: {icon} No Destination", "scheduled_posts.row_title_thread.placeholder": "Thread to: {icon} No Destination", "scheduled_posts.row_title_thread.placeholder_tooltip": "The channel either doesn’t exist or you do not have access to it.", + "scheduledPosts.title": "{prefix}Scheduled - {displayName} {siteName}", "search_bar.channels": "Channels", "search_bar.clear": "Clear", "search_bar.file_types": "File types", From 5ba80d51ae2300a3f937e417739aa5a8ed2c113f Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Thu, 13 Feb 2025 12:12:51 +0100 Subject: [PATCH 2/7] Updates the property field and value update methods to use a single query for multiple entities (#30198) * Updates the property field and value update methods to use a single query for multiple entities * Update server/channels/store/sqlstore/property_field_store.go Co-authored-by: Julien Tant <785518+JulienTant@users.noreply.github.com> --------- Co-authored-by: Miguel de la Cruz (aider) Co-authored-by: Julien Tant <785518+JulienTant@users.noreply.github.com> --- .../store/sqlstore/property_field_store.go | 80 ++++++++++++------- .../store/sqlstore/property_value_store.go | 63 ++++++++------- .../store/storetest/property_field_store.go | 43 +++++++++- .../store/storetest/property_value_store.go | 44 +++++++++- 4 files changed, 169 insertions(+), 61 deletions(-) diff --git a/server/channels/store/sqlstore/property_field_store.go b/server/channels/store/sqlstore/property_field_store.go index c434f1702b5..1f10ec84757 100644 --- a/server/channels/store/sqlstore/property_field_store.go +++ b/server/channels/store/sqlstore/property_field_store.go @@ -128,44 +128,66 @@ func (s *SqlPropertyFieldStore) Update(fields []*model.PropertyField) (_ []*mode defer finalizeTransactionX(transaction, &err) updateTime := model.GetMillis() - for _, field := range fields { - field.UpdateAt = updateTime + isPostgres := s.DriverName() == model.DatabaseDriverPostgres + nameCase := sq.Case("id") + typeCase := sq.Case("id") + attrsCase := sq.Case("id") + targetIDCase := sq.Case("id") + targetTypeCase := sq.Case("id") + deleteAtCase := sq.Case("id") + ids := make([]string, len(fields)) + for i, field := range fields { + field.UpdateAt = updateTime if vErr := field.IsValid(); vErr != nil { return nil, errors.Wrap(vErr, "property_field_update_isvalid") } - queryString, args, err := s.getQueryBuilder(). - Update("PropertyFields"). - Set("Name", field.Name). - Set("Type", field.Type). - Set("Attrs", field.Attrs). - Set("TargetID", field.TargetID). - Set("TargetType", field.TargetType). - Set("UpdateAt", field.UpdateAt). - Set("DeleteAt", field.DeleteAt). - Where(sq.Eq{"id": field.ID}). - ToSql() - if err != nil { - return nil, errors.Wrap(err, "property_field_update_tosql") - } - - result, err := transaction.Exec(queryString, args...) - if err != nil { - return nil, errors.Wrapf(err, "failed to update property field with id: %s", field.ID) - } - - count, err := result.RowsAffected() - if err != nil { - return nil, errors.Wrap(err, "property_field_update_rowsaffected") - } - if count == 0 { - return nil, store.NewErrNotFound("PropertyField", field.ID) + ids[i] = field.ID + whenID := sq.Expr("?", field.ID) + if isPostgres { + nameCase = nameCase.When(whenID, sq.Expr("?::text", field.Name)) + typeCase = typeCase.When(whenID, sq.Expr("?::property_field_type", field.Type)) + attrsCase = attrsCase.When(whenID, sq.Expr("?::jsonb", field.Attrs)) + targetIDCase = targetIDCase.When(whenID, sq.Expr("?::text", field.TargetID)) + targetTypeCase = targetTypeCase.When(whenID, sq.Expr("?::text", field.TargetType)) + deleteAtCase = deleteAtCase.When(whenID, sq.Expr("?::bigint", field.DeleteAt)) + } else { + nameCase = nameCase.When(whenID, sq.Expr("?", field.Name)) + typeCase = typeCase.When(whenID, sq.Expr("?", field.Type)) + attrsCase = attrsCase.When(whenID, sq.Expr("?", field.Attrs)) + targetIDCase = targetIDCase.When(whenID, sq.Expr("?", field.TargetID)) + targetTypeCase = targetTypeCase.When(whenID, sq.Expr("?", field.TargetType)) + deleteAtCase = deleteAtCase.When(whenID, sq.Expr("?", field.DeleteAt)) } } + builder := s.getQueryBuilder(). + Update("PropertyFields"). + Set("Name", nameCase). + Set("Type", typeCase). + Set("Attrs", attrsCase). + Set("TargetID", targetIDCase). + Set("TargetType", targetTypeCase). + Set("UpdateAt", updateTime). + Set("DeleteAt", deleteAtCase). + Where(sq.Eq{"id": ids}) + + result, err := transaction.ExecBuilder(builder) + if err != nil { + return nil, errors.Wrap(err, "property_field_update_exec") + } + + count, err := result.RowsAffected() + if err != nil { + return nil, errors.Wrap(err, "property_field_update_rowsaffected") + } + if count != int64(len(fields)) { + return nil, errors.Errorf("failed to update, some property fields were not found, got %d of %d", count, len(fields)) + } + if err := transaction.Commit(); err != nil { - return nil, errors.Wrap(err, "property_field_update_commit") + return nil, errors.Wrap(err, "property_field_update_commit_transaction") } return fields, nil diff --git a/server/channels/store/sqlstore/property_value_store.go b/server/channels/store/sqlstore/property_value_store.go index 80c686aa0db..1e39cb0c79c 100644 --- a/server/channels/store/sqlstore/property_value_store.go +++ b/server/channels/store/sqlstore/property_value_store.go @@ -136,45 +136,54 @@ func (s *SqlPropertyValueStore) Update(values []*model.PropertyValue) (_ []*mode defer finalizeTransactionX(transaction, &err) updateTime := model.GetMillis() - for _, value := range values { - value.UpdateAt = updateTime + isPostgres := s.DriverName() == model.DatabaseDriverPostgres + valueCase := sq.Case("id") + deleteAtCase := sq.Case("id") + ids := make([]string, len(values)) - if err := value.IsValid(); err != nil { - return nil, errors.Wrap(err, "property_value_update_isvalid") + for i, value := range values { + value.UpdateAt = updateTime + if vErr := value.IsValid(); vErr != nil { + return nil, errors.Wrap(vErr, "property_value_update_isvalid") } + ids[i] = value.ID valueJSON := value.Value if s.IsBinaryParamEnabled() { valueJSON = AppendBinaryFlag(valueJSON) } - queryString, args, err := s.getQueryBuilder(). - Update("PropertyValues"). - Set("Value", valueJSON). - Set("UpdateAt", value.UpdateAt). - Set("DeleteAt", value.DeleteAt). - Where(sq.Eq{"id": value.ID}). - ToSql() - if err != nil { - return nil, errors.Wrap(err, "property_value_update_tosql") - } - - result, err := transaction.Exec(queryString, args...) - if err != nil { - return nil, errors.Wrapf(err, "failed to update property value with id: %s", value.ID) - } - - count, err := result.RowsAffected() - if err != nil { - return nil, errors.Wrap(err, "property_value_update_rowsaffected") - } - if count == 0 { - return nil, store.NewErrNotFound("PropertyValue", value.ID) + if isPostgres { + valueCase = valueCase.When(sq.Expr("?", value.ID), sq.Expr("?::jsonb", valueJSON)) + deleteAtCase = deleteAtCase.When(sq.Expr("?", value.ID), sq.Expr("?::bigint", value.DeleteAt)) + } else { + valueCase = valueCase.When(sq.Expr("?", value.ID), sq.Expr("?", valueJSON)) + deleteAtCase = deleteAtCase.When(sq.Expr("?", value.ID), sq.Expr("?", value.DeleteAt)) } } + builder := s.getQueryBuilder(). + Update("PropertyValues"). + Set("Value", valueCase). + Set("DeleteAt", deleteAtCase). + Set("UpdateAt", updateTime). + Where(sq.Eq{"id": ids}) + + result, err := transaction.ExecBuilder(builder) + if err != nil { + return nil, errors.Wrap(err, "property_value_update_exec") + } + + count, err := result.RowsAffected() + if err != nil { + return nil, errors.Wrap(err, "property_value_update_rowsaffected") + } + if count != int64(len(values)) { + return nil, errors.Errorf("failed to update, some property values were not found, got %d of %d", count, len(values)) + } + if err := transaction.Commit(); err != nil { - return nil, errors.Wrap(err, "property_value_update_commit") + return nil, errors.Wrap(err, "property_value_update_commit_transaction") } return values, nil diff --git a/server/channels/store/storetest/property_field_store.go b/server/channels/store/storetest/property_field_store.go index a2880c21424..19fe3d68012 100644 --- a/server/channels/store/storetest/property_field_store.go +++ b/server/channels/store/storetest/property_field_store.go @@ -146,8 +146,7 @@ func testUpdatePropertyField(t *testing.T, _ request.CTX, ss store.Store) { } updatedField, err := ss.PropertyField().Update([]*model.PropertyField{field}) require.Zero(t, updatedField) - var enf *store.ErrNotFound - require.ErrorAs(t, err, &enf) + require.ErrorContains(t, err, "failed to update, some property fields were not found, got 0 of 1") }) t.Run("should fail if the property field is not valid", func(t *testing.T) { @@ -280,6 +279,46 @@ func testUpdatePropertyField(t *testing.T, _ request.CTX, ss store.Store) { require.Equal(t, groupID, updated2.GroupID) require.Equal(t, originalUpdateAt2, updated2.UpdateAt) }) + + t.Run("should not update any fields if one update points to a nonexisting one", func(t *testing.T) { + // Create a valid field + field1 := &model.PropertyField{ + GroupID: model.NewId(), + Name: "First field", + Type: model.PropertyFieldTypeText, + } + + _, err := ss.PropertyField().Create(field1) + require.NoError(t, err) + + originalUpdateAt := field1.UpdateAt + + // Try to update both the valid field and a nonexistent one + field2 := &model.PropertyField{ + ID: model.NewId(), + GroupID: model.NewId(), + Name: "Second field", + Type: model.PropertyFieldTypeText, + TargetID: model.NewId(), + TargetType: "test_type", + CreateAt: 1, + Attrs: map[string]any{ + "key": "value", + }, + } + + field1.Name = "Updated First" + + _, err = ss.PropertyField().Update([]*model.PropertyField{field1, field2}) + require.Error(t, err) + require.ErrorContains(t, err, "failed to update, some property fields were not found") + + // Check that the valid field was not updated + updated1, err := ss.PropertyField().Get(field1.ID) + require.NoError(t, err) + require.Equal(t, "First field", updated1.Name) + require.Equal(t, originalUpdateAt, updated1.UpdateAt) + }) } func testDeletePropertyField(t *testing.T, _ request.CTX, ss store.Store) { diff --git a/server/channels/store/storetest/property_value_store.go b/server/channels/store/storetest/property_value_store.go index 271f1500d04..41a4ba3cf0f 100644 --- a/server/channels/store/storetest/property_value_store.go +++ b/server/channels/store/storetest/property_value_store.go @@ -149,8 +149,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { } updatedValue, err := ss.PropertyValue().Update([]*model.PropertyValue{value}) require.Zero(t, updatedValue) - var enf *store.ErrNotFound - require.ErrorAs(t, err, &enf) + require.ErrorContains(t, err, "failed to update, some property values were not found, got 0 of 1") }) t.Run("should fail if the property value is not valid", func(t *testing.T) { @@ -220,7 +219,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.Greater(t, updated2.UpdateAt, updated2.CreateAt) }) - t.Run("should not update any fields if one update is invalid", func(t *testing.T) { + t.Run("should not update any values if one update is invalid", func(t *testing.T) { // Create two valid values groupID := model.NewId() value1 := &model.PropertyValue{ @@ -266,6 +265,45 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.Equal(t, groupID, updated2.GroupID) require.Equal(t, originalUpdateAt2, updated2.UpdateAt) }) + + t.Run("should not update any values if one update points to a nonexisting one", func(t *testing.T) { + // Create a valid value + value1 := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: model.NewId(), + FieldID: model.NewId(), + Value: json.RawMessage(`"Value 1"`), + } + + _, err := ss.PropertyValue().Create(value1) + require.NoError(t, err) + + originalUpdateAt := value1.UpdateAt + + // Try to update both the valid value and a nonexistent one + value2 := &model.PropertyValue{ + ID: model.NewId(), + TargetID: model.NewId(), + CreateAt: 1, + TargetType: "test_type", + GroupID: model.NewId(), + FieldID: model.NewId(), + Value: json.RawMessage(`"Value 2"`), + } + + value1.Value = json.RawMessage(`"Updated Value 1"`) + + _, err = ss.PropertyValue().Update([]*model.PropertyValue{value1, value2}) + require.Error(t, err) + require.ErrorContains(t, err, "failed to update, some property values were not found") + + // Check that the valid value was not updated + updated1, err := ss.PropertyValue().Get(value1.ID) + require.NoError(t, err) + require.Equal(t, json.RawMessage(`"Value 1"`), updated1.Value) + require.Equal(t, originalUpdateAt, updated1.UpdateAt) + }) } func testDeletePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { From f85a8c61a49fe590ed5c9f1e3a068973b1593265 Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Thu, 13 Feb 2025 12:21:46 +0100 Subject: [PATCH 3/7] Adds websocket messages to Custom Profile Attributes (#30163) * Adds websocket messages to Custom Profile Attributes The app layer now fires a websocket event as part of the operations over Custom Profile Attribute fields and values. It updates as well the Patch method for CPA values so all the changes are commited as part of the same transaction. To be able to do this last operation, the change adds methods to upsert CPA values in both the store and the property service. * Fix i18n strings --------- Co-authored-by: Mattermost Build Co-authored-by: Miguel de la Cruz --- .../api4/custom_profile_attributes_test.go | 92 ++++++++++ .../channels/app/custom_profile_attributes.go | 79 +++++---- .../channels/app/properties/property_value.go | 13 ++ .../channels/store/retrylayer/retrylayer.go | 29 ++- .../store/sqlstore/property_value_store.go | 91 ++++++++++ server/channels/store/store.go | 5 +- .../storetest/mocks/PropertyFieldStore.go | 12 +- .../storetest/mocks/PropertyValueStore.go | 42 ++++- .../store/storetest/property_value_store.go | 165 ++++++++++++++++++ .../channels/store/timerlayer/timerlayer.go | 24 ++- server/i18n/en.json | 12 +- server/public/model/websocket_message.go | 4 + 12 files changed, 505 insertions(+), 63 deletions(-) diff --git a/server/channels/api4/custom_profile_attributes_test.go b/server/channels/api4/custom_profile_attributes_test.go index 37e278bdfb5..de8daafc5ce 100644 --- a/server/channels/api4/custom_profile_attributes_test.go +++ b/server/channels/api4/custom_profile_attributes_test.go @@ -9,6 +9,7 @@ import ( "fmt" "os" "testing" + "time" "github.com/mattermost/mattermost/server/public/model" "github.com/stretchr/testify/require" @@ -54,6 +55,8 @@ func TestCreateCPAField(t *testing.T) { }, "an invalid field should be rejected") th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) { + webSocketClient := th.CreateConnectedWebSocketClient(t) + name := model.NewId() field := &model.PropertyField{ Name: fmt.Sprintf(" %s\t", name), // name should be sanitized @@ -67,6 +70,27 @@ func TestCreateCPAField(t *testing.T) { require.NotZero(t, createdField.ID) require.Equal(t, name, createdField.Name) require.Equal(t, "default", createdField.Attrs["visibility"]) + + t.Run("a websocket event should be fired as part of the field creation", func(t *testing.T) { + var wsField model.PropertyField + require.Eventually(t, func() bool { + select { + case event := <-webSocketClient.EventChannel: + if event.EventType() == model.WebsocketEventCPAFieldCreated { + fieldData, err := json.Marshal(event.GetData()["field"]) + require.NoError(t, err) + require.NoError(t, json.Unmarshal(fieldData, &wsField)) + return true + } + default: + return false + } + return false + }, 5*time.Second, 100*time.Millisecond) + + require.NotEmpty(t, wsField.ID) + require.Equal(t, createdField, &wsField) + }) }, "a user with admin permissions should be able to create the field") } @@ -149,6 +173,8 @@ func TestPatchCPAField(t *testing.T) { }) th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) { + webSocketClient := th.CreateConnectedWebSocketClient(t) + field := &model.PropertyField{ Name: model.NewId(), Type: model.PropertyFieldTypeText, @@ -163,6 +189,27 @@ func TestPatchCPAField(t *testing.T) { CheckOKStatus(t, resp) require.NoError(t, err) require.Equal(t, newName, patchedField.Name) + + t.Run("a websocket event should be fired as part of the field patch", func(t *testing.T) { + var wsField model.PropertyField + require.Eventually(t, func() bool { + select { + case event := <-webSocketClient.EventChannel: + if event.EventType() == model.WebsocketEventCPAFieldUpdated { + fieldData, err := json.Marshal(event.GetData()["field"]) + require.NoError(t, err) + require.NoError(t, json.Unmarshal(fieldData, &wsField)) + return true + } + default: + return false + } + return false + }, 5*time.Second, 100*time.Millisecond) + + require.NotEmpty(t, wsField.ID) + require.Equal(t, patchedField, &wsField) + }) }, "a user with admin permissions should be able to patch the field") } @@ -197,6 +244,8 @@ func TestDeleteCPAField(t *testing.T) { }) th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) { + webSocketClient := th.CreateConnectedWebSocketClient(t) + field := &model.PropertyField{ Name: model.NewId(), Type: model.PropertyFieldTypeText, @@ -213,6 +262,26 @@ func TestDeleteCPAField(t *testing.T) { deletedField, appErr := th.App.GetCPAField(createdField.ID) require.Nil(t, appErr) require.NotZero(t, deletedField.DeleteAt) + + t.Run("a websocket event should be fired as part of the field deletion", func(t *testing.T) { + var fieldID string + require.Eventually(t, func() bool { + select { + case event := <-webSocketClient.EventChannel: + if event.EventType() == model.WebsocketEventCPAFieldDeleted { + var ok bool + fieldID, ok = event.GetData()["field_id"].(string) + require.True(t, ok) + return true + } + default: + return false + } + return false + }, 5*time.Second, 100*time.Millisecond) + + require.Equal(t, createdField.ID, fieldID) + }) }, "a user with admin permissions should be able to delete the field") } @@ -470,6 +539,8 @@ func TestPatchCPAValues(t *testing.T) { th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) t.Run("any team member should be able to create their own values", func(t *testing.T) { + webSocketClient := th.CreateConnectedWebSocketClient(t) + values := map[string]json.RawMessage{} value := "Field Value" values[createdField.ID] = json.RawMessage(fmt.Sprintf(`" %s "`, value)) // value should be sanitized @@ -490,6 +561,27 @@ func TestPatchCPAValues(t *testing.T) { actualValue = "" require.NoError(t, json.Unmarshal(values[createdField.ID], &actualValue)) require.Equal(t, value, actualValue) + + t.Run("a websocket event should be fired as part of the value changes", func(t *testing.T) { + var wsValues map[string]json.RawMessage + require.Eventually(t, func() bool { + select { + case event := <-webSocketClient.EventChannel: + if event.EventType() == model.WebsocketEventCPAValuesUpdated { + valuesData, err := json.Marshal(event.GetData()["values"]) + require.NoError(t, err) + require.NoError(t, json.Unmarshal(valuesData, &wsValues)) + return true + } + default: + return false + } + return false + }, 5*time.Second, 100*time.Millisecond) + + require.NotEmpty(t, wsValues) + require.Equal(t, patchedValues, wsValues) + }) }) t.Run("any team member should be able to patch their own values", func(t *testing.T) { diff --git a/server/channels/app/custom_profile_attributes.go b/server/channels/app/custom_profile_attributes.go index a3cd1fafed1..caf9ee23fba 100644 --- a/server/channels/app/custom_profile_attributes.go +++ b/server/channels/app/custom_profile_attributes.go @@ -97,6 +97,10 @@ func (a *App) CreateCPAField(field *model.PropertyField) (*model.PropertyField, } } + message := model.NewWebSocketEvent(model.WebsocketEventCPAFieldCreated, "", "", "", nil, "") + message.Add("field", newField) + a.Publish(message) + return newField, nil } @@ -122,6 +126,10 @@ func (a *App) PatchCPAField(fieldID string, patch *model.PropertyFieldPatch) (*m } } + message := model.NewWebSocketEvent(model.WebsocketEventCPAFieldUpdated, "", "", "", nil, "") + message.Add("field", patchedField) + a.Publish(message) + return patchedField, nil } @@ -150,6 +158,10 @@ func (a *App) DeleteCPAField(id string) *model.AppError { } } + message := model.NewWebSocketEvent(model.WebsocketEventCPAFieldDeleted, "", "", "", nil, "") + message.Add("field_id", id) + a.Publish(message) + return nil } @@ -193,49 +205,54 @@ func (a *App) GetCPAValue(valueID string) (*model.PropertyValue, *model.AppError } func (a *App) PatchCPAValue(userID string, fieldID string, value json.RawMessage) (*model.PropertyValue, *model.AppError) { + values, appErr := a.PatchCPAValues(userID, map[string]json.RawMessage{fieldID: value}) + if appErr != nil { + return nil, appErr + } + + return values[0], nil +} + +func (a *App) PatchCPAValues(userID string, fieldValueMap map[string]json.RawMessage) ([]*model.PropertyValue, *model.AppError) { groupID, err := a.cpaGroupID() if err != nil { return nil, model.NewAppError("PatchCPAValues", "app.custom_profile_attributes.cpa_group_id.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - // make sure field exists in this group - existingField, appErr := a.GetCPAField(fieldID) - if appErr != nil { - return nil, model.NewAppError("PatchCPAValue", "app.custom_profile_attributes.property_field_not_found.app_error", nil, "", http.StatusNotFound).Wrap(appErr) - } else if existingField.DeleteAt > 0 { - return nil, model.NewAppError("PatchCPAValue", "app.custom_profile_attributes.property_field_not_found.app_error", nil, "", http.StatusNotFound) - } - - existingValues, appErr := a.ListCPAValues(userID) - if appErr != nil { - return nil, model.NewAppError("PatchCPAValue", "app.custom_profile_attributes.property_value_list.app_error", nil, "", http.StatusNotFound).Wrap(err) - } - var existingValue *model.PropertyValue - for key, value := range existingValues { - if value.FieldID == fieldID { - existingValue = existingValues[key] - break + valuesToUpdate := []*model.PropertyValue{} + for fieldID, value := range fieldValueMap { + // make sure field exists in this group + existingField, appErr := a.GetCPAField(fieldID) + if appErr != nil { + return nil, model.NewAppError("PatchCPAValue", "app.custom_profile_attributes.property_field_not_found.app_error", nil, "", http.StatusNotFound).Wrap(appErr) + } else if existingField.DeleteAt > 0 { + return nil, model.NewAppError("PatchCPAValue", "app.custom_profile_attributes.property_field_not_found.app_error", nil, "", http.StatusNotFound) } - } - if existingValue != nil { - existingValue.Value = value - _, err = a.ch.srv.propertyService.UpdatePropertyValue(existingValue) - if err != nil { - return nil, model.NewAppError("PatchCPAValue", "app.custom_profile_attributes.property_value_update.app_error", nil, "", http.StatusInternalServerError).Wrap(err) - } - } else { - propertyValue := &model.PropertyValue{ + value := &model.PropertyValue{ GroupID: groupID, TargetType: "user", TargetID: userID, FieldID: fieldID, Value: value, } - existingValue, err = a.ch.srv.propertyService.CreatePropertyValue(propertyValue) - if err != nil { - return nil, model.NewAppError("PatchCPAValue", "app.custom_profile_attributes.property_value_creation.app_error", nil, "", http.StatusInternalServerError).Wrap(err) - } + valuesToUpdate = append(valuesToUpdate, value) } - return existingValue, nil + + updatedValues, err := a.Srv().propertyService.UpsertPropertyValues(valuesToUpdate) + if err != nil { + return nil, model.NewAppError("PatchCPAValues", "app.custom_profile_attributes.property_value_upsert.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + + updatedFieldValueMap := map[string]json.RawMessage{} + for _, value := range updatedValues { + updatedFieldValueMap[value.FieldID] = value.Value + } + + message := model.NewWebSocketEvent(model.WebsocketEventCPAValuesUpdated, "", "", "", nil, "") + message.Add("user_id", userID) + message.Add("values", updatedFieldValueMap) + a.Publish(message) + + return updatedValues, nil } diff --git a/server/channels/app/properties/property_value.go b/server/channels/app/properties/property_value.go index 7082ace2d78..dbb79afed9a 100644 --- a/server/channels/app/properties/property_value.go +++ b/server/channels/app/properties/property_value.go @@ -36,6 +36,19 @@ func (ps *PropertyService) UpdatePropertyValues(values []*model.PropertyValue) ( return ps.valueStore.Update(values) } +func (ps *PropertyService) UpsertPropertyValue(value *model.PropertyValue) (*model.PropertyValue, error) { + values, err := ps.UpsertPropertyValues([]*model.PropertyValue{value}) + if err != nil { + return nil, err + } + + return values[0], nil +} + +func (ps *PropertyService) UpsertPropertyValues(values []*model.PropertyValue) ([]*model.PropertyValue, error) { + return ps.valueStore.Upsert(values) +} + func (ps *PropertyService) DeletePropertyValue(id string) error { return ps.valueStore.Delete(id) } diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index cd629956cc1..a6f738d80b9 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -9054,11 +9054,11 @@ func (s *RetryLayerPropertyFieldStore) SearchPropertyFields(opts model.PropertyF } -func (s *RetryLayerPropertyFieldStore) Update(field []*model.PropertyField) ([]*model.PropertyField, error) { +func (s *RetryLayerPropertyFieldStore) Update(fields []*model.PropertyField) ([]*model.PropertyField, error) { tries := 0 for { - result, err := s.PropertyFieldStore.Update(field) + result, err := s.PropertyFieldStore.Update(fields) if err == nil { return result, nil } @@ -9243,11 +9243,32 @@ func (s *RetryLayerPropertyValueStore) SearchPropertyValues(opts model.PropertyV } -func (s *RetryLayerPropertyValueStore) Update(field []*model.PropertyValue) ([]*model.PropertyValue, error) { +func (s *RetryLayerPropertyValueStore) Update(values []*model.PropertyValue) ([]*model.PropertyValue, error) { tries := 0 for { - result, err := s.PropertyValueStore.Update(field) + result, err := s.PropertyValueStore.Update(values) + if err == nil { + return result, nil + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + +func (s *RetryLayerPropertyValueStore) Upsert(values []*model.PropertyValue) ([]*model.PropertyValue, error) { + + tries := 0 + for { + result, err := s.PropertyValueStore.Upsert(values) if err == nil { return result, nil } diff --git a/server/channels/store/sqlstore/property_value_store.go b/server/channels/store/sqlstore/property_value_store.go index 1e39cb0c79c..d9d12603a32 100644 --- a/server/channels/store/sqlstore/property_value_store.go +++ b/server/channels/store/sqlstore/property_value_store.go @@ -189,6 +189,97 @@ func (s *SqlPropertyValueStore) Update(values []*model.PropertyValue) (_ []*mode return values, nil } +func (s *SqlPropertyValueStore) Upsert(values []*model.PropertyValue) (_ []*model.PropertyValue, err error) { + if len(values) == 0 { + return nil, nil + } + + transaction, err := s.GetMaster().Beginx() + if err != nil { + return nil, errors.Wrap(err, "property_value_upsert_begin_transaction") + } + defer finalizeTransactionX(transaction, &err) + + updatedValues := make([]*model.PropertyValue, len(values)) + updateTime := model.GetMillis() + for i, value := range values { + value.PreSave() + value.UpdateAt = updateTime + + if err := value.IsValid(); err != nil { + return nil, errors.Wrap(err, "property_value_upsert_isvalid") + } + + valueJSON := value.Value + if s.IsBinaryParamEnabled() { + valueJSON = AppendBinaryFlag(valueJSON) + } + + builder := s.getQueryBuilder(). + Insert("PropertyValues"). + Columns("ID", "TargetID", "TargetType", "GroupID", "FieldID", "Value", "CreateAt", "UpdateAt", "DeleteAt"). + Values(value.ID, value.TargetID, value.TargetType, value.GroupID, value.FieldID, valueJSON, value.CreateAt, value.UpdateAt, value.DeleteAt) + + if s.DriverName() == model.DatabaseDriverMysql { + builder = builder.SuffixExpr(sq.Expr( + "ON DUPLICATE KEY UPDATE Value = ?, UpdateAt = ?, DeleteAt = ?", + valueJSON, + value.UpdateAt, + 0, + )) + + if _, err := transaction.ExecBuilder(builder); err != nil { + return nil, errors.Wrap(err, "property_value_upsert_exec") + } + + // MySQL doesn't support RETURNING, so we need to fetch + // the new field to get its ID in case we hit a DUPLICATED + // KEY and the value.ID we have is not the right one + gBuilder := s.tableSelectQuery.Where(sq.Eq{ + "GroupID": value.GroupID, + "TargetID": value.TargetID, + "FieldID": value.FieldID, + "DeleteAt": 0, + }) + + var values []*model.PropertyValue + if gErr := transaction.SelectBuilder(&values, gBuilder); gErr != nil { + return nil, errors.Wrap(gErr, "property_value_upsert_select") + } + + if len(values) != 1 { + return nil, errors.New("property_value_upsert_select_length") + } + + updatedValues[i] = values[0] + } else { + builder = builder.SuffixExpr(sq.Expr( + "ON CONFLICT (GroupID, TargetID, FieldID) WHERE DeleteAt = 0 DO UPDATE SET Value = ?, UpdateAt = ?, DeleteAt = ? RETURNING *", + valueJSON, + value.UpdateAt, + 0, + )) + + var values []*model.PropertyValue + if err := transaction.SelectBuilder(&values, builder); err != nil { + return nil, errors.Wrapf(err, "failed to upsert property value with id: %s", value.ID) + } + + if len(values) != 1 { + return nil, errors.New("property_value_upsert_select_length") + } + + updatedValues[i] = values[0] + } + } + + if err := transaction.Commit(); err != nil { + return nil, errors.Wrap(err, "property_value_upsert_commit") + } + + return updatedValues, nil +} + func (s *SqlPropertyValueStore) Delete(id string) error { builder := s.getQueryBuilder(). Update("PropertyValues"). diff --git a/server/channels/store/store.go b/server/channels/store/store.go index 4683e52aea1..88f2d29fe02 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -1090,7 +1090,7 @@ type PropertyFieldStore interface { Get(id string) (*model.PropertyField, error) GetMany(ids []string) ([]*model.PropertyField, error) SearchPropertyFields(opts model.PropertyFieldSearchOpts) ([]*model.PropertyField, error) - Update(field []*model.PropertyField) ([]*model.PropertyField, error) + Update(fields []*model.PropertyField) ([]*model.PropertyField, error) Delete(id string) error } @@ -1099,7 +1099,8 @@ type PropertyValueStore interface { Get(id string) (*model.PropertyValue, error) GetMany(ids []string) ([]*model.PropertyValue, error) SearchPropertyValues(opts model.PropertyValueSearchOpts) ([]*model.PropertyValue, error) - Update(field []*model.PropertyValue) ([]*model.PropertyValue, error) + Update(values []*model.PropertyValue) ([]*model.PropertyValue, error) + Upsert(values []*model.PropertyValue) ([]*model.PropertyValue, error) Delete(id string) error DeleteForField(id string) error } diff --git a/server/channels/store/storetest/mocks/PropertyFieldStore.go b/server/channels/store/storetest/mocks/PropertyFieldStore.go index 4afd0e81ffb..b32b267da7e 100644 --- a/server/channels/store/storetest/mocks/PropertyFieldStore.go +++ b/server/channels/store/storetest/mocks/PropertyFieldStore.go @@ -152,9 +152,9 @@ func (_m *PropertyFieldStore) SearchPropertyFields(opts model.PropertyFieldSearc return r0, r1 } -// Update provides a mock function with given fields: field -func (_m *PropertyFieldStore) Update(field []*model.PropertyField) ([]*model.PropertyField, error) { - ret := _m.Called(field) +// Update provides a mock function with given fields: fields +func (_m *PropertyFieldStore) Update(fields []*model.PropertyField) ([]*model.PropertyField, error) { + ret := _m.Called(fields) if len(ret) == 0 { panic("no return value specified for Update") @@ -163,10 +163,10 @@ func (_m *PropertyFieldStore) Update(field []*model.PropertyField) ([]*model.Pro var r0 []*model.PropertyField var r1 error if rf, ok := ret.Get(0).(func([]*model.PropertyField) ([]*model.PropertyField, error)); ok { - return rf(field) + return rf(fields) } if rf, ok := ret.Get(0).(func([]*model.PropertyField) []*model.PropertyField); ok { - r0 = rf(field) + r0 = rf(fields) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]*model.PropertyField) @@ -174,7 +174,7 @@ func (_m *PropertyFieldStore) Update(field []*model.PropertyField) ([]*model.Pro } if rf, ok := ret.Get(1).(func([]*model.PropertyField) error); ok { - r1 = rf(field) + r1 = rf(fields) } else { r1 = ret.Error(1) } diff --git a/server/channels/store/storetest/mocks/PropertyValueStore.go b/server/channels/store/storetest/mocks/PropertyValueStore.go index d5b3827ab81..0218927b435 100644 --- a/server/channels/store/storetest/mocks/PropertyValueStore.go +++ b/server/channels/store/storetest/mocks/PropertyValueStore.go @@ -170,9 +170,9 @@ func (_m *PropertyValueStore) SearchPropertyValues(opts model.PropertyValueSearc return r0, r1 } -// Update provides a mock function with given fields: field -func (_m *PropertyValueStore) Update(field []*model.PropertyValue) ([]*model.PropertyValue, error) { - ret := _m.Called(field) +// Update provides a mock function with given fields: values +func (_m *PropertyValueStore) Update(values []*model.PropertyValue) ([]*model.PropertyValue, error) { + ret := _m.Called(values) if len(ret) == 0 { panic("no return value specified for Update") @@ -181,10 +181,10 @@ func (_m *PropertyValueStore) Update(field []*model.PropertyValue) ([]*model.Pro var r0 []*model.PropertyValue var r1 error if rf, ok := ret.Get(0).(func([]*model.PropertyValue) ([]*model.PropertyValue, error)); ok { - return rf(field) + return rf(values) } if rf, ok := ret.Get(0).(func([]*model.PropertyValue) []*model.PropertyValue); ok { - r0 = rf(field) + r0 = rf(values) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]*model.PropertyValue) @@ -192,7 +192,37 @@ func (_m *PropertyValueStore) Update(field []*model.PropertyValue) ([]*model.Pro } if rf, ok := ret.Get(1).(func([]*model.PropertyValue) error); ok { - r1 = rf(field) + r1 = rf(values) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// Upsert provides a mock function with given fields: values +func (_m *PropertyValueStore) Upsert(values []*model.PropertyValue) ([]*model.PropertyValue, error) { + ret := _m.Called(values) + + if len(ret) == 0 { + panic("no return value specified for Upsert") + } + + var r0 []*model.PropertyValue + var r1 error + if rf, ok := ret.Get(0).(func([]*model.PropertyValue) ([]*model.PropertyValue, error)); ok { + return rf(values) + } + if rf, ok := ret.Get(0).(func([]*model.PropertyValue) []*model.PropertyValue); ok { + r0 = rf(values) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*model.PropertyValue) + } + } + + if rf, ok := ret.Get(1).(func([]*model.PropertyValue) error); ok { + r1 = rf(values) } else { r1 = ret.Error(1) } diff --git a/server/channels/store/storetest/property_value_store.go b/server/channels/store/storetest/property_value_store.go index 41a4ba3cf0f..b360380b627 100644 --- a/server/channels/store/storetest/property_value_store.go +++ b/server/channels/store/storetest/property_value_store.go @@ -22,6 +22,7 @@ func TestPropertyValueStore(t *testing.T, rctx request.CTX, ss store.Store, s Sq t.Run("GetPropertyValue", func(t *testing.T) { testGetPropertyValue(t, rctx, ss) }) t.Run("GetManyPropertyValues", func(t *testing.T) { testGetManyPropertyValues(t, rctx, ss) }) t.Run("UpdatePropertyValue", func(t *testing.T) { testUpdatePropertyValue(t, rctx, ss) }) + t.Run("UpsertPropertyValue", func(t *testing.T) { testUpsertPropertyValue(t, rctx, ss) }) t.Run("DeletePropertyValue", func(t *testing.T) { testDeletePropertyValue(t, rctx, ss) }) t.Run("SearchPropertyValues", func(t *testing.T) { testSearchPropertyValues(t, rctx, ss) }) t.Run("DeleteForField", func(t *testing.T) { testDeleteForField(t, rctx, ss) }) @@ -306,6 +307,170 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { }) } +func testUpsertPropertyValue(t *testing.T, _ request.CTX, ss store.Store) { + t.Run("should fail if the property value is not valid", func(t *testing.T) { + value := &model.PropertyValue{ + TargetID: "", + TargetType: "test_type", + GroupID: model.NewId(), + FieldID: model.NewId(), + Value: json.RawMessage(`"test value"`), + } + updatedValue, err := ss.PropertyValue().Upsert([]*model.PropertyValue{value}) + require.Zero(t, updatedValue) + require.ErrorContains(t, err, "model.property_value.is_valid.app_error") + + value.TargetID = model.NewId() + value.GroupID = "" + updatedValue, err = ss.PropertyValue().Upsert([]*model.PropertyValue{value}) + require.Zero(t, updatedValue) + require.ErrorContains(t, err, "model.property_value.is_valid.app_error") + }) + + t.Run("should be able to insert new property values", func(t *testing.T) { + value1 := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: model.NewId(), + FieldID: model.NewId(), + Value: json.RawMessage(`"value 1"`), + } + + value2 := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: model.NewId(), + FieldID: model.NewId(), + Value: json.RawMessage(`"value 2"`), + } + + values, err := ss.PropertyValue().Upsert([]*model.PropertyValue{value1, value2}) + require.NoError(t, err) + require.Len(t, values, 2) + require.NotEmpty(t, values[0].ID) + require.NotEmpty(t, values[1].ID) + require.NotZero(t, values[0].CreateAt) + require.NotZero(t, values[1].CreateAt) + + valuesFromStore, err := ss.PropertyValue().GetMany([]string{values[0].ID, values[1].ID}) + require.NoError(t, err) + require.Len(t, valuesFromStore, 2) + }) + + t.Run("should be able to update existing property values", func(t *testing.T) { + // Create initial value + value := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: model.NewId(), + FieldID: model.NewId(), + Value: json.RawMessage(`"initial value"`), + } + _, err := ss.PropertyValue().Create(value) + require.NoError(t, err) + valueID := value.ID + + time.Sleep(10 * time.Millisecond) + + // Update via upsert + value.ID = "" + value.Value = json.RawMessage(`"updated value"`) + values, err := ss.PropertyValue().Upsert([]*model.PropertyValue{value}) + require.NoError(t, err) + require.Len(t, values, 1) + require.Equal(t, valueID, values[0].ID) + require.Equal(t, json.RawMessage(`"updated value"`), values[0].Value) + require.Greater(t, values[0].UpdateAt, values[0].CreateAt) + + // Verify in database + updated, err := ss.PropertyValue().Get(valueID) + require.NoError(t, err) + require.Equal(t, json.RawMessage(`"updated value"`), updated.Value) + require.Greater(t, updated.UpdateAt, updated.CreateAt) + }) + + t.Run("should handle mixed insert and update operations", func(t *testing.T) { + // Create first value + existingValue := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: model.NewId(), + FieldID: model.NewId(), + Value: json.RawMessage(`"existing value"`), + } + _, err := ss.PropertyValue().Create(existingValue) + require.NoError(t, err) + + // Prepare new value + newValue := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: model.NewId(), + FieldID: model.NewId(), + Value: json.RawMessage(`"new value"`), + } + + // Update existing and insert new via upsert + existingValue.Value = json.RawMessage(`"updated existing"`) + values, err := ss.PropertyValue().Upsert([]*model.PropertyValue{existingValue, newValue}) + require.NoError(t, err) + require.Len(t, values, 2) + + // Verify both values + newValueUpserted, err := ss.PropertyValue().Get(newValue.ID) + require.NoError(t, err) + require.Equal(t, json.RawMessage(`"new value"`), newValueUpserted.Value) + existingValueUpserted, err := ss.PropertyValue().Get(existingValue.ID) + require.NoError(t, err) + require.Equal(t, json.RawMessage(`"updated existing"`), existingValueUpserted.Value) + }) + + t.Run("should not perform any operation if one of the fields is invalid", func(t *testing.T) { + // Create initial valid value + existingValue := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: model.NewId(), + FieldID: model.NewId(), + Value: json.RawMessage(`"existing value"`), + } + _, err := ss.PropertyValue().Create(existingValue) + require.NoError(t, err) + + originalValue := *existingValue + + // Prepare an invalid value + invalidValue := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: "", // Invalid: empty group ID + FieldID: model.NewId(), + Value: json.RawMessage(`"new value"`), + } + + // Try to update existing and insert invalid via upsert + existingValue.Value = json.RawMessage(`"should not update"`) + _, err = ss.PropertyValue().Upsert([]*model.PropertyValue{existingValue, invalidValue}) + require.Error(t, err) + require.Contains(t, err.Error(), "model.property_value.is_valid.app_error") + + // Verify the existing value was not changed + retrieved, err := ss.PropertyValue().Get(existingValue.ID) + require.NoError(t, err) + require.Equal(t, originalValue.Value, retrieved.Value) + require.Equal(t, originalValue.UpdateAt, retrieved.UpdateAt) + + // Verify the invalid value was not inserted + results, err := ss.PropertyValue().SearchPropertyValues(model.PropertyValueSearchOpts{ + TargetID: invalidValue.TargetID, + Page: 0, + PerPage: 10, + }) + require.NoError(t, err) + require.Empty(t, results) + }) +} + func testDeletePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { t.Run("should fail on nonexisting value", func(t *testing.T) { err := ss.PropertyValue().Delete(model.NewId()) diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index 4d4a8b6580b..0867090d747 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -7185,10 +7185,10 @@ func (s *TimerLayerPropertyFieldStore) SearchPropertyFields(opts model.PropertyF return result, err } -func (s *TimerLayerPropertyFieldStore) Update(field []*model.PropertyField) ([]*model.PropertyField, error) { +func (s *TimerLayerPropertyFieldStore) Update(fields []*model.PropertyField) ([]*model.PropertyField, error) { start := time.Now() - result, err := s.PropertyFieldStore.Update(field) + result, err := s.PropertyFieldStore.Update(fields) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil { @@ -7329,10 +7329,10 @@ func (s *TimerLayerPropertyValueStore) SearchPropertyValues(opts model.PropertyV return result, err } -func (s *TimerLayerPropertyValueStore) Update(field []*model.PropertyValue) ([]*model.PropertyValue, error) { +func (s *TimerLayerPropertyValueStore) Update(values []*model.PropertyValue) ([]*model.PropertyValue, error) { start := time.Now() - result, err := s.PropertyValueStore.Update(field) + result, err := s.PropertyValueStore.Update(values) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil { @@ -7345,6 +7345,22 @@ func (s *TimerLayerPropertyValueStore) Update(field []*model.PropertyValue) ([]* return result, err } +func (s *TimerLayerPropertyValueStore) Upsert(values []*model.PropertyValue) ([]*model.PropertyValue, error) { + start := time.Now() + + result, err := s.PropertyValueStore.Upsert(values) + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("PropertyValueStore.Upsert", success, elapsed) + } + return result, err +} + func (s *TimerLayerReactionStore) BulkGetForPosts(postIds []string) ([]*model.Reaction, error) { start := time.Now() diff --git a/server/i18n/en.json b/server/i18n/en.json index ae5b7c51ed9..ec9892aa767 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -5039,16 +5039,8 @@ "translation": "Unable to update Custom Profile Attribute field" }, { - "id": "app.custom_profile_attributes.property_value_creation.app_error", - "translation": "Cannot create property value" - }, - { - "id": "app.custom_profile_attributes.property_value_list.app_error", - "translation": "Unable to retrieve property values" - }, - { - "id": "app.custom_profile_attributes.property_value_update.app_error", - "translation": "Cannot update property value" + "id": "app.custom_profile_attributes.property_value_upsert.app_error", + "translation": "Unable to upsert Custom Profile Attribute fields" }, { "id": "app.custom_profile_attributes.search_property_fields.app_error", diff --git a/server/public/model/websocket_message.go b/server/public/model/websocket_message.go index 5736320be70..fd1ca2dda9c 100644 --- a/server/public/model/websocket_message.go +++ b/server/public/model/websocket_message.go @@ -94,6 +94,10 @@ const ( WebsocketScheduledPostCreated WebsocketEventType = "scheduled_post_created" WebsocketScheduledPostUpdated WebsocketEventType = "scheduled_post_updated" WebsocketScheduledPostDeleted WebsocketEventType = "scheduled_post_deleted" + WebsocketEventCPAFieldCreated WebsocketEventType = "custom_profile_attributes_field_created" + WebsocketEventCPAFieldUpdated WebsocketEventType = "custom_profile_attributes_field_updated" + WebsocketEventCPAFieldDeleted WebsocketEventType = "custom_profile_attributes_field_deleted" + WebsocketEventCPAValuesUpdated WebsocketEventType = "custom_profile_attributes_values_updated" WebSocketMsgTypeResponse = "response" WebSocketMsgTypeEvent = "event" From 4615ca5f28c4e4110b8adeeaf7310df870e51dcd Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Thu, 13 Feb 2025 10:17:41 -0500 Subject: [PATCH 4/7] MM-62944 Fix fileupload settings not being clickable (#30182) Co-authored-by: Mattermost Build --- .../src/components/admin_console/file_upload_setting.tsx | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/webapp/channels/src/components/admin_console/file_upload_setting.tsx b/webapp/channels/src/components/admin_console/file_upload_setting.tsx index 0e6207e317e..5b956d6003d 100644 --- a/webapp/channels/src/components/admin_console/file_upload_setting.tsx +++ b/webapp/channels/src/components/admin_console/file_upload_setting.tsx @@ -40,6 +40,10 @@ export default class FileUploadSetting extends React.PureComponent }; } + handleChooseClick = () => { + this.fileInputRef.current?.click(); + }; + handleChange = () => { const files = this.fileInputRef.current?.files; if (files && files.length > 0) { @@ -92,6 +96,7 @@ export default class FileUploadSetting extends React.PureComponent type='button' className='btn btn-tertiary' disabled={this.props.disabled} + onClick={this.handleChooseClick} > Date: Thu, 13 Feb 2025 08:23:50 -0700 Subject: [PATCH 5/7] [MM-62553]+[MM-62554] Property Architecture: cursor based pagination (#30119) * refactor: Replace pagination with cursor-based pagination for custom profile attributes * remove pagination loop on property value retrieval for CPA * add migrations to optimize pagination on property fields and values * adapt test to remove pagination check * update migrations list * postgres: drop index concurrently * concurrent index manipulation must be done outside of a Tx * fix: Correct SQL index drop syntax from "OM" to "ON" in migration files * test: Add CountForGroup test cases for property field store * refactor: Add CountForGroup method to PropertyFieldStore interface and implementations * Fix style and i18n * feat: Add optional deleted property field filtering to CountForGroup method * refactor: Update CountForGroup to support optional deleted property fields * test: Add comprehensive tests for CountForGroup with includeDeleted parameter * adapt test + gen layers * rename property service method and set the includeDelete to false * refactor: Remove redundant constant and use CustomProfileAttributesFieldLimit directly * fix tests --------- Co-authored-by: Mattermost Build --- .../channels/app/custom_profile_attributes.go | 28 ++- .../app/custom_profile_attributes_test.go | 57 +++++ .../channels/app/properties/property_field.go | 4 + server/channels/db/migrations/migrations.list | 8 + ...dex_pagination_on_property_values.down.sql | 14 ++ ...index_pagination_on_property_values.up.sql | 14 ++ ...dex_pagination_on_property_fields.down.sql | 14 ++ ...index_pagination_on_property_fields.up.sql | 14 ++ ...dex_pagination_on_property_values.down.sql | 2 + ...index_pagination_on_property_values.up.sql | 2 + ...dex_pagination_on_property_fields.down.sql | 2 + ...index_pagination_on_property_fields.up.sql | 2 + .../channels/store/retrylayer/retrylayer.go | 21 ++ .../store/sqlstore/property_field_store.go | 34 ++- .../store/sqlstore/property_value_store.go | 17 +- server/channels/store/store.go | 1 + .../storetest/mocks/PropertyFieldStore.go | 28 +++ .../store/storetest/property_field_store.go | 116 ++++++++-- .../store/storetest/property_value_store.go | 24 +- .../channels/store/timerlayer/timerlayer.go | 16 ++ server/i18n/en.json | 4 + server/public/model/property_field.go | 27 ++- server/public/model/property_field_test.go | 218 ++++++++++++++++++ server/public/model/property_value.go | 28 ++- server/public/model/property_value_test.go | 186 +++++++++++++++ 25 files changed, 820 insertions(+), 61 deletions(-) create mode 100644 server/channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.down.sql create mode 100644 server/channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.up.sql create mode 100644 server/channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.down.sql create mode 100644 server/channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.up.sql create mode 100644 server/channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.down.sql create mode 100644 server/channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.up.sql create mode 100644 server/channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.down.sql create mode 100644 server/channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.up.sql create mode 100644 server/public/model/property_field_test.go create mode 100644 server/public/model/property_value_test.go diff --git a/server/channels/app/custom_profile_attributes.go b/server/channels/app/custom_profile_attributes.go index caf9ee23fba..09c5f7e5d1f 100644 --- a/server/channels/app/custom_profile_attributes.go +++ b/server/channels/app/custom_profile_attributes.go @@ -12,7 +12,9 @@ import ( "github.com/pkg/errors" ) -const CustomProfileAttributesFieldLimit = 20 +const ( + CustomProfileAttributesFieldLimit = 20 +) var cpaGroupID string @@ -58,7 +60,6 @@ func (a *App) ListCPAFields() ([]*model.PropertyField, *model.AppError) { opts := model.PropertyFieldSearchOpts{ GroupID: groupID, - Page: 0, PerPage: CustomProfileAttributesFieldLimit, } @@ -76,12 +77,12 @@ func (a *App) CreateCPAField(field *model.PropertyField) (*model.PropertyField, return nil, model.NewAppError("CreateCPAField", "app.custom_profile_attributes.cpa_group_id.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - existingFields, appErr := a.ListCPAFields() - if appErr != nil { - return nil, appErr + fieldCount, err := a.Srv().propertyService.CountActivePropertyFieldsForGroup(groupID) + if err != nil { + return nil, model.NewAppError("CreateCPAField", "app.custom_profile_attributes.count_property_fields.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - if len(existingFields) >= CustomProfileAttributesFieldLimit { + if fieldCount >= CustomProfileAttributesFieldLimit { return nil, model.NewAppError("CreateCPAField", "app.custom_profile_attributes.limit_reached.app_error", nil, "", http.StatusUnprocessableEntity).Wrap(err) } @@ -171,19 +172,16 @@ func (a *App) ListCPAValues(userID string) ([]*model.PropertyValue, *model.AppEr return nil, model.NewAppError("GetCPAFields", "app.custom_profile_attributes.cpa_group_id.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - opts := model.PropertyValueSearchOpts{ - GroupID: groupID, - TargetID: userID, - Page: 0, - PerPage: 999999, - IncludeDeleted: false, - } - fields, err := a.Srv().propertyService.SearchPropertyValues(opts) + values, err := a.Srv().propertyService.SearchPropertyValues(model.PropertyValueSearchOpts{ + GroupID: groupID, + TargetID: userID, + PerPage: CustomProfileAttributesFieldLimit, + }) if err != nil { return nil, model.NewAppError("ListCPAValues", "app.custom_profile_attributes.list_property_values.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - return fields, nil + return values, nil } func (a *App) GetCPAValue(valueID string) (*model.PropertyValue, *model.AppError) { diff --git a/server/channels/app/custom_profile_attributes_test.go b/server/channels/app/custom_profile_attributes_test.go index e972f270fb8..e3c3fb542d0 100644 --- a/server/channels/app/custom_profile_attributes_test.go +++ b/server/channels/app/custom_profile_attributes_test.go @@ -517,3 +517,60 @@ 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) + }) +} diff --git a/server/channels/app/properties/property_field.go b/server/channels/app/properties/property_field.go index d93b81547ec..d3b59681903 100644 --- a/server/channels/app/properties/property_field.go +++ b/server/channels/app/properties/property_field.go @@ -19,6 +19,10 @@ func (ps *PropertyService) GetPropertyFields(ids []string) ([]*model.PropertyFie return ps.fieldStore.GetMany(ids) } +func (ps *PropertyService) CountActivePropertyFieldsForGroup(groupID string) (int64, error) { + return ps.fieldStore.CountForGroup(groupID, false) +} + func (ps *PropertyService) SearchPropertyFields(opts model.PropertyFieldSearchOpts) ([]*model.PropertyField, error) { return ps.fieldStore.SearchPropertyFields(opts) } diff --git a/server/channels/db/migrations/migrations.list b/server/channels/db/migrations/migrations.list index 8bf3e7a518e..08884044431 100644 --- a/server/channels/db/migrations/migrations.list +++ b/server/channels/db/migrations/migrations.list @@ -257,6 +257,10 @@ channels/db/migrations/mysql/000129_add_property_system_architecture.down.sql channels/db/migrations/mysql/000129_add_property_system_architecture.up.sql channels/db/migrations/mysql/000130_system_console_stats.down.sql channels/db/migrations/mysql/000130_system_console_stats.up.sql +channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.down.sql +channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.up.sql +channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.down.sql +channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.up.sql channels/db/migrations/postgres/000001_create_teams.down.sql channels/db/migrations/postgres/000001_create_teams.up.sql channels/db/migrations/postgres/000002_create_team_members.down.sql @@ -515,3 +519,7 @@ channels/db/migrations/postgres/000129_add_property_system_architecture.down.sql channels/db/migrations/postgres/000129_add_property_system_architecture.up.sql channels/db/migrations/postgres/000130_system_console_stats.down.sql channels/db/migrations/postgres/000130_system_console_stats.up.sql +channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.down.sql +channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.up.sql +channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.down.sql +channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.up.sql diff --git a/server/channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.down.sql b/server/channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.down.sql new file mode 100644 index 00000000000..339619c9817 --- /dev/null +++ b/server/channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.down.sql @@ -0,0 +1,14 @@ +SET @preparedStatement = (SELECT IF( + ( + SELECT COUNT(*) FROM INFORMATION_SCHEMA.STATISTICS + WHERE table_name = 'PropertyValues' + AND table_schema = DATABASE() + AND index_name = 'idx_propertyvalues_create_at_id' + ) > 0, + 'DROP INDEX idx_propertyvalues_create_at_id ON PropertyValues;', + 'SELECT 1' +)); + +PREPARE removeIndexIfExists FROM @preparedStatement; +EXECUTE removeIndexIfExists; +DEALLOCATE PREPARE removeIndexIfExists; diff --git a/server/channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.up.sql b/server/channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.up.sql new file mode 100644 index 00000000000..ec2c8558bb6 --- /dev/null +++ b/server/channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.up.sql @@ -0,0 +1,14 @@ +SET @preparedStatement = (SELECT IF( + ( + SELECT COUNT(*) FROM INFORMATION_SCHEMA.STATISTICS + WHERE table_name = 'PropertyValues' + AND table_schema = DATABASE() + AND index_name = 'idx_propertyvalues_create_at_id' + ) > 0, + 'SELECT 1', + 'CREATE INDEX idx_propertyvalues_create_at_id ON PropertyValues(CreateAt, ID);' +)); + +PREPARE createIndexIfNotExists FROM @preparedStatement; +EXECUTE createIndexIfNotExists; +DEALLOCATE PREPARE createIndexIfNotExists; diff --git a/server/channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.down.sql b/server/channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.down.sql new file mode 100644 index 00000000000..2197078e7c5 --- /dev/null +++ b/server/channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.down.sql @@ -0,0 +1,14 @@ +SET @preparedStatement = (SELECT IF( + ( + SELECT COUNT(*) FROM INFORMATION_SCHEMA.STATISTICS + WHERE table_name = 'PropertyFields' + AND table_schema = DATABASE() + AND index_name = 'idx_propertyfields_create_at_id' + ) > 0, + 'DROP INDEX idx_propertyfields_create_at_id ON PropertyFields;', + 'SELECT 1' +)); + +PREPARE removeIndexIfExists FROM @preparedStatement; +EXECUTE removeIndexIfExists; +DEALLOCATE PREPARE removeIndexIfExists; diff --git a/server/channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.up.sql b/server/channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.up.sql new file mode 100644 index 00000000000..35295733468 --- /dev/null +++ b/server/channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.up.sql @@ -0,0 +1,14 @@ +SET @preparedStatement = (SELECT IF( + ( + SELECT COUNT(*) FROM INFORMATION_SCHEMA.STATISTICS + WHERE table_name = 'PropertyFields' + AND table_schema = DATABASE() + AND index_name = 'idx_propertyfields_create_at_id' + ) > 0, + 'SELECT 1', + 'CREATE INDEX idx_propertyfields_create_at_id ON PropertyFields(CreateAt, ID);' +)); + +PREPARE createIndexIfNotExists FROM @preparedStatement; +EXECUTE createIndexIfNotExists; +DEALLOCATE PREPARE createIndexIfNotExists; diff --git a/server/channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.down.sql b/server/channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.down.sql new file mode 100644 index 00000000000..763fadfb0d3 --- /dev/null +++ b/server/channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.down.sql @@ -0,0 +1,2 @@ +-- morph:nontransactional +DROP INDEX CONCURRENTLY IF EXISTS idx_propertyvalues_create_at_id; diff --git a/server/channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.up.sql b/server/channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.up.sql new file mode 100644 index 00000000000..4b4a17885ec --- /dev/null +++ b/server/channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.up.sql @@ -0,0 +1,2 @@ +-- morph:nontransactional +CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_propertyvalues_create_at_id ON PropertyValues(CreateAt, ID) diff --git a/server/channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.down.sql b/server/channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.down.sql new file mode 100644 index 00000000000..397d82a9ad2 --- /dev/null +++ b/server/channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.down.sql @@ -0,0 +1,2 @@ +-- morph:nontransactional +DROP INDEX CONCURRENTLY IF EXISTS idx_propertyfields_create_at_id; diff --git a/server/channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.up.sql b/server/channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.up.sql new file mode 100644 index 00000000000..7c6801e081b --- /dev/null +++ b/server/channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.up.sql @@ -0,0 +1,2 @@ +-- morph:nontransactional +CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_propertyfields_create_at_id ON PropertyFields(CreateAt, ID) diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index a6f738d80b9..6353a85c4ba 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -8949,6 +8949,27 @@ func (s *RetryLayerProductNoticesStore) View(userID string, notices []string) er } +func (s *RetryLayerPropertyFieldStore) CountForGroup(groupID string, includeDeleted bool) (int64, error) { + + tries := 0 + for { + result, err := s.PropertyFieldStore.CountForGroup(groupID, includeDeleted) + if err == nil { + return result, nil + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + func (s *RetryLayerPropertyFieldStore) Create(field *model.PropertyField) (*model.PropertyField, error) { tries := 0 diff --git a/server/channels/store/sqlstore/property_field_store.go b/server/channels/store/sqlstore/property_field_store.go index 1f10ec84757..d19737b1cfe 100644 --- a/server/channels/store/sqlstore/property_field_store.go +++ b/server/channels/store/sqlstore/property_field_store.go @@ -78,9 +78,26 @@ func (s *SqlPropertyFieldStore) GetMany(ids []string) ([]*model.PropertyField, e return fields, nil } +func (s *SqlPropertyFieldStore) CountForGroup(groupID string, includeDeleted bool) (int64, error) { + var count int64 + builder := s.getQueryBuilder(). + Select("COUNT(id)"). + From("PropertyFields"). + Where(sq.Eq{"GroupID": groupID}) + + if !includeDeleted { + builder = builder.Where(sq.Eq{"DeleteAt": 0}) + } + + if err := s.GetReplica().GetBuilder(&count, builder); err != nil { + return int64(0), errors.Wrap(err, "failed to count Sessions") + } + return count, nil +} + func (s *SqlPropertyFieldStore) SearchPropertyFields(opts model.PropertyFieldSearchOpts) ([]*model.PropertyField, error) { - if opts.Page < 0 { - return nil, errors.New("page must be positive integer") + if err := opts.Cursor.IsValid(); err != nil { + return nil, fmt.Errorf("cursor is invalid: %w", err) } if opts.PerPage < 1 { @@ -88,10 +105,19 @@ func (s *SqlPropertyFieldStore) SearchPropertyFields(opts model.PropertyFieldSea } builder := s.tableSelectQuery. - OrderBy("CreateAt ASC"). - Offset(uint64(opts.Page * opts.PerPage)). + OrderBy("CreateAt ASC, Id ASC"). Limit(uint64(opts.PerPage)) + if !opts.Cursor.IsEmpty() { + builder = builder.Where(sq.Or{ + sq.Gt{"CreateAt": opts.Cursor.CreateAt}, + sq.And{ + sq.Eq{"CreateAt": opts.Cursor.CreateAt}, + sq.Gt{"Id": opts.Cursor.PropertyFieldID}, + }, + }) + } + if !opts.IncludeDeleted { builder = builder.Where(sq.Eq{"DeleteAt": 0}) } diff --git a/server/channels/store/sqlstore/property_value_store.go b/server/channels/store/sqlstore/property_value_store.go index d9d12603a32..d0c37ce501c 100644 --- a/server/channels/store/sqlstore/property_value_store.go +++ b/server/channels/store/sqlstore/property_value_store.go @@ -83,8 +83,8 @@ func (s *SqlPropertyValueStore) GetMany(ids []string) ([]*model.PropertyValue, e } func (s *SqlPropertyValueStore) SearchPropertyValues(opts model.PropertyValueSearchOpts) ([]*model.PropertyValue, error) { - if opts.Page < 0 { - return nil, errors.New("page must be positive integer") + if err := opts.Cursor.IsValid(); err != nil { + return nil, fmt.Errorf("cursor is invalid: %w", err) } if opts.PerPage < 1 { @@ -92,10 +92,19 @@ func (s *SqlPropertyValueStore) SearchPropertyValues(opts model.PropertyValueSea } builder := s.tableSelectQuery. - OrderBy("CreateAt ASC"). - Offset(uint64(opts.Page * opts.PerPage)). + OrderBy("CreateAt ASC, Id ASC"). Limit(uint64(opts.PerPage)) + if !opts.Cursor.IsEmpty() { + builder = builder.Where(sq.Or{ + sq.Gt{"CreateAt": opts.Cursor.CreateAt}, + sq.And{ + sq.Eq{"CreateAt": opts.Cursor.CreateAt}, + sq.Gt{"Id": opts.Cursor.PropertyValueID}, + }, + }) + } + if !opts.IncludeDeleted { builder = builder.Where(sq.Eq{"DeleteAt": 0}) } diff --git a/server/channels/store/store.go b/server/channels/store/store.go index 88f2d29fe02..5809f74c55e 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -1089,6 +1089,7 @@ type PropertyFieldStore interface { Create(field *model.PropertyField) (*model.PropertyField, error) Get(id string) (*model.PropertyField, error) GetMany(ids []string) ([]*model.PropertyField, error) + CountForGroup(groupID string, includeDeleted bool) (int64, error) SearchPropertyFields(opts model.PropertyFieldSearchOpts) ([]*model.PropertyField, error) Update(fields []*model.PropertyField) ([]*model.PropertyField, error) Delete(id string) error diff --git a/server/channels/store/storetest/mocks/PropertyFieldStore.go b/server/channels/store/storetest/mocks/PropertyFieldStore.go index b32b267da7e..a1baac5217e 100644 --- a/server/channels/store/storetest/mocks/PropertyFieldStore.go +++ b/server/channels/store/storetest/mocks/PropertyFieldStore.go @@ -14,6 +14,34 @@ type PropertyFieldStore struct { mock.Mock } +// CountForGroup provides a mock function with given fields: groupID, includeDeleted +func (_m *PropertyFieldStore) CountForGroup(groupID string, includeDeleted bool) (int64, error) { + ret := _m.Called(groupID, includeDeleted) + + if len(ret) == 0 { + panic("no return value specified for CountForGroup") + } + + var r0 int64 + var r1 error + if rf, ok := ret.Get(0).(func(string, bool) (int64, error)); ok { + return rf(groupID, includeDeleted) + } + if rf, ok := ret.Get(0).(func(string, bool) int64); ok { + r0 = rf(groupID, includeDeleted) + } else { + r0 = ret.Get(0).(int64) + } + + if rf, ok := ret.Get(1).(func(string, bool) error); ok { + r1 = rf(groupID, includeDeleted) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // Create provides a mock function with given fields: field func (_m *PropertyFieldStore) Create(field *model.PropertyField) (*model.PropertyField, error) { ret := _m.Called(field) diff --git a/server/channels/store/storetest/property_field_store.go b/server/channels/store/storetest/property_field_store.go index 19fe3d68012..d6f8ce6a409 100644 --- a/server/channels/store/storetest/property_field_store.go +++ b/server/channels/store/storetest/property_field_store.go @@ -5,6 +5,7 @@ package storetest import ( "database/sql" + "fmt" "testing" "time" @@ -21,6 +22,7 @@ func TestPropertyFieldStore(t *testing.T, rctx request.CTX, ss store.Store, s Sq t.Run("UpdatePropertyField", func(t *testing.T) { testUpdatePropertyField(t, rctx, ss) }) t.Run("DeletePropertyField", func(t *testing.T) { testDeletePropertyField(t, rctx, ss) }) t.Run("SearchPropertyFields", func(t *testing.T) { testSearchPropertyFields(t, rctx, ss) }) + t.Run("CountForGroup", func(t *testing.T) { testCountForGroup(t, rctx, ss) }) } func testCreatePropertyField(t *testing.T, _ request.CTX, ss store.Store) { @@ -356,6 +358,97 @@ func testDeletePropertyField(t *testing.T, _ request.CTX, ss store.Store) { }) } +func testCountForGroup(t *testing.T, _ request.CTX, ss store.Store) { + t.Run("should return 0 for group with no properties", func(t *testing.T) { + count, err := ss.PropertyField().CountForGroup(model.NewId(), false) + require.NoError(t, err) + require.Equal(t, int64(0), count) + }) + + t.Run("should return correct count for group with properties", func(t *testing.T) { + groupID := model.NewId() + + // Create 5 property fields + for i := 0; i < 5; i++ { + field := &model.PropertyField{ + GroupID: groupID, + Name: fmt.Sprintf("Field %d", i), + Type: model.PropertyFieldTypeText, + } + _, err := ss.PropertyField().Create(field) + require.NoError(t, err) + } + + count, err := ss.PropertyField().CountForGroup(groupID, false) + require.NoError(t, err) + require.Equal(t, int64(5), count) + }) + + t.Run("should not count deleted properties when includeDeleted is false", func(t *testing.T) { + groupID := model.NewId() + + // Create 5 property fields + for i := 0; i < 5; i++ { + field := &model.PropertyField{ + GroupID: groupID, + Name: fmt.Sprintf("Field %d", i), + Type: model.PropertyFieldTypeText, + } + _, err := ss.PropertyField().Create(field) + require.NoError(t, err) + } + + // Create one more and delete it + deletedField := &model.PropertyField{ + GroupID: groupID, + Name: "To be deleted", + Type: model.PropertyFieldTypeText, + } + _, err := ss.PropertyField().Create(deletedField) + require.NoError(t, err) + + err = ss.PropertyField().Delete(deletedField.ID) + require.NoError(t, err) + + // Count should be 5 since the deleted field shouldn't be counted + count, err := ss.PropertyField().CountForGroup(groupID, false) + require.NoError(t, err) + require.Equal(t, int64(5), count) + }) + + t.Run("should count deleted properties when includeDeleted is true", func(t *testing.T) { + groupID := model.NewId() + + // Create 5 property fields + for i := 0; i < 5; i++ { + field := &model.PropertyField{ + GroupID: groupID, + Name: fmt.Sprintf("Field %d", i), + Type: model.PropertyFieldTypeText, + } + _, err := ss.PropertyField().Create(field) + require.NoError(t, err) + } + + // Create one more and delete it + deletedField := &model.PropertyField{ + GroupID: groupID, + Name: "To be deleted", + Type: model.PropertyFieldTypeText, + } + _, err := ss.PropertyField().Create(deletedField) + require.NoError(t, err) + + err = ss.PropertyField().Delete(deletedField.ID) + require.NoError(t, err) + + // Count should be 6 since we're including deleted fields + count, err := ss.PropertyField().CountForGroup(groupID, true) + require.NoError(t, err) + require.Equal(t, int64(6), count) + }) +} + func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { groupID := model.NewId() targetID := model.NewId() @@ -406,18 +499,9 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { expectedError bool expectedIDs []string }{ - { - name: "negative page", - opts: model.PropertyFieldSearchOpts{ - Page: -1, - PerPage: 10, - }, - expectedError: true, - }, { name: "negative per_page", opts: model.PropertyFieldSearchOpts{ - Page: 0, PerPage: -1, }, expectedError: true, @@ -426,7 +510,6 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { name: "filter by group_id", opts: model.PropertyFieldSearchOpts{ GroupID: groupID, - Page: 0, PerPage: 10, }, expectedIDs: []string{field1.ID, field2.ID}, @@ -435,7 +518,6 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { name: "filter by group_id including deleted", opts: model.PropertyFieldSearchOpts{ GroupID: groupID, - Page: 0, PerPage: 10, IncludeDeleted: true, }, @@ -445,7 +527,6 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { name: "filter by target_type", opts: model.PropertyFieldSearchOpts{ TargetType: "test_type", - Page: 0, PerPage: 10, }, expectedIDs: []string{field1.ID, field3.ID}, @@ -454,7 +535,6 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { name: "filter by target_id", opts: model.PropertyFieldSearchOpts{ TargetID: targetID, - Page: 0, PerPage: 10, }, expectedIDs: []string{field1.ID, field2.ID}, @@ -463,7 +543,6 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { name: "pagination page 0", opts: model.PropertyFieldSearchOpts{ GroupID: groupID, - Page: 0, PerPage: 2, IncludeDeleted: true, }, @@ -472,8 +551,11 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { { name: "pagination page 1", opts: model.PropertyFieldSearchOpts{ - GroupID: groupID, - Page: 1, + GroupID: groupID, + Cursor: model.PropertyFieldSearchCursor{ + CreateAt: field2.CreateAt, + PropertyFieldID: field2.ID, + }, PerPage: 2, IncludeDeleted: true, }, @@ -490,7 +572,7 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { } require.NoError(t, err) - var ids = make([]string, len(results)) + ids := make([]string, len(results)) for i, field := range results { ids[i] = field.ID } diff --git a/server/channels/store/storetest/property_value_store.go b/server/channels/store/storetest/property_value_store.go index b360380b627..de72b466757 100644 --- a/server/channels/store/storetest/property_value_store.go +++ b/server/channels/store/storetest/property_value_store.go @@ -567,18 +567,9 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { expectedError bool expectedIDs []string }{ - { - name: "negative page", - opts: model.PropertyValueSearchOpts{ - Page: -1, - PerPage: 10, - }, - expectedError: true, - }, { name: "negative per_page", opts: model.PropertyValueSearchOpts{ - Page: 0, PerPage: -1, }, expectedError: true, @@ -587,7 +578,6 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { name: "filter by group_id", opts: model.PropertyValueSearchOpts{ GroupID: groupID, - Page: 0, PerPage: 10, }, expectedIDs: []string{value1.ID, value2.ID}, @@ -597,7 +587,6 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { opts: model.PropertyValueSearchOpts{ GroupID: groupID, TargetType: "test_type", - Page: 0, PerPage: 10, }, expectedIDs: []string{value1.ID}, @@ -608,7 +597,6 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { GroupID: groupID, TargetType: "test_type", IncludeDeleted: true, - Page: 0, PerPage: 10, }, expectedIDs: []string{value1.ID, value4.ID}, @@ -617,7 +605,6 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { name: "filter by target_id", opts: model.PropertyValueSearchOpts{ TargetID: targetID, - Page: 0, PerPage: 10, }, expectedIDs: []string{value1.ID, value2.ID}, @@ -627,7 +614,6 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { opts: model.PropertyValueSearchOpts{ GroupID: groupID, TargetID: targetID, - Page: 0, PerPage: 10, }, expectedIDs: []string{value1.ID, value2.ID}, @@ -636,7 +622,6 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { name: "filter by field_id", opts: model.PropertyValueSearchOpts{ FieldID: fieldID, - Page: 0, PerPage: 10, }, expectedIDs: []string{value1.ID}, @@ -646,7 +631,6 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { opts: model.PropertyValueSearchOpts{ FieldID: fieldID, IncludeDeleted: true, - Page: 0, PerPage: 10, }, expectedIDs: []string{value1.ID, value4.ID}, @@ -655,7 +639,6 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { name: "pagination page 0", opts: model.PropertyValueSearchOpts{ GroupID: groupID, - Page: 0, PerPage: 1, }, expectedIDs: []string{value1.ID}, @@ -664,7 +647,10 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { name: "pagination page 1", opts: model.PropertyValueSearchOpts{ GroupID: groupID, - Page: 1, + Cursor: model.PropertyValueSearchCursor{ + CreateAt: value1.CreateAt, + PropertyValueID: value1.ID, + }, PerPage: 1, }, expectedIDs: []string{value2.ID}, @@ -680,7 +666,7 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { } require.NoError(t, err) - var ids = make([]string, len(results)) + ids := make([]string, len(results)) for i, value := range results { ids[i] = value.ID } diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index 0867090d747..d0af751ca04 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -7105,6 +7105,22 @@ func (s *TimerLayerProductNoticesStore) View(userID string, notices []string) er return err } +func (s *TimerLayerPropertyFieldStore) CountForGroup(groupID string, includeDeleted bool) (int64, error) { + start := time.Now() + + result, err := s.PropertyFieldStore.CountForGroup(groupID, includeDeleted) + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("PropertyFieldStore.CountForGroup", success, elapsed) + } + return result, err +} + func (s *TimerLayerPropertyFieldStore) Create(field *model.PropertyField) (*model.PropertyField, error) { start := time.Now() diff --git a/server/i18n/en.json b/server/i18n/en.json index ec9892aa767..781bae8c767 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -5006,6 +5006,10 @@ "id": "app.custom_group.unique_name", "translation": "group name is not unique" }, + { + "id": "app.custom_profile_attributes.count_property_fields.app_error", + "translation": "Unable to count the number of fields for the custom profile attribute group" + }, { "id": "app.custom_profile_attributes.cpa_group_id.app_error", "translation": "Cannot register Custom Profile Attributes property group" diff --git a/server/public/model/property_field.go b/server/public/model/property_field.go index 4a07d65712f..531b81ceb87 100644 --- a/server/public/model/property_field.go +++ b/server/public/model/property_field.go @@ -4,6 +4,7 @@ package model import ( + "errors" "net/http" "strings" ) @@ -141,11 +142,35 @@ func (pf *PropertyField) Patch(patch *PropertyFieldPatch) { } } +type PropertyFieldSearchCursor struct { + PropertyFieldID string + CreateAt int64 +} + +func (p PropertyFieldSearchCursor) IsEmpty() bool { + return p.PropertyFieldID == "" && p.CreateAt == 0 +} + +func (p PropertyFieldSearchCursor) IsValid() error { + if p.IsEmpty() { + return nil + } + + if p.CreateAt <= 0 { + return errors.New("create at cannot be negative or zero") + } + + if !IsValidId(p.PropertyFieldID) { + return errors.New("property field id is invalid") + } + return nil +} + type PropertyFieldSearchOpts struct { GroupID string TargetType string TargetID string IncludeDeleted bool - Page int + Cursor PropertyFieldSearchCursor PerPage int } diff --git a/server/public/model/property_field_test.go b/server/public/model/property_field_test.go new file mode 100644 index 00000000000..eac0adc211d --- /dev/null +++ b/server/public/model/property_field_test.go @@ -0,0 +1,218 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package model + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPropertyField_PreSave(t *testing.T) { + t.Run("sets ID if empty", func(t *testing.T) { + pf := &PropertyField{} + pf.PreSave() + assert.NotEmpty(t, pf.ID) + assert.Len(t, pf.ID, 26) // Length of NewId() + }) + + t.Run("keeps existing ID", func(t *testing.T) { + pf := &PropertyField{ID: "existing_id"} + pf.PreSave() + assert.Equal(t, "existing_id", pf.ID) + }) + + t.Run("sets CreateAt if zero", func(t *testing.T) { + pf := &PropertyField{} + pf.PreSave() + assert.NotZero(t, pf.CreateAt) + }) + + t.Run("sets UpdateAt equal to CreateAt", func(t *testing.T) { + pf := &PropertyField{} + pf.PreSave() + assert.Equal(t, pf.CreateAt, pf.UpdateAt) + }) +} + +func TestPropertyField_IsValid(t *testing.T) { + t.Run("valid field", func(t *testing.T) { + pf := &PropertyField{ + ID: NewId(), + GroupID: NewId(), + Name: "test field", + Type: PropertyFieldTypeText, + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.NoError(t, pf.IsValid()) + }) + + t.Run("invalid ID", func(t *testing.T) { + pf := &PropertyField{ + ID: "invalid", + GroupID: NewId(), + Name: "test field", + Type: PropertyFieldTypeText, + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pf.IsValid()) + }) + + t.Run("invalid GroupID", func(t *testing.T) { + pf := &PropertyField{ + ID: NewId(), + GroupID: "invalid", + Name: "test field", + Type: PropertyFieldTypeText, + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pf.IsValid()) + }) + + t.Run("empty name", func(t *testing.T) { + pf := &PropertyField{ + ID: NewId(), + GroupID: NewId(), + Name: "", + Type: PropertyFieldTypeText, + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pf.IsValid()) + }) + + t.Run("invalid type", func(t *testing.T) { + pf := &PropertyField{ + ID: NewId(), + GroupID: NewId(), + Name: "test field", + Type: "invalid", + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pf.IsValid()) + }) + + t.Run("zero CreateAt", func(t *testing.T) { + pf := &PropertyField{ + ID: NewId(), + GroupID: NewId(), + Name: "test field", + Type: PropertyFieldTypeText, + CreateAt: 0, + UpdateAt: GetMillis(), + } + require.Error(t, pf.IsValid()) + }) + + t.Run("zero UpdateAt", func(t *testing.T) { + pf := &PropertyField{ + ID: NewId(), + GroupID: NewId(), + Name: "test field", + Type: PropertyFieldTypeText, + CreateAt: GetMillis(), + UpdateAt: 0, + } + require.Error(t, pf.IsValid()) + }) +} + +func TestPropertyField_SanitizeInput(t *testing.T) { + t.Run("trims spaces from name", func(t *testing.T) { + pf := &PropertyField{Name: " test field "} + pf.SanitizeInput() + assert.Equal(t, "test field", pf.Name) + }) +} + +func TestPropertyField_Patch(t *testing.T) { + t.Run("patches all fields", func(t *testing.T) { + pf := &PropertyField{ + Name: "original name", + Type: PropertyFieldTypeText, + TargetID: "original_target", + TargetType: "original_type", + } + + patch := &PropertyFieldPatch{ + Name: NewPointer("new name"), + Type: NewPointer(PropertyFieldTypeSelect), + TargetID: NewPointer("new_target"), + TargetType: NewPointer("new_type"), + Attrs: &map[string]any{"key": "value"}, + } + + pf.Patch(patch) + + assert.Equal(t, "new name", pf.Name) + assert.Equal(t, PropertyFieldTypeSelect, pf.Type) + assert.Equal(t, "new_target", pf.TargetID) + assert.Equal(t, "new_type", pf.TargetType) + assert.EqualValues(t, StringInterface{"key": "value"}, pf.Attrs) + }) + + t.Run("patches only specified fields", func(t *testing.T) { + pf := &PropertyField{ + Name: "original name", + Type: PropertyFieldTypeText, + TargetID: "original_target", + TargetType: "original_type", + } + + patch := &PropertyFieldPatch{ + Name: NewPointer("new name"), + } + + pf.Patch(patch) + + assert.Equal(t, "new name", pf.Name) + assert.Equal(t, PropertyFieldTypeText, pf.Type) + assert.Equal(t, "original_target", pf.TargetID) + assert.Equal(t, "original_type", pf.TargetType) + }) +} + +func TestPropertyFieldSearchCursor_IsValid(t *testing.T) { + t.Run("empty cursor is valid", func(t *testing.T) { + cursor := PropertyFieldSearchCursor{} + assert.NoError(t, cursor.IsValid()) + }) + + t.Run("valid cursor", func(t *testing.T) { + cursor := PropertyFieldSearchCursor{ + PropertyFieldID: NewId(), + CreateAt: GetMillis(), + } + assert.NoError(t, cursor.IsValid()) + }) + + t.Run("invalid PropertyFieldID", func(t *testing.T) { + cursor := PropertyFieldSearchCursor{ + PropertyFieldID: "invalid", + CreateAt: GetMillis(), + } + assert.Error(t, cursor.IsValid()) + }) + + t.Run("zero CreateAt", func(t *testing.T) { + cursor := PropertyFieldSearchCursor{ + PropertyFieldID: NewId(), + CreateAt: 0, + } + assert.Error(t, cursor.IsValid()) + }) + + t.Run("negative CreateAt", func(t *testing.T) { + cursor := PropertyFieldSearchCursor{ + PropertyFieldID: NewId(), + CreateAt: -1, + } + assert.Error(t, cursor.IsValid()) + }) +} diff --git a/server/public/model/property_value.go b/server/public/model/property_value.go index 17ef1709be1..ea2d75a0898 100644 --- a/server/public/model/property_value.go +++ b/server/public/model/property_value.go @@ -6,6 +6,8 @@ package model import ( "encoding/json" "net/http" + + "github.com/pkg/errors" ) type PropertyValue struct { @@ -63,12 +65,36 @@ func (pv *PropertyValue) IsValid() error { return nil } +type PropertyValueSearchCursor struct { + PropertyValueID string + CreateAt int64 +} + +func (p PropertyValueSearchCursor) IsEmpty() bool { + return p.PropertyValueID == "" && p.CreateAt == 0 +} + +func (p PropertyValueSearchCursor) IsValid() error { + if p.IsEmpty() { + return nil + } + + if p.CreateAt <= 0 { + return errors.New("create at cannot be negative or zero") + } + + if !IsValidId(p.PropertyValueID) { + return errors.New("property field id is invalid") + } + return nil +} + type PropertyValueSearchOpts struct { GroupID string TargetType string TargetID string FieldID string IncludeDeleted bool - Page int + Cursor PropertyValueSearchCursor PerPage int } diff --git a/server/public/model/property_value_test.go b/server/public/model/property_value_test.go new file mode 100644 index 00000000000..f9e4cc62984 --- /dev/null +++ b/server/public/model/property_value_test.go @@ -0,0 +1,186 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package model + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPropertyValue_PreSave(t *testing.T) { + t.Run("sets ID if empty", func(t *testing.T) { + pv := &PropertyValue{} + pv.PreSave() + assert.NotEmpty(t, pv.ID) + assert.Len(t, pv.ID, 26) // Length of NewId() + }) + + t.Run("keeps existing ID", func(t *testing.T) { + pv := &PropertyValue{ID: "existing_id"} + pv.PreSave() + assert.Equal(t, "existing_id", pv.ID) + }) + + t.Run("sets CreateAt if zero", func(t *testing.T) { + pv := &PropertyValue{} + pv.PreSave() + assert.NotZero(t, pv.CreateAt) + }) + + t.Run("sets UpdateAt equal to CreateAt", func(t *testing.T) { + pv := &PropertyValue{} + pv.PreSave() + assert.Equal(t, pv.CreateAt, pv.UpdateAt) + }) +} + +func TestPropertyValue_IsValid(t *testing.T) { + t.Run("valid value", func(t *testing.T) { + value := json.RawMessage(`{"test": "value"}`) + pv := &PropertyValue{ + ID: NewId(), + TargetID: NewId(), + TargetType: "test_type", + GroupID: NewId(), + FieldID: NewId(), + Value: value, + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.NoError(t, pv.IsValid()) + }) + + t.Run("invalid ID", func(t *testing.T) { + pv := &PropertyValue{ + ID: "invalid", + TargetID: NewId(), + TargetType: "test_type", + GroupID: NewId(), + FieldID: NewId(), + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pv.IsValid()) + }) + + t.Run("invalid TargetID", func(t *testing.T) { + pv := &PropertyValue{ + ID: NewId(), + TargetID: "invalid", + TargetType: "test_type", + GroupID: NewId(), + FieldID: NewId(), + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pv.IsValid()) + }) + + t.Run("empty TargetType", func(t *testing.T) { + pv := &PropertyValue{ + ID: NewId(), + TargetID: NewId(), + TargetType: "", + GroupID: NewId(), + FieldID: NewId(), + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pv.IsValid()) + }) + + t.Run("invalid GroupID", func(t *testing.T) { + pv := &PropertyValue{ + ID: NewId(), + TargetID: NewId(), + TargetType: "test_type", + GroupID: "invalid", + FieldID: NewId(), + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pv.IsValid()) + }) + + t.Run("invalid FieldID", func(t *testing.T) { + pv := &PropertyValue{ + ID: NewId(), + TargetID: NewId(), + TargetType: "test_type", + GroupID: NewId(), + FieldID: "invalid", + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pv.IsValid()) + }) + + t.Run("zero CreateAt", func(t *testing.T) { + pv := &PropertyValue{ + ID: NewId(), + TargetID: NewId(), + TargetType: "test_type", + GroupID: NewId(), + FieldID: NewId(), + CreateAt: 0, + UpdateAt: GetMillis(), + } + require.Error(t, pv.IsValid()) + }) + + t.Run("zero UpdateAt", func(t *testing.T) { + pv := &PropertyValue{ + ID: NewId(), + TargetID: NewId(), + TargetType: "test_type", + GroupID: NewId(), + FieldID: NewId(), + CreateAt: GetMillis(), + UpdateAt: 0, + } + require.Error(t, pv.IsValid()) + }) +} + +func TestPropertyValueSearchCursor_IsValid(t *testing.T) { + t.Run("empty cursor is valid", func(t *testing.T) { + cursor := PropertyValueSearchCursor{} + assert.NoError(t, cursor.IsValid()) + }) + + t.Run("valid cursor", func(t *testing.T) { + cursor := PropertyValueSearchCursor{ + PropertyValueID: NewId(), + CreateAt: GetMillis(), + } + assert.NoError(t, cursor.IsValid()) + }) + + t.Run("invalid PropertyValueID", func(t *testing.T) { + cursor := PropertyValueSearchCursor{ + PropertyValueID: "invalid", + CreateAt: GetMillis(), + } + assert.Error(t, cursor.IsValid()) + }) + + t.Run("zero CreateAt", func(t *testing.T) { + cursor := PropertyValueSearchCursor{ + PropertyValueID: NewId(), + CreateAt: 0, + } + assert.Error(t, cursor.IsValid()) + }) + + t.Run("negative CreateAt", func(t *testing.T) { + cursor := PropertyValueSearchCursor{ + PropertyValueID: NewId(), + CreateAt: -1, + } + assert.Error(t, cursor.IsValid()) + }) +} From 1a58f923e0c888e31c12d559f0e575c6d6118453 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Thu, 13 Feb 2025 21:10:34 +0530 Subject: [PATCH 6/7] [aider assisted] MM-61888: Add ClientSideUserIds field to MetricsSettings (#30127) We add a new config setting to allow the admin to set a fixed list of userIDs to track for all client side webapp metrics. This gives the admin to get a deeper look at how the application is behaving for a single user. A new section in the system console is also added for the user to edit this setting from the UI. https://mattermost.atlassian.net/browse/MM-61888 ```release-note A new config setting MetricsSettings.ClientSideUserIds is added where you can set the user ids you want to track for client side webapp metrics. ``` * fix lint errors ```release-note NONE ``` * fixing tests ```release-note NONE ``` --- server/channels/api4/metrics_test.go | 18 +- server/channels/app/metrics.go | 10 +- server/einterfaces/metrics.go | 18 +- server/einterfaces/mocks/MetricsInterface.go | 54 +- server/enterprise/metrics/metrics.go | 108 ++-- server/i18n/en.json | 10 +- server/public/model/config.go | 32 +- server/public/model/incoming_webhook.go | 2 +- .../client_side_userids_setting.test.tsx.snap | 470 ++++++++++++++++++ .../admin_console/admin_definition.tsx | 10 + .../client_side_userids_setting.test.tsx | 133 +++++ .../client_side_userids_setting.tsx | 81 +++ webapp/channels/src/i18n/en.json | 3 + 13 files changed, 861 insertions(+), 88 deletions(-) create mode 100644 webapp/channels/src/components/admin_console/__snapshots__/client_side_userids_setting.test.tsx.snap create mode 100644 webapp/channels/src/components/admin_console/client_side_userids_setting.test.tsx create mode 100644 webapp/channels/src/components/admin_console/client_side_userids_setting.tsx diff --git a/server/channels/api4/metrics_test.go b/server/channels/api4/metrics_test.go index 8f6e6a436be..c22560fe440 100644 --- a/server/channels/api4/metrics_test.go +++ b/server/channels/api4/metrics_test.go @@ -91,7 +91,11 @@ func TestSubmitMetrics(t *testing.T) { t.Run("metrics enabled and valid", func(t *testing.T) { metricsMock := setupMetricsMock() - metricsMock.On("IncrementClientLongTasks", mock.AnythingOfType("string"), mock.AnythingOfType("string"), mock.AnythingOfType("float64")).Return() + metricsMock.On("IncrementClientLongTasks", + mock.AnythingOfType("string"), + mock.AnythingOfType("string"), + mock.AnythingOfType("string"), + mock.AnythingOfType("float64")).Return() platform.RegisterMetricsInterface(func(_ *platform.PlatformService, _, _ string) einterfaces.MetricsInterface { return metricsMock @@ -159,7 +163,11 @@ func TestSubmitMetrics(t *testing.T) { t.Run("metrics recorded for API errors", func(t *testing.T) { metricsMock := setupMetricsMock() - metricsMock.On("IncrementClientLongTasks", mock.AnythingOfType("string"), mock.AnythingOfType("string"), mock.AnythingOfType("float64")).Return() + metricsMock.On("IncrementClientLongTasks", + mock.AnythingOfType("string"), + mock.AnythingOfType("string"), + mock.AnythingOfType("string"), + mock.AnythingOfType("float64")).Return() platform.RegisterMetricsInterface(func(_ *platform.PlatformService, _, _ string) einterfaces.MetricsInterface { return metricsMock @@ -190,7 +198,11 @@ func TestSubmitMetrics(t *testing.T) { t.Run("metrics recorded for URL length limit errors", func(t *testing.T) { metricsMock := setupMetricsMock() - metricsMock.On("IncrementClientLongTasks", mock.AnythingOfType("string"), mock.AnythingOfType("string"), mock.AnythingOfType("float64")).Return() + metricsMock.On("IncrementClientLongTasks", + mock.AnythingOfType("string"), + mock.AnythingOfType("string"), + mock.AnythingOfType("string"), + mock.AnythingOfType("float64")).Return() platform.RegisterMetricsInterface(func(_ *platform.PlatformService, _, _ string) einterfaces.MetricsInterface { return metricsMock diff --git a/server/channels/app/metrics.go b/server/channels/app/metrics.go index eb316b29d5a..7bf8d2dc509 100644 --- a/server/channels/app/metrics.go +++ b/server/channels/app/metrics.go @@ -19,7 +19,7 @@ func (a *App) RegisterPerformanceReport(rctx request.CTX, report *model.Performa for _, c := range report.Counters { switch c.Metric { case model.ClientLongTasks: - a.Metrics().IncrementClientLongTasks(commonLabels["platform"], commonLabels["agent"], c.Value) + a.Metrics().IncrementClientLongTasks(commonLabels["platform"], commonLabels["agent"], userID, c.Value) default: // we intentionally skip unknown metrics } @@ -50,22 +50,26 @@ func (a *App) RegisterPerformanceReport(rctx request.CTX, report *model.Performa case model.ClientFirstContentfulPaint: a.Metrics().ObserveClientFirstContentfulPaint(commonLabels["platform"], commonLabels["agent"], + userID, h.Value/1000) case model.ClientLargestContentfulPaint: a.Metrics().ObserveClientLargestContentfulPaint( commonLabels["platform"], commonLabels["agent"], h.GetLabelValue("region", model.AcceptedLCPRegions, "other"), + userID, h.Value/1000) case model.ClientInteractionToNextPaint: a.Metrics().ObserveClientInteractionToNextPaint( commonLabels["platform"], commonLabels["agent"], h.GetLabelValue("interaction", model.AcceptedInteractions, "other"), + userID, h.Value/1000) case model.ClientCumulativeLayoutShift: a.Metrics().ObserveClientCumulativeLayoutShift(commonLabels["platform"], commonLabels["agent"], + userID, h.Value) case model.ClientPageLoadDuration: a.Metrics().ObserveClientPageLoadDuration(commonLabels["platform"], @@ -76,20 +80,24 @@ func (a *App) RegisterPerformanceReport(rctx request.CTX, report *model.Performa commonLabels["platform"], commonLabels["agent"], h.GetLabelValue("fresh", model.AcceptedTrueFalseLabels, ""), + userID, h.Value/1000) case model.ClientTeamSwitchDuration: a.Metrics().ObserveClientTeamSwitchDuration( commonLabels["platform"], commonLabels["agent"], h.GetLabelValue("fresh", model.AcceptedTrueFalseLabels, ""), + userID, h.Value/1000) case model.ClientRHSLoadDuration: a.Metrics().ObserveClientRHSLoadDuration(commonLabels["platform"], commonLabels["agent"], + userID, h.Value/1000) case model.ClientGlobalThreadsLoadDuration: a.Metrics().ObserveGlobalThreadsLoadDuration(commonLabels["platform"], commonLabels["agent"], + userID, h.Value/1000) case model.MobileClientLoadDuration: a.Metrics().ObserveMobileClientLoadDuration(commonLabels["platform"], diff --git a/server/einterfaces/metrics.go b/server/einterfaces/metrics.go index bd2930899ac..62ee54ddbee 100644 --- a/server/einterfaces/metrics.go +++ b/server/einterfaces/metrics.go @@ -108,16 +108,16 @@ type MetricsInterface interface { ObserveClientTimeToLastByte(platform, agent, userID string, elapsed float64) ObserveClientTimeToDomInteractive(platform, agent, userID string, elapsed float64) ObserveClientSplashScreenEnd(platform, agent, pageType, userID string, elapsed float64) - ObserveClientFirstContentfulPaint(platform, agent string, elapsed float64) - ObserveClientLargestContentfulPaint(platform, agent, region string, elapsed float64) - ObserveClientInteractionToNextPaint(platform, agent, interaction string, elapsed float64) - ObserveClientCumulativeLayoutShift(platform, agent string, elapsed float64) - IncrementClientLongTasks(platform, agent string, inc float64) + ObserveClientFirstContentfulPaint(platform, agent, userID string, elapsed float64) + ObserveClientLargestContentfulPaint(platform, agent, region, userID string, elapsed float64) + ObserveClientInteractionToNextPaint(platform, agent, interaction, userID string, elapsed float64) + ObserveClientCumulativeLayoutShift(platform, agent, userID string, elapsed float64) + IncrementClientLongTasks(platform, agent, userID string, inc float64) ObserveClientPageLoadDuration(platform, agent, userID string, elapsed float64) - ObserveClientChannelSwitchDuration(platform, agent, fresh string, elapsed float64) - ObserveClientTeamSwitchDuration(platform, agent, fresh string, elapsed float64) - ObserveClientRHSLoadDuration(platform, agent string, elapsed float64) - ObserveGlobalThreadsLoadDuration(platform, agent string, elapsed float64) + ObserveClientChannelSwitchDuration(platform, agent, fresh, userID string, elapsed float64) + ObserveClientTeamSwitchDuration(platform, agent, fresh, userID string, elapsed float64) + ObserveClientRHSLoadDuration(platform, agent, userID string, elapsed float64) + ObserveGlobalThreadsLoadDuration(platform, agent, userID string, elapsed float64) ObserveMobileClientLoadDuration(platform string, elapsed float64) ObserveMobileClientChannelSwitchDuration(platform string, elapsed float64) ObserveMobileClientTeamSwitchDuration(platform string, elapsed float64) diff --git a/server/einterfaces/mocks/MetricsInterface.go b/server/einterfaces/mocks/MetricsInterface.go index 95c2b6462f9..b0919c55cf2 100644 --- a/server/einterfaces/mocks/MetricsInterface.go +++ b/server/einterfaces/mocks/MetricsInterface.go @@ -78,9 +78,9 @@ func (_m *MetricsInterface) IncrementChannelIndexCounter() { _m.Called() } -// IncrementClientLongTasks provides a mock function with given fields: platform, agent, inc -func (_m *MetricsInterface) IncrementClientLongTasks(platform string, agent string, inc float64) { - _m.Called(platform, agent, inc) +// IncrementClientLongTasks provides a mock function with given fields: platform, agent, userID, inc +func (_m *MetricsInterface) IncrementClientLongTasks(platform string, agent string, userID string, inc float64) { + _m.Called(platform, agent, userID, inc) } // IncrementClusterEventType provides a mock function with given fields: eventType @@ -303,29 +303,29 @@ func (_m *MetricsInterface) ObserveAPIEndpointDuration(endpoint string, method s _m.Called(endpoint, method, statusCode, originClient, pageLoadContext, elapsed) } -// ObserveClientChannelSwitchDuration provides a mock function with given fields: platform, agent, fresh, elapsed -func (_m *MetricsInterface) ObserveClientChannelSwitchDuration(platform string, agent string, fresh string, elapsed float64) { - _m.Called(platform, agent, fresh, elapsed) +// ObserveClientChannelSwitchDuration provides a mock function with given fields: platform, agent, fresh, userID, elapsed +func (_m *MetricsInterface) ObserveClientChannelSwitchDuration(platform string, agent string, fresh string, userID string, elapsed float64) { + _m.Called(platform, agent, fresh, userID, elapsed) } -// ObserveClientCumulativeLayoutShift provides a mock function with given fields: platform, agent, elapsed -func (_m *MetricsInterface) ObserveClientCumulativeLayoutShift(platform string, agent string, elapsed float64) { - _m.Called(platform, agent, elapsed) +// ObserveClientCumulativeLayoutShift provides a mock function with given fields: platform, agent, userID, elapsed +func (_m *MetricsInterface) ObserveClientCumulativeLayoutShift(platform string, agent string, userID string, elapsed float64) { + _m.Called(platform, agent, userID, elapsed) } -// ObserveClientFirstContentfulPaint provides a mock function with given fields: platform, agent, elapsed -func (_m *MetricsInterface) ObserveClientFirstContentfulPaint(platform string, agent string, elapsed float64) { - _m.Called(platform, agent, elapsed) +// ObserveClientFirstContentfulPaint provides a mock function with given fields: platform, agent, userID, elapsed +func (_m *MetricsInterface) ObserveClientFirstContentfulPaint(platform string, agent string, userID string, elapsed float64) { + _m.Called(platform, agent, userID, elapsed) } -// ObserveClientInteractionToNextPaint provides a mock function with given fields: platform, agent, interaction, elapsed -func (_m *MetricsInterface) ObserveClientInteractionToNextPaint(platform string, agent string, interaction string, elapsed float64) { - _m.Called(platform, agent, interaction, elapsed) +// ObserveClientInteractionToNextPaint provides a mock function with given fields: platform, agent, interaction, userID, elapsed +func (_m *MetricsInterface) ObserveClientInteractionToNextPaint(platform string, agent string, interaction string, userID string, elapsed float64) { + _m.Called(platform, agent, interaction, userID, elapsed) } -// ObserveClientLargestContentfulPaint provides a mock function with given fields: platform, agent, region, elapsed -func (_m *MetricsInterface) ObserveClientLargestContentfulPaint(platform string, agent string, region string, elapsed float64) { - _m.Called(platform, agent, region, elapsed) +// ObserveClientLargestContentfulPaint provides a mock function with given fields: platform, agent, region, userID, elapsed +func (_m *MetricsInterface) ObserveClientLargestContentfulPaint(platform string, agent string, region string, userID string, elapsed float64) { + _m.Called(platform, agent, region, userID, elapsed) } // ObserveClientPageLoadDuration provides a mock function with given fields: platform, agent, userID, elapsed @@ -333,9 +333,9 @@ func (_m *MetricsInterface) ObserveClientPageLoadDuration(platform string, agent _m.Called(platform, agent, userID, elapsed) } -// ObserveClientRHSLoadDuration provides a mock function with given fields: platform, agent, elapsed -func (_m *MetricsInterface) ObserveClientRHSLoadDuration(platform string, agent string, elapsed float64) { - _m.Called(platform, agent, elapsed) +// ObserveClientRHSLoadDuration provides a mock function with given fields: platform, agent, userID, elapsed +func (_m *MetricsInterface) ObserveClientRHSLoadDuration(platform string, agent string, userID string, elapsed float64) { + _m.Called(platform, agent, userID, elapsed) } // ObserveClientSplashScreenEnd provides a mock function with given fields: platform, agent, pageType, userID, elapsed @@ -343,9 +343,9 @@ func (_m *MetricsInterface) ObserveClientSplashScreenEnd(platform string, agent _m.Called(platform, agent, pageType, userID, elapsed) } -// ObserveClientTeamSwitchDuration provides a mock function with given fields: platform, agent, fresh, elapsed -func (_m *MetricsInterface) ObserveClientTeamSwitchDuration(platform string, agent string, fresh string, elapsed float64) { - _m.Called(platform, agent, fresh, elapsed) +// ObserveClientTeamSwitchDuration provides a mock function with given fields: platform, agent, fresh, userID, elapsed +func (_m *MetricsInterface) ObserveClientTeamSwitchDuration(platform string, agent string, fresh string, userID string, elapsed float64) { + _m.Called(platform, agent, fresh, userID, elapsed) } // ObserveClientTimeToDomInteractive provides a mock function with given fields: platform, agent, userID, elapsed @@ -388,9 +388,9 @@ func (_m *MetricsInterface) ObserveFilesSearchDuration(elapsed float64) { _m.Called(elapsed) } -// ObserveGlobalThreadsLoadDuration provides a mock function with given fields: platform, agent, elapsed -func (_m *MetricsInterface) ObserveGlobalThreadsLoadDuration(platform string, agent string, elapsed float64) { - _m.Called(platform, agent, elapsed) +// ObserveGlobalThreadsLoadDuration provides a mock function with given fields: platform, agent, userID, elapsed +func (_m *MetricsInterface) ObserveGlobalThreadsLoadDuration(platform string, agent string, userID string, elapsed float64) { + _m.Called(platform, agent, userID, elapsed) } // ObserveMobileClientChannelSwitchDuration provides a mock function with given fields: platform, elapsed diff --git a/server/enterprise/metrics/metrics.go b/server/enterprise/metrics/metrics.go index c5491366e31..84bb6836bed 100644 --- a/server/enterprise/metrics/metrics.go +++ b/server/enterprise/metrics/metrics.go @@ -55,6 +55,8 @@ type MetricsInterfaceImpl struct { Registry *prometheus.Registry + ClientSideUserIds map[string]bool + DbMasterConnectionsGauge prometheus.GaugeFunc DbReadConnectionsGauge prometheus.GaugeFunc DbSearchConnectionsGauge prometheus.GaugeFunc @@ -240,7 +242,7 @@ func init() { }) } -// New creates a new MetricsInterface. The driver and datasoruce parameters are added during +// New creates a new MetricsInterface. The driver and datasource parameters are added during // migrating configuration store to the new platform service. Once the store and license are migrated, // we will be able to remove server dependency and lean on platform service during initialization. func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterfaceImpl { @@ -248,6 +250,12 @@ func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterf Platform: ps, } + // Initialize ClientSideUserIds map + m.ClientSideUserIds = make(map[string]bool) + for _, userId := range ps.Config().MetricsSettings.ClientSideUserIds { + m.ClientSideUserIds[userId] = true + } + m.Registry = prometheus.NewRegistry() options := collectors.ProcessCollectorOpts{ Namespace: MetricsNamespace, @@ -1196,7 +1204,7 @@ func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterf Help: "Duration from when a browser starts to request a page from a server until when it starts to receive data in response (seconds)", ConstLabels: additionalLabels, }, - []string{"platform", "agent"}, + []string{"platform", "agent", "user_id"}, m.Platform.Log(), ) m.Registry.MustRegister(m.ClientTimeToFirstByte) @@ -1209,7 +1217,7 @@ func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterf Help: "Duration from when a browser starts to request a page from a server until when it receives the last byte of the resource or immediately before the transport connection is closed, whichever comes first. (seconds)", ConstLabels: additionalLabels, }, - []string{"platform", "agent"}, + []string{"platform", "agent", "user_id"}, m.Platform.Log(), ) m.Registry.MustRegister(m.ClientTimeToLastByte) @@ -1223,7 +1231,7 @@ func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterf Buckets: []float64{.1, .25, .5, 1, 2.5, 5, 7.5, 10, 12.5, 15}, ConstLabels: additionalLabels, }, - []string{"platform", "agent"}, + []string{"platform", "agent", "user_id"}, m.Platform.Log(), ) m.Registry.MustRegister(m.ClientTimeToDOMInteractive) @@ -1237,7 +1245,7 @@ func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterf Buckets: []float64{.1, .25, .5, 1, 2.5, 5, 7.5, 10, 12.5, 15}, ConstLabels: additionalLabels, }, - []string{"platform", "agent", "page_type"}, + []string{"platform", "agent", "page_type", "user_id"}, m.Platform.Log(), ) m.Registry.MustRegister(m.ClientSplashScreenEnd) @@ -1253,7 +1261,7 @@ func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterf Buckets: []float64{.005, .01, .025, .05, .1, .25, .5, 1, 2.5, 5, 10, 15, 20}, ConstLabels: additionalLabels, }, - []string{"platform", "agent"}, + []string{"platform", "agent", "user_id"}, ) m.Registry.MustRegister(m.ClientFirstContentfulPaint) @@ -1268,7 +1276,7 @@ func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterf Buckets: []float64{.005, .01, .025, .05, .1, .25, .5, 1, 2.5, 5, 10, 15, 20}, ConstLabels: additionalLabels, }, - []string{"platform", "agent", "region"}, + []string{"platform", "agent", "region", "user_id"}, ) m.Registry.MustRegister(m.ClientLargestContentfulPaint) @@ -1280,7 +1288,7 @@ func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterf Help: "Measure of how long it takes for a user to see the effects of clicking with a mouse, tapping with a touchscreen, or pressing a key on the keyboard (seconds)", ConstLabels: additionalLabels, }, - []string{"platform", "agent", "interaction"}, + []string{"platform", "agent", "interaction", "user_id"}, ) m.Registry.MustRegister(m.ClientInteractionToNextPaint) @@ -1292,7 +1300,7 @@ func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterf Help: "Measure of how much a page's content shifts unexpectedly", ConstLabels: additionalLabels, }, - []string{"platform", "agent"}, + []string{"platform", "agent", "user_id"}, ) m.Registry.MustRegister(m.ClientCumulativeLayoutShift) @@ -1304,7 +1312,7 @@ func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterf Help: "Counter of the number of times that the browser's main UI thread is blocked for more than 50ms by a single task", ConstLabels: additionalLabels, }, - []string{"platform", "agent"}, + []string{"platform", "agent", "user_id"}, ) m.Registry.MustRegister(m.ClientLongTasks) @@ -1317,7 +1325,7 @@ func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterf Buckets: []float64{.005, .01, .025, .05, .1, .25, .5, 1, 2.5, 5, 10, 20, 40}, ConstLabels: additionalLabels, }, - []string{"platform", "agent"}, + []string{"platform", "agent", "user_id"}, m.Platform.Log(), ) m.Registry.MustRegister(m.ClientPageLoadDuration) @@ -1330,7 +1338,7 @@ func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterf Help: "Duration of the time taken from when a user clicks on a channel in the LHS to when posts in that channel become visible (seconds)", ConstLabels: additionalLabels, }, - []string{"platform", "agent", "fresh"}, + []string{"platform", "agent", "fresh", "user_id"}, ) m.Registry.MustRegister(m.ClientChannelSwitchDuration) @@ -1342,7 +1350,7 @@ func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterf Help: "Duration of the time taken from when a user clicks on a team in the LHS to when posts in that team become visible (seconds)", ConstLabels: additionalLabels, }, - []string{"platform", "agent", "fresh"}, + []string{"platform", "agent", "fresh", "user_id"}, ) m.Registry.MustRegister(m.ClientTeamSwitchDuration) @@ -1354,7 +1362,7 @@ func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterf Help: "Duration of the time taken from when a user clicks to open a thread in the RHS until when posts in that thread become visible (seconds)", ConstLabels: additionalLabels, }, - []string{"platform", "agent"}, + []string{"platform", "agent", "user_id"}, ) m.Registry.MustRegister(m.ClientRHSLoadDuration) @@ -1366,7 +1374,7 @@ func New(ps *platform.PlatformService, driver, dataSource string) *MetricsInterf Help: "Duration of the time taken from when a user clicks to open Threads in the LHS until when the global threads view becomes visible (milliseconds)", ConstLabels: additionalLabels, }, - []string{"platform", "agent"}, + []string{"platform", "agent", "user_id"}, ) m.Registry.MustRegister(m.ClientGlobalThreadsLoadDuration) @@ -2024,63 +2032,81 @@ func (mi *MetricsInterfaceImpl) DecrementHTTPWebSockets(originClient string) { mi.HTTPWebsocketsGauge.With(prometheus.Labels{"origin_client": originClient}).Dec() } +func (mi *MetricsInterfaceImpl) getEffectiveUserID(userID string) string { + if mi.ClientSideUserIds[userID] { + return userID + } + return "" +} + func (mi *MetricsInterfaceImpl) ObserveClientTimeToFirstByte(platform, agent, userID string, elapsed float64) { - mi.ClientTimeToFirstByte.With(prometheus.Labels{"platform": platform, "agent": agent}, userID).Observe(elapsed) + effectiveUserID := mi.getEffectiveUserID(userID) + mi.ClientTimeToFirstByte.With(prometheus.Labels{"platform": platform, "agent": agent, "user_id": effectiveUserID}, userID).Observe(elapsed) } func (mi *MetricsInterfaceImpl) ObserveClientTimeToLastByte(platform, agent, userID string, elapsed float64) { - mi.ClientTimeToLastByte.With(prometheus.Labels{"platform": platform, "agent": agent}, userID).Observe(elapsed) + effectiveUserID := mi.getEffectiveUserID(userID) + mi.ClientTimeToLastByte.With(prometheus.Labels{"platform": platform, "agent": agent, "user_id": effectiveUserID}, userID).Observe(elapsed) } func (mi *MetricsInterfaceImpl) ObserveClientTimeToDomInteractive(platform, agent, userID string, elapsed float64) { - mi.ClientTimeToDOMInteractive.With(prometheus.Labels{"platform": platform, "agent": agent}, userID).Observe(elapsed) + effectiveUserID := mi.getEffectiveUserID(userID) + mi.ClientTimeToDOMInteractive.With(prometheus.Labels{"platform": platform, "agent": agent, "user_id": effectiveUserID}, userID).Observe(elapsed) } func (mi *MetricsInterfaceImpl) ObserveClientSplashScreenEnd(platform, agent, pageType, userID string, elapsed float64) { - mi.ClientSplashScreenEnd.With(prometheus.Labels{"platform": platform, "agent": agent, "page_type": pageType}, userID).Observe(elapsed) + effectiveUserID := mi.getEffectiveUserID(userID) + mi.ClientSplashScreenEnd.With(prometheus.Labels{"platform": platform, "agent": agent, "page_type": pageType, "user_id": effectiveUserID}, userID).Observe(elapsed) } -func (mi *MetricsInterfaceImpl) ObserveClientFirstContentfulPaint(platform, agent string, elapsed float64) { - mi.ClientFirstContentfulPaint.With(prometheus.Labels{"platform": platform, "agent": agent}).Observe(elapsed) +func (mi *MetricsInterfaceImpl) ObserveClientFirstContentfulPaint(platform, agent, userID string, elapsed float64) { + effectiveUserID := mi.getEffectiveUserID(userID) + mi.ClientFirstContentfulPaint.With(prometheus.Labels{"platform": platform, "agent": agent, "user_id": effectiveUserID}).Observe(elapsed) } -func (mi *MetricsInterfaceImpl) ObserveClientLargestContentfulPaint(platform, agent, region string, elapsed float64) { - mi.ClientLargestContentfulPaint.With(prometheus.Labels{"platform": platform, "agent": agent, "region": region}).Observe(elapsed) +func (mi *MetricsInterfaceImpl) ObserveClientLargestContentfulPaint(platform, agent, region, userID string, elapsed float64) { + effectiveUserID := mi.getEffectiveUserID(userID) + mi.ClientLargestContentfulPaint.With(prometheus.Labels{"platform": platform, "agent": agent, "region": region, "user_id": effectiveUserID}).Observe(elapsed) } -func (mi *MetricsInterfaceImpl) ObserveClientInteractionToNextPaint(platform, agent, interaction string, elapsed float64) { - mi.ClientInteractionToNextPaint.With(prometheus.Labels{"platform": platform, "agent": agent, "interaction": interaction}).Observe(elapsed) +func (mi *MetricsInterfaceImpl) ObserveClientInteractionToNextPaint(platform, agent, interaction, userID string, elapsed float64) { + effectiveUserID := mi.getEffectiveUserID(userID) + mi.ClientInteractionToNextPaint.With(prometheus.Labels{"platform": platform, "agent": agent, "interaction": interaction, "user_id": effectiveUserID}).Observe(elapsed) } -func (mi *MetricsInterfaceImpl) ObserveClientCumulativeLayoutShift(platform, agent string, elapsed float64) { - mi.ClientCumulativeLayoutShift.With(prometheus.Labels{"platform": platform, "agent": agent}).Observe(elapsed) +func (mi *MetricsInterfaceImpl) ObserveClientCumulativeLayoutShift(platform, agent, userID string, elapsed float64) { + effectiveUserID := mi.getEffectiveUserID(userID) + mi.ClientCumulativeLayoutShift.With(prometheus.Labels{"platform": platform, "agent": agent, "user_id": effectiveUserID}).Observe(elapsed) } -func (mi *MetricsInterfaceImpl) IncrementClientLongTasks(platform, agent string, inc float64) { - mi.ClientLongTasks.With(prometheus.Labels{"platform": platform, "agent": agent}).Add(inc) +func (mi *MetricsInterfaceImpl) IncrementClientLongTasks(platform, agent, userID string, inc float64) { + effectiveUserID := mi.getEffectiveUserID(userID) + mi.ClientLongTasks.With(prometheus.Labels{"platform": platform, "agent": agent, "user_id": effectiveUserID}).Add(inc) } func (mi *MetricsInterfaceImpl) ObserveClientPageLoadDuration(platform, agent, userID string, elapsed float64) { - mi.ClientPageLoadDuration.With(prometheus.Labels{ - "platform": platform, - "agent": agent, - }, userID).Observe(elapsed) + effectiveUserID := mi.getEffectiveUserID(userID) + mi.ClientPageLoadDuration.With(prometheus.Labels{"platform": platform, "agent": agent, "user_id": effectiveUserID}, userID).Observe(elapsed) } -func (mi *MetricsInterfaceImpl) ObserveClientChannelSwitchDuration(platform, agent, fresh string, elapsed float64) { - mi.ClientChannelSwitchDuration.With(prometheus.Labels{"platform": platform, "agent": agent, "fresh": fresh}).Observe(elapsed) +func (mi *MetricsInterfaceImpl) ObserveClientChannelSwitchDuration(platform, agent, fresh, userID string, elapsed float64) { + effectiveUserID := mi.getEffectiveUserID(userID) + mi.ClientChannelSwitchDuration.With(prometheus.Labels{"platform": platform, "agent": agent, "fresh": fresh, "user_id": effectiveUserID}).Observe(elapsed) } -func (mi *MetricsInterfaceImpl) ObserveClientTeamSwitchDuration(platform, agent, fresh string, elapsed float64) { - mi.ClientTeamSwitchDuration.With(prometheus.Labels{"platform": platform, "agent": agent, "fresh": fresh}).Observe(elapsed) +func (mi *MetricsInterfaceImpl) ObserveClientTeamSwitchDuration(platform, agent, fresh, userID string, elapsed float64) { + effectiveUserID := mi.getEffectiveUserID(userID) + mi.ClientTeamSwitchDuration.With(prometheus.Labels{"platform": platform, "agent": agent, "fresh": fresh, "user_id": effectiveUserID}).Observe(elapsed) } -func (mi *MetricsInterfaceImpl) ObserveClientRHSLoadDuration(platform, agent string, elapsed float64) { - mi.ClientRHSLoadDuration.With(prometheus.Labels{"platform": platform, "agent": agent}).Observe(elapsed) +func (mi *MetricsInterfaceImpl) ObserveClientRHSLoadDuration(platform, agent, userID string, elapsed float64) { + effectiveUserID := mi.getEffectiveUserID(userID) + mi.ClientRHSLoadDuration.With(prometheus.Labels{"platform": platform, "agent": agent, "user_id": effectiveUserID}).Observe(elapsed) } -func (mi *MetricsInterfaceImpl) ObserveGlobalThreadsLoadDuration(platform, agent string, elapsed float64) { - mi.ClientGlobalThreadsLoadDuration.With(prometheus.Labels{"platform": platform, "agent": agent}).Observe(elapsed) +func (mi *MetricsInterfaceImpl) ObserveGlobalThreadsLoadDuration(platform, agent, userID string, elapsed float64) { + effectiveUserID := mi.getEffectiveUserID(userID) + mi.ClientGlobalThreadsLoadDuration.With(prometheus.Labels{"platform": platform, "agent": agent, "user_id": effectiveUserID}).Observe(elapsed) } func (mi *MetricsInterfaceImpl) ObserveDesktopCpuUsage(platform, version, process string, usage float64) { diff --git a/server/i18n/en.json b/server/i18n/en.json index 781bae8c767..bf574dcf44f 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -9076,6 +9076,14 @@ "id": "model.config.is_valid.message_export.global_relay.smtp_username.app_error", "translation": "Message export job GlobalRelaySettings.SmtpUsername must be set." }, + { + "id": "model.config.is_valid.metrics_client_side_user_id.app_error", + "translation": "Invalid client side user id: {{.Id}}" + }, + { + "id": "model.config.is_valid.metrics_client_side_user_ids.app_error", + "translation": "Number of elements in ClientSideUserIds {{.CurrentLength}} is higher than maximum limit of {{.MaxLength}}." + }, { "id": "model.config.is_valid.move_thread.domain_invalid.app_error", "translation": "Invalid domain for move thread settings" @@ -9426,7 +9434,7 @@ }, { "id": "model.incoming_hook.id.app_error", - "translation": "Invalid Id." + "translation": "Invalid Id: {{.Id}}." }, { "id": "model.incoming_hook.parse_data.app_error", diff --git a/server/public/model/config.go b/server/public/model/config.go index 161a6146640..2e0236b5661 100644 --- a/server/public/model/config.go +++ b/server/public/model/config.go @@ -1071,11 +1071,12 @@ func (s *ClusterSettings) SetDefaults() { } type MetricsSettings struct { - Enable *bool `access:"environment_performance_monitoring,write_restrictable,cloud_restrictable"` - BlockProfileRate *int `access:"environment_performance_monitoring,write_restrictable,cloud_restrictable"` - ListenAddress *string `access:"environment_performance_monitoring,write_restrictable,cloud_restrictable"` // telemetry: none - EnableClientMetrics *bool `access:"environment_performance_monitoring,write_restrictable,cloud_restrictable"` - EnableNotificationMetrics *bool `access:"environment_performance_monitoring,write_restrictable,cloud_restrictable"` + Enable *bool `access:"environment_performance_monitoring,write_restrictable,cloud_restrictable"` + BlockProfileRate *int `access:"environment_performance_monitoring,write_restrictable,cloud_restrictable"` + ListenAddress *string `access:"environment_performance_monitoring,write_restrictable,cloud_restrictable"` // telemetry: none + EnableClientMetrics *bool `access:"environment_performance_monitoring,write_restrictable,cloud_restrictable"` + EnableNotificationMetrics *bool `access:"environment_performance_monitoring,write_restrictable,cloud_restrictable"` + ClientSideUserIds []string `access:"environment_performance_monitoring,write_restrictable,cloud_restrictable"` // telemetry: none } func (s *MetricsSettings) SetDefaults() { @@ -1098,6 +1099,23 @@ func (s *MetricsSettings) SetDefaults() { if s.EnableNotificationMetrics == nil { s.EnableNotificationMetrics = NewPointer(true) } + + if s.ClientSideUserIds == nil { + s.ClientSideUserIds = []string{} + } +} + +func (s *MetricsSettings) isValid() *AppError { + const maxLength = 5 + if len(s.ClientSideUserIds) > maxLength { + return NewAppError("MetricsSettings.IsValid", "model.config.is_valid.metrics_client_side_user_ids.app_error", map[string]any{"MaxLength": maxLength, "CurrentLength": len(s.ClientSideUserIds)}, "", http.StatusBadRequest) + } + for _, id := range s.ClientSideUserIds { + if !IsValidId(id) { + return NewAppError("MetricsSettings.IsValid", "model.config.is_valid.metrics_client_side_user_id.app_error", map[string]any{"Id": id}, "", http.StatusBadRequest) + } + } + return nil } type ExperimentalSettings struct { @@ -3806,6 +3824,10 @@ func (o *Config) IsValid() *AppError { return NewAppError("Config.IsValid", "model.config.is_valid.cluster_email_batching.app_error", nil, "", http.StatusBadRequest) } + if appErr := o.MetricsSettings.isValid(); appErr != nil { + return appErr + } + if appErr := o.CacheSettings.isValid(); appErr != nil { return appErr } diff --git a/server/public/model/incoming_webhook.go b/server/public/model/incoming_webhook.go index 81b035434e8..4fcdbf381e7 100644 --- a/server/public/model/incoming_webhook.go +++ b/server/public/model/incoming_webhook.go @@ -66,7 +66,7 @@ type IncomingWebhooksWithCount struct { func (o *IncomingWebhook) IsValid() *AppError { if !IsValidId(o.Id) { - return NewAppError("IncomingWebhook.IsValid", "model.incoming_hook.id.app_error", nil, "", http.StatusBadRequest) + return NewAppError("IncomingWebhook.IsValid", "model.incoming_hook.id.app_error", map[string]any{"Id": o.Id}, "", http.StatusBadRequest) } if o.CreateAt == 0 { diff --git a/webapp/channels/src/components/admin_console/__snapshots__/client_side_userids_setting.test.tsx.snap b/webapp/channels/src/components/admin_console/__snapshots__/client_side_userids_setting.test.tsx.snap new file mode 100644 index 00000000000..9df71e9dba5 --- /dev/null +++ b/webapp/channels/src/components/admin_console/__snapshots__/client_side_userids_setting.test.tsx.snap @@ -0,0 +1,470 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`components/AdminConsole/ClientSideUserIdsSetting initial state with multiple items 1`] = ` + + + } + inputId="MySetting" + label={ + + } + setByEnv={false} + > +
+ +
+ + + +
+ + + Set the user ids you want to track for client side metrics. Separate values with a comma. + + +
+
+
+ + +`; + +exports[`components/AdminConsole/ClientSideUserIdsSetting initial state with no items 1`] = ` + + + } + inputId="MySetting" + label={ + + } + setByEnv={false} + > +
+ +
+ + + +
+ + + Set the user ids you want to track for client side metrics. Separate values with a comma. + + +
+
+
+ + +`; + +exports[`components/AdminConsole/ClientSideUserIdsSetting initial state with one item 1`] = ` + + + } + inputId="MySetting" + label={ + + } + setByEnv={false} + > +
+ +
+ + + +
+ + + Set the user ids you want to track for client side metrics. Separate values with a comma. + + +
+
+
+ + +`; + +exports[`components/AdminConsole/ClientSideUserIdsSetting renders properly when disabled 1`] = ` + + + } + inputId="MySetting" + label={ + + } + setByEnv={false} + > +
+ +
+ + + +
+ + + Set the user ids you want to track for client side metrics. Separate values with a comma. + + +
+
+
+ + +`; + +exports[`components/AdminConsole/ClientSideUserIdsSetting renders properly when set by environment variable 1`] = ` + + + } + inputId="MySetting" + label={ + + } + setByEnv={true} + > +
+ +
+ + + +
+ + + Set the user ids you want to track for client side metrics. Separate values with a comma. + + +
+ +
+ + + This setting has been set through an environment variable. It cannot be changed through the System Console. + + +
+ +
+
+ + +`; diff --git a/webapp/channels/src/components/admin_console/admin_definition.tsx b/webapp/channels/src/components/admin_console/admin_definition.tsx index 47220aed712..ac5f0fed6d1 100644 --- a/webapp/channels/src/components/admin_console/admin_definition.tsx +++ b/webapp/channels/src/components/admin_console/admin_definition.tsx @@ -49,6 +49,7 @@ import CompanyInfo, {searchableStrings as billingCompanyInfoSearchableStrings} f import CompanyInfoEdit from './billing/company_info_edit'; import BleveSettings, {searchableStrings as bleveSearchableStrings} from './bleve_settings'; import BrandImageSetting from './brand_image_setting/brand_image_setting'; +import ClientSideUserIdsSetting from './client_side_userids_setting'; import ClusterSettings, {searchableStrings as clusterSearchableStrings} from './cluster_settings'; import CustomEnableDisableGuestAccountsSetting from './custom_enable_disable_guest_accounts_setting'; import CustomTermsOfServiceSettings from './custom_terms_of_service_settings'; @@ -1963,6 +1964,15 @@ const AdminDefinition: AdminDefinitionType = { it.configIsFalse('MetricsSettings', 'Enable'), ), }, + { + type: 'custom', + key: 'MetricsSettings.ClientSideUserIds', + component: ClientSideUserIdsSetting, + isDisabled: it.any( + it.not(it.userHasWritePermissionOnResource(RESOURCE_KEYS.ENVIRONMENT.PERFORMANCE_MONITORING)), + it.configIsFalse('MetricsSettings', 'EnableClientMetrics'), + ), + }, { type: 'text', key: 'MetricsSettings.ListenAddress', diff --git a/webapp/channels/src/components/admin_console/client_side_userids_setting.test.tsx b/webapp/channels/src/components/admin_console/client_side_userids_setting.test.tsx new file mode 100644 index 00000000000..f7632b7582f --- /dev/null +++ b/webapp/channels/src/components/admin_console/client_side_userids_setting.test.tsx @@ -0,0 +1,133 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; + +import {mountWithIntl} from 'tests/helpers/intl-test-helper'; + +import ClientSideUserIdsSetting from './client_side_userids_setting'; + +describe('components/AdminConsole/ClientSideUserIdsSetting', () => { + const baseProps = { + id: 'MySetting', + value: ['userid1', 'userid2'], + onChange: jest.fn(), + disabled: false, + setByEnv: false, + }; + + describe('initial state', () => { + test('with no items', () => { + const props = { + ...baseProps, + value: [], + }; + + const wrapper = mountWithIntl( + , + ); + expect(wrapper).toMatchSnapshot(); + + expect(wrapper.state('value')).toEqual(''); + }); + + test('with one item', () => { + const props = { + ...baseProps, + value: ['userid1'], + }; + + const wrapper = mountWithIntl( + , + ); + expect(wrapper).toMatchSnapshot(); + + expect(wrapper.state('value')).toEqual('userid1'); + }); + + test('with multiple items', () => { + const props = { + ...baseProps, + value: ['userid1', 'userid2', 'id3'], + }; + + const wrapper = mountWithIntl( + , + ); + expect(wrapper).toMatchSnapshot(); + + expect(wrapper.state('value')).toEqual('userid1,userid2,id3'); + }); + }); + + describe('onChange', () => { + test('called on change to empty', () => { + const props = { + ...baseProps, + onChange: jest.fn(), + }; + + const wrapper = mountWithIntl( + , + ); + + wrapper.find('input').simulate('change', {target: {value: ''}}); + + expect(props.onChange).toBeCalledWith(baseProps.id, []); + }); + + test('called on change to one item', () => { + const props = { + ...baseProps, + onChange: jest.fn(), + }; + + const wrapper = mountWithIntl( + , + ); + + wrapper.find('input').simulate('change', {target: {value: ' id2 '}}); + + expect(props.onChange).toBeCalledWith(baseProps.id, ['id2']); + }); + + test('called on change to two items', () => { + const props = { + ...baseProps, + onChange: jest.fn(), + }; + + const wrapper = mountWithIntl( + , + ); + + wrapper.find('input').simulate('change', {target: {value: 'id1, id99'}}); + + expect(props.onChange).toBeCalledWith(baseProps.id, ['id1', 'id99']); + }); + }); + + test('renders properly when disabled', () => { + const props = { + ...baseProps, + disabled: true, + }; + + const wrapper = mountWithIntl( + , + ); + expect(wrapper).toMatchSnapshot(); + }); + + test('renders properly when set by environment variable', () => { + const props = { + ...baseProps, + setByEnv: true, + }; + + const wrapper = mountWithIntl( + , + ); + expect(wrapper).toMatchSnapshot(); + }); +}); diff --git a/webapp/channels/src/components/admin_console/client_side_userids_setting.tsx b/webapp/channels/src/components/admin_console/client_side_userids_setting.tsx new file mode 100644 index 00000000000..81422f4dca3 --- /dev/null +++ b/webapp/channels/src/components/admin_console/client_side_userids_setting.tsx @@ -0,0 +1,81 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React, {PureComponent} from 'react'; +import type {ChangeEvent} from 'react'; +import {defineMessage, FormattedMessage} from 'react-intl'; + +import LocalizedPlaceholderInput from 'components/localized_placeholder_input'; + +import Setting from './setting'; + +type Props = { + id: string; + value: string[]; + onChange: (id: string, valueAsArray: string[]) => void; + disabled: boolean; + setByEnv: boolean; +} + +type State = { + value: string; +} + +export default class ClientSideUserIdsSetting extends PureComponent { + constructor(props: Props) { + super(props); + + this.state = { + value: this.arrayToString(props.value), + }; + } + + stringToArray = (str: string): string[] => { + return str.split(',').map((s) => s.trim()).filter(Boolean); + }; + + arrayToString = (arr: string[]): string => { + return arr.join(','); + }; + + handleChange = (e: ChangeEvent): void => { + const valueAsArray = this.stringToArray(e.target.value); + + this.props.onChange(this.props.id, valueAsArray); + + this.setState({ + value: e.target.value, + }); + }; + + render() { + return ( + + } + helpText={ + + } + inputId={this.props.id} + setByEnv={this.props.setByEnv} + > + + + ); + } +} diff --git a/webapp/channels/src/i18n/en.json b/webapp/channels/src/i18n/en.json index 9eebb9f0739..bab9930e8ba 100644 --- a/webapp/channels/src/i18n/en.json +++ b/webapp/channels/src/i18n/en.json @@ -656,6 +656,9 @@ "admin.customization.announcement.enableBannerTitle": "Enable System-wide Notifications:", "admin.customization.appDownloadLinkDesc": "Add a link to a download page for the Mattermost apps. When a link is present, an option to \"Download Mattermost Apps\" will be added in the Product Menu so users can find the download page. Leave this field blank to hide the option from the Product Menu.", "admin.customization.appDownloadLinkTitle": "Mattermost Apps Download Page Link:", + "admin.customization.clientSideUserIds": "Client side user ids:", + "admin.customization.clientSideUserIdsDesc": "Set the user ids you want to track for client side metrics. Separate values with a comma.", + "admin.customization.clientSideUserIdsPlaceholder": "E.g.: \"userid1,userid2\"", "admin.customization.customUrlSchemes": "Custom URL Schemes:", "admin.customization.customUrlSchemesDesc": "Allows message text to link if it begins with any of the comma-separated URL schemes listed. By default, the following schemes will create links: \"http\", \"https\", \"ftp\", \"tel\", and \"mailto\".", "admin.customization.customUrlSchemesPlaceholder": "E.g.: \"git,smtp\"", From 41e0f97176e59990a3b7913b74c8685facd53e37 Mon Sep 17 00:00:00 2001 From: Julien Tant <785518+JulienTant@users.noreply.github.com> Date: Thu, 13 Feb 2025 10:35:27 -0700 Subject: [PATCH 7/7] Remove call for removed attribute Page (#30207) --- server/channels/store/storetest/property_value_store.go | 1 - 1 file changed, 1 deletion(-) diff --git a/server/channels/store/storetest/property_value_store.go b/server/channels/store/storetest/property_value_store.go index de72b466757..cad73212ac6 100644 --- a/server/channels/store/storetest/property_value_store.go +++ b/server/channels/store/storetest/property_value_store.go @@ -463,7 +463,6 @@ func testUpsertPropertyValue(t *testing.T, _ request.CTX, ss store.Store) { // Verify the invalid value was not inserted results, err := ss.PropertyValue().SearchPropertyValues(model.PropertyValueSearchOpts{ TargetID: invalidValue.TargetID, - Page: 0, PerPage: 10, }) require.NoError(t, err)