Skip to content

Add new remote params #707

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.

Sign up for GitHub

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

Jump to bottom
Merged
Jeasmine merged 1 commit into from
Sep 2, 2020
Merged

Add new remote params #707

Jeasmine merged 1 commit into from
Sep 2, 2020

Conversation

@Jeasmine
Copy link
Contributor

@Jeasmine Jeasmine commented Jul 6, 2020 •
edited by jwilm
Loading

  • Add locationShared remote param
  • Add requiresUserPrivacyConcent remote param
  • Keep public method
  • Add tests for new getters

This change is Reviewable

@Jeasmine Jeasmine force-pushed the feature/remote-params branch from 3a1005c to 14572a6 Compare July 22, 2020 15:46
@Jeasmine Jeasmine changed the base branch from master to major_release_3.0.0 July 22, 2020 15:46
@Jeasmine Jeasmine force-pushed the feature/remote-params branch from 14572a6 to 1e6c8b5 Compare July 22, 2020 15:47
mikechoch
mikechoch suggested changes Jul 28, 2020
View reviewed changes
@Jeasmine Jeasmine force-pushed the feature/remote-params branch 2 times, most recently from c18f863 to 9758106 Compare August 11, 2020 18:46
@Jeasmine Jeasmine requested review from emawby and jkasten2 August 12, 2020 18:05
mikechoch
mikechoch suggested changes Aug 14, 2020
View reviewed changes
mikechoch
mikechoch approved these changes Aug 14, 2020
View reviewed changes
Copy link
Contributor

@mikechoch mikechoch left a comment

Choose a reason for hiding this comment

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

LGTM, once the comment is resolved

@Jeasmine Jeasmine mentioned this pull request Aug 21, 2020
Merged
jkasten2
jkasten2 reviewed Aug 28, 2020
View reviewed changes
iOS_SDK/OneSignalSDK/Source/OneSignal.m Outdated
if (result[IOS_RECEIVE_RECEIPTS_ENABLE] != (id)[NSNull null])
[OneSignalUserDefaults.initShared saveBoolForKey:OSUD_RECEIVE_RECEIPTS_ENABLED withValue:[result[IOS_RECEIVE_RECEIPTS_ENABLE] boolValue]];

if (result[IOS_LOCATION_SHARED])
Copy link
Member

@jkasten2 jkasten2 Aug 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This doesn't match the logic we have for Android. We need to check for key presence here.
See this commit on Android:
https://github.com/OneSignal/OneSignal-Android-SDK/pull/1124/files#diff-8f1d87ed84bc5cc2e615dcfb826df87bR2522

And this comment for the remote param override logic we want for fallback support.
OneSignal/OneSignal-Android-SDK#1124 (review)

Lastly let's make sure a tests covers all 3 states of the remote param can be define. Not present, true, or false.

Copy link
Contributor Author

@Jeasmine Jeasmine Sep 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Added matched logic with Android, we will need the delayed task controller to match 100% the logic, on iOS we are not waiting for remote params due to this missing behavior

jkasten2
jkasten2 requested changes Aug 28, 2020
View reviewed changes
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.

Need to match Android logic for remote param fallback support for all params. See my comment above.

@Jeasmine Jeasmine changed the title Add new remote params WIP - Add new remote params Aug 31, 2020
@Jeasmine Jeasmine force-pushed the feature/remote-params branch 3 times, most recently from cd0cb4e to 3312868 Compare September 1, 2020 16:20
@Jeasmine Jeasmine changed the title WIP - Add new remote params Add new remote params Sep 1, 2020
@Jeasmine Jeasmine force-pushed the feature/remote-params branch from 3312868 to 94830c2 Compare September 1, 2020 17:13
@Jeasmine
Add new remote params
5e93fb5 
   * Add locationShared remote param
   * Add requiresUserPrivacyConcent remote param
   * Keep public method
   * Add tests for new getters
@Jeasmine Jeasmine force-pushed the feature/remote-params branch from 94830c2 to 5e93fb5 Compare September 1, 2020 17:54
@Jeasmine Jeasmine requested review from emawby and jkasten2 September 2, 2020 21:06
@Jeasmine Jeasmine merged commit c0ade33 into major_release_3.0.0 Sep 2, 2020
@Jeasmine Jeasmine deleted the feature/remote-params branch September 2, 2020 22:02
@nan-li nan-li mentioned this pull request Feb 13, 2025
Merged
18 tasks
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

@emawby emawby Awaiting requested review from emawby

+1 more reviewer

@mikechoch mikechoch mikechoch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Jeasmine @jkasten2 @emawby @mikechoch