Skip to content

Add main network WPCOM blog ID to sync functions#15056

Merged
mdbitz merged 6 commits intomasterfrom
add/sync-multisite-primary-id
Mar 30, 2020
Merged

Add main network WPCOM blog ID to sync functions#15056
mdbitz merged 6 commits intomasterfrom
add/sync-multisite-primary-id

Conversation

@gravityrail
Copy link
Copy Markdown
Contributor

@gravityrail gravityrail commented Mar 20, 2020

Syncs the WPCOM ID of the main site in a multisite network so that we can collate documents from the same network in the ES Reader index.

Changes proposed in this Pull Request:

  • Sync the WPCOM ID of the main site in a multisite network

@gravityrail gravityrail requested a review from a team March 20, 2020 00:44
@github-actions github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Mar 20, 2020
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Mar 20, 2020 •
edited
Loading

Warnings
⚠️

pre-commit hook was skipped for one or more commits
⚠️ "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 an 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 against 95eced0

@jeherve jeherve added the [Status] Needs Review This PR is ready for review. label Mar 20, 2020
@jeherve jeherve added this to the 8.4 milestone Mar 20, 2020
@jeherve jeherve force-pushed the add/sync-multisite-primary-id branch from 4d8710a to 26208a9 Compare March 20, 2020 09:45
@jeherve 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 Mar 20, 2020
jeherve
jeherve previously requested changes Mar 20, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Tests appear to be failing right now:

PHP Fatal error:  Call to undefined method Automattic\Jetpack\Sync\Modules\Full_Sync_Immediately::get_enqueue_status() in /tmp/wordpress-master/src/wp-content/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-full.php on line 345

@bisko bisko self-assigned this Mar 27, 2020
@gravityrail
Copy link
Copy Markdown
Contributor Author

gravityrail commented Mar 27, 2020

@bisko I just had the thought that we should move this out of multisite callables, and always set this value.

The reason is that some sites might migrate away from being a multisite and keep the same WPCOM blog ID (for example, every site that migrates to Atomic) and we need to overwrite the multisite callable option with false so that the migrates site no longer appears in search results for the network.

This also means we need to trigger a full reindex if we see that the site is no longer part of a multisite - so, if jetpack_callable_is_multi_site value changes.

mdbitz
mdbitz previously requested changes Mar 27, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@mdbitz mdbitz left a comment

Choose a reason for hiding this comment

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

Testing shows this is returning the correct value on multi-sites, however based on comments and consistency in other functions we need to update the single site behavior so it returns the WP.com ID and not false.

@bisko bisko added the [Status] Needs Review This PR is ready for review. label Mar 30, 2020
@brbrr brbrr removed the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Mar 30, 2020
@bisko
Copy link
Copy Markdown
Contributor

bisko commented Mar 30, 2020

Tests appear to be failing right now:

PHP Fatal error:  Call to undefined method Automattic\Jetpack\Sync\Modules\Full_Sync_Immediately::get_enqueue_status() in /tmp/wordpress-master/src/wp-content/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-full.php on line 345

This test seems to be failing locally too with a different error message, probably due to environment configuration. It's also failing when trying it locally on the master branch. I think this should not be blocking for this PR and we should fix the tests in the near future.

cc @jeherve @mdbitz

@mdbitz
[not verified] Skip Term Relationships tests as these fail against ma…
95eced0 
…ster now that the tests are re-enabled.

Added to backlog to revise.
@mdbitz mdbitz dismissed stale reviews from jeherve and themself March 30, 2020 17:13

existing tests were failing, now marked as skipped until they can be revised.

mdbitz
mdbitz approved these changes Mar 30, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@mdbitz mdbitz left a comment

Choose a reason for hiding this comment

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

Verified Changes that main blog_id is returned and test Suite is executed.

@mdbitz mdbitz merged commit a21dc21 into master Mar 30, 2020
@mdbitz mdbitz deleted the add/sync-multisite-primary-id branch March 30, 2020 17:19
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review This PR is ready for review. labels Mar 30, 2020
@jeherve jeherve mentioned this pull request Mar 31, 2020
Closed
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
Changelog: add #15056
3a190a9
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
8.4 Changelog (#15075)
0eb0b35 
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

* Clarify changelog description
@gibrown gibrown mentioned this pull request Jul 20, 2020
Open
@jeherve jeherve removed the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mdbitz mdbitz mdbitz approved these changes

@jeherve jeherve jeherve left review comments

@roccotripaldi roccotripaldi Awaiting requested review from roccotripaldi

+1 more reviewer

@bisko bisko bisko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@bisko bisko

Labels

[Feature] Search For all things related to Search [Package] Sync

Projects

None yet

Milestone

8.4

Development

Successfully merging this pull request may close these issues.

7 participants

@gravityrail @jetpackbot @bisko @jeherve @mdbitz @brbrr @matticbot