klaviyo / klaviyo-swift-sdk

SDK that allows users to incorporate Klaviyo's event and person tracking functionality within iOS applications. Written in Swift.
MIT License
10 stars 10 forks source link

CHNL-6305: Fix set profile properties method #163

Closed ajaysubra closed 3 months ago

ajaysubra commented 3 months ago

Description

Check List

Manual Test Plan

  1. Set profile with most properties and then set profile properties and profile properties override the earlier stuff set with the profile.
  2. Setting profile properties first and then set profile should override the previously set profile properties.
  3. Tested w/o setting token and making sure profile request was successful.

Supporting Materials

ajaysubra commented 3 months ago

Makes sense with your annotations, two questions remain in my mind:

  • Can the snapshot tests for push token request reflect profile attributes, so that this important case is covered by tests
  • Does the event API suffer the same sort of problem? If I set profile attributes and created an event, would the profile attributes get sent to the backend appropriately?
  1. Yeah, I'm looking into writing unit tests, which is why I left the PR in the draft state. Sorry I should have mentioned that in the PR.
  2. That's a good question. Events is handled separately from profile attributes updates. What I mean by that is, as long as there is something in pendingProfile when flushing the queue we will make a profile/token request until pendingProfile is nil. Lines 247-249.

image

In order words, you could make an events request either before or after setting profile attributes, the event request will not have the profile attributes. Instead there will be a profile/token request that is made after the profile attributes are set. This is the current setup, there could be a world where this is captured in the events request and we can avoid a network call but I'll keep that out of the scope of this PR.

evan-masseau commented 3 months ago

Sorry, I didn't even notice this was a draft! I knew you'd said something about tests somewhere.
Sounds good about the events question, just wanted to be sure we weren't already merging profile changes into events calls too. Definitely not something I expected you to add now.