docs: Design section for the managing Firestore data. #1150
Conversation
bootstraponline
force-pushed
the
design_doc_firestore_data
branch
from
a13d15b to
0821900
Compare
bootstraponline
force-pushed
the
design_doc_firestore_data
branch
2 times, most recently
from
e471ba2 to
e0515d9
Compare
bootstraponline
force-pushed
the
design_doc_firestore_data
branch
from
e0515d9 to
9bbe10d
Compare
There was a problem hiding this comment.
Good job, @solid-dimakoniaiev & @solid-yuriiuvarov! I've added a couple of initial suggestions here - please take a look.
| and [allowed domains](https://github.com/platform-platform/monorepo/blob/master/docs/19_security_audit_document.md#the-allowed_email_domains-collection) collections data. | ||
| To implement these methods, the repository using the [dartbase_admin package](https://pub.dev/packages/dartbase_admin) as described in the [decision](#decision). | ||
|
|
||
| ### FirebaseService |
There was a problem hiding this comment.
I believe it would be great to create a separate Service for Firestore to make it more SRP. The FirebaseService will be responsible for configuring the Firebase project, and the FirestoreService will be responsible for managing the Firestore Database data. What do you think?
| ### GcloudService | ||
|
|
||
| The `GcloudService` is an existing service interface, in which we should add the following methods required for this integration: | ||
| - `downloadServiceKey` - downloads a service account's key; |
There was a problem hiding this comment.
| - `downloadServiceKey` - downloads a service account's key; | |
| - `createServiceKey` - downloads a service account's key; |
Does it make sense since it seems like this method will create the key firstly and then download it?
| ### FirebaseService | ||
|
|
||
| The `FirebaseService` is an existing service interface, in which we should add the following methods required for this integration: | ||
| - `seedFirestoreData` - seeds Firestore database with required data; |
There was a problem hiding this comment.
Would it be better to name this method something like setInitialData or addIntialData?
|
|
||
| ### GcloudService | ||
|
|
||
| The `GcloudService` is an existing service interface, in which we should add the following methods required for this integration: |
There was a problem hiding this comment.
It would be great to add more context here about why do we need these methods. The same for other sections, I believe it would be great to have a bit more context.
| class FirestoreConfigRepository { | ||
| - _firestore : Firestore | ||
| + addAllowedDomain(String domain) : Future<void> | ||
| + setFeatureConfig(bool isPasswordSignInOptionEnabled, bool isDebugMenuEnabled): Future<void> |
There was a problem hiding this comment.
Should this class have a method used to init the FirebaseAdmin SDK?
|
|
||
| The `FirebaseService` is an existing service interface, in which we should add the following methods required for this integration: | ||
| - `seedFirestoreData` - seeds Firestore database with required data; | ||
| - `initAdminFirestore` - initializes Firestore database with Admin privileges. |
There was a problem hiding this comment.
I think the FirebaseService shouldn't know anything about implementation details, so it should not know anything about Firestore Admin i believe.
| } | ||
| } | ||
|
|
||
| Deployer --> FirebaseServiceAdapter : uses |
There was a problem hiding this comment.
| Deployer --> FirebaseServiceAdapter : uses | |
| Deployer --> FirebaseService : uses |
?
| ### FirebaseServiceAdapter | ||
|
|
||
| The `FirebaseServiceAdapter` is an adapter for the [`FirebaseService`](#FirebaseService), which implements new added methods using the [`FirestoreConfigRepository`](#FirestoreConfigRepository) and the [dartbase_admin package](https://pub.dev/packages/dartbase_admin). | ||
|
|
There was a problem hiding this comment.
Should we mention somewhere here that we should move the FeatureConfig and FeatureConfigData to the core package?
|
|
||
| The `GcloudService` is an existing service interface, in which we should add the following methods required for this integration: | ||
| - `downloadServiceKey` - downloads a service account's key; | ||
| - `deactivateServiceKey` - deactivates a service account's key. |
There was a problem hiding this comment.
Should we add any methods to the GcloudCli class to implement these methods in the GcloudCliServiceAdapter?
Fixes #1131
Checklist