Migrate custom status to use term_meta #43
Conversation
…ed metadata fields on the backend
|
|
||
| // This is taxonomy name used to store all our custom statuses | ||
| const TAXONOMY_KEY = 'post_status'; | ||
| const TAXONOMY_KEY = 'vw_post_status'; |
There was a problem hiding this comment.
Changed the taxonomy so it won't interfere with Edit Flow. I left the slug as is as that would create more problems imo, since we have some statuses that cannot be deleted and their slug matches core slugs. We would need to add in branching logic to tackle that. This keeps it a bit simpler.
|
|
||
| // The metadata keys for the custom status term | ||
| const METADATA_POSITION_KEY = 'position'; | ||
| const METADATA_REQ_EDITORIAL_FIELDS_KEY = 'required_metadata_fields'; |
There was a problem hiding this comment.
I went back and forth on calling this ID or fields. I left it as fields to mark the purpose of this field. Unlike the users field, there are no full EM objects being given back.
There was a problem hiding this comment.
How about required_metadata_field_ids?
There was a problem hiding this comment.
I had that, but it became such a mouthful that I reverted it. I'll change the fields to ids
|
|
||
| wp_localize_script( 'vip-workflow-custom-status-configure', 'VW_CUSTOM_STATUS_CONFIGURE', [ | ||
| 'custom_statuses' => $this->get_custom_statuses(), | ||
| 'editorial_metadatas' => EditorialMetadata::get_editorial_metadata_terms(), |
There was a problem hiding this comment.
There's taxonomy conflicts that happen due to the different inits calling get_terms/get_term. As a result, this is way safer to do so the client can handle the mapping. We could do some mapping but I didn't want more conflicts happening here to.
Perhaps after custom status is migrated, we can revisit this. I don't feel like the refactor would fix this but I could be wrong.
| <script type="text/javascript"> | ||
| var custom_statuses = <?php echo json_encode( $all_statuses ); ?>; | ||
| var current_status = '<?php echo esc_js( $selected ); ?>'; | ||
| var current_status_name = '<?php echo esc_js( $selected_name ); ?>'; |
| $required_metadata_fields = $args['required_metadata_fields'] ?? []; | ||
| $required_user_ids = $args['required_user_ids'] ?? []; | ||
|
|
||
| // In case of failure, data cleanup happens which includes the term and the meta keys. |
There was a problem hiding this comment.
This ensures we don't have bad data sitting around that we need to handle.
Ideally, once all our features are clean we should have a regular cron job that goes in and deletes all the bad data that could exist.
| return $allcaps; | ||
| } | ||
|
|
||
| // ToDo: Once custom status has been refactored, the below CRUD methods should be unified with the |
There was a problem hiding this comment.
This was my little todo to create a generic DB utility class that could handle CRUD operations rather than us repeating the code everywhere.
There was a problem hiding this comment.
Agree! This would be a great way to pull EM and custom-status meta getters and setters into per-datum classes.
There was a problem hiding this comment.
That being said, let's create a separate issue for this ToDo and ideally remove all of the ToDos in this PR.
There was a problem hiding this comment.
Yip, I was gonna do that before a merge just so all the issues are made in one go. 100% agreed though
| $field = 'term_id'; | ||
| $custom_status = get_term( $value, self::TAXONOMY_KEY ); | ||
| } else { | ||
| $custom_status = get_term_by( $field, $value, self::TAXONOMY_KEY ); |
There was a problem hiding this comment.
This is much faster imo.
| const [ requiredUsers, setRequiredUsers ] = useState( customStatus?.meta?.required_users || [] ); | ||
|
|
||
| // Taxonomy conflicts arise if this is done server side, so this transient field is only set here. | ||
| const [ requiredMetadatas, setRequiredMetadatas ] = useState( () => { |
There was a problem hiding this comment.
Due to the taxonomy problems, this is needed. I noticed I had some bad data sitting around so this helps to clean it up whenever an update happens. It's fine if its silent right now, but ideally we should handle this on the backend with a cron cleanup job that has proper logging.
There was a problem hiding this comment.
What do you mean by "taxonomy problems"/"taxonomy conflicts" here? I'm not sure what problem this is solving.
There was a problem hiding this comment.
What I originally had:
- Filter against
get_termandget_termsin EM that used it's taxonomy to populate each term with the necessaryterm_meta. - Filter against
get_termandget_termsin CS that used it's taxonomy to populate each term with the necessaryterm_meta.
What ended up happening was related to init problems:
- Within EM, CS taxonomy was given back. That was rejected because we only target the EM taxonomy so no
term_metawas populated. - Within CS, EM taxonomy was given back. That was rejected because we only target the CS taxonomy so no
term_metawas populated.
So the safest option to me was:
Just do this on the client side for now.
What I'd like to do ideally:
Add a GET api to CS and EM endpoints and add this functionality there. Ideally that's how all this data massaging is done and it should hopefully not have as many conflicts.
| use VIPWorkflow\Modules\Custom_Status; | ||
| use VIPWorkflow\VIP_Workflow; | ||
|
|
||
| class RequiredFieldsCronCleaner { |
There was a problem hiding this comment.
Common utility class that handles the cleanup of the required fields without needing to duplicate the code everytime. This would be an ideal candidate for a cron cleaner as well imo.
There was a problem hiding this comment.
I see the use of Cron in the name here, which I would take to mean there's scheduled cron calls. I'm not seeing anything cron-related though, am I missing something?
There was a problem hiding this comment.
I added it on purpose because we can add cron to this down the line, to handle data cleanup. I meant for this to be a central cleaner of data I suppose.
| return true; | ||
| } | ||
|
|
||
| private static function is_valid_user_ids( $user_ids ) { |
There was a problem hiding this comment.
Made the user lookup a lot faster by making it a simple batch SQL query.
| } | ||
|
|
||
| // ToDo: Optimize this to batch fetch metadata terms, and only include the ID field. | ||
| foreach ( $metadata_ids as $metadata_id ) { |
There was a problem hiding this comment.
Same thing as below needs to happen here, I've punted it for later as that's a new method that needs to go into EM.
| public static function insert_editorial_metadata_term( array $args ): WP_Term|WP_Error { | ||
| $term_to_save = [ | ||
| 'slug' => $args['slug'] ?? '', | ||
| 'slug' => $args['slug'] ?? sanitize_title( $args['name'] ), |
There was a problem hiding this comment.
Noticed via tests that this wasn't being set properly if it wasn't passed in.
| add_action( 'admin_menu', [ $this, 'add_admin_menu' ], 6 /* Prior to default registration of sub-pages */ ); | ||
|
|
||
| // These seven-ish methods are hacks for fixing bugs in WordPress core | ||
| add_action( 'admin_init', [ $this, 'check_timestamp_on_publish' ] ); |
There was a problem hiding this comment.
I ended up using the chance to clear our this core hack that was in place. Essentially, by adding date_floaing to our custom statuses it's able to get pulled into any of the get_post_stati calls as they check if the date_floaing is true or not. This allows the timestamp to get set correctly. To test this, you have to ensure your timezone offset is set correctly. Before I added the date_floating, if you took out the hacks you'd see the GMT time only be set even if your timezone was set correctly. Also, the exact time of publishing was off a bit.
With this, we should be down 2 core hacks which is nice.
There was a problem hiding this comment.
|
Taking a look at this PR! From initial testing I'm unable to get editorial metadata to register correctly. I tried deleting the I'm also seeing a truckload of error logs via I also tried deleting the |
|
Okay! Fresh install fixed it, I think I'm all set to test. |
| return []; | ||
| } ); | ||
|
|
||
| console.log( requiredMetadatas ); |
There was a problem hiding this comment.
Left-over console log here.
|
|
||
| public static function init(): void { | ||
| // Remove deleted users from required users, and if a user is reassigned, update the required users | ||
| add_action( 'delete_user', [ __CLASS__, 'remove_deleted_user_from_required_users' ], 10, 2 ); |
There was a problem hiding this comment.
I think this is a step toward more complexity from the prior implementation in required-user-ids.php, because we're combining the cleanup of several different types of metadata into a larger file, as opposed to having small encapsulated behaviors in meta-specific files. I see what you're trying to do here with combining functionality, but overall I think the prior logic is easy to read, simple, and directly scoped to the metadata type. These two filters aren't really connected other than the fact that they have similar logic for filtering a list.
If we add cleanup for new features, we'll probably need to expand this file to support those types, or if it doesn't fit this abstraction we'll need to change the abstraction or expand the file with unrelated code. Where possible, I think the best way to fight complexity is to keep file sizes smaller and class responsibilities self-contained, and I'm feeling like the design here goes in the opposite direction.
There was a problem hiding this comment.
Talked it over, and we will do a hybrid approach:
- Keep the individual files focused on cleaning up one field
- Move the common code to a utility file
| * @param string $taxonomy_key The key of the taxonomy to get the required fields from | ||
| * @return array The required ids required to make changes to the status | ||
| */ | ||
| public static function get( int $term_id, string $taxonomy_key ): array { |
There was a problem hiding this comment.
These should probably be private functions, unless we're expecting to call RequiredFieldsCronCleaner::get( ... )/update( ... ) from other files.
There was a problem hiding this comment.
Yip, this should be private
| $status->$key = $value; | ||
| } | ||
| } | ||
| $statuses = array_map( [ $this, 'add_metadata_to_term' ], $statuses ); |
There was a problem hiding this comment.
I think a great alternative to add_metadata_to_term() would be if individual metadata fields could add themselves, e.g.
$statuses = array_map( function( $status ) {
$term_meta = apply_filters( 'vw_register_custom_status_meta', [], $status );
$status->meta = $term_meta;
}, $statuses );Then if we have individual files for metadata fields, we could have self-contained method where we add metadata for a particular field, like this for user IDs:
add_filter( 'vw_register_custom_status_meta', [ __CLASS__, 'add_term_meta'] );
function add_term_meta( $term_meta, $status ) {
$user_ids = get_term_meta( $term->term_id, self::META_KEY, true );
if ( ! is_array( $user_ids ) ) {
$user_ids = [];
}
$term_meta[ self::META_KEY ] = $user_ids;
return $term_meta;
}This would give us an easier way to add new metadata without continuing to grow this function and isolate individual metadata handling. What do you think?
| * @param array $args The arguments to encode | ||
| * @return string Arguments encoded in base64 | ||
| */ | ||
| public static function get_encoded_description( $args = array() ) { |
There was a problem hiding this comment.
🎉 for removing the base64 encoding!
| return $term; | ||
| } | ||
|
|
||
| // if metadata is already set, don't overwrite it |
There was a problem hiding this comment.
When does this check become necessary?
alecgeatches
left a comment
alecgeatches
left a comment
There was a problem hiding this comment.
Big PR! Lot of changes in here I'd like to discuss. First though, here are a couple of UI issues I noticed:
-
Unable to removed saved EM fields on a custom status:
To replicate, save some required EM fields on a status, re-open the status, and attempt to remove a saved field. This crashes React with this error:
Uncaught TypeError: requiredMetadata is undefined <anonymous> metadata-select-form-token-field.js:22 MetadataSelectFormTokenField metadata-select-form-token-field.js:21 -- React 16 -- handleOnChange metadata-select-form-token-field.js:82 deleteToken components.js:60419 onTokenClickRemove components.js:60276 onClick components.js:59996 key components.js:2265 -- React 32 -- handleEditStatus workflow-manager.js:168 key components.js:2265 -- React 22 -- <anonymous> custom-status-configure.js:12 domReady dom-ready.js:71 <anonymous> custom-status-configure.js:8 <anonymous> custom-status-configure.js:14019 <anonymous> custom-status-configure.js:14021 metadata-select-form-token-field.js:22:3 -
Probably more importantly, I think the "Required Metadata Fields" might not be a good candidate for a
<FormTokenField>control:
It feels awkward to have to type a couple letters of a EM field to see a dropdown, especially when there's only a handful of options. I really just want a list of EM fields without needing to remember and type their names specifically. A multiselect-like component would be nice, or maybe a
<ComboboxControl>for EM field selection that dumps the selected fields into another control.If you disagree or now doesn't seem like the time, the
__experimentalExpandOnFocusflag for<FormTokenField>improves the UI a bit. It's a big buggy in my testing whether or not suggestions show up on first click (it seems like you need to type something and then backspace to see all suggestions), but in general it makes finding EM fields easier. -
This is more of an unrelated UI thing I noticed today, but it would be great if EM text fields saved and dismissed on enter key press. I kept expecting that to work when using those fields:
There are some other design changes I've discussed in comments. The overarching discussion revolves around centralizing our meta management code versus splitting it into files that manage each bit of metadata.
I'm in favor of splitting meta management into smaller self-contained files that use filters to interact with the bigger picture. With that noted, this PR is still a great improvement over where we were, so that discussion isn't a blocker. I would like to plan splitting some of these larger files into separate self-contained classes in the future, though, or at least discuss it.
…and registration method and remove console.log
|
I've fixed everything save for 2 items:
|
Description
term_meta, similar to Migrate EM to use term_meta #41Testing