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

Comments: Redirect front-end comment edit links to Calypso. #8069

Merged
merged 4 commits into from
Dec 26, 2017

Conversation

kwight
Copy link
Contributor

@kwight kwight commented Oct 27, 2017

Needs Automattic/wp-calypso#20777.

Just as we've added the ability to redirect front-end post and page edit links to Calypso in #7732 , this PR gives the same treatment to front-end comment edit links. The same Jetpack option is used for activating (edit_links_calypso_redirect).

screen shot 2017-10-27 at 4 18 33 pm

When this option is enabled, comment edit links will be filtered with an href pointing to Calypso, in the format /comments/all/:site?commentId=:comment-id&action=edit.

See Automattic/wp-calypso#17221.

Testing

  • Switch to this branch.
  • Ensure the option returns as enabled. One way is to edit class.jetpack.php L619 and replace the first check Jetpack::get_option( 'edit_links_calypso_redirect' ) with just true.
  • Verify that a front-end Edit link for a comment redirects to Calypso.

Proposed changelog entry for your changes:

Edit links for comments in the frontend can redirect to calypso if the Jetpack option edit_links_calypso_redirect is enabled

add_filter( 'get_edit_post_link', array( $this, 'point_edit_links_to_calypso' ), 1, 2 );
if ( Jetpack::get_option( 'edit_links_calypso_redirect' ) && ! is_admin() ) {
add_filter( 'get_edit_post_link', array( $this, 'point_edit_post_links_to_calypso' ), 1, 2 );
add_filter( 'edit_comment_link', array( $this, 'point_edit_comment_links_to_calypso' ), 1, 3 );
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be more appropriate to use get_edit_comment_link filter here instead, the same way as we do for edit post links.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I... could have sworn I looked for that, and didn't find it 🙃 Yep, for sure, much easier.

@@ -669,6 +670,15 @@ function point_edit_links_to_calypso( $default_url, $post_id ) {
return esc_url( sprintf( 'https://wordpress.com/%s/%s/%d', $path_prefix, $site_slug, $post_id ) );
}

function point_edit_comment_links_to_calypso( $link, $comment_id, $text ) {
$url = wp_parse_url( get_home_url() );
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this is going to break on Jetpack sites with subfolder installs. I suggest following the existing pattern in point_edit_post_links_to_calypso to avoid that:

$site_slug = Jetpack::build_raw_urls( get_home_url() );

@@ -669,6 +670,15 @@ function point_edit_links_to_calypso( $default_url, $post_id ) {
return esc_url( sprintf( 'https://wordpress.com/%s/%s/%d', $path_prefix, $site_slug, $post_id ) );
}

function point_edit_comment_links_to_calypso( $link, $comment_id, $text ) {
$url = wp_parse_url( get_home_url() );
$html = sprintf( '<a class="comment-edit-link" href="%s">%s</a>',
Copy link
Member

Choose a reason for hiding this comment

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

Once we switch to get_edit_comment_link filter, we can simplify this part - we'll only need to return the href.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @vindl ! Updated in 64643f8.

@@ -669,6 +670,14 @@ function point_edit_links_to_calypso( $default_url, $post_id ) {
return esc_url( sprintf( 'https://wordpress.com/%s/%s/%d', $path_prefix, $site_slug, $post_id ) );
}

function point_edit_comment_links_to_calypso( $url ) {
wp_parse_str( wp_parse_url( $url, PHP_URL_QUERY ), $query_args );
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you walk me through what's happening here? Does wp_parse_url not return an array of the url parts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. PHP_URL_QUERY says, "just return the query key value of that array as a string" (it's the only part we're interested in). wp_parse_str (which uses parse_str) parses the query params it finds in that string and assigns them to the $query_args array. The one we want that contains the comment id is the c param (which is awkwardly assigned as amp;c).

I could just use basic regex (\d+$?) to strip whatever number off the end of the string, but that feels a little more fragile over time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the caller not have access to the comment id here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd at the least add a quick comment since amp;c is a bit unexpected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, good idea; added in 948498f.

@@ -669,6 +670,14 @@ function point_edit_links_to_calypso( $default_url, $post_id ) {
return esc_url( sprintf( 'https://wordpress.com/%s/%s/%d', $path_prefix, $site_slug, $post_id ) );
}

function point_edit_comment_links_to_calypso( $url ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why the signature is different for this function versus point_edit_post_links_to_calypso? I think it's clearer to pass through a fallback and a comment id explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

point_edit_post_links_to_calypso is hooked to get_edit_post_link, which passes in the original link, the post ID (very convenient for our needs), and a context (not needed in our situation, so we only grab the first two ).

The get_edit_comment_link filter, however, only gives us the link we'll be filtering, hence all the parsing madness.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Got it thanks for clarifying!

`point_edit_comment_links_to_calypso`.
function point_edit_comment_links_to_calypso( $url ) {
// Take the `query` key value from the URL, and parse its parts to the $query_args. `amp;c` matches the comment ID.
wp_parse_str( wp_parse_url( $url, PHP_URL_QUERY ), $query_args );
return esc_url( sprintf( 'https://wordpress.com/comments/all/%s/?commentId=%d&action=edit',
Copy link
Contributor

Choose a reason for hiding this comment

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

@kwight the hotlink for edit will be https://wordpress.com/comment/{site_slug}/{comment_id}?action=edit

This is Calypso's PR Automattic/wp-calypso#20777

@kwight kwight added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Dec 15, 2017
@kwight kwight requested a review from a team December 15, 2017 16:52
@gwwar
Copy link
Contributor

gwwar commented Dec 20, 2017

Urls look correct here. @rodrigoi could you update the related wpcom diff to match?

@vindl vindl added this to the 5.7 milestone Dec 22, 2017
Copy link
Member

@vindl vindl left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

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.

Tests well and code looks good.

@oskosk oskosk added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Dec 26, 2017
@oskosk oskosk merged commit 5ad8100 into master Dec 26, 2017
@oskosk oskosk deleted the add/edit-comments-to-calypso branch December 26, 2017 20:18
@kraftbj kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants