-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PM-13347] Web app impacts on the Remove Bitwarden Families policy #12056
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #12056 +/- ##
==========================================
- Coverage 33.47% 33.47% -0.01%
==========================================
Files 2878 2880 +2
Lines 89941 90010 +69
Branches 17116 17130 +14
==========================================
+ Hits 30106 30127 +21
- Misses 57455 57497 +42
- Partials 2380 2386 +6 ☔ View full report in Codecov by Sentry. |
No New Or Fixed Issues Found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need a more accurate PR title
apps/web/src/app/billing/settings/sponsored-families.component.ts
Outdated
Show resolved
Hide resolved
this.availableSponsorshipOrgs$ = this.organizationService.organizations$.pipe( | ||
switchMap((orgs) => | ||
combineLatest( | ||
orgs.map((o) => | ||
from( | ||
this.PolicyApiService.getPolicyStatus( | ||
o.id, | ||
PolicyType.FreeFamiliesSponsorshipPolicy, | ||
), | ||
).pipe( | ||
map((isFreeFamilyPolicyEnabled) => ({ | ||
organization: o, | ||
isPolicyEnabled: isFreeFamilyPolicyEnabled ?? false, | ||
})), | ||
), | ||
), | ||
), | ||
), | ||
map((orgsWithPolicies) => | ||
orgsWithPolicies | ||
.filter( | ||
({ organization, isPolicyEnabled }) => | ||
organization.familySponsorshipAvailable && !isPolicyEnabled, | ||
) | ||
.map(({ organization }) => organization), | ||
), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: before refactoring this, check out my comment below as that will affect your approach here. However I still wanted to leave this comment FYI.
This is quite difficult to understand what's going on here. It is a fairly complex set of operations though. I had to rewrite it myself to be able to provide any feedback here, so here's what I came up with:
const getPolicyStatus$ = (org: Organization) =>
from(
this.PolicyApiService.getPolicyStatus(org.id, PolicyType.FreeFamiliesSponsorshipPolicy),
).pipe(map((policyEnabled) => ({ organization: org, isPolicyEnabled: policyEnabled })));
this.availableSponsorshipOrgs$ = this.organizationService.organizations$.pipe(
map((orgs) => orgs.filter((o) => o.familySponsorshipAvailable)),
switchMap((orgs) => forkJoin(orgs.map((o) => getPolicyStatus$(o)))),
map((orgsWithPolicies) =>
orgsWithPolicies.filter(({ isPolicyEnabled }) => !isPolicyEnabled),
),
map((orgsWithPolicies) => orgsWithPolicies.map(({ organization }) => organization)),
);
It actually turned out fairly similar to yours. I'd say the main differences are:
- filter out orgs without family sponsorships available before calling the api, just to save on api traffic
- extract the nested api call to its own method to avoid unnecessary nesting. I think maintaining a flat structure to rxjs logic is really important for readability
- use
forkJoin
instead ofcombineLatest
.forkJoin
will wait for all inner observables to complete, then emit an array of the results, kind of likePromise.all
. This is a better fit for this case rather thancombineLatest
which emits ongoing updates. - destructuring callback arguments is a bit tidier and easier to read
apps/web/src/app/billing/settings/sponsoring-org-row.component.ts
Outdated
Show resolved
Hide resolved
const enterpriseOrganizations = organizations.filter( | ||
(org) => org.productTierType === ProductTierType.Enterprise, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally avoid checking the plan type directly. Would canManageSponsorships
work here?
}, | ||
), | ||
); | ||
return firstValueFrom(result$); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all new code, if you want to use observables then you should use them all the way through your methods (e.g. return an observable, then pipe the result) rather than changing them back into promises.
libs/common/src/admin-console/services/policy/policy-api.service.ts
Outdated
Show resolved
Hide resolved
constructor(private policyService: PolicyService) {} | ||
|
||
getPolicyStatus$(orgId: string, policyType: PolicyType) { | ||
return this.policyService.policies$.pipe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use policyService.getAll$
instead. That will get you the policyType that you want and has additional business logic to make sure you're getting valid policies. (Really policies$
should be deprecated.) Then all you would need to filter at your end is the orgId.
The only "gotcha" with getAll$
is that it will automatically assume that your policy shouldn't be enforced against owners and admins. If this is intended to disable F4E for everyone, add an exception for your policy in policyService.isExemptFromPolicy
like so:
case PolicyType.FreeFamiliesSponsorshipPolicy:
// policy applies to everyone
return false;
belongToMultipleEnterpriseOrgs: boolean; | ||
} { | ||
const enterpriseOrganizations = organizations.filter( | ||
(org) => org.productTierType === ProductTierType.Enterprise, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unresolved feedback from last review: #12056 (comment)
<ng-container *ngIf="isFreeFamilyFlagEnabled; else defaultState"> | ||
<bit-nav-item | ||
[text]="'sponsoredFamilies' | i18n" | ||
route="settings/sponsored-families" | ||
*ngIf="(hasFamilySponsorshipAvailable$ | async) && showFreeFamilyLink" | ||
></bit-nav-item> | ||
</ng-container> | ||
<ng-template #defaultState> | ||
<bit-nav-item | ||
[text]="'sponsoredFamilies' | i18n" | ||
route="settings/sponsored-families" | ||
*ngIf="hasFamilySponsorshipAvailable$ | async" | ||
></bit-nav-item> | ||
</ng-template> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This group of conditionals feels overly complex. Add the end of the day we are rendering the same item, so it should be possible to use only a single ngIf
(ideally by moving the logic to the TS file):
<bit-nav-item
[text]="'sponsoredFamilies' | i18n"
route="settings/sponsored-families"
*ngIf="showSponsoredFamilies$ | async"
></bit-nav-item>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usage of PolicyService
is looking good, and the api calls have been removed, which were my main concerns from an AC Team perspective. I've left some feedback below but I'll leave the rest to Billing Team 👍
this.hasFamilySponsorshipAvailable$ = this.organizationService.canManageSponsorships$; | ||
|
||
// We want to hide the subscription menu for organizations that provide premium. | ||
// Except if the user has premium personally or has a billing history. | ||
this.showSubscription$ = combineLatest([ | ||
this.showSubscription$ = forkJoin([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forkJoin
only emits after the source observables complete. Make sure that these observables complete before using it here. (I'm not sure they do)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an existing codebase, it is not the part of changes i added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry about that, I have reverted the change. Thanks for pointing that out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AC changes look good, thanks for working through it.
this.isFreeFamilyFlagEnabled = await this.configService.getFeatureFlag( | ||
FeatureFlag.DisableFreeFamiliesSponsorship, | ||
); | ||
|
||
if (this.isFreeFamilyFlagEnabled) { | ||
const enterpriseOrgStatus$ = this.isFreeFamilyFlagEnabled | ||
? this.freeFamiliesPolicyService.checkEnterpriseOrganizationsAndFetchPolicy() | ||
: of(null); | ||
|
||
this.showSponsoredFamilies$ = combineLatest([ | ||
enterpriseOrgStatus$, | ||
this.organizationService.canManageSponsorships$, | ||
]).pipe( | ||
map(([orgStatus, canManageSponsorships]) => { | ||
const showFreeFamilyLink = | ||
orgStatus && | ||
!(orgStatus.belongToOneEnterpriseOrgs && orgStatus.isFreeFamilyPolicyEnabled); | ||
return canManageSponsorships && showFreeFamilyLink; | ||
}), | ||
); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still a lot of billing-specific logic for a shared layout component. I would move all of this logic into the service you created:
this.showFreeFamilies$ = this.freeFamiliesPolicyService.showFreeFamilies$;
<bit-nav-item
[text]="'sponsoredFamilies' | i18n"
route="settings/sponsored-families"
*ngIf="showFreeFamilies$ | async"
></bit-nav-item>
Even better is to move the logic and template into its own component:
<billing-free-families-nav-item></billing-free-families-nav-item>
da8d403
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New component looks good! Last request from me is to move it into a folder owned by the Billing team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DS-owned files look good to me! Thanks for iterating @cyprain-okeke 👏
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-13347
📔 Objective
When the Remove Bitwarden Families policy is on, the product will follow the following behaviors depending on the # of organizations the user is part of.
Individual vault impacts - Scenario 1
Given the users email address is only associated with 1 enterprise organization, when the policy is turned on, then hide the Free Bitwarden Families page from the navigation
Individual vault impacts - Scenario 2
Given the email address is associated with multiple enterprise organization, the policy is turned on, and my sponsorship is status is active, when the user attempts to interact with the three dot menu, then the only option is to remove the sponsorship
Individual vault impacts - Scenario 3
Given the email address is associated with multiple enterprise organization, the policy is turned on, and my sponsorship is status is sent, when the user attempts to interact with the three dot menu, then the only option is to remove the sponsorship and we hide the Resend email option
📸 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