Jetpack: Sync when an attachment is added to a post for the first time by gititon · Pull Request #10096 · Automattic/jetpack

gititon · 2018-09-04T21:04:36Z

This PR changes attachment update syncing to be triggered by attachment_updated, and syncs a specific action when the update changes the post_parent from 0 to non-zero, which is indicative of an attachment being attached to a post.

Testing Instructions:
Attach an existing post from your media library to a post by clicking Add Media in the post. You should see the jetpack_sync_save_update_attachment sync action come through on your wpcom sandbox.

phpunit tests were added to test this functionality as well

matticbot · 2018-09-04T21:04:42Z

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D17928-code. (newly created revision)

jetpackbot · 2018-09-04T21:10:41Z

	Warnings
⚠️	"Testing instructions" are missing for this PR. Please add some
⚠️	"Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is automated check which relies on PULL_REQUEST_TEMPLATE.We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS

roccotripaldi · 2018-09-04T21:24:53Z

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

@@ -9,8 +9,10 @@ public function init_listeners( $callable ) {
 		add_action( 'edit_attachment', array( $this, 'send_attachment_info' ) );


We'll need to suppress edit_attachment activities when we know its an attach_attachement activity.

jeherve

A quick change since 6.5 is now behind us.

jeherve · 2018-09-05T13:48:21Z

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

+			/**
+			 * Fires when an existing attachment is added to a post for the first time
+			 *
+			 * @since 6.5.0


Since 6.5 is out, you'll need to update this to 6.6.0 I'm afraid!

No prob, no big rush on this one!

matticbot · 2018-09-05T16:06:07Z

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D17928-code. (updated diff)

matticbot · 2018-09-05T16:06:51Z

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D17928-code. (updated diff)

enejb · 2018-09-05T16:25:42Z

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

+		$this->post->post_parent          = 0;
+		$modified_attachment              = clone $this->post;
+		$modified_attachment->post_parent = 1000;
+		do_action( 'attachment_updated', $this->post->ID, $modified_attachment, $this->post );


Instead of calling this do_action directly can we call the wp_insert_post with the expected parameters here?

This would help test that it would work across different version of WP.

roccotripaldi

Tests really well!
There's still a few small things to address.

roccotripaldi · 2018-09-05T16:47:45Z

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

-			 * Previously this action was synced using jetpack_sync_save_add_attachment action.
-			 */
-			do_action( 'jetpack_sync_save_update_attachment', $attachment_id, $attachment );
+			do_action( 'jetpack_sync_save_update_attachment', $attachment_id, $attachment_after );


We'll need to include the old docblock as well

gititon · 2018-09-05T21:42:32Z

@enejb @jeherve @roccotripaldi would you mind taking another look? Thanks!

enejb · 2018-09-06T13:07:42Z

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

+		$attach_attachment_event = $this->server_event_storage->get_most_recent_event( 'jetpack_sync_save_attach_attachment' );
+		$update_attachment_event = $this->server_event_storage->get_most_recent_event( 'jetpack_sync_save_update_attachment' );
+		$add_attachment_event    = $this->server_event_storage->get_most_recent_event( 'jetpack_sync_save_add_attachment' );
+


Lets also check that we have all the data that will make it so possible that the attachment is stored on the .com side as expected.

enejb · 2018-09-06T13:09:51Z

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

+
+	function process_update( $attachment_id, $attachment_after, $attachment_before ) {
+		// Check whether attachment was added to a post for the first time
+		if ( 0 === $attachment_before->post_parent && 0 !== $attachment_after->post_parent ) {


Should we be checking here if the attachment has changed?
Is it also possible for the attachment to be removed?
Does that ever happen?

matticbot · 2018-09-06T17:30:35Z

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D17928-code. (updated diff)

enejb

The changes in the PR look good and I was able to see the new activity show up on the .com side.

Before merging it lets update the .com side first so that we don't potentially loose data for folks running the latest master.

addressed!

@see

* Readme: add boilerplate for next release, 6.6 * Add 6.5 to the changelog.txt file * Set boilerplate testing list for 6.6 * Readme: update stable tag to 6.5 * Add bullets to 6.5 changelog items * Readme: add link to previous changelogs This will help folks who want to know more about past releases, while keeping the readme.txt short so as to not overwhelm translators and site owners only looking for information about the last release. * Changelog: add information at the top of the changelog file. * Changelog: add #10054 * Changelog: add #10078 * Changelog: add #10079 * Changelog: add #10064 * Changelog: add #10094 * Changelog: add #10096 * Testing list: add more information based on #10087 * Changelog: add #9847 * Changelog: add #10084 * Changelog: add #9918 * Changelog: add #7614 * Changelog: add #10116 * Changelog: add #10108 * Changelog: add #10041 * Changelog: add #10121 * Changelog: add #10134 * Changelog: add #10130 * Changelog: add #10109 * changelog: add #10137 * changelog: add #9952 * changelog: add #10120 * changelog: add #10162 * Changelog: add #10163 * Changelog: add #10092 * changelog: add #10156 * Changelog: add #10154 * changelog: add #10122 * Changelog: add #10101 * changelog: add #10105 * changelog: add #10190 * Changelog: add #10196 * changelog: add #10152 * Changelog: add #10153 * Testing list: add more details to Site Verification testing steps. @see #10143 (comment) * changelog: add #10194 * Changelog: add #10193

gititon added 2 commits September 4, 2018 16:59

Jetpack: Sync when an attachment is added to a post for the first time

9377913

Jetpack: Sync when an attachment is added to a post for the first time

2dd8b61

gititon added [Status] Needs Review This PR is ready for review. [Package] Sync labels Sep 4, 2018

gititon requested review from enejb and roccotripaldi September 4, 2018 21:04

gititon requested a review from a team as a code owner September 4, 2018 21:04

roccotripaldi suggested changes Sep 4, 2018

View reviewed changes

jeherve previously requested changes Sep 5, 2018

View reviewed changes

Update

2e3de8d

Update

cf2bf5d

Update

afe3d20

enejb reviewed Sep 5, 2018

View reviewed changes

roccotripaldi reviewed Sep 5, 2018

View reviewed changes

gititon added 2 commits September 5, 2018 13:36

Update

6d653fe

Update

ac67a58

enejb reviewed Sep 6, 2018

View reviewed changes

Update

c894ad9

enejb approved these changes Sep 6, 2018

View reviewed changes

roccotripaldi approved these changes Sep 6, 2018

View reviewed changes

gititon merged commit 829e45a into master Sep 6, 2018

matticbot added the [Status] Needs Changelog label Sep 6, 2018

gititon deleted the sync-attachment-attached branch September 6, 2018 19:20

gititon mentioned this pull request Sep 7, 2018

Activity Log: existing image attached to a post Automattic/wp-calypso#26906

Closed

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog [Status] Needs Review This PR is ready for review. labels Sep 14, 2018

jeherve added a commit that referenced this pull request Sep 24, 2018

Changelog: add #10096

3acdb3b

		@@ -9,8 +9,10 @@ public function init_listeners( $callable ) {
		add_action( 'edit_attachment', array( $this, 'send_attachment_info' ) );

Conversation

gititon commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matticbot commented Sep 4, 2018

Uh oh!

jetpackbot commented Sep 4, 2018

Uh oh!

roccotripaldi Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

jeherve left a comment

Choose a reason for hiding this comment

Uh oh!

jeherve Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

roccotripaldi Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

matticbot commented Sep 5, 2018

Uh oh!

matticbot commented Sep 5, 2018

Uh oh!

enejb Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

roccotripaldi left a comment

Choose a reason for hiding this comment

Uh oh!

roccotripaldi Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

gititon commented Sep 5, 2018

Uh oh!

enejb Sep 6, 2018

Choose a reason for hiding this comment

Uh oh!

enejb Sep 6, 2018

Choose a reason for hiding this comment

Uh oh!

matticbot commented Sep 6, 2018

Uh oh!

enejb left a comment • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gititon commented Sep 4, 2018 •

edited

Loading

enejb left a comment •

edited

Loading