-
Notifications
You must be signed in to change notification settings - Fork 29
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
when fetching posts for items, exclude drafts #101
Conversation
this is a PR for issue #98 |
Thanks for starting this. I'm guessing that, in the vast majority of cases, |
Yeah that seems fine by me. I added a filter so the post statuses could be changed easily. I put the same filter in two spots, which seems a bit weird to me. Normally, I'd put that logic into a function/method, and add the filter onto its return value, and call it from the two spots. But I'm not sure if we want to embark on that extra bit of refactoring here... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mnelson4 Thanks for working on this! The technique looks good to me. Since we're using the same filter in two places, would you mind moving the logic into a standalone function (maybe in functions.php)? That'll also make it easier to add the proper hook documentation.
includes/class-ajax-handlers.php
Outdated
'order' => 'DESC' | ||
'order' => 'DESC', | ||
'post_status' => apply_filters( | ||
'anth_included_post_stasuses', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we please change this to 'anthologize_source_item_post_statuses'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
yeah I'll give it a shot |
… in order to avoid having the same filter twice
I presumptuously added a new method to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! Thank you.
Ah, I hadn't noticed. In general, best to use the whole unique identifier, unless there are character limits (like the 20-character limit on post type names, thus |
👍 |
When fetching posts from the project organizer, exclude draft posts by default. Specifically, only include published, pending, future, and private posts.
Maybe we should only include published and private posts by default, though?
Or maybe we should have a group of checkboxes (or multi-select) to show all the post statuses to choose from?
This isn't really meant to be the final PR, this is mostly meant to just get the ball rolling.