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

Error code when RUCSS API can't be reached #6289

Closed
MathieuLamiot opened this issue Nov 24, 2023 · 3 comments · Fixed by #6347
Closed

Error code when RUCSS API can't be reached #6289

MathieuLamiot opened this issue Nov 24, 2023 · 3 comments · Fixed by #6347
Assignees
Labels
effort: [XS] < 1 day of estimated development time type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@MathieuLamiot
Copy link
Contributor

Context
Related to this GH issue: #6222
QA team notices that part of the specification was missing in this thread: if the API can't be reached (not responding, timeout, ...) and that we don't have an HTTP answer, it is not clear how the plugin behaves. For instance, see this possible issue.
This means that we don't know exactly what happens in case the API can't be reached and we would not be able to identify the issue when debugging a user for instance.

Expected behavior
As mentioned in this GH issue, we would need a specific error_code and/or error_message to be stored into the RUCSS table in the DB, in case the API can't be reached.

Acceptance Criteria

  • If the API can be reached (normal case) there is no change of behavior (see Rework RUCSS retry strategy #6222)
  • Have a pending entry in RUCSS table. Disconnect from the internet or prevent the "fetch result" call to reach the API. Once the fetch has been tried, check the RUCSS table. The specific error_code and/or message should be registered for this job.
@CrochetFeve0251 CrochetFeve0251 self-assigned this Nov 27, 2023
@CrochetFeve0251
Copy link
Contributor

Root cause

The root cause is due to the fact WordPress returning an empty response when not reaching the endpoint.

Scope a solution

A simple solution would be to add a default error message when we are getting a empty result at this level inside the client.

Estimate effort

Effort XS

@CrochetFeve0251 CrochetFeve0251 added the effort: [XS] < 1 day of estimated development time label Nov 27, 2023
@MathieuLamiot
Copy link
Contributor Author

@CrochetFeve0251 Sounds good for the error message.

About the error code, it is handled just above it seems:

$this->response_code = is_array( $response )

What I am afraid of is that sometime, the HTTP Response code returned here could match one of the code we use within the payload. For instance, the HTTP Response code could be 408 because of API timeout. We would store a error_code 408 in the RUCSS DB and trigger the retry mechanism, which is not what we want. To make sure we don't have conflict, WDYT of something like:
In the API client, if HTTP Response Code != 200, then force

  • $this->response_code to 504 (or another value that you see fit)

  • $this->error_message = "API Client Error: " .
    is_array( $response )? wp_remote_retrieve_response_code( $response ): $response->get_error_code() .

      		is_array( $response )
      		? wp_remote_retrieve_response_message( $response )
      		: $response->get_error_message();
    

This would avoid collision on "code" stored in the RUCSS DB + to keep the complete information for debug purposes.

@MathieuLamiot
Copy link
Contributor Author

After discussion with @CrochetFeve0251, we will keep this fix here very simple and not tackle the HTTP code / RUCSS code collision in this issue. As a result, the proposed solution is OK.
#6293

@Miraeld Miraeld self-assigned this Dec 6, 2023
@Miraeld Miraeld added the type: enhancement Improvements that slightly enhance existing functionality and are fast to implement label Dec 6, 2023
@Miraeld Miraeld removed their assignment Dec 7, 2023
@piotrbak piotrbak added this to the 3.15.7 milestone Dec 14, 2023
@jeawhanlee jeawhanlee self-assigned this Dec 21, 2023
@remyperona remyperona mentioned this issue Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [XS] < 1 day of estimated development time 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.

5 participants