Update/sync user language choice by enejb · Pull Request #6053 · Automattic/jetpack

enejb · 2017-01-07T01:11:36Z

Fixes #5535

Sync the user local if it is different from the sites or if the user sets it.

Changes proposed in this Pull Request:

Add sync event when the user local changes.
Full Sync doesn't send user local if it is the same as the sites.

Testing instructions:

Do the test pass?

Proposed changelog entry for your changes:

Make sure your changes respect WordPress' coding standards.
Did you make changes, or create a new .js file? If Gulp is installed on your testing environment, run gulp js:hint before to commit your changes. It will allow you to detect errors in Javascript files.
Did you create a new action or filter? Add inline documentation to help others understand how to use the action or the filter.
Create unit tests if you can. If you're not familiar with Unit Testing, you can check this tutorial.

ebinnion · 2017-01-09T18:13:32Z

tests/php/sync/test_class.jetpack-sync-full.php

 		$this->server_replica_storage->reset();
 		$this->sender->reset_data();
-
+		


There's extra whitespace here.

ebinnion · 2017-01-09T18:31:20Z

tests/php/sync/test_class.jetpack-sync-users.php

+	public function test_delete_user_locale_is_synced() {
+		global $wp_version;
+		if ( version_compare( $wp_version, 4.7, '<' ) ) {
+			$this->markTestSkipped( 'WP 4.7 and up supports user local' );


local => locale.

ebinnion · 2017-01-09T18:56:45Z

sync/class.jetpack-sync-module-users.php

+			return $user;
+		}
+
+		// Only set the user local if it is different from the site local


It might be a bit easier to write this as:

public function add_to_user( $user ) { $user->allowed_mime_types = get_allowed_mime_types( $user ); if ( ! function_exists( 'get_user_locale' ) ) { return $user; } // Only set the user local if it is different from the site local if ( get_locale() !== get_user_locale( $user->ID ) ) { $user->locale = get_user_locale( $user->ID ); } return $user; }

In case we need to add more meta in the future.

ebinnion · 2017-01-09T18:57:54Z

I left a bit of minor feedback, but overall, this LGTM. 👍

lezama · 2017-01-15T20:22:08Z

sync/class.jetpack-sync-module-users.php


-		return $user;
+
+			return $user;


miss spaced :P

enejb · 2017-01-30T20:02:20Z

@ebinnion and @lezama I think this is ready to merge.

lezama · 2017-02-01T17:58:42Z

sync/class.jetpack-sync-module-users.php

space after if

lezama · 2017-02-01T18:01:14Z

sync/class.jetpack-sync-module-users.php

I think it would be cleaner if we sync a jetpack_sync_delete_user_locale?

lezama · 2017-02-01T18:03:29Z

tests/php/sync/server/class.jetpack-sync-test-replicastore.php

extra space before =

enejb · 2017-02-02T22:45:31Z

@lezama Added fixes as per your suggestions.

jeherve · 2017-02-27T17:43:05Z

sync/class.jetpack-sync-module-users.php

+	function maybe_save_user_meta( $meta_id, $user_id, $meta_key, $value ) {
+		if ( $meta_key === 'locale' ) {
+			if ( current_filter() === 'deleted_user_meta' ) {
+				do_action( 'jetpack_sync_user_locale_delete', $user_id );


Could you add a docblock here?

jeherve · 2017-02-27T17:43:09Z

sync/class.jetpack-sync-module-users.php

+			if ( current_filter() === 'deleted_user_meta' ) {
+				do_action( 'jetpack_sync_user_locale_delete', $user_id );
+			} else {
+				do_action( 'jetpack_sync_user_locale', $user_id, $value );


Could you add a docblock here?

Added documentation

Sync the user locale value so that we have a better idea what need the user like the interface in.

Sync the user local when doing full sync.

…_sync_user_local

lezama

👍

Related: #6053

@SInCE

* Readme: remove old release and add skeleton for 4.8. * Changelog: add #6572 * Changelog: add #6567 * Changelog: add #6542 * Changelog: add #6527 * Changelog: add #6508 * Changelog: add #6478 * Changelog: add #6477 * Changelog: add #6249 * Update stable version and remove old version from readme. * Changelog: add 4.7.1 to changelog. * Readme: add new contributor. * Sync: update docblock @SInCE version. Related: #6053 * Changelog: add release post. * changelog: add #6053 * Changelog: add #6413 * Changelog: add #6482 * Changelog: add #6584 * Changelog add #6603 * Changelog: add #6606 * Changelog: add #6611 * Changelog: add #6635 * Changelog: add #6639 * Changelog: add #6684 * Changelog: add #6710 * Changelog: add #6711 * Changelog: add #5461 * Testing list: update Settings UI feedback prompt. Props @MichaelArestad * Changelog: add #6789 * Changelog: add #6778 * Changelog: add #6777 * Changelog: add #6775 * Changelog: add #6755 * Changelog: add #6731 * Changelog: add #6721 * Changelog: add #6705 * Changelog: add #6702 * Changelog: add #6671 * Changelog: add #6637 * Changelog: add #6582 * Changelog: add #6566 * Changelog: add #6555 * Changelog: add #6529 * Changelog: add #6344 * Changelog: add #5763 * Changelog: add #5503 * Changelog: update #6637 changelog. @see 40e115c#commitcomment-21523982 * Changelog: add #6699 * Changelog: add #6632 * Changelog: add #6769 * Changelog: add #6707 * Changelog: add #6590

enejb added [Package] Sync [Status] Needs Review This PR is ready for review. labels Jan 7, 2017

enejb added this to the 4.5 milestone Jan 7, 2017

enejb self-assigned this Jan 7, 2017

enejb requested review from ebinnion, gravityrail and lezama January 7, 2017 01:11

ebinnion reviewed Jan 9, 2017

View reviewed changes

jeherve modified the milestones: 4.5, 4.6 Jan 11, 2017

lezama reviewed Jan 15, 2017

View reviewed changes

sync/class.jetpack-sync-module-users.php Outdated

return $user;

return $user;

Copy link
Copy Markdown

Contributor

lezama Jan 15, 2017

Choose a reason for hiding this comment

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

miss spaced :P

enejb force-pushed the update/sync-user-language-choice branch from eab9ae9 to f4cbc0d Compare January 30, 2017 20:01

jeherve modified the milestones: 2/17 - February, 4.7.0 - March 2017 Jan 30, 2017

lezama reviewed Feb 1, 2017

View reviewed changes

sync/class.jetpack-sync-module-users.php Outdated

Copy link
Copy Markdown

Contributor

lezama Feb 1, 2017

Choose a reason for hiding this comment

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

space after if

lezama reviewed Feb 1, 2017

View reviewed changes

tests/php/sync/server/class.jetpack-sync-test-replicastore.php Outdated

Copy link
Copy Markdown

Contributor

lezama Feb 1, 2017

Choose a reason for hiding this comment

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

extra space before =

lezama added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Feb 1, 2017

enejb added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Feb 2, 2017

enejb force-pushed the update/sync-user-language-choice branch from 5b91254 to 14c2331 Compare February 23, 2017 18:43

jeherve previously requested changes Feb 27, 2017

View reviewed changes

jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Feb 27, 2017

enejb force-pushed the update/sync-user-language-choice branch from be08830 to c56fa87 Compare March 2, 2017 20:58

enejb added 6 commits March 8, 2017 09:59

Sync: Sync get_user_locale

50bd0da

Sync the user locale value so that we have a better idea what need the user like the interface in.

Add support for full sync

82d693b

Sync the user local when doing full sync.

Do Not sync user locale if it is the same as the sites.

dd11934

Use jetpack_sync_user_locale_delete when the user deletes the jetpack…

0c05404

…_sync_user_local

Add action documentation.

8b95baf

Remove white space and unused variables

18f4029

enejb force-pushed the update/sync-user-language-choice branch from c56fa87 to 18f4029 Compare March 8, 2017 18:02

Minor remove space

544b79e

lezama approved these changes Mar 9, 2017

View reviewed changes

lezama added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 9, 2017

samhotchkiss merged commit 217e752 into master Mar 13, 2017

samhotchkiss deleted the update/sync-user-language-choice branch March 13, 2017 17:34

samhotchkiss removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 13, 2017

jeherve added the [Status] Needs Changelog label Mar 13, 2017

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Mar 27, 2017

jeherve added a commit that referenced this pull request Mar 28, 2017

Sync: update docblock @SInCE version.

4400a99

Related: #6053

jeherve added a commit that referenced this pull request Mar 28, 2017

changelog: add #6053

072c445

		$this->server_replica_storage->reset();
		$this->sender->reset_data();

Conversation

enejb commented Jan 7, 2017

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebinnion commented Jan 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enejb commented Jan 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enejb commented Feb 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lezama left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants