Sync: Sync previous state when sycing post saves. by enejb · Pull Request #8443 · Automattic/jetpack

enejb · 2018-01-03T00:29:23Z

Sync previous state with in the wp_insert_post

Currently we don't have a good way to tell what the previous state was previous state with we sync posts.

Creating a new action would help keep things backwards compatible.

Changes proposed in this Pull Request:

Testing instructions:

Do the tests pass?
With the .com patch do you does you site still publicize and send subscription emails?
Does the data save and are you able to get the post to show up in related posts?

Proposed changelog entry for your changes:

lezama · 2018-01-03T02:45:43Z

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

does it make sense to keep this logic this side of the things?

lezama · 2018-01-03T02:46:25Z

+💯

lezama · 2018-01-03T02:46:58Z

I like the idea, also less code Jetpack side 👍

enejb · 2018-01-03T23:41:56Z

Related .com code D9089-code

gititon · 2018-01-04T21:52:18Z

tests/php/sync/server/class.jetpack-sync-server-replicator.php

why keep the wp_insert_post case below this, unless this won't handle all post types?

Good point! Will remove it.

gititon · 2018-01-05T15:48:32Z

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

Previously, $just_published was a boolean, which made sense because we were specifying whether $just_published was true or false. With your changes, I'm not sure it makes sense to continue to assign a boolean to $previous_state. It might make more sense to assign a constant representing the previous state to $previous_state instead.

make sense.

gititon · 2018-01-05T15:55:00Z

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

Should we consider simply sending a $state parameter that is an array with keys 'is_autosave', 'previous_state', etc.?

I didn't want to change things up to much. In terms of what the data shape is that we pass back. But now is the time to do that. I think we could definitely do that.

gititon · 2018-01-05T15:55:47Z

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

Can you please remove this stray comment while you're in here?

gititon · 2018-01-05T15:58:07Z

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

Stray comment.

gititon · 2018-01-05T15:58:59Z

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

Is one of these lines (either 91 or 92) the current state and one of them the previous state? If so, can we identify them in the object as such?

enejb · 2018-01-08T22:56:54Z

@gititon and @lezama Can you take a look at this again?

lezama · 2018-01-09T11:36:56Z

looks way cleaner now 💯 😍

let's ship it! we should merge .com side first

lezama · 2018-01-10T00:17:16Z

@enejb let's wait on another @gititon pass before merging!

enejb · 2018-01-11T19:33:24Z

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

roccotripaldi

This is testing well!

changes fixed

jeherve

A small change to make the parser happy.

jeherve · 2018-01-12T19:18:01Z

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

Could you add the @module sync text to you Sync hooks, so it gets categorized by the parser?
https://developer.jetpack.com/module/sync/

Thank you!

Will do. I had no idea we had such magic ❤️

zinigor

Changes make sense, LGTM!

Sync previous state with in the wp_insert_post

I did a bit of digging and it turns out that we use both convetions. Lets stick with jetpack_sync_* since clearly the action is being used for sync.

This will categorize the sync action for https://developer.jetpack.com/module/sync/

Fixed

* Changelog 5.8: create base for changelog. * Update 5.8 release post link * fix 5.8 release date * Updates to plugin description * Changelog: add #8499 * Changelog: add #8506 * Changelog: add #8509 * Changelog: add #8516 * Changelog: add #8517 * Changelog: add #8523 * Changelog: add #8547 * Changelog: add #8496 * Changelog: add #8584 * Changelog: add #8595 * Changelog: add #8445 * Changelog: add #8431 * Changelog: add #8284 * Changelog: add #8270 * Changelog: add #8124 * Changelog: add #8581 * Changelog: add #8463 * Changelog: add #8568 (#8646) * Updates to testing list and changelog * Changelog: add #8443 * Changelog: add #8459 * Changelog: add #8469 * Changelog: add #8464 * Changelog: add #8478 and #8479 * Changelog: add #8483 * Changelog: add #8488 * Changelog: add #8513 * Changelog: add #8555 * Changelog: add #8565 * Changelog: add #8601 * Changelog: add #8612 * Changelog: add first pass at Search items. * Changelog: add more info to help test Search. * Changelog: add #8144 * Changelog: add #8313 * Changelog: add #8419 * Changelog: add #8465 * Changelog: add #8515 * Changelog: add #8587 * Changelog: add #8591 * Changelog: add #8659 * Changelog: add #8661 * Changelog: add #8671 * Changelog: add 5.7.1 to archived changelog too. * Reverted changes to readme, removed entry about backups.

enejb requested a review from a team as a code owner January 3, 2018 00:29

enejb changed the title ~~Get the idea across~~ Sync: Sync previous state when sycing post saves. Jan 3, 2018

lezama reviewed Jan 3, 2018

View reviewed changes

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

Copy link
Copy Markdown

Contributor

lezama Jan 3, 2018

Choose a reason for hiding this comment

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

does it make sense to keep this logic this side of the things?

jeherve added [Package] Sync [Status] In Progress labels Jan 3, 2018

enejb force-pushed the update/sync-post-data branch from c26a4bd to 5d5d036 Compare January 3, 2018 21:49

gititon previously requested changes Jan 5, 2018

View reviewed changes

enejb force-pushed the update/sync-post-data branch from 5858374 to 64326c5 Compare January 8, 2018 19:02

enejb added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Jan 8, 2018

enejb mentioned this pull request Jan 10, 2018

Sync/fix trash post one event #8301

Closed

enejb commented Jan 11, 2018

View reviewed changes

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

Copy link
Copy Markdown

Member Author

enejb Jan 11, 2018

Choose a reason for hiding this comment

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

spacing

lezama force-pushed the update/sync-post-data branch from d48d519 to f12f1c0 Compare January 12, 2018 14:50

roccotripaldi approved these changes Jan 12, 2018

View reviewed changes

jeherve previously requested changes Jan 12, 2018

View reviewed changes

zinigor approved these changes Jan 12, 2018

View reviewed changes

zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Jan 12, 2018

enejb added 5 commits January 12, 2018 11:48

Get the idea across

d5b72e4

Sync previous state with in the wp_insert_post

Fixes to items

7d8e42d

Fix more broken tests

9fac213

More test fixes

c29e7ca

Push fixes as suggested.

e35ee9d

enejb added 5 commits January 12, 2018 11:48

Only 4 arguments get returned

eeb679f

Update jetpack_save_post to jetpack_sync_save_post

0437417

I did a bit of digging and it turns out that we use both convetions. Lets stick with jetpack_sync_* since clearly the action is being used for sync.

More updates to the new string

01cba61

More cleanup

520b1ad

Update doc bock

5f031f1

This will categorize the sync action for https://developer.jetpack.com/module/sync/

enejb force-pushed the update/sync-post-data branch from f12f1c0 to 5f031f1 Compare January 12, 2018 19:48

enejb merged commit ccc5ce7 into master Jan 12, 2018

matticbot added the [Status] Needs Changelog label Jan 12, 2018

enejb deleted the update/sync-post-data branch January 12, 2018 20:10

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

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Jan 29, 2018

jeherve added a commit that referenced this pull request Jan 29, 2018

Changelog: add #8443

06eec0e

lupus2k added a commit that referenced this pull request Jul 26, 2024

Fix for desktop and remove changes from #8443

f8fbc37

Conversation

enejb commented Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

lezama commented Jan 3, 2018

Uh oh!

lezama commented Jan 3, 2018

Uh oh!

enejb commented Jan 3, 2018

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 commented Jan 8, 2018

Uh oh!

lezama commented Jan 9, 2018

Uh oh!

lezama commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

zinigor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

enejb commented Jan 3, 2018 •

edited

Loading