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: Migration from 16.x to 17.x (latest version) #185

Merged
merged 2 commits into from
Jan 25, 2024

Conversation

markvdouw
Copy link

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

Migration from 16.x to 17.x (latest version)
Reference: https://github.com/urbanairship/android-library/blob/17.0.0/documentation/migration/migration-guide-16-17.md

Testing Plan

  • Unit test pass
  • Regression testing (manual) - PENDING
  • {explain how this has been tested, and what, if any, additional testing should be done}

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

Initial work for: https://mparticle-eng.atlassian.net/browse/SQDSDKS-5988

Copy link
Contributor

@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.

Need Rob's questions answered then I'll take another look

@markvdouw markvdouw force-pushed the feat/migration_16_17 branch from 4e8bd7a to b4e241a Compare January 18, 2024 16:31
}
//The onChange fun within the AirshipChannelListener was removed.
//PushNotificationStatusListener reporting changes instead (ref. [#https://github.com/urbanairship/android-library/blob/17.0.0/documentation/migration/migration-guide-16-17.md])
airship.pushManager.addNotificationStatusListener { callChannelIdListener() }
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like all this is doing is setting the Airship channel ID as an attribute on mParticle. The channel in practice never changes but we do have a path in the SDK that we could regenerate it. If thats the case the onCreate will get called again.

I think all you need to do is sync on onAirshipReady and sync on channel created.

override fun onChannelCreated(s: String) {
callChannelIdListener()
override fun onChannelCreated(channelId: String) {

Copy link
Contributor

Choose a reason for hiding this comment

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

if the method isn't used anymore should we just remove it?

@@ -206,7 +202,7 @@ class UrbanAirshipKit : KitIntegration(), KitIntegration.PushListener, KitIntegr
.apply()
}
if (identityType == configuration?.userIdField) {
UAirship.shared().namedUser.id = null
UAirship.shared().contact.reset() // Previously setting namedUser to null but now is immutable
Copy link
Contributor

@rlepinski rlepinski Jan 18, 2024

Choose a reason for hiding this comment

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

This is basically the same thing as setting it null if the user is only managed through the SDK, but its possible that if the was set to a user server side this would now reset that association. I think it would be safer to only reset it if its not set through the SDK:

if (UAirship.shared().contact.namedUserId != null) {
    UAirship.shared().contact.reset()
}

@markvdouw
Copy link
Author

@rlepinski Is the change aligned with what you are recommending?

@@ -9,6 +9,7 @@ import com.urbanairship.AirshipConfigOptions
import com.urbanairship.Autopilot
import com.urbanairship.UAirship
import com.urbanairship.channel.AirshipChannelListener
import com.urbanairship.push.PushNotificationStatus
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is being used

@rlepinski
Copy link
Contributor

@markvdouw Yep!

@markvdouw markvdouw merged commit 7d7335b into development Jan 25, 2024
16 checks passed
@markvdouw markvdouw deleted the feat/migration_16_17 branch January 25, 2024 15:06
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.

4 participants