Skip to content
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

Feature set language callbacks #1536

Merged
merged 9 commits into from
Mar 1, 2022
Merged

Conversation

tanaynigam
Copy link
Contributor

@tanaynigam tanaynigam commented Feb 22, 2022

Description

Line Summary

Add set language callback

Details

Motivation

The feature allows the user to use the onSuccess and onFailure callbacks for setLanguage method

Public API Changes

The documentation will need to be updated for Android to provide these new methods

  • Add OSDeviceInfoCompletionHandler
  • Add setLanguage(String, OSDeviceInfoCompletionHandler)

Testing

Unit Tests

Add TestSetLanguageHandler to set up callbacks for setLanguage
Add following Unit test cases

  • Add shouldSetLanguageWithResponse to test for success callback
  • Add shouldFailToSetLanguageWithResponse to test for failure callback

This change is Reviewable

* Add getLanguage to OneSignalStateSynchronizer and UserStatePushSynchronizer
* Add OSDeviceInfoError and OSDeviceInfoCompletionHandler to OneSignal.java
* Add deviceInfoCompletionHandler
* Add deviceInfoHandlersPerformOnSuccess
* Add deviceInfoHandlersPerformOnFailure
* Update updateDeviceInfo by adding OSDeviceInfoCompletionHandler
* Add setLanguage with completion handler
* setLanguage without completion handler calls null on completion handler
* Add OSDeviceInfo completion handler to setLanguage
@nan-li nan-li self-requested a review February 22, 2022 21:57
* Add OSLanguageCompletionHandler
* Add OSLanguageError
* Update setLanguage with OSLanguageCompletionHandler
* Add TestSetLanguageHandler for unit test setLanguage with callback setup
* Add shouldSetLanguageWithResponse to check for success callback
* Add shouldFailToSetLanguageWithResponse to check for failure callback
@tanaynigam tanaynigam changed the title Feature/set language callbacks Feature set language callbacks Feb 24, 2022
Copy link
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.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nan-li and @tanaynigam)


OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java, line 232 at r1 (raw file):

   }

   static class OSDeviceInfoError {

Since OneSignalStateSynchronizer produces this internal type it should be defined there. Along with OSDeviceInfoCompletionHandler below.


OneSignalSDK/unittest/src/test/java/com/test/onesignal/MainOneSignalClassRunner.java, line 2810 at r1 (raw file):

      // now test to make sure the handler still fires for a call to
      // setLanguage() that doesn't modify existing language (no JSON delta)

This comment isn't accurate, above the language is "fr" and below it is "es" so there is a change.

* Move OSDeviceInfoError and OSDeviceInfoCompletionHandler to OneSignalStateSynchronizer
* Add appropriate imports in files OneSignal.java, OneSignalStateSynchronizer and UserStateSynchronizer
* Update comment in setLanguage with success handler in Unit Test
Copy link
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.

:lgtm:

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nan-li and @tanaynigam)

@tanaynigam tanaynigam merged commit ce6b49b into main Mar 1, 2022
@tanaynigam tanaynigam deleted the feature/set_language_callbacks branch March 1, 2022 23:53
@nan-li nan-li mentioned this pull request Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants