Skip to content

Aligning recovery code with connect flow #5762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

CDRussell
Copy link
Member

@CDRussell CDRussell commented Mar 11, 2025

Task/Issue URL: https://app.asana.com/0/1209272522976873/1209569901379925

Description

Aligns recovery code flow with connect code flow.

  • The feature is disabled by default. It will be enabled for all later via a FF update.
  • You’ll need 2 or more devices to complete the tests.
  • Suggested logcat filter: package:mine & (message~:"Sync-exchange|InvitationFlow”)

Steps to test this PR

There is a matrix of test cases detailed here: test scenarios

Feature enabled

  • Fresh install internal builds on two devices.
  • Enable exchangeKeysToSyncWithAnotherDevice FF on all devices.
  • Run through the test scenarios in the linked task, verifying it behaves as expected in each case

Feature disabled on all devices

In these tests, we want to ensure existing behaviour happens and there’s no regressions introduced even when the FF is disabled.

  • Ensure exchangeKeysToSyncWithAnotherDevice is disabled on all devices.
  • Run through the various sync scenarios ensuring everything behaves identically to production, devices can be joined etc..

Feature enabled on some, disabled on others

  • Enable the feature on one device, disable it on another
  • Verify that if the device where it is disabled is signed in and has its barcode scanned, this flow will work
  • Verify that if the device where it is enabled is signed in and has its barcode scanned, this won’t work

Smoke test cross-platform

  • Help test compatibility against as many of macOS/iOS/Windows builds as you can 🙏
  • Latest builds from other platforms available here:

Verify errors states

  • Use your sync knowledge to pay particular to the places where errors might happen that aren’t already caught (e.g., what else is missing a runCatching, which other error flows / codes aren’t handled well enough etc…)

Verify pixels

  • Use your sync knowledge to consider if any pixels are erroneously being affected.

Copy link
Member Author

CDRussell commented Mar 11, 2025

@CDRussell CDRussell force-pushed the feature/cristian/sync/align_recovery_with_connect_flow branch 4 times, most recently from 3e16354 to 11c24a9 Compare March 12, 2025 16:48
@CDRussell CDRussell changed the base branch from develop to graphite-base/5762 March 13, 2025 14:36
@CDRussell CDRussell force-pushed the feature/cristian/sync/align_recovery_with_connect_flow branch from 11c24a9 to 85dcf1a Compare March 13, 2025 14:36
@CDRussell CDRussell changed the base branch from graphite-base/5762 to feature/craig/automatically_refreshing_sync_activity March 13, 2025 14:37
@CDRussell CDRussell force-pushed the feature/cristian/sync/align_recovery_with_connect_flow branch 2 times, most recently from 0ff482b to cff88fe Compare March 17, 2025 09:57
@CDRussell CDRussell force-pushed the feature/craig/automatically_refreshing_sync_activity branch from ceb1f46 to 7348ea6 Compare March 17, 2025 09:57
CDRussell added a commit that referenced this pull request Mar 17, 2025
Task/Issue URL:
https://app.asana.com/1/137249556945/project/608920331025315/task/1209669301264637

### Description
Allows the sync settings screen to automatically refresh while it is
being viewed. This is in part to provide a better UX to
#5762.

### Steps to test this PR

- [x] Enter sync settings with sync disabled. Verify everything looks as
expected and there is no visual updates to the UI
- [x] Enable sync. Verify everything looks as expected and there is no
visual updates to the UI.
- [x] Sync with another device, such that you can see two devices in
this view; your device, plus one other
- [x] On the other device, turn off sync
- [x] Back on the first device, verify it updates within the next 5s to
show the other device is now gone
- [x] Disable `sync / automaticallyUpdateSyncSettings` feature flag and
return to the sync settings screen. Repeat the above test and verify the
other device doesn’t automatically update since background refreshing is
disabled (i.e., fallback to original behaviour when flag disabled)
- [x] Smoke test any other related sync scenarios you think might be
problematic because of this

Co-authored-by: Craig Russell <[email protected]>
Base automatically changed from feature/craig/automatically_refreshing_sync_activity to develop March 17, 2025 10:15
@CDRussell CDRussell force-pushed the feature/cristian/sync/align_recovery_with_connect_flow branch 7 times, most recently from 067af5f to 6a825cc Compare March 18, 2025 11:51
@CDRussell CDRussell force-pushed the feature/cristian/sync/align_recovery_with_connect_flow branch from 6a825cc to 7a703cc Compare March 18, 2025 12:23
@CDRussell CDRussell marked this pull request as ready for review March 18, 2025 12:53
@CDRussell CDRussell force-pushed the feature/cristian/sync/align_recovery_with_connect_flow branch 3 times, most recently from 9cedfd5 to 53cc621 Compare March 18, 2025 17:58
@cmonfortep cmonfortep force-pushed the feature/cristian/sync/align_recovery_with_connect_flow branch from 53cc621 to 193b7ae Compare March 19, 2025 15:56
@CDRussell CDRussell force-pushed the feature/cristian/sync/align_recovery_with_connect_flow branch from 193b7ae to 978ce08 Compare March 19, 2025 17:11
Copy link
Contributor

@cmonfortep cmonfortep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good.

I need to test, and I want to take another detailed pass around pixels.

@CDRussell CDRussell force-pushed the feature/cristian/sync/align_recovery_with_connect_flow branch 6 times, most recently from 498edca to 16ea2ba Compare March 20, 2025 16:32
@CDRussell CDRussell requested a review from cmonfortep March 20, 2025 16:34
@CDRussell
Copy link
Member Author

CDRussell commented Mar 20, 2025

Appreciate the first pass review @cmonfortep; all feedback addressed. back to you for a 2nd round.

Copy link
Contributor

@cmonfortep cmonfortep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All test cases worked.

Found one issue with the pixel, added a comment. It's from reader device (unauthenticated) reading QR code from another device (authenticated). We are not signaling that pixel.

@CDRussell CDRussell force-pushed the feature/cristian/sync/align_recovery_with_connect_flow branch from 16ea2ba to 239b86d Compare March 21, 2025 09:31
@CDRussell CDRussell force-pushed the feature/cristian/sync/align_recovery_with_connect_flow branch from 239b86d to ee2865a Compare March 21, 2025 09:36
@CDRussell CDRussell merged commit 8b79ca0 into develop Mar 21, 2025
6 checks passed
Copy link
Member Author

Merge activity

  • Mar 21, 5:53 AM EDT: A user merged this pull request with Graphite.

@CDRussell CDRussell deleted the feature/cristian/sync/align_recovery_with_connect_flow branch March 21, 2025 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants