[V13] User notifications not sent correctly when having more than 400 users

src/Umbraco.Core/Persistence/Repositories/IUserRepository.cs

@@ -109,8 +109,23 @@ IEnumerable<IUser> GetPagedResultsByQuery(
 
     void ClearLoginSession(Guid sessionId);
 
+    /// <summary>
+    ///    Gets a page of users, ordered by Id and starting from the provided Id.
+    /// </summary>
+    /// <param name="id">The user Id to start retrieving users from.</param>
+    /// <param name="count">The number of users to return.</param>
+    /// <returns>A page of <see cref="IUser"/> instances.</returns>
+    [Obsolete("No longer used in Umbraco. Scheduled for removal in Umbraco 18.")]
     IEnumerable<IUser> GetNextUsers(int id, int count);
 
+    /// <summary>
+    ///    Gets a page of approved users, ordered by Id and starting from the provided Id.
+    /// </summary>
+    /// <param name="id">The user Id to start retrieving users from.</param>
+    /// <param name="count">The number of users to return.</param>
+    /// <returns>A page of <see cref="IUser"/> instances.</returns>
+    IEnumerable<IUser> GetNextApprovedUsers(int id, int count) => Enumerable.Empty<IUser>();
+
     /// <summary>
     ///     Invalidates sessions for users that aren't associated with the current collection of providers.
     /// </summary>

src/Umbraco.Core/Services/IUserService.cs

@@ -231,8 +231,23 @@ IEnumerable<IUser> GetAll(
     /// </returns>
     IEnumerable<IUser> GetAllNotInGroup(int groupId);
 
+    /// <summary>
+    ///    Gets a page of users, ordered by Id and starting from the provided Id.
+    /// </summary>
+    /// <param name="id">The user Id to start retrieving users from.</param>
+    /// <param name="count">The number of users to return.</param>
+    /// <returns>A page of <see cref="IUser"/> instances.</returns>
+    [Obsolete("No longer used in Umbraco. Scheduled for removal in Umbraco 18.")]
     IEnumerable<IUser> GetNextUsers(int id, int count);
 
+    /// <summary>
+    ///    Gets a page of approved users, ordered by Id and starting from the provided Id.
+    /// </summary>
+    /// <param name="id">The user Id to start retrieving users from.</param>
+    /// <param name="count">The number of users to return.</param>
+    /// <returns>A page of <see cref="IUser"/> instances.</returns>
+    IEnumerable<IUser> GetNextApprovedUsers(int id, int count) => Enumerable.Empty<IUser>();
+
     /// <summary>
     ///     Invalidates sessions for users that aren't associated with the current collection of providers.
     /// </summary>

src/Umbraco.Core/Services/NotificationService.cs

@@ -96,7 +96,7 @@ public void SendNotifications(
 
         // see notes above
         var id = Constants.Security.SuperUserId;
-        const int pagesz = 400; // load batches of 400 users
+        const int UserBatchSize = 400; // load batches of 400 users
         do
         {
             var notifications = GetUsersNotifications(new List<int>(), action, Enumerable.Empty<int>(), Constants.ObjectTypes.Document)?.ToList();
@@ -106,10 +106,10 @@ public void SendNotifications(
             }
 
             // users are returned ordered by id, notifications are returned ordered by user id
-            var users = _userService.GetNextUsers(id, pagesz).Where(x => x.IsApproved).ToList();
-            foreach (IUser user in users)
+            var approvedUsers = _userService.GetNextApprovedUsers(id, UserBatchSize).ToList();
+            foreach (IUser approvedUser in approvedUsers)
             {
-                Notification[] userNotifications = notifications.Where(n => n.UserId == user.Id).ToArray();
+                Notification[] userNotifications = notifications.Where(n => n.UserId == approvedUser.Id).ToArray();
                 foreach (Notification notification in userNotifications)
                 {
                     // notifications are inherited down the tree - find the topmost entity
@@ -130,14 +130,14 @@ public void SendNotifications(
                     }
 
                     // queue notification
-                    NotificationRequest req = CreateNotificationRequest(operatingUser, user, entityForNotification, prevVersionDictionary[entityForNotification.Id], actionName, siteUri, createSubject, createBody);
+                    NotificationRequest req = CreateNotificationRequest(operatingUser, approvedUser, entityForNotification, prevVersionDictionary[entityForNotification.Id], actionName, siteUri, createSubject, createBody);
                     Enqueue(req);
                     break;
                 }
             }
 
             // load more users if any
-            id = users.Count == pagesz ? users.Last().Id + 1 : -1;
+            id = approvedUsers.Count == UserBatchSize ? approvedUsers.Last().Id + 1 : -1;
         }
         while (id > 0);
     }

src/Umbraco.Core/Services/UserService.cs

@@ -712,6 +712,7 @@ public IEnumerable<IUser> GetAll(long pageIndex, int pageSize, out long totalRec
         }
     }
 
+    /// <inheritdoc/>
     public IEnumerable<IUser> GetNextUsers(int id, int count)
     {
         using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true))
@@ -720,6 +721,15 @@ public IEnumerable<IUser> GetNextUsers(int id, int count)
         }
     }
 
+    /// <inheritdoc/>
+    public IEnumerable<IUser> GetNextApprovedUsers(int id, int count)
+    {
+        using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true))
+        {
+            return _userRepository.GetNextApprovedUsers(id, count);
+        }
+    }
+
     /// <inheritdoc />
     public void InvalidateSessionsForRemovedProviders(IEnumerable<string> currentLoginProviders)
     {

src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs

@@ -1053,13 +1053,25 @@ private Sql<ISqlContext> ApplySort(Sql<ISqlContext> sql, Expression<Func<IUser,
         return sql;
     }
 
-    public IEnumerable<IUser> GetNextUsers(int id, int count)
+    /// <inheritdoc/>
+    public IEnumerable<IUser> GetNextUsers(int id, int count) => PerformGetNextUsers(id, false, count);
+
+    /// <inheritdoc/>
+    public IEnumerable<IUser> GetNextApprovedUsers(int id, int count) => PerformGetNextUsers(id, true, count);
+
+    private IEnumerable<IUser> PerformGetNextUsers(int id, bool approvedOnly, int count)
     {
         Sql<ISqlContext> idsQuery = SqlContext.Sql()
             .Select<UserDto>(x => x.Id)
             .From<UserDto>()
-            .Where<UserDto>(x => x.Id >= id)
-            .OrderBy<UserDto>(x => x.Id);
+            .Where<UserDto>(x => x.Id >= id);
+
+        if (approvedOnly)
+        {
+            idsQuery = idsQuery.Where<UserDto>(x => x.Disabled == false);
+        }
+
+        idsQuery = idsQuery.OrderBy<UserDto>(x => x.Id);
 
         // first page is index 1, not zero
         var ids = Database.Page<int>(1, count, idsQuery).Items.ToArray();

tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/UserServiceTests.cs

@@ -967,6 +967,73 @@ public void Can_Get_Assigned_StartNodes_For_User()
         }
     }
 
+    [Test]
+    public void Can_Get_Next_Users_In_Batches()
+    {
+        var users = UserBuilder.CreateMulipleUsers(10).ToArray();
+        UserService.Save(users);
+
+        var userBatch1 = UserService.GetNextUsers(Constants.Security.SuperUserId, 3);
+        var userBatch2 = UserService.GetNextUsers(1, 6);
+        var userBatch3 = UserService.GetNextUsers(4, 5);
+        var userBatch4 = UserService.GetNextUsers(9, 5);
+        var allUsers = UserService.GetNextUsers(Constants.Security.SuperUserId, int.MaxValue);
+
+        Assert.AreEqual(3, userBatch1.Count());
+        Assert.AreEqual(Constants.Security.SuperUserId, userBatch1.First().Id);
+        Assert.AreEqual(2, userBatch1.Last().Id);
+
+        Assert.AreEqual(6, userBatch2.Count());
+        Assert.AreEqual(1, userBatch2.First().Id);
+        Assert.AreEqual(6, userBatch2.Last().Id);
+
+        Assert.AreEqual(5, userBatch3.Count());
+        Assert.AreEqual(4, userBatch3.First().Id);
+        Assert.AreEqual(8, userBatch3.Last().Id);
+
+        Assert.AreEqual(2, userBatch4.Count());
+        Assert.AreEqual(9, userBatch4.First().Id);
+        Assert.AreEqual(10, userBatch4.Last().Id);
+
+        Assert.AreEqual(11, allUsers.Count());
+    }
+
+    [Test]
+    public void Can_Get_Next_Approved_Users_In_Batches()
+    {
+        var users = UserBuilder.CreateMulipleUsers(10).ToArray();
+        for (int i = 0; i < users.Length; i++)
+        {
+            users[i].IsApproved = !(i == 0 || i == 6);  // Setup all users as approved except for a couple.
+        }
+
+        UserService.Save(users);
+
+        var userBatch1 = UserService.GetNextApprovedUsers(Constants.Security.SuperUserId, 3);
+        var userBatch2 = UserService.GetNextApprovedUsers(1, 6);
+        var userBatch3 = UserService.GetNextApprovedUsers(4, 5);
+        var userBatch4 = UserService.GetNextApprovedUsers(9, 5);
+        var allApprovedUsers = UserService.GetNextApprovedUsers(Constants.Security.SuperUserId, int.MaxValue);
+
+        Assert.AreEqual(3, userBatch1.Count());
+        Assert.AreEqual(Constants.Security.SuperUserId, userBatch1.First().Id);
+        Assert.AreEqual(3, userBatch1.Last().Id);
+
+        Assert.AreEqual(6, userBatch2.Count());
+        Assert.AreEqual(2, userBatch2.First().Id);
+        Assert.AreEqual(8, userBatch2.Last().Id);
+
+        Assert.AreEqual(5, userBatch3.Count());
+        Assert.AreEqual(4, userBatch3.First().Id);
+        Assert.AreEqual(9, userBatch3.Last().Id);
+
+        Assert.AreEqual(2, userBatch4.Count());
+        Assert.AreEqual(9, userBatch4.First().Id);
+        Assert.AreEqual(10, userBatch4.Last().Id);
+
+        Assert.AreEqual(9, allApprovedUsers.Count());
+    }
+
     private Content[] BuildContentItems(int numberToCreate)
     {
         var template = TemplateBuilder.CreateTextPageTemplate();