[PM-10560] Create notification database storage #4688

Thomas-Avery · 2024-08-21T22:57:19Z

🎟️ 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

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

codecov · 2024-08-21T23:07:31Z

Codecov Report

Attention: Patch coverage is 20.39801% with 160 lines in your changes missing coverage. Please review.

Project coverage is 41.76%. Comparing base (b103e8f) to head (4341f10).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...ationCenter/Repositories/NotificationRepository.cs	0.00%	61 Missing ⚠️
...enter/Repositories/NotificationStatusRepository.cs	0.00%	36 Missing ⚠️
...enter/Repositories/NotificationStatusRepository.cs	0.00%	24 Missing ⚠️
...ationCenter/Repositories/NotificationRepository.cs	0.00%	14 Missing ⚠️
...c/Core/NotificationCenter/Entities/Notification.cs	0.00%	13 Missing ⚠️
.../NotificationCenter/Entities/NotificationStatus.cs	0.00%	4 Missing ⚠️
...onCenter/Models/Filter/NotificationStatusFilter.cs	0.00%	2 Missing ⚠️
...ucture.Dapper/DapperServiceCollectionExtensions.cs	0.00%	2 Missing ⚠️
...ramework/NotificationCenter/Models/Notification.cs	75.00%	2 Missing ⚠️
...rk/NotificationCenter/Models/NotificationStatus.cs	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4688      +/-   ##
==========================================
- Coverage   41.83%   41.76%   -0.07%     
==========================================
  Files        1294     1305      +11     
  Lines       61740    61941     +201     
  Branches     5695     5703       +8     
==========================================
+ Hits        25827    25868      +41     
- Misses      34722    34882     +160     
  Partials     1191     1191

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-08-21T23:23:07Z

Checkmarx One – Scan Summary & Details – 35fb76a5-58d4-49fb-9b02-22d1d99ef64c

New Issues

Issue	Source File / Package	Checkmarx Insight
Privacy_Violation	/src/Api/Auth/Controllers/TwoFactorController.cs: 488	Attack Vector
Privacy_Violation	/src/Api/Vault/Models/Request/CipherRequestModel.cs: 169	Attack Vector
Privacy_Violation	/src/Api/Controllers/DevicesController.cs: 133	Attack Vector
Privacy_Violation	/src/Api/Auth/Controllers/AccountsController.cs: 549	Attack Vector
Privacy_Violation	/src/Api/Auth/Controllers/AccountsController.cs: 829	Attack Vector
Privacy_Violation	/src/Api/Auth/Controllers/AccountsController.cs: 412	Attack Vector
Privacy_Violation	/src/Api/Vault/Controllers/CiphersController.cs: 906	Attack Vector
Privacy_Violation	/src/Api/Auth/Controllers/AccountsController.cs: 847	Attack Vector
Privacy_Violation	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 259	Attack Vector
Privacy_Violation	/src/Api/Controllers/DevicesController.cs: 159	Attack Vector
Privacy_Violation	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 403	Attack Vector
Privacy_Violation	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 350	Attack Vector
Log_Forging	/src/Billing/Controllers/StripeController.cs: 164	Attack Vector
Log_Forging	/src/Billing/Controllers/StripeController.cs: 164	Attack Vector
Log_Forging	/src/Billing/Controllers/StripeController.cs: 164	Attack Vector
Log_Forging	/src/Billing/Controllers/StripeController.cs: 164	Attack Vector
Log_Forging	/src/Billing/Controllers/StripeController.cs: 164	Attack Vector
Log_Forging	/src/Billing/Controllers/StripeController.cs: 164	Attack Vector
Log_Forging	/src/Api/Vault/Controllers/CiphersController.cs: 217	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 445	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 118	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 165	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 174	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 191	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 200	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 231	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 249	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 288	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 297	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 306	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 323	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 332	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 340	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 393	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 404	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 412	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 423	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 103	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 821	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 839	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 541	Attack Vector
Log_Forging	/src/Api/Vault/Controllers/CiphersController.cs: 898	Attack Vector
Log_Forging	/src/Api/Controllers/DevicesController.cs: 124	Attack Vector
Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 239	Attack Vector
Log_Forging	/src/Api/Controllers/DevicesController.cs: 150	Attack Vector
Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 315	Attack Vector
Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 378	Attack Vector

Fixed Issues

Severity	Issue	Source File / Package
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 121
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 107
	CSRF	/src/Api/Controllers/DevicesController.cs: 74
	CSRF	/src/Api/Controllers/DevicesController.cs: 61
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 98
	CSRF	/src/Api/Auth/Controllers/TwoFactorController.cs: 118
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 98
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 229
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 478
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 283
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 478
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 176
	CSRF	/src/Api/Billing/Public/Controllers/OrganizationController.cs: 47
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 551
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 503
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 565
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 445
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 357
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 337
	CSRF	/src/Api/Controllers/DevicesController.cs: 74
	CSRF	/src/Api/Controllers/DevicesController.cs: 61
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 255
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 929
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 107
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 121
	CSRF	/src/Api/AdminConsole/Public/Controllers/OrganizationController.cs: 43
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 676
	Privacy_Violation	/src/Api/AdminConsole/Controllers/OrganizationAuthRequestsController.cs: 59
	Privacy_Violation	/src/Core/Auth/UserFeatures/TdeOffboardingPassword/TdeOffboardingPasswordCommand.cs: 81
	Privacy_Violation	/src/Core/Auth/UserFeatures/TdeOffboardingPassword/TdeOffboardingPasswordCommand.cs: 81
	Privacy_Violation	/src/Core/Auth/UserFeatures/UserMasterPassword/SetInitialMasterPasswordCommand.cs: 59
	Privacy_Violation	/src/Core/Auth/UserFeatures/UserMasterPassword/SetInitialMasterPasswordCommand.cs: 59
	Privacy_Violation	/src/Api/AdminConsole/Controllers/OrganizationAuthRequestsController.cs: 59
	Log_Forging	/src/Api/AdminConsole/Controllers/ProvidersController.cs: 72
	Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 882
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164
	Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 246
	Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 366

src/Infrastructure.Dapper/NotificationCenter/Repositories/NotificationStatusRepository.cs

withinfocus

Few things, plus I believe you're still working with @rkac-bw on the DB tuning right?

src/Core/NotificationCenter/Enums/Priority.cs

src/Core/NotificationCenter/Repositories/INotificationRepository.cs

...otificationCenter/dbo/Stored Procedures/NotificationStatus_ReadByNotificationIdAndUserId.sql

src/Sql/NotificationCenter/dbo/Stored Procedures/Notification_DeleteById.sql

src/Sql/NotificationCenter/dbo/Stored Procedures/Notification_ReadByUserId.sql

src/Sql/NotificationCenter/dbo/Tables/Notification.sql

mzieniukbw · 2024-09-06T10:54:54Z

@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.
I suggest we do the DB optimisations at later stage, as part of separate Jira ticket, so we can continue with service and api layers in the meantime.

withinfocus

Few small things but very close!

withinfocus · 2024-09-06T12:30:44Z

src/Infrastructure.EntityFramework/NotificationCenter/Repositories/NotificationRepository.cs

+        }
+
+        var notifications = await notificationQuery
+            .OrderByDescending(n => n.Priority)


ℹ️ I am anticipating that we'll tweak this in the future.

src/Sql/NotificationCenter/dbo/Stored Procedures/Notification_ReadById.sql

src/Sql/NotificationCenter/dbo/Stored Procedures/Notification_ReadByUserId.sql

src/Sql/NotificationCenter/dbo/Stored Procedures/Notification_ReadByUserIdAndStatus.sql

src/Sql/NotificationCenter/dbo/Tables/Notification.sql

withinfocus

Amazing work here!

mzieniukbw · 2024-09-09T16:06:39Z

@withinfocus i fixed the SP that i broke in previous commits, see ddbacac.
In essence by removing the Notification_ReadByUserId and only leaving Notification_ReadByUserIdAndStatus i forgot to update it, to also work when no status filter is provided.

…equirements and EF

mzieniukbw · 2024-09-09T17:24:13Z

@withinfocus i have pushed one more change to that SP with bec9dec, to closer meet the requirement of status filtering:

Filter not provided = no check on notification status
Filter provided = only notifications with status

is read

null / not provided: no check on that column

true: read date is populated

false: read date is null

is deleted

same thing

withinfocus

Had a chat with @rkac-bw and it's good to go, with a potential follow-up for tuning on performance.

withinfocus · 2024-09-09T17:36:36Z

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.

Thomas-Avery added 7 commits August 21, 2024 17:50

Add new tables

cf33439

Add stored procedures

7372fc8

Add core entities and models

43978f0

Setup EF

f47de64

Add repository interfaces

b523175

Add dapper repos

ee4d927

Add EF repos

0bbd1c1

Add order by

ebd2946

mzieniukbw reviewed Aug 22, 2024

View reviewed changes

src/Infrastructure.Dapper/NotificationCenter/Repositories/NotificationStatusRepository.cs Show resolved Hide resolved

Thomas-Avery and others added 7 commits August 22, 2024 15:07

EF updates

899d8f6

Merge branch 'refs/heads/main' into km/pm-10560

373148a

PM-10560: Notifications repository matching requirements.

e3b4cb2

PM-10560: Notifications repository matching requirements.

51119ef

PM-10560: Migration scripts

91336b9

PM-10560: EF index optimizations

95288c1

Merge branch 'refs/heads/main' into km/pm-10560

06ad155

mzieniukbw marked this pull request as ready for review September 3, 2024 09:46

mzieniukbw requested a review from a team as a code owner September 3, 2024 09:46

PM-10560: Cleanup

cb472a8

withinfocus requested changes Sep 3, 2024

View reviewed changes

mzieniukbw added 4 commits September 6, 2024 11:07

PM-10560: Priority in natural order, Repository, sql simplifications

006f1b4

PM-10560: Title column update

78058d9

PM-10560: Incorrect EF migration removal

a8083de

PM-10560: EF migrations

bd1c6d0

mzieniukbw requested a review from withinfocus September 6, 2024 10:50

Merge branch 'refs/heads/main' into km/pm-10560

748e2e0

withinfocus reviewed Sep 6, 2024

View reviewed changes

mzieniukbw added 4 commits September 9, 2024 12:47

Merge branch 'refs/heads/main' into km/pm-10560

07184be

PM-10560: Added views, SP naming simplification

34ea5a3

PM-10560: Notification entity Title update, EF migrations

e54064a

PM-10560: Removing Notification_ReadByUserId

acef48b

mzieniukbw requested a review from withinfocus September 9, 2024 13:55

withinfocus previously approved these changes Sep 9, 2024

View reviewed changes

PM-10560: Notification ReadByUserIdAndStatus fix

ddbacac

mzieniukbw dismissed withinfocus’s stale review via ddbacac September 9, 2024 16:05

withinfocus previously approved these changes Sep 9, 2024

View reviewed changes

PM-10560: Notification ReadByUserIdAndStatus fix to be in line with r…

bec9dec

…equirements and EF

mzieniukbw dismissed withinfocus’s stale review via bec9dec September 9, 2024 17:22

mzieniukbw requested a review from withinfocus September 9, 2024 17:24

withinfocus approved these changes Sep 9, 2024

View reviewed changes

Merge branch 'main' into km/pm-10560

4341f10

mzieniukbw merged commit 4c0f8d5 into main Sep 9, 2024
54 checks passed

mzieniukbw deleted the km/pm-10560 branch September 9, 2024 19:52

mzieniukbw mentioned this pull request Sep 12, 2024

PM-10560: Adding Cascades back to Notification Center #4769

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-10560] Create notification database storage #4688

[PM-10560] Create notification database storage #4688

Thomas-Avery commented Aug 21, 2024

codecov bot commented Aug 21, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

withinfocus left a comment

mzieniukbw commented Sep 6, 2024

withinfocus left a comment

withinfocus Sep 6, 2024

withinfocus left a comment

mzieniukbw commented Sep 9, 2024 •

edited

Loading

mzieniukbw commented Sep 9, 2024

withinfocus left a comment

withinfocus commented Sep 9, 2024

[PM-10560] Create notification database storage #4688

[PM-10560] Create notification database storage #4688

Conversation

Thomas-Avery commented Aug 21, 2024

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

codecov bot commented Aug 21, 2024 • edited Loading

Codecov Report

github-actions bot commented Aug 21, 2024 • edited Loading

New Issues

Fixed Issues

withinfocus left a comment

Choose a reason for hiding this comment

mzieniukbw commented Sep 6, 2024

withinfocus left a comment

Choose a reason for hiding this comment

withinfocus Sep 6, 2024

Choose a reason for hiding this comment

withinfocus left a comment

Choose a reason for hiding this comment

mzieniukbw commented Sep 9, 2024 • edited Loading

mzieniukbw commented Sep 9, 2024

withinfocus left a comment

Choose a reason for hiding this comment

withinfocus commented Sep 9, 2024

codecov bot commented Aug 21, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

mzieniukbw commented Sep 9, 2024 •

edited

Loading