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

feat: Remove the existing code and added new method 'getUserAudience' #542

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

Mansi-mParticle
Copy link
Collaborator

Instructions

  1. PR target branch should be against development
  2. PR title name should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-title-check.yml
  3. PR branch prefix should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-branch-check-name.yml

Summary

Testing Plan

  • Was this tested locally? If not, explain why.
  • Verified through compilation and execution of a local application.

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

@Mansi-mParticle Mansi-mParticle changed the title feat: Remove the existing code and added new method 'getUserAudience' -cherry-pick feat: Remove the existing code and added new method 'getUserAudience' Feb 11, 2025
@Mansi-mParticle Mansi-mParticle force-pushed the feat/6131-audience-api-android-cherry-pick branch from a4374af to 89242e9 Compare February 13, 2025 17:15
Copy link
Member

@rmi22186 rmi22186 left a comment

Choose a reason for hiding this comment

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

Minor comments and questions.

Comment on lines 105 to +107
protected MPUrl getUrl(Endpoint endpoint, @Nullable String identityPath) throws MalformedURLException {
return getUrl(endpoint, identityPath, false);
return getUrl(endpoint, identityPath, false,null);
}
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it's new relative to the previous PR's getUrl changes here

can you clarify why? if my memory serves, i recall you discussing refactoring this after it being a pretty confusing section of the code for getting the URL , right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This method is specifically for the Identity API. In this case, Audience parameter should be set to null.

@rmi22186
Copy link
Member

take a look at these as well -
image

Is there anything major?

Copy link
Member

@rmi22186 rmi22186 left a comment

Choose a reason for hiding this comment

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

seems lke a few comments were marked as resolved, but i think they still need updating.

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.

2 participants