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

Remove query args from paginate_links urls #2037

Merged
merged 4 commits into from Jul 23, 2020

Conversation

JuanchoPestana
Copy link
Contributor

Fixes #1912

Changes proposed in this Pull Request:

  • The paginate_links() function adds the url args (from the current url) to each pagination link. The actions (mark as filled, not filled, etc) in the job dashboard were triggered with the url (For example: url/?action=mark_as_filled). The consequences of this were that if you changed the page, the pagination link would carry the "action, job_id, and _wpnonce" parameters to the new page, causing the action to be triggered again; thus, getting the message that the job was already filled/not filled.
  • This PR removes the args from the url for the pagination links so that the actions are not triggered again when changing pages.

Testing instructions:

  1. Make sure you have enough jobs on your jobs dashboard that they are paginated at least to a page 2.
  2. On the jobs dashboard, mark a job as filled; you will get the correct "X has been filled" message.
  3. Click on the "2" or the "next arrow" in the pagination at the bottom of the jobs list.
  4. You should not see any messages

Proposed changelog entry for your changes:

  • Remove args from pagination_link urls

@JuanchoPestana JuanchoPestana mentioned this pull request Jul 10, 2020
Comment on lines 283 to 285
public function filter_paginate_links( $link ) {
return remove_query_arg( [ 'action', 'job_id', '_wpnonce' ], $link );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I find myself concerned with this again. I had imagined before that this would only be called if a WPJM-related shortcode existed on the page, but looking again I think this would be called on any page that calls the paginate_links function. This seems dangerous to me.

Do you think it would make sense to add a condition here to ensure that it only happens on the page(s) we want? E.g. see the shortcode_action_handler method. Maybe something like:

Suggested change
public function filter_paginate_links( $link ) {
return remove_query_arg( [ 'action', 'job_id', '_wpnonce' ], $link );
}
public function filter_paginate_links( $link ) {
global $post;
if ( is_page() && has_shortcode( $post->post_content, 'job_dashboard' ) ) {
return remove_query_arg( [ 'action', 'job_id', '_wpnonce' ], $link );
}
return $link;
}

If so, maybe we could also extract that conditional logic to a private method (assuming we use the same logic as in shortcode_action_handler). Maybe private function is_job_dashboard_page() { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Alex!
You're totally right! It's a bit too aggressive... Your suggestion works perfectly! I just implemented it...

Copy link
Contributor

@alexsanford alexsanford left a comment

Choose a reason for hiding this comment

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

Looks good, works well! Thanks for the PR 🚀

@alexsanford alexsanford merged commit 17c8d7f into Automattic:master Jul 23, 2020
@jom jom added this to the 1.34.3 milestone Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark as Filled action generates incorrect messages with paginated jobs on job dashboard
3 participants