-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix: Navigated back to the home screen after granting for Camera permission #47222
Fix: Navigated back to the home screen after granting for Camera permission #47222
Conversation
@DylanDylann Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
src/libs/fileDownload/FileUtils.ts
Outdated
@@ -76,6 +79,11 @@ function showCameraPermissionsAlert() { | |||
text: Localize.translateLocal('common.settings'), | |||
onPress: () => { | |||
Linking.openSettings(); | |||
// In case of ios, app reload when we enable camera permission from settings | |||
// we are saving last screen so we can navigate to it after app reload | |||
if (getPlatform() === CONST.PLATFORM.IOS && screenName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use getPlatform function. Let's create a new .ios file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created FileUtils.ios.ts file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ravindra-encoresky Ops, Don't need to create a new FileUtils.ios.ts. WƯe should introduce a new function, this function only be available on .ios file, and this func will be empty on other platforms. Then we use this function in showCameraPermissionsAlert func
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DylanDylann Do you want something like this?
Under src/lib/updateLastScreen
- index.ts - will have empty updateLastScreen function
- index.ios.ts - will have updateLastScreen function to save last screen
and I updateLastScreen will be called from showCameraPermissionsAlert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ravindra-encoresky Yes. We should avoid making duplicated code when creating new FileUtils.ts
} | ||
interceptAnonymousUser(() => { | ||
updateLastScreen(''); | ||
IOU.startMoneyRequest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check this comment
#46422 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ravindra-encoresky Please check this comment
@ravindra-encoresky Hi, how about the above comments? |
Also please record videos on all platforms to ensure that your change don't break any other things |
@DylanDylann hi, yes checked the comments. I'll add the changes soon. Sure will add the video as well. |
@ravindra-encoresky Friendly bump |
@DylanDylann I'm on it, all things done, just have to push it, due to the holidays here wasn't able to. I'll do it earliest. thanks. |
@ravindra-encoresky Bump on this comment: #47222 (comment) |
// There's no way we can check for the BLOCKED status without requesting the permission first | ||
// https://github.com/zoontek/react-native-permissions/blob/a836e114ce3a180b2b23916292c79841a267d828/README.md?plain=1#L670 | ||
CameraPermission.requestCameraPermission?.() | ||
.then((status: string) => { | ||
setCameraPermissionStatus(status); | ||
|
||
if (status === RESULTS.BLOCKED) { | ||
FileUtils.showCameraPermissionsAlert(); | ||
FileUtils.showCameraPermissionsAlert({screenName: SCREENS.RIGHT_MODAL.MONEY_REQUEST, iouType}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ravindra-encoresky I think we should save the current route in Onyx (using Navigation.getActiveRoute function)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you are correct. I am making necessary changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DylanDylann i have made the necessary changes to use activeRoute and it is working as expected
* Navigate to scan receipt screen after it enabling camera permission from setting | ||
* This will only works for ios application because we are saving last screen only for ios | ||
*/ | ||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you put this useEffect
into this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find any other file for navigating to the Receipt Scan screen when the app opens. btw I looking to optimise it by using activeRoute instead of hardcoding last screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DylanDylann Another screen where I could place this useEffect is SidebarScreen.tsx. Please let me know if I’m missing any part of the flow for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ravindra-encoresky Let's try in Expensify.tsx file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DylanDylann done
I have added videos for all platforms except iOS Safari, as the camera doesn’t work in the Safari browser on the emulator. Additionally, I am unable to find a way to run the mobile web version on a physical iOS device. |
src/ONYXKEYS.ts
Outdated
@@ -400,6 +400,9 @@ const ONYXKEYS = { | |||
/** Stores the information about currently edited advanced approval workflow */ | |||
APPROVAL_WORKFLOW: 'approvalWorkflow', | |||
|
|||
/** screen to open after reloading app after changing app permission from settings */ | |||
LAST_SCREEN: 'last_screen', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LAST_SCREEN: 'last_screen', | |
LAST_ROUTE: 'lastRoute', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anh also update other places to use route
instead of screen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/ONYXKEYS.ts
Outdated
@@ -400,6 +400,9 @@ const ONYXKEYS = { | |||
/** Stores the information about currently edited advanced approval workflow */ | |||
APPROVAL_WORKFLOW: 'approvalWorkflow', | |||
|
|||
/** screen to open after reloading app after changing app permission from settings */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** screen to open after reloading app after changing app permission from settings */ | |
/** Stores the route to open after changing app permission from settings */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/libs/saveLastScreen/index.ios.ts
Outdated
import {updateLastScreen} from '@libs/actions/App'; | ||
import Navigation from '@libs/Navigation/Navigation'; | ||
|
||
export default function saveLastScreen() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export default function saveLastScreen() { | |
export default function saveLastRoute() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/libs/fileDownload/FileUtils.ts
Outdated
@@ -76,6 +77,9 @@ function showCameraPermissionsAlert() { | |||
text: Localize.translateLocal('common.settings'), | |||
onPress: () => { | |||
Linking.openSettings(); | |||
// In case of ios, app reload when we enable camera permission from settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// In case of ios, app reload when we enable camera permission from settings | |
// In the case of ios, the App reloads when we update camera permission from settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/libs/saveLastScreen/index.ts
Outdated
@@ -0,0 +1,3 @@ | |||
/** we are only saving last screen in case of ios */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** we are only saving last screen in case of ios */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/libs/saveLastScreen/index.ts
Outdated
@@ -0,0 +1,3 @@ | |||
/** we are only saving last screen in case of ios */ | |||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | |||
export default function saveLastScreen() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export default function saveLastScreen() {} | |
const saveLastRoute: GetPlaidDesktopMessage = () => {}; | |
export default saveLastRoute; | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DylanDylann There is a issue of type mismatch for return (Type 'void' is not assignable to type 'TranslationPaths | undefined)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ravindra-encoresky ahh my bad, please remove GetPlaidDesktopMessage
src/Expensify.tsx
Outdated
@@ -335,6 +357,9 @@ export default withOnyx<ExpensifyProps, ExpensifyOnyxProps>({ | |||
lastVisitedPath: { | |||
key: ONYXKEYS.LAST_VISITED_PATH, | |||
}, | |||
lastRoute: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ravindra-encoresky Please use useOnyx
Some minor comments, @ravindra-encoresky Could you address them soon? |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-08-26.at.14.51.07.movAndroid: mWeb ChromeScreen.Recording.2024-08-26.at.14.58.44.moviOS: NativeScreen.Recording.2024-08-26.at.14.59.42.moviOS: mWeb SafariScreenRecording_08-26-2024.15-16-14_1.MP4MacOS: Chrome / SafariScreen.Recording.2024-08-26.at.15.03.45.movMacOS: DesktopScreen.Recording.2024-08-26.at.15.03.29.mov |
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/Expensify.tsx
Outdated
@@ -236,6 +238,16 @@ function Expensify({ | |||
Audio.setAudioModeAsync({playsInSilentModeIOS: true}); | |||
}, []); | |||
|
|||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should replace this with a useLayoutEffect
so that we can run Navigation.navigate
to route to the correct screen before the first paint of Expensify.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Tested again, working perfectly.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.0.26-1 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.0.26-6 🚀
|
Details
Fixed Issues
$ #46422
PROPOSAL: $ #46422 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
android_web.mov
iOS: Native
355539874-76950b43-82cc-444f-bd0f-99fa09ef6bdc.mov
iOS: mWeb Safari
Only iOS Native affectedMacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov