Add methods to hide filled and expired posts #2449

anaemnesis · 2023-06-06T21:19:33Z

Changes proposed in this Pull Request

Adds three new methods: (1) get_filled_job_listings(), (2), get_expired_job_listings and (3) maybe_hide_filled_expired_job_listings_from_search
The first two methods only run if the "Hide Filled Jobs" or "Hide Expired Jobs" options are enabled. If they aren't, an empty array is returned.
The third method returns early if the "Hide Filled Jobs" and "Hide Expired Jobs" options are disabled. If they aren't, a post__not_in parameter is applied to $query->set when not in WP Admin, and when on a WP search results page.

Unlike the previous implementation, this one relies on get_posts() and meta_query but without the expensive OR relation.

Because expired jobs already have their post status updated to expired if a post's expiry date is set in the past or elapses, we don't need to meta_query for expired posts but can instead look for job_listings with an expired post status.

We don't have the same luxury with filled posts, but we can do a singular meta_query without any relations to retrieve these.

My understanding is that these changes should be far less expensive than the previous implementation.

To further help with performance, the results of get_filled_job_listings and get_expired_job_listings are stored in a transient that is expired every 24 hours, which lines up with the various job expiration methods.

Either transient is then cleared on post update if: (1) we're looking at a job listing, (2) the '_filled' post meta has been updated, or (3) the post status has changed from publish to expired, trash or vice-versa.

Testing instructions

Check out branch
Create a few job posts, setting one or some to Expired with an expiry date in the past, or Filled by checking the Filled checkbox.
Check transients to ensure hide_filled_jobs_transient and hide_expired_jobs_transient is not set. With WP CLI these can be checked with wp transient get hide_filled_jobs_transient and wp transient get hide_expired_jobs_transient
Navigate to Job Listings --> Settings --> Job Listings and enable (as desired), the Hide Filled Positions and Hide expired listings in your job archives/search options
Check your transient again, which should now be populated with the post IDs of the posts you marked as expired or filled
Check WP Search results to make sure these posts, and only these posts, are excluded

If you would like to test on a large dataset, WP CLI can be used to bulk create posts:

Bulk create Expired posts

wp post generate --count=1000 --post_type=job_listing --post_status=expired

Bulk generate Filled posts

Filled posts are a little more troublesome because we're working with meta values, and wp post generate, as well as wp post create, do not touch these values.

So, you could do something like:

# generate our posts
wp post generate --count=1000 --post_type=job_listing --post_status=publish

# grab the IDs of some arbitrary number of posts, say, 100, and update the post meta
for post_id in $(wp post list --post_type=job_listing --post_status=publish --numberposts=100 --fields=ID); do
    wp post meta update \
        $post_id \
        _filled \
        1 \
       && echo "Post ID $post_id changed"
done

The above can be added to a file on your environment, like create-filled-posts.sh. Make sure to chmod +x $file and then ./$file.sh to execute the for loop and create your posts.

Note that when creating posts programmatically you may need to manually clear the transients.

- Adds two new methods to hide filled and expired posts - Will investigate to see if it's possible to do the same, but more performantly and without modifying `meta_query`

- Use get_posts to retrieve a list of Job IDs that are either expired, or marked as filled - Store IDs in a transient for caching/performance - Transient for the moment is set to cache for a day - Clear transients on post update if the post status changes, or post meta is updated - Transients are then regenerated when searching if the relevant "hide" option is enabled

- Updates the early return so that return properly if neither "hide_*" option is enabled, but if one is, allow the method to continue.

wp-job-manager-functions.php

includes/class-wp-job-manager-post-types.php

Add checks for: - is_main_query - is_archive - 'job_listing' is the post type To ensure that we don't impact any non-main queries, queries that don't pertain to 'job_listing', and that might also happen on archive pages.

- && 'job_listing' === $query->get( 'post_type' )

- Remove now unnecessary method to retrieve expired posts by ID - Exclude expired posts by setting the query to 'publish' post status only, if $hide_expired is enabled - Removed related expired transient code - Set the get_filled_job_listings() method to retrieve all posts, rather than 5

includes/class-wp-job-manager-post-types.php

wp-job-manager-functions.php

includes/class-wp-job-manager-post-types.php

- When using get_posts() to retrieved posts with a _filled meta value of 1, make sure we're only querying posts with a 'publish' post status.

- Removes $query->set() to hide $expired posts - Instead, changes the 'public' argue on register_post_status( 'expired' ) to 'true' or 'false' depending on whether we're on a search page, or the 'job_manager_hide_expired' option is enabled (or not in either case) - Also changes the filled jobs transient delete so that it triggers on 'update_post_meta', vs. 'save_post'

anaemnesis · 2023-06-22T10:47:59Z

includes/class-wp-job-manager-post-types.php

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


@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

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

I ended up reverting this due to issues checking conditionals like is_archive(). Committed 1a7b7da

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.

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

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.

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

82e75e6

…to quiet PHPCS. The change to `update_post_meta` broke tests, so revert that for now.

jom

I'll do some more testing later, but this is looking good! I added a few minor comments.

jom · 2023-06-26T11:24:10Z

includes/class-wp-job-manager-post-types.php

+		/**
+		 * 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.
+		 */


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

includes/class-wp-job-manager-post-types.php

jom · 2023-06-26T11:30:17Z

includes/class-wp-job-manager-post-types.php

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


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

includes/class-wp-job-manager-post-types.php

jom · 2023-06-26T11:40:22Z

wp-job-manager-functions.php

+
+/**
+ * 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 );


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?

@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:

Set the transient

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.

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.

@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

- Renamed to $is_expired_hidden to (hopefully) make the true/false more readable.

…return plainly.

…the transient will return an empty array if there are no post IDs anyway.

Setting 'public' to 'false' dynamically workes fine with $_GET['s'], however, the same method fails to return true when checking is_archive(), is_tax(), is_wpjm_*(), etc. This previous implementation is able to check conditionals as expected, keeps the code in one place, and works.

Refactors the maybe_hide_expired_posts() method to set the 'public' static to 'false', then back to 'true', during/after query generation.

aaronfc · 2023-07-06T14:20:23Z

includes/class-wp-job-manager-post-types.php

@@ -81,13 +81,15 @@ public function __construct() {
 		add_filter( 'wp_insert_post_data', [ $this, 'fix_post_name' ], 10, 2 );
 		add_action( 'add_post_meta', [ $this, 'maybe_add_geolocation_data' ], 10, 3 );
 		add_action( 'update_post_meta', [ $this, 'update_post_meta' ], 10, 4 );
+		add_action( 'updated_post_meta', [ $this, 'delete_filled_job_listing_transient' ], 10, 4 );


I believe this should be update_post_meta and not updated ? I couldn't find the updated_post_meta hook.

It is a sneaky dynamic one: http://developer.wordpress.org/reference/hooks/updated_(meta_type)_meta/

aaronfc · 2023-07-06T14:22:43Z

includes/class-wp-job-manager-post-types.php

+			! is_admin()
+			&& $query->is_main_query()
+			&& ( $query->is_search() || $query->is_archive() )


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.

Committed 27b3be7

aaronfc · 2023-07-06T16:41:03Z

includes/class-wp-job-manager-post-types.php

+			! is_admin()
+			&& $query->is_main_query()
+			&& ( $query->is_search() || $query->is_archive() )
+		) {


Same here about the comment explaining the logic.

Committed 27b3be7

aaronfc · 2023-07-06T16:44:21Z

includes/class-wp-job-manager-post-types.php

+	public function make_expired_public(): void {
+
+		global $wp_post_statuses;
+
+		if ( isset( $wp_post_statuses['expired'] ) ) {
+			$wp_post_statuses['expired']->public = true;
+		}
+
+	}


I couldn´t find any information about this global. Can you make a brief summary on what is it and what is this supposed to do?

@jom and I discussed this global in a call. AFAIK it's first mentioned here in post.php:

https://github.com/WordPress/wordpress-develop/blob/bea4307daa21a3f97d159898b0ac676332e0e7de/src/wp-includes/post.php#L1287-L1292

function register_post_status( $post_status, $args = array() ) { global $wp_post_statuses; if ( ! is_array( $wp_post_statuses ) ) { $wp_post_statuses = array(); }

It is an array of all post statuses. What we're doing here is checking that the expired post status is registered, and if it is, we either set its public argument to true or false via the maybe_hide_expired_job_listings() method.

From there, if $hide_expired is true, $this->make_expired_private(); sets the public argument to false during query generation, after which it sets it back to true with add_action:

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

Lines 732 to 741 in 82e75e6

if (

! is_admin()

&& $query->is_main_query()

&& ( $query->is_search() || $query->is_archive() )

) {

$this->make_expired_private();

add_action( 'posts_selection', [ $this, 'make_expired_public' ] );

}

Add methods to hide filled and expired posts

5afad7d

- Adds two new methods to hide filled and expired posts - Will investigate to see if it's possible to do the same, but more performantly and without modifying `meta_query`

anaemnesis linked an issue Jun 6, 2023 that may be closed by this pull request

WordPress search returns expired job #1884

Open

anaemnesis added 4 commits June 7, 2023 09:21

Remove unused method

7a924f6

Remove an 's' from 'jobs' in method name

2fccb36

Update maybe_hide_filled_expired_job_listings_from_search logic

eeb42e0

- Updates the early return so that return properly if neither "hide_*" option is enabled, but if one is, allow the method to continue.

anaemnesis self-assigned this Jun 13, 2023

Account for 'trash' post status

068a162

anaemnesis commented Jun 13, 2023

View reviewed changes

wp-job-manager-functions.php Outdated Show resolved Hide resolved

anaemnesis marked this pull request as ready for review June 13, 2023 20:45

anaemnesis requested review from gikaragia, aaronfc and a team June 13, 2023 20:45

yscik reviewed Jun 19, 2023

View reviewed changes

includes/class-wp-job-manager-post-types.php Outdated Show resolved Hide resolved

anaemnesis added 3 commits June 21, 2023 10:35

Add additional query checks

d1d04d4

Add checks for: - is_main_query - is_archive - 'job_listing' is the post type To ensure that we don't impact any non-main queries, queries that don't pertain to 'job_listing', and that might also happen on archive pages.

Remove post_type check, as search page is querying "any"

cd94313

- && 'job_listing' === $query->get( 'post_type' )

anaemnesis requested a review from yscik June 21, 2023 10:51

yscik reviewed Jun 21, 2023

View reviewed changes

includes/class-wp-job-manager-post-types.php Outdated Show resolved Hide resolved

wp-job-manager-functions.php Outdated Show resolved Hide resolved

includes/class-wp-job-manager-post-types.php Show resolved Hide resolved

anaemnesis added 2 commits June 21, 2023 13:31

Explicitly query 'publish' post status

fc8e201

- When using get_posts() to retrieved posts with a _filled meta value of 1, make sure we're only querying posts with a 'publish' post status.

anaemnesis commented Jun 22, 2023

View reviewed changes

anaemnesis requested review from jom and yscik June 22, 2023 10:48

anaemnesis added 2 commits June 22, 2023 11:53

Nonce verification for $_GET['s'] isn't necessary here, so let's try …

27685ac

…to quiet PHPCS. The change to `update_post_meta` broke tests, so revert that for now.

Update issue reference

d3b5123

jom reviewed Jun 26, 2023

View reviewed changes

yscik added this to the 1.41.0 milestone Jun 27, 2023

anaemnesis added 2 commits June 28, 2023 09:54

Rename $is_expired_hidden variable to be more readable

d23b3a3

- Renamed to $is_expired_hidden to (hopefully) make the true/false more readable.

Update PHP doc for the get_filled_job_listings() method to state the …

ea63e66

…return plainly.

anaemnesis added 3 commits June 28, 2023 10:08

Early return on get_filled_job_listings() method isn't necessary, as …

3a210fb

…the transient will return an empty array if there are no post IDs anyway.

Delete 'hide_filled_jobs_transient' on 'updated_post_meta' action.

de746c4

anaemnesis requested a review from jom July 2, 2023 21:27

gikaragia modified the milestone: 1.41.0 Jul 4, 2023

Refactor maybe_hide_expired_posts()

82e75e6

Refactors the maybe_hide_expired_posts() method to set the 'public' static to 'false', then back to 'true', during/after query generation.

gikaragia removed this from the 1.41.0 milestone Jul 5, 2023

aaronfc reviewed Jul 6, 2023

View reviewed changes

Add comments explaining conditional logic in the maybe_hide_* methods.

27b3be7

anaemnesis requested a review from aaronfc July 7, 2023 10:03

anaemnesis assigned aaronfc Jul 14, 2023

aaronfc removed their request for review July 25, 2023 06:17

yscik removed their request for review July 31, 2023 11:49

gikaragia marked this pull request as draft September 21, 2023 14:11

jom removed their request for review November 24, 2023 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add methods to hide filled and expired posts #2449

Add methods to hide filled and expired posts #2449

anaemnesis commented Jun 6, 2023 •

edited

anaemnesis Jun 22, 2023

jom Jun 26, 2023

anaemnesis Jul 2, 2023

jom Jul 3, 2023 •

edited

anaemnesis Jul 3, 2023 •

edited

jom Jul 3, 2023

anaemnesis Jul 4, 2023

jom left a comment

jom Jun 26, 2023

jom Jun 26, 2023

jom Jun 26, 2023 •

edited

anaemnesis Jun 28, 2023

jom Jun 28, 2023 •

edited

anaemnesis Jul 2, 2023

aaronfc Jul 6, 2023

jom Jul 6, 2023

aaronfc Jul 6, 2023

anaemnesis Jul 7, 2023

aaronfc Jul 6, 2023

anaemnesis Jul 7, 2023

aaronfc Jul 6, 2023

anaemnesis Jul 7, 2023

	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' ),
	]
	);

	if (
	! is_admin()
	&& $query->is_main_query()
	&& ( $query->is_search() \|\| $query->is_archive() )
	) {

	$this->make_expired_private();

	add_action( 'posts_selection', [ $this, 'make_expired_public' ] );
	}

Add methods to hide filled and expired posts #2449

Are you sure you want to change the base?

Add methods to hide filled and expired posts #2449

Conversation

anaemnesis commented Jun 6, 2023 • edited

Changes proposed in this Pull Request

Testing instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jom Jul 3, 2023 • edited

Choose a reason for hiding this comment

anaemnesis Jul 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jom Jun 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jom Jun 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anaemnesis commented Jun 6, 2023 •

edited

jom Jul 3, 2023 •

edited

anaemnesis Jul 3, 2023 •

edited

jom Jun 26, 2023 •

edited

jom Jun 28, 2023 •

edited