Refactor the remaining modules #59

ingeniumed · 2024-10-04T11:08:55Z

Description

This is a big PR and needs to be reviewed and tested thoroughly. I have refactored the remaining modules to be independent, so all the remaining Edit Flow logic is gone. I've left the folder structure in place as that felt like it'd be way too much. That would be the next item on the list imo.

I have also updated the minimum PHP version to 8.1 and the minimum WordPress version to 6.4.

Module specific changes:

Custom Status: I've made the pending review status mandatory. That felt like it's necessary because some weird bugs were beginning to show up where the pending status was being duplicated. If no statuses are present, we assume just draft and pending are allowed and those are the default statuses in core to begin with. So I've left that as is. I've also shifted all the core hacks away to their own class.

Settings: Everything is isolated to this module, but all options related operations happen through one utility. It ensures that the default option values exist and if we do enable migrations that can go there. I've also updated the settings updated notice to be an admin banner similar to our regular notice.

Items remaining that we need to tackle for the future:

Supporting post types beyond the two default ones is not present right now. Technically we didn't have it working properly before, because we deleted the Edit Flow code. So we need to that back in to ensure we support that.

Fixes #45, #46, and #49

Steps to Test

Setup the plugin from scratch (delete everything)
Ensure all the defaults get setup
Ensure everything works

…uble status creations and the lack of settings updated message

modules/custom-status/custom-status.php

modules/settings/settings.php

modules/settings/views/settings.php

ingeniumed · 2024-10-08T04:01:05Z

modules/shared/php/options-utilities.php

+	const OPTIONS_GROUP_NAME = 'vip_workflow_options';
+
+	// Its key to centralize these here, so we can easily update them in the future
+	private const DEFAULT_OPTIONS = [


I've kept the default options here but I'd like to move this eventually to a typed class that we can rely on easily.

ingeniumed · 2024-10-08T04:03:01Z

modules/shared/php/options-utilities.php

+		$module_options = get_option( $module_options_key, new stdClass() );
+
+		// Ensure all default options are set
+		foreach ( self::DEFAULT_OPTIONS as $key => $value ) {


This is the new location for the default options being saved.

tests/modules/custom-status/meta/test-required-metadata-id-handler.php

…sts, and shift away another helper method that's shared

ingeniumed · 2024-10-08T04:27:47Z

modules/shared/php/core-hacks.php

+
+use WP_Post;
+
+class CoreHacks {


This is the central class with all the core hacks

modules/custom-status/custom-status.php

modules/shared/php/options-utilities.php

modules/custom-status/custom-status.php

alecgeatches · 2024-10-08T22:39:45Z

modules/custom-status/custom-status.php

+	 *
+	 * @return array
+	 */
+	private static function get_core_post_statuses(): array {


Similar point as some other comments, but I'm not sure why we need "core" post statuses, any more than we need core EM fields. If we have 0 or 1 statuses, it would be great if we could handle that gracefully in code. Don't need to do that in this PR, just a general thought.

Actually, this is something that we should remove entirely. If the post type is unsupported, none of our code should load which means we shouldn't even be doing anything.

I added this back in because without this, the sidebar fails to load. We need to issue that initial status change into draft, after which we can let Gutenberg take over. This is because initially the post type is not available as it's a clean new post.

This is also why I added pending review as a restricted status because technically that and draft are required by default in core. Us allowing that to be renamed or altered is going to mean extra logic on our end that we need to handle. That is,

Pending review can be renamed so the label that's used in the custom status registration would need to be tested

But, the slug itself won't be and will stay the same

Instead, for now I think we should just leave pending review and draft as statuses that cannot be altered or touched in any way. Not saying we don't do it mind you just saying for now we do that and revisit it.

modules/custom-status/meta/position-handler.php

modules/settings/settings.php

…n of the editorial metadata lookup

ingeniumed · 2024-10-09T05:16:57Z

modules/custom-status/custom-status.php

 	 */
-	private function get_custom_statuses_with_editorial_metadata() {
+	private static function modify_custom_statuses_with_editorial_metadata(): array {


I made a much more optimized version of the previous method. I also updated the description to mention why it's there. No matter what I did, something broke so I had to do it this way.

If there is a good way to link two taxonomies together to mirror how a foreign key relationship works then we can do that. Otherwise, we need to add this in.

I think a better option is to stop forcing a UI optimization, and instead handle this how a regular backend talking to a client via a rest api would:

Add a get all and get by id to CS and EM

Scrap the server side rendered CS and EM fields

Have the UI make a call to the get all to render the list, and get by id to render the individual fields as needed

Lazy load everything to have the UI render quickly and individually make the necessary calls to assemble all the information needed

This would still benefit from the VIP caching logic. It'll be a bit tricky for the block editor, so we can start with the admin and slowly implement it for the block editor.

I agree that this is a bit weird of a setup. I'm still not sold on REST being the solution, as we're doing the same work but asyncronously and client-side after the page loads. For page state that determines if the save button is disabled, it'd be great to not have to wait for REST calls to determine what to show.

Another idea here could be to pass custom statuses and relevant EM fields separately. Custom statuses have an array of EM IDs, so it wouldn't be hard to have the frontend code in charge of selecting and rendering the relevant fields. It would be cleaner then stuffing data into an existing object, which feels icky here, and still let us avoid the REST roundtrips after load.

This is still 👍 for now, just thinking about how we'd change this in the future.

alecgeatches · 2024-10-09T16:07:12Z

tests/modules/custom-status/rest/test-custom-status-endpoint.php

 	 */
 	protected function setUp(): void {
 		parent::setUp();

+		// Reset all module options
+		OptionsUtilities::reset_all_module_options();


If we need to reset options and install for each test, we could put these in WorkflowTestCase setUp()/tearDown() methods.

Addressed in this comment below.

…ods, remove unnecessary per-test cleanup

alecgeatches · 2024-10-09T17:21:28Z

@ingeniumed Per the test comment above, I spent time looking into why we needed to sprinkle

OptionsUtilities::reset_all_module_options();
CustomStatus::setup_install();

around some tests but not others. I found some inconsistencies in our tests, and was able to fix those and simplify our testing code in c69cb67. The main overview:

Some of our tests were extending WP_UnitTestCase, and some were extending the vanilla PHP TestCase. It's hard to find explicit documentation for WP_UnitTestCase, but here's what I discovered:
- The WP_UnitTestCase_Base class that WP_UnitTestCase extends is in this repository. From looking at this class, we can see it automatically wraps up everything in a transaction and handles data cleanup. It also clears out WordPress globals and other things that could cause edge cases in tests.
- These tests were failing because those that specifically extended WP_UnitTestCase were getting automatic cleanup and causing our initialized data to disappear.
- WordPress also has some related documentation for using WP_UnitTestCase in Writing PHP Tests.
- I found some setup documentation for integrating WP's classes with composer in the wp-phpunit/docs repository, which is why bootstrap.php changed.
With this information, all tests have been changed to extend WP_UnitTestCase. This gives us some advantages:
- We also get factories for common object types like user and post. I replaced our custom create_user() method with $this->factory()->user->create() calls.
- We no longer need to process post/EM field cleanup, because that data is erased on each test run now anyway. Although WP_UnitTestCase has an overhead per-test, we get some of that time back by not needing to explicitly run cleanup for individual data.

As a side note I noticed that options changes still need to be explicitly cleared, so WorkflowTestCase does the reset/setup steps in its setUp() so none of our tests need to worry about it.

Overall, this should make our tests more WordPressy, resilient to weird edge-cases, and make it easier for us to add more tests.

alecgeatches · 2024-10-09T17:42:03Z

I'm going to merge. Thank you for all of the hard work on this PR!

ingeniumed added 5 commits October 4, 2024 16:12

Refactor custom status to be static

0630576

migrate the workflow class

fcc0e6b

Refactor the remaining modules

9fe1fc4

Fix missing imports

e653884

Fix the settings not showing up

2ccbe18

ingeniumed self-assigned this Oct 4, 2024

ingeniumed added 2 commits October 4, 2024 21:12

Fix return type

a7b56a2

Fix the default options not being set, the settings menu ordering, do…

086e8f9

…uble status creations and the lack of settings updated message