[BRANDCON-8316] Instrument analytics for BC in-chat navigation behavior - #6579
[BRANDCON-8316] Instrument analytics for BC in-chat navigation behavior#6579loganzartman wants to merge 13 commits into
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch.
|
1896144 to
41457e9
Compare
41457e9 to
b42c6a5
Compare
| onEvent: (event) => { | ||
| if (event.eventType === 'card:clicked') recordBcNavClick('product_card_cta', event.data?.element?.productPageURL); | ||
| else if (event.eventType === 'cta:clicked') recordBcNavClick('cta', event.data?.element?.productPageURL); | ||
| else if (event.eventType === 'link:clicked') recordBcNavClick(event.data?.element?.linkType ?? 'inline_hyperlink', event.data?.element?.href); |
There was a problem hiding this comment.
new event type; pending acom-brand-concierge-web-client PR
Covers getChatSessionId cookie parsing and the bcAnalytics navigation:backNavigation payload (event name + required fields). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JasonHowellSlavin
left a comment
There was a problem hiding this comment.
LGTM. I might review the comments, we tend to try and be as comment free in our code in Milo.
The only thing staying approval on my end is that everything done here should also be done in /libs/c2/blocks/brand-concierge
| if (event.eventType === 'card:clicked') recordBcNavClick('product_card_cta', event.data?.element?.productPageURL); | ||
| else if (event.eventType === 'cta:clicked') recordBcNavClick('cta', event.data?.element?.productPageURL); | ||
| else if (event.eventType === 'link:clicked') recordBcNavClick(event.data?.element?.linkType ?? 'inline_hyperlink', event.data?.element?.href); |
There was a problem hiding this comment.
Seems like a decent spot to use a switch case.
|
|
||
| setCssGnavHeight(); | ||
|
|
||
| const emitAnalyticsIfBackNav = () => { |
There was a problem hiding this comment.
@loganzartman Can this be moved to bc-analytics.js? We recently did a massive refactor to cut down on extraneous stuff in the base block code and this being an analytics specific function, it seems a good fit to move there.
Also, there is a brand-concierge-global block that will likely need this same treatment. It imports from the base brand-concierge block, so moving to bc-analytics will be helpful for that block also:
There was a problem hiding this comment.
yeah, that's a good idea. I've pulled everything into bc-analytics with a single initAnalytics hook for the block. does that seem good?
There was a problem hiding this comment.
Seems ok, there are other analytics elements to this that are invoked when the web client is bootstrapped and as long as the rest of it still works, then this looks great.
Again, though, we should add this to the brand-concierge-global block (the one that is used for gnav as seen on bacom stage here so it gets this goodness as well:
There was a problem hiding this comment.
sorry, I misread your first comment. I've added the init call to brand-concierge-global as well. I didn't realize these were both the brand concierge block.
Thread the widget's cta:clicked `source` enum through history state into the navigation:backNavigation payload, so back-nav events are attributable to the CTA surface. Use an options object for recordNavClick. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@loganzartman So we are trying to do some testing on the milo code contributions here, but the name format of the source branch is making it impossible to test, specifically the "/" and caps in the branch name. If we rename so we can test it, it will close the PR. For context, we might ordinarily test something like this using one of two methods: An AEM url, using this pattern: https://[branch-name]--milo--adobecom.aem.page/[path to test page] Neither of these methods will work with the current branch name format. As an alternative to renaming and hence closing your pr, you can push this same set of changes to a new branch, such as a simple: brandcon-8316 That way we could test your proposed changes using that other branch name without closing this PR. |
|
@loganzartman do we need to wait for https://github.com/adobe-brand-concierge/acom-brand-concierge-web-agent/pull/247 get merging before do any validation? and if you can give some user cases to check, what will be better |
@JackySun9 I've asked Analysts to validate this, will let you know once we have a go ahead |
|
Reminder to set the |
|
This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR. |
adds analytics for when users navigate back from an outgoing same-tab link.
navigation:backNavigationanalytics event type matching spec in https://jira.corp.adobe.com/browse/BRANDCON-8316pageshow), and non-cached back/forward (via performance entries check)onEventto record link information in browser history entrydepends on web client PR: https://github.com/adobe-brand-concierge/acom-brand-concierge-web-agent/pull/247
but merge/deploy order is not important
Resolves: BRANDCON-8316
Test URLs:
sample payloads
CTA click:
{ "eventType": "navigation:backNavigation", "data": { "clickType": "product_card_cta", "sessionId": "e973c3ad-1a5a-4b01-bd9f-765d5b58d4e1", "sourcePage": "http://localhost:6456/drafts/nala/blocks/brand-concierge/brand-concierge", "destinationPage": "https://business.adobe.com/products/brand-visibility.html", "loginStatus": "logged-in", "navigatedBack": true } }Inline hyperlink:
{ "eventType": "navigation:backNavigation", "data": { "clickType": "inline_hyperlink", "sessionId": "e973c3ad-1a5a-4b01-bd9f-765d5b58d4e1", "sourcePage": "http://localhost:6456/drafts/nala/blocks/brand-concierge/brand-concierge", "destinationPage": "https://business.adobe.com/products/brand-visibility.html", "loginStatus": "logged-in", "navigatedBack": true } }