Skip to content

Integrated locale handling changes in WordPressKit#10950

Merged
4 commits merged intodevelopfrom
issue/wpkit-78-locale-integration
Feb 6, 2019
Merged

Integrated locale handling changes in WordPressKit#10950
4 commits merged intodevelopfrom
issue/wpkit-78-locale-integration

Conversation

@ghost
Copy link

@ghost ghost commented Feb 5, 2019

Description

Fixes #78, which exposes the ability to inject different locale keys into WordPressComRestApi.

The code can be reviewed as-is; I have only affixed the DO NOT MERGE label to reflect the fact that it is dependent on the review, approval, merge, & versioning of this PR in WordPressKit.

Note: A separate issue has been created in WordPressKit to transition the existing service methods to leverage this implicit locale-handling behavior.

To test:

  • Checkout the branch, confirm that it builds & that existing tests pass.
  • Review the changes here, which are limited to implicit locale handling of current v2 endpoint calls, as well as the separate of v1 & v2 service methods in ActivityServiceRemote.
  • Smoke-test network behavior to confirm no regressions, with an emphasis on modified services (e.g. Rewind, Enhanced Site Creation).

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@ghost ghost added this to the 11.8 milestone Feb 5, 2019
@ghost ghost self-assigned this Feb 5, 2019
@ghost ghost requested review from SergioEstevao and frosty February 5, 2019 19:22
let api: WordPressComRestApi
if let wpcomApi = accountService.defaultWordPressComAccount()?.wordPressComRestApi {
api = wpcomApi
if let account = accountService.defaultWordPressComAccount(), let token = account.authToken {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about creating a method on the Account object called wordPressComRestV2APi and that will return the API object set to the correct v2 locale?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this suggestion – it would remove a bit of duplication 👍

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for this suggestion! I considered it during the initial implementation; it didn't seem to help as much as I had hoped.

Because AccountService.defaultWordPressComAccount() is optional, you still have some duplication in the event that returns nil. For example:

let api: WordPressComRestApi
if let account = accountService.defaultWordPressComAccount() {
    api = account.wordPressComRestV2Api
} else {
    api = WordPressComRestApi.anonymousApi(userAgent: WPUserAgent.wordPress(), localeKey: WordPressComRestApi.LocaleKeyV2)
}

In any case, I'll proceed to make this change.

Copy link
Contributor

@frosty frosty left a comment

Choose a reason for hiding this comment

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

✅ Reviewed the changes
✅ Built and ran tests
✅ Smoke-tested site creation and a few other areas

Looking good, I'd just echo Sergio's suggestion for a possible improvement.

@ghost ghost removed the [Status] DO NOT MERGE label Feb 6, 2019
@ghost
Copy link
Author

ghost commented Feb 6, 2019

Thanks for the feedback, @SergioEstevao! I have pushed a commit that incorporates your feedback. Would you be willing to take another look?

Copy link
Contributor

@SergioEstevao SergioEstevao left a comment

Choose a reason for hiding this comment

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

Looking good! Thanks for all your patience!

@ghost ghost merged commit 06ff19c into develop Feb 6, 2019
@ghost ghost deleted the issue/wpkit-78-locale-integration branch February 6, 2019 22:39
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

v2 endpoints require modified locale key

3 participants