PM-37911: Feat: Update Organization model#6960
Conversation
github-actions
Bot
added
app:password-manager
david-livefront
force-pushed
the
PM-37911-organization-network-model
branch
from
aee1dd1 to
2f37379
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6960 +/- ##
==========================================
- Coverage 86.26% 84.31% -1.96%
==========================================
Files 1008 1008
Lines 66332 66433 +101
Branches 9308 9316 +8
==========================================
- Hits 57221 56010 -1211
- Misses 5902 7223 +1321
+ Partials 3209 3200 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
david-livefront
force-pushed
the
PM-37911-organization-network-model
branch
4 times, most recently
from
0d72f25 to
0851a91
Compare
SaintPatrck
added
the
ai-review-vnext
Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR expands the Code Review DetailsNo findings. |
david-livefront
force-pushed
the
PM-37911-organization-network-model
branch
4 times, most recently
from
7671d18 to
c4cab72
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| userIsClaimedByOrganization: Boolean = false, | ||
| limitItemDeletion: Boolean = false, | ||
| useSso: Boolean = false, | ||
| useOrganizationDomains: Boolean = false, | ||
| shouldUseKeyConnector: Boolean = false, | ||
| useScim: Boolean = false, | ||
| useCustomPermissions: Boolean = false, | ||
| useResetPassword: Boolean = false, | ||
| useSecretsManager: Boolean = false, | ||
| usePasswordManager: Boolean = false, | ||
| useActivateAutofillPolicy: Boolean = false, | ||
| useAutomaticUserConfirmation: Boolean = false, | ||
| ssoBound: Boolean = false, | ||
| resetPasswordEnrolled: Boolean = false, | ||
| organizationUserId: String? = "mockOrganizationUserId-$number", | ||
| hasPublicAndPrivateKeys: Boolean = false, | ||
| isProviderUser: Boolean = false, | ||
| isMember: Boolean = false, | ||
| familySponsorshipAvailable: Boolean = false, | ||
| productTierType: ProductTierType = ProductTierType.FREE, | ||
| accessSecretsManager: Boolean = false, | ||
| limitCollectionCreation: Boolean = false, | ||
| limitCollectionDeletion: Boolean = false, | ||
| allowAdminAccessToAllCollectionItems: Boolean = false, | ||
| useAccessIntelligence: Boolean = false, | ||
| useAdminSponsoredFamilies: Boolean = false, | ||
| useDisableSmAdsForUsers: Boolean = false, | ||
| isAdminInitiated: Boolean = false, | ||
| ssoEnabled: Boolean = false, | ||
| ssoMemberDecryptionType: MemberDecryptionType? = null, | ||
| usePhishingBlocker: Boolean = false, | ||
| useMyItems: Boolean = false, | ||
| ): ProfileOrganization = | ||
| ProfileOrganization( | ||
| id = id, | ||
| name = name, | ||
| status = status, | ||
| type = type, | ||
| enabled = enabled, | ||
| usePolicies = usePolicies, | ||
| useGroups = useGroups, | ||
| useDirectory = useDirectory, | ||
| useEvents = shouldUseEvents, | ||
| useTotp = useTotp, | ||
| use2fa = use2fa, | ||
| useApi = useApi, | ||
| useSso = useSso, | ||
| useOrganizationDomains = useOrganizationDomains, | ||
| useKeyConnector = shouldUseKeyConnector, | ||
| useScim = useScim, | ||
| useCustomPermissions = useCustomPermissions, | ||
| useResetPassword = useResetPassword, | ||
| useSecretsManager = useSecretsManager, | ||
| usePasswordManager = usePasswordManager, | ||
| useActivateAutofillPolicy = useActivateAutofillPolicy, | ||
| useAutomaticUserConfirmation = useAutomaticUserConfirmation, | ||
| selfHost = selfHost, | ||
| usersGetPremium = usersGetPremium, | ||
| seats = seats, | ||
| maxCollections = maxCollections, | ||
| maxStorageGb = maxStorageGb, | ||
| ssoBound = ssoBound, | ||
| identifier = identifier, | ||
| permissions = permissions, | ||
| resetPasswordEnrolled = resetPasswordEnrolled, | ||
| userId = userId, | ||
| organizationUserId = organizationUserId, | ||
| hasPublicAndPrivateKeys = hasPublicAndPrivateKeys, | ||
| providerId = providerId, | ||
| providerName = providerName, | ||
| providerType = providerType, | ||
| isProviderUser = isProviderUser, | ||
| isMember = isMember, | ||
| familySponsorshipFriendlyName = familySponsorshipFriendlyName, | ||
| familySponsorshipAvailable = familySponsorshipAvailable, | ||
| productTierType = productTierType, | ||
| keyConnectorEnabled = keyConnectorEnabled, | ||
| keyConnectorUrl = keyConnectorUrl, | ||
| familySponsorshipLastSyncDate = familySponsorshipLastSyncDate, | ||
| familySponsorshipValidUntil = familySponsorshipValidUntil, | ||
| familySponsorshipToDelete = familySponsorshipToDelete, | ||
| accessSecretsManager = accessSecretsManager, | ||
| limitCollectionCreation = limitCollectionCreation, | ||
| limitCollectionDeletion = limitCollectionDeletion, | ||
| limitItemDeletion = limitItemDeletion, | ||
| allowAdminAccessToAllCollectionItems = allowAdminAccessToAllCollectionItems, | ||
| userIsManagedByOrganization = userIsClaimedByOrganization, |
There was a problem hiding this comment.
🎨 SUGGESTED: The parameter userIsClaimedByOrganization populates the SDK's userIsManagedByOrganization field, which is semantically a different concept.
Details and fix
On line 49 the helper accepts userIsClaimedByOrganization, but on line 135 it is mapped to userIsManagedByOrganization. The network model has both userIsClaimedByOrganization and userIsManagedByOrganization as distinct fields, and the production mapper at SyncResponseJsonExtensions.kt:122 correctly uses this.userIsManagedByOrganization. As-is, a future test that does createMockSdkProfileOrganization(userIsClaimedByOrganization = true) will not produce an equivalent of createMockOrganizationNetwork(userIsClaimedByOrganization = true).toSdkProfileOrganization(), since the network mock only flips userIsClaimedByOrganization while the SDK mock flips userIsManagedByOrganization.
Suggested fix: rename the parameter to userIsManagedByOrganization (default false) so the helper matches the SDK's actual field. This also makes it possible to assert the correct mapping path when a future test needs to verify userIsManagedByOrganization = true.
There was a problem hiding this comment.
@claude userIsManagedByOrganization is the deprecated name for the userIsClaimedByOrganization. We want to use the new name wherever possible
There was a problem hiding this comment.
Claude finished @david-livefront's task in 44s —— View job
Response
Thanks for the clarification — you're right, and my earlier suggestion was wrong on this. Tracing through:
- The network model only exposes the new name:
SyncResponseJson.kt:455-456definesuserIsClaimedByOrganization. - The SDK still uses the deprecated name:
ProfileOrganization.userIsManagedByOrganization. - The mapper bridges them at
SyncResponseJsonExtensions.kt:122(userIsManagedByOrganization = this.userIsClaimedByOrganization). - The helper does the same bridge at
ProfileOrganizationUtil.kt:135so the parameter name on both the network and SDK mock factories reads asuserIsClaimedByOrganization, which is the convention you want.
That means createMockSdkProfileOrganization(userIsClaimedByOrganization = true) and createMockOrganizationNetwork(userIsClaimedByOrganization = true).toSdkProfileOrganization() do produce equivalent SDK objects — the concern in my original comment doesn't hold. No change needed.
david-livefront
force-pushed
the
PM-37911-organization-network-model
branch
from
c4cab72 to
c86916a
Compare
david-livefront
force-pushed
the
PM-37911-organization-network-model
branch
from
c86916a to
f3f06cc
Compare
| */ | ||
| @Suppress("MaxLineLength") | ||
| fun List<SyncResponseJson.Profile.Organization>.toSdkProfileOrganizations(): List<ProfileOrganization> = | ||
| this.mapNotNull { it.toSdkProfileOrganization() } |
There was a problem hiding this comment.
This function is currently used only for tests, but it will keep us in-sync with the SDK.
I will also be updating other spots in the app to use ProfileOrganization in a future PR.
|
Thanks @SaintPatrck |
2 participants
🎟️ Tracking
PM-37911
📔 Objective
This PR updates the
Organizationnetwork model to include significantly more data. This data is required in order to match the SDKProfileOrganization.