Jetpack Sync: Add sync action for network site enable and disable themes by gititon · Pull Request #7180 · Automattic/jetpack

gititon · 2017-05-12T16:13:24Z

Jetpack Sync: Add sync action for network/multisite enable and disable themes

Changes proposed in this Pull Request:

Add sync action for network/multisite enable and disable themes

Testing instructions:

phpunit runs new tests when on multisite

Proposed changelog entry for your changes:

Addition of jetpack_network_disabled_themes and jetpack_network_enabled_themes hooks

… some reason

lezama · 2017-05-12T17:22:52Z

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

+			 *
+			 * @param mixed $disabled_themes, Array of info about network disabled themes
+			 */
+			do_action( 'jetpack_network_disabled_themes', $disabled_themes );


we could send the enabled ones too? what do you think?

Themes can't be both changed from enabled to disabled and changed from disabled to enabled by the same user action. When themes are disabled, the Activity Log needs to know which themes were disabled. When themes are enabled, the Activity Log needs to know which themes were enabled. Why would we send themes that are unrelated to the activity/action?

not sure really, I thought it could be useful to always know the active ones

lezama · 2017-05-12T17:23:39Z

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

+		if ( 'enable-selected' === $action || 'enable' === $action ) {
+			$enabled_theme_names = array_keys( array_diff_key( $value, $old_value ) );
+			$enabled_themes = array();
+			foreach ( $enabled_theme_names as $name ) {


we could probably extract this to its own method

lezama · 2017-05-12T19:45:27Z

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

+			 *
+			 * @since 5.0.0
+			 *
+			 * @param mixed $disabled_themes, Array of info about network disabled themes


we should add the second parameter too

lezama · 2017-05-12T19:45:36Z

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

+		 *
+		 * @since 5.0.0
+		 *
+		 * @param mixed $enabled_themes , Array of info about network enabled themes


we should add the second parameter too

lezama · 2017-05-12T19:47:40Z

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

+		do_action( 'jetpack_network_enabled_themes', $enabled_themes, array_keys( $value ) );
+	}
+
+	private function get_multisite_changed_themes( $theme_names ) {


we could call this method get_themes_details, we could use it with non multisite themes too

lezama

💟

enejb · 2017-05-12T20:42:01Z

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

+		 *
+		 * Note that 'allowedthemes' is dynamic, i.e. do_action is called on "update_site_option_{$option}"
+		 *
+		 * @since 2.9.0


I would remove the since part. Since the version number refers to WP and not Jetpack.

enejb · 2017-05-12T20:42:08Z

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

+		 *
+		 * Note that 'allowedthemes' is dynamic, i.e. do_action is called on "update_site_option_{$option}"
+		 *
+		 * @since 2.9.0


same as here

enejb · 2017-05-12T20:42:16Z

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

+		 *
+		 * Note that 'allowedthemes' is dynamic, i.e. do_action is called on "update_site_option_{$option}"
+		 *
+		 * @since 2.9.0


enejb · 2017-05-12T20:42:22Z

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

+		 *
+		 * Note that 'allowedthemes' is dynamic, i.e. do_action is called on "update_site_option_{$option}"
+		 *
+		 * @since 2.9.0


enejb · 2017-05-12T20:45:22Z

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

+			 * @param mixed $disabled_themes, Array of info about network disabled themes
+			 * @param mixed array_keys( $value ), Array of slugs of all enabled themes
+			 */
+			do_action( 'jetpack_network_disabled_themes', $disabled_themes, array_keys( $value ) );


Can we make array_keys( $value ) more descriptive. Something like
$enabled_theme_slugs would read better

enejb · 2017-05-12T20:45:27Z

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

+		 * @param mixed $enabled_themes , Array of info about network enabled themes
+		 * @param mixed array_keys( $value ), Array of slugs of all enabled themes
+		 */
+		do_action( 'jetpack_network_enabled_themes', $enabled_themes, array_keys( $value ) );


Can we make array_keys( $value ) more descriptive. Something like
$enabled_theme_slugs would read better

enejb · 2017-05-12T20:46:20Z

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

+	public function sync_network_allowed_themes_change( $option, $value, $old_value, $network_id ) {
+		if ( count( $old_value ) > count( $value ) )  {
+			$disabled_theme_names = array_keys( array_diff_key( $old_value, $value ) );
+			$disabled_themes = $this->get_theme_details_for_slugs( $disabled_theme_names );


Something like $newly_disabled_themes might be a bit more descriptive.

enejb · 2017-05-12T20:48:47Z

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

+
+		$event_data = $this->server_event_storage->get_most_recent_event( 'jetpack_network_enabled_themes' );
+
+		foreach ( $test_themes as $theme ) {


Lets also test that the slug is also being set.

enejb · 2017-05-12T20:49:21Z

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

+
+		$event_data = $this->server_event_storage->get_most_recent_event( 'jetpack_network_disabled_themes' );
+
+		foreach ( $test_themes as $theme ) {


lets test for the slug here too.

enejb

Left some minor feedback. Would it make sense to update split up the big test?

enejb

Thanks for the fixes!

@see

* Changelog: first pass at a changelog for 5.0 * Changelog: delete 4.9 testing list. * Changelog: update minimum WP version to match ver. in jetpack.php Fixes #7158 * Changelog: add #6051 * Changelog: add #6753 * Changelog: add #6928 * Changelog: add #6964 * Changelog: add #7014 * Changelog: add #7057 * Changelog: add #7060 * Changelog: add #7068 * Changelog: add #7070 * Changelog: add #7072 * Changelog: add #7071 * Changelog: add release date and post shortlink. * Changelog: add #7094 * Changelog: add #7100 * Changelog: add #7108 * Changelog: add #7113 * Changelog: add #7123 * Changelog: add #7135 * Changelog: add #7143 * Changelog: add #7151 * Changelog: add #6996 * Changelog: add #7105 * Changelog: add #7132 * Changelog: add #7166 * Changelog: fix typo in 4.9 changelog. * Changelog: remove older releases' changelogs. @see p1HpG7-42e-p2 * Changelog: add #7090 * Changelog: add #7095 * Changelog: add #7112 * Changelog: add #7115 * Changelog: add #7122 * Changelog: add #7137 * Changelog: add #7138 * Changelog: add #7140 * Changelog: add #7154 * Changelog: add ##7155 * Changelog: add #7163 * Changelog: add #7167 * Changelog: add #7171 * Changelog: add #7180 * Changelog: add #7181 * Changelog: add #7183 * Changelog: add #7184 * Changelog: add #7189 * Changelog: add #7191 * Changelog: add #7193 * Changelog: add #7198 * Changelog: add #7200 * Changelog: add #7209 * Changelog: add #7212 * Testing list: add instructions for #7115 * Changelog: add #7188 * Changelog: add #7205 * Changelog: add #7225 * Changelog: add #6872 * Changelog: add #7107 * Changelog: add #7118 * Changelog: add #7142 * Changelog: add #7170 * Changelog: add #7210 * Changelog: add #7218 * Changelog: add #7232 * Changelog: add #7211 * Changelog: add #7213 * Changelog: add #7229 * Changelog: add #7230 * Changelog: add #7214 * Draft changelog for 5.0 * Changelog updates: 2nd pass at a clearer changelog. - Fix typos. - Use consistent tense and tone across all changelog. - Remove unclear items. * Changelog: add #7026 * Changelog: add #7058 * Changelog: add #7125 * Changelog: add #7249 * Changelog: add #7185 * add mentions of image widget migration * Changelog: add info about new output for CLI command. * Changelog: add WP version number matching the new Image Widget.

gititon added 9 commits May 11, 2017 16:22

Jetpack Sync: Sync network allowed themes changes

98566e8

Jetpack Sync: Added missing paren

9bd66c7

Jetpack Sync: Update

f29c81c

Jetpack Sync: Adding conditional to network theme enable logic

a98d046

Jetpack Sync: Handle action being -1 and action2 containing value for…

51b8e11

… some reason

Jetpack Sync: Trying intval

e4ae0c3

Jetpack Sync: Changing semicolon to colon

052f327

Jetpack Sync: Test

6184e2d

Jetpack Sync: Added do action comment

6d5414b

gititon added [Package] Sync [Pri] Normal [Status] Ready to Merge Go ahead, you can push that green button! [Team] Poseidon labels May 12, 2017

gititon requested review from enejb, lezama and roccotripaldi May 12, 2017 16:13

gititon added 2 commits May 12, 2017 12:15

Jetpack Sync: Whitespace

713e732

Jetpack Sync: Whitespace

2225e2e

lezama reviewed May 12, 2017

View reviewed changes

gititon added 4 commits May 12, 2017 13:38

Jetpack Sync: Moving duplicate code to function

97a8ca1

Jetpack Sync: Removing request checking

ac59b97

Jetpack Sync: Updating tests

3758cf9

Jetpack Sync: Whitespace

f8f6bba

lezama reviewed May 12, 2017

View reviewed changes

Jetpack Sync: PR feedback

aa06c07

lezama approved these changes May 12, 2017

View reviewed changes

enejb reviewed May 12, 2017

View reviewed changes

enejb requested changes May 12, 2017

View reviewed changes

Jetpack Sync: PR Feedback

7e79d70

enejb approved these changes May 12, 2017

View reviewed changes

gititon merged commit 9a5667f into master May 12, 2017

matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 12, 2017

jeherve deleted the add/sync_allowed_theme branch May 15, 2017 12:55

jeherve added the [Status] Needs Changelog label May 15, 2017

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels May 23, 2017

jeherve added a commit that referenced this pull request May 23, 2017

Changelog: add #7180

7189ab1

jeherve added a commit that referenced this pull request May 29, 2017

Changelog: add #7180

f620d1f


		$event_data = $this->server_event_storage->get_most_recent_event( 'jetpack_network_enabled_themes' );

		foreach ( $test_themes as $theme ) {

Conversation

gititon commented May 12, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lezama May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lezama left a comment

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!

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

enejb left a comment

Choose a reason for hiding this comment

Uh oh!

enejb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lezama May 12, 2017 •

edited

Loading