Split Organization.LimitCollectionCreationDeletion into two separate business rules
#4730
Conversation
|
New Issues
Fixed Issues
|
addisonbeck
force-pushed
the
ac/addison/pm-10863/entity-updates
branch
4 times, most recently
from
1f19ac7
to
eca4cfc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4730 +/- ##
==========================================
+ Coverage 41.57% 41.59% +0.02%
==========================================
Files 1357 1358 +1
Lines 64056 64112 +56
Branches 5891 5900 +9
==========================================
+ Hits 26631 26669 +38
- Misses 36208 36224 +16
- Partials 1217 1219 +2 ☔ View full report in Codecov by Sentry. |
addisonbeck
force-pushed
the
ac/addison/pm-10863/entity-updates
branch
3 times, most recently
from
30604fe
to
03574a7
Compare
addisonbeck
force-pushed
the
ac/addison/pm-10863/rename-limit-collection-creation-deletion
branch
19 times, most recently
from
bd9fd83
to
5e231f7
Compare
addisonbeck
force-pushed
the
ac/addison/pm-10863/rename-limit-collection-creation-deletion
branch
from
87d6654
to
69a1adf
Compare
addisonbeck
force-pushed
the
ac/addison/pm-10863/entity-updates
branch
2 times, most recently
from
8d358e1
to
c07a494
Compare
addisonbeck
force-pushed
the
ac/addison/pm-10863/entity-updates
branch
from
c07a494
to
66c93e6
Compare
addisonbeck
force-pushed
the
ac/addison/pm-10863/rename-limit-collection-creation-deletion
branch
from
c37f77f
to
af50aac
Compare
addisonbeck
force-pushed
the
ac/addison/pm-10863/entity-updates
branch
from
66c93e6
to
0623639
Compare
| @@ -55,6 +55,9 @@ public OrganizationResponseModel(Organization organization, string obj = "organi | |||
| SmServiceAccounts = organization.SmServiceAccounts; | |||
| MaxAutoscaleSmSeats = organization.MaxAutoscaleSmSeats; | |||
| MaxAutoscaleSmServiceAccounts = organization.MaxAutoscaleSmServiceAccounts; | |||
| LimitCollectionCreation = organization.LimitCollectionCreation; | |||
| LimitCollectionDeletion = organization.LimitCollectionDeletion; | |||
| // Deperectated: https://bitwarden.atlassian.net/browse/PM-10863 | |||
There was a problem hiding this comment.
I actually love this misspelling
There was a problem hiding this comment.
So do I, but I did correct it anyway.
Commit: Fix mispelling
| var limitCollectionCreationEnabled = !(_featureService.IsEnabled(FeatureFlagKeys.LimitCollectionCreationDeletionSplit) | ||
| ? organizationAbility is { LimitCollectionCreation: false } | ||
| : organizationAbility is { LimitCollectionCreationDeletion: false }); |
There was a problem hiding this comment.
I think the true/false case is around the wrong way here. Also the variable is called enabled but it's set to true if the setting is disabled. Maybe the ! operator has been applied in the wrong place?
Would this work?
| var limitCollectionCreationEnabled = !(_featureService.IsEnabled(FeatureFlagKeys.LimitCollectionCreationDeletionSplit) | |
| ? organizationAbility is { LimitCollectionCreation: false } | |
| : organizationAbility is { LimitCollectionCreationDeletion: false }); | |
| var limitCollectionCreation = _featureService.IsEnabled(FeatureFlagKeys.LimitCollectionCreationDeletionSplit | |
| ? organizationAbility is { LimitCollectionCreation: true } | |
| : organizationAbility is { LimitCollectionCreationDeletion: true }); |
There was a problem hiding this comment.
This conditional logic is really messy, but changing it too much breaks tests. An exact implementation of your suggestion makes logical sense, and is what I tried to do at first, but something here isn't right and it seemed out of scope for me to spend a lot of time on it.
You can see this by looking at the Github Actions runs on the PR. They pass right before this commit which is an exact implementation of your suggestion that fails.
As for this specifically:
Also the variable is called enabled but it's set to true if the setting is disabled.
That's not true. The variable is set to true if the setting is enabled. The right side of this equality operator is completely wrapped in an inversion, so the output of the whole ternary is inverted. The ternary evaluates to true if the setting is false and then that whole thing is flipped before the assignment to the variable.
Again, I know this is not very pleasant. I'm open to suggestions on how to improve this without getting off track too much from the work being done.
I'll drop the busted commit once you check it out.
| var limitCollectionDeletionEnabled = !(_featureService.IsEnabled(FeatureFlagKeys.LimitCollectionCreationDeletionSplit) | ||
| ? organizationAbility is { LimitCollectionDeletion: false } | ||
| : organizationAbility is { LimitCollectionCreationDeletion: false }); | ||
|
|
||
| var canDeleteManagedCollections = | ||
| !limitCollectionDeletionEnabled || | ||
| org is { Type: OrganizationUserType.Owner or OrganizationUserType.Admin }; |
| /* | ||
| * Version 14 added LimitCollectionCreationDeletion and Version 15 added AllowAdminAccessToAllCollectionItems, | ||
| * however these are just user settings and it is not worth failing validation if they mismatch. | ||
| * They are intentionally excluded. | ||
| */ | ||
|
|
There was a problem hiding this comment.
I think it's worth keeping some version of this comment, otherwise there's just an unexplained gap in the validation for version 14. Maybe
Version 14 added LimitCollectionCreationDeletion and Version 15 added AllowAdminAccessToAllCollectionItems, however they are no longer used and are intentionally excluded from validation.
There was a problem hiding this comment.
Added as written
Commit: Re-add contextual comment regarding dropped license properties
| @@ -253,7 +253,8 @@ public async Task GetManyDetailsByUserAsync_Works(IUserRepository userRepository | |||
| Assert.Equal(orgUser1.Permissions, result.Permissions); | |||
| Assert.Equal(organization.SmSeats, result.SmSeats); | |||
| Assert.Equal(organization.SmServiceAccounts, result.SmServiceAccounts); | |||
| Assert.Equal(organization.LimitCollectionCreationDeletion, result.LimitCollectionCreationDeletion); | |||
There was a problem hiding this comment.
Do we want to keep the existing assertion until we remove the property altogether?
There was a problem hiding this comment.
Yes. I've added this back.
| LimitCollectionCreationDeletion = true, | ||
| AllowAdminAccessToAllCollectionItems = true, |
There was a problem hiding this comment.
You should keep these values here given that we're continuing to include them in the license file for now.
There was a problem hiding this comment.
Added back!
| @@ -300,7 +313,6 @@ public void UpdateFromLicense(OrganizationLicense license) | |||
| UseSecretsManager = license.UseSecretsManager; | |||
| SmSeats = license.SmSeats; | |||
| SmServiceAccounts = license.SmServiceAccounts; | |||
| LimitCollectionCreationDeletion = license.LimitCollectionCreationDeletion; | |||
There was a problem hiding this comment.
This method is used when updating an existing org from a license file. You've removed the LimitCollectionCreationDeletion assignment, but not AllowAdminAccessToAllCollectionItems, which is inconsistent.
Also, the OrganizationService.SignUpAsync method (which creates a new org from a license file) has this changed feature flagged, but here it's not feature flagged.
There was a problem hiding this comment.
I've added this removed property assignment back, and feature toggled both it and the AllowAdminAccessToAllCollectionItems assignment. This should look the same between Create and Update operations now.
Commit: Improve feature toggling scenerios for self hosted org creation/update
There was a problem hiding this comment.
Please explain to Priya how this will work for older self-hosted customers. Not everyone upgrades immediately, there is likely to be a period where they see 1 setting in self-host but the 2 new settings in cloud, and still need to update it via the license file. Your mapping code will handle this by ORing the 2 new values, but we may have to answer some questions about it in the meantime.
There was a problem hiding this comment.
Relatedly - how are we timing the rollout? If we enable the feature flag in cloud first, then self-hosted customers will be in this state at least until the next self-hosted release. That's probably unavoidable but worth discussing.
There was a problem hiding this comment.
I'm planning to record some demos. I'll include one of an out-of-date self hosted combination and attach in Jira with an explanation. As for the rollout: what you said is what I was thinking. I don't know what else we could do other than release to self host with the feature on alongside cloud and hope for the best. Seems better to lag a little and explain.
There was a problem hiding this comment.
Note that the endpoint for this (OrganizationsController.PutCollectionManagement) is still blocked for self-hosted. Does this need to be changed in this PR?
There was a problem hiding this comment.
I've unblocked this endpoint. Thanks for catching that.
addisonbeck
force-pushed
the
ac/addison/pm-10863/entity-updates
branch
4 times, most recently
from
b634f66
to
0103218
Compare
addisonbeck
force-pushed
the
ac/addison/pm-10863/entity-updates
branch
from
0103218
to
d9703a0
Compare
🎟️ Tracking
🧵 Jira Ticket: PM-10863
📚 Stacked PRs
server: SplitLimitCollectionCreationDeletioninto two database columns #4709server: SplitOrganization.LimitCollectionCreationDeletioninto two separate business rules #4730⬆️ YOU ARE HERE
clients: SplitOrganization.LimitCollectionCreationDeletioninto two separate business rules clients#11223server: Turn onLimitCollectionCreationDeletionSplitfor self host #4808server: RemoveLimitCollectionCreationDeletionSplitfeature flag #4809clients: RemoveLimitCollectionCreationDeletionSplitfeature flag clients#11258server: DropLimitCollectionCreationDeletionfrom the database #4810📔 Objective
🤳 The Bigger Picture
There is an option in the Admin Console that removes collection creation and
deletion controls from all organization members that are not in the Owner and
Admin roles. This PR stack splits this conjoined option into two separate
controls: one to block
createoperations and one to blockdeleteoperations. This is a part of an effort to make these settings better align
with what customers expect it to do.
👉 This Pull Request 👍
In the previous PR we added two new database columns to facilitate a rename &
split operation on an existing column. In this PR we go about implementing
the new columns in
servermodels and implementing the desired functionalityof those new properties throughout business logic and tests.
This PR and the two following it encompass all of the functional changes to
the system implemented by this PR stack. The next PR implements the feature
in the admin console UI in
clients, and the one after that enables thefeature for self hosted installations. The first and second PRs are
maintenance tasks to support evolutionary database design and entity
framework.
🧮 Side Effects
There are several side effects involved in this PR. This includes:
organization properties.
AllowAdminsAccessToAllCollectionsfrom the license file as part of thiswork.
backwards compatibility.
📸 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