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

Add a "get help" link to the authentication request failed message. #5484

Closed
eugene-manuilov opened this issue Jun 30, 2022 · 8 comments
Closed
Labels
P1 Medium priority PHP QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature

Comments

@eugene-manuilov
Copy link
Collaborator

eugene-manuilov commented Jun 30, 2022

Feature Description

When the request to the authentication proxy fails, we show the wp_die screen with the appropriate message to the user. We need to update that message to include the new "get help" link that leads to the {proxy}/support/?error_id=request_to_auth_proxy_failed URL which can be obtained using the getErrorTroubleshootingLinkURL selector introduced in #5423.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The Setup:: handle_action_setup_start method should be updated to include the new "get help" link at the end of the error message for failed requests to the authentication server. The "get help" link should lead to the {proxy}/support/?error_id=request_to_auth_proxy_failed URL.
    if ( is_wp_error( $oauth_setup_redirect ) ) {
    $error_message = $oauth_setup_redirect->get_error_message();
    if ( empty( $error_message ) ) {
    $error_message = $oauth_setup_redirect->get_error_code();
    }
    /* translators: %s: Error message or error code. */
    wp_die( esc_html( sprintf( __( 'The request to the authentication proxy has failed with an error: %s', 'google-site-kit' ), $error_message ) ) );
    }
    if ( ! filter_var( $oauth_setup_redirect, FILTER_VALIDATE_URL ) ) {
    wp_die( esc_html__( 'The request to the authentication proxy has failed. Please, try again later.', 'google-site-kit' ) );
    }

Implementation Brief

  • In includes/Core/Authentication/Setup.php:
    • In the class __construct method, create a new class property named proxy_support_link_url which should call the get_proxy_support_link_url method from the Authentication class instance (accessible via the $authentication parameter).
      public function __construct(
      Context $context,
      User_Options $user_options,
      Authentication $authentication
      ) {
      $this->context = $context;
      $this->user_options = $user_options;
      $this->authentication = $authentication;
      $this->credentials = $authentication->credentials();
      $this->google_proxy = $authentication->get_google_proxy();
      }
    • In the handle_action_setup_start method:
      • Before checking error for $oauth_setup_redirect, create a new variable named $oauth_proxy_failed_help_link.
        if ( is_wp_error( $oauth_setup_redirect ) ) {
        • The variable should contain a sprintf function with:
          • translatable string as the first parameter. The string should contain an <a> tag with:
            • href attribute, value for which should be the first placeholder (%1$s - the troubleshooting URL).
            • target attribute, value for which should be _blank.
            • content should be the second placeholder (%2$s - the Get Help string).
          • the second parameter should be /?error_id=request_to_auth_proxy_failed appeneded to $this->proxy_support_link_url. The entire URL should be escaped using esc_url().
          • The third parameter should be Get Help (translatable).
      • The first error message should be updated:
        wp_die( esc_html( sprintf( __( 'The request to the authentication proxy has failed with an error: %s', 'google-site-kit' ), $error_message ) ) );
        • The existing placeholder (%s) should be changed to %1$s.
        • After the existing string, a full stop should be added followed by a second placeholder (%2$s - the get help link) and a full stop.
        • A new third parameter should be added to the sprintf function call, which should simply be the $oauth_proxy_failed_help_link variable.
      • The second error message should be updated:
        wp_die( esc_html__( 'The request to the authentication proxy has failed. Please, try again later.', 'google-site-kit' ) );
        • The string should be wrappped with a sprintf function.
        • A placeholder (%s) should be appended to the existing string followed by a full stop.
        • The second parameter for the sprintf function should be the $oauth_proxy_failed_help_link variable.

Test Coverage

In tests/phpunit/integration/Core/Authentication/Setup_Test.php:

  • Update the test_handle_action_setup_start__wp_error test case to check for the full error message (the first error message in the IB).
  • Add a new test case to check for the second error message in the IB.

QA Brief

  • QAing this would require reproducing an error when the Sign in with Google button is clicked. In order to get the error:
    • Go to the Site Kit connect splash screen (disconnect/reset if required).
    • Add this snippet to the site's wp-config.php file: define( 'WP_HTTP_BLOCK_EXTERNAL', true );.
    • Once added, click on the Sign in with Google button.
  • The error message should contain a Get Help link, clicking on which should open the relevant support page in a new tab.

QA:Eng

  • In order to reproduce the second error, the only way I could find was to alter the condition.
    if ( ! filter_var( $oauth_setup_redirect, FILTER_VALIDATE_URL ) ) {
    wp_die(
    sprintf(
    /* translators: %s: Get Help link. */
    esc_html__( 'The request to the authentication proxy has failed. Please, try again later. %s.', 'google-site-kit' ),
    wp_kses(
    $oauth_proxy_failed_help_link,
    array(
    'a' => array(
    'href' => array(),
    'target' => array(),
    ),
    )
    )
    )
    );
    }
    • Alter the condition here so that wp_die() is called even though the filter_var() is truthy.
    • Go to the Site Kit connect splash screen (disconnect/reset if required).
    • Once added, click on the Sign in with Google button.
  • The error message should contain a Get Help link, clicking on which should open the relevant support page in a new tab.

Changelog entry

  • Add a "get help" link to the message that appears when a request to the authentication proxy fails.
@eugene-manuilov eugene-manuilov added P1 Medium priority Type: Enhancement Improvement of an existing feature PHP labels Jun 30, 2022
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Jul 26, 2022
@techanvil techanvil self-assigned this Jul 27, 2022
@techanvil
Copy link
Collaborator

IB ✅

@nfmohit I added a couple of points about writing tests and bumped the estimate to allow some extra time.

@techanvil techanvil removed their assignment Jul 27, 2022
@nfmohit
Copy link
Collaborator

nfmohit commented Jul 27, 2022

@nfmohit I added a couple of points about writing tests and bumped the estimate to allow some extra time.

Brilliant, thank you @techanvil!

@nfmohit nfmohit self-assigned this Aug 2, 2022
@nfmohit nfmohit removed their assignment Aug 3, 2022
@nfmohit nfmohit added the QA: Eng Requires specialized QA by an engineer label Aug 3, 2022
@techanvil techanvil assigned techanvil and nfmohit and unassigned techanvil Aug 4, 2022
@nfmohit nfmohit assigned techanvil and unassigned nfmohit Aug 5, 2022
@techanvil techanvil removed their assignment Aug 8, 2022
@wpdarren wpdarren self-assigned this Aug 8, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Aug 8, 2022

QA Update: ⚠️

@nfmohit thanks for the QAB instructions! I've followed them though and when I click on the sign in with Google button, this message appears rather than the expected behaviour in the QAB. 🤔

image

@wpdarren
Copy link
Collaborator

wpdarren commented Aug 8, 2022

QA Update: ✅

Verified:

  • The error message should contain a Get Help link, clicking on which should open a support page in a new tab.

image

Leaving in the queue for QA:Eng part of the QAB.

@hussain-t hussain-t self-assigned this Aug 9, 2022
@hussain-t
Copy link
Collaborator

QA:Eng verified ✅

  • The error message should contain a Get help link, clicking on which should open a support page in a new tab.

Screenshot 2022-08-09 at 2 29 36 PM

  • The link matches /support/?error_id=request_to_auth_proxy_failed.

Screenshot 2022-08-09 at 2 29 49 PM

@hussain-t hussain-t removed their assignment Aug 9, 2022
@felixarntz
Copy link
Member

@nfmohit @techanvil Not a blocker, but I found one minor caveat in #5635 (review). If you find a few minutes to address this in a PR against main, that'd be perfect.

@felixarntz felixarntz reopened this Aug 11, 2022
@nfmohit
Copy link
Collaborator

nfmohit commented Aug 11, 2022

@nfmohit @techanvil Not a blocker, but I found one minor caveat in #5635 (review). If you find a few minutes to address this in a PR against main, that'd be perfect.

@felixarntz On it now. Thank you for pointing it out!

@nfmohit nfmohit self-assigned this Aug 11, 2022
@nfmohit nfmohit removed their assignment Aug 11, 2022
@eugene-manuilov eugene-manuilov self-assigned this Aug 11, 2022
eugene-manuilov added a commit that referenced this issue Aug 11, 2022
Use `add_query_arg()` for get help URL
@eugene-manuilov
Copy link
Collaborator Author

@felixarntz #5681 PR has been merged into main. Moving this ticket back into Approval, since it has already been QA-ed.

@eugene-manuilov eugene-manuilov removed their assignment Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority PHP QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants