-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PM-10563] Notification Center API #4852
Conversation
# Conflicts: # test/Core.Test/NotificationCenter/AutoFixture/NotificationFixtures.cs
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4852 +/- ##
==========================================
+ Coverage 43.15% 43.39% +0.23%
==========================================
Files 1452 1456 +4
Lines 66301 66412 +111
Branches 6073 6078 +5
==========================================
+ Hits 28615 28817 +202
+ Misses 36398 36304 -94
- Partials 1288 1291 +3 ☔ View full report in Codecov by Sentry. |
New Issues
|
src/Api/NotificationCenter/Controllers/NotificationsController.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this. The integration tests are awesome. 🚀
A few minor things to look at.
src/Api/NotificationCenter/Controllers/NotificationsController.cs
Outdated
Show resolved
Hide resolved
src/Api/NotificationCenter/Controllers/NotificationsController.cs
Outdated
Show resolved
Hide resolved
src/Api/NotificationCenter/Controllers/NotificationsController.cs
Outdated
Show resolved
Hide resolved
test/Api.IntegrationTest/NotificationCenter/Controllers/NotificationsControllerTests.cs
Outdated
Show resolved
Hide resolved
test/Api.IntegrationTest/NotificationCenter/Controllers/NotificationsControllerTests.cs
Outdated
Show resolved
Hide resolved
test/Api.IntegrationTest/NotificationCenter/Controllers/NotificationsControllerTests.cs
Outdated
Show resolved
Hide resolved
test/Api.IntegrationTest/NotificationCenter/Controllers/NotificationsControllerTests.cs
Outdated
Show resolved
Hide resolved
test/Api.IntegrationTest/NotificationCenter/Controllers/NotificationsControllerTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DB changes already approved.
# Conflicts: # src/SharedWeb/Utilities/ServiceCollectionExtensions.cs
458a1a1
The change is ready to be picked up by QA. |
# Conflicts: # src/SharedWeb/Utilities/ServiceCollectionExtensions.cs
ba8c06c
Ready to be merged. |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-10563
📔 Objective
Notification API:
MarkNotificationDeletedCommand
andMarkNotificationReadCommand
during create operation - now UTC format📸 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