-
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
Conversation
@ShridharGoel don't we need the crash buttons in |
I've updated this locally, will update the PR once tested. |
c02b28c
to
a07c2df
Compare
@getusha Updated. |
@ShridharGoel you've got lint errors here |
8bc7e4f
to
c0615dd
Compare
@ShridharGoel we have a couple of checks failing and a conflict! |
8f4e5ff
to
c0615dd
Compare
3cb9b38
to
c0615dd
Compare
818b10e
to
4d26455
Compare
@getusha thanks for your review. Looks like all your comments have been addressed and it's ready for another review |
|
||
return ( | ||
<View> | ||
{isCrashlyticsDebugEnabled ? ( |
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 we want this button to show on staging?
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.
This will show in staging if crashlytics_debug_enabled is true.
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.
This will show in staging if crashlytics_debug_enabled is true.
So we don't need to set that in firebase.json
for staging?
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 think we want to show this option only when a developer manually enables the crashlytics_debug_enabled
flag. Do let me know your thoughts.
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 think we want to show this option only when a developer manually enables the crashlytics_debug_enabled flag. Do let me know your thoughts.
On staging how are we going to enable it?
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 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 comment
The 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 firebase.json
in this PR
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.
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 comment
The 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.
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 comment
The 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.
@ShridharGoel i am not seeing any logs that shows the data being sent Screen.Recording.2024-07-13.at.3.16.44.in.the.afternoon.mov |
Bump @ShridharGoel |
@getusha You need to check in Android Studio logcat. |
@ShridharGoel i'll check it thanks, we have a couple of comments ^ |
@ShridharGoel lets add steps for staging, otherwise mark this as |
Updated. |
@getusha can you take a look at @ShridharGoel's changes? |
lgtm i will need to confirm the logs from logcat |
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.
Wasn't able to confirm the data being sent to Firebase, but the rest looks good to me.
https://expensify.slack.com/archives/C01GTK53T8Q/p1722351381164999?thread_ts=1722282113.203869&cid=C01GTK53T8Q
@ShridharGoel could you please update the PR description? thanks! |
Updated. |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
✋ 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.15-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.15-9 🚀
|
Details
Associate crashes with user info.
Fixed Issues
$ #42651
PROPOSAL: #42651 (comment)
Testing
Update firebase.json to set
crashlytics_debug_enabled
to true.Update
platformSetup/index.native.ts
to this:Native crash would not work on iOS debug build because we are on an old version. See invertase/react-native-firebase#6226
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop