-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Add User-friendly Attribute Names and Values to Order Line Items Metadata #27492
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
Conversation
Previously, I was using my own function. This time, we're using the function available in WC_Order_Item but sanitizing the results after.
This is just the schema and the actual functionality isn't implemented yet.
This resolves the intentionally failing WC_Tests_API_Orders_V2::test_get_item_with_line_items_meta_data.
This currently fails because the expected meta_data properties are only available in V3.
This currently fails because it's not implemented yet.
I suppose I could have deleted the attribute and term first but I think this is better and shorter. Maybe.
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.
Note that I have already approved the commits in original PR once, so only one review is needed.
|
cc @shiki |
The new meta_data output will include keys like `display_key` and `display_value`. The assertion will be re-added in a later commit.
Previously, we were using the `$formatted_meta_data` to build the final array. However, this does not consider the fact that `WC_Order_Item->get_formatted_meta_data` can exclude `meta_data` from the result. There would be less `meta_data` objects return than the previous implementation. This fixes the issue by using the `$data['meta_data']` value as the main list of meta data and only using `$formatted_meta_data` to optionally apply the `display_key` and `display_value` properties.
|
@vedanshujain I have submitted a PR to fix the failing unit test. → #27559 |
|
@vedanshujain Any chance this might be reviewed soonish and merged? Trying to establish a timeline when we can tell users to expect a resolution to the fulfillment issues in the mobile app. Thanks! 😃 |
|
Hi @claudiosanches! Let me know if there's anything I can help to get this merged. 🙂 I'm not quite sure what the Travis failures are. Are they coding style issues? |
This resolves the intentionally failing WC_Tests_API_Orders_V2::test_get_item_with_line_items_meta_data.
This currently fails because the expected meta_data properties are only available in V3.
This currently fails because it's not implemented yet.
I suppose I could have deleted the attribute and term first but I think this is better and shorter. Maybe.
|
Looks like the test is failing because the formatting function is excluding non-scalar entities (an array in our case). This a bug that needs to be fixed before we merge, I am working on the fix, should have it ready by tomorrow. |
|
@vedanshujain Ah, I see. I believe that's the one I fixed in this PR → #27559. |
ecef3f7 to
8e12de5
Compare
|
LGTM now asked for another review. Marking as a high impact change since it changes the returned data format of a nested property. |
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.
|
Merging as tests passed, the code standard test is failing on all PRs for unrelated reason looks like. |

From woocommerce/woocommerce-rest-api#229