Skip to content
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

feat(onboarding): add AC notif for importing old accounts #17078

Open
wants to merge 1 commit into
base: feat/new-onboarding-metrics
Choose a base branch
from

Conversation

jrainville
Copy link
Member

What does the PR do

Fixes #17028

When an old user imports an account, we now fetch the backups in the background and show an AC notification.

When the fetch is successful, the AC notif switches to a success message.

If after a timeout we detect that we didn't fetch anything or just part, we show an error and the possibility to try again.

Based on top of #17058

Status-go PR: status-im/status-go#6255

Affected areas

Onboarding and AC. In the status-go side, backup handling

Architecture compliance

Screenshot of functionality (including design for comparison)

  • I've checked the design and this PR matches it (kindof lol)
backup-fetching.webm

@micieslak @caybro if I could request your help please. I managed to make all the states (pending, failure, success), but the badge looks ugly in all but the pending state.
If you could give me a hand in making sure the badge icon looks ok, I'd appreciate. See in the video how the checkmark is way to big for no reason.

Impact on end user

Instead of waiting on a screen for the fetching to work only at the end they could move to the main screen, now they can see the app straight away and data is fetched in the background and shows up when it's ready.

How to test

  • Have an account that is backed up and import it using the new onboading with the recovery phrase
  • To test the failure state, either let me know and I can tell you how to hack the code to simulate a failure or use the recovery phrase for an account that hasn't been used in 30 days. Or just use a random mnemonic

Risk

Described potential risks and worst case scenarios.

Tick one:

  • Low risk: 2 devs MUST perform testing as specified above and attach their results as comments to this PR before merging.
  • High risk: QA team MUST perform additional testing in the specified affected areas before merging.

Worst case, the bakcup fetching doesn't work

@jrainville jrainville requested review from micieslak, caybro, alexjba and a team as code owners January 15, 2025 19:03
@status-im-auto
Copy link
Member

status-im-auto commented Jan 15, 2025

Jenkins Builds

Click to see older builds (14)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ ee35755 #1 2025-01-15 19:11:26 ~8 min macos/aarch64 🍎dmg
✔️ ee35755 #1 2025-01-15 19:12:55 ~9 min tests/nim 📄log
✔️ ee35755 #1 2025-01-15 19:15:09 ~11 min tests/ui 📄log
✔️ ee35755 #1 2025-01-15 19:22:08 ~18 min macos/x86_64 🍎dmg
✔️ ee35755 #1 2025-01-15 19:23:18 ~20 min linux-nix/x86_64 📦tgz
✔️ ee35755 #1 2025-01-15 19:24:13 ~20 min linux/x86_64 📦tgz
✔️ ee35755 #1 2025-01-15 19:34:29 ~31 min windows/x86_64 💿exe
✔️ a61d968 #2 2025-01-15 21:05:10 ~5 min macos/aarch64 🍎dmg
✔️ a61d968 #2 2025-01-15 21:09:11 ~9 min tests/nim 📄log
✔️ a61d968 #2 2025-01-15 21:10:58 ~11 min tests/ui 📄log
✔️ a61d968 #2 2025-01-15 21:11:24 ~11 min macos/x86_64 🍎dmg
✔️ a61d968 #2 2025-01-15 21:17:18 ~17 min linux-nix/x86_64 📦tgz
✔️ a61d968 #2 2025-01-15 21:19:25 ~19 min linux/x86_64 📦tgz
✔️ a61d968 #2 2025-01-15 21:25:47 ~25 min windows/x86_64 💿exe
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 6958aa5 #3 2025-01-16 16:36:47 ~6 min macos/aarch64 🍎dmg
✔️ 6958aa5 #3 2025-01-16 16:39:56 ~9 min tests/nim 📄log
✔️ 6958aa5 #3 2025-01-16 16:42:17 ~11 min tests/ui 📄log
✔️ 6958aa5 #3 2025-01-16 16:43:24 ~12 min macos/x86_64 🍎dmg
✔️ 6958aa5 #3 2025-01-16 16:48:58 ~18 min linux-nix/x86_64 📦tgz
✔️ 6958aa5 #3 2025-01-16 16:51:26 ~21 min linux/x86_64 📦tgz
✔️ 6958aa5 #3 2025-01-16 17:00:43 ~30 min windows/x86_64 💿exe
✔️ a3fcb11 #4 2025-01-16 17:19:09 ~7 min macos/aarch64 🍎dmg
✔️ a3fcb11 #4 2025-01-16 17:21:09 ~9 min tests/nim 📄log
a3fcb11 #4 2025-01-16 17:23:00 ~11 min tests/ui 📄log
✔️ a3fcb11 #4 2025-01-16 17:23:08 ~11 min macos/x86_64 🍎dmg
✔️ a3fcb11 #4 2025-01-16 17:28:34 ~17 min linux-nix/x86_64 📦tgz
✔️ a3fcb11 #4 2025-01-16 17:32:57 ~21 min linux/x86_64 📦tgz
✔️ a3fcb11 #4 2025-01-16 17:37:53 ~26 min windows/x86_64 💿exe

Copy link
Member

@caybro caybro left a comment

Choose a reason for hiding this comment

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

Looks good in general!

asset {
width: 24
height: width
name: d.assetName
Copy link
Member

Choose a reason for hiding this comment

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

Always the same, no need to put it into d

color: Theme.palette.primaryColor1
bgWidth: 40
bgHeight: 40
bgColor: d.assetBgColor
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@jrainville jrainville force-pushed the feat/new-onboarding-profile-fetching branch from ee35755 to a61d968 Compare January 15, 2025 20:59
@jrainville jrainville force-pushed the feat/new-onboarding-metrics branch from d2d7fe9 to 0e412d4 Compare January 16, 2025 16:29
@jrainville jrainville force-pushed the feat/new-onboarding-profile-fetching branch from a61d968 to 6958aa5 Compare January 16, 2025 16:30
@jrainville jrainville force-pushed the feat/new-onboarding-metrics branch from 0e412d4 to aa5321c Compare January 16, 2025 17:11
Fixes #17028

When an old user imports an account, we now fetch the backups in the background and show an AC notification.

When the fetch is successful, the AC notif switches to a success message.

If after a timeout we detect that we didn't fetch anything or just part, we show an error and the possibility to try again.
@jrainville jrainville force-pushed the feat/new-onboarding-profile-fetching branch from 6958aa5 to a3fcb11 Compare January 16, 2025 17:11
Copy link
Member

@caybro caybro left a comment

Choose a reason for hiding this comment

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

👍

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.

3 participants