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

SMS integration #1263

Merged
merged 23 commits into from
Mar 12, 2021
Merged

SMS integration #1263

merged 23 commits into from
Mar 12, 2021

Conversation

Jeasmine
Copy link
Contributor

@Jeasmine Jeasmine commented Jan 25, 2021

SMS Integration

Emulates current channel email behavior, but in this case, for the new channel SMS.

Since most of the work between SMS and Email is the same, a parent class UserStateSecondaryChannelSynchronizer was created. This will hold all core functionality.

  • Added setSMSNumber public function
  • Add SMS observers
  • Add SMS data to DeviceState
  • Add UserStateSMSSynchronizer and UserStateSecondaryChannelSynchronizer files, share email and SMS logic under UserStateSecondaryChannelSynchronizer
  • Move players and channel tests to SynchronizerIntegrationTests

Difference between SMS and email, not parent_player_id is sent (this is removed on logout PR), we relay on external_id

It is recommended to review commit by commit. The core implementation is done on the firsts commit, then on each test commit changes are made to make the feature truly work.

This change is Reviewable

@Jeasmine Jeasmine added the WIP Work In Progress label Jan 25, 2021
@Jeasmine Jeasmine force-pushed the feature/sms-integration branch 4 times, most recently from e51f00b to 4846470 Compare February 1, 2021 19:19
@Jeasmine Jeasmine changed the title WIP - SMS integration SMS integration Feb 2, 2021
@Jeasmine Jeasmine removed the WIP Work In Progress label Feb 2, 2021
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.

Overall PR looks good, really like the work put into splitting up the tests here!

Reviewed 10 of 22 files at r1, 14 of 15 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @emawby, @gonzalonarbaiz, and @Jeasmine)


OneSignalSDK/onesignal/src/main/java/com/onesignal/UserStateEmailSynchronizer.java, line 48 at r3 (raw file):

    @Override
    void logoutSMS() {

This class has logoutEmail() and logoutSMS(), I would expect logoutChannel() instead. I haven't looked at your other PR so this might be addressed.


OneSignalSDK/unittest/src/test/java/com/test/onesignal/LocationIntegrationTests.java, line 86 at r3 (raw file):

)
@RunWith(RobolectricTestRunner.class)
public class LocationIntegrationTests {

💯 Nice clean up moving this to a different file!

Copy link
Contributor Author

@Jeasmine Jeasmine left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @emawby, @gonzalonarbaiz, and @jkasten2)


OneSignalSDK/onesignal/src/main/java/com/onesignal/UserStateEmailSynchronizer.java, line 48 at r3 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

This class has logoutEmail() and logoutSMS(), I would expect logoutChannel() instead. I haven't looked at your other PR so this might be addressed.

I Will address this on the logout PR! thanks for pointing this out, I agree 100%


OneSignalSDK/unittest/src/test/java/com/test/onesignal/LocationIntegrationTests.java, line 86 at r3 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

💯 Nice clean up moving this to a different file!

🔥

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.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @emawby and @gonzalonarbaiz)

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 10 of 10 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @emawby and @gonzalonarbaiz)

* Creation of base UserStateSMSSynchronizer for the new SMS channel
* Call SMS Synchronizer methods under OneSignalStateSynchronizer
* Add UserStateSecondaryChannelSynchronizer class
* Move shared methods between SMS and Email to UserStateSecondaryChannelSynchronizer
* Add OneSignal.java setSMSNumber public method
* Add UserStateSMS
* Add all combinations for setSMSNumber
* Move External Id tests to SynchronizerIntegrationTests
* Move email user state tests to SynchronizerIntegrationTests
* Add SMS State classes
* Add Onesignal SMS state observers
* Add OneSignal get and save sms id
* Create tests for sms only
* Create tests for sms and email
* Add failure and success callbacks for SMS
* Add JSONObject response for SMS success callback
* Share string constants between all UserStateSynchronizer
* Create setChannelId under UserStateSecondaryChannelSynchronizer
* Add test for onfocus with all channels available
* Fix channel id save method
* Move location tests from MainOneSignalClassRunner file to LocationIntegrationTests
* Add SMS Observer tests
* Add DeviceState SMS data
* Add DeviceState SMS tests
* Call on_purchase on all channels
* Update on_purchase test to include SMS
* Add tests for combination cases between external id and SMS auth hash
* Add logoutSMS() public method
* Add logout SMS functionality to UserStateSynchronizers
* Remove parent player update from SMS
* Update tests after removal of parent player id link from SMS
* Add logoutSMS tests
* On non UnitTest enviroment push channel ends too quickly and recreats sms uses
* Call SMS channel first
@Jeasmine Jeasmine merged commit 31a4d1d into master Mar 12, 2021
@Jeasmine Jeasmine deleted the feature/sms-integration branch March 12, 2021 22:08
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.

3 participants