Skip to content

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Apr 14, 2023

Proposed changes:

This registers a new "Tools > Advertising" menu, appearing when your site is eligible for Blaze.

The contents of the menu will be loaded remotely from Calypso.

This borrows from the Stats Admin package, where we've implemented something similar (the Stats page is loaded from Calypso).

To do

  • Add tests
  • Figure out if the menu location should change in different environments (e.g. sites that do not use the Jetpack plugin, but may require Blaze on its own). peeHDf-Zg-p2
  • Maybe add gating so the menu is not publicly accessible until this is ready for prime-time.
  • Figure out if this menu should be hidden or handled differently on WordPress.com sites or WoA sites, where we already have the Tools > Advertising menu. peeHDf-Zg-p2

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?
  • Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

  • No

Testing instructions:

  1. Ensure that Blaze links still appear under the published posts in Posts > All Posts, and that the links work.
  2. Ensure that you're still offered to Blaze a post in the post-publish panel in the editor (after publishing a post), and that the Blaze link properly redirects you to the Advertising dashboard in Calypso.
  3. Add add_filter( 'jetpack_blaze_dashboard_enable', '__return_true' ); to a functionality plugin on your site.
  4. You should see a new Tools > Advertising menu when your site is connected to WordPress.com.

To populate the contents of the dashboard, you have 2 options:

  • You can patch D112493-code in your WordPress.com sandbox.
  • You can build your own copy of the dashboard:
    • Build Blaze and Jetpack: jetpack build packages/blaze && jetpack build plugins/jetpack
    • Clone https://github.com/Automattic/wp-calypso
    • Install deps with yarn
    • Go to apps/blaze-dashboard
    • Launch the app with BLAZE_DASHBOARD_PACKAGE_PATH=/.../jetpack/projects/packages/blaze yarn dev, while entering the correct path to your package on your machine.

  1. Visit the Tools > Advertising menu on your local installation.
  2. Now re-test items 1 and 2 of this list; they should still work, but the links should now point to the wp-admin dashboard.

The menu itself will be loaded remotely from Calypso.
@jeherve jeherve self-assigned this Apr 14, 2023
@github-actions github-actions bot added [Package] Blaze [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Apr 14, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 14, 2023

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ⚠️ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

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 🤖


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.


Jetpack plugin:

  • Next scheduled release: July 5, 2023.
  • Scheduled code freeze: June 26, 2023.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 14, 2023

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

  • To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack plugin. Once the plugin is active, go to Jetpack > Jetpack Beta > Jetpack and enable the add/blaze-dashboard-menu branch.

  • To test on Simple, run the following command on your sandbox:

    bin/jetpack-downloader test jetpack add/blaze-dashboard-menu
    

Interested in more tips and information?

  • In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
  • Read more about our development workflow here: PCYsg-eg0-p2
  • Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

@jeherve jeherve marked this pull request as ready for review May 31, 2023 08:48
@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed DO NOT MERGE don't merge it! labels May 31, 2023
jeherve added 4 commits May 31, 2023 10:56
@sbarbosa
Copy link
Contributor

Ok @jeherve, @sergeymitr. I filtered the wrong parameters that we were sending to WordPress.com API.

I used the same approach as the commit mentioned by Jeremy; the only problem was that it only worked when the route didn't expect any query parameters.
For the rest of the routes, I had to change the initial regex to include the possibility of those query parameters (without this, I was getting 404 from Jetpack for requests like index.php?rest_route=/jetpack/v4/blaze-app/sites/[site_id]/blaze/posts?page=1&order_by=modified&order=desc ).

Let me know what you think and if there is a better way to consider the query params in the register_rest_route when the site is using plain permalinks.

@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Jun 21, 2023
@samiff samiff added this to the jetpack/12.3 milestone Jun 21, 2023
Copy link
Contributor

@samiff samiff left a comment

Choose a reason for hiding this comment

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

This tested well for me on my local dev site, even trying with plain permalinks. Two quick questions.

  1. Are Woo products intended to show the Blaze option?
  2. Where are we tracking issues with the Advertising page contents? For example, one of the buttons seems off on my end:
    Screenshot 2023-06-21 at 2 59 38 PM

@samiff samiff added [Status] Needs Testing We need to add this change to the testing call for this month's release [Status] UI Changes Add this to PRs that change the UI so documentation can be updated. labels Jun 21, 2023
@jeherve
Copy link
Member Author

jeherve commented Jun 22, 2023

  1. Are Woo products intended to show the Blaze option?

Yes. Blaze supports posts, pages, and products.

Where are we tracking issues with the Advertising page contents? For example, one of the buttons seems off on my end:

I think posting in #blaze-dashboard-redesign about it is fine, but since you mentioned it here @sbarbosa will know what to do with it :)

Co-authored-by: Samiff <samiff@users.noreply.github.com>
@jeherve jeherve requested a review from sergeymitr June 22, 2023 09:18
@sbarbosa
Copy link
Contributor

Thanks for the test @samiff. You can have an updated version of the dashboard in this Calypso PR:
Automattic/wp-calypso#78209

You can follow the testing steps in that PR to see that page version. I adjusted some styles and changed the header.

I will update the Phabricator diff when I merge that PR.

@sergeymitr
Copy link
Contributor

I noticed a couple of potential issues with the /wpcom/v2/sites/%d/blaze/posts endpoint.

I have permalinks set to "plain", meaning the page URL's looks like /?p=123.
Calypso page list takes that into account, using the /?page_id=123 links.
However, when I go to https://wordpress.com/advertising/[site-url], it tries to use post-name permalinks (e.g. /cart/), which lead to 404.
So for some reason the endpoint doesn't take permalink settings into account.

Another problem appeared on a new JN site where the same page started displaying "About" page which never existed on that website (clean JN site with only Jetpack installed).
Then I installed WooCommerce, which created a bunch of pages, but they never appeared in Calypso's "Tools -> Advertising", only displaying that non-existing "About" page.

The blaze/posts endpoint does not see them, but sees some other page that never existed (ID: 1):
Screen Shot 2023-06-22 at 10 10 32

Pages in the database.
Post ID: 1 is the default "Hello World" that gets created automatically. Never been edited.
Screen Shot 2023-06-22 at 10 05 28

They show up in "Calypso -> Posts", so they did get synced and WPCOM knows about them:
Screen Shot 2023-06-22 at 10 12 09

@sergeymitr sergeymitr 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 Jun 22, 2023
@jeherve
Copy link
Member Author

jeherve commented Jun 22, 2023

I noticed a couple of potential issues with the /wpcom/v2/sites/%d/blaze/posts endpoint.

These may be sync issues since the endpoint does a direct WP_Query on WordPress.com (see the WPCOM_REST_API_V2_Endpoint_Blaze class on WordPress.com). Do you think you could take a look at that?

Since those issues are not directly related to this PR, but to the endpoint it is using, and since this PR doesn't surface the UI just yet (it's still hidden behind a filter), do you think we could move forward with merging the PR?

Copy link
Contributor

@samiff samiff left a comment

Choose a reason for hiding this comment

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

since this PR doesn't surface the UI just yet (it's still hidden behind a filter), do you think we could move forward with merging the PR?

I think that's a good plan of action since we've also got some other dependent PRs.

@samiff samiff added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Jun 22, 2023
@sergeymitr sergeymitr dismissed their stale review June 22, 2023 19:54

My concerns are discussed and dismissed.

@jeherve jeherve merged commit 5a3083b into trunk Jun 23, 2023
@jeherve jeherve deleted the add/blaze-dashboard-menu branch June 23, 2023 07:10
@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jun 23, 2023
@sdixon194 sdixon194 removed the [Status] Needs Testing We need to add this change to the testing call for this month's release label Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Blaze [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Pri] Normal [Status] UI Changes Add this to PRs that change the UI so documentation can be updated. [Tests] Includes Tests [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants