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

Stop preload while prewarmup is running #3925

Merged
merged 7 commits into from May 25, 2021
Merged

Stop preload while prewarmup is running #3925

merged 7 commits into from May 25, 2021

Conversation

engahmeds3ed
Copy link
Contributor

@engahmeds3ed engahmeds3ed commented May 19, 2021 •
edited

Description

We found when preload is enabled and the RUCSS option is enabled, that it's a big headache on the client's server so we need to delay preload till prewarmup finishes its work.

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Manually enable RUCSS when preload is working => should stop preload and start prewarmup
  • Manually enable RUCSS when preload is enabled but not working => should start prewarmup and when finished the preload will start.
  • Manually enable RUCSS while preload is disabled => should start prewarmup only
  • When preload is enabled and manually clicked the button for forcing preload from the top menu or settings dashboard while prewarmup is working => prewarmup will continue and nothing will happen with preload.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@engahmeds3ed engahmeds3ed self-assigned this May 19, 2021
Tabrisrp
Tabrisrp reviewed May 19, 2021
View reviewed changes
inc/API/preload.php Outdated Show resolved Hide resolved
@engahmeds3ed engahmeds3ed marked this pull request as ready for review May 19, 2021 19:59
@Tabrisrp Tabrisrp added type: bug Indicates an unexpected problem or unintended behavior severity: moderate Feature isn't working as expected but has work around to get same value labels May 19, 2021
@Tabrisrp
Copy link
Contributor

Can you update the checklist items, notably the How Has This Been Tested? section, to detail how you have tested this change?

@engahmeds3ed
Copy link
Contributor Author

I will do that tomorrow morning after adjusting the method maybe_cancel_preload to also cancel preload when RUCC option is enabled so if preload was working that should stop it.

@Tabrisrp Tabrisrp requested a review from a team May 21, 2021 13:11
@piotrbak
Copy link
Contributor

@engahmeds3ed I think we're good here. Please confirm that the below testing is enough:

  1. Prewarmup working + Preload triggered from the top menu = Preload not activated
  2. Prewarmup working + Activate the preload (it was disabled before) = Preload not activated
  3. Preload working + Activate RUCSS = Preload is stopped and Prewarmup starts
  4. Preload enabled but not working + Activate RUCSS = Prewarmup starts

After each scenario, the Preload is triggered.
testrail-report-182.pdf

@engahmeds3ed
Copy link
Contributor Author

Thanks @piotrbak
Yes, I think this is enough, just a small thing:
Preload also can be manually triggered from the dashboard button

image

so we need to make sure that point number 1 is valid with this button also.
(Not a big deal, both are sharing the same functionality)

Thanks.

@engahmeds3ed engahmeds3ed merged commit fd29255 into develop May 25, 2021
@engahmeds3ed engahmeds3ed deleted the feature/start_preload_after_prewarmup branch May 25, 2021 03:56
crystinutzaa pushed a commit that referenced this pull request Jun 3, 2021
@Tabrisrp @engahmeds3ed
@mostafa-hisham @NataliaDrause @GeekPress
3.9.0.1 (#3986)
48f5466 
* Fixes #3478 Enable installation through composer v2 (PR #3928)

* update woocommerce from wpackagist
* update woocommerce paths
* use composer v2 in GH actions

* Re-add change to remove disable preload fonts when CPCSS is enabled (PR #3927)

* remove callback disabling preload fonts
* remove associated tests

* Stop preload while prewarmup is running (PR #3925)

* start automatic preload just after prewarmup finishes its work
* Block manual preload till finishing prewarmup
* check the RUCSS option enabled along with allow_optimization
* fix old tests
* [WIP] stop preload with enabling RUCSS
* migrate maybe_cancel_preload to RUCSS admin subscriber
* fix old tests

* remove charset=UTF-8 (PR #3946)

* remove charset=UTF-8
* remove charset=UTF-8 from delete

* Add unit tests code coverage (PR #3922)

* add code coverage command
* update incorrect annotations
* add cove coverage action
* enable xdebug for coverage

* Fix failing tests related to adding prewarmup functionality (PR #3949)

* Guard our code by checking if table is exists before updating prewarmup
* With switch_theme only start scanner when RUCSS is enabled

* change trigger event to push

* Fixes RUCSS tests: update time to prevent tests failures (PR #3973)

* Fixes #3600 add wpstage to staging servers list (PR #3948)

* add wpstage to staging servers

* remove image optimization when WP_ROCKET_WHITE_LABEL_ACCOUNT (PR #3956)

* remove image optimization menu when WP_ROCKET_WHITE_LABEL_ACCOUNT is used

* Add Hubspot iframe inline JS exclusion from combine JS (PR #3943)

* Fixes #3945 Display warning when WP Meteor is enabled with WP Rocket delay JS (PR #3974)

* add new add_plugins_incompatibility() method
* add options class to settings class dependencies
* updating existing tests
* add add_plugins_incompatibility() test file
* add unit & integration tests

* Fixes #3924 Remove unnecessary more info link in RUCSS pre-warmup copy (PR #3981)

* Compatibility with Impreza theme (PR #3972)

* Add Impreza inline script pattern to exclude
* Add Fixtures

Co-authored-by: Rémy Perona <remperona@gmail.com>

* Update delay JS script (PR #3971)

* update delay JS script
* update delay JS fixture
* add touchchancel and touchforcechange events
* update fixture with new events

* Fixes #2876 Remove beacon button when white label is enabled (PR #3965)

* remove beacon btn if white label is true
* remove ask support button too

* Add additional delay JS exclusions (PR #3975)

* Add additional exclusions from delay JS
* Update fixtures

* update plugin version to 3.9.0.1

* update operator in version_compare

Co-authored-by: Ahmed Saed <eng.ahmeds3ed@gmail.com>
Co-authored-by: mostafa-hisham <mostafa.hisham.mahmoud@gmail.com>
Co-authored-by: Natalia Drause <60236665+NataliaDrause@users.noreply.github.com>
Co-authored-by: Jonathan Buttigieg <jonathan@wp-media.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tabrisrp Tabrisrp Tabrisrp approved these changes

@piotrbak piotrbak piotrbak approved these changes

Assignees

@engahmeds3ed engahmeds3ed

Labels
module: preload module: remove unused css needs: qa severity: moderate Feature isn't working as expected but has work around to get same value type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@engahmeds3ed @Tabrisrp @piotrbak