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 tag type fix #1696

Merged
merged 6 commits into from May 1, 2024
Merged

Add tag type fix #1696

merged 6 commits into from May 1, 2024

Conversation

dombartenope
Copy link

@dombartenope dombartenope commented Apr 22, 2024

Description

One Line Summary

This PR fixes an issue where addTag will set an empty string, deleting existing keys, if a number is set as the value. Previously this value was of type 'string', and it will now booleans and numbers by casting the value to a string before sending the request to the OneSignal sever.

Details

Motivation

Reports of crashes due to sending unix timestamps as integers, when added as a value to the addTag method.

On the current major release / latest update, if passing in a unix timestamp as the value argument to the addTags method, you will see an empty string is sent as the value when making a change. This would delete a data tag per other SDK logic that determines an empty value to be the same as a "remove" request.

Scope

OPTIONAL - Other

The changes to the RTCOneSignalEventEmitter, are to determine whether the type is an NSNumber or not. The changes made to index.ts were for intellisense (allowing the boolean and number types to be used without an error thrown by the IDE) and to set the value to a string in case the value was a boolean (which would default to 1 or 0 otherwise).

Testing

Manual testing

I have tested this on both Android and iOS devices:
image
image

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
    • 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 Reviewable

@jennantilla
Copy link
Contributor

Hi @dombartenope thank you so much for this PR!

My observations of this bug and how behavior varied by platform:

addTag:

  • iOS: replaced int value with empty string (thereby deleting the previous tag value)
  • Android: crashed

addTags:

  • iOS: crashed
  • Android: did not add tag with int value but added tag with string value

Since all the SDKs require a string value parameter for tagging methods, I've reverted the addTag function signature. If the developer is using Typescript, they should get a warning that they should only pass in a string value. However, we'll ensure that no sneaky non-string values get to the bridge by converting them within the RN methods.

Let me know if you have any questions or concerns!

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
- ensure consistency of string conversion methods
- check for null or undefined values instead of falsy checks
@jennantilla jennantilla requested a review from nan-li May 1, 2024 21:39
@jennantilla jennantilla merged commit b6eb761 into OneSignal:main May 1, 2024
4 checks passed
@jennantilla jennantilla mentioned this pull request May 1, 2024
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.

None yet

3 participants