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

RUCSS - Display a notice when the API URL cannot be accessed #3947

Closed
vmanthos opened this issue May 25, 2021 · 7 comments · Fixed by #5240
Closed

RUCSS - Display a notice when the API URL cannot be accessed #3947

vmanthos opened this issue May 25, 2021 · 7 comments · Fixed by #5240
Assignees
Labels
effort: [S] 1-2 days of estimated development time module: remove unused css needs: copywriting needs: documentation Issues which need to create or update a documentation priority: low Issues that can wait type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@vmanthos
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Currently, if a firewall is blocking outgoing connections to our RUCSS generator, nothing will be displayed to the user. This means a customer will have to reach out to our support for a solution.

Describe the solution you'd like

We should consider displaying error messages related to the API the way we already do for CPCSS.
Those will include URLs to our documentation and should reduce the number of tickets about RUCSS API issues.

Additional context

Slack discussion: https://wp-media.slack.com/archives/C43T1AYMQ/p1621863311062000

@piotrbak piotrbak added module: remove unused css type: enhancement Improvements that slightly enhance existing functionality and are fast to implement labels May 28, 2021
@GeekPress GeekPress added needs: copywriting priority: high Issues which should be resolved as quickly as possible labels Jul 22, 2021
@GeekPress
Copy link
Contributor

@vmanthos @webtrainingwheels Do we have a doc about that particular problem we could redirect the user in the notice?

By the way, do you have any ideas about the text in this notice?

@vmanthos
Copy link
Contributor Author

Do we have a doc about that particular problem we could redirect the user in the notice?
We can redirect them here:
https://docs.wp-rocket.me/article/1529-remove-unused-css#basic-requirements
where we mention they need to allowlist the RUCSS IP.

do you have any ideas about the text in this notice?

The following could work:

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

If you prefer the message to be in one line maybe this will do:

It seems a security plugin or the server's firewall prevents WP Rocket from accessing the Remove Unused CSS generator. Please allowist the following IP address 135.125.83.227 in your security plugin, if you are using one. Also, contact your host to allowlist the IP from the sever's firewall.

@webtrainingwheels may have a way to make this fancier/shorter.

@GeekPress
Copy link
Contributor

@vmanthos Thanks a lot for the text. We can use the first one which will be easier to read.

@GeekPress GeekPress added this to the 3.9.3 milestone Aug 2, 2021
@mostafa-hisham mostafa-hisham added GROOMING IN PROGRESS Use this label when the issue is currently being groomed. needs: grooming and removed needs: grooming GROOMING IN PROGRESS Use this label when the issue is currently being groomed. labels Aug 3, 2021
@iCaspar iCaspar added the GROOMING IN PROGRESS Use this label when the issue is currently being groomed. label Aug 9, 2021
@iCaspar
Copy link
Contributor

iCaspar commented Aug 9, 2021

Reproduce the Problem:
We do not currently display any notices when resources are not fetched and WP_ROCKET_RUCSS_DEBUG is not set.

Scope a solution:
Possible Solution 1:
We have a similar (nearly identical) issue #4126.
Should we implement the suggested force_stop in rest_ensure_response(), we could add the link to documentation into the Pre-warmup could not finish. notice.

Possible Solution 2:
In the reference to "the way we already do for CPCSS" -- CPCSS generates a collection of notices and displays them in the WP Admin notices section at the top of the page. In the case of RUCSS we decided during alpha against listing all the errors there. I recall 2 considerations for not doing it: The list of unprocessed files could be very long, the errors are being reported in conjunction with the status bar element via react rather than through the WP Notices and needed to be kept in the same react section of the page.

That said, we could:

  1. In send_warmup_request(), check for whether the response is an error and set a "resources_scanner_API_call_failed" flag in the options.
  2. When the pre-warmup hour is expired, after setting warmup status completed here, we can also check for the flag and if there has been a bad response, set a new WP Notice.

Note: Here we'd be setting the WP notice once at the end of the pre-warmup hour. I've considered that we could set the notice repeatedly every time there is a bad fetch response, however, it's likely that if there is an issue here, there will be multiple bad responses causing the WP notice to re-appear throughout the hour that the fetching process could run. On the other hand, we shouldn't short-circuit/stop the whole pre-warmup process for just a single bad fetch either. The down-side of setting it once at the end is that a user will have to wait the hour before seeing any notice appear.

Estimate Effort:
Either per solution for #4126 or [S] for "capture the flag."

@iCaspar iCaspar added effort: [S] 1-2 days of estimated development time and removed needs: grooming GROOMING IN PROGRESS Use this label when the issue is currently being groomed. labels Aug 9, 2021
@remyperona remyperona removed this from the 3.9.3 milestone Aug 30, 2021
@piotrbak
Copy link
Contributor

piotrbak commented Nov 15, 2021

This is going to be handled by:
#4338

@GeekPress
Copy link
Contributor

@piotrbak Until to work on this which require some UI work too, we can display a notice like we are doing for CPCSS.

@GeekPress GeekPress added this to the 3.11.6 milestone Jun 29, 2022
@piotrbak piotrbak added needs: grooming and removed effort: [S] 1-2 days of estimated development time labels Jul 3, 2022
@jeawhanlee jeawhanlee self-assigned this Jul 11, 2022
@jeawhanlee
Copy link
Contributor

Scope a solution

We can set a new option of an integer maybe rocket_api_response_error in this check that we will increment for each failed response.

if ( 200 !== $this->response_code ) {

also reset option before this line when there is a successful call.
$this->response_body = wp_remote_retrieve_body( $response );

Then we create a new method in WP_Rocket\Engine\Optimization\RUCSS\Admin\Settings to check if rocket_api_response_error is >1 then we create a new notice and hook to admin_notices in WP_Rocket\Engine\Optimization\RUCSS\Admin\Subscriber

PS: With this I don't think the user will be getting this notice the first time they enable RUCSS though.

Estimate Effort

[S]
what do you think @Tabrisrp

@jeawhanlee jeawhanlee added the effort: [S] 1-2 days of estimated development time label Jul 11, 2022
@jeawhanlee jeawhanlee removed their assignment Jul 11, 2022
@CrochetFeve0251 CrochetFeve0251 self-assigned this Jul 12, 2022
@piotrbak piotrbak modified the milestones: 3.12.1, 3.12.2 Sep 11, 2022
@piotrbak piotrbak added priority: low Issues that can wait and removed priority: high Issues which should be resolved as quickly as possible labels Oct 10, 2022
@GeekPress GeekPress added this to the 3.12.4 milestone Oct 26, 2022
@piotrbak piotrbak removed this from the 3.12.4 milestone Dec 29, 2022
@piotrbak piotrbak added this to the 3.13.2 milestone Mar 22, 2023
@piotrbak piotrbak added needs: copywriting needs: documentation Issues which need to create or update a documentation labels Apr 9, 2023
@piotrbak piotrbak removed this from the 3.13.2 milestone Apr 23, 2023
github-merge-queue bot pushed a commit that referenced this issue Jul 13, 2023
@piotrbak piotrbak added this to the 3.14.2 milestone Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [S] 1-2 days of estimated development time module: remove unused css needs: copywriting needs: documentation Issues which need to create or update a documentation priority: low Issues that can wait type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants