bitwarden
Migrate existing organizations state from `StateService` → `StateProvider`
Type of change
- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other
Objective
This PR migrates OrganizationService over to StateProvider.
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
References
-
AC-2009: Transition OrganizationService to use StateProvider - https://github.com/bitwarden/clients/pull/7776
- https://github.com/bitwarden/clients/pull/7781
- https://github.com/bitwarden/clients/pull/7841
Codecov Report
Attention: Patch coverage is 50.40650% with 61 lines in your changes are missing coverage. Please review.
Project coverage is 26.20%. Comparing base (
ca86288) to head (d2f9644).
Additional details and impacted files
@@ Coverage Diff @@
## main #7895 +/- ##
=======================================
Coverage 26.20% 26.20%
=======================================
Files 2280 2280
Lines 66727 66735 +8
Branches 12549 12550 +1
=======================================
+ Hits 17483 17489 +6
- Misses 47881 47883 +2
Partials 1363 1363
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Checkmarx One – Scan Summary & Details – 25ceb7aa-26da-48db-8216-4e6673958871
No New Or Fixed Issues Found
@justindbaur that makes sense. It is important to me to make incremental steps here for several reasons. Is there any good way for me to "queue" up a migration so that the migration can be written and tested without being implemented yet? I had thought of just removing it from the migration list & renaming the file without a number. Then applying the migration would just involve numbering the migration and adding it to the list. The still seems a little complicated though, so I'm wondering if you've got any better ideas?
@addisonbeck There is no way to queue up a migration, moving the backing location of your data is kind of an all or nothing move. If you're trying to keep things small you can delete some of the type assertions in here: https://github.com/bitwarden/clients/pull/7895/files#diff-397e257f75aaf8565e7025b97bbf6d6c0830fb174aceaa38522097a74422f2dcR54-R103
If the properties aren't referenced in the migration code they don't really contribute, what's is most important are the types for the account object shape leading up to your data. Then your data can be represented with an unknown.
Then, if you use the same observable names that the state service ones use you can switch over their backing data right now. Then you can have another 1 or 2 PR's that rename those observables to the new name if you want and one for deleting the state service methods and account property.
With those changes I think this PR would actually be slightly smaller.
Renaming the exposed methods & observables separately is a good idea. I'll go that route, thanks!
@justindbaur @eliykat This ended up getting bigger since many of OrganizationService's methods were made async. Would y'all mind giving it a review before I open it up to teams to review the changes to OrganizationService callers?
Apologies for the miscommunication about making interface changes. I was under the impression you asked for them. I'm not even remotely upset about reverting, as I was not enjoying how large the PR had gotten, even though I did think the changes were for the best.
That said, I didn't completely revert things. Here's how the change set looks now:
- ~~There are 0 interface changes that impact callers. Every method signature that existed before still exists now and has not been altered. This means we don't have to bug other teams and can know where the bugs are coming from if they show up. Yay ✨~~ This is no longer true
- I kept all of the new interface methods and mappers. I marked old methods as deprecated, and pointed them to the new ones in their function bodies. This isn't always the most elegant solution (there are several really bad uses of observables in order to facilitate the not-promise based methods that were checking the current value of
_organizationswithout async/await), but it does mean we will have an easy time deleting them later. - I kept all the mapper functions, and still wrote everything as if one observable and a collection of operator functions is the direction we'd go in later when we update callers. I know you had some concerns about this, and figured we'd still talk it through in more detail later, but I do have some arguments in favor of this pattern so I didn't want to scrap what was already written.
- Once this has had some time to prove itself in production I'd like to handle updating callers to use observable patterns as tech debt on a team-by-team basis, or even more gradually if desired.
A tech debt task has been created for paying the tech debt associated with this PR that led to so much discussion: The service's continued use of promises for get operations.