feat(onboarding)_: add a notification when importing old account #6255
Conversation
Jenkins BuildsClick to see older builds (48)
|
jrainville
force-pushed
the
feat/new-onboarding-fetching-notification
branch
from
cc655fc to
4ed5c9a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6255 +/- ##
===========================================
+ Coverage 61.84% 61.86% +0.02%
===========================================
Files 843 843
Lines 111287 111366 +79
===========================================
+ Hits 68823 68900 +77
+ Misses 34497 34486 -11
- Partials 7967 7980 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
|
jrainville
force-pushed
the
feat/new-onbaording
branch
from
e446e61 to
cf3c5ca
Compare
jrainville
force-pushed
the
feat/new-onboarding-fetching-notification
branch
3 times, most recently
from
746cb48 to
68eb5b5
Compare
There was a problem hiding this comment.
Looks ok to me, but if @saledjenic would be able to take a look as well, it would be great 🤞
protocol/messenger.go
Outdated
| notifications, err := m.persistence.GetActivityCenterNotificationsByID(hexBytesIds) | ||
| if err != nil { | ||
| m.logger.Error("failed to get activity center notification", zap.Error(err)) | ||
| } else if len(notifications) == 1 { |
There was a problem hiding this comment.
Not sure I get this condition. Can there be more than one notification with the same ID?
There was a problem hiding this comment.
Actually I used the wrong function. I should have used GetActivityCenterNotificationByID instead.
I'll fix
protocol/messenger.go
Outdated
|
|
||
| // Add a timeout to mark the backup syncing as failed after 1 minute and 30 seconds | ||
| time.AfterFunc(1*time.Minute+30*time.Second, func() { | ||
| if m.fetchingBackedUpDataCompleted { |
There was a problem hiding this comment.
Doesn't it need a mutex or the risk of race condition is neglectable?
protocol/messenger.go
Outdated
| return response, nil | ||
| } | ||
|
|
||
| func (m *Messenger) startBackupFetchingTracking(response *MessengerResponse) error { |
There was a problem hiding this comment.
Would it make sense to move it to messenger_backup_handler.go ?
| select { | ||
| case m.wakuBackedUpDataResponseChan <- response: | ||
| default: | ||
| } |
There was a problem hiding this comment.
Can you please elaborate why changed to non-blocking?
There was a problem hiding this comment.
It blocked my test. To be honest, I don't fully know how this code works, I just discovered that if I change this code to this, it makes my test work and the other test that uses it still works, so it sounded fine.
There was a problem hiding this comment.
Actually, all channel reads/writes operations must be wrapped into a select/case, with some cancellation options: Code smell #1: Accessing channels.
| s.Require().Len(bob1.fetchingBackedUpDataProgress, 3) | ||
| s.Require().Equal(uint32(1), bob1.fetchingBackedUpDataProgress[SyncWakuSectionKeyContacts].TotalNumber) | ||
|
|
||
| // Parse a backup with a higher clock so reset the fetching |
There was a problem hiding this comment.
I am wondering what will happen if the previous backup has completed before the next one kicks in?
There was a problem hiding this comment.
Good question. I think in theory, it would mark the whole fetching as completed, so a false positive.
The old Nim logic had a minimum of 30 seconds before letting the user go to the next page, even if it said it received all data.
Maybe I should add that sort of timer too? Or maybe it doesn't matter, because the data will still get processed when the new backup comes in. Though the user would have a false sense of it being completed, so they could close the app too early.
| @@ -9,6 +9,11 @@ type FetchingBackupedDataDetails struct { | |||
| TotalNumber uint32 `json:"totalNumber,omitempty"` | |||
| } | |||
|
|
|||
| type FetchingBackedUpDataTracking struct { | |||
There was a problem hiding this comment.
It is not used in any kind of response. Not sure if it is a proper place for it (could be messenger's implementation detail).
There was a problem hiding this comment.
Not sure. I put it with the other Backup type. I'm open to move it though
There was a problem hiding this comment.
I agree, perhaps just put it in the messenger_backup.go or smth. Probably together with the BackupFetchStatus that I proposed
jrainville
force-pushed
the
feat/new-onboarding-fetching-notification
branch
from
68eb5b5 to
43276bf
Compare
protocol/messenger.go
Outdated
|
|
||
| fetchingBackedUpDataProgress map[string]wakusync.FetchingBackedUpDataTracking | ||
| lastKnownBackedUpMsgClock uint64 | ||
| fetchingBackedUpDataCompleted bool | ||
| fetchingBackedUpDataCompletedMutex sync.Mutex |
There was a problem hiding this comment.
Not a big deal, but Messenger is already so huge, maybe we could compose these properties into a single BackupFetchStatus struct? It will also enable to remove the fetchingBackedUpData prefix in each field.
protocol/messenger_backup_handler.go
Outdated
| m.fetchingBackedUpDataCompletedMutex.Lock() | ||
| defer m.fetchingBackedUpDataCompletedMutex.Unlock() | ||
|
|
||
| if !m.fetchingBackedUpDataCompleted { | ||
| evaluate := true | ||
| if m.lastKnownBackedUpMsgClock > message.Clock { | ||
| evaluate = false | ||
| } else if m.lastKnownBackedUpMsgClock < message.Clock { | ||
| // Reset the progress tracker because we have access to a more recent copy of the backup | ||
| m.lastKnownBackedUpMsgClock = message.Clock | ||
| m.fetchingBackedUpDataProgress = make(map[string]wakusync.FetchingBackedUpDataTracking) | ||
| for backupName, details := range response.FetchingBackedUpDataDetails() { | ||
| m.fetchingBackedUpDataProgress[backupName] = wakusync.FetchingBackedUpDataTracking{ | ||
| LoadedItems: make(map[uint32]bool), | ||
| TotalNumber: details.TotalNumber, | ||
| } | ||
| } | ||
| if len(m.fetchingBackedUpDataProgress) == 0 { | ||
| evaluate = false | ||
| } | ||
| } | ||
|
|
||
| // Evaluate the progress of the backup | ||
| if evaluate { | ||
| // Set the new items before evaluating | ||
| for backupName, details := range response.FetchingBackedUpDataDetails() { | ||
| m.fetchingBackedUpDataProgress[backupName].LoadedItems[details.DataNumber] = true | ||
| } | ||
|
|
||
| receivedEverything := true | ||
| for _, tracker := range m.fetchingBackedUpDataProgress { | ||
| if len(tracker.LoadedItems)-1 < int(tracker.TotalNumber) { | ||
| receivedEverything = false | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if receivedEverything { | ||
| m.fetchingBackedUpDataCompleted = true | ||
|
|
||
| // Update the AC notification and add it to the response | ||
| notification, err := m.persistence.GetActivityCenterNotificationByID(types.FromHex(backupSyncingNotificationID)) | ||
| if err != nil { | ||
| errors = append(errors, err) | ||
| } else if notification != nil { | ||
| notification.UpdatedAt = m.GetCurrentTimeInMillis() | ||
| notification.Type = ActivityCenterNotificationTypeBackupSyncingSuccess | ||
| _, err = m.persistence.SaveActivityCenterNotification(notification, true) | ||
| if err != nil { | ||
| errors = append(errors, err) | ||
| } else { | ||
| state.Response.AddActivityCenterNotification(notification) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think it would be best to extract this to a function.
Then you can use early return pattern, avoid special flags like evaluate and receivedEverything and it will make the code flat without 7 layers of enclosure.
func (m *Messenger) updateBackupFetchProgress(message *protobuf.Backup, response *wakusync.WakuBackedUpDataResponse, state *ReceivedMessageState) error {
m.fetchingBackedUpDataCompletedMutex.Lock()
defer m.fetchingBackedUpDataCompletedMutex.Unlock()
if m.fetchingBackedUpDataCompleted {
return nil
}
if m.lastKnownBackedUpMsgClock > message.Clock {
return nil
}
if m.lastKnownBackedUpMsgClock < message.Clock {
// Reset the progress tracker because we have access to a more recent copy of the backup
m.lastKnownBackedUpMsgClock = message.Clock
m.fetchingBackedUpDataProgress = make(map[string]wakusync.FetchingBackedUpDataTracking)
for backupName, details := range response.FetchingBackedUpDataDetails() {
m.fetchingBackedUpDataProgress[backupName] = wakusync.FetchingBackedUpDataTracking{
LoadedItems: make(map[uint32]bool),
TotalNumber: details.TotalNumber,
}
}
if len(m.fetchingBackedUpDataProgress) == 0 {
return nil
}
}
// Evaluate the progress of the backup
// Set the new items before evaluating
for backupName, details := range response.FetchingBackedUpDataDetails() {
m.fetchingBackedUpDataProgress[backupName].LoadedItems[details.DataNumber] = true
}
for _, tracker := range m.fetchingBackedUpDataProgress {
if len(tracker.LoadedItems)-1 < int(tracker.TotalNumber) {
// have not received everything yet
return nil
}
}
m.fetchingBackedUpDataCompleted = true
// Update the AC notification and add it to the response
notification, err := m.persistence.GetActivityCenterNotificationByID(types.FromHex(backupSyncingNotificationID))
if err != nil {
return err
}
if notification == nil {
return nil
}
notification.UpdatedAt = m.GetCurrentTimeInMillis()
notification.Type = ActivityCenterNotificationTypeBackupSyncingSuccess
_, err = m.persistence.SaveActivityCenterNotification(notification, true)
if err != nil {
return err
}
state.Response.AddActivityCenterNotification(notification)
return nil
}Then call it like this here:
| m.fetchingBackedUpDataCompletedMutex.Lock() | |
| defer m.fetchingBackedUpDataCompletedMutex.Unlock() | |
| if !m.fetchingBackedUpDataCompleted { | |
| evaluate := true | |
| if m.lastKnownBackedUpMsgClock > message.Clock { | |
| evaluate = false | |
| } else if m.lastKnownBackedUpMsgClock < message.Clock { | |
| // Reset the progress tracker because we have access to a more recent copy of the backup | |
| m.lastKnownBackedUpMsgClock = message.Clock | |
| m.fetchingBackedUpDataProgress = make(map[string]wakusync.FetchingBackedUpDataTracking) | |
| for backupName, details := range response.FetchingBackedUpDataDetails() { | |
| m.fetchingBackedUpDataProgress[backupName] = wakusync.FetchingBackedUpDataTracking{ | |
| LoadedItems: make(map[uint32]bool), | |
| TotalNumber: details.TotalNumber, | |
| } | |
| } | |
| if len(m.fetchingBackedUpDataProgress) == 0 { | |
| evaluate = false | |
| } | |
| } | |
| // Evaluate the progress of the backup | |
| if evaluate { | |
| // Set the new items before evaluating | |
| for backupName, details := range response.FetchingBackedUpDataDetails() { | |
| m.fetchingBackedUpDataProgress[backupName].LoadedItems[details.DataNumber] = true | |
| } | |
| receivedEverything := true | |
| for _, tracker := range m.fetchingBackedUpDataProgress { | |
| if len(tracker.LoadedItems)-1 < int(tracker.TotalNumber) { | |
| receivedEverything = false | |
| break | |
| } | |
| } | |
| if receivedEverything { | |
| m.fetchingBackedUpDataCompleted = true | |
| // Update the AC notification and add it to the response | |
| notification, err := m.persistence.GetActivityCenterNotificationByID(types.FromHex(backupSyncingNotificationID)) | |
| if err != nil { | |
| errors = append(errors, err) | |
| } else if notification != nil { | |
| notification.UpdatedAt = m.GetCurrentTimeInMillis() | |
| notification.Type = ActivityCenterNotificationTypeBackupSyncingSuccess | |
| _, err = m.persistence.SaveActivityCenterNotification(notification, true) | |
| if err != nil { | |
| errors = append(errors, err) | |
| } else { | |
| state.Response.AddActivityCenterNotification(notification) | |
| } | |
| } | |
| } | |
| } | |
| } | |
| err = m.updateBackupFetchProgress(message, &response, state) | |
| if err != nil { | |
| errors = append(errors, err) | |
| } |
There was a problem hiding this comment.
Nice way cleaner! I do prefer early returns, but not sure why I didn't think of just extracting it.
| } | ||
|
|
||
| // Add a timeout to mark the backup syncing as failed after 1 minute and 30 seconds | ||
| time.AfterFunc(1*time.Minute+30*time.Second, func() { |
There was a problem hiding this comment.
In theory, there should be no timeout.
Backup fetching is done with a store node request(s). We know if any data was found, and we know if there was a timeout fetching, which id defined on the store node request level.
Do you think we can take that out here?
There was a problem hiding this comment.
That sounds like a good idea. I'm not sure how to hook to that though. I don't think we specifically call "fetchAllBackups". We seem to only set processBackedupMessages which tells the Messenger to handle those backup messages.
I'm not sure where we do the call to fetch all those backup messages. In theory, we should call that only on restore, but I don't see it using the fetchBackup flag, so maybe we have a small issue?
There was a problem hiding this comment.
Yeah now when I think of this, there's probably no simple way to do it in current architecture...
| select { | ||
| case m.wakuBackedUpDataResponseChan <- response: | ||
| default: | ||
| } |
There was a problem hiding this comment.
Actually, all channel reads/writes operations must be wrapped into a select/case, with some cancellation options: Code smell #1: Accessing channels.
| @@ -9,6 +9,11 @@ type FetchingBackupedDataDetails struct { | |||
| TotalNumber uint32 `json:"totalNumber,omitempty"` | |||
| } | |||
|
|
|||
| type FetchingBackedUpDataTracking struct { | |||
There was a problem hiding this comment.
I agree, perhaps just put it in the messenger_backup.go or smth. Probably together with the BackupFetchStatus that I proposed
Needed for status-im/status-desktop#17028 When a user imports an old account, we start fetching the backups. Now, we show an activity center notification to let the user know, because the onboarding doesn't block during the fetching anymore, so it happens in the background. After a timeout, the notification turns into a failure state (full or partial). The user can then restart the fetching. I added a test to validate the logic of calculating if the fetching was a success. That logic used to be in Nim.
jrainville
force-pushed
the
feat/new-onboarding-fetching-notification
branch
from
43276bf to
e4aad76
Compare
There was a problem hiding this comment.
@igor-sirotin thanks for the review. I addressed your comments.
I'm not sure how to hook on the mailserver one though
protocol/messenger_backup_handler.go
Outdated
| m.fetchingBackedUpDataCompletedMutex.Lock() | ||
| defer m.fetchingBackedUpDataCompletedMutex.Unlock() | ||
|
|
||
| if !m.fetchingBackedUpDataCompleted { | ||
| evaluate := true | ||
| if m.lastKnownBackedUpMsgClock > message.Clock { | ||
| evaluate = false | ||
| } else if m.lastKnownBackedUpMsgClock < message.Clock { | ||
| // Reset the progress tracker because we have access to a more recent copy of the backup | ||
| m.lastKnownBackedUpMsgClock = message.Clock | ||
| m.fetchingBackedUpDataProgress = make(map[string]wakusync.FetchingBackedUpDataTracking) | ||
| for backupName, details := range response.FetchingBackedUpDataDetails() { | ||
| m.fetchingBackedUpDataProgress[backupName] = wakusync.FetchingBackedUpDataTracking{ | ||
| LoadedItems: make(map[uint32]bool), | ||
| TotalNumber: details.TotalNumber, | ||
| } | ||
| } | ||
| if len(m.fetchingBackedUpDataProgress) == 0 { | ||
| evaluate = false | ||
| } | ||
| } | ||
|
|
||
| // Evaluate the progress of the backup | ||
| if evaluate { | ||
| // Set the new items before evaluating | ||
| for backupName, details := range response.FetchingBackedUpDataDetails() { | ||
| m.fetchingBackedUpDataProgress[backupName].LoadedItems[details.DataNumber] = true | ||
| } | ||
|
|
||
| receivedEverything := true | ||
| for _, tracker := range m.fetchingBackedUpDataProgress { | ||
| if len(tracker.LoadedItems)-1 < int(tracker.TotalNumber) { | ||
| receivedEverything = false | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if receivedEverything { | ||
| m.fetchingBackedUpDataCompleted = true | ||
|
|
||
| // Update the AC notification and add it to the response | ||
| notification, err := m.persistence.GetActivityCenterNotificationByID(types.FromHex(backupSyncingNotificationID)) | ||
| if err != nil { | ||
| errors = append(errors, err) | ||
| } else if notification != nil { | ||
| notification.UpdatedAt = m.GetCurrentTimeInMillis() | ||
| notification.Type = ActivityCenterNotificationTypeBackupSyncingSuccess | ||
| _, err = m.persistence.SaveActivityCenterNotification(notification, true) | ||
| if err != nil { | ||
| errors = append(errors, err) | ||
| } else { | ||
| state.Response.AddActivityCenterNotification(notification) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Nice way cleaner! I do prefer early returns, but not sure why I didn't think of just extracting it.
| } | ||
|
|
||
| // Add a timeout to mark the backup syncing as failed after 1 minute and 30 seconds | ||
| time.AfterFunc(1*time.Minute+30*time.Second, func() { |
There was a problem hiding this comment.
That sounds like a good idea. I'm not sure how to hook to that though. I don't think we specifically call "fetchAllBackups". We seem to only set processBackedupMessages which tells the Messenger to handle those backup messages.
I'm not sure where we do the call to fetch all those backup messages. In theory, we should call that only on restore, but I don't see it using the fetchBackup flag, so maybe we have a small issue?
Needed for status-im/status-desktop#17028
When a user imports an old account, we start fetching the backups. Now, we show an activity center notification to let the user know, because the onboarding doesn't block during the fetching anymore, so it happens in the background.
After a timeout, the notification turns into a failure state (full or partial). The user can then restart the fetching.
I added a test to validate the logic of calculating if the fetching was a success. That logic used to be in Nim.