From ad35eba60b5e2ac4dcbf7ab48c998255ca4f738d Mon Sep 17 00:00:00 2001 From: Harshil Sharma <18575143+harshilsharma63@users.noreply.github.com> Date: Mon, 6 Apr 2026 08:47:24 +0530 Subject: [PATCH] Added nil checks (#35755) * Added nil checks * Added test for DM and GM * Updated operation order --- server/channels/app/channel.go | 12 +- .../app/post_persistent_notification.go | 27 ++- .../app/post_persistent_notification_test.go | 218 ++++++++++++++++++ 3 files changed, 250 insertions(+), 7 deletions(-) diff --git a/server/channels/app/channel.go b/server/channels/app/channel.go index 15ea18039ad..59486ffc963 100644 --- a/server/channels/app/channel.go +++ b/server/channels/app/channel.go @@ -1616,6 +1616,12 @@ func (a *App) DeleteChannel(rctx request.CTX, channel *model.Channel, userID str map[string]any{"Reason": archiveRejectionReason}, "", http.StatusBadRequest) } + deleteAt := model.GetMillis() + + if err := a.Srv().Store().Channel().Delete(channel.Id, deleteAt); err != nil { + return model.NewAppError("DeleteChannel", "app.channel.delete.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + if user != nil { T := i18n.GetUserTranslations(user.Locale) @@ -1671,12 +1677,6 @@ func (a *App) DeleteChannel(rctx request.CTX, channel *model.Channel, userID str return model.NewAppError("DeleteChannel", "app.post_persistent_notification.delete_by_channel.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - deleteAt := model.GetMillis() - - if err := a.Srv().Store().Channel().Delete(channel.Id, deleteAt); err != nil { - return model.NewAppError("DeleteChannel", "app.channel.delete.app_error", nil, "", http.StatusInternalServerError).Wrap(err) - } - a.Srv().Platform().InvalidateCacheForChannel(channel) var message *model.WebSocketEvent diff --git a/server/channels/app/post_persistent_notification.go b/server/channels/app/post_persistent_notification.go index 0babd91bf99..ddbf59681e3 100644 --- a/server/channels/app/post_persistent_notification.go +++ b/server/channels/app/post_persistent_notification.go @@ -163,13 +163,25 @@ func (a *App) forEachPersistentNotificationPost(posts []*model.Post, fn func(pos return err } + var postsForPersistentNotificationCleanup []*model.Post + for _, post := range posts { channel := channelsMap[post.ChannelId] + if channel == nil { + postsForPersistentNotificationCleanup = append(postsForPersistentNotificationCleanup, post) + continue + } + team := teamsMap[channel.TeamId] // GMs and DMs don't belong to any team if channel.IsGroupOrDirect() { team = &model.Team{} + } else if team == nil { + // cleanup persistent notification for posts with missing teams when they are not DM or GM + postsForPersistentNotificationCleanup = append(postsForPersistentNotificationCleanup, post) + continue } + profileMap := channelProfileMap[channel.Id] // Ensure the sender is always in the profile map: for example, system admins can post @@ -210,6 +222,14 @@ func (a *App) forEachPersistentNotificationPost(posts []*model.Post, fn func(pos } } + if len(postsForPersistentNotificationCleanup) > 0 { + for _, post := range postsForPersistentNotificationCleanup { + if appErr := a.DeletePersistentNotification(request.EmptyContext(a.Log()), post); appErr != nil { + a.Log().Warn("Failed to delete persistent notification for post", mlog.String("post_id", post.Id), mlog.String("channel_id", post.ChannelId), mlog.Err(appErr)) + } + } + } + return nil } @@ -219,9 +239,14 @@ func (a *App) persistentNotificationsAuxiliaryData(channelsMap map[string]*model channelKeywords := make(map[string]MentionKeywords, len(channelsMap)) channelNotifyProps := make(map[string]map[string]model.StringMap, len(channelsMap)) for _, c := range channelsMap { + team := teamsMap[c.TeamId] + if team == nil && !c.IsGroupOrDirect() { + continue + } + // In DM, notifications can't be send to any 3rd person. if c.Type != model.ChannelTypeDirect { - groups, err := a.getGroupsAllowedForReferenceInChannel(c, teamsMap[c.TeamId]) + groups, err := a.getGroupsAllowedForReferenceInChannel(c, team) if err != nil { return nil, nil, nil, nil, errors.Wrapf(err, "failed to get profiles for channel %s", c.Id) } diff --git a/server/channels/app/post_persistent_notification_test.go b/server/channels/app/post_persistent_notification_test.go index e60dcab3bfb..94b2f6213c9 100644 --- a/server/channels/app/post_persistent_notification_test.go +++ b/server/channels/app/post_persistent_notification_test.go @@ -192,6 +192,224 @@ func TestDeletePersistentNotification(t *testing.T) { }) } +func TestForEachPersistentNotificationPost(t *testing.T) { + mainHelper.Parallel(t) + + t.Run("should cleanup posts whose channel no longer exists", func(t *testing.T) { + th := SetupWithStoreMock(t) + + user1 := &model.User{Id: "uid1", Username: "user-1"} + profileMap := map[string]*model.User{user1.Id: user1} + team := &model.Team{Id: "tid"} + channel := &model.Channel{Id: "chid", TeamId: team.Id, Type: model.ChannelTypeOpen} + + // post1 belongs to an existing channel; post2 belongs to a deleted/missing channel + post1 := &model.Post{Id: "pid1", ChannelId: channel.Id, Message: "hello @user-1", UserId: user1.Id} + post2 := &model.Post{Id: "pid2", ChannelId: "deleted-channel-id", Message: "hello", UserId: user1.Id} + + mockStore := th.App.Srv().Store().(*storemocks.Store) + + mockChannel := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannel) + // Only return channel for post1; post2's channel is missing + mockChannel.On("GetChannelsByIds", mock.Anything, mock.Anything).Return([]*model.Channel{channel}, nil) + mockChannel.On("GetAllChannelMembersNotifyPropsForChannel", mock.Anything, mock.Anything).Return(map[string]model.StringMap{}, nil) + + mockTeam := storemocks.TeamStore{} + mockStore.On("Team").Return(&mockTeam) + mockTeam.On("GetMany", mock.Anything).Return([]*model.Team{team}, nil) + + mockUser := storemocks.UserStore{} + mockStore.On("User").Return(&mockUser) + mockUser.On("GetAllProfilesInChannel", mock.Anything, mock.Anything, mock.Anything).Return(profileMap, nil) + + mockGroup := storemocks.GroupStore{} + mockStore.On("Group").Return(&mockGroup) + mockGroup.On("GetGroups", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Group{}, nil) + + // DeletePersistentNotification mocks - the cleanup path calls GetSingle then Delete + mockPostPersistentNotification := storemocks.PostPersistentNotificationStore{} + mockStore.On("PostPersistentNotification").Return(&mockPostPersistentNotification) + mockPostPersistentNotification.On("GetSingle", post2.Id).Return(&model.PostPersistentNotifications{PostId: post2.Id}, nil) + mockPostPersistentNotification.On("Delete", []string{post2.Id}).Return(nil) + + th.App.Srv().SetLicense(getLicWithSkuShortName(model.LicenseShortSkuProfessional)) + cfg := th.App.Config() + *cfg.ServiceSettings.PostPriority = true + *cfg.ServiceSettings.AllowPersistentNotifications = true + + fnCalled := []string{} + err := th.App.forEachPersistentNotificationPost([]*model.Post{post1, post2}, func(post *model.Post, _ *model.Channel, _ *model.Team, _ *MentionResults, _ model.UserMap, _ map[string]map[string]model.StringMap) error { + fnCalled = append(fnCalled, post.Id) + return nil + }) + require.NoError(t, err) + + // The callback should only be called for post1 (valid channel) + assert.Equal(t, []string{"pid1"}, fnCalled) + // post2 persistent notification should have been cleaned up + mockPostPersistentNotification.AssertCalled(t, "Delete", []string{post2.Id}) + }) + + t.Run("should cleanup posts whose team no longer exists", func(t *testing.T) { + th := SetupWithStoreMock(t) + + user1 := &model.User{Id: "uid1", Username: "user-1"} + user2 := &model.User{Id: "uid2", Username: "user-2"} + profileMap := map[string]*model.User{user1.Id: user1, user2.Id: user2} + team := &model.Team{Id: "tid"} + channel := &model.Channel{Id: "chid", TeamId: team.Id, Type: model.ChannelTypeOpen} + // channelWithMissingTeam has a TeamId that won't be in teamsMap + channelWithMissingTeam := &model.Channel{Id: "chid2", TeamId: "deleted-team-id", Type: model.ChannelTypeOpen} + + post1 := &model.Post{Id: "pid1", ChannelId: channel.Id, Message: "hello @user-1", UserId: user2.Id} + post2 := &model.Post{Id: "pid2", ChannelId: channelWithMissingTeam.Id, Message: "hello @user-1", UserId: user2.Id} + + mockStore := th.App.Srv().Store().(*storemocks.Store) + + mockChannel := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannel) + // Both channels exist, but only one team exists + mockChannel.On("GetChannelsByIds", mock.Anything, mock.Anything).Return([]*model.Channel{channel, channelWithMissingTeam}, nil) + mockChannel.On("GetAllChannelMembersNotifyPropsForChannel", mock.Anything, mock.Anything).Return(map[string]model.StringMap{}, nil) + + mockTeam := storemocks.TeamStore{} + mockStore.On("Team").Return(&mockTeam) + // Only return the team for channel, not for channelWithMissingTeam + mockTeam.On("GetMany", mock.Anything).Return([]*model.Team{team}, nil) + + mockUser := storemocks.UserStore{} + mockStore.On("User").Return(&mockUser) + mockUser.On("GetAllProfilesInChannel", mock.Anything, mock.Anything, mock.Anything).Return(profileMap, nil) + + mockGroup := storemocks.GroupStore{} + mockStore.On("Group").Return(&mockGroup) + mockGroup.On("GetGroups", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Group{}, nil) + + mockPostPersistentNotification := storemocks.PostPersistentNotificationStore{} + mockStore.On("PostPersistentNotification").Return(&mockPostPersistentNotification) + mockPostPersistentNotification.On("GetSingle", post2.Id).Return(&model.PostPersistentNotifications{PostId: post2.Id}, nil) + mockPostPersistentNotification.On("Delete", []string{post2.Id}).Return(nil) + + th.App.Srv().SetLicense(getLicWithSkuShortName(model.LicenseShortSkuProfessional)) + cfg := th.App.Config() + *cfg.ServiceSettings.PostPriority = true + *cfg.ServiceSettings.AllowPersistentNotifications = true + + fnCalled := []string{} + err := th.App.forEachPersistentNotificationPost([]*model.Post{post1, post2}, func(post *model.Post, _ *model.Channel, _ *model.Team, _ *MentionResults, _ model.UserMap, _ map[string]map[string]model.StringMap) error { + fnCalled = append(fnCalled, post.Id) + return nil + }) + require.NoError(t, err) + + // The callback should only be called for post1 (valid team) + assert.Equal(t, []string{"pid1"}, fnCalled) + // post2 persistent notification should have been cleaned up due to missing team + mockPostPersistentNotification.AssertCalled(t, "Delete", []string{post2.Id}) + }) + + t.Run("should not cleanup DM posts that have no team", func(t *testing.T) { + th := SetupWithStoreMock(t) + + user1 := &model.User{Id: "uid1", Username: "user-1"} + user2 := &model.User{Id: "uid2", Username: "user-2"} + profileMap := map[string]*model.User{user1.Id: user1, user2.Id: user2} + dmChannel := &model.Channel{Id: "dm-chid", TeamId: "", Type: model.ChannelTypeDirect, Name: model.GetDMNameFromIds(user1.Id, user2.Id)} + + post1 := &model.Post{Id: "pid1", ChannelId: dmChannel.Id, Message: "hello", UserId: user1.Id} + + mockStore := th.App.Srv().Store().(*storemocks.Store) + + mockChannel := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannel) + mockChannel.On("GetChannelsByIds", mock.Anything, mock.Anything).Return([]*model.Channel{dmChannel}, nil) + + mockTeam := storemocks.TeamStore{} + mockStore.On("Team").Return(&mockTeam) + mockTeam.On("GetMany", mock.Anything).Return([]*model.Team{}, nil) + + mockUser := storemocks.UserStore{} + mockStore.On("User").Return(&mockUser) + mockUser.On("GetAllProfilesInChannel", mock.Anything, mock.Anything, mock.Anything).Return(profileMap, nil) + + mockGroup := storemocks.GroupStore{} + mockStore.On("Group").Return(&mockGroup) + mockGroup.On("GetGroups", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Group{}, nil) + + mockPostPersistentNotification := storemocks.PostPersistentNotificationStore{} + mockStore.On("PostPersistentNotification").Return(&mockPostPersistentNotification) + + th.App.Srv().SetLicense(getLicWithSkuShortName(model.LicenseShortSkuProfessional)) + cfg := th.App.Config() + *cfg.ServiceSettings.PostPriority = true + *cfg.ServiceSettings.AllowPersistentNotifications = true + + fnCalled := []string{} + err := th.App.forEachPersistentNotificationPost([]*model.Post{post1}, func(post *model.Post, _ *model.Channel, _ *model.Team, _ *MentionResults, _ model.UserMap, _ map[string]map[string]model.StringMap) error { + fnCalled = append(fnCalled, post.Id) + return nil + }) + require.NoError(t, err) + + // The callback should be called for the DM post even though there's no team + assert.Equal(t, []string{"pid1"}, fnCalled) + // Delete should NOT have been called — DMs don't need a team + mockPostPersistentNotification.AssertNotCalled(t, "Delete", mock.Anything) + }) + + t.Run("should not cleanup GM posts that have no team", func(t *testing.T) { + th := SetupWithStoreMock(t) + + user1 := &model.User{Id: "uid1", Username: "user-1"} + user2 := &model.User{Id: "uid2", Username: "user-2"} + user3 := &model.User{Id: "uid3", Username: "user-3"} + profileMap := map[string]*model.User{user1.Id: user1, user2.Id: user2, user3.Id: user3} + gmChannel := &model.Channel{Id: "gm-chid", TeamId: "", Type: model.ChannelTypeGroup} + + post1 := &model.Post{Id: "pid1", ChannelId: gmChannel.Id, Message: "hello @user-2", UserId: user1.Id} + + mockStore := th.App.Srv().Store().(*storemocks.Store) + + mockChannel := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannel) + mockChannel.On("GetChannelsByIds", mock.Anything, mock.Anything).Return([]*model.Channel{gmChannel}, nil) + mockChannel.On("GetAllChannelMembersNotifyPropsForChannel", mock.Anything, mock.Anything).Return(map[string]model.StringMap{}, nil) + + mockTeam := storemocks.TeamStore{} + mockStore.On("Team").Return(&mockTeam) + mockTeam.On("GetMany", mock.Anything).Return([]*model.Team{}, nil) + + mockUser := storemocks.UserStore{} + mockStore.On("User").Return(&mockUser) + mockUser.On("GetAllProfilesInChannel", mock.Anything, mock.Anything, mock.Anything).Return(profileMap, nil) + + mockGroup := storemocks.GroupStore{} + mockStore.On("Group").Return(&mockGroup) + mockGroup.On("GetGroups", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Group{}, nil) + + mockPostPersistentNotification := storemocks.PostPersistentNotificationStore{} + mockStore.On("PostPersistentNotification").Return(&mockPostPersistentNotification) + + th.App.Srv().SetLicense(getLicWithSkuShortName(model.LicenseShortSkuProfessional)) + cfg := th.App.Config() + *cfg.ServiceSettings.PostPriority = true + *cfg.ServiceSettings.AllowPersistentNotifications = true + + fnCalled := []string{} + err := th.App.forEachPersistentNotificationPost([]*model.Post{post1}, func(post *model.Post, _ *model.Channel, _ *model.Team, _ *MentionResults, _ model.UserMap, _ map[string]map[string]model.StringMap) error { + fnCalled = append(fnCalled, post.Id) + return nil + }) + require.NoError(t, err) + + // The callback should be called for the GM post even though there's no team + assert.Equal(t, []string{"pid1"}, fnCalled) + // Delete should NOT have been called — GMs don't need a team + mockPostPersistentNotification.AssertNotCalled(t, "Delete", mock.Anything) + }) +} + func TestSendPersistentNotifications(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t)