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.

Sign up for GitHub

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

Jump to bottom

Facebook Pixel event_id discrepancy #486

Merged
merged 6 commits into from
Apr 5, 2022
Merged

Facebook Pixel event_id discrepancy #486

merged 6 commits into from
Apr 5, 2022

Conversation

rohithkaza
Copy link
Contributor

@rohithkaza rohithkaza commented Apr 4, 2022 •
edited by SumanthPuram
Loading

Description of the change

Assigning event_id in device mode as per cloud mode.

Type of change

  • Bug fix (non-breaking change that fixes an issue)

Related issues

Fix #1

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers mentioned in a comment
  • Changes have been reviewed by at least one other engineer
  • Issue from task tracker has a link to this pull request

This change is Reviewable

@rohithkaza rohithkaza self-assigned this Apr 4, 2022
github-actions[bot]
github-actions bot reviewed Apr 4, 2022
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size-limit report

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/rudder-analytics.min.js 35.65 KB (0%) 714 ms (0%) 329 ms (+6.23% 🔺) 1.1 s

@saikumarrs
Copy link
Member

The branch should be created off of the production-staging branch and PR also needs to be raised against the same.

@rohithkaza rohithkaza changed the base branch from production to production-staging April 4, 2022 14:22
@rohithkaza rohithkaza requested a review from utsabc April 4, 2022 14:44
@rohithkaza rohithkaza added the Ready for review Changes are ready to be reviewed label Apr 4, 2022
utsabc
utsabc reviewed Apr 4, 2022
View reviewed changes
integrations/FacebookPixel/browser.js Outdated
@@ -310,7 +310,7 @@ class FacebookPixel {
: this.formatRevenue(price),
},
{
eventID: messageId,
event_id: traits.event_id || context.traits.event_id || properties.event_id || messageId,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than doing this multiple time can we use an util function to achieve this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@rohithkaza rohithkaza changed the base branch from production-staging to v1-staging April 4, 2022 16:07
utsabc
utsabc reviewed Apr 4, 2022
View reviewed changes
utils/getEventId.js Outdated
@@ -0,0 +1,4 @@
function getEventId(message){
return traits.event_id || context.traits.event_id || properties.event_id || messageId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

message is passed but traits.event_id is referenced, but we are not extracting traits hence traits, context, properties and messageId will always be undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Committed new changes, please check once

akashrpo
akashrpo reviewed Apr 5, 2022
View reviewed changes
integrations/FacebookPixel/browser.js Outdated
@@ -73,7 +74,7 @@ class FacebookPixel {

page(rudderElement) {
const { properties, messageId } = rudderElement.message;
window.fbq("track", "PageView", properties, { eventID: messageId });
window.fbq("track", "PageView", properties, { getEventId(message) });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be { event_id: getEventId(message) } ?

Suggested change
window.fbq("track", "PageView", properties, { getEventId(message) });
window.fbq("track", "PageView", properties, { event_id: getEventId(message) });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

utsabc
utsabc reviewed Apr 5, 2022
View reviewed changes
integrations/FacebookPixel/browser.js Outdated
@@ -73,7 +74,7 @@ class FacebookPixel {

page(rudderElement) {
const { properties, messageId } = rudderElement.message;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const { properties, messageId } = rudderElement.message;
const { properties } = rudderElement.message;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

utsabc
utsabc reviewed Apr 5, 2022
View reviewed changes
utils/getEventId.js Outdated
@@ -0,0 +1,4 @@
function getEventId(message){
return message.traits.event_id || message.context.traits.event_id || message.properties.event_id || message.messageId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this file to FacebookPixel folder as its specif to its use case. And rename this file as util.js

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

rohithkaza and others added 2 commits April 5, 2022 12:34
@rohithkaza @utsabc
Update integrations/FacebookPixel/browser.js
bb3770d 
Co-authored-by: Utsab Chowdhury <utsab.c97@gmail.com>
@rohithkaza
updated util.js file path
48ecef1
@rohithkaza rohithkaza merged commit fb68d30 into v1-staging Apr 5, 2022
@rohithkaza rohithkaza deleted the fbpixel-eventid-error branch April 5, 2022 12:49
@rohithkaza rohithkaza removed the Ready for review Changes are ready to be reviewed label Aug 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@saikumarrs saikumarrs saikumarrs approved these changes

@utsabc utsabc utsabc approved these changes

@MoumitaM MoumitaM Awaiting requested review from MoumitaM

@ItsSudip ItsSudip Awaiting requested review from ItsSudip

@akashrpo akashrpo Awaiting requested review from akashrpo

Assignees

@rohithkaza rohithkaza

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rohithkaza @saikumarrs @utsabc @akashrpo