Identity reset backup fixes #3790
Conversation
stefanceriu
requested review from
jmartinesp and
poljar
and removed request for
a team and
jmartinesp
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3790 +/- ##
==========================================
+ Coverage 84.06% 84.07% +0.01%
==========================================
Files 261 261
Lines 27352 27365 +13
==========================================
+ Hits 22993 23008 +15
+ Misses 4359 4357 -2 ☔ View full report in Codecov by Sentry. |
| @@ -423,6 +423,39 @@ impl Backups { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| /// Deletes the current backup version from the server | |||
There was a problem hiding this comment.
I think that this need a bit more of a warning, feel free to add some disable() mehod from, and to, this one.
As usual, an doc example wouldn't hurt either.
|
|
||
| olm_machine.backup_machine().disable_backup().await?; | ||
|
|
||
| self.set_state(BackupState::Unknown); |
There was a problem hiding this comment.
Both the OK and error case go into the Unknown state, so you can just have one set_state() in the main part of the function, just before you return the result.
| @@ -780,11 +780,15 @@ async fn test_reset_identity() { | |||
| assert_eq!(client.encryption().recovery().state(), RecoveryState::Enabled); | |||
|
|
|||
| // Disabling backups | |||
| Mock::given(method("DELETE")) | |||
| .and(path("_matrix/client/r0/room_keys/version/1")) | |||
| Mock::given(method("GET")) | |||
There was a problem hiding this comment.
You are not quite testing the happy path if you never need to mock the DELETE endpoint. You're returning a 404 on the GET version call, so the SDK concludes that a backup doesn't exist and doesn't need to be deleted.
A valid test, but if we had a test for the happy path, which does need to delete the backup as well.
stefanceriu
force-pushed
the
stefan/identityResetBackupFixes
branch
from
294c424 to
50973fe
Compare
…s if they weren't previously enabled locally - also change back the state from `disabling` to `unknown`
…ed locally, resulting in conflicts and failing to correctly setup the newly reset session
stefanceriu
force-pushed
the
stefan/identityResetBackupFixes
branch
from
50973fe to
1402382
Compare
This PR handles 2 different issues:
backups().disablemethod does not delete the remote backup if the backups haven't been setup yet locallyWe've introduced a new
delete_backupmethod what will delete remote backups and that can be used from the identity reset flows to correctly setup the fresh session.