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

Feature/set locale #467

Merged
merged 5 commits into from
Dec 20, 2021
Merged

Feature/set locale #467

merged 5 commits into from
Dec 20, 2021

Conversation

fitja
Copy link
Contributor

@fitja fitja commented Dec 2, 2021

Proposed Changes

Currently the locale parameter is sent along when calling start(), but there is no way to override it.

This PR introduces setLocale API which can be called before or after start().
The Leanplum class now have private property for overriding the system locale.

Similar approach has been done in Leanplum/Leanplum-JavaScript-SDK#140

/**
* Tests setting the user locale after Leanplum start
*/
- (void) test_set_locale
Copy link
Contributor

Choose a reason for hiding this comment

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

You can simplify the test to:

- (void) test_set_locale
{
    NSLocale *locale = [[NSLocale alloc] initWithLocaleIdentifier:@"ab_CD"];
    
    XCTestExpectation *request_expectation =
    [self expectationWithDescription:@"request_expectation"];
    [LPRequestSender validate_request_args_dictionary:^(NSDictionary *args) {
        XCTAssertTrue([args[LP_KEY_LOCALE] isEqualToString:[locale localeIdentifier]]);
        XCTAssertFalse([args[LP_KEY_LOCALE] isEqualToString:[[NSLocale currentLocale] localeIdentifier]]);
        [request_expectation fulfill];
    }];
    
    [Leanplum setLocale:locale];
    [self waitForExpectations:@[request_expectation] timeout:1.0];
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests has been updated

@@ -2249,6 +2249,14 @@ + (void)setTrafficSourceInfoInternal:(NSDictionary *)info
[[LPRequestSender sharedInstance] send:request];
}

+ (void)setLocale:(NSLocale *)locale
{
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use case - would this be called before starting Leanplum or at some point after it started?
If it has started, the locale in the start API call will be the system one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was for calling it after Leanplum is started, in order to simplify a bit complex variations of starting.

If you think it is better to move it to start, and/or to add some stored property for overriding the system's locale, so the start would take that one, I'm ok with it as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improved in a way that setLocale now can be called both before and after Leanplum is started

@batadamnjanovic
Copy link

Hi @nzagorchev, thanks for the collaboration.

Do you maybe know when will be the release with this change? It will mean a lot to us to organise some things in this sprint.

Thanks!

@nzagorchev
Copy link
Contributor

Hi @batadamnjanovic,
We plan a release in early January.

Can you update the PR, so I can merge it into master after that?
Thanks

@fitja
Copy link
Contributor Author

fitja commented Dec 20, 2021

Hi @nzagorchev the PR has been updated (proposed changes) also the branch has been updated with master
Can you check if there is anything more to update/change so the PR can be merged?
Thanks!

@nzagorchev nzagorchev merged commit 58b7f9c into Leanplum:master Dec 20, 2021
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