fix: NewClient dialog flow (WPD-284) #1785
Conversation
|
|
github-actions
bot
added
🚨 Potential breaking changes
👕 size: XL
type: bug / fix 🐞
labels
|
storing those events in the global DB add a layer of complication that is unnecessary, for example on login we need to remember to clear those events. we can keep those events in the user self DB and if you want to globally observe the list you can observe active accounts -> get events for each account separately |
Codecov Report
@@ Coverage Diff @@
## develop #1785 +/- ##
=============================================
+ Coverage 53.46% 53.50% +0.04%
- Complexity 1512 1525 +13
=============================================
Files 960 965 +5
Lines 35713 35844 +131
Branches 3163 3171 +8
=============================================
+ Hits 19094 19179 +85
- Misses 15360 15399 +39
- Partials 1259 1266 +7 |
There was a problem hiding this comment.
My main concern is that by making Client serializable and storing it as JSON we have to be super careful.
Our data classes in Logic weren't made to be persisted. We have Entities and all the tooling to detect breaking changes there.
Any change in the data class will break the stored JSON in a way won't notice, unlike with DB schema changes.
So, if it is possible to use the actual entities, that would be preferable.
There was a problem hiding this comment.
Isn't it simpler to add a isNew flag to the client column instead of storing a JSON client list in the Metadata table?
If we add a new field to the Client model, it can break the JSON blob that this PR is storing in the Metadata table.
Our migration tests will not catch this change, and we'd need to write complicated migrations using JSON functions in SQLite or whatever other solution.
There was a problem hiding this comment.
Adding a flag to the client table will have the issue if you register a client and delete, while other clients will miss the notification that a new client was added.
100% agree with moving the storing logic to persistence
How about adding a table to it? So, no JSON strings, and we can keep the data even if the client is removed.
There was a problem hiding this comment.
maybe i'll just add removing that new client from the DB, when we receive Event.User.ClientRemove ???
There was a problem hiding this comment.
did it, you can check it :)
|
Will this also be needed on |
| deviceType = deviceType?.let { fromDeviceTypeEntity(deviceType) }, | ||
| label = null, | ||
| model = model, | ||
| isVerified = true, |
There was a problem hiding this comment.
I wouldn't expect new clients to be verified by default, but I guess doesn't really matter for what you are using this for.
There was a problem hiding this comment.
true, it's not used anywhere in this flow, but I'll change it to false just in case :)
good question 🤔 |
What's new in this PR?
Issues
A "this account was used on ..." dialog is shown in the media share extension
Aslo issue that I found while working on it:
If user didn't open the app for some time and during this time more than 1 NewClient came -> User will see a dialog only about 1 NewClient (the last one).
Causes (Optional)
NewClients event is not saved anywhere, there was only Singleton
NewClientManagerwhich we were using for emitting such an event into observers (ObserveNewClientsUseCase) and it emits new clients one by oneSolutions
Add new DB-table
NewClientand save NewClients there. Now we can observe and not observe NewClients depending on screen, without losing any NewClient.ObserveNewClientsUseCaseemits a list of newClients.Important:
Breaking changes! Merge only when android PR ready for merge