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

MOEN-34181: Initial Release which supports mParticle Swift API intigrations #1

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

MOEN-34181: Initial Release which supports mParticle Swift API intigrations #1

wants to merge 4 commits into from

Conversation

msoumya-engg-sdk
Copy link
Contributor

No description provided.

umangmoe
umangmoe requested changes Sep 19, 2024
View reviewed changes
Copy link
Collaborator

@umangmoe umangmoe left a comment

Choose a reason for hiding this comment

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

Some of the release and setup scripts are common with other repos. Please move them to the automation repo and use it from one place for easy maintenance.

Sources/mParticle-MoEngage/mParticle_MoEngage.swift
}

// MARK: User attributes and identities
extension MPKitMoEngage {
Copy link
Collaborator

Choose a reason for hiding this comment

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

for user attribute tracking we might want to validate the format. We support multiple formats and have a special handling for them.
Check how would that work here.

@msoumya-engg-sdk
Copy link
Contributor Author

Some of the release and setup scripts are common with other repos. Please move them to the automation repo and use it from one place for easy maintenance.

moved the scripts as part of https://github.com/moengage/sdk-automation-scripts/pull/4

for user attribute tracking we might want to validate the format. We support multiple formats and have a special handling for them.
Check how would that work here.

Added mapping for mParticle documented attributes.

@msoumya-engg-sdk msoumya-engg-sdk force-pushed the MOEN-34181_mPartcle branch 2 times, most recently from e69f4e9 to 89b7b54 Compare September 24, 2024 05:17
BrandonStalnaker
BrandonStalnaker approved these changes Oct 21, 2024
View reviewed changes
Copy link

@BrandonStalnaker BrandonStalnaker left a comment

Choose a reason for hiding this comment

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

I limited my review to the kit itself. Everything looks though I did have 1 question you might want to adress

Sources/mParticle-MoEngage/mParticle_MoEngage.swift Outdated

// WORKAROUND: for setATTStatus:withATTStatusTimestampMillis: callback not invoked
if let _ = request.userIdentities?[MPIdentity.iosAdvertiserId.rawValue as NSNumber] {
MoEngageSDKAnalytics.sharedInstance.enableIDFATracking(forAppID: settings.workspaceId)
Copy link

Choose a reason for hiding this comment

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

This would enable IDFATracking for any value of ATTStatus. Is that what you intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BrandonStalnaker during testing with older mParticle version we found a bug where IDFA tracking callback wasn't invoked correctly. After using the latest version, this is not needed anymore, hence removed it.

BrandonStalnaker
BrandonStalnaker approved these changes Oct 28, 2024
View reviewed changes
Copy link

@BrandonStalnaker BrandonStalnaker left a comment

Choose a reason for hiding this comment

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

The kit code itself looks good to me. I wouldn't commit with the broken CI (either remove or fix) but this is your repository so it should follow your standards.

@msoumya-engg-sdk
Copy link
Contributor Author

The kit code itself looks good to me. I wouldn't commit with the broken CI (either remove or fix) but this is your repository so it should follow your standards.

Thanks @BrandonStalnaker, regarding the failing CI: some of the Kit changes require new release of MoEngage-iOS-SDK hence failing now. Once that release is done and dependency version are updated CI will start succeeding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BrandonStalnaker BrandonStalnaker BrandonStalnaker approved these changes

@umangmoe umangmoe Awaiting requested review from umangmoe

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@msoumya-engg-sdk @umangmoe @BrandonStalnaker