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.

Sign up for GitHub

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

Jump to bottom

Add get help link to authentication request failed message #5635

Merged
merged 11 commits into from
Aug 8, 2022
Merged

Add get help link to authentication request failed message #5635

merged 11 commits into from
Aug 8, 2022

Conversation

nfmohit
Copy link
Collaborator

@nfmohit nfmohit commented Aug 2, 2022 •
edited by techanvil
Loading

Summary

Addresses issue:

Relevant technical choices

This PR simply adds "Get Help" links to the authentication request failed message. It also updates test cases to address this change.

PR Author Checklist

  • My code is tested and passes existing unit tests.
  • My code has an appropriate set of unit tests which all pass.
  • My code is backward-compatible with WordPress 4.7 and PHP 5.6.
  • My code follows the WordPress coding standards.
  • My code has proper inline documentation.
  • I have added a QA Brief on the issue linked above.
  • I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

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

Code Reviewer Checklist

  • Run the code.
  • Ensure the acceptance criteria are satisfied.
  • Reassess the implementation with the IB.
  • Ensure no unrelated changes are included.
  • Ensure CI checks pass.
  • Check Storybook where applicable.
  • Ensure there is a QA Brief.

Merge Reviewer Checklist

  • Ensure the PR has the correct target branch.
  • Double-check that the PR is okay to be merged.
  • Ensure the corresponding issue has a ZenHub release assigned.
  • Add a changelog message to the issue.

techanvil
techanvil requested changes Aug 4, 2022
View reviewed changes
Copy link
Collaborator

@techanvil techanvil left a comment

Choose a reason for hiding this comment

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

Hey @nfmohit, this is looking good. I have left a handful of minor comments around testing for you to take a look at.

includes/Core/Authentication/Setup.php Outdated
*
* @return string The get help link.
*/
public function get_oauth_proxy_failed_help_link() {
Copy link
Collaborator

@techanvil techanvil Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This helper method is only used within this class (when looking at the production code), and it's not apparent that it'll be needed outside of the class. Therefore I'd suggest it should not be public - I'd go for private instead.

I know it's used in the unit tests, but it's generally not good practice to allow unit tests to dictate the public API of the code they are testing. Within the tests themselves I think testing for the string literal is fine in this case - and indeed, that will have the added benefit of actually verifying the result of get_oauth_proxy_failed_help_link().

Suggested change
public function get_oauth_proxy_failed_help_link() {
private function get_oauth_proxy_failed_help_link() {

tests/phpunit/integration/Core/Authentication/Setup_Test.php Outdated
$setup = new Setup( $context, new User_Options( $context ), new Authentication( $context ) );
$context = new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE, new MutableInput() );
$setup = new Setup( $context, new User_Options( $context ), new Authentication( $context ) );
$oauth_proxy_failed_help_link = $setup->get_oauth_proxy_failed_help_link();
Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned above, I don't think this test should be referencing get_oauth_proxy_failed_help_link()...

Suggested change
$oauth_proxy_failed_help_link = $setup->get_oauth_proxy_failed_help_link();

tests/phpunit/integration/Core/Authentication/Setup_Test.php Outdated
Comment on lines 210 to 214
sprintf(
'The request to the authentication proxy has failed with an error: %1$s %2$s.',
$error,
$oauth_proxy_failed_help_link
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead, just include the expected string directly - this is a more robust test, and clearer as to what the expectation is.

Suggested change
sprintf(
'The request to the authentication proxy has failed with an error: %1$s %2$s.',
$error,
$oauth_proxy_failed_help_link
),
sprintf(
'The request to the authentication proxy has failed with an error: %1$s <a href="https://sitekit.withgoogle.com/support/?error_id=request_to_auth_proxy_failed" target="_blank">Get Help</a>.',
$error
),

tests/phpunit/integration/Core/Authentication/Setup_Test.php Outdated

$context = new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE, new MutableInput() );
$setup = new Setup( $context, new User_Options( $context ), new Authentication( $context ) );
$oauth_proxy_failed_help_link = $setup->get_oauth_proxy_failed_help_link();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same applies here

Suggested change
$oauth_proxy_failed_help_link = $setup->get_oauth_proxy_failed_help_link();

tests/phpunit/integration/Core/Authentication/Setup_Test.php Outdated
Comment on lines 273 to 276
sprintf(
'The request to the authentication proxy has failed. Please, try again later. %s.',
$oauth_proxy_failed_help_link
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

And again, test for the literal string here...

Suggested change
sprintf(
'The request to the authentication proxy has failed. Please, try again later. %s.',
$oauth_proxy_failed_help_link
),
'The request to the authentication proxy has failed. Please, try again later. <a href="https://sitekit.withgoogle.com/support/?error_id=request_to_auth_proxy_failed" target="_blank">Get Help</a>.',

tests/phpunit/integration/Core/Authentication/Setup_Test.php Outdated

add_filter(
'pre_http_request',
function( $preempt, $args, $url ) use ( $context, &$proxy_server_requests, $has_credentials ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

$has_credentials is unused within the function

Suggested change
function( $preempt, $args, $url ) use ( $context, &$proxy_server_requests, $has_credentials ) {
function( $preempt, $args, $url ) use ( $context, &$proxy_server_requests ) {

@nfmohit
Copy link
Collaborator Author

nfmohit commented Aug 5, 2022

Hey @nfmohit, this is looking good. I have left a handful of minor comments around testing for you to take a look at.

Very valid suggestions! I've addressed them. Thank you @techanvil!

@nfmohit nfmohit requested a review from techanvil August 5, 2022 12:49
techanvil
techanvil approved these changes Aug 8, 2022
View reviewed changes
Copy link
Collaborator

@techanvil techanvil left a comment

Choose a reason for hiding this comment

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

Nice one @nfmohit! LGTM 🎉

@techanvil techanvil merged commit 3aa8c72 into develop Aug 8, 2022
@techanvil techanvil deleted the enhancement/5484-authentication-failed-get-help branch August 8, 2022 09:41
felixarntz
felixarntz reviewed Aug 11, 2022
View reviewed changes
includes/Core/Authentication/Setup.php
return sprintf(
/* translators: 1: Support link URL. 2: Get help string. */
__( '<a href="%1$s" target="_blank">%2$s</a>', 'google-site-kit' ),
esc_url( $this->proxy_support_link_url . '/?error_id=request_to_auth_proxy_failed' ),
Copy link
Member

Choose a reason for hiding this comment

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

Please use add_query_arg() to add query parameters instead of manually concatenating pieces of a URL.

Not only is that a best practice in WordPress, it is also more error proof: Right now, the code here only works as long as we know or can assume that $this->proxy_support_link_url does not end in a trailing slash. With add_query_arg, we don't have that problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Duly noted and very valid concern. Thank you for noticing and pointing it out @felixarntz!

@felixarntz felixarntz mentioned this pull request Aug 11, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixarntz felixarntz felixarntz left review comments

@techanvil techanvil techanvil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nfmohit @felixarntz @techanvil