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 plugin urls to auto update emails. #3100

Closed

Conversation

costdev
Copy link
Contributor

@costdev costdev commented Aug 17, 2022

This PR refreshes #1688 with changes requested by @peterwilsoncc and adds PHPUnit tests.

Trac ticket: https://core.trac.wordpress.org/ticket/53049

@costdev costdev force-pushed the add_plugin_urls_to_auto_update_emails branch 3 times, most recently from 167e396 to 2999b8d Compare August 17, 2022 05:04
@costdev costdev marked this pull request as ready for review August 17, 2022 05:16
Comment on lines +65 to +72
$has_successful = ! empty( $successful );
$has_failed = ! empty( $failed );

if ( ! $has_successful && ! $has_failed ) {
$this->markTestSkipped( 'This test requires at least one successful or failed plugin update object.' );
}

$type = $has_successful && $has_failed ? 'mixed' : ( ! $has_failed ? 'success' : 'fail' );
Copy link
Contributor Author

@costdev costdev Aug 17, 2022

Choose a reason for hiding this comment

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

This isn't strictly necessary and could be replaced with a $type argument in each dataset.

I included this in the tests to:

  • let contributors focus on testing the update objects.
  • reduce the size of the datasets.
  • reduce the potential for human error by failing to note the correct $type when copying/pasting datasets.

@costdev costdev force-pushed the add_plugin_urls_to_auto_update_emails branch from 2999b8d to 05db467 Compare August 17, 2022 05:50
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

String changes look good, thanks.

A couple of notes inline about the tests.

__( '- %1$s (from version %2$s to %3$s)' ),
$body_message .= sprintf(
/* translators: 1: Plugin name, 2: Current version number, 3: New version number, 4: Plugin URL. */
__( '- %1$s (from version %2$s to %3$s)%4$s' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
__( '- %1$s (from version %2$s to %3$s)%4$s' ),
__( '- %1$s (from version %2$s to %3$s) %4$s' ),

🔢 This applies to others, they'll need a space too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The space is included in $item_url:

$item_url = ' : ' . esc_url( $item->item->url );

Otherwise, with an empty $item_url, there would be a trailing space.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.

I think this might be problematic for i18n as it assumes the sentence works with : http://example.com/colins-plugin and I don't think that's reliably the case.

Unfortunately this involves doubling the number of strings again so I'll defer to people who know much better than I for confirmation. @SergeyBiryukov @audrasjb do you have thoughts here?

tests/phpunit/tests/admin/wpAutomaticUpdater.php Outdated Show resolved Hide resolved
@costdev costdev force-pushed the add_plugin_urls_to_auto_update_emails branch from e8ea3e4 to e33ac48 Compare August 18, 2022 00:27
This method is used more widely in the test suite.
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

Thank you, these test changes look good to me ✅

I've marked it for approval but commit is pending the feedback on i18n.

Copy link
Contributor

@audrasjb audrasjb left a comment

Choose a reason for hiding this comment

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

Hi there 👋
This looks good to me in terms of i18n / polyglots workflow.

@dream-encode
Copy link
Contributor

Merged into core in https://core.trac.wordpress.org/changeset/54212.

@costdev costdev deleted the add_plugin_urls_to_auto_update_emails branch September 19, 2022 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants