Add get tags #671

jinliu9508 · 2023-12-05T20:36:11Z

Description

One Line Summary

Add getTags that returns a map of key-value tags

Details

Motivation

Allow user to get access of tags

Affected code checklist

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
- WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is

shepherd-l

Will the iOS implementation of getTags be included in this PR?

I forgot to mention but, in the Unity SDK we manage a CHANGELOG. Could you update it with your change?

I recommend manually testing the getTags method on Android and iOS with the example app
You can also add a getTags button to the example app to make testing easier in the future

Nit: could you also update the Migration Guide to include the new method

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @jinliu9508)

OneSignalExample/Assets/StreamingAssets/SwiftPlugin.framework.meta line 0 at r1 (raw file):
What does deleting this file do?

jkasten2

Let's revert the changes to OneSignalExampleScene.unity in this PR, as it doesn't look related.

OneSignalExample/Assets/OneSignal/CHANGELOG.md

jinliu9508 · 2024-01-11T20:51:25Z

Let's revert the changes to OneSignalExampleScene.unity in this PR, as it doesn't look related.

I added a testing button "Get Tags" under the button "Remove Tag" in the example scene so tester can see what getTags() returns. This is a generated file so there are code changes that don't look relevant.

shepherd-l

Looks good! I have some minor nits
I tested GetTags on an emulated Pixel 4 with Android 12 and physical iPhone 12 with iOS 15.5 and the correct values were returned

Reviewed 3 of 4 files at r1, 4 of 11 files at r3, 4 of 8 files at r4.
Reviewable status: 11 of 18 files reviewed, 7 unresolved discussions (waiting on @jinliu9508, @jkasten2, and @nan-li)

com.onesignal.unity.android/Runtime/AndroidUserManager.cs line 57 at r4 (raw file):

            AndroidJavaObject obj = _user.Call<AndroidJavaObject>("getTags");
            Dictionary<string, string> dictionary = obj.MapToDictionary();
            UnityEngine.Debug.Log(dictionary.Count);

I think you forgot to remove a debug log, could you remove it?
Since the "dictionary" variable is only used for the log, you could also remove it or decide to return it instead

OneSignalExample/Assets/OneSignal/CHANGELOG.md line 19 at r4 (raw file):

## [5.0.5]
### Changed

I think we should also add that we updated the OneSignal Android and iOS SDK to highlight native changes

It should look something like this. You can also see the same comment in the Changelog history

Updated included Android SDK to [5.0.5](https://github.com/OneSignal/OneSignal-Android-SDK/releases/tag/5.0.5)
Updated included iOS SDK to [5.0.5](https://github.com/OneSignal/OneSignal-iOS-SDK/releases/tag/5.0.5)

OneSignalExample/Assets/OneSignal/MIGRATION_GUIDE_v3_to_v5.md line 177 at r4 (raw file):

| `void AddSms(string sms)`                             | *Add a new SMS subscription to the current user.*                                                                                                                                                                                        |
| `void RemoveSms(string sms)`                          | *Remove an SMS subscription from the current user.*                                                                                                                                                                                      |
| `Dictionary<string, string> GetTags()`                | *Return a copy of all tags for the current user. Tags are key:value pairs used as building blocks for targeting specific users and/or personalizing messages.*         |

I think we should also highlight that get tags returns the local tags on the user

OneSignalExample/Assets/OneSignal/Example/OneSignalExampleBehaviour.cs line 380 at r4 (raw file):

        Dictionary<string, string> dict = OneSignal.User.GetTags();
        string dictionaryString = "{";
        foreach(KeyValuePair < string, string > keyValues in dict) {

Nit: could you remove the extra spaces next to the "<" and ">" to look like <string, string>

OneSignalExample/Assets/OneSignal/Example/OneSignalExampleScene.unity line 11343 at r4 (raw file):

    m_VerticalOverflow: 0
    m_LineSpacing: 1
  m_Text: Get Tag

Nit but could you change the button to say "Get Tags" to match the GetTags method name

OneSignalExample/Assets/StreamingAssets/SwiftPlugin.framework.meta line at r1 (raw file):

Previously, shepherd-l (Shepherd) wrote…

What does deleting this file do?

Why was this file deleted? I don't think we need to delete this file

jinliu9508

I have committed a change addressing all of the concerns, please take a look again!

Reviewed 3 of 4 files at r1, 7 of 11 files at r3, 8 of 8 files at r4.
Reviewable status: 11 of 18 files reviewed, 7 unresolved discussions (waiting on @jkasten2, @nan-li, and @shepherd-l)

jinliu9508

Reviewable status: 11 of 18 files reviewed, 5 unresolved discussions (waiting on @jkasten2, @nan-li, and @shepherd-l)

com.onesignal.unity.android/Runtime/AndroidUserManager.cs line 57 at r4 (raw file):

Previously, shepherd-l (Shepherd) wrote…

I think you forgot to remove a debug log, could you remove it?
Since the "dictionary" variable is only used for the log, you could also remove it or decide to return it instead

Done.

OneSignalExample/Assets/OneSignal/CHANGELOG.md line 22 at r3 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

This should be under the "Changed" section instead of "Fixed".

Done.

OneSignalExample/Assets/OneSignal/CHANGELOG.md line 19 at r4 (raw file):

Previously, shepherd-l (Shepherd) wrote…

I think we should also add that we updated the OneSignal Android and iOS SDK to highlight native changes

It should look something like this. You can also see the same comment in the Changelog history

Updated included Android SDK to [5.0.5](https://github.com/OneSignal/OneSignal-Android-SDK/releases/tag/5.0.5)

Updated included iOS SDK to [5.0.5](https://github.com/OneSignal/OneSignal-iOS-SDK/releases/tag/5.0.5)

Done.

OneSignalExample/Assets/OneSignal/MIGRATION_GUIDE_v3_to_v5.md line 177 at r4 (raw file):

Previously, shepherd-l (Shepherd) wrote…

I think we should also highlight that get tags returns the local tags on the user

Done.

OneSignalExample/Assets/StreamingAssets/SwiftPlugin.framework.meta line at r1 (raw file):

Previously, shepherd-l (Shepherd) wrote…

Why was this file deleted? I don't think we need to delete this file

Done.

shepherd-l

Reviewable status: 11 of 18 files reviewed, 1 unresolved discussion (waiting on @jinliu9508, @jkasten2, and @nan-li)

Add public getTags() method that returns tags from the local user

jinliu9508 changed the base branch from main to user-model/main December 5, 2023 20:36

shepherd-l suggested changes Dec 5, 2023

View reviewed changes

nan-li self-requested a review December 18, 2023 18:41

jinliu9508 force-pushed the add-getTags branch from 1ce61f8 to 8361d64 Compare January 2, 2024 18:03

emawby assigned shepherd-l Jan 3, 2024

jkasten2 requested changes Jan 10, 2024

View reviewed changes

OneSignalExample/Assets/OneSignal/CHANGELOG.md Outdated Show resolved Hide resolved

Add public getTags

bd2c14b

jinliu9508 force-pushed the add-getTags branch from e991cbe to bd2c14b Compare January 11, 2024 20:48

jinliu9508 requested review from shepherd-l and jkasten2 January 11, 2024 20:52

shepherd-l approved these changes Jan 11, 2024

View reviewed changes

Fix some minor issues

bf84748

jinliu9508 commented Jan 11, 2024

View reviewed changes

jinliu9508 requested a review from shepherd-l January 11, 2024 22:49

shepherd-l approved these changes Jan 11, 2024

View reviewed changes

jinliu9508 merged commit a5b4f7e into user-model/main Jan 17, 2024
1 of 2 checks passed

jinliu9508 deleted the add-getTags branch January 17, 2024 17:35

jinliu9508 added a commit that referenced this pull request Feb 23, 2024

Merge pull request #671 from OneSignal/add-getTags

5000307

Add public getTags() method that returns tags from the local user

jinliu9508 added a commit that referenced this pull request Feb 27, 2024

Merge pull request #671 from OneSignal/add-getTags

d3f305c

Add public getTags() method that returns tags from the local user

jinliu9508 added a commit that referenced this pull request Feb 27, 2024

Merge pull request #671 from OneSignal/add-getTags

4956488

Add public getTags() method that returns tags from the local user

jinliu9508 added a commit that referenced this pull request Feb 27, 2024

Merge pull request #671 from OneSignal/add-getTags

fb59306

Add public getTags() method that returns tags from the local user

jinliu9508 added a commit that referenced this pull request Feb 27, 2024

Merge pull request #671 from OneSignal/add-getTags

aae4b19

Add public getTags() method that returns tags from the local user

jinliu9508 added a commit that referenced this pull request Mar 1, 2024

Merge pull request #671 from OneSignal/add-getTags

1e38883

Add public getTags() method that returns tags from the local user

jinliu9508 added a commit that referenced this pull request Mar 4, 2024

Merge pull request #671 from OneSignal/add-getTags

de2727f

Add public getTags() method that returns tags from the local user

jinliu9508 added a commit that referenced this pull request Mar 4, 2024

Merge pull request #671 from OneSignal/add-getTags

8eecad8

Add public getTags() method that returns tags from the local user

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add get tags #671

Add get tags #671

jinliu9508 commented Dec 5, 2023 •

edited

Loading

shepherd-l left a comment •

edited

Loading

jkasten2 left a comment

jinliu9508 commented Jan 11, 2024

shepherd-l left a comment

jinliu9508 left a comment

jinliu9508 left a comment

shepherd-l left a comment

Add get tags #671

Add get tags #671

Conversation

jinliu9508 commented Dec 5, 2023 • edited Loading

Description

One Line Summary

Details

Motivation

Affected code checklist

Checklist

Overview

Testing

Final pass

shepherd-l left a comment • edited Loading

Choose a reason for hiding this comment

jkasten2 left a comment

Choose a reason for hiding this comment

jinliu9508 commented Jan 11, 2024

shepherd-l left a comment

Choose a reason for hiding this comment

jinliu9508 left a comment

Choose a reason for hiding this comment

jinliu9508 left a comment

Choose a reason for hiding this comment

shepherd-l left a comment

Choose a reason for hiding this comment

jinliu9508 commented Dec 5, 2023 •

edited

Loading

shepherd-l left a comment •

edited

Loading