Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Boost: Add active modules to speed score request #21471

Merged
merged 4 commits into from
Oct 25, 2021
Merged

Boost: Add active modules to speed score request #21471

merged 4 commits into from
Oct 25, 2021

Conversation

karthikax
Copy link
Contributor

@karthikax karthikax commented Oct 20, 2021 •
edited
Loading

Add Boost active modules data to page speed score request so that the same can be recorded as event properties while logging speed score data in Tracks.

Changes proposed in this Pull Request:

Jetpack product discussion

NA

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

Not at the moment. However, this PR sends the active modules list to the page speed score request.
The request to API happens as an async job. Update to the async job changes the data we track.
Reference to the async job update: D68731

Testing instructions:

  • Setup WordPress.com sandbox
  • Prepare your sandbox to test the API: FG /sandboxes/testing-apis-in-your-sandbox/
  • Apply this patch in your sandbox: D68731
  • Navigate to Jetpack Boost settings page: /wp-admin/admin.php?page=jetpack-boost
  • Click the Refresh button under the "Overall score" section.
  • Once the updated score is displayed, wait for 10 to 12 mins.
  • Verify that the events jetpack_boost_pagespeed_result and jetpack_boost_pagespeed_opportunity has the event properties corresponding to the active modules in the following format: module_critical_css: true

@karthikax karthikax added [Status] In Progress [Plugin] Boost A feature to speed up the site and improve performance. labels Oct 20, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Oct 20, 2021 •
edited
Loading

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 you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Boost plugin:

  • Next scheduled release: November 30, 2021.
  • Scheduled code freeze: November 23, 2021.

@karthikax karthikax marked this pull request as ready for review October 21, 2021 14:08
@karthikax karthikax added [Status] Needs Team Review Obsolete. Use Needs Review instead. and removed [Status] In Progress labels Oct 21, 2021
@davidlonjon davidlonjon added the [Boost Feature] Speed Score issues related to Jetpack Boost's Speed Score label Oct 22, 2021
davidlonjon
davidlonjon reviewed Oct 22, 2021
View reviewed changes
Copy link
Contributor

@davidlonjon davidlonjon left a comment

Choose a reason for hiding this comment

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

This looks good to me. I cannot 100% test it as I don't have a WPCom sandbox so I'll let someone else on the team to give the green light.

haqadn
haqadn previously requested changes Oct 22, 2021
View reviewed changes
pyronaur
pyronaur reviewed Oct 25, 2021
View reviewed changes
projects/plugins/boost/app/lib/class-speed-score-request.php
*
* @var array $active_modules Active modules.
*/
private $active_modules;
Copy link
Member

Choose a reason for hiding this comment

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

The constructor already does this, but perhaps worth adding that here as well - private $active_modules = [] to be clear and ensure that the default value is an empty array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since 3 other properties do not declare the default properties, I think not declaring the default value will have better code consistency
Also, as you mentioned, the constructor already does this and the assigned default value will never be used before that, I don't think this is needed.
Screenshot 2021-10-25 at 12 36 59 PM

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good! 👍

@pyronaur pyronaur dismissed haqadn’s stale review October 25, 2021 06:54

Suggestions applied

@karthikax karthikax enabled auto-merge (squash) October 25, 2021 12:29
@karthikax karthikax merged commit f29c4d2 into master Oct 25, 2021
@karthikax karthikax deleted the update/speed-score-tracking branch October 25, 2021 12:43
@github-actions github-actions bot added this to the boost/1.3.1 milestone Oct 25, 2021
@github-actions github-actions bot removed the [Status] Needs Team Review Obsolete. Use Needs Review instead. label Oct 25, 2021
davidlonjon added a commit that referenced this pull request Oct 26, 2021
@davidlonjon
Merge branch 'master' into add/boost-e2e-ci-integration
b4a66c0 
* master: (27 commits)
  Admin Page: Update upgrade buttons Tracks events (#21483)
  Debug Helper IDC Simulator: Add the ability to spoof the home url (#21516)
  Map block: Add `address` block attribute (#21412)
  Update wordpress monorepo (#21522)
  Stats Page: Add VideoPress nudge (#21513)
  Improve Jetpack products illustrations file size (#21514)
  Update dependency mediawiki/mediawiki-codesniffer to v38 (#21523)
  Update dependency yoast/phpunit-polyfills to v1.0.2 (#21524)
  Widgets: add Strava to Social Icons Widget and Social Menu (#21518)
  Boost: Add active modules to speed score request (#21471)
  Markdown: Better accessibility for Footnotes (#21495)
  Jetpack Plugin: Add VideoPress upgrade page (#21497)
  Admin Menu: moves Add new (plugin) menu item to the top for Atomic sites (#21506)
  General: Check the return value of get_comment() (#21080)
  Instagram Widget: link to user connection if user is not connected (#21512)
  Search: Create a new search package, rename Search plugin (#21502)
  Identity Crisis: Remove the unused jetpack_idc_option transient (#21492)
  Stats: do not update the wpcom blog details (#21441)
  Admin: do not display upgrade messages when on offline mode (#21444)
  jetpack: Catch calls to `exit()` and `die()` in PHPUnit (#21043)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidlonjon davidlonjon davidlonjon left review comments

@pyronaur pyronaur pyronaur approved these changes

@haqadn haqadn haqadn left review comments

@thingalon thingalon Awaiting requested review from thingalon

Assignees
No one assigned
Labels
[Boost Feature] Speed Score issues related to Jetpack Boost's Speed Score [Plugin] Boost A feature to speed up the site and improve performance.
Projects
None yet
Milestone
boost/1.3.1
Development

Successfully merging this pull request may close these issues.
4 participants
@karthikax @davidlonjon @pyronaur @haqadn