Supports setOnce #51
Supports setOnce #51
Conversation
|
By analyzing the blame information on this pull request, we identified and undefined to be a potential reviewer. |
| @@ -21,6 +21,10 @@ - (id)initWithSettings:(NSDictionary *)settings andAmplitude:(Amplitude *)amplit | |||
| self.traitsToIncrement = [NSSet setWithArray:self.settings[@"traitsToIncrement"]]; | |||
| } | |||
|
|
|||
| if (self.settings[@"traitsToSetOnce"] != (id)[NSNull null]) { | |||
There was a problem hiding this comment.
Do we need the cast here? Also does this handle nil?
There was a problem hiding this comment.
I thought we determined that settings.traitsToSetOnce could be @[] or null, not nil
There was a problem hiding this comment.
But does this check handle all those cases? It seems to me it would only handle the null case.
| @@ -73,7 +77,7 @@ - (void)identify:(SEGIdentifyPayload *)payload | |||
| [self.amplitude setUserId:payload.userId]; | |||
| SEGLog(@"[Amplitude setUserId:%@]", payload.userId); | |||
|
|
|||
| if ([self.traitsToIncrement count] > 0) { | |||
| if ([self.traitsToIncrement count] > 0 || [self.traitsToSetOnce count] > 0) { | |||
There was a problem hiding this comment.
Maybe we should consider removing the else block (not in this PR but as a follow up) as this block will do the same thing if these settings are both empty.
There was a problem hiding this comment.
Hmm but I don't want to return within this block so that it checks groups. I can move groups up in another PR and then return out of this block and remove the else
There was a problem hiding this comment.
Hmmm, I think this is wrong. This going to call incrementOrSetTraits and setUserProperties. We don't want that. What I was I saying is I think we don't need to have this check, and don't need setUserProperties at all, the incrementOrSetTraits on it's own will handle correctly (no early return here).
Either way let's go back to what you had before and make the change in a follow up PR.
ladanazita
force-pushed
the
adds/setOnce
branch
from
a97c935
to
6e94ca6
Compare
setOncesets the value of a user property only once. SubsequentsetOnceoperations on that user property will be ignored.Checks
traitsagainst values insettings.traitsToSetOnce