-
Notifications
You must be signed in to change notification settings - Fork 376
Add toJSONObject method to OSInAppMessage
#1461
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
Conversation
4031da6 to
3efad9c
Compare
jkasten2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/OSInAppMessage.java
Outdated
Show resolved
Hide resolved
- Add this method for the wrappers to use - `OSInAppMessageInternal` extends `OSInAppMessage` so its `toJSONObject` method must be `public` because we can't override to reduce visibility
3efad9c to
3c59786
Compare
- Update JSON stringifier to use key of "messageId" instead of "id" - "id" is expected instead of "messageId" when parsing JSON from the backend, so it is used in the constructor for `OSInAppMessageInternal`
- some unit tests will create an IAM based off JSON of another IAM by calling toJSONObject() - toJSONObject uses key of "messageId" so we need to replace that with "id" for creating an IAM - added convertIAMtoJSONObject() that replaces "messageId" with "id" - refactored code to use this method instead of toJSONObject() - removed OSTestInAppMessageInternal constructor that took argument of OSInAppMessageInternal in favor of passing in JSON only
527c4b0 to
94c7523
Compare
- tests OSInAppMessageInternal (not OSInAppMessage) - only tests "messageId" as this test was added when modifying the JSON key from "id" to "messageId"
nan-li
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @Jeasmine and @jkasten2)
OneSignalSDK/onesignal/src/main/java/com/onesignal/OSInAppMessage.java, line 55 at r1 (raw file):
Previously, nan-li (Nan) wrote…
Ah ok! I have seen that practice but didn't realize it is preferred.
Done!
jkasten2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @emawby, @Jeasmine, and @rgomezp)
Description
One Line Summary
Add JSON stringifier to the public
OSInAppMessageclass and refactor JSON stringifier for internalOSInAppMessageInternalclass to return JSON key of"messageId"instead of"id".Details
Motivation
We want the public
OSInAppMessageto have atoJSONObjectmethod for the wrappers to use. This JSON Object only has one item of"messageId". Since IAMs have the propertymessageIdand our documentation refer tomessageId, this key was chosen over"id".The need for this method first arose when adding the InAppMessage Lifecycle Handler to React Native.
We also modified the
OSInAppMessageInternalclass'stoJSONObjectto return a JSON key of"messageId"instead of"id"for clarity and consistency to the public class.Background Context
Previously, the current
OSInAppMessageInternalclass was namedOSInAppMessagebefore being refactored into an internal InAppMessage class and a public InAppMessage class.Worth noting,
"id"is expected instead of"messageId"when parsing JSON from the backend, so it is used in the constructor.Scope
We didn't use any InAppMessage
toJSONObjectin SDK code, except in unit tests (see below) and those are removed now. The existingOSInAppMessageInternaltoJSONObjectmethod was package-private (made public in this PR) so users couldn't have used this method prior to this PR. This means changing the JSON key from"id"to"messageId"has no public API changes (but introduces a new method to the public).Currently, all of the
OSInAppMessageobjects are actuallyOSInAppMessageInternalobjects. We don't create any superclass OSInAppMessage objects in the existing codebase. This means when a user callsOSInAppMessage.toJSONObject(), they will receive the results ofOSInAppMessageInternal.toJSONObject().Testing
Unit testing
Some unit tests were creating an IAM based off the JSON of another IAM by calling its
toJSONObject().With the above changes, some tests in
InAppMessageIntegrationTestsbroke because IAM constructors expect"id"instead of"messageId"when parsing the JSON, so we need to replace that with"id"for creating an IAM.Added method:
convertIAMtoJSONObject(OSInAppMessageInternal inAppMessage)"messageId"with"id"toJSONObject()Removed a constructor:
OSTestInAppMessageInternal(OSInAppMessageInternal inAppMessage)inAppMessage.toJSONObject()convertIAMtoJSONObjectand pass this JSON into constructorAdded test:
testInAppMessageInternalToJSONObject_messageIdOSInAppMessageInternal(notOSInAppMessage)"messageId", the primary focusManual testing
✅ Tested by calling
OSInAppMessage.toJSONObject()in demo app:Example of the JSON
{ "messageId": "abe479ed-dc54-4446-898a-a82b67cfd5aa", "variants": {"all":{"default":"2a8934de-d3c1-4734-8b7c-796cd691456b"}}, "displayDuration": 0, "redisplay": {"limit":1,"delay":0},"triggers":[], "has_liquid": false }✅ Tested in react-native-onesignal via the InAppMessageLifecycleHandler by logging the
messageandmessage.messageIdmessageobject, they receive the entire JSON object above, not just themessageIdAffected code checklist
Checklist
Overview
Testing
Final pass
This change is