[AC 1427]Add New Organisation Properties Update DB Objects and create migration #2980

cyprain-okeke · 2023-05-30T11:48:17Z

Type of change

- [ ] Bug fix
- [X] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

In preparation for the organization's secret manager billing feature, the UsePasswordManager, SmSeats,SmServiceAccounts,MaxAutoscaleSmSeats and MaxAutoscaleSmSeats columns need to be added to the Organization table, relevant db objects need to be updated, and migrations need to be created for the different database providers.

Code changes

I updated the Organization entity class with the UsePasswordManager, SmSeats,SmServiceAccounts,MaxAutoscaleSmSeats and MaxAutoscaleSmSeats properties, updated the organization table, stored procedures and functions SQL scripts, and created migrations for SQL server and EF core (SQLite, MySQL and Postgres).
I updated the Organization response models to include the new columns and updated ef core queries to return the new columns.

Before you submit

Please check for formatting errors (dotnet format --verify-no-changes) (required)
If making database changes - make sure you also update Entity Framework queries and/or migrations
Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team

…bjects_and_create_migration

cturnbull-bitwarden · 2023-05-30T18:58:44Z

src/Api/Models/Response/Organizations/OrganizationResponseModel.cs

@@ -81,6 +86,11 @@ public OrganizationResponseModel(Organization organization, string obj = "organi
    public bool UseCustomPermissions { get; set; }
    public bool SelfHost { get; set; }
    public bool HasPublicAndPrivateKeys { get; set; }
+    public bool UsePasswordManager { get; set; } = true;


This default initialization value seems to be ignored since it's already initialized by the constructor. What's the reason for including it here? Perhaps just to document the intended default value?

That's a good point, what do you think @cyprain-okeke?

Cy's response (@cyprain-okeke looks like you accidentally edited my comment instead, so I'm just posting this separately for you)

I agree with you but The = true part of the declaration assigns a default value to the property. It means that if no value is explicitly assigned to UsePasswordManager, it will default to true.

So, in this case, the property UsePasswordManager will default to true if no value is explicitly assigned to it.

Yes but it's a non-nullable bool, so it will always have a value assigned in the constructor, and this default assignment will never be used. (Please correct me if I'm wrong!)

cturnbull-bitwarden · 2023-05-30T19:02:35Z

src/Api/Models/Response/Organizations/OrganizationResponseModel.cs

+    public int? SmSeats { get; set; }
+    public int? SmServiceAccounts { get; set; }
+    public int? MaxAutoscaleSmSeats { get; set; } = null;
+    public int? MaxAutoscaleSmServiceAccounts { get; set; } = null;


Specifying null as the default value is redundant here given the datatype is already an int?

~~The default value is 0, not null: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/default-values~~

I agree with your comment above though that these default assignments are probably not necessary.

This null has been removed. I agree with Conner

Oops sorry, the default value is indeed null for a nullable type. d'oh.

@cyprain-okeke This change is not showing, please double check that you've removed these assignments and pushed to remote.

src/Core/Entities/Organization.cs

…bjects_and_create_migration

r-tome

The migration file is missing sp_refreshsqlmodule for the dependant stored procedures.
Other than that is looks good to me but left some questions/suggestions.

util/Migrator/DbScripts/2023-05-27_00_OrganizationSecretsManagerBillingColumns.sql

util/MySqlMigrations/Migrations/20230530114306_AddSecretsManagerBillingFieldToOrganization.cs

.../PostgresMigrations/Migrations/20230530114251_AddSecretsManagerBillingFieldToOrganization.cs

util/SqliteMigrations/Migrations/20230530114320_AddSecretsManagerBillingFieldToOrganization.cs

src/Sql/dbo/Stored Procedures/Organization_Create.sql

src/Sql/dbo/Stored Procedures/Organization_Update.sql

src/Core/Entities/Organization.cs

…eate_migration' of https://github.com/bitwarden/server into ac-1427/add_new_org_properties_update_db_objects_and_create_migration

…bjects_and_create_migration

eliykat

I haven't reviewed EF migrations on the assumption that @cturnbull-bitwarden and @r-tome already reviewed them (thank you both!). Just a few questions/comments.

This seems safe to merge into master because we're not using any of these values yet, but we do need to decide whether we're using a feature branch or feature flagging. I've raised that in #team-eng-adminconsole and we should make a decision there before merging. (This PR itself would not be feature flagged, but if we're going to use a feature branch then it should go there.)

src/Sql/dbo/Tables/Organization.sql

src/Sql/dbo/Views/OrganizationUserOrganizationDetailsView.sql

src/Core/Entities/Organization.cs

eliykat · 2023-06-02T01:02:00Z

src/Api/Models/Response/Organizations/OrganizationResponseModel.cs

+    public int? SmSeats { get; set; }
+    public int? SmServiceAccounts { get; set; }
+    public int? MaxAutoscaleSmSeats { get; set; } = null;
+    public int? MaxAutoscaleSmServiceAccounts { get; set; } = null;


~~The default value is 0, not null: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/default-values~~

I agree with your comment above though that these default assignments are probably not necessary.

eliykat · 2023-06-02T01:04:05Z

src/Api/Models/Response/Organizations/OrganizationResponseModel.cs

@@ -81,6 +86,11 @@ public OrganizationResponseModel(Organization organization, string obj = "organi
    public bool UseCustomPermissions { get; set; }
    public bool SelfHost { get; set; }
    public bool HasPublicAndPrivateKeys { get; set; }
+    public bool UsePasswordManager { get; set; } = true;


That's a good point, what do you think @cyprain-okeke?

util/Migrator/DbScripts/2023-05-27_00_OrganizationSecretsManagerBillingColumns.sql

…bjects_and_create_migration

eliykat

Looks good, I believe the only outstanding changes are those requested by @cturnbull-bitwarden in OrganizationResponseModel.cs, you said you agreed but I can't see the updated code.

PS. As discussed in the team channel, it looks like we'll be feature flagging this initiative rather than using a feature branch. Because these are database updates that do not affect any existing functionality, it's fine to target master.

@cyprain-okeke The changes suggested by @cturnbull-bitwarden have been done. Thanks for reminding me of that.

…eate_migration' of https://github.com/bitwarden/server into ac-1427/add_new_org_properties_update_db_objects_and_create_migration

…bjects_and_create_migration

…eate_migration' of https://github.com/bitwarden/server into ac-1427/add_new_org_properties_update_db_objects_and_create_migration

r-tome

Great work!

eliykat

lgtm as well, thanks Cy!

…bjects_and_create_migration

cyprain-okeke added 3 commits May 30, 2023 12:35

Add new properties to organization

1ebeeda

Add new properties to organization

6f00a46

Create migration

c497ca2

cyprain-okeke added the needs-qa label May 30, 2023

cyprain-okeke requested a review from a team May 30, 2023 11:48

cyprain-okeke changed the title ~~Ac 1427/add new org properties update db objects and create migration~~ [AC 1427]Add New Organisation Properties Update DB Objects and create migration May 30, 2023

cyprain-okeke added 5 commits May 30, 2023 14:17

Add the columns to the view

66cfeaa

Fix the syntax error

de69dde

Change the namespaces

7e9a68f

Remove the comma on the stripe file

0590fde

Merge branch 'master' into ac-1427/add_new_org_properties_update_db_o…

07224ec

…bjects_and_create_migration

cturnbull-bitwarden reviewed May 30, 2023

View reviewed changes

src/Core/Entities/Organization.cs Outdated Show resolved Hide resolved

cyprain-okeke and others added 2 commits May 31, 2023 15:38

Remove the nulls

b2371a1

Merge branch 'master' into ac-1427/add_new_org_properties_update_db_o…

2ff0a89

…bjects_and_create_migration

cyprain-okeke requested a review from eliykat May 31, 2023 15:33

r-tome requested changes Jun 1, 2023

View reviewed changes

cyprain-okeke added 3 commits June 1, 2023 14:39

Resolving the PR comments

2c67ece

Merge branch 'ac-1427/add_new_org_properties_update_db_objects_and_cr…

e5de4fc

…eate_migration' of https://github.com/bitwarden/server into ac-1427/add_new_org_properties_update_db_objects_and_create_migration

Merge branch 'master' into ac-1427/add_new_org_properties_update_db_o…

d868ffc

…bjects_and_create_migration

cyprain-okeke requested a review from r-tome June 1, 2023 13:55

eliykat requested a review from Thomas-Avery June 2, 2023 00:33

eliykat requested changes Jun 2, 2023

View reviewed changes

cyprain-okeke and others added 2 commits June 2, 2023 11:28

Add a refresh for OrganizationView

a658e77

Merge branch 'master' into ac-1427/add_new_org_properties_update_db_o…

aedef82

…bjects_and_create_migration

cyprain-okeke requested review from eliykat and cturnbull-bitwarden June 2, 2023 12:01

eliykat requested changes Jun 5, 2023

View reviewed changes

Remove the True default values

06a10b2

cyprain-okeke and others added 4 commits June 5, 2023 08:19

Merge branch 'ac-1427/add_new_org_properties_update_db_objects_and_cr…

d97fec3

…eate_migration' of https://github.com/bitwarden/server into ac-1427/add_new_org_properties_update_db_objects_and_create_migration

Merge branch 'master' into ac-1427/add_new_org_properties_update_db_o…

7007fdf

…bjects_and_create_migration

Resolve the comments

b43898c

Merge branch 'ac-1427/add_new_org_properties_update_db_objects_and_cr…

1455a30

…eate_migration' of https://github.com/bitwarden/server into ac-1427/add_new_org_properties_update_db_objects_and_create_migration

r-tome approved these changes Jun 5, 2023

View reviewed changes

eliykat approved these changes Jun 6, 2023

View reviewed changes

cturnbull-bitwarden mentioned this pull request Jun 9, 2023

[AC-1429] Add new secrets manager fields to organization edit page in admin portal #3009

Merged

Merge branch 'master' into ac-1427/add_new_org_properties_update_db_o…

621503e

…bjects_and_create_migration

cyprain-okeke removed the needs-qa label Jun 12, 2023

cyprain-okeke merged commit fed0828 into master Jun 12, 2023

cyprain-okeke deleted the ac-1427/add_new_org_properties_update_db_objects_and_create_migration branch June 12, 2023 22:48

shane-melton mentioned this pull request Jun 14, 2023

[AC-1423] Update organization subscription cloud page bitwarden/clients#5614

Merged

shane-melton mentioned this pull request Jun 28, 2023

[AC-1418] Add secrets manager manage subscription component bitwarden/clients#5661

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AC 1427]Add New Organisation Properties Update DB Objects and create migration #2980

[AC 1427]Add New Organisation Properties Update DB Objects and create migration #2980

cyprain-okeke commented May 30, 2023

cturnbull-bitwarden May 30, 2023

eliykat Jun 2, 2023 •

edited

Loading

eliykat Jun 5, 2023 •

edited

Loading

eliykat Jun 5, 2023

cturnbull-bitwarden May 30, 2023

eliykat Jun 2, 2023 •

edited

Loading

cyprain-okeke Jun 2, 2023

eliykat Jun 5, 2023

r-tome left a comment

eliykat left a comment

eliykat Jun 2, 2023 •

edited

Loading

eliykat Jun 2, 2023 •

edited

Loading

eliykat left a comment •

edited by cyprain-okeke

Loading

r-tome left a comment

eliykat left a comment

[AC 1427]Add New Organisation Properties Update DB Objects and create migration #2980

[AC 1427]Add New Organisation Properties Update DB Objects and create migration #2980

Conversation

cyprain-okeke commented May 30, 2023

Type of change

Objective

Code changes

Before you submit

cturnbull-bitwarden May 30, 2023

Choose a reason for hiding this comment

eliykat Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

eliykat Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

eliykat Jun 5, 2023

Choose a reason for hiding this comment

cturnbull-bitwarden May 30, 2023

Choose a reason for hiding this comment

eliykat Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

cyprain-okeke Jun 2, 2023

Choose a reason for hiding this comment

eliykat Jun 5, 2023

Choose a reason for hiding this comment

r-tome left a comment

Choose a reason for hiding this comment

eliykat left a comment

Choose a reason for hiding this comment

eliykat Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

eliykat Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

eliykat left a comment • edited by cyprain-okeke Loading

Choose a reason for hiding this comment

r-tome left a comment

Choose a reason for hiding this comment

eliykat left a comment

Choose a reason for hiding this comment

eliykat Jun 2, 2023 •

edited

Loading

eliykat Jun 5, 2023 •

edited

Loading

eliykat Jun 2, 2023 •

edited

Loading

eliykat Jun 2, 2023 •

edited

Loading

eliykat Jun 2, 2023 •

edited

Loading

eliykat left a comment •

edited by cyprain-okeke

Loading