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 #3947: Add a notice on request fail on SAAS server from RUCSS #5240

Merged
merged 45 commits into from
Jul 13, 2023

Conversation

CrochetFeve0251
Copy link
Contributor

Description

In this PR we had a notice for the user to check if the IP from the SAAS server is authorized when we can't reach it.
For that we did a check on the RUCSS API client and linked a notice to it.

Fixes #3947

Type of change

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

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

No instead of an option I used a transient to ensure it will disappear in the time and not stay for months on the admin from the user.

How Has This Been Tested?

  • Automated Test
  • On my local env

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

@CrochetFeve0251 CrochetFeve0251 added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement module: remove unused css labels Jul 13, 2022
@CrochetFeve0251 CrochetFeve0251 self-assigned this Jul 13, 2022
@CrochetFeve0251 CrochetFeve0251 requested a review from a team July 18, 2022 09:01
@CrochetFeve0251 CrochetFeve0251 marked this pull request as ready for review July 18, 2022 09:01
@remyperona remyperona added this to the 3.11.6 milestone Jul 19, 2022
@Mai-Saad Mai-Saad self-requested a review August 26, 2022 15:35
@Mai-Saad
Copy link
Contributor

Mai-Saad commented Aug 26, 2022

@CrochetFeve0251 @piotrbak please find exploratory test notes/Qs below:

  • Success/process notice is displayed with the blocking notices once enable RUCSS.
    Screenshot from 2022-08-26 18-46-22
    Screenshot from 2022-08-26 17-38-17

  • Shouldn't it be just a notice (blue) not an error (red) like CPCSS? and shouldn't it be dismissable?

  • Blocked notice is not displayed if we clear used CSS from the admin bar => I think it shall be displayed. what do you think?

  • Do we need to do anything if we clear used CSS for this URL from page view? @piotrbak

  • Enhancement for the notice UI, msg just beside WP Rocket + the options to allow IP to be bullet points like that below: What do you think?

WP Rocket: It seems a security plugin or the server's firewall prevents WP Rocket from accessing the Remove Unused CSS generator. The following IP address 135.125.83.227 should be allowlisted:

  • In the security plugin, if you are using one
  • The server's firewall - your host can help you with this

@piotrbak
Copy link
Contributor

piotrbak commented Sep 9, 2022

  1. Success/process notice is displayed with the blocking notices once enable RUCSS.
    We should not display the success message when the Used CSS is not created for the homepage. The requests are blocked, so the Used CSS is not generated.
  2. Shouldn't it be just a notice (blue) not an error (red) like CPCSS? and shouldn't it be dismissable?
    It should be red
  3. Blocked notice is not displayed if we clear used CSS from the admin bar
    While on the dashboard page, we should display the red notice unless it's dismissed. Scenario should be handled:
  • RUCSS is enabled and working
  • Requests are blocked in wp-config.php or in the firewall
  • We clear Used CSS
  • The notice is displayed in the dashboard.
  1. Do we need to do anything if we clear used CSS for this URL from page view?
    No, the notice should be displayed only in our dashboard.
  2. Enhancement for the notice UI, msg just beside WP Rocket + the options to allow IP to be bullet points like that below: What do you think?
    Yes, totally agree about the bullet points 👍

…-fail-RUCSS' into fix/3947-add-a-notice-on-request-fail-RUCSS

# Conflicts:
#	inc/Engine/Optimization/RUCSS/Admin/Settings.php
@CrochetFeve0251
Copy link
Contributor Author

CrochetFeve0251 commented Sep 12, 2022

  1. Success/process notice is displayed with the blocking notices once enable RUCSS.
    We should not display the success message when the Used CSS is not created for the homepage. The requests are blocked, so the Used CSS is not generated.

    1. Shouldn't it be just a notice (blue) not an error (red) like CPCSS? and shouldn't it be dismissable?
      It should be red

    2. Blocked notice is not displayed if we clear used CSS from the admin bar
      While on the dashboard page, we should display the red notice unless it's dismissed. Scenario should be handled:

    • RUCSS is enabled and working

    • Requests are blocked in wp-config.php or in the firewall

    • We clear Used CSS

    • The notice is displayed in the dashboard.

    1. Do we need to do anything if we clear used CSS for this URL from page view?
      No, the notice should be displayed only in our dashboard.

    2. Enhancement for the notice UI, msg just beside WP Rocket + the options to allow IP to be bullet points like that below: What do you think?
      Yes, totally agree about the bullet points +1

For 3. if it doesn't show up that means we didn't done any request and if so it means that we have no way to know if we will have an error. For the me in this case the notice doesn't have to display.

For 4. do we need to add custom css if we want a bullet list do we still need to had it.

@piotrbak
Copy link
Contributor

@CrochetFeve0251 The list without bullets doesn't look good, but we have the same behaviour for all our notices, let's don't change this for now. There's an improvement to do though:

  1. Make sure each
  2. starts from he capitilised letter
  3. In the server's firewall instead of the server's firewall

I'll consult wording here and we might change it on the develop though.

Copy link
Contributor

@engahmeds3ed engahmeds3ed left a comment

Choose a reason for hiding this comment

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

Small enhancements :)

Also, I think we need to add the transient name here:

'wpr_dynamic_lists_delayjs',

to be removed with plugin uninstallation.

inc/Engine/Optimization/RUCSS/Admin/Settings.php Outdated Show resolved Hide resolved
inc/Engine/Optimization/RUCSS/Admin/Subscriber.php Outdated Show resolved Hide resolved
Comment on lines 500 to 502
public function has_error_notice() {
return (bool) get_transient( 'wp_rocket_rucss_errors_count' );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why not moving this method to the settings page and use it inside can_display_notice method?
This will save you from the above bailouts in methods display_processing_notice and display_success_notice.

Also we can move the above bailout in the method display_error_notice to be inside the settings class also.

What do u think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@engahmeds3ed For moving that method there an issue. display_error_notice also use it.
Then yes we could move that logic to the Settings class.

inc/Engine/Optimization/RUCSS/Admin/Settings.php Outdated Show resolved Hide resolved
inc/Engine/Optimization/RUCSS/Admin/Settings.php Outdated Show resolved Hide resolved
$reason_2_message = __( "In the server's firewall - your host can help you with this", 'rocket' );

$message = sprintf(
// translators: %1$s = plugin name.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is needed here

inc/Engine/Optimization/RUCSS/Admin/Settings.php Outdated Show resolved Hide resolved
inc/Engine/Optimization/RUCSS/Admin/Subscriber.php Outdated Show resolved Hide resolved
@vmanthos
Copy link
Contributor

  1. If, while RUCSS is blocked, the user clears the used CSS the notice is displayed:
    image

If there isn't any used CSS in wpr_rucss_used_css to clear maybe we shouldn't display the notice.
Note that if blocking of the SaaS came after Remove Unused CSS has been working for a while, it makes sense to display the notice.

@piotrbak What do you think?

  1. Also, when the blocking is lifted, but the notice hasn't been dismissed it will not be automatically dismissed.
    It will be displayed until a successful request is made to the SaaS.

Is there anything we can/want to do about this?

  1. About the bullets in this comment, it's my understanding we won't go with the CSS changes.

Instead, maybe we can prefix each line with a dash: -
image

@piotrbak
Copy link
Contributor

@vmanthos
If there isn't any used CSS in wpr_rucss_used_css to clear maybe we shouldn't display the notice.
I think that's a good enhancement for the future.

Also, when the blocking is lifted, but the notice hasn't been dismissed it will not be automatically dismissed. It will be displayed until a successful request is made to the SaaS.
That sounds totally okay, no changes needed.

Instead, maybe we can prefix each line with a dash: -
Yes, that sounds reasonable 🆗

@vmanthos
Copy link
Contributor

Thank you, @piotrbak! 🙏

@CrochetFeve0251 We should implement item number 3, here:
#5240 (comment)

@Mai-Saad
Copy link
Contributor

Mai-Saad commented Jul 13, 2023

@CrochetFeve0251 Thanks for the updates. the transient is deleted now after deleting WPR and the - is added.
Remain the following notes:
1- Currently, when the notice is displayed:
if user 1 dismisses it, notice will never be displayed again to user 1, and whatever actions triggering it are made.
User 2, will see the notice till user 2 dismisses it. => Nothing is needed here as per discussion with @piotrbak
2- We recommend change - In the server's firewall - your host can help you with this to - In the server's firewall, your host can help you with this or - In the server's firewall. Your host can help you with this => @piotrbak what do you think?

@piotrbak
Copy link
Contributor

@CrochetFeve0251 Sorry for the wording changes... We'll go with this one:
In the server's firewall. Your host can help you with this

Copy link
Contributor

@Mai-Saad Mai-Saad left a comment

Choose a reason for hiding this comment

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

Working as expected
testrail-report-502.pdf

@Mai-Saad Mai-Saad added this pull request to the merge queue Jul 13, 2023
Merged via the queue into develop with commit c342e10 Jul 13, 2023
@Mai-Saad Mai-Saad deleted the fix/3947-add-a-notice-on-request-fail-RUCSS branch July 13, 2023 13:51
@piotrbak
Copy link
Contributor

piotrbak commented Jul 17, 2023

Since the requirements have changed, we need the wording to be as follows:

It seems a security plugin or the server's firewall prevents WP Rocket from accessing the Remove Unused CSS generator. IPs listed here in our documentation should be added to your allowlists:
 - In the security plugin, if you are using one
 - In the server's firewall. Your host can help you with this

here in our documentation will be linking to https://docs.wp-rocket.me/article/1529-remove-unused-css#basic-requirements

The French doc is here:
https://fr.docs.wp-rocket.me/article/1577-supprimer-les-ressources-css-inutilisees#basic-requirements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: remove unused css type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RUCSS - Display a notice when the API URL cannot be accessed
7 participants