clients icon indicating copy to clipboard operation
clients copied to clipboard

[PM-5971] Fix Payment Method Warning Bugs

Open amorask-bitwarden opened this issue 1 year ago • 4 comments
trafficstars

Type of change

- [X] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

This PR refactors the implementation of the payment method warning banners to continuously poll the /organizations/{organizationId}/billing-status endpoint for whether the active user's organizations risk subscription failure. It uses the responses from those endpoints to update an ActiveUserState observable used to determine whether or not to display the payment method warning banners.

Key Code Changes

Rendering

  • organization-layout.component.*: Render newly refactored component if FF is on.
  • user-layout.component.*: Render newly refactored component if FF is on.
  • providers-layout.componen.*: Render newly refactored component if FF is on.
  • setup.component.*: Render newly refactored component if FF is on.
  • payment-method-warning-banners.component.*: Consume the new paymentMethodWarningService.paymentMethodWarnings$ observable and display the resulting banners accordingly.

Business Logic

  • organization-api.service.*: Rename endpoint method to target /billing-status per https://github.com/bitwarden/server/pull/3790.
  • payment-method-warning.service.*: Main bulk of the new logic around fetching each organization's billing status on a timer and updating the ActiveUserState observable.
  • app.component.ts: Clear session storage from payment-method-warning.service when user logs out.
  • adjust-payment.component.ts: Hide the payment method warning banner when a user adds a payment method.
  • feature-flag.enum.ts: Add new feature flat to control display.

Unrelated Refactor

  • organization-billing.service.*: Move request models to their own file. Refactor existing methods.
  • secrets-manager-trial-billing-step.component.ts: Use new organizationBillingService method name.
  • secrets-manager-trial-free-stepper.component.ts: Use new organizationBillingService method name.

Screenshots

https://github.com/bitwarden/clients/assets/144709477/f23e89e4-e1d9-4416-9912-8a9db0b069b6

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team
  • Ensure that all UI additions follow WCAG AA requirements

amorask-bitwarden avatar Feb 12 '24 22:02 amorask-bitwarden

Codecov Report

Attention: Patch coverage is 56.70103% with 42 lines in your changes are missing coverage. Please review.

Project coverage is 24.96%. Comparing base (c8e36b6) to head (b52e077).

Files Patch % Lines
apps/web/src/app/app.component.ts 0.00% 12 Missing :warning:
...thod-warnings/payment-method-warnings.component.ts 40.00% 5 Missing and 1 partial :warning:
...in-console/providers/providers-layout.component.ts 0.00% 5 Missing :warning:
...p/admin-console/providers/setup/setup.component.ts 0.00% 4 Missing :warning:
...common/src/billing/services/billing-api.service.ts 0.00% 3 Missing :warning:
...nizations/layouts/organization-layout.component.ts 60.00% 2 Missing :warning:
...src/app/billing/shared/adjust-payment.component.ts 33.33% 2 Missing :warning:
apps/web/src/app/layouts/user-layout.component.ts 60.00% 2 Missing :warning:
libs/angular/src/services/jslib-services.module.ts 0.00% 2 Missing :warning:
...rc/app/admin-console/providers/providers.module.ts 0.00% 1 Missing :warning:
... and 3 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7923      +/-   ##
==========================================
+ Coverage   24.92%   24.96%   +0.04%     
==========================================
  Files        2239     2242       +3     
  Lines       65596    65637      +41     
  Branches    12376    12379       +3     
==========================================
+ Hits        16347    16388      +41     
  Misses      47916    47916              
  Partials     1333     1333              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 12 '24 22:02 codecov[bot]

Logo Checkmarx One – Scan Summary & Details1f656889-8e4f-48d7-94f5-de49f1870d4f

No New Or Fixed Issues Found

bitwarden-bot avatar Feb 13 '24 00:02 bitwarden-bot

@MGibson1 Really appreciate the thorough review; learning a lot with this one that's very helpful. I think I resolved the remaining feedback. I also added tests for the PaymentMethodWarningsService.

amorask-bitwarden avatar Feb 15 '24 17:02 amorask-bitwarden

Hi @eliykat, thank you for the review. I've addressed all of your feedback here: https://github.com/bitwarden/clients/pull/7923/commits/c14dfc83918b4c839bad531eed9f5f046973f5f0 except for the part about the standalone component where you asked @MGibson1 for input.

In the interest of not holding this open too long, would it be possible to leave the component as is and open a ticket to refactor it into a standalone component if need be down the line? Thanks in advance!

amorask-bitwarden avatar Feb 22 '24 18:02 amorask-bitwarden

@MGibson1 @eliykat @cyprain-okeke @r-tome Thank you all very much for your help and reviews here. I think I've resolved the remainder of the feedback and resolved the conflicts related to the wide-scale layout changes.

If I could one more round of re-reviews from each of you to wrap this up when you have the chance, I'd greatly appreciate it.

amorask-bitwarden avatar Feb 27 '24 14:02 amorask-bitwarden