Skip to content

[PM-16921] Improve validation tax information when subscribing to pre…#5247

Closed
jonashendrickx wants to merge 8 commits intomainfrom
PM-16921
Closed

[PM-16921] Improve validation tax information when subscribing to pre…#5247
jonashendrickx wants to merge 8 commits intomainfrom
PM-16921

Conversation

@jonashendrickx
Copy link
Copy Markdown
Contributor

@jonashendrickx jonashendrickx commented Jan 10, 2025

…mium

🎟️ Tracking

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

📔 Objective

📸 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

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 6.66667% with 14 lines in your changes missing coverage. Please review.

Project coverage is 46.88%. Comparing base (c9a42d8) to head (f3b3b6c).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/Api/Billing/Controllers/AccountsController.cs 0.00% 10 Missing ⚠️
...Api/Models/Request/Accounts/PremiumRequestModel.cs 20.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5247      +/-   ##
==========================================
+ Coverage   46.69%   46.88%   +0.18%     
==========================================
  Files        1609     1609              
  Lines       73195    72905     -290     
  Branches     6560     6522      -38     
==========================================
  Hits        34181    34181              
+ Misses      37578    37288     -290     
  Partials     1436     1436              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 10, 2025

Logo
Checkmarx One – Scan Summary & Details9712d5ea-e570-4a28-ad97-dd782136959b

New Issues (3)

Checkmarx found the following issues in this Pull Request

Severity Issue Source File / Package Checkmarx Insight
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationDomainController.cs: 94
detailsMethod Verify at line 94 of /src/Api/AdminConsole/Controllers/OrganizationDomainController.cs gets a parameter from a user request from orgId. This...
ID: T3s4Spy2%2BYNmmyxwb16mjxjKe6k%3D
Attack Vector
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationDomainController.cs: 94
detailsMethod Verify at line 94 of /src/Api/AdminConsole/Controllers/OrganizationDomainController.cs gets a parameter from a user request from id. This pa...
ID: zYvEEYpPId7m9lgEpRbyJ%2BvSLNk%3D
Attack Vector
LOW Missing_CSP_Header /src/Core/MailTemplates/Handlebars/Billing/BusinessUnitConversionInvite.html.hbs: 12
detailsA Content Security Policy is not explicitly defined within the web-application.
ID: IIshHvFH05usl0d7kdglsEkC7i8%3D
Attack Vector
Fixed Issues (6)

Great job! The following issues were fixed in this Pull Request

Severity Issue Source File / Package
MEDIUM CSRF /src/Api/Controllers/DevicesController.cs: 75
MEDIUM CSRF /src/Api/AdminConsole/Controllers/GroupsController.cs: 164
MEDIUM CSRF /src/Api/Controllers/DevicesController.cs: 62
MEDIUM CSRF /bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 86
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 133
MEDIUM CSRF /bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 96

@rr-bw rr-bw requested review from ike-kottlowski and removed request for rr-bw January 10, 2025 18:28
ike-kottlowski
ike-kottlowski previously approved these changes Jan 10, 2025
@jonashendrickx jonashendrickx changed the title [BEEEP][PM-26932] Improve validation tax information when subscribing to pre… [BEEEP][PM-16932] Improve validation tax information when subscribing to pre… Jan 20, 2025
@jonashendrickx jonashendrickx changed the title [BEEEP][PM-16932] Improve validation tax information when subscribing to pre… [BEEEP][PM-16921] Improve validation tax information when subscribing to pre… Jan 20, 2025
Copy link
Copy Markdown
Contributor

@amorask-bitwarden amorask-bitwarden left a comment

Choose a reason for hiding this comment

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

I don't think there's any path left that actually invokes StripePaymentService.UpdatePaymentMethod. I've done a bad job of starting to remove this old stuff after it's been left unused, but I'll try to get around to it during BEEEP time - that's on me.

You're going to want to look in:

  • OrganizationBillingService
  • PremiumUserBillingService

@jonashendrickx jonashendrickx changed the title [BEEEP][PM-16921] Improve validation tax information when subscribing to pre… [PM-16921] Improve validation tax information when subscribing to pre… Mar 26, 2025
Comment thread src/Core/Services/Implementations/StripePaymentService.cs
Copy link
Copy Markdown
Contributor

@amorask-bitwarden amorask-bitwarden left a comment

Choose a reason for hiding this comment

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

Accidentally approved, please see comment.

@jonashendrickx jonashendrickx requested a review from a team as a code owner March 28, 2025 05:54
@jonashendrickx jonashendrickx requested a review from r-tome March 28, 2025 05:54
Jonas Hendrickx added 2 commits April 9, 2025 10:57
# Conflicts:
#	src/Api/Auth/Controllers/AccountsController.cs
#	src/Core/Services/Implementations/StripePaymentService.cs
@jonashendrickx jonashendrickx requested a review from a team April 9, 2025 09:27
@sonarqubecloud
Copy link
Copy Markdown

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.

6 participants