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-18955] Improve performance of Billing invocations on Collections page #14159

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

Conversation

jonashendrickx
Copy link
Member

@jonashendrickx jonashendrickx commented Apr 7, 2025

…ions

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-18955

📔 Objective

There were two options to solve checking whether a payment method already exists for a trial subscription:

  • Whether country and postal code are entered for a customer, both of which are required for sales tax compliance. Already exposed by the metadata endpoint. However, it's not a guarantee that the payment method still exists once the billing information has been entered

  • customer.invoice_settings has a property with the default payment id. We would have to return it with the metadata endpoint. Is more thorough than bullet point i18n #1.

📸 Screenshots

Screen.Recording.2025-04-07.at.16.41.13.mov

⏰ 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

@jonashendrickx jonashendrickx requested review from a team as code owners April 7, 2025 13:42
@jonashendrickx jonashendrickx requested a review from JimmyVo16 April 7, 2025 13:42
Copy link

codecov bot commented Apr 7, 2025

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.

Project coverage is 37.07%. Comparing base (ad2e3a4) to head (4cc8421).
Report is 17 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...nsole/organizations/collections/vault.component.ts 0.00% 7 Missing ⚠️
...web/src/app/billing/services/trial-flow.service.ts 0.00% 1 Missing ⚠️
...src/app/billing/shared/payment-method.component.ts 0.00% 1 Missing ⚠️
...app/secrets-manager/overview/overview.component.ts 0.00% 1 Missing ⚠️
...response/organization-billing-metadata.response.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14159      +/-   ##
==========================================
+ Coverage   37.05%   37.07%   +0.02%     
==========================================
  Files        3194     3203       +9     
  Lines       92352    92531     +179     
  Branches    16571    16593      +22     
==========================================
+ Hits        34217    34308      +91     
- Misses      55592    55677      +85     
- Partials     2543     2546       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

github-actions bot commented Apr 7, 2025

Logo
Checkmarx One – Scan Summary & Details03d8cee1-5a43-4e86-8880-12ae8eb4f348

Great job, no security vulnerabilities found in this Pull Request

@jonashendrickx jonashendrickx added the needs-qa Marks a PR as requiring QA approval label Apr 8, 2025
JimmyVo16
JimmyVo16 previously approved these changes Apr 8, 2025
Copy link
Contributor

@JimmyVo16 JimmyVo16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from AC

@jonashendrickx jonashendrickx removed the needs-qa Marks a PR as requiring QA approval label Apr 8, 2025
Copy link

sonarqubecloud bot commented Apr 9, 2025

Copy link
Contributor

@cd-bitwarden cd-bitwarden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, approved ✅

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