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

Request sender improvements #404

Merged
merged 10 commits into from Oct 22, 2020
Merged

Conversation

dejan2k
Copy link
Contributor

@dejan2k dejan2k commented Oct 1, 2020

No description provided.

Unit tests updated
# Conflicts:
#	Leanplum-SDK/Classes/Managers/Networking/LPFileTransferManager.m
#	Leanplum-SDK/Classes/Managers/Networking/LPRequestSender.h
#	Leanplum-SDK/Classes/Managers/Networking/LPRequestSender.m
* Create Request Headers for network call
*/
+ (NSDictionary *)createHeaders;
+ (NSDictionary *)notificationSettingsToRequestParams:(NSDictionary *)settings;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be moved to LPPushNotificationManager/Handler instead here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is only for the way those are sent in a request, I believe it should be here

@@ -1110,22 +1111,12 @@ + (void)startWithUserId:(NSString *)userId
BOOL exitOnSuspend = [[[[NSBundle mainBundle] infoDictionary]
objectForKey:@"UIApplicationExitsOnSuspend"] boolValue];
LPRequest *request = [LPRequestFactory stopWithParams:nil];
[[LPRequestSender sharedInstance] sendIfConnected:request sync:exitOnSuspend];
request.sendSynchronously = exitOnSuspend;
Copy link
Contributor

Choose a reason for hiding this comment

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

Get rid of sendSyncronously flag

@@ -72,7 +72,9 @@ + (void)uploadAppIconsOnDevMode
[request onError:^(NSError *error) {
LPLog(LPError, @"Fail to upload app icons: %@", error.localizedDescription);
}];
[[LPRequestSender sharedInstance] sendNow:request withDatas:requestDatas];
request.datas = requestDatas;
request.requestType = Immediate;
Copy link
Contributor

Choose a reason for hiding this comment

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

Move requestType to be as part of the builder pattern instead setting it like this

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, it will make it more clear if the request will be batched or send immediately.

@property (atomic) BOOL sent;
@property (nonatomic, copy, nullable) LPNetworkResponseBlock responseBlock;
@property (nonatomic, copy, nullable) LPNetworkErrorBlock errorBlock;
@property (nonatomic, strong) NSString *requestId;
@property (nonatomic, assign) LPRequestType requestType;
@property (nonatomic, assign) BOOL sendSynchronously;
Copy link
Contributor

Choose a reason for hiding this comment

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

Get rid of sendSynchronously

return true;
}

- (void)sendNow:(LPRequest *)request sync:(BOOL)sync
Copy link
Contributor

Choose a reason for hiding this comment

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

Please get rid off sync flag from all methods and always execute on LPOperationQueue

[self sendNow:request withDatas:@{key: data}];

[self.countAggregator incrementCount:@"send_now_with_data_lp"];
[self sendNow:request sync:request.sendSynchronously];
Copy link
Contributor

Choose a reason for hiding this comment

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

Please get rid off sync flag from all methods and always execute on LPOperationQueue

# Conflicts:
#	Example/Leanplum-SDK.xcodeproj/project.pbxproj
@dejan2k dejan2k merged commit 4ae1372 into master Oct 22, 2020
@delete-merged-branch delete-merged-branch bot deleted the SDK-51_request_sender_improvement branch October 22, 2020 10:48
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.

None yet

3 participants