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

[Wallet] Prompt users with connectivity issues to switch to forno #2526

Merged
merged 20 commits into from
Jan 24, 2020

Conversation

annakaz
Copy link
Contributor

@annakaz annakaz commented Jan 23, 2020

Description

When geth is taking too long to sync (>15 seconds for web3 to sync or seeing connectivity issues with geth), prompt them to turn on forno. Do not prompt if users have already declined to avoid an instrusive experience, and wait to prompt until after they have gone through the welcome screen

Tested

On a pixel 2 by changing sync to take too long and confirming that users are prompted to switch to forno

Other changes

Cancel geth sagas when switching to forno rather than checking within sagas whether forno is enabled

Related issues

Backwards compatibility

Yes

@annakaz annakaz requested a review from a team January 23, 2020 23:20
"promptZeroSyncModal": {
"header": "Cambiar modo de conexión?",
"body": "Hemos notado que tienes problemas para conectarte. Recomendamos habilitar el modo de ahorro de datos para que pueda seguir usando Celo Wallet con una conexión intermitente.",
"switchToDataSaver": "Habilitar Data Saver"
Copy link
Contributor

Choose a reason for hiding this comment

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

Once #2539 merges please use Ahorro de Datos here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated translation

type: Actions.CANCEL_GETH_SAGA,
})

interface SetPromptZeroSync {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, I think the name convention we use elsewhere is SetPromptZeroSyncAction, though I guess not in this file for some reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated all actions in this file to use the normal naming convention

// Suggest switch to forno for network-related errors
if (yield select(promptZeroSyncIfNeededSelector)) {
yield put(setPromptZeroSync(false))
navigate(Screens.DataSaver, { promptModalVisible: true })
Copy link
Contributor

Choose a reason for hiding this comment

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

I assumed we would just enable forno silently in this case. @nityas can you confirm that navigating the user to the Data Saver screen is the desired behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The user gets navigated to the DataSaver screen with a modal, then once they confirm/decline the switch they go back to wherever they were

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed with @nityas that this prompt is the expected behavior!

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. confirmed-- it adds a layer of centralization and we do not want to do this without user consent

packages/mobile/src/invite/JoinCelo.tsx Show resolved Hide resolved
packages/mobile/src/geth/selectors.ts Show resolved Hide resolved
@annakaz annakaz assigned jmrossy and unassigned annakaz Jan 24, 2020
@codecov
Copy link

codecov bot commented Jan 24, 2020

Codecov Report

Merging #2526 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #2526    +/-   ##
========================================
  Coverage   73.24%   73.24%            
========================================
  Files         556      556            
  Lines       13824    13824            
  Branches     1722     1425   -297     
========================================
  Hits        10125    10125            
  Misses       3418     3418            
  Partials      281      281
Flag Coverage Δ
#mobile 74.1% <ø> (ø) ⬆️
#web 72.02% <ø> (ø) ⬆️
Impacted Files Coverage Δ
packages/mobile/test/schemas.ts 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7c5cfc...1311a2e. Read the comment docs.

@annakaz annakaz requested a review from a team January 24, 2020 19:59
yield fork(monitorGeth)
const gethRelatedSagas = yield all([fork(monitorAppState), fork(monitorGeth)])
yield take(Actions.CANCEL_GETH_SAGA)
yield cancel(gethRelatedSagas)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you confirm this cancels the individual forks and not just the synthesized one created by all()? If monitorGeth's logs no longer show up after cancel that would confirm it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes confirmed that monitorGeth logs go away! Agree that the redux saga cancel all documentation is unclear though (https://redux-saga.js.org/docs/advanced/TaskCancellation.html / outdated post redux-saga/redux-saga#988 (comment))

@@ -63,6 +63,7 @@ export const vNeg1Schema = {
geth: {
initialized: 'INITIALIZED',
connected: true,
promptZeroSyncIfNeeded: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

state schema changes, including new additions, need to go in the latest schema (v5 below) since it didn't exist in v-1

@jmrossy jmrossy assigned annakaz and unassigned jmrossy Jan 24, 2020
@annakaz
Copy link
Contributor Author

annakaz commented Jan 24, 2020

Note that build is failing on master due to attestation service (unrelated to this PR)

@annakaz annakaz merged commit 5c4f07e into master Jan 24, 2020
aaronmgdr added a commit that referenced this pull request Jan 28, 2020
* master:
  🧹Web cleanup (readme + static dir) (#2562)
  Add readable proposals to governance:view command (#2545)
  Add explicit gas to exchange transactions to prevent errors (#2552)
  Fix off-by-one error in attributing signatures to blocks in CLI (#2559)
  ✅ add test for phone Input component (#2554)
  Add Youtube to Footer +  (#2556)
  Fix rounding error in Election.sol (#2540)
  [Wallet] Bump @celo/client to 0.0.266 (#2551)
  [Wallet] E2E test improvements (#2542)
  Deployed integration (#2550)
  do not fetch affiliates (#2508)
  Added more CLI checks for registering validators and groups (#2491)
  Micro Improvement to web tests (#2527)
  [Wallet] Prompt users with connectivity issues to switch to forno (#2526)
  cli: Fix voter rewards presentation (#2543)
  [Wallet] Fix missing spanish translation  (#2539)
  Downtime slashing when epoch changes (#2436)
@mcortesi mcortesi deleted the annakaz/prompt-forno branch February 14, 2020 22:03
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.

Users SBAT be prompted to switch to forno when experiencing connectivity issues
3 participants