[PM-31978] Expand Organization Ability documentation#6970
Conversation
eliykat
requested review from
a team,
amorask-bitwarden,
r-tome and
sven-bitwarden
and removed request for
a team
|
|
|
Claude finished @eliykat's task in 3m 25s —— View job Reviewing PR #6970
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6970 +/- ##
==========================================
+ Coverage 56.26% 56.46% +0.20%
==========================================
Files 1983 1996 +13
Lines 87651 88039 +388
Branches 7815 7845 +30
==========================================
+ Hits 49317 49715 +398
+ Misses 36504 36487 -17
- Partials 1830 1837 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
connerbw
requested review from
kdenney
and removed request for
amorask-bitwarden
|
@eliykat The only question I have - you outlined the difference between organization abilities and feature flags, but what considerations go into organization abilities vs policies? Is that worth documenting here or is it already elsewhere? |
kdenney
left a comment
kdenney
left a comment
There was a problem hiding this comment.
I was able to confirm these patterns and what changes are needed. Please let me know if I missed anything or if you have any more questions. FYI our team is planning to address this as technical debt to clean up in a future sprint so hopefully this is greatly simplified soon.
| > ⚠️ **WARNING:** Mistakes in organization license changes can disable the entire organization for self-hosted customers! | ||
| > Double-check your work and ask for help if unsure. | ||
| > | ||
| > **Note:** Do not add new properties to the `OrganizationLicense` file - make sure you use the claims-based system below. |
There was a problem hiding this comment.
I confirmed with Conner that this is wrong. New features do need added to OrganizationLicense because those properties are still used in the UpdateOrganizationLicenseCommand to update the organization properties from the license properties. So this needs removed and other changes below as well.
Note: I will likely miss a few spots here but if you add a property and add it to LicenseConstants and then run tests in UpdateOrganizationLicenseCommandTests.cs, the test failures will guide you to all of the changes required so that should help identify areas that need documented.
| **Update tests:** | ||
|
|
||
| - `test/Core.Test/Billing/Organizations/Commands/UpdateOrganizationLicenseCommandTests.cs` | ||
| - Exclude from test comparison (line ~91) |
There was a problem hiding this comment.
I believe this should be removed. Since we are adding the properties to both classes, I believe we do want this test to include them in this comparison.
|
|
||
| **Update tests:** | ||
|
|
||
| - `test/Core.Test/Billing/Organizations/Commands/UpdateOrganizationLicenseCommandTests.cs` |
There was a problem hiding this comment.
It looks to me like you probably want to also add the new property to this test: UpdateLicenseAsync_WithClaimsPrincipal_ExtractsAllPropertiesFromClaims
| > Double-check your work and ask for help if unsure. | ||
| > | ||
| > **Note:** Do not add new properties to the `OrganizationLicense` file - make sure you use the claims-based system below. | ||
|
|
There was a problem hiding this comment.
It is also required to add the new property to the UpdateFromLicense() method in src/Core/AdminConsole/Entities/Organization.cs
|
|
||
| **Update license verification:** | ||
|
|
||
| - `src/Core/Billing/Organizations/Models/OrganizationLicense.cs` |
There was a problem hiding this comment.
I'm pretty sure you also need to add the new property to the GetDataBytes() method in this file to the section at the bottom below this comment:
// any new fields added need to be added here so that they're ignored
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
@kdenney and @sven-bitwarden your comments should be addressed in the last commits. |
kdenney
left a comment
kdenney
left a comment
There was a problem hiding this comment.
Licensing changes all look good now. Nice job!
eliykat
deleted the
ac/pm-31978/expand-organizationability-documentation
branch
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-31978
📔 Objective
In #6781 we added initial documentation around the organization ability pattern. This PR incorporates additional documentation from Confluence, including a step-by-step guide with file references. This has been cross-checked and updated. The confluence doc will be deleted or archived to avoid confusion.
📸 Screenshots