-
Notifications
You must be signed in to change notification settings - Fork 851
Reduce time-to-first-use for Blaze #46500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce time-to-first-use for Blaze #46500
Conversation
Query local database when Jetpack Sync is not ready, reducing time-to-first-use for Blaze. Falls back to WPCOM API when sync is complete for full stats functionality.
|
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! |
Code Coverage SummaryCoverage changed in 1 file.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
projects/packages/blaze/src/class-dashboard-rest-controller.php
Outdated
Show resolved
Hide resolved
|
@sbarbosa , I have a few questions related to this PR:
|
It's true that we don't have much unit testing in this module. It would be nice to add some, at least for these new funcitons that we are creating. The module will now have more internal logic as before, so it would be nice to cover part of it at least. But its probably fine also to move that task for the future too.
There are easier ways to test this. When we create the Jetpack PR, it will create a feature branch that you can easily test by installing the Jetpack Beta plugin. So technically, you can just:
I think this should test the real production steps. This comment (#46500 (comment)) also adds other ways to test the branch, but this change will only apply to self-hosted installations, so the testing steps I shared should be enough to test the sync delay fix. @EkaterinaStancheva, one thing to consider here is that this will need to be the last PR we merge. We can merge this before we implement all of the other changes, because if not, the user will see the post but will get errors the moment they try to promote a campaign. |
|
@sbarbosa Thank you very much for insights!
As we discussed yesterday, I added
I updated the PR description to add the testing steps you shared. I went through them, and everything works fine.
I added |
sbarbosa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good and works as described.
I tested the changes on the Blaze Ads plugin and also worked fine 👍🏻
j6ll
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @EkaterinaStancheva I tested the changes but seeing some behaviour that could be a bug.
No branch loaded - see the message as expected

Branch now loaded - don't see message, posts response is empty

Can see posts here (I created them before connecting Jetpack)

j6ll
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @EkaterinaStancheva for working on this - I left some minor comments to check. There are a couple I'm not 100% on, but worth a quick check.
projects/packages/blaze/src/class-dashboard-rest-controller.php
Outdated
Show resolved
Hide resolved
projects/packages/blaze/src/class-dashboard-rest-controller.php
Outdated
Show resolved
Hide resolved
projects/packages/blaze/src/class-dashboard-rest-controller.php
Outdated
Show resolved
Hide resolved
projects/packages/blaze/src/class-dashboard-rest-controller.php
Outdated
Show resolved
Hide resolved
@j6ll I’m testing by creating an ephemeral site with Jetpack Beta, pointing to this branch. And I haven’t experienced any issues. I’m not sure how to reproduce your test. How do you create the ephemeral site in a way that you can switch between branches? Do you think this could be a cache issue caused by switching between branches? @sbarbosa, I would also appreciate your thoughts. I tested multiple times and haven’t encountered any problems. However, I assume I might be missing something. 🤷♀️ |
sbarbosa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏻
I just deployed all of the related PRs, so we are now able to use this branch and run a full test (to detect if we missed a change in the DSP).
@EkaterinaStancheva, would it be possible to merge the trunk into this PR so we get the latest change to the create campaign endpoint?
That way, it will refresh the feature branch, and we will be able to use an ephemeral site for the test.
@sbarbosa I already merged the trunk branch into this one. The create campaign endpoint should be there. |
|
Awesome, @EkaterinaStancheva. I ran a clean test of the full flow and successfully created a campaign using data from an unsynced post. During the process, all of the related endpoints returned Didn't find any blockers related to the flow. I think we already fixed all of the affected endpoints. |
Hey @EkaterinaStancheva I do the following:
|
I am not sure about these testing steps. Technically, the Blaze Ads plugin has not yet been updated to this Jetpack Branch. So if you want to test these changes with the Blaze Ads plugin, you have two options:
In both scenarios, you will need to configure everything before connecting the site to Jetpack, because it usually takes a couple of minutes to sync a blank site. You will need to run the test quickly. Additionally, I am not sure why you are sandboxing the Blaze dashboard. We haven't made changes to the dashboard, so the test can be done with the production code (no need for a sandbox here). |
|
@sbarbosa @EkaterinaStancheva - Sorry folks, there's been a mix-up here. These testing instructions are not for this PR. My bad! |
j6ll
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worked great for me. I saw the post straight away. I no longer see a sync message. This is a great improvement. Thank you, Ekaterina!

Problem
Currently, Blaze Ads onboarding requires Jetpack Sync to complete before the plugin is usable, as all post queries are forwarded to the WPCOM API which doesn't have access to posts until sync completes:
Solution
get_blaze_poststo check sync status and route to local database queries when sync is not ready, or WPCOM API when sync is complete.get_dsp_blaze_poststo bypass DSP proxy and reuseget_blaze_postsfor consistent routing logic.Proposed changes:
get_blaze_postsnow checksare_posts_ready()and routes to local DB or WPCOM accordinglyget_dsp_blaze_postsmaps DSP parameters and reusesget_blaze_postsfor consistent routingJetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
get_dsp_blaze_postssync_readyresponse parameter. When ready, go through the steps above again and verify that everything works as expected.