Skip to content

Commit e471c1f

Browse files
vantovicVedran AntovićAndyButland
authored
[V13] User notifications not sent correctly when having more than 400 users (#18370)
Co-authored-by: Vedran Antović <[email protected]> Co-authored-by: Andy Butland <[email protected]>
1 parent 0fb91ef commit e471c1f

File tree

6 files changed

+128
-9
lines changed

6 files changed

+128
-9
lines changed

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,8 +109,23 @@ IEnumerable<IUser> GetPagedResultsByQuery(
109109

110110
void ClearLoginSession(Guid sessionId);
111111

112+
/// <summary>
113+
/// Gets a page of users, ordered by Id and starting from the provided Id.
114+
/// </summary>
115+
/// <param name="id">The user Id to start retrieving users from.</param>
116+
/// <param name="count">The number of users to return.</param>
117+
/// <returns>A page of <see cref="IUser"/> instances.</returns>
118+
[Obsolete("No longer used in Umbraco. Scheduled for removal in Umbraco 18.")]
112119
IEnumerable<IUser> GetNextUsers(int id, int count);
113120

121+
/// <summary>
122+
/// Gets a page of approved users, ordered by Id and starting from the provided Id.
123+
/// </summary>
124+
/// <param name="id">The user Id to start retrieving users from.</param>
125+
/// <param name="count">The number of users to return.</param>
126+
/// <returns>A page of <see cref="IUser"/> instances.</returns>
127+
IEnumerable<IUser> GetNextApprovedUsers(int id, int count) => Enumerable.Empty<IUser>();
128+
114129
/// <summary>
115130
/// Invalidates sessions for users that aren't associated with the current collection of providers.
116131
/// </summary>

‎src/Umbraco.Core/Services/IUserService.cs‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,8 +231,23 @@ IEnumerable<IUser> GetAll(
231231
/// </returns>
232232
IEnumerable<IUser> GetAllNotInGroup(int groupId);
233233

234+
/// <summary>
235+
/// Gets a page of users, ordered by Id and starting from the provided Id.
236+
/// </summary>
237+
/// <param name="id">The user Id to start retrieving users from.</param>
238+
/// <param name="count">The number of users to return.</param>
239+
/// <returns>A page of <see cref="IUser"/> instances.</returns>
240+
[Obsolete("No longer used in Umbraco. Scheduled for removal in Umbraco 18.")]
234241
IEnumerable<IUser> GetNextUsers(int id, int count);
235242

243+
/// <summary>
244+
/// Gets a page of approved users, ordered by Id and starting from the provided Id.
245+
/// </summary>
246+
/// <param name="id">The user Id to start retrieving users from.</param>
247+
/// <param name="count">The number of users to return.</param>
248+
/// <returns>A page of <see cref="IUser"/> instances.</returns>
249+
IEnumerable<IUser> GetNextApprovedUsers(int id, int count) => Enumerable.Empty<IUser>();
250+
236251
/// <summary>
237252
/// Invalidates sessions for users that aren't associated with the current collection of providers.
238253
/// </summary>

‎src/Umbraco.Core/Services/NotificationService.cs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ public void SendNotifications(
9696

9797
// see notes above
9898
var id = Constants.Security.SuperUserId;
99-
const int pagesz = 400; // load batches of 400 users
99+
const int UserBatchSize = 400; // load batches of 400 users
100100
do
101101
{
102102
var notifications = GetUsersNotifications(new List<int>(), action, Enumerable.Empty<int>(), Constants.ObjectTypes.Document)?.ToList();
@@ -106,10 +106,10 @@ public void SendNotifications(
106106
}
107107

108108
// users are returned ordered by id, notifications are returned ordered by user id
109-
var users = _userService.GetNextUsers(id, pagesz).Where(x => x.IsApproved).ToList();
110-
foreach (IUser user in users)
109+
var approvedUsers = _userService.GetNextApprovedUsers(id, UserBatchSize).ToList();
110+
foreach (IUser approvedUser in approvedUsers)
111111
{
112-
Notification[] userNotifications = notifications.Where(n => n.UserId == user.Id).ToArray();
112+
Notification[] userNotifications = notifications.Where(n => n.UserId == approvedUser.Id).ToArray();
113113
foreach (Notification notification in userNotifications)
114114
{
115115
// notifications are inherited down the tree - find the topmost entity
@@ -130,14 +130,14 @@ public void SendNotifications(
130130
}
131131

132132
// queue notification
133-
NotificationRequest req = CreateNotificationRequest(operatingUser, user, entityForNotification, prevVersionDictionary[entityForNotification.Id], actionName, siteUri, createSubject, createBody);
133+
NotificationRequest req = CreateNotificationRequest(operatingUser, approvedUser, entityForNotification, prevVersionDictionary[entityForNotification.Id], actionName, siteUri, createSubject, createBody);
134134
Enqueue(req);
135135
break;
136136
}
137137
}
138138

139139
// load more users if any
140-
id = users.Count == pagesz ? users.Last().Id + 1 : -1;
140+
id = approvedUsers.Count == UserBatchSize ? approvedUsers.Last().Id + 1 : -1;
141141
}
142142
while (id > 0);
143143
}

‎src/Umbraco.Core/Services/UserService.cs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -712,6 +712,7 @@ public IEnumerable<IUser> GetAll(long pageIndex, int pageSize, out long totalRec
712712
}
713713
}
714714

715+
/// <inheritdoc/>
715716
public IEnumerable<IUser> GetNextUsers(int id, int count)
716717
{
717718
using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true))
@@ -720,6 +721,15 @@ public IEnumerable<IUser> GetNextUsers(int id, int count)
720721
}
721722
}
722723

724+
/// <inheritdoc/>
725+
public IEnumerable<IUser> GetNextApprovedUsers(int id, int count)
726+
{
727+
using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true))
728+
{
729+
return _userRepository.GetNextApprovedUsers(id, count);
730+
}
731+
}
732+
723733
/// <inheritdoc />
724734
public void InvalidateSessionsForRemovedProviders(IEnumerable<string> currentLoginProviders)
725735
{

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

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1053,13 +1053,25 @@ private Sql<ISqlContext> ApplySort(Sql<ISqlContext> sql, Expression<Func<IUser,
10531053
return sql;
10541054
}
10551055

1056-
public IEnumerable<IUser> GetNextUsers(int id, int count)
1056+
/// <inheritdoc/>
1057+
public IEnumerable<IUser> GetNextUsers(int id, int count) => PerformGetNextUsers(id, false, count);
1058+
1059+
/// <inheritdoc/>
1060+
public IEnumerable<IUser> GetNextApprovedUsers(int id, int count) => PerformGetNextUsers(id, true, count);
1061+
1062+
private IEnumerable<IUser> PerformGetNextUsers(int id, bool approvedOnly, int count)
10571063
{
10581064
Sql<ISqlContext> idsQuery = SqlContext.Sql()
10591065
.Select<UserDto>(x => x.Id)
10601066
.From<UserDto>()
1061-
.Where<UserDto>(x => x.Id >= id)
1062-
.OrderBy<UserDto>(x => x.Id);
1067+
.Where<UserDto>(x => x.Id >= id);
1068+
1069+
if (approvedOnly)
1070+
{
1071+
idsQuery = idsQuery.Where<UserDto>(x => x.Disabled == false);
1072+
}
1073+
1074+
idsQuery = idsQuery.OrderBy<UserDto>(x => x.Id);
10631075

10641076
// first page is index 1, not zero
10651077
var ids = Database.Page<int>(1, count, idsQuery).Items.ToArray();

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

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -967,6 +967,73 @@ public void Can_Get_Assigned_StartNodes_For_User()
967967
}
968968
}
969969

970+
[Test]
971+
public void Can_Get_Next_Users_In_Batches()
972+
{
973+
var users = UserBuilder.CreateMulipleUsers(10).ToArray();
974+
UserService.Save(users);
975+
976+
var userBatch1 = UserService.GetNextUsers(Constants.Security.SuperUserId, 3);
977+
var userBatch2 = UserService.GetNextUsers(1, 6);
978+
var userBatch3 = UserService.GetNextUsers(4, 5);
979+
var userBatch4 = UserService.GetNextUsers(9, 5);
980+
var allUsers = UserService.GetNextUsers(Constants.Security.SuperUserId, int.MaxValue);
981+
982+
Assert.AreEqual(3, userBatch1.Count());
983+
Assert.AreEqual(Constants.Security.SuperUserId, userBatch1.First().Id);
984+
Assert.AreEqual(2, userBatch1.Last().Id);
985+
986+
Assert.AreEqual(6, userBatch2.Count());
987+
Assert.AreEqual(1, userBatch2.First().Id);
988+
Assert.AreEqual(6, userBatch2.Last().Id);
989+
990+
Assert.AreEqual(5, userBatch3.Count());
991+
Assert.AreEqual(4, userBatch3.First().Id);
992+
Assert.AreEqual(8, userBatch3.Last().Id);
993+
994+
Assert.AreEqual(2, userBatch4.Count());
995+
Assert.AreEqual(9, userBatch4.First().Id);
996+
Assert.AreEqual(10, userBatch4.Last().Id);
997+
998+
Assert.AreEqual(11, allUsers.Count());
999+
}
1000+
1001+
[Test]
1002+
public void Can_Get_Next_Approved_Users_In_Batches()
1003+
{
1004+
var users = UserBuilder.CreateMulipleUsers(10).ToArray();
1005+
for (int i = 0; i < users.Length; i++)
1006+
{
1007+
users[i].IsApproved = !(i == 0 || i == 6); // Setup all users as approved except for a couple.
1008+
}
1009+
1010+
UserService.Save(users);
1011+
1012+
var userBatch1 = UserService.GetNextApprovedUsers(Constants.Security.SuperUserId, 3);
1013+
var userBatch2 = UserService.GetNextApprovedUsers(1, 6);
1014+
var userBatch3 = UserService.GetNextApprovedUsers(4, 5);
1015+
var userBatch4 = UserService.GetNextApprovedUsers(9, 5);
1016+
var allApprovedUsers = UserService.GetNextApprovedUsers(Constants.Security.SuperUserId, int.MaxValue);
1017+
1018+
Assert.AreEqual(3, userBatch1.Count());
1019+
Assert.AreEqual(Constants.Security.SuperUserId, userBatch1.First().Id);
1020+
Assert.AreEqual(3, userBatch1.Last().Id);
1021+
1022+
Assert.AreEqual(6, userBatch2.Count());
1023+
Assert.AreEqual(2, userBatch2.First().Id);
1024+
Assert.AreEqual(8, userBatch2.Last().Id);
1025+
1026+
Assert.AreEqual(5, userBatch3.Count());
1027+
Assert.AreEqual(4, userBatch3.First().Id);
1028+
Assert.AreEqual(9, userBatch3.Last().Id);
1029+
1030+
Assert.AreEqual(2, userBatch4.Count());
1031+
Assert.AreEqual(9, userBatch4.First().Id);
1032+
Assert.AreEqual(10, userBatch4.Last().Id);
1033+
1034+
Assert.AreEqual(9, allApprovedUsers.Count());
1035+
}
1036+
9701037
private Content[] BuildContentItems(int numberToCreate)
9711038
{
9721039
var template = TemplateBuilder.CreateTextPageTemplate();

0 commit comments

Comments
 (0)