Skip to content
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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

cyprain-okeke
Copy link
Contributor

🎟️ 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

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

Copy link

codecov bot commented Nov 19, 2024

Codecov Report

Attention: Patch coverage is 32.46753% with 52 lines in your changes missing coverage. Please review.

Project coverage is 33.47%. Comparing base (7d6da0a) to head (278a238).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...p/billing/services/free-families-policy.service.ts 21.05% 28 Missing and 2 partials ⚠️
...p/billing/settings/sponsored-families.component.ts 27.77% 12 Missing and 1 partial ⚠️
...p/billing/settings/sponsoring-org-row.component.ts 41.66% 6 Missing and 1 partial ⚠️
...shared/billing-free-families-nav-item.component.ts 71.42% 1 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented Nov 19, 2024

Logo
Checkmarx One – Scan Summary & Detailsfbc42134-e225-4c5f-918d-a8dce61016cc

No New Or Fixed Issues Found

Copy link
Member

@jonashendrickx jonashendrickx left a 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

@cyprain-okeke cyprain-okeke changed the title [PM-13347] Web app impacts [PM-13347] Web app impacts on the Remove Bitwarden Families policy Nov 20, 2024
Comment on lines 96 to 122
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),
),
);
Copy link
Member

@eliykat eliykat Nov 21, 2024

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:

  1. filter out orgs without family sponsorships available before calling the api, just to save on api traffic
  2. 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
  3. use forkJoin instead of combineLatest. forkJoin will wait for all inner observables to complete, then emit an array of the results, kind of like Promise.all. This is a better fit for this case rather than combineLatest which emits ongoing updates.
  4. destructuring callback arguments is a bit tidier and easier to read

apps/web/src/app/layouts/user-layout.component.ts Outdated Show resolved Hide resolved
Comment on lines 142 to 144
const enterpriseOrganizations = organizations.filter(
(org) => org.productTierType === ProductTierType.Enterprise,
);
Copy link
Member

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$);
Copy link
Member

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.

constructor(private policyService: PolicyService) {}

getPolicyStatus$(orgId: string, policyType: PolicyType) {
return this.policyService.policies$.pipe(
Copy link
Member

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,
Copy link
Member

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)

Comment on lines 27 to 40
<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>
Copy link
Contributor

@willmartian willmartian Nov 22, 2024

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>

eliykat
eliykat previously approved these changes Nov 25, 2024
Copy link
Member

@eliykat eliykat left a 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 👍

apps/web/src/app/layouts/user-layout.component.ts Outdated Show resolved Hide resolved
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([
Copy link
Member

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)

Copy link
Contributor Author

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

Copy link
Contributor Author

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

apps/web/src/app/layouts/user-layout.component.ts Outdated Show resolved Hide resolved
@eliykat eliykat dismissed their stale review November 25, 2024 04:12

Meant to request changes

eliykat
eliykat previously approved these changes Nov 25, 2024
Copy link
Member

@eliykat eliykat left a 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.

jonashendrickx
jonashendrickx previously approved these changes Nov 26, 2024
Comment on lines 49 to 70
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;
}),
);
}

Copy link
Contributor

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>

Copy link
Contributor

@willmartian willmartian left a 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.

Copy link
Contributor

@willmartian willmartian left a 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 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants