-
Notifications
You must be signed in to change notification settings - Fork 376
Fix: NPE when setting timezone on app focus #2505
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
base: main
Are you sure you want to change the base?
Conversation
📊 Diff Coverage ReportDiff Coverage ReportThreshold: 80% Changed Files Coverage
Overall Coverage90/157 lines covered (57.3%) ❌ Coverage Check FailedFiles below 80% threshold:
|
| // Detect any user properties updates that changed | ||
| _propertiesModel.timezone = TimeUtils.getTimeZoneId() | ||
| // Skip updating timezone when onesignalId is not set in the property model | ||
| if (_propertiesModel.hasProperty(PropertiesModel::onesignalId.name)) { |
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 seems to be decent workaround to prevent the crash. And this method will be called frequently and the timezone could be updated later when the oneSignaId is actually available
nan-li
left a 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.
This guard works but there's a deeper issue. There's no clear signal in the code that setting timezone requires onesignal ID to exist first. This is the kind of implicit dependency that's impossible to catch without already knowing about it.
Good catch on this @nan-li . i hope the new fix that we discussed will address this issue. |
|
@nan-li @abdulraqeeb33 I have updated with another approach by updating the timezone onSessionStart and onSessionActive. Please take a look. |
2216734 to
a34ba62
Compare
abdulraqeeb33
left a 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.
the newer approach looks fine to me, but the tests are broken @jinliu9508
The test is now removed because onFocus no longer updates timezone |
7dc8c2b to
160375c
Compare
I have updated the PR description to include a manual test I found that can reproduce the exact error. After the fix we can see that the error for this particular logical path is fixed. |
Description
One Line Summary
Fix NPE from setting timezone on app focus when onesignalId is missing from the property model.
Details
Motivation
Fix NPE from setting timezone on app focus when onesignalId is missing from the property model.
This change was added in #2378
Scope
Update the timezone in onSessionStart and onSessionActive instead of in UserManager.onFocus, since the propertiesModel may not be fully initialized at that time.
Related Issues
#2413
Testing
Unit testing
Add a test case for updating the timezone when onSessionStart() and onSessionActive is called.
Manual testing
Step to reproduce:
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is