-
Notifications
You must be signed in to change notification settings - Fork 38
PropositionItem class added along with unified track functionality #507
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
PropositionItem class added along with unified track functionality #507
Conversation
db46fb6 to
45eb54a
Compare
45eb54a to
8e5f825
Compare
a166752 to
1ef7a89
Compare
| import { useRouter } from 'expo-router'; | ||
|
|
||
| const SURFACES = ['android-cbe-preview', 'cbe/json', 'android-cc']; | ||
| const SURFACES = ['android-cbe-preview', 'android-cc', 'android-cc-naman']; |
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.
Are these surfaces to be kept in the main branch?
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.
removed
| // For class components, call initializeWithAppId inside componentDidMount. | ||
| MobileCore.setLogLevel(LogLevel.DEBUG); | ||
| MobileCore.initializeWithAppId("YOUR-APP-ID") | ||
| MobileCore.initializeWithAppId("3149c49c3910/4f6b2fbf2986/launch-7d78a5fd1de3-development") |
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.
Please remove the hardcoded appId
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.
done
f6708a6 to
77a0b2d
Compare
...id/src/main/java/com/adobe/marketing/mobile/reactnative/messaging/RCTAEPMessagingModule.java
Outdated
Show resolved
Hide resolved
...id/src/main/java/com/adobe/marketing/mobile/reactnative/messaging/RCTAEPMessagingModule.java
Show resolved
Hide resolved
| } | ||
| // Resolve PropositionItem by UUID | ||
| if (uuid == null) { | ||
| Log.d(TAG, "[MessagingBridge] Null uuid provided; no-op."); |
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.
Write better log message, Remove no-op
| for (Map.Entry<Surface, List<Proposition>> entry : propositionsMap.entrySet()) { | ||
| List<Proposition> propositions = entry.getValue(); | ||
| if (propositions == null) continue; | ||
| for (Proposition p : propositions) { |
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.
nit: rename p please
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.
done
|
|
||
| private final Map<String, Proposition> propositionItemByUuid = new ConcurrentHashMap<>(); | ||
|
|
||
| private String extractActivityId(Proposition proposition) { |
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.
nit: Write a comment (doc) over method. Like : Parsing proposition and extracting activity ID from it.
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.
added
| Map<String, Object> eventData = proposition.toEventData(); | ||
| if (eventData == null) return null; | ||
| Object scopeDetailsObj = eventData.get("scopeDetails"); | ||
| if (!(scopeDetailsObj instanceof Map)) return null; |
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.
We can add one more log here, like we did at bottom, in case of null.
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.
added
| try { | ||
| String key = extractActivityId(p); | ||
| if (key != null) { | ||
| propositionItemByUuid.put(key, p); |
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.
Why have we put, proposition here, instead of proposition item, and put that in comment in code also.
(When we are declaring this object)
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.
added comments for the same
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.
Please change the base branch of the PR from main. You can use this branch feat/messagingEnhancements-7.2.0
Please update the PR title as well.
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.
Have we marked deprecated to the old content card tracking API?
Make them deprecated or just remove that section in the Readme.
packages/messaging/README.md
Outdated
|
|
||
| ### Unified track | ||
|
|
||
| A unified tracking API is available for any proposition item (e.g., Content Cards, HTML, JSON, code-based items). Prefer calling `track` directly on the item. |
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.
I am not too clear about "Prefer calling track directly on the item." what are you comparing this to?
Are we going to deprecated the
Messaging.trackContentCardDisplay(proposition, contentCard);
Messaging.trackContentCardInteraction(proposition, contentCard);
How about Start with the API
PropositionItem.track
A unified tracking API is available for any proposition item (Content Cards, HTML, JSON, code-based items). You can use the same track() method regardless of content type, making your code more consistent and maintainable.
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.
added
packages/messaging/README.md
Outdated
| propositionItem.track(MessagingEdgeEventType.DISPLAY); | ||
|
|
||
| // Overload 2: interaction + event + optional tokens | ||
| propositionItem.track('content_card_clicked', MessagingEdgeEventType.INTERACT, null); |
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.
Remove Overload 2: international +...
// Track interaction with custom data
propositionItem.track('content_card_clicked', MessagingEdgeEventType.INTERACT, null);
// Track with tokens for sub-item tracking
propositionItem.track('button_click', MessagingEdgeEventType.INTERACT, ['token-1', 'token-2']);
}
}
}
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.
done
packages/messaging/README.md
Outdated
| if (msgProp.items.length > 0) { | ||
| const propositionItem = msgProp.items[0]; | ||
|
|
||
| // Overload 1: event only |
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.
Remove // Overload 1: event only
Use
// Track display event
(We have mentioned Overload 1 in the syntax, we can skip it in the example)
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.
done
| for (const surface of SURFACES_WITH_CONTENT_CARDS) { | ||
| const propositions = messages[surface] || []; | ||
|
|
||
| for (const proposition of propositions) { |
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.
If you plan to deprecate this method trackContentCardDisplay
let's just mark it and the we might also have to update the documentation
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.
yes have added in the typescript and also in documentation too
| * | ||
| * This mirrors the native Android PropositionItem class functionality. | ||
| */ | ||
| export class PropositionItem { |
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.
If possible, can we add few test cases on the rn side for the new API?
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.
added tests
cacheung
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.
just a minor format issue.
| const propositionItem = msgProp.items[0]; | ||
|
|
||
| // Track interaction with custom data | ||
| propositionItem.track('content_card_clicked', MessagingEdgeEventType.INTERACT, null); |
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.
nit: there is an extra space before
// Track ....
"propositionItem.track('content_card_clicked', MessagingEdgeEventType.INTERACT, null);"
They are not align with the next // Track ....
30ffaaf
into
feat/messagingEnhancements-7.2.0
* PropositionItem class added along with unified track functionality (#507) * proposition item classes, track funcitonality etc * proposition item uuid added * json proposition item removed * removed unused guide * added htmlProposition , jsonPropositon item class * removed unused apis of content card * generatePropositionInteractionXdm removed * ios implementation of track functionality * uuid vs proposition map added * MessagingProposition class fixes * get proposition for surfaces android fixes * ios get proposition for surface fixes * getproposition for surface updates * MessagingView commented code removed * removed proposition cache with parent * clear cache in update proposition * message util clean up * proposition Item tracking usage refactored * activityID extraction function reusable * white space fixes * added read me for track api usage * removed unnecessary checks before calling track api for android native * refactored extra null checks before calling track api at ios native side * Addeed ios and android logs for null cases * added comments and logs for android native code * marked the trackContentCardInteraction and trackContentCardDisplay as deprecated * added tests for propositionItem.track * read me updated for proposition Item track usage --------- Co-authored-by: Naman Arora <[email protected]> * add handleJavascriptMessage in AEPMessaging (#519) * Messaging - handle javascript message * add js event listener and ios * typecaste Message object * address PR comments * refactor log statements * address PR comments * revert env file id changes * clear js handler when onDismiss is called * messaging version bump for 7.2.0 release (#526) * typecaste to Message object (#527) * read me updates regarding deprecated apis --------- Co-authored-by: namArora3112 <[email protected]> Co-authored-by: Naman Arora <[email protected]>
Description
https://jira.corp.adobe.com/browse/MOB-23860
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: