-
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
Associate crashes with user info #43063
Changes from all commits
49691ca
6a87f2b
e7c6950
4ff36f0
5a4322c
a07c2df
782dbd1
c0615dd
1038428
af7b29d
117be83
d9353aa
090c470
050b284
ca62e42
b5090b9
1cb89e1
9eeee55
3e5f033
6833ee4
10cd8b5
2e6eba0
598c159
eef37d5
d0d3484
06e2616
fe9ce0f
3afbed8
4d26455
50ab2dd
e71d115
d4e3795
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
import React from 'react'; | ||
import {View} from 'react-native'; | ||
import Button from '@components/Button'; | ||
import TestToolRow from '@components/TestToolRow'; | ||
import useLocalize from '@hooks/useLocalize'; | ||
import testCrash from '@libs/testCrash'; | ||
import firebase from '../../../firebase.json'; | ||
|
||
/** | ||
* Adds a button in native dev builds to test the crashlytics integration with user info. | ||
*/ | ||
function TestCrash() { | ||
const {translate} = useLocalize(); | ||
|
||
const isCrashlyticsDebugEnabled = firebase?.['react-native']?.crashlytics_debug_enabled ?? false; | ||
|
||
const toolRowTitle = translate('initialSettingsPage.troubleshoot.testCrash'); | ||
|
||
return ( | ||
roryabraham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<View> | ||
{isCrashlyticsDebugEnabled ? ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't we want this button to show on staging? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will show in staging if crashlytics_debug_enabled is true. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So we don't need to set that in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we want to show this option only when a developer manually enables the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On staging how are we going to enable it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want this PR to work for both staging and production, but not for dev or AdHoc builds by default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ShridharGoel, the button will show for both staging and production, right? I don't see the changes to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The user ID changes will work for staging and production. The testing button would show only when firebase.json is updated manually. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry i am confused here, then why not remove the testing button after confirming the data is being sent to firebase is correct? if it will only be visible only on dev (which i assume is the only way to update firebase.json) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think having the button would help us in testing it if needed in the future. Though it wouldn't be needed by QA team. |
||
<TestToolRow title={toolRowTitle}> | ||
<Button | ||
small | ||
text={toolRowTitle} | ||
onPress={testCrash} | ||
/> | ||
</TestToolRow> | ||
) : null} | ||
</View> | ||
); | ||
} | ||
|
||
export default TestCrash; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import React from 'react'; | ||
import {View} from 'react-native'; | ||
|
||
function TestCrash() { | ||
return <View />; | ||
} | ||
|
||
export default TestCrash; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import crashlytics from '@react-native-firebase/crashlytics'; | ||
|
||
const setCrashlyticsUserId = (accountID: string | number) => { | ||
crashlytics().setUserId(accountID.toString()); | ||
}; | ||
|
||
export default setCrashlyticsUserId; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
const setCrashlyticsUserId = (_: string | number) => {}; | ||
export default setCrashlyticsUserId; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import crashlytics from '@react-native-firebase/crashlytics'; | ||
|
||
const testCrash = () => { | ||
crashlytics().crash(); | ||
}; | ||
export default testCrash; |
roryabraham marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
const testCrash = () => {}; | ||
export default testCrash; |
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.
And i think a comment would be helpful in both files, but since it's probably temporary component i am not sure.