-
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-10560] Create notification database storage #4688
Conversation
New Issues
Fixed Issues
|
src/Infrastructure.Dapper/NotificationCenter/Repositories/NotificationStatusRepository.cs
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.
Few things, plus I believe you're still working with @rkac-bw on the DB tuning right?
src/Core/NotificationCenter/Repositories/INotificationRepository.cs
Outdated
Show resolved
Hide resolved
...otificationCenter/dbo/Stored Procedures/NotificationStatus_ReadByNotificationIdAndUserId.sql
Outdated
Show resolved
Hide resolved
src/Sql/NotificationCenter/dbo/Stored Procedures/Notification_DeleteById.sql
Outdated
Show resolved
Hide resolved
src/Sql/NotificationCenter/dbo/Stored Procedures/Notification_ReadByUserId.sql
Outdated
Show resolved
Hide resolved
@withinfocus On DB optimisations I have provided the new schema, queries and testing data to @rkac-bw for further investigation. We have few ideas, but nothing seems to be set in stone yet. |
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.
Few small things but very close!
} | ||
|
||
var notifications = await notificationQuery | ||
.OrderByDescending(n => n.Priority) |
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.
ℹ️ I am anticipating that we'll tweak this in the future.
src/Sql/NotificationCenter/dbo/Stored Procedures/Notification_ReadById.sql
Outdated
Show resolved
Hide resolved
src/Sql/NotificationCenter/dbo/Stored Procedures/Notification_ReadByUserId.sql
Outdated
Show resolved
Hide resolved
src/Sql/NotificationCenter/dbo/Stored Procedures/Notification_ReadByUserIdAndStatus.sql
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.
Amazing work here!
@withinfocus i fixed the SP that i broke in previous commits, see ddbacac. |
…equirements and EF
@withinfocus i have pushed one more change to that SP with bec9dec, to closer meet the requirement of status filtering:
|
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.
Had a chat with @rkac-bw and it's good to go, with a potential follow-up for tuning on performance.
I've noticed a couple database testing issues with locks or the instance not being up in time which is strange. I wonder if we need to adjust the delays. |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-10560
📔 Objective
The purpose of this PR is to add the database and repository layer for the notification center work.
⏰ 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