[PM-15084] Push global notification creation to affected clients #5079
Conversation
This enables the Notification Center created global notifications to be sent to affected devices of the same server installation. All clients connected to any of the server instance of that installation id would receive it. This is useful for notifying all clients of an installation about upcoming maintenance. This works both for Self-Hosted, but also for Cloud, assuming an installation id is set.
mzieniukbw
requested review from
a team,
withinfocus and
MGibson1
and removed request for
a team and
Patrick-Pimentel-Bitwarden
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## km/pm-10564 #5079 +/- ##
===============================================
+ Coverage 43.34% 43.42% +0.08%
===============================================
Files 1412 1412
Lines 65030 65161 +131
Branches 5940 5963 +23
===============================================
+ Hits 28184 28299 +115
- Misses 35608 35622 +14
- Partials 1238 1240 +2 ☔ View full report in Codecov by Sentry. |
|
New Issues
Fixed Issues
|
# Conflicts: # src/Core/Models/PushNotification.cs # src/Core/NotificationHub/NotificationHubPushNotificationService.cs # src/Core/Services/Implementations/AzureQueuePushNotificationService.cs # src/Core/Services/Implementations/NotificationsApiPushNotificationService.cs # src/Core/Services/Implementations/RelayPushNotificationService.cs # src/Core/Services/NoopImplementations/NoopPushNotificationService.cs # test/Core.Test/NotificationHub/NotificationHubPushNotificationServiceTests.cs # test/Core.Test/Services/AzureQueuePushNotificationServiceTests.cs
Renamed PushType's `SyncNotification` to `Notification`, since the push type payload does not require, is not dependent on the sync mechanism.
| Title = notification.Title, | ||
| Body = notification.Body, | ||
| CreationDate = notification.CreationDate, | ||
| RevisionDate = notification.RevisionDate | ||
| }; | ||
|
|
||
| if (notification.UserId.HasValue) | ||
| if (notification.Global && installationId.HasValue) |
There was a problem hiding this comment.
💭 Should there be some logging or an exception thrown if none of these conditions are met?
This is applicable to the other services with the same logic.
| @@ -228,15 +252,20 @@ public async Task PushNotificationStatusAsync(Notification notification, Notific | |||
| DeletedDate = notificationStatus.DeletedDate | |||
| }; | |||
|
|
|||
| if (notification.UserId.HasValue) | |||
| if (notification.Global && installationId.HasValue) | |||
There was a problem hiding this comment.
Same here. Should there be some logging or an exception thrown if none of these conditions are met?
|
|
||
| namespace Bit.Test.Common.AutoFixture.Attributes; | ||
|
|
||
| public class RepeatingPatternBitAutoDataAttribute : BitAutoDataAttribute |
There was a problem hiding this comment.
💭
I really like the idea of what we're trying to solve here. It's annoying to type out and easy to miss some of those inline data sets.
I think it would have been easier to have this in a separate PR, since it's introducing a new testing tool/attribute.
Xunit has a few different solutions with their idea of sharing test data ClassData MemberData and I know there are a few libraries that similarly do what you have proposed.
For this specifically, I think combining xUnit's idea of ClassData with BitAutoData is an alternative to consider here.
Something like (this is rough code could be cleaned up)
public class ClassBitAutoDataAttribute : BitAutoDataAttribute
{
public ClassBitAutoDataAttribute(Type @class)
{
Class = @class;
}
public Type Class { get; private set; }
public override IEnumerable<object[]> GetData(MethodInfo testMethod)
{
IEnumerable<object[]> data = Activator.CreateInstance(Class) as IEnumerable<object[]>;
if (data == null)
{
throw new ArgumentException(
string.Format(
CultureInfo.CurrentCulture,
"{0} must implement IEnumerable<object[]> to be used as ClassData for the test method named '{1}' on {2}",
Class.FullName,
testMethod.Name,
testMethod.DeclaringType.FullName
)
);
}
foreach (var classData in data)
{
var bitData = base.GetData(testMethod).First();
for (var i = 0; i < classData.Length; i++)
{
bitData[i] = classData[i];
}
yield return bitData;
}
}
}
Then we could have one class to share data across tests
public class SendAsyncTestData : TheoryData<bool, bool, bool>
{
public SendAsyncTestData()
{
Add(false, false, false);
Add(true, false, false);
Add(false, true, false);
Add(true, true, false);
Add(false, false, true);
Add(true, false, true);
Add(false, true, true);
}
};
Usage in an individual test.
[Theory]
[ClassBitAutoDataAttribute(typeof(SendAsyncTestData))]
public async Task SendAsync_UserIdSet_SendPayloadToUserAsync(bool haveIdentifier, bool haveDeviceId,
bool haveOrganizationId, SutProvider<PushController> sutProvider, Guid installationId, Guid userId,
Guid identifier, Guid deviceId)
{
This would give us more flexibility and allow for generating (looping through combinations) the test data within the test data class if required.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-15084
This is Part 4.
See Part 3: #5057
📔 Objective
When a global notification is created via the service layer, distributes a “notification created” or "notification updated" push type event to related clients of the user.
Global notifications are sent to servers of the same installation id. All clients connected to any of the server instance of that installation id would receive it.
This is useful for notifying all clients of an installation about upcoming maintenance.
This works both for Self-Hosted, but also for Cloud (bitwarden.com), assuming an installation id is set.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes