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

fix: anonymousId overwriting userId at MoEngage destination #3914

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

jmlandi
Copy link

@jmlandi jmlandi commented Dec 5, 2024

What are the changes introduced in this PR?

Changed the preference for userId over anonymousId for MoEngage destination.

What is the related Linear task?

No related Linear task for this change.

Please explain the objectives of your changes below

Any changes to existing capabilities/behavior? Mention the reason & what changes were made.

This change adjusts MoEngage destination behavior to prefer userId over anonymousId when both are provided.
Currently, when both userId and anonymousId are present, the anonymous ID overwrites the user ID, leading to unexpected behavior, such as users receiving an anonymousId from a web session that overrides the main user identification.

For more details, see the opened issue #3892.

Any new dependencies introduced with this change?

No.

Any new generic utility introduced or modified? Please explain the changes.

No.

Any technical or performance-related considerations with this change?

No.

@coderabbitai review


Developer checklist

  • My code follows the style guidelines of this project.
  • No breaking changes are being introduced.
  • All related docs linked with the PR.
  • All changes manually tested.
  • Any documentation changes needed with this change.
  • Is the PR limited to 10 file changes?
  • Is the PR limited to one Linear task?
  • Are relevant unit and component test cases added in new readability format?

Reviewer checklist

  • Is the type of change in the PR title appropriate as per the changes?
  • Verified that there are no credentials or confidential data exposed with the changes.

@jmlandi jmlandi requested a review from a team as a code owner December 5, 2024 02:26
@contributor-support
Copy link

Thank you @jmlandi for contributing this PR.
Please sign the Contributor License Agreement (CLA) before merging.

@gitcommitshow
Copy link
Collaborator

gitcommitshow commented Dec 5, 2024

@jmlandi without this change, did you see any issue in the event you receive at the destination.
Can you please share the transformed event sample you see (you may use the RudderStack live debugger for MoEngage destination to get it)?

The reason I am asking for this is because engineering team tested the current MoEngage destination (without this PR change) and it worked as expected.

This is what I learned from the engineering team

The variable you changed acts only as a placeholder for the data transformer we send back to server which has no real value, this property is not even sent to destination.
The actual payload is the payload variable you need to follow this

const payload = constructPayload(message, MAPPING_CONFIG[category.name]);

And the order by which the customer_id is filled is this order
You can use the identify API to identify a user in MoEngage. You can also create a new user property or update existing user properties using the identify call.
MoEngage needs a unique identifier to identify a user. So, you must provide a userId in your identify call that RudderStack passes as the MoEngage customer_id . If userId is absent, RudderStack sends the anonymousId field instead.
So our implementation is in accordance to our docs.
So can you please confirm if the destination is not working as documented? If not then we have to debug

Expanding more on why the current code works. This is how the response object looks like (which is an instruction for RudderStack data plane internal service to make the appropriate request to the destination).

{
                "version": "1",
                "type": "REST",
                "method": "POST",
                "endpoint": "https://api-01.moengage.com/v1/customer/1231",
                "headers": {
                    "Content-Type": "application/json",
                    "MOE-APPKEY": "1231",
                    "Authorization": "Basic Auth"
                },
                "params": {},
                "body": {
                    "JSON": {
                        "customer_id": "random_anonId", // ---------> This gets sent to MoEngage and already set correctly i.e. userId || anon_Id
                        "type": "customer",
                        "attributes": {
                            "first_name": "patti",
                            "last_name": "jackson",
                            "email": "[email protected]",
                            "approved_reservation_count": 0,
                            "is_host": "false",
                            "is_renter": "true",
                            "phone": "18172292269",
                            "phone_verified": "true",
                            "reservation_count": 1,
                            "cu_date_time": "2024-11-12T16:30:53.728Z",
                            "ucid": "2024-11-12T16:30:53.728Z",
                            "cu_date": "2024-11-12T16:30:53.728Z"
                        }
                    },
                    "JSON_ARRAY": {},
                    "XML": {},
                    "FORM": {}
                },
                "files": {},
                "userId": "random_anonId" // --------------> This data is irrelevant to MoEngage (the PR changes this property)
            }

The response.body.JSON is what gets sent as the payload to the final destination. This PR changed the response.userId, not the response.body.JSON.customer_id which gets sent to the destination. And that property seems to be set correctly userId || anon_id already.

Let me know if you have any question or suggestion to further improve this code

@jmlandi
Copy link
Author

jmlandi commented Dec 5, 2024

@gitcommitshow, thank you for your response. I understand that the customer_id is the identifier referred to in MoEngage's Create Event Documentation. However, we are still encountering cases where users are being registered with the anonymousId instead of the userId. I am also reaching out to MoEngage for further investigation on their side.

That said, the userId in the message seems to be overwritten, which I believe is not the expected behavior—even if it doesn't fully solve my issue. Could you confirm if this is correct? Here is evidence from a recent event.

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.25%. Comparing base (0965f30) to head (16b9652).
Report is 6 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3914      +/-   ##
===========================================
- Coverage    90.48%   90.25%   -0.24%     
===========================================
  Files          615      620       +5     
  Lines        32359    32518     +159     
  Branches      7687     7712      +25     
===========================================
+ Hits         29281    29350      +69     
- Misses        2822     2903      +81     
- Partials       256      265       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gitcommitshow
Copy link
Collaborator

gitcommitshow commented Dec 6, 2024

Thanks for sharing the details @jmlandi.

That said, the userId in the message seems to be overwritten, which I believe is not the expected behavior—even if it doesn't fully solve my issue. Could you confirm if this is correct?

Yes, that is correct. So we will go ahead with this PR as a fix for this different issue

And we will separately investigate that "users being registered with the anonymousId" issue, I believe you're already in touch with your TAM and they have taken up that issue. You may continue discussion with them.

@gitcommitshow
Copy link
Collaborator

@jmlandi you may delete the attached image in your comment now

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.

6 participants