add new headers #165
add new headers #165
Conversation
| @@ -49,6 +49,7 @@ struct KlaviyoAPI { | |||
| requestFailed(request, error, 0.0) | |||
| return .failure(.internalRequestError(error)) | |||
| } | |||
| urlRequest.allHTTPHeaderFields?["X-Klaviyo-Retry-Attempt"] = "\(attemptNumber)/50" | |||
There was a problem hiding this comment.
maybe could use a constant here.
There was a problem hiding this comment.
Just to be super sure, this header should be formatted 1/50 -> 50/50 ?
There was a problem hiding this comment.
yah that was my understanding...could totally have missed that detail though!
There was a problem hiding this comment.
Makes sense to me, just want to be sure we don't accidentally start at 0 (or end at 49)
There was a problem hiding this comment.
Yah I'm going to test it through and ensure it does the desired thing.
There was a problem hiding this comment.
@ndurell Lets name this header X-Klaviyo-Attempt-Count then?
There was a problem hiding this comment.
I think we landed on
X-Klaviyo-Retry-Attempt
Earlier
There was a problem hiding this comment.
Yeah its a nitpick, but just responding to the above thread, that name is a little ambiguous: is 1/50 the first retry or first attempt.
There was a problem hiding this comment.
Starting from 1 now. Still doing some testing on my end but so far it's looking good.
Also, renamed header to X-Klaviyo-Attempt-Count
* mobile header indicating traffic coming from mobile sdk * retry attempt header indicating number of retries
ndurell
force-pushed
the
noah/CHNL-6823/new-headers-for-ios
branch
from
8223b86 to
9ddcfe8
Compare
|
For viz, the related android PR |
Done. Let me know if it looks good. |
| ▿ some: 1 key/value pair | ||
| ▿ (2 elements) | ||
| - key: "X-Klaviyo-Retry-Attempt" | ||
| - value: "0/50" |
There was a problem hiding this comment.
When an actual device sends, the first send will be 1/50, ya?
I had to do this in my tests too, just want to confirm.
There was a problem hiding this comment.
looks like he didn't update tests yet.
There was a problem hiding this comment.
Ah yeah I see now, it failed CI
There was a problem hiding this comment.
Yeah I tested this using a proxy, it starts at 1. I will update the tests.
Description
Check List
Manual Test Plan
Supporting Materials