[PM-15179] Implement add-existing-organization-dialog.component #13010
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13010 +/- ##
==========================================
- Coverage 35.50% 35.48% -0.03%
==========================================
Files 3008 3010 +2
Lines 90929 90996 +67
Branches 16914 16924 +10
==========================================
+ Hits 32286 32287 +1
- Misses 56141 56207 +66
Partials 2502 2502 ☔ View full report in Codecov by Sentry. |
| @@ -40,6 +45,22 @@ export class WebProviderService { | |||
| return response; | |||
| } | |||
|
|
|||
| async addExistingOrganization(providerId: string, organizationId: string): Promise<void> { | |||
There was a problem hiding this comment.
Will this replace the method immediately above it, addOrganizationToProvider? If so, can it use the vNext suffix or have a jsdoc comment to distinguish between them?
There was a problem hiding this comment.
Addressed in f131e9c
| await this.billingApiService.addExistingOrganizationToProvider(providerId, { | ||
| key: encryptedOrgKey.encryptedString, | ||
| organizationId, | ||
| }); |
There was a problem hiding this comment.
I don't think this should be a billing-related api service or endpoint. The main action being done is to associate the provider with the organization, and the change in billing is only a side-effect. Looking at the server PR, ProviderClientsController (owned by Billing Team) seems to be slowly eroding ProviderOrganizationsController (owned by AC Team).
I think we need to maintain the team split here, where AC Team owns the general Provider Portal, its entities and their relationships, and there's some interface out to Billing Team's code to handle billing concerns. (Similar to how we own the creation of orgs, but billing code is called as a side-effect.)
In concrete terms for these PRs, this would mean:
- in clients,
billingApiService.addExistingOrganizationToProvideris moved into theProviderApiService(which I'll move under our ownership) - in server,
ProviderClientsControlleris merged intoProviderOrganizationsController - new dialog also potentially comes under AC ownership
I know Billing Team end up doing a lot of work in Provider Portal because it has a lot of billing impacts, but I want to avoid code just drifting between teams over time due to resource allocation by Product. Instead we should be building good interfaces between teams, or revisiting team boundaries.
Open for discussion here, let me know what you think.
There was a problem hiding this comment.
@eliykat Seems fair to me. The clients changes are easy. For server, does your team have a desired approach to endpoint naming with regards to legacy vs. consolidated billing endpoint versions?
For example:
POST /providers/{providerId}/organizations is the endpoint for legacy client creation.
When I pull the ProviderClientsController into this controller, how does your team want that endpoint to be named? POST /providers/{providers}/organizations/vnext or something along those lines?
There was a problem hiding this comment.
Is the legacy endpoint, /providers/<id>/organizations, still needed for anything? Can it be deleted now, or will it be deleted eventually?
Given that the 2 controllers cleanly split old vs. new code, maybe keeping the ProviderClientsController but moving it under AC Team ownership is the simplest and easiest solution; rather than integrating old & new code and having to rip out old code later.
There was a problem hiding this comment.
It cannot be deleted yet. We do still have some providers on the old experience, but Priya mentioned to me those were going to be removed after some communication likely by the end of this quarter.
As such, in the server PR, I've moved the ProviderClientsController and associated tests under AC ownership. I'll manage the removal of all legacy Provider Portal code when we get the green-light to do so.
In this PR, I moved the new API calls to the provider-api.service and updated the web-provider.service method to use VNext.
|
@eliykat Can I get another look here when you have a second based on this comment? #13010 (comment) |
amorask-bitwarden
dismissed stale reviews from cturnbull-bitwarden and eliykat
via
3306127
amorask-bitwarden
deleted the
billing/PM-15179/add-existing-orgs-provider-portal
branch
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-15179
📔 Objective
This PR implements the new
add-existing-organization-dialog.componentto the Provider Portal.serverPR: bitwarden/server#5310📸 Screenshots
owner-add.mov
service-user-add.mov
⏰ 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