Skip to content

fix: NullPointerException getTargetSdkVersion#1848

Merged
jkasten2 merged 1 commit intoOneSignal:mainfrom
kyunkakata:main
Sep 26, 2023
Merged

fix: NullPointerException getTargetSdkVersion#1848
jkasten2 merged 1 commit intoOneSignal:mainfrom
kyunkakata:main

Conversation

@kyunkakata
Copy link
Copy Markdown
Contributor

@kyunkakata kyunkakata commented Sep 22, 2023 •
edited by niels-s
Loading

Description

Sometime, the application starts but the context was not initialized in OneSignal module soon enough and it will be null. This happens sometime with React Native application. And the block of above code will be executed when NotificationPermissionController is initialized --> Context is null. So it should be moved to block of code of the function prompt below. So when we call from JS promptForPushNotificationsWithUserResponse later, the context should be initialized.

Details

Motivation

Fix the crash OneSignal/react-native-onesignal#1516

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

@kyunkakata kyunkakata mentioned this pull request Sep 22, 2023
Closed
3 tasks
jkasten2
jkasten2 requested changes Sep 22, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@jkasten2 jkasten2 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kyunkakata thanks for the PR! I left some feedback that needs to be addressed before we can accept this PR.

Comment thread OneSignalSDK/onesignal/src/main/java/com/onesignal/NotificationPermissionController.kt Outdated
jkasten2
jkasten2 requested changes Sep 26, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kyunkakata Thanks for addressing the feedback. Could you clean up your commit history to be a single commit? You can use git rebase on your fork to do so or make a new fork if that is easier.

@kyunkakata
fix: NullPointerException getTargetSdkVersion
896ec50 
fix: change to lazy variable

lint

lint

fix: wrong lazy implement

fix: add check sdk back
@kyunkakata
Copy link
Copy Markdown
Contributor Author

kyunkakata commented Sep 26, 2023

Done!

jkasten2
jkasten2 approved these changes Sep 26, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixups! Approved and merging it into main.

@jkasten2 jkasten2 merged commit 08b951c into OneSignal:main Sep 26, 2023
@joaquinvaz
Copy link
Copy Markdown

joaquinvaz commented Oct 4, 2023

@jkasten2 can we get a release on this please? we are having this issue on production and need a fix asap please

@nan-li
Copy link
Copy Markdown
Contributor

nan-li commented Oct 4, 2023

I'm encountering an issue running the DevApp after this PR merged. Will investigate further.
SDKVersionChange

@kyunkakata kyunkakata mentioned this pull request Oct 7, 2023
Closed
18 tasks
@kyunkakata
Copy link
Copy Markdown
Contributor Author

kyunkakata commented Oct 7, 2023

I'm encountering an issue running the DevApp after this PR merged. Will investigate further. SDKVersionChange

I have removed the annotation in this pull request #1866. Please check if this resolves the problem.

@jkasten2 jkasten2 mentioned this pull request Dec 6, 2023
Merged
18 tasks
@nan-li nan-li mentioned this pull request Dec 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkasten2 jkasten2 jkasten2 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kyunkakata @joaquinvaz @nan-li @jkasten2