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.

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

Fix #6526 LCP/ATF messages that is not working correctly #6562

Conversation

Khadreal
Copy link
Contributor

@Khadreal Khadreal commented Apr 16, 2024

Description

Fixed ATF/LCP message not showing

Fixes #6526

Documentation

User documentation

Explain how this code impacts users.

Technical documentation

Explain how this code works. Diagram & drawings are welcomed.

Type of change

  • Bug fix (non-breaking change which fixes an issue).

New dependencies

List any new dependencies that are required for this change.

Risks

List possible performance & security issues or risks, and explain how they have been mitigated.

Checklists

Feature validation

  • I validated all the Acceptance Criteria. If possible, provide sreenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I implemented built-in tests to cover the new/changed code.

Code style

  • I wrote self-explanatory code about what it does.

Copy link

codacy-production bot commented Apr 16, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-0.03% (target: -0.10%) 30.00% (target: 50.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (1c4062a) 36764 8212 22.34%
Head commit (e523287) 36828 (+64) 8216 (+4) 22.31% (-0.03%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#6562) 10 3 30.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

The deprecated coverage status is not being sent today as part of the scheduled brownout. Know more

@Khadreal Khadreal requested a review from a team April 16, 2024 16:15
@Khadreal Khadreal linked an issue Apr 16, 2024 that may be closed by this pull request
@remyperona
Copy link
Contributor

Changes LGTM, can you update:

  • The PR title to respect our naming convention
  • The checklist
  • The assignee, label, milestone

@Khadreal Khadreal changed the title Bug/lcp atf messages#6526 Fix #6526 LCP/ATF messages that is not working correctly Apr 17, 2024
@Khadreal Khadreal self-assigned this Apr 17, 2024
@Khadreal Khadreal added type: bug Indicates an unexpected problem or unintended behavior lcp labels Apr 17, 2024
@Mai-Saad
Copy link
Contributor

Mai-Saad commented Apr 19, 2024

@Khadreal Thanks for the PR. Please find exploratory test notes below:

  1. The successful old RUCSS notice with a fresh install of 3.16 is still displayed
  2. The new notices when enabling RUCSS aren't displayed => counter https://github.com/wp-media/wp-rocket/pull/6562/files#diff-25f931bcb7ef216b9bad8ed4e4827d5207d72c450cc54a27e5e0db362be9e8cdR104 and success https://github.com/wp-media/wp-rocket/pull/6562/files#diff-25f931bcb7ef216b9bad8ed4e4827d5207d72c450cc54a27e5e0db362be9e8cdR147
  3. @piotrbak Do we need any updates in the notice for nulled license https://github.com/wp-media/wp-rocket/pull/6562/files#diff-25f931bcb7ef216b9bad8ed4e4827d5207d72c450cc54a27e5e0db362be9e8cdR232 ?

For testing the PR

@jeawhanlee
Copy link
Contributor

jeawhanlee commented Apr 19, 2024

@Khadreal

Something overlooked during the grooming:

we can do something like this: if ( false !== $transient || ( ! $this->options->get( 'remove_unused_css', 0 ) ) )

Somethings that was not implemented in this PR:

@Khadreal
Copy link
Contributor Author

@jeawhanlee not just that, the get_transient is still referencing the old value on line 138. I've changed that and couple of other things

@Khadreal Khadreal requested review from remyperona and a team April 19, 2024 08:57
@MathieuLamiot
Copy link
Contributor

@Khadreal I checked [the test plan shared by Mai above]. There are issues for 2 tests related to this PR I think. Can you check? (https://wpmediaqa.testrail.io/index.php?/runs/view/842&group_by=cases:section_id&group_order=asc):

Enregistrement.de.l.ecran.2024-04-24.a.19.15.19.mov
image

@Khadreal Khadreal requested review from remyperona and a team April 25, 2024 15:17
Copy link

codacy-production bot commented Apr 25, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+15.97% (target: -0.10%) 45.45% (target: 50.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (1c4062a) 36764 8212 22.34%
Head commit (6af9077) 36974 (+210) 14164 (+5952) 38.31% (+15.97%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#6562) 11 5 45.45%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy will stop sending the deprecated coverage status from June 5th, 2024. Learn more

@MathieuLamiot
Copy link
Contributor

@MathieuLamiot MathieuLamiot merged commit b935c75 into feature/lcp-above-the-fold-optimization Apr 25, 2024
11 of 13 checks passed
@MathieuLamiot MathieuLamiot deleted the bug/lcp-atf-messages#6526 branch April 25, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Messages of LCP/ATF isnot working correctly
5 participants