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

Sharing : Add wp_sharing_email_send_post_subject filter #8284

Merged
merged 6 commits into from Jan 25, 2018

Conversation

Umangvaghela
Copy link
Contributor

@Umangvaghela Umangvaghela commented Dec 1, 2017

Fixes #8165

Changes proposed in this Pull Request:

  • add wp_sharing_email_send_post_subject for allow modifying the email share Subject Line

Testing instructions:

@Umangvaghela Umangvaghela requested a review from a team as a code owner December 1, 2017 06:58
@Umangvaghela Umangvaghela changed the title add wp_sharing_email_send_post_subject filter Sharing : Add wp_sharing_email_send_post_subject filter Dec 1, 2017
@jeherve jeherve added [Feature] Sharing Post sharing, sharing buttons [Pri] Normal [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Dec 1, 2017
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

In my review I just suggested a few changes to match coding standards.

@@ -51,7 +51,9 @@ function sharing_email_send_post( $data ) {
// Make sure to pass the title through the normal sharing filters.
$title = $data['sharing_source']->get_share_title( $data['post']->ID );

wp_mail( $data['target'], '[' . __( 'Shared Post', 'jetpack' ) . '] ' . $title, $content, $headers );
$subject = apply_filters('wp_sharing_email_send_post_subject',sprintf(__('Shared Post','jetpack')));
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a docblock before the filter, as with the other filters in this file?

@@ -51,7 +51,9 @@ function sharing_email_send_post( $data ) {
// Make sure to pass the title through the normal sharing filters.
$title = $data['sharing_source']->get_share_title( $data['post']->ID );

wp_mail( $data['target'], '[' . __( 'Shared Post', 'jetpack' ) . '] ' . $title, $content, $headers );
$subject = apply_filters('wp_sharing_email_send_post_subject',sprintf(__('Shared Post','jetpack')));
Copy link
Member

Choose a reason for hiding this comment

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

Could you add spacing around quotes, parenthesis, and before sprintf?

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Dec 1, 2017
@Umangvaghela
Copy link
Contributor Author

@jeherve

Greetings of the day !!

Thanks for reply.
I add changes which is you suggested me. Please check it and provide the feedback for the same.

Thanks

@jeherve jeherve added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Dec 4, 2017
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

A few changes to match the previous subject line.

*
* @param string $var Sharing Email Send Post Subject. Default is "Shared Post".
*/
$subject = apply_filters( 'wp_sharing_email_send_post_subject', __( 'Shared Post', 'jetpack' ) );
Copy link
Member

Choose a reason for hiding this comment

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

Could you add square brackets ([]) around the subject line, like in the original message?

Thanks!

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Dec 15, 2017
@Umangvaghela
Copy link
Contributor Author

@jeherve

Changes Done.

Thanks

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

One last change and we should be good to go!

Thank you!

* Filter the Sharing Email Send Post Subject.
*
* @module sharedaddy
*
Copy link
Member

Choose a reason for hiding this comment

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

Could you add @since 5.7.0 here too?

@Umangvaghela
Copy link
Contributor Author

@jeherve

Done.

Thanks

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Dec 18, 2017
@oskosk oskosk added this to the 5.7 milestone Dec 20, 2017
oskosk
oskosk previously requested changes Dec 20, 2017
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @Umangvaghela .

I left some comments about the usage of $title.

*
* @param string $var Sharing Email Send Post Subject. Default is "Shared Post".
*/
$subject = apply_filters( 'wp_sharing_email_send_post_subject', '[' . __( 'Shared Post', 'jetpack' ) . '] ' );
Copy link
Contributor

@oskosk oskosk Dec 20, 2017

Choose a reason for hiding this comment

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

I think something is missing here.

The $title needs to be concatenated in the subject.

*/
$subject = apply_filters( 'wp_sharing_email_send_post_subject', '[' . __( 'Shared Post', 'jetpack' ) . '] ' );

wp_mail( $data['target'], $subject, $title, $content, $headers );
Copy link
Contributor

@oskosk oskosk Dec 20, 2017

Choose a reason for hiding this comment

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

Here $title shouldn't be the third argument. $title should be concatenated into the subject`.

@oskosk oskosk added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Dec 20, 2017
@oskosk oskosk removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Dec 21, 2017
@oskosk oskosk added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Dec 21, 2017
@Umangvaghela
Copy link
Contributor Author

@oskosk

Changes Done.

Thanks

@oskosk
Copy link
Contributor

oskosk commented Dec 21, 2017

Thanks for the changes @Umangvaghela ! I've relabeled as needs review...

Code looks good now, but how do I test that the funcionality updated here is not affected? "Testing instructions" on the description of the PR, pleaseee 😄

@Umangvaghela
Copy link
Contributor Author

Umangvaghela commented Dec 24, 2017

@oskosk

Testing instruction.

1 ) Enable sharing email option.
2 ) Add filter "wp_sharing_email_send_post_subject" in active theme function.php.
3 ) Update the subject if you want.
4 ) Share post by email sharing option.

Thanks

@oskosk oskosk modified the milestones: 5.7, 5.8 Dec 26, 2017
@Umangvaghela
Copy link
Contributor Author

@oskosk

Hello Sir

Can you told me anything is missing for "testing description" or In PR. If you find anyhting else so told me I will change or add it as soon as possible. :)

Thanks,

@Automattic Automattic deleted a comment from Umangvaghela Jan 19, 2018
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Sorry, but this PR missed the 5.7.0 release, and we're aiming for 5.8.0 now. Can you please fix the version number?
Thank you!

@zinigor zinigor added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jan 23, 2018
@Umangvaghela
Copy link
Contributor Author

@zinigor

changes done.

Thanks

@jeherve jeherve added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 25, 2018
@zinigor zinigor dismissed oskosk’s stale review January 25, 2018 09:29

Thanks for the review, Osk! Your comments have been addressed.

@zinigor zinigor merged commit 1f7eccb into Automattic:master Jan 25, 2018
@zinigor zinigor removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Jan 25, 2018
jeherve added a commit that referenced this pull request Jan 25, 2018
zinigor pushed a commit that referenced this pull request Jan 30, 2018
* Changelog 5.8: create base for changelog.

* Update 5.8 release post link

* fix 5.8 release date

* Updates to plugin description

* Changelog: add #8499

* Changelog: add #8506

* Changelog: add #8509

* Changelog: add #8516

* Changelog: add #8517

* Changelog: add #8523

* Changelog: add #8547

* Changelog: add #8496

* Changelog: add #8584

* Changelog: add #8595

* Changelog: add #8445

* Changelog: add #8431

* Changelog: add #8284

* Changelog: add #8270

* Changelog: add #8124

* Changelog: add #8581

* Changelog: add #8463

* Changelog: add #8568 (#8646)

* Updates to testing list and changelog

* Changelog: add #8443

* Changelog: add #8459

* Changelog: add #8469

* Changelog: add #8464

* Changelog: add #8478 and #8479

* Changelog: add #8483

* Changelog: add #8488

* Changelog: add #8513

* Changelog: add #8555

* Changelog: add #8565

* Changelog: add #8601

* Changelog: add #8612

* Changelog: add first pass at Search items.

* Changelog: add more info to help test Search.

* Changelog: add #8144

* Changelog: add #8313

* Changelog: add #8419

* Changelog: add #8465

* Changelog: add #8515

* Changelog: add #8587

* Changelog: add #8591

* Changelog: add #8659

* Changelog: add #8661

* Changelog: add #8671

* Changelog: add 5.7.1 to archived changelog too.

* Reverted changes to readme, removed entry about backups.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Sharing Post sharing, sharing buttons [Pri] Normal [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants