Skip to content

PTK: Check if theres a scheduled action before logging warning.#51143

Merged
tjcafferkey merged 3 commits intotrunkfrom
fix/register-ptk-patterns-warning-log
Sep 4, 2024
Merged

PTK: Check if theres a scheduled action before logging warning.#51143
tjcafferkey merged 3 commits intotrunkfrom
fix/register-ptk-patterns-warning-log

Conversation

@tjcafferkey
Copy link
Copy Markdown
Contributor

Submission Review Guidelines:

Changes proposed in this Pull Request:

The previous implementation would log a warning every time the patterns were empty, which could happen frequently due to the asynchronous nature of pattern fetching. This led to:

  1. Unnecessary noise in log files
  2. Potential performance impact from excessive logging
  3. Confusion for users and developers trying to diagnose issues

By only logging when patterns are empty and no fetch is scheduled, we ensure that warnings are only generated in genuinely problematic situations, such as when the pattern fetching mechanism has failed entirely.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Before checking out this branch:

  1. Delete transient ptk_patterns
  2. Load your homepage
  3. Check that the log is present in your log file

After checking out this branch:

  1. Delete transient ptk_patterns
  2. Load your homepage
  3. Check that the log is not present in your log file

Changelog entry

  • Automatically create a changelog entry from the details below.
  • This Pull Request does not require a changelog entry. (Comment required below)
Changelog Entry Details

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Changelog Entry Comment

Comment

@tjcafferkey tjcafferkey added Bug The issue is a confirmed bug. focus: patterns labels Sep 4, 2024
@tjcafferkey tjcafferkey self-assigned this Sep 4, 2024
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Sep 4, 2024
@tjcafferkey tjcafferkey requested a review from gigitux September 4, 2024 12:33
@tjcafferkey tjcafferkey marked this pull request as ready for review September 4, 2024 12:33
@woocommercebot woocommercebot requested a review from a team September 4, 2024 12:34
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Sep 4, 2024

Hi @gigitux, @woocommerce/woo-fse

Apart from reviewing the code changes, please make sure to review the testing instructions and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Sep 4, 2024

Test using WordPress Playground

The changes in this pull request can be previewed and tested using a WordPress Playground instance.
WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Test this pull request with WordPress Playground.

Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit.

Copy link
Copy Markdown
Contributor

@gigitux gigitux left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix! The PR LGTM! I left a minor comment about the code comment, but feel free to merge as it is if you prefer!

@tjcafferkey tjcafferkey enabled auto-merge (squash) September 4, 2024 12:50
@tjcafferkey tjcafferkey merged commit c074c55 into trunk Sep 4, 2024
@tjcafferkey tjcafferkey deleted the fix/register-ptk-patterns-warning-log branch September 4, 2024 13:23
@github-actions github-actions bot added this to the 9.4.0 milestone Sep 4, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Sep 4, 2024
@nigeljamesstevenson nigeljamesstevenson added status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Sep 4, 2024
@RiaanKnoetze
Copy link
Copy Markdown

Related: https://wordpress.org/support/topic/ptk-pattern-store/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug The issue is a confirmed bug. plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants