Render order edit page

[vedanshujain]

All Submissions:

• Have you followed the WooCommerce Contributing guideline?
• Does your code follow the WordPress' coding standards?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Implements an order edit screen which is based on the WP's default post edit screen. With COT, we can't use the default post edit screen since it needs a post object to work properly. This PR implements a lightweight version of edit-form-advanced.php with reusing code as much as possible.

Existing order meta boxes are also modified to work with both orders and post objects.

Note that this only implements rendering, saving an order will not work just with this PR.

Closes #32471 .

How to test the changes in this Pull Request:

1. Use the instructions in Admin list table [COT] #32864 to enable the COT-based UI.
2. Click on any order to open the order edit page.
3. Check that data is populating correctly in all default meta boxes.

Note that since this uses a lot of current code with some changes, it's also necessary to test out the current order edit screen. To do that:

1. Switch back to post-based data store from Settings > Advanced > DataStores.
2. Refresh the WooCommerce homepage so that correct links for the order list and edit tables are populated.
3. Go to the edit screen for any order. Check that meta box functionality, especially for file download and custom meta box is working as expected.

Known issue

• Currently, refund data will not populate on the refund screen since the functionality is not yet implemented.

Other information:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes, as applicable?
• Have you successfully run tests with your changes locally?
• Have you created a changelog file for each project being changed, ie pnpm changelog add --filter=<project>?

FOR PR REVIEWER ONLY:

• I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

[barryhughes]

A little unsure if we should address here or in a follow-up PR, but the Screen Options pull-down (for the COT editor) is the same as for the order list table. Screenshot:

Number of items per page and column header checkboxes don't really make sense here. To compare, with the CPT-editor we see:

[barryhughes]

There is a minor different with the meta box title at the top of the editor page:

CPT	COT

"Order" is missing before the order ID. I don't think this matters a lot, in fact I think this is a little cleaner, but flagging it as:

• It's happening because $order_type_object is null over here (we are trying to access properties on null)
• I was idly wondering if this might be a problem elsewhere (ie, for third party code also attempting to work with the post type object)

[barryhughes]

This is looking great, nice work. I left some comments above (mostly minor), but still need to perform some more testing relating to the existing order editor.

[barryhughes]

Porting some extensions that integrate with the existing order editor to this new COT order seemed straightforward; one minor detail is custom meta boxes with context: 'side', priority: 'high' previously rendered above Order Actions, like the following:

Now they render below it—but since meta boxes are user orderable in any case, and since most extensions will anyway require a couple of tweaks to integrate with the new editor, I don't think we really need to change anything.

[github-actions]

Test Results Summary

Commit SHA: b0f491d

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	115	0	0	2	0	117	0m 49s
E2E Tests	185	0	0	1	0	186	16m 59s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

[vedanshujain]

It's happening because $order_type_object is null over here (we are trying to access properties on null)

This does look like a backward compatibility issue, I have added an issue to track a fix here #33767

A little unsure if we should address this here or in a follow-up PR, but the Screen Options pull-down (for the COT editor) is the same as for the order list table. Screenshot:

Where do I see these options from? For me, there is just a Help action. I will check if it's disabled somehow.

[barryhughes]

Where do I see these options from? For me, there is just a Help action.

That's the right spot, but there should also be a Screen Options pulldown next to it:

It's rendering for me in both (CPT and COT) flavors of the list table as well as the order editor.

Edit: nope, I'm wrong. On testing the latest changes I don't see it within the COT editor (but I do see it in the CPT editor). Should we add it, or, log as a follow-up issue perhaps?

[barryhughes]

An interesting thing: we are passing an order object to do_meta_boxes() here whereas, in the legacy editor, do_meta_boxes() receives WP_Post objects instead.

Execution proceeds from do_meta_boxes() along to WC_Meta_Box_Order_Actions::output() and this method interacts with what it thinks is a post object, and tries to access post object properties directly.

Everything still mostly works (though, I'm really only testing the rendering of course) but unfortunately it results in a lot of notices being generated since some of the properties that exist for WP_Post objects don't exist for order objects and, even where there is overlap, direct access to properties is a no-no for orders and results in doing-it-wrong notices.

In my testing, I see 39 notice-level errors (related to this issue) being emitted each time the editor is loaded. Apologies for not spotting this on my earlier pass.

[barryhughes]

Looking good!

Two remaining items from my perspective:

• A lot of doing-it-wrong and getting-property-of-non-object notices (see this note)
• Unsure if we want to tackle adding the Screen Options pulldown in this PR, or separately (see this note)

[vedanshujain]

Note that I am working on adding a compatibility file that will have a bunch of corresponding functions for files containing equivalent post functions https://codex.wordpress.org/Function_Reference. This is needed to properly make the doing_it_wrong message go away.

[barryhughes]

Sounds good: provisionally hitting approve, then (I'm not completely clear if you are planning to tackle those things through a different PR or PRs, but if so this is at a good point to merge) 👍

[Konamiman]

Overall looks good, but I hereby request a change for the sake of code maintainability.

[Konamiman]

Thanks for adding this. However (sorry I know it's more work) I think that methods shouldn't be static, instead the class should be registered in the container and the instance should be retrieved using wc_get_container (or declared as an argument of the init method in classes using it).

The reason for this is testability: it's very easy to replace a class with a mock in unit tests if the class has been registered in the container, but mocking static methods requires the usage of the Code Hacker which is an ugly workaround reserved for legacy code. Yes we have utility classes with static methods (e.g. ArrayUtil) but in these cases the methods are idempotent: the output depends exclusively on the input (no shared/global state involded) so there's really nothing to mock.

Also this is not an Internal class so we can't play the "we'll modify it later" card in this case. 😄

[vedanshujain]

In this case, there is no shared state as well. If you are referring to the global theorder then see my next comment.

[Konamiman]

I meant "shared state" in a generic sense, as in "anything that is not strictly the arguments passed to the method". In this case, the result of the methods of the class depend on the database state in various ways.

Let me illustrate my concern about testability with an example. Imagine that we are testing a second class that calls OrderUtil::get_post_or_object_meta($some_post), and we want to verify that the tested class behaves differently depending on whether the passed post has a given meta key or not:

• If get_post_or_object_meta is static, in order to setup the test we need to either actually introduce some meta values in the database (cumbersome) or patch the static method with the code hacker (not ideal, this should only be needed for legacy code).
• But if get_post_or_object_meta is a regular instance method, and the tested class does wc_get_container()->get(OrderUtil::class)->get_post_or_object_meta($some_post) instead, then in the test we can easily replace the instance of OrderUtil with a mock that directly returns whatever we want (especially since we introduced the DynamicDecorator class).

I realize that static methods are easier to use, and especially for external contributors being able to just e.g. OrderUtil::custom_orders_table_usage_is_enabled() would be very conveniet. But this is something to be considered and solved separately (e.g. by adding a second static version of the method, or adding a proxy class with static methods of the class, or something of that style).

[vedanshujain]

I see thanks, I have updated by adding instance method of these functions in a8d4a7f. Note that custom_orders_table_usage_is_enabled is not instansicized as it is already a static proxy for an instance method.

[Konamiman]

I think we should avoid introducing new global objects, for maintainability and testing purposes. I suggest replacing this one with a pair of set_current_order and get_current_order methods in the OrderUtil object (which, as per my previous comment, would be a shared instance registered in the container); the order object itself would be stored as a private property in the OrderUtil instance.

[vedanshujain]

This is not a new object, if you see the usages, this object is already defined by the callee function. This method only helps in initialising by either a Post or an Order object, but when it's called global $theorder is already being used.

[Konamiman]

Ok, I get it. But anyway, global objects still make things difficult to test.

So I wonder if we could kind of "soft deprecate" the global variable by encapsulating it in a pair of set_current_order and get_current_order methods, and start using those ourselves (instead of directly the global object) for any new code, thus encouraging others to do the same.

In that case, this method can stay as is, but perhpas it could be renamed to init_current_order.

[vedanshujain]

I agree with your point about soft deprecating theorder, but I don't think it should be part of this PR. It's going to need much intentional and careful approach, rather than doing it in an unrelated PR.

[github-actions]

Hi @vedanshujain, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

• Add the release: add testing instructions label