Skip to content

Conversation

@vedanshujain
Copy link
Contributor

From woocommerce/woocommerce-rest-api#229

Fixes #9.

Ref: p91TBi-2Lt-p2

This will also allow us to fix these issues for the WooCommerce iOS and Android apps:

woocommerce/woocommerce-android#1286
woocommerce/woocommerce-ios#1127
woocommerce/woocommerce-ios#2280

Background

For both the iOS and Android apps, we regularly receive feedback from users that they could not view the details of the variable products in the orders. Example: C013AAPA4G0-p1596070344020400-slack. To find a solution, @AmandaRiu investigated what the iOS and Android apps can do using the REST API and posted her findings at p91TBi-2Lt-p2.

The apps use the line_items name in the GET /v3/orders response. However, the name does not include all the selected variations. Consider an order for a variable product with color "Blue" and size "Medium". The API endpoint would return something like this (simplified result):

"line_items": [
    {
        "id": 5,
        "name": "Variable Hoodie - Blue",

        "meta_data": [
            {
                "id": 51,
                "key": "pa_store-level-size",
                "value": "medium",
            },
            {
                "id": 52,
                "key": "product-level-color",
                "value": "Blue",
            },
        ],

    }

As shown above, the size was not included in the name. This is how it would look like in the iOS app:

WCAdmin shows the attribute that was omitted:

Why Not Use Metadata?

We could use the meta_data array to find the missing attributes. However, this may be unreliable because:

We have to analyze name to figure out which attribute is already filled in. This can include finding the dash separator, which I think can be customized too.
The value in meta_data is the sanitized value. In the example above, "medium" should be "Medium" (capital M).

Solution

The name not showing all the attributes is probably attributed to the generate_product_title function in WooCommerce. It's probably possible to solve the issue by making sure that all attributes are included. However, this won't solve one of our other design requirements (woocommerce/woocommerce-ios#2280) which is to present the attributes separately:

We would still have to do a string operation to split out the attribute values.

In this PR, I'm proposing adding the displayable attribute name and values in the meta_data instead:

"line_items": [
    {
        "id": 5,
        "name": "Variable Hoodie - Blue",

        "meta_data": [
            {
                "id": 51,
                "key": "pa_store-level-size",
                "value": "medium",
                "display_key": "Store Level Size",
                "display_value": "Medium"
            },
            {
                "id": 52,
                "key": "product-level-color",
                "value": "Blue",
                "display_key": "Product Level Color",
                "display_value": "Blue"
            },
        ],

    }

With these available properties, the API clients (e.g. iOS and Android apps) would be able to easily decide how to present the attributes.

There is still the problem with the name possibly showing one of the attributes. I'm planning to submit another PR to fix that after this one.
Schema

I added the new properties above in the schema.

                        "meta_data": {
                            "description": "Meta data.",
                            "type": "array",
                            "context": [
                                "view",
                                "edit"
                            ],
                            "items": {
                                "type": "object",
                                "properties": {
                                    "id": {
                                        "description": "Meta ID.",
                                        "type": "integer",
                                        "context": [
                                            "view",
                                            "edit"
                                        ],
                                        "readonly": true
                                    },
                                    "key": {
                                        "description": "Meta key.",
                                        "type": "string",
                                        "context": [
                                            "view",
                                            "edit"
                                        ]
                                    },
                                    "value": {
                                        "description": "Meta value.",
                                        "type": "mixed",
                                        "context": [
                                            "view",
                                            "edit"
                                        ]
                                    },
                                    "display_key": {
                                        "description": "Meta key for UI display.",
                                        "type": "string",
                                        "context": [
                                            "view",
                                            "edit"
                                        ]
                                    },
                                    "display_value": {
                                        "description": "Meta value for UI display.",
                                        "type": "string",
                                        "context": [
                                            "view",
                                            "edit"
                                        ]
                                    }
                                }
                            }
                        },

Changes

These new properties are only applied to Orders API V3 since we are hoping to get fixed in the mobile apps soon. I did this by overriding the get_order_item_data in Controllers\Version3\ WC_REST_Orders_Controller.

Please let me know if I should include this in V4 as well.
Testing

This is just a suggestion but please feel free to test other scenarios. 🙂

In wp-admin → Products → Attributes, add a new attribute and add terms to that attribute.
Add a new Variable product in wp-admin → Products → Add New.
Set this product as Variable.
In the product's Attributes section, add the attribute created in Step 1. Set this as "Used for variations". Add values.
Add a Custom product attribute. Set this as "Used for variations". Add values.
Save the attributes.
In the product's Variations section, create variations for all the options. Or create a few variations only.
Set prices for all the variations.
Publish the product.
As a customer, place an order of the created Variable product. Make note of the Order ID.
In the API, run GET /wp-json/wc/v3/orders/{OrderID}.
Confirm that the line_items meta_data has the display_key and display_value properties and they are correct.

Unit Test

I have also added a unit test that I think covers the manual testing scenario above.

shiki added 13 commits August 26, 2020 23:46
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.
@vedanshujain vedanshujain changed the title Fix/api/229 Add User-friendly Attribute Names and Values to Order Line Items Metadata Aug 26, 2020
Copy link
Contributor Author

@vedanshujain vedanshujain left a 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.

@vedanshujain
Copy link
Contributor Author

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.
@shiki
Copy link
Contributor

shiki commented Sep 1, 2020

@vedanshujain I have submitted a PR to fix the failing unit test. → #27559

@astralbodies
Copy link

@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! 😃

@peterfabian peterfabian added this to the 4.7.0 milestone Oct 2, 2020
@shiki
Copy link
Contributor

shiki commented Oct 12, 2020

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?

shiki and others added 9 commits October 13, 2020 22:32
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.
@vedanshujain
Copy link
Contributor Author

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.

@shiki
Copy link
Contributor

shiki commented Oct 13, 2020

@vedanshujain Ah, I see. I believe that's the one I fixed in this PR → #27559.

@vedanshujain vedanshujain added the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Oct 14, 2020
@vedanshujain
Copy link
Contributor Author

LGTM now asked for another review. Marking as a high impact change since it changes the returned data format of a nested property.

peterfabian
peterfabian previously approved these changes Oct 14, 2020
Copy link
Contributor

@peterfabian peterfabian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine, tests are passing now, thanks folks, just a small nitpick, but not a blocker.

I tested v2 and v3 using Postman with some funky attributes and it seems to work fine:
Screenshot 2020-10-14 at 13 18 30

I assume adding new fields should not be a big problem for bw compatibility, so should be good to go I think.

@vedanshujain
Copy link
Contributor Author

Merging as tests passed, the code standard test is failing on all PRs for unrelated reason looks like.

@vedanshujain vedanshujain merged commit ae610d3 into master Oct 14, 2020
@vedanshujain vedanshujain deleted the fix/api/229 branch October 14, 2020 16:32
@woocommercebot woocommercebot added release: add changelog Mark all PRs that have not had their changelog entries added. [auto] release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Oct 14, 2020
@claudiosanches claudiosanches removed the release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] label Oct 19, 2020
@claudiosanches claudiosanches removed their request for review March 15, 2021 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release: add changelog Mark all PRs that have not had their changelog entries added. [auto] release: highlight Issues that have a high user impact and need to be discussed/paid attention to.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

woocommerce.css overrides WooThemes CSS

7 participants