Sync: Add Sync Events for menus. by enejb · Pull Request #7014 · Automattic/jetpack

enejb · 2017-04-20T19:01:50Z

This will allows us to know when a user is creating /updating / removing a menu
and when the user is adding new items and updating them on the menu.

Changes proposed in this Pull Request:

Add events that let us track changes to menus.

Testing instructions:

Do the test pass?

Proposed changelog entry for your changes:

lezama · 2017-04-20T19:24:19Z

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

+	}
+
+	public function update_nav_menu_add_item( $menu_id, $nav_item_id, $nav_item_args ) {
+		$menu_data = wp_get_nav_menu_object( $menu_id ); //


empty comment

lezama · 2017-04-20T19:25:45Z

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

+
+	}
+
+	function test_sync_updating_a_menu() {


could we split this one on multiple tests?

gititon

Nice work! As discussed in Slack, it may make sense to split out the menu code into its own sync module and accompanying test class.

jeherve

Just a minor change to make our parser happy.

jeherve · 2017-04-23T19:34:01Z

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

+		/**
+		 * Helps Sync log that a got cleared from inactive.
+		 *
+		 * @param int $menu_id, the id of the menu


Could you reorder the docblock to make sure the parser can catch everything?

Thanks!

Sorry I don't quite understand what you mean by reorder?

gititon · 2017-05-01T17:34:33Z

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

Did you mean for it to return 'menus' instead of 'themes'?

gititon · 2017-05-01T18:17:14Z

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

That a what got cleared?

This will allows us to know when a user is updating the creating /updating/ removing a menu and when the user is adding new items and updating them on the menu.

gititon · 2017-05-01T19:21:07Z

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

+		/**
+		 * Helps sync log that a nav menu was updated.
+		 *
+		 * Helps Sync log that a got cleared from inactive.


"Helps Sync log that a got cleared from inactive." That a what got cleared from inactive?

Actually, do you mean to have this line in the comment at all since it is beneath one that says the call is to "Helps sync log that a nav menu was updated."?

jeherve

Just a minor change to make phpcs happy.

jeherve · 2017-05-01T20:45:03Z

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

+	}
+
+	public function remove_just_added_menu_item( $nav_item_id, $post_after  ) {
+		if ( $post_after->post_type !== 'nav_menu_item' ) {


Could you use Yoda conditions here?

gititon · 2017-05-03T19:24:41Z

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

+		$this->assertEquals( $event->args[2], $link_item );
+		$this->assertEquals( $event->args[3]['menu-item-title'], 'LINK TO LINKS' );
+		$this->assertEquals( $event->args[3]['menu-item-url'], 'http://example.com' );
+


gititon · 2017-05-03T19:24:53Z

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

+		$this->assertEquals( $event->args[2], $link_item );
+		$this->assertEquals( $event->args[3]['menu-item-title'], 'make it https MORE LINKS' );
+		$this->assertEquals( $event->args[3]['menu-item-url'], 'https://example.com' );
+


gititon · 2017-05-03T19:25:15Z

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

+		$this->assertTrue( (bool) $event );
+		$this->assertEquals( $event->args[0], $menu_id );
+		$this->assertEquals( $event->args[1]['menu-name'], 'FUN' );
+


enejb · 2017-05-04T01:26:11Z

@jeherve Do you mind reviewing it again?

jeherve

Looking good!

@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.

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

enejb requested review from gititon and lezama April 20, 2017 19:01

enejb changed the title ~~Add Sync Events for menus.~~ Sync: Add Sync Events for menus. Apr 20, 2017

lezama reviewed Apr 20, 2017

View reviewed changes

gititon reviewed Apr 21, 2017

View reviewed changes

jeherve added the [Team] Poseidon label Apr 23, 2017

jeherve requested changes Apr 23, 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] Needs Review This PR is ready for review. labels Apr 23, 2017

gititon reviewed May 1, 2017

View reviewed changes

enejb added 2 commits May 1, 2017 11:37

Add Sync Events for menus.

4cace5e

This will allows us to know when a user is updating the creating /updating/ removing a menu and when the user is adding new items and updating them on the menu.

Spit up the tests

c9b4509

enejb force-pushed the add/sync-menu-events branch from 61107a9 to bac6c45 Compare May 1, 2017 18:39

Add doc blocks

48abe13

enejb force-pushed the add/sync-menu-events branch from 78dba82 to 48abe13 Compare May 1, 2017 18:42

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 May 1, 2017

gititon reviewed May 1, 2017

View reviewed changes

Minor documentation fix

cacb630

jeherve reviewed May 1, 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] Needs Review This PR is ready for review. labels May 1, 2017

Use Yoda condition.

a27a0c7

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 May 2, 2017

gititon reviewed May 3, 2017

View reviewed changes

Removed extra lines from test file

04e1dae

Remove whitespace

be62279

jeherve approved these changes May 4, 2017

View reviewed changes

lezama merged commit 9d30b1e into master May 4, 2017

lezama deleted the add/sync-menu-events branch May 4, 2017 15:27

matticbot removed the [Status] Needs Review This PR is ready for review. label May 4, 2017

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

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

Changelog: add #7014

36beca3

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

Changelog: add #7014

dbb28a1

Conversation

enejb commented Apr 20, 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!

gititon left a comment

Choose a reason for hiding this comment

Uh oh!

jeherve 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!

jeherve 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!

enejb commented May 4, 2017

Uh oh!

jeherve left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants