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

CHNL-6305: Added set profile attributes bridge method #135

Merged
merged 5 commits into from
Mar 15, 2024

Conversation

ajaysubra
Copy link
Collaborator

Description

During the initial release of our beta react native SDK, we somehow missed bridging methods for setProfileAttributes on iOS. This PR aims to add those missing bridging methods.

Check List

  • Are you changing anything with the public API?
  • Are your changes backwards compatible with previous SDK Versions?

Changelog / Code Overview

Test Plan

Performed manual testing using different profile property attributes such as latitude, longitude, city etc.

Related Issues/Tickets

@ajaysubra ajaysubra requested a review from a team as a code owner March 13, 2024 17:28
@ajaysubra ajaysubra requested review from evan-masseau and removed request for a team March 13, 2024 17:28
@@ -1379,7 +1379,7 @@ SPEC CHECKSUMS:
React-utils: debda2c206770ee2785bdebb7f16d8db9f18838a
ReactCommon: ddb128564dcbfa0287d3d1a2d10f8c7457c971f6
SocketRocket: f32cd54efbe0f095c4d7594881e52619cfe80b17
Yoga: 2b33a7ac96c58cdaa7b810948fc6a2a76ed2d108
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure why this changed 🤷🏽

@@ -97,7 +97,9 @@ export const setPushToken = async () => {

export const setProfileAttribute = async () => {
try {
Klaviyo.setProfileAttribute('CUSTOM', generateRandomName(12));
Klaviyo.setProfileAttribute(ProfileProperty.CITY, generateRandomName(5));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added a couple properties outside of the custom one so that manual testing is easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.
Yeah I'd love to update this sample app someday to have an input form here instead of just generating random data.

@@ -82,6 +82,11 @@ public class KlaviyoBridge: NSObject {
KlaviyoSDK().set(profile: profile)
}

@objc
public static func setProfileAttribute(_ key: String, value: String) {
KlaviyoSDK().set(profileAttribute: getProfileKey(key), value: value)
Copy link
Contributor

@evan-masseau evan-masseau Mar 13, 2024

Choose a reason for hiding this comment

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

Doesn't really matter, but in the Android code we just used CUSTOM for all incoming keys 🤷


    @ReactMethod
    override fun setProfileAttribute(
      propertyKey: String,
      value: String,
    ) {
      Klaviyo.setProfileAttribute(ProfileKey.CUSTOM(propertyKey), value)
    }

Copy link
Contributor

@evan-masseau evan-masseau left a comment

Choose a reason for hiding this comment

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

LGTM

@evan-masseau
Copy link
Contributor

evan-masseau commented Mar 13, 2024

iOS CI failed because of cached cocoapods mismatching with that lockfile change. I still don't understand why it is using cache if the lockfile changed. But I set off a re-run, which should ignore cache, lets see!
Either way, I wouldn't hold up merging this due to the CI failure!

EDIT: NVM the re-run succeeded

@ajaysubra ajaysubra merged commit c281cbf into master Mar 15, 2024
4 of 5 checks passed
@ajaysubra ajaysubra deleted the as/chnl-6305-set-profile-attributes branch March 15, 2024 15:39
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.

3 participants