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 methods to hide filled and expired posts #2449

Draft
wants to merge 20 commits into
base: trunk
Choose a base branch
from
Draft

Add methods to hide filled and expired posts #2449

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
5afad7d
Add methods to hide filled and expired posts
anaemnesis Jun 6, 2023
7a924f6
Remove unused method
anaemnesis Jun 7, 2023
41f374d
Hide expired and filled jobs
anaemnesis Jun 13, 2023
2fccb36
Remove an 's' from 'jobs' in method name
anaemnesis Jun 13, 2023
eeb42e0
Update maybe_hide_filled_expired_job_listings_from_search logic
anaemnesis Jun 13, 2023
068a162
Account for 'trash' post status
anaemnesis Jun 13, 2023
d1d04d4
Add additional query checks
anaemnesis Jun 21, 2023
cd94313
Remove post_type check, as search page is querying "any"
anaemnesis Jun 21, 2023
3fedb2b
Refactor hide/filled
anaemnesis Jun 21, 2023
fc8e201
Explicitly query 'publish' post status
anaemnesis Jun 21, 2023
2522485
Refactor how $expired posts are hidden from search
anaemnesis Jun 22, 2023
27685ac
Nonce verification for $_GET['s'] isn't necessary here, so let's try …
anaemnesis Jun 22, 2023
d3b5123
Update issue reference
anaemnesis Jun 23, 2023
d23b3a3
Rename $is_expired_hidden variable to be more readable
anaemnesis Jun 28, 2023
ea63e66
Update PHP doc for the get_filled_job_listings() method to state the …
anaemnesis Jun 28, 2023
3a210fb
Early return on get_filled_job_listings() method isn't necessary, as …
anaemnesis Jun 28, 2023
1a7b7da
Revert how expired posts are hidden to previous implementation
anaemnesis Jul 2, 2023
de746c4
Delete 'hide_filled_jobs_transient' on 'updated_post_meta' action.
anaemnesis Jul 2, 2023
82e75e6
Refactor maybe_hide_expired_posts()
anaemnesis Jul 4, 2023
27b3be7
Add comments explaining conditional logic in the maybe_hide_* methods.
anaemnesis Jul 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 74 additions & 1 deletion includes/class-wp-job-manager-post-types.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ public function __construct() {
add_action( 'transition_post_status', [ $this, 'track_job_submission' ], 10, 3 );

add_action( 'parse_query', [ $this, 'add_feed_query_args' ] );
add_action( 'pre_get_posts', [ $this, 'maybe_hide_filled_job_listings_from_search' ] );

// Single job content.
$this->job_content_filter( true );
Expand Down Expand Up @@ -389,14 +390,29 @@ public function register_post_types() {
*/
add_feed( self::get_job_feed_name(), [ $this, 'job_feed' ] );

/**
* This code checks if (1) we're on a search or (2) the option to hide expired job listings is enabled.
* If either condition is false, we're going to set the register_post_status() 'public' arg to false,
* otherwise it will be true.
*
* This will prevent single expired job listings from showing a '404', instead of an
* 'Expired' notice, when viewing the single job listing and not logged in.
*
* This will also address historical issues stemming from addressing the issue raised here
* https://github.com/Automattic/WP-Job-Manager/issues/1884.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this comment. Really helped with figuring out the logic.

$is_site_search = ( isset( $_GET['s'] ) ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended
$is_expired_searchable = (bool) get_option( 'job_manager_hide_expired' );
jom marked this conversation as resolved.
Show resolved Hide resolved
$is_expired_public = ! $is_site_search || ! $is_expired_searchable;

/**
* Post status
*/
register_post_status(
'expired',
[
'label' => _x( 'Expired', 'post status', 'wp-job-manager' ),
'public' => true,
'public' => $is_expired_public,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jom Here's the refactored implementation for setting 'public on the post status registration. The logic is slightly different than discussed but noted in the comment. In testing:

  • Job listings in WP Admin show and are counted as expected
  • Expired Jobs are hidden from search results (or not, if the option is disabled)
  • Expired Jobs, when viewed logged out, show the 'Expired' notice instead of a 404

Copy link
Member

Choose a reason for hiding this comment

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

The variable name threw me off a bit, but I think the ultimate logic is correct!

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 ended up reverting this due to issues checking conditionals like is_archive(). Committed 1a7b7da

Copy link
Member

@jom jom Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The main issue we were avoiding with the other strategy was $query->set( 'post_status', 'publish' ) removing other custom public statuses from the search. I actually wonder if this was fixed in core (5.9) with https://core.trac.wordpress.org/changeset/49830. It should now be respecting our exclude_from_search when registering the expired post status. Did you find in testing that expired posts were showing up in search results still?

I wonder if we should be setting exclude_from_search based on our setting. Since that fix in core, I'm guessing we're hiding expired results from search even when we shouldn't be.

Copy link
Contributor Author

@anaemnesis anaemnesis Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, gotcha.

exclude_from_search is already set to true:

WP-Job-Manager/includes/class-wp-job-manager-post-types.php

Lines 395 to 407 in 0568f04

register_post_status(
'expired',
[
'label' => _x( 'Expired', 'post status', 'wp-job-manager' ),
'public' => true,
'protected' => true,
'exclude_from_search' => true,
'show_in_admin_all_list' => true,
'show_in_admin_status_list' => true,
// translators: Placeholder %s is the number of expired posts of this type.
'label_count' => _n_noop( 'Expired <span class="count">(%s)</span>', 'Expired <span class="count">(%s)</span>', 'wp-job-manager' ),
]
);

This does not exclude expired jobs from WP Search.

This seems to be getting overwritten by the exclude_from_search parameter on the post-type registration:

WP-Job-Manager/includes/class-wp-job-manager-post-types.php

Line 369 in 0568f04

'exclude_from_search' => false,

If this is set to true, then posts with an expired post status are, as expected, excluded from search -- but so is any job type post.

When I was testing the 'public' => manipulation, it worked for $GET_['s'] but did not work for archive pages as helper methods were always returning false. Maybe there's a way around that, that I haven't thought of?

I'm wondering if another option would be to keep the $query->set(), but instead of setting it to publish, retrieve all post statuses with get_post_stati(), remove what we don't want, and then pass everything else. Though, that seems a bit... hacky?

$post_statuses     = get_post_stati();
$excluded_statuses = [ 'future', 'draft', 'pending', 'trash', 'auto-draft', 'preview', 'private', 'request-pending', 'request-confirmed', 'request-failed', 'request-completed', 'expired' ];
$valid_statuses    = array_diff( $post_statuses, $excluded_statuses );

$query->set( 'post_status', $valid_statuses );

IIUC, this is more to do with the issue report here https://core.trac.wordpress.org/ticket/44737, which is sitting stale. Making the suggested fix does fix the behaviour, but then that's a whole core-related thing.

cc @yscik

Copy link
Member

Choose a reason for hiding this comment

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

This seems to be getting overwritten by the exclude_from_search parameter on the post-type registration:

I saw https://core.trac.wordpress.org/ticket/44737, but then saw the lines get changed in https://core.trac.wordpress.org/ticket/48556 and thought that would fix it, but now I can see what it is doing in the } elseif ( ! $this->is_singular ) { block.

I'm wondering if another option would be to keep the $query->set(), but instead of setting it to publish, retrieve all post statuses with get_post_stati(), remove what we don't want, and then pass everything else. Though, that seems a bit... hacky?

This is what I was going to suggest before I went down the 44737 rabbit hole because it seems like WP is doing this in a way that would work for us:
https://github.com/WordPress/WordPress/blob/32e94b4de108fb090ebc678b48113b8954f7f188/wp-includes/class-wp-query.php#L2587-L2593

I think if we did this we could just removed expired (I'm guessing)? It is a bit hack-y, but might be okay.

The only other thought I had is modifying the global that holds the post status objects here after our conditionals and then restore public => true in the posts_selection action, but that might feel more hacky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jom Here's the latest approach, which we talked about

82e75e6

'protected' => true,
'exclude_from_search' => true,
'show_in_admin_all_list' => true,
Expand Down Expand Up @@ -659,6 +675,62 @@ public function job_feed() {
remove_filter( 'posts_search', 'get_job_listings_keyword_search' );
}

/**
* Get filled jobs.
*
* @return array
jom marked this conversation as resolved.
Show resolved Hide resolved
*/
public function get_filled_job_listings(): array {
if ( ! get_option( 'job_manager_hide_filled_positions' ) ) {
return [];
}
jom marked this conversation as resolved.
Show resolved Hide resolved

$filled_jobs_transient = get_transient( 'hide_filled_jobs_transient' );
if ( false === $filled_jobs_transient ) {
$filled_jobs_transient = get_posts(
[
'post_status' => 'publish',
'post_type' => 'job_listing',
yscik marked this conversation as resolved.
Show resolved Hide resolved
'fields' => 'ids',
'posts_per_page' => -1,
'meta_query' => [
[
'key' => '_filled',
'value' => '1',
'compare' => '=',
],
],
]
);
set_transient( 'hide_filled_jobs_transient', $filled_jobs_transient, DAY_IN_SECONDS );
}
return $filled_jobs_transient;
}

/**
* Maybe exclude expired and/or filled job listings from search and archive pages.
*
* @param $query WP_Query $query
*
* @return void
*/
public function maybe_hide_filled_job_listings_from_search( WP_Query $query ): void {
$hide_filled_positions = get_option( 'job_manager_hide_filled_positions' );

if ( ! $hide_filled_positions ) {
return;
}

if (
! is_admin()
&& $query->is_main_query()
&& ( $query->is_search() || $query->is_archive() )
Comment on lines +721 to +723
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to add a comment here clarifying the reason for this logic: We want the filtering to apply only to search and archive public facing queries. We don't want it to apply in the admin panel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Committed 27b3be7

) {

$query->set( 'post__not_in', $this->get_filled_job_listings() );
}
}

/**
* Adds query arguments in order to make sure that the feed properly queries the 'job_listing' type.
*
Expand Down Expand Up @@ -1898,4 +1970,5 @@ public function delete_user_add_job_listings_post_type( $types ) {

return $types;
}

}
18 changes: 18 additions & 0 deletions wp-job-manager-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1712,3 +1712,21 @@ function job_manager_get_salary_unit_options( $include_empty = true ) {
*/
return apply_filters( 'job_manager_get_salary_unit_options', $options, $include_empty );
}

/**
* Delete filled_jobs_transient and expired_jobs_transient transients when a job listing is saved or updated.
*
* @param int $post_id The ID of the post being saved.
* @param WP_Post $post The post object being saved.
*/
function delete_filled_job_listing_transient_on_post_meta_update( int $post_id, WP_Post $post ): void {

if ( 'job_listing' === $post->post_type ) {

if ( '1' === get_post_meta( $post_id, '_filled', true ) ) {
delete_transient( 'hide_filled_jobs_transient' );
}
}

}
add_action( 'save_post', 'delete_filled_job_listing_transient_on_post_meta_update', 10, 2 );
Copy link
Member

@jom jom Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm curious about why this is separated from the other logic in this PR? Do we think this function is useful in the public API?

There is also the [possibly edge] case where a job went from filled to not filled. Do you think that is worth flushing this transient after each job listing safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jom Ah, I hadn't realised the functions file was dedicated to the API. I'll move this to post-types.php.

One thing I'm having difficulty with is getting the method to trigger on updated_post_meta. What I'd want to do ideally, following Peter's comment above, is:

  1. Set the transient
  2. If a job has the _filled meta updated, either add that ID to the transient or remove it

Clearing it on every post save is far too much and makes the transient pointless imo.

Copy link
Member

@jom jom Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I always get a little nervous about manipulating cached data (besides flushing it) because there are so many weird scenarios that could affect it. However, I suppose the weird ones would just happen for the life of the transient. In this case, one possible strangeness could happen in post status changes because to populate the transient you're querying published posts. While the query is inefficient generally, I think it isn't so bad for it to run it again when a possible change has been detected.

What issues were you experiencing with updated_post_meta? It be possible to do a slightly lightweight flush on that and transition_post_status.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jom So I've learned that I was doing a check like:

if ( '_filled' !== $meta_key || 'job_listing' !== get_post_type( $object_id ) ) {
	return;
}

But in debugging, I noticed that $meta_key is only ever returning _edit_lock, so what actually works is:

if ( '_edit_lock' !== $meta_key || 'job_listing' !== get_post_type( $object_id ) ) {
	return;
}

But I don't imagine checking against _edit_lock is all that useful. I'm stumped on how to check that the _filled meta was changed. I've committed what I have now at de746c4