Added nil checks (#35755)

* Added nil checks * Added test for DM and GM * Updated operation order
2026-05-28 04:35:04 -04:00 · 2026-04-06 08:47:24 +05:30 · 2026-04-06 08:47:24 +05:30 · ad35eba60b
commit ad35eba60b
parent 2b45e43e05
3 changed files with 250 additions and 7 deletions
--- 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
--- 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)
 			}
--- 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)