[pm-14025] Remove usage of ActiveUserState from organization.service #11799
+449
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #11799 +/- ##
==========================================
+ Coverage 33.44% 33.48% +0.04%
==========================================
Files 2844 2849 +5
Lines 89043 89271 +228
Branches 16982 17012 +30
==========================================
+ Hits 29777 29891 +114
- Misses 56929 57026 +97
- Partials 2337 2354 +17 ☔ View full report in Codecov by Sentry. |
eliykat
requested changes
Nov 11, 2024
There was a problem hiding this comment.
Please fix the naming of these classes while you're here:
- abstract class has no prefix or suffix:
organization.service.ts - the default (common) implementation has a "default" prefix:
default-organization.service.ts
Or for this PR, vnext-organization.service.ts and default-vnext.organization.service.ts.
There was a problem hiding this comment.
I approve of removing deprecated / duplicate interfaces from this class!
There was a problem hiding this comment.
Just to be clear on this one:
- rename the class definitions in organization.service.ts to include the "default" prefix
- rename organization.service.ts to default-organization.service.ts
- rename new vnext-[filename] files to default-vnext[filename]
This would include updating the import statements across some 150 files. Just want to double check before making that wide a change. The other requested changes have been committed to this branch.
There was a problem hiding this comment.
I've commented on each file to clarify exact naming.
No need to rename the current organization service given that we're about to replace it anyway. But I thought that you're going to have to update all the calling locations when you wire up this new implementation (in the next PR), so it's a good opportunity to update the name while we're at it.
| */ | ||
| export abstract class vNextOrganizationService { | ||
| /** | ||
| * Publishes state for all organizations under the active user. |
There was a problem hiding this comment.
This needs to be updated - it's now for the specified UserId, not the active user.
EDIT: same for other comments in this file.
...common/src/admin-console/abstractions/organization/vnext-organization.service.abstraction.ts
Outdated
Show resolved
Hide resolved
...common/src/admin-console/abstractions/organization/vnext-organization.service.abstraction.ts
Outdated
Show resolved
Hide resolved
...common/src/admin-console/abstractions/organization/vnext-organization.service.abstraction.ts
Outdated
Show resolved
Hide resolved
libs/common/src/admin-console/services/organization/vnext-organization.service.ts
Outdated
Show resolved
Hide resolved
libs/common/src/admin-console/services/organization/vnext-organization.service.ts
Outdated
Show resolved
Hide resolved
| const fakeUserId = Utils.newGuid() as UserId; | ||
| let fakeAccountService: FakeAccountService; | ||
| let fakeStateProvider: FakeStateProvider; | ||
| let fakeActiveUserState: FakeActiveUserState<Record<string, OrganizationData>>; |
There was a problem hiding this comment.
ActiveUserState is being deprecated, that includes test fakes such this one. Also, the sut no longer has any concept of an "active user", so we shouldn't use it in our tests. You should be able to delete FakeActiveUserState and just rely on FakeStateProvider. See the collection service vnext tests as an example.
Where the tests refer to an active user vs. a non-active user, they should be rewritten to just use 2 separate fake user states (without any reference to "active"). They would now test that altering 1 user state does not alter the other user's state.
|
New Issues
Fixed Issues
|
eliykat
reviewed
Nov 12, 2024
...rc/admin-console/abstractions/organization/deafult-vnext.organization.service.abstraction.ts
Outdated
Show resolved
Hide resolved
libs/common/src/admin-console/services/organization/vnext-organization.service.ts
Outdated
Show resolved
Hide resolved
libs/common/src/admin-console/services/organization/vnext-organization.service.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I've commented on each file to clarify exact naming.
No need to rename the current organization service given that we're about to replace it anyway. But I thought that you're going to have to update all the calling locations when you wire up this new implementation (in the next PR), so it's a good opportunity to update the name while we're at it.
eliykat
requested changes
Nov 12, 2024
...rc/admin-console/abstractions/organization/deafult-vnext.organization.service.abstraction.ts
Outdated
Show resolved
Hide resolved
libs/common/src/admin-console/services/organization/vnext-organization.service.spec.ts
Outdated
Show resolved
Hide resolved
libs/common/src/admin-console/services/organization/vnext-organization.service.spec.ts
Outdated
Show resolved
Hide resolved
libs/common/src/admin-console/services/organization/vnext-organization.service.spec.ts
Outdated
Show resolved
Hide resolved
…rvice' of github.com:bitwarden/clients into ac/pm-14025-Remove-ActiveUserState-from-organization.service
eliykat
requested changes
Nov 15, 2024
There was a problem hiding this comment.
Just this and then we should be good to go.
The next stage of this is to update all callers to use the new interface and implementation. That will affect a lot of calling locations, so you may want to consider whether to update 1 client at a time (1 client per PR) rather than all at once.
libs/common/src/admin-console/abstractions/organization/vnext.organization.service.ts
Outdated
Show resolved
Hide resolved
eliykat
approved these changes
Nov 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-14025
📔 Objective
We are deprecating ActiveUserState. Remove it from OrganizationService so that its methods always require a UserId or an Observable.
This PR adds vNextOrganizationService. The stateful methods are fully tested but not wired up yet. The goal is to focus on the new service implementation in isolation, and then wire it up in a following PR.
📸 Screenshots
⏰ 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