[Wallet] Prevent exiting force update screen #5430
Conversation
gnardini
requested review from
cmcewen,
i1skn,
jeanregisser and
jmrossy
as code owners
|
@jeanregisser I'm sorry, I updated the PR just before your comment :( You might want to take another look. It does prevent swiping back and pressing the back button, yes! |
| let updateRequired = false | ||
| if (minRequiredVersion) { | ||
| const version = DeviceInfo.getVersion() | ||
| updateRequired = isVersionBelowMinimum(version, minRequiredVersion) | ||
| if (!updateRequired) { | ||
| dispatch(setRequiredVersion(null)) | ||
| } | ||
| } |
There was a problem hiding this comment.
We shouldn't dispatch during render.
This logic should live in a saga.
There was a problem hiding this comment.
This is run only once. Which saga do you think this belongs on? I understand where you're coming from, but this seemed like the most intuitive way to solve this and I don't see a big risk dispatching here.
packages/mobile/src/app/actions.ts
Outdated
| @@ -26,6 +26,7 @@ export enum Actions { | |||
| UNLOCK = 'APP/UNLOCK', | |||
| SET_SESSION_ID = 'SET_SESSION_ID', | |||
| OPEN_URL = 'APP/OPEN_URL', | |||
| SET_REQUIRED_VERSION = 'APP/SET_REQUIRED_VERSION', | |||
There was a problem hiding this comment.
Would be nice to avoid introducing new setters and try to gradually migrate to events.
How about we use MIN_APP_VERSION_DETERMINED instead?
| const updateRequired = React.useMemo(() => { | ||
| if (!minRequiredVersion) { | ||
| return false | ||
| } | ||
| const version = DeviceInfo.getVersion() | ||
| const versionBelowMinimum = isVersionBelowMinimum(version, minRequiredVersion) | ||
| if (!versionBelowMinimum) { | ||
| dispatch(minAppVersionDetermined(null)) | ||
| } | ||
| return versionBelowMinimum | ||
| }, [minRequiredVersion]) |
There was a problem hiding this comment.
Still feels wrong, I don't understand why we need to call dispatch here.
This should be in a selector I think.
There was a problem hiding this comment.
Well, I just wanted to avoid doing this device check every time, if we already know the user updated the app I feel it would be better to have the minRequiredVersion as null but I guess we can just not dispatch anything and check against the latest minRequiredVersion found every time.
Is something like this what you had in mind?
There was a problem hiding this comment.
Yes I think so! Happy to chat about this tomorrow, I might have missed something here.
gnardini
force-pushed
the
force-update
branch
from
f048992
to
c8d6318
Compare
There was a problem hiding this comment.
Question - how is the swipe and hardware back button being disabled?
My understanding is that swipes need to be disabled like this: https://github.com/celo-org/celo-monorepo/blob/master/packages/mobile/src/navigator/Headers.tsx#L24
and hardware back button needs to be overwritten with an event listener like this: https://github.com/celo-org/celo-monorepo/blob/master/packages/mobile/src/identity/PhoneNumberLookupQuotaScreen.tsx#L42
They were disabled with two mechanisms, depending on the situation:
The two cases you pointed out are for when there are other screens in the stack, but it's not the case here. |
|
Changed the implementation so that the minVersion is read in real time from Firebase, |
👍
### Description The Update screen could be dismissed before this PR. It shouldn't be dismissable, the intention is to force the update. I had to add the min required version in Redux to force the screen when backgrounding and reopening the app since if the app was reloaded the screen could still be skipped. This happened specially on Android. ### Tested Locally changing the app version on both platforms. ### Related issues - Fixes #5429
|
Hey @gnardini i have verified tried to verify this task with taking play store v1.2 but not getting Force update screen. |
Description
The Update screen could be dismissed before this PR. It shouldn't be dismissable, the intention is to force the update.
I had to add the min required version in Redux to force the screen when backgrounding and reopening the app since if the app was reloaded the screen could still be skipped. This happened specially on Android.
Tested
Locally changing the app version on both platforms.
Related issues