Fix 4472 - Display notice when DelayJS is activated with Autoptimize Aggregate JS already in effect

[iCaspar]

Description

Please include a summary of the change and which issue is fixed/closed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #4472
Fixes #3707

Type of change

• Enhancement (non-breaking change which improves an existing functionality)

Is the solution different from the one proposed during the grooming?

Yes
In the grooming I had thought we would be adding a helper notice into the DelayJS selection field. In fact, we are adding a notice to the notices area at the top of the page.
Also, to do this, we had been adding the notice via the 'settings_errors transient, but that was failing to display messages properly. Instead, this adds a method in the DelayJS Admin/Subscriber to check if Autoptimize Aggregate JS is active, and if so, uses add_settings_errors() for display.

How Has This Been Tested?

• New Integration tests to confirm that the notice is added under expected conditions.
• Check that notice is displayed properly on local environment.

Checklist:

Please delete the options that are not relevant.

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• 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

[iCaspar]

This approach should work, as the integration tests prove that notice data is added to the settings_errors transient array. But the notice is not appearing, due to the same bug that is causing #4314, namely there is something preventing any notice being displayed from the settings_errors transient when settings are updated.

Marking this as blocked by #4314.

[Mai-Saad]

@iCaspar Please find notes below based on test analysis and exploratory test:

1- If Delay JS was enabled, then we enable auto-optimize with the aggregate JS features => no warning will be displayed (we will need to re-enable delay js to see the warning), can we display the warning without needing to re-enable Delay JS (maybe with cache clear, refresh settings)?

2- When Autooptimize aggregate js files and inline js is enabled along with WPR LL video and preview image, result in removing LL videos script from HTML => Do we need a separate GH issue for that? or is it something we can handle on this PR? (same already on trunk)

3- @piotrbak Q: Why not handle the current conflict, the same way as we do with minify/combine CSS/JS (disable the option and display red msg)?

4- Just for UI confirmation => Current msg which is displayed only when enable delay js while aggregate options are enabled in auto-optimize is as attached, is this what we need?

[piotrbak]

@Mai-Saad When it comes to the 3:
Minify and Combine CSS/JS are exactly the same features. It's not the case with DelayJS, as it's not causing errors, rather not working as good as it should, we decided to go with a educative warning approach.

@iCaspar Can we make the warning border yellow instead of blue?

[piotrbak]

@Mai-Saad

For 2:
Does it cause any issues with the video or lazyload?

For 4:
We're going to display the message when:

1. Delay JS is enabled
2. Autoptimize JS Aggregation is enabled

CSS files are disabled because the CSS optimisations are enabled in the Autoptimize, below we have an explanation

[iCaspar]

@piotrbak @Mai-Saad
On 1, please confirm that the User Story above is accurate. Per the current version of the story:
We will show the message when Autoptimize>JS Aggregation is already active and then DelayJS is activated (the behavior Mai observed). If we also want it to appear when the events happen in the reverse order (DelayJS is already active and then JSAggregation is activated) we should specify that behavior in the story. Otherwise, I think it is working as intended here.

On 2, interaction with Lazyload -- this PR is ONLY about messaging -- see the User Story. It will not include any fixes for conflicts with LazyLoad (or anything else).

On 3, see 2.

On 4, the message is (again) specified in the User Story.

[piotrbak]

@iCaspar Will it be possible to cover both scenarios? We want to display the warning in all possible use cases. The same should be applied to other Autoptimize compatibilities. Sorry for the misunderstanding.

[iCaspar]

@piotrbak No problem. Let's update the User Story and I can make that change this morning.

[engahmeds3ed]

Small minor comments

[Mai-Saad]

Working as expected.

As per discussion, we agreed on the following notes:

• Notice won't be displayed after dismissing with plugin(s) reactivation (with same settings), import same settings
• Notice will be normally displayed when delay js is enabled independent of exclusions (i.e with default exclusions, notice is displayed)
• When enabling autoptimize while LL video is enabled, LL video will be automatically unchecked (this is already on the trunk and added ticket for this)
  testrail-report-311.pdf