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

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 1 commit
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
35 changes: 21 additions & 14 deletions 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,7 +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_expired_job_listings_from_search' ] );
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 @@ -390,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/wpjobmanager.com/issues/420.
*/
$is_site_search = ( isset( $_GET['s'] ) );
$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 @@ -699,28 +714,20 @@ public function get_filled_job_listings(): array {
*
* @return void
*/
public function maybe_hide_filled_expired_job_listings_from_search( WP_Query $query ): void {
public function maybe_hide_filled_job_listings_from_search( WP_Query $query ): void {
$hide_filled_positions = get_option( 'job_manager_hide_filled_positions' );
$hide_expired = get_option( 'job_manager_hide_expired' );

if ( ! $hide_filled_positions && ! $hide_expired ) {
if ( ! $hide_filled_positions ) {
return;
}

if (
! is_admin()
&& $query->is_main_query()
&& $query->is_search()
|| $query->is_archive() && 'job_listing' === $query->get( 'post_type' )
&& ( $query->is_search() || $query->is_archive() )
) {

if ( $hide_expired ) {
$query->set( 'post_status', 'publish' );
}

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

Expand Down
4 changes: 2 additions & 2 deletions wp-job-manager-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1719,7 +1719,7 @@ function job_manager_get_salary_unit_options( $include_empty = true ) {
* @param int $post_id The ID of the post being saved.
* @param WP_Post $post The post object being saved.
*/
function delete_job_listings_transients_on_save( int $post_id, WP_Post $post ): void {
function delete_filled_job_listing_transient_on_post_meta_update( int $post_id, WP_Post $post ): void {

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

Expand All @@ -1729,4 +1729,4 @@ function delete_job_listings_transients_on_save( int $post_id, WP_Post $post ):
}

}
add_action( 'save_post', 'delete_job_listings_transients_on_save', 10, 2 );
add_action( 'update_post_meta', 'delete_filled_job_listing_transient_on_post_meta_update', 10, 2 );