fix(backup)_: do not show the backup notifi backup during normal fetch #6550
Conversation
Jenkins BuildsClick to see older builds (24)
|
| @@ -127,6 +127,10 @@ func (m *Messenger) handleBackup(state *ReceivedMessageState, message *protobuf. | |||
| } | |||
|
|
|||
| func (m *Messenger) updateBackupFetchProgress(message *protobuf.Backup, response *wakusync.WakuBackedUpDataResponse, state *ReceivedMessageState) error { | |||
| if m.backedUpFetchingStatus == nil { | |||
| return nil | |||
| } | |||
There was a problem hiding this comment.
Im not sure this is needed. I don't think we had any crashes, but I just realized that on normal app runs (way after the actual restore), this code could still run and it's better to guard against any crashes.
| @@ -324,11 +324,6 @@ func (m *Messenger) RequestAllHistoricMessages(forceFetchingBackup, withRetries | |||
|
|
|||
| allResponses := &MessengerResponse{} | |||
| if forceFetchingBackup || !backupFetched { | |||
There was a problem hiding this comment.
I wonder if we should add the if m.processBackedupMessages here too. What happens here is that even though we are not restoring the account, we end up doing a full backup fetch anyway for no reason?
I did not touch it right now just in case it would cause a regression, but I don't see the point of doing a full (30 days) fetch of backups if we created an account from scratch. Let me know if I'm missing something. The person that added that code was Andrea 4 years ago 😅
Also, if processBackedupMessages is false, we don,t even handle the backups:
func (m *Messenger) HandleBackup(state *ReceivedMessageState, message *protobuf.Backup, statusMessage *v1protocol.StatusMessage) error {
if !m.processBackedupMessages {
return nil
}cc @status-im/status-go-guild
There was a problem hiding this comment.
I don't think it makes much sense to fetch backups for newly created accounts. Adding this condition seems reasonable to me and definitely better than removing backup fetching tracking.
There was a problem hiding this comment.
I think you're right, the messages will be skipped in HandleBackup. But to me the issue is actually HandleBackup, not here.
If I would call RequestAllHistoricMessages with forceFetchingBackup = true, I'd expect backup to be fetched and the messages to be processed.
I'd keep as is for now, we should carefully refactor the way backup is handled.
There was a problem hiding this comment.
So I think:
-
either the condition should be
forceFetchingBackup || (!backupFetched && m.processBackedupMessages
both here and inHandleBackup(which will bring some weird dependencies between those functions) -
or we remove
forceFetchingBackupoption completely
I think it's only called from desktop inasyncFetchWakuBackupMessagesTask. Which I think is bound to the explicit button in settings and is only used (if used) for testing.
I'd prefer (2), as it would remove the code, not add it 😄
There was a problem hiding this comment.
I think it's only called from desktop in asyncFetchWakuBackupMessagesTask. Which I think is bound to the explicit button in settings and is only used (if used) for testing.
It is indeed only used in Desktop, in the backupSyncingComponent in the ActivityCenterPopup of all places.
I'll dig some more to understand why we added this force fetch, but I'm very tempted to just remove it as you proposed
There was a problem hiding this comment.
Source of the code
Ok I found it. It's used to force the re-fecth of everything in the onboarding.
So when re-importing from the seed phrase, we fetch everything. In the old onboarding, we waited on the page until everything was fetched, now we do it in the background.
In both cases, the user can click Retry if something didn't get fetched.
It at least confirms that it's only used in the case were were are re-importing an old account, ie processBackedupMessages is gonna be set to true.
Remaining problem
However, I just realized that removing the forceFetchingBackup param won't help us in this case, because RequestAllHistoricMessages gets called during the normal flow of the app too (like going back from sleep).
So at somepoint, we'll end up doing the full history fetch anyway.
Possible solutions
I guess we have two options:
a. we could set BackupFetched to true for accounts created from scratch
b. just add the processBackedupMessages like in @igor-sirotin 's option 1.
I think option b is the simpler one, but it does feel weird that BackupFetched will stay false for the whole life of that account.
We could also rename it to fetchAndProcessBackedupMessages so that it's clearer that it's not just about not parsing them, because it's weird to think that we might want to fetch backups but never process them (useless request and computation)
There was a problem hiding this comment.
Hmm, setting BackupFetched to true for new accounts sounds good to me for now.
@osmaczko wdyt?
There was a problem hiding this comment.
Hmm, setting
BackupFetchedtotruefor new accounts sounds good to me for now. @osmaczko wdyt?
Not ideal, because it is untrue, the backup will never be fetched for new accounts.. but it is good enough for now indeed.
There was a problem hiding this comment.
I updated the code to do as you guys proposed. I put it in the same commit by accident, but it's still pretty small so it shouldn't be an issue
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #6550 +/- ##
===========================================
- Coverage 60.52% 60.46% -0.07%
===========================================
Files 833 833
Lines 104367 104370 +3
===========================================
- Hits 63171 63110 -61
- Misses 33650 33709 +59
- Partials 7546 7551 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
jrainville
force-pushed
the
fix/wrong-backup-fetching-notification
branch
2 times, most recently
from
5427ecc to
a0e93c8
Compare
|
@igor-sirotin @osmaczko @seanstrom friendly reminder to review |
| @@ -324,11 +324,6 @@ func (m *Messenger) RequestAllHistoricMessages(forceFetchingBackup, withRetries | |||
|
|
|||
| allResponses := &MessengerResponse{} | |||
| if forceFetchingBackup || !backupFetched { | |||
There was a problem hiding this comment.
I don't think it makes much sense to fetch backups for newly created accounts. Adding this condition seems reasonable to me and definitely better than removing backup fetching tracking.
| @@ -324,11 +324,6 @@ func (m *Messenger) RequestAllHistoricMessages(forceFetchingBackup, withRetries | |||
|
|
|||
| allResponses := &MessengerResponse{} | |||
| if forceFetchingBackup || !backupFetched { | |||
There was a problem hiding this comment.
I think you're right, the messages will be skipped in HandleBackup. But to me the issue is actually HandleBackup, not here.
If I would call RequestAllHistoricMessages with forceFetchingBackup = true, I'd expect backup to be fetched and the messages to be processed.
I'd keep as is for now, we should carefully refactor the way backup is handled.
jrainville
force-pushed
the
fix/wrong-backup-fetching-notification
branch
from
a0e93c8 to
9cb6526
Compare
Fixes status-im/status-desktop#17555 We were adding the Notification of the back ups by accident when creating a normal account (not restored), because I added the code to create the notification in the `RequestAllHistoricMessages` function as well, even though the Messenger already does it. So for normal restores, it probably tried to add it twice, though they have the same ID, so it wasn't noticed
jrainville
force-pushed
the
fix/wrong-backup-fetching-notification
branch
from
9cb6526 to
44782a2
Compare
Fixes status-im/status-desktop#17555
We were adding the Notification of the back ups by accident when creating a normal account (not restored), because I added the code to create the notification in the
RequestAllHistoricMessagesfunction as well, even though the Messenger already does it. So for normal restores, it probably tried to add it twice, though they have the same ID, so it wasn't noticed