Repository navigation
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Premium Analytics plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 2 files.
1 file is newly checked for coverage.
|
| 'woocommerce_analytics' => true, | ||
| 'terms' => true, | ||
| 'term_relationships' => true, | ||
| 'posts' => true, |
There was a problem hiding this comment.
Could we send only product and variation IDs here instead of posts => true? The posts module accepts an ID list, and WordPress.com only builds its product table from product and product_variation posts. On a test store with 25 products this resent 98 posts, so stores with large blogs would resend everything.
There was a problem hiding this comment.
Good call, updated it to only send the product and variation IDs. I did add a cap of 5,000 IDs though, since full sync stores its config in the status option and rewrites it after every batch. Above that it falls back to resending all posts, which for a store that size are mostly products anyway.
| } | ||
| } | ||
|
|
||
| if ( $can_retry ) { |
There was a problem hiding this comment.
Should we clear the state option when a run can't go on, so a later dashboard load schedules it again? I noticed two paths leave it set for good: the retry window ends while sync isn't allowed, or another full sync with an explicit module list replaces this one after started_at is set.
There was a problem hiding this comment.
Nice catch on both! When the retry window runs out it now clears the state, so the next dashboard load schedules a new run. A started run also gets released on the next check if another full sync replaced it, and if our own sync finished but the tracker missed the end, it marks the data synced from the stored status instead.
Fixed in 16562de.
| } | ||
|
|
||
| // Wait for the first analytics full sync; if it runs under Premium Analytics, the tracker marks the data synced. | ||
| if ( (int) get_option( Sync_Status_Tracker::INITIAL_ANALYTICS_SYNC_OPTION, 0 ) <= 0 ) { |
There was a problem hiding this comment.
Would it be worth also checking the standalone plugin's woocommerce_analytics_initial_full_sync_finished option here? It's only set by that plugin and survives deactivation, so stores that never ran it could skip the WordPress.com probe, and a wrong probe answer couldn't start a full sync on them.
There was a problem hiding this comment.
Agreed, added this. Without that option Premium Analytics sent the lookup data itself, so we now mark it as synced without asking WordPress.com. The only edge case is a store that used the plugin's "reset initial full sync" tool before switching, which I think is fine to accept.
Fixed in 47863da.
| } | ||
| ); | ||
| if ( ! $active ) { | ||
| // An array of IDs is a partial sync, which leaves the other orders without lookup data. |
There was a problem hiding this comment.
nit: The woocommerce_analytics module reads an array config as a start_date / end_date range, not IDs (same for the partial analytics sync test row), so maybe:
| // An array of IDs is a partial sync, which leaves the other orders without lookup data. | |
| // An array config is a date range, a partial sync that leaves the other orders without lookup data. |
There was a problem hiding this comment.
Thanks, applied this and updated the partial analytics sync test row to use a date range as well.
Fixed in 16562de.
|
Thanks for the thorough review @chihsuan! Addressed all four inline, would you mind giving it another look? :) |
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for the updates! @louwie17 This looks ready to merge once the inline note is addressed. 👍
Just a thought, I wonder if it makes sense to move WooCommerce-specific sync code like this backfill to woocommerce-stats in a follow-up, keeping PA's sync generic for all consumers? #53282 already starts that, but it hasn't merged yet.
| } | ||
|
|
||
| $state = get_option( self::STATE_OPTION ); | ||
| if ( false !== $state && ! $this->forget_lost_run( $state ) ) { |
There was a problem hiding this comment.
Could we also release a scheduled run whose action is gone? It's an edge case: if the pending action is canceled in Tools › Scheduled Actions, or fails because Jetpack was off, the state stays at started_at: 0. Every later check then returns here, so the backfill never runs again.
There was a problem hiding this comment.
Good catch, fixed in 977726d. A scheduled run is now released when its action is no longer pending or running, so the next dashboard load schedules a new one.
Stores that synced orders through the standalone WooCommerce Analytics plugin hold order stats on WordPress.com without product, tax and coupon lookup rows. Start one woocommerce_analytics full sync through Action Scheduler when an earlier analytics full sync finished, orders and lookup rows exist, and no full sync under Premium Analytics has sent the data yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…kfilling Check for a backfill when the dashboard polls /jetpack/v4/sync/status or loads, instead of on every admin_init. Before starting the full sync, ask WordPress.com's products report for the oldest completed order's product; when it is there, the history already arrived and no full sync starts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ress.com accepts WordPress.com rejected the probe with HTTP 400: date_type takes created, paid or completed, and add_query_arg() left the + of the date offsets unencoded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ckfill WordPress.com builds its product table from synced product posts and their product_type terms, and skips them while it doesn't know an analytics plugin is active. On a store that synced its catalog before then, reports show products without name, SKU or type, and type-filtered widgets stay empty. Add terms, term_relationships and posts to the backfill's full sync, and treat a reported product without a type as missing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Send only product and variation IDs in the backfill's posts full sync, and skip the catalog modules when the store has no products. Forget a run whose retries ran out, or whose full sync another one replaced, so the next dashboard check schedules a new one; settle a finished run the tracker missed. Correct the tracker comment: an array config for woocommerce_analytics is a date range. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… ran the standalone plugin Without the plugin's woocommerce_analytics_initial_full_sync_finished, Premium Analytics sent every order's lookup data itself, so mark the data synced without asking WordPress.com. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…is large Full sync stores its config in the status option, rewrites it after every batch and sends it in jetpack_full_sync_start, so above 5,000 product and variation IDs resend all posts instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A canceled or failed action left the state at started_at 0, so every later check stopped there and the backfill never ran again. Forget the run when no action is pending or running. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7e8df5a to
977726d
Compare
Just a heads-up, I merged trunk into this branch to bring it up to date. |
Fixes #
Proposed changes
Stores that ran the standalone WooCommerce Analytics plugin before Premium Analytics hold order stats on WordPress.com but no product, tax or coupon lookup rows: the plugin strips that data from the shared
woocommerce_analyticssync module. Nothing resends it after Premium Analytics takes over. Incremental sync only sends newly imported orders, the dashboard doesn't start a sync because the milestone is already set from the plugin's sync, and a checksum didn't repair it on a test store.Sync\Lookup_Data_Backfill, registered fromConfiguration::configure_sync()so it runs only with WooCommerce active. It checks for a backfill:GET /jetpack/v4/sync/status(rest_post_dispatch, priority 11, afterSync_Status_Trackersets the milestone from the stored full sync status at 10);current_screen+Analytics::is_dashboard_request()). This covers stores whose milestone was already set and whose dashboard no longer polls.woocommerce_analytics_initial_full_sync_finishedis set,wc_order_statshas rows and at least one lookup table has rows. The plugin's option is only set by that plugin and survives deactivation; without it Premium Analytics sent every order's lookup data itself. Without that option, with no orders or with empty lookup tables, it marks the data as synced and stops without asking WordPress.com.wpcom/v2/sites/<id>/analytics/reports/products(signed as the blog) for a day either side of itsdate_created, withdate_type=created. The products report is built from the product lookup table joined with order stats, so the product is missing exactly when the lookup rows are. A completed order is never in the report's default excluded statuses. Only an old order can tell: orders synced after the takeover carry lookup rows.premium_analytics_lookup_backfill:woocommerce_analytics, plusterms,term_relationshipsandpostslimited to product and variation IDs when the store has products.wc_product_lookuptable (name, SKU, type, stock status) from synced product posts and theirproduct_typeterms, but only once its syncedjetpack_connection_active_pluginslists an analytics plugin. A catalog synced before that has no rows there, so reports show products without names or types and type-filtered widgets such as Top performing products stay empty. Modules are sent in config order, so terms go before posts.postsgets only product and variation IDs, up to 5,000; above that it resends all posts, because full sync stores its config in the status option, rewrites it after every batch and sends it injetpack_full_sync_start.term_relationshipscan't be limited, so it resends all relationships. The normal full syncs (site registration, the dashboard's initial sync, manual syncs) are unchanged.jetpack-premium-analyticsin that option, which enabling Premium Analytics alone doesn't store; that fix is in a separate PR.jetpack_premium_analytics_lookup_backfillrecords the run (scheduled_at,started_at), so it runs once per store and a second action for the same run does nothing.Sync_Status_Trackersetsjetpack_premium_analytics_lookup_data_syncedwhen a full sync covering the wholewoocommerce_analyticsmodule ends, even when the milestone already exists (on these stores it came from the plugin's sync). A store whose first analytics full sync runs under Premium Analytics sets it there and never checks.woocommerce-packagesPhan stubs for the Action Scheduler functions, and drop the suppressions they made unnecessary.Follows the structure of
RefundFixFullSyncin woocommerce/woocommerce-analytics#1823.Related product discussion/links
Does this pull request change what data or activity we track or use?
No. It resends lookup data Premium Analytics already syncs for new orders, and reads one products report from WordPress.com.
Testing instructions
jetpack_premium_analytics_lookup_backfillaction appears. Other admin pages don't schedule it.woocommerce_analytics,terms,term_relationshipsandposts(a list of product IDs) in its config and contextpremium_analytics_lookup_backfill:wp eval 'print_r( \Automattic\Jetpack\Sync\Modules::get_module( "full-sync" )->get_status() );'wp option get jetpack_premium_analytics_lookup_data_syncedholds a timestamp and the product, tax and coupon reports on the WooCommerce tab include past orders, and Top performing products lists product names.wp option delete jetpack_premium_analytics_lookup_data_synced jetpack_premium_analytics_lookup_backfill, open the dashboard and run the action. It setsjetpack_premium_analytics_lookup_data_syncedwithout starting a full sync.🤖 Generated with Claude Code