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

Add public getTags method #1609

Merged
merged 5 commits into from
Dec 5, 2023
Merged

Add public getTags method #1609

merged 5 commits into from
Dec 5, 2023

Conversation

jennantilla
Copy link
Contributor

@jennantilla jennantilla commented Dec 4, 2023

Description

One Line Summary

Add public OneSignal.User.getTags() method to return local tags for the current user.

Details

Add getTags public method that will return local tags for the current user on both Android and iOS.

Motivation

We removed the getTags method in v5 of the SDKs to discourage use of OneSignal tags as a data store. We have heard feedback from app developers that they have a need for this method still, so we are adding back an implementation to return the local tags.

Scope

Keep note it returns the local tags and not via a server fetch so any tags added mid-session through the REST API will not be reflected.

Testing

Unit testing

None

Manual testing

Successfully call new method getTags on iPhone 15 emulator running iOS 17.0.1 and Pixel 7 running Android 14.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • 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.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

@nan-li nan-li self-requested a review December 5, 2023 02:54
The try/catch block is unnecessary since creating a new JSONObject from a Map will not throw a JSONException.
@jennantilla jennantilla requested a review from nan-li December 5, 2023 04:11
@jennantilla jennantilla merged commit 19d5950 into major_release_5.0.0 Dec 5, 2023
4 checks passed
@jennantilla jennantilla mentioned this pull request Dec 5, 2023
@rgomezp rgomezp deleted the feature/getTags branch June 15, 2024 07:31
@rgomezp
Copy link
Contributor

rgomezp commented Jun 15, 2024

@emawby @nan-li @jennantilla why is getTags asynchronous here? This is not consistent with our other SDKs and with the description "returns local tags" https://documentation.onesignal.com/docs/mobile-sdk-reference#gettags

@nan-li
Copy link
Contributor

nan-li commented Jun 16, 2024

@rgomezp The call needs to go over the bridge to call the native function and receive the result. It's not consistent with the native SDKs since they don't need to do this.

@rgomezp
Copy link
Contributor

rgomezp commented Jun 17, 2024

@nan-li I forgot about that limitation. Thanks!

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