[Woo POS][Catalog i2] Handle unsupported product types

[iamgabrielma]

Closes WOOMOB-2089

Description

When decoding items from the catalog endpoint, the resulting file may contain unsupported product types like subscription_variation, and could be not limited to Subscriptions but other custom product types like Bookings and so on. Due to technical constraints in the backend, we will filter these in the client for the time being.

Changes

• Add unsupported case to decodable POSCatalogItem
• Refactor the default case for CodingKeys, where we explicitly look for simple (products) and variable (variable parent product) keys and map them to product(POSProduct), leaving the default to skip any other type.

Test Steps

• Add new product and variable product with variations to your site (duplicate option in wp-admin comes handy!)
• In the app go to POS and observe that the new product and variable product appears and behaves correctly.

[wpmobilebot]

📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS Prototype
	Build Number	pr16575-e5028e2
	Version	23.9
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	e5028e2
	Installation URL	043qahjcb4uo0

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

[joshheald]

WDYT about parsing and persisting these unsupported products, then filtering them using the pre-defined queries, as we do with downloadable products?

The advantages include:

• We can show "Unsupported product" when the user scans a barcode for these products.
• Changes via incremental updates are handled in the same place (no duplication of the filtering.)
• Counts of products/variations match all the way through the process.
• We don't get hanging references to unsupported variations on their supported parent products.
• All the client-side filtering is done together
• If we have a usecase for unfiltered queries, we can still do them

This was what I envisaged when you and Jirka raised it in Slack, anyway...

From a quick look at the code, I think it's what already happens for unsupported types, including this one, so there may not need to be any code changes. I think it's how we already handle bookings, grouped products, etc – they're in the db but never shown.

1. The text for product_type is parsed to productTypeKey, then we compute a ProductType
2. The productType could be known (including variable_subscription) or unknown, (like for subscription_variation) and parsed as custom(name:)
3. When we get products from the database, the baseQuery filters out everything except simple and variable

Do you think this is enough? Have you seen any problems from including the subscription_variation items in the catalog? I think it's a bit odd that they go through the product parsing route... but OTOH, that is a good default generally.

If you agree, I think we can close this one, if not I can look closer.

[iamgabrielma]

Have you seen any problems from including the subscription_variation items in the catalog?

I'm not sure at the moment, I'll have to look further into this. For example in the parent PR the response from the catalog will fail to decode if we have subscription_variations in the data, as these do not seem to have a name property (there is one, but inside the image object, not in the top level) and when decoding them to POSProduct we require a name.

[joshheald]

Perhaps a more future-proof fix for that would be to direct those down the variation decoding path, and ensure they get filtered in the same way once they're there?

[joshheald]

Or make the decoding more permissive, ignoring things we can't decode rather than failing. Probably best to do both those things, tbh.

[iamgabrielma]

I'm not quite sure to follow, so I'm sure I'm missing something here. This is what I'm currently reading:

1. On one end, we could be more permissive when decoding and "try" to parse the unsupported products, in this case for the subscription variation and passing it specifically through the POSProductVariation:

        case "variation", "subscription_variation":
            do {
                self = .variation(try container.decode(POSProductVariation.self, forKey: .data))
            } catch {
                self = .unsupported
            }

We still ignore it, there is no decoding, and if this is done through scanner we would see the "no products found" message. But we end with the same result that the current approach of ignoring it directly without trying, isn't it?

It is more resilient though, if one expected product or variation is malformed for some reason then the whole catalog does not fail, we just skip that one. Is that what you meant?

2. Another option is to actually decode them and persist them to storage, but I believe this implies an schema update and migration (as PersistedProductVariation does not have a type field) to store it, however, not the variable subscription or the parent subscription are supported.

I think I'm missing something as I'm not seeing the benefit of the suggestions right away.

[joshheald]

Another option is to actually decode them and persist them to storage, but I believe this implies an schema update and migration (as PersistedProductVariation does not have a type field) to store it, however, not the variable subscription or the parent subscription are supported.

Exactly this. We would need to add a type field to do it. If this isn't part of the JSON (I'm unsure, but Jirka said that these came through on the incremental update response as well...) we'd have to pass in the "variation" or "subscription_variation" that we're using for this case.

Limited value for two specific strings in the case, but perhaps easier to see the value if we made it:

if (type.contains("variation") { 
    /*handle as variation, pass in type, save all of them even if they have custom(name:) for the type*/ 
} else { 
    /*handle as product*/ 
}

Doing that lets us see product not supported, both for variations and other types of unsupported product. That could even help us know what demand there is for different types to work in POS.

Making it flexible to continue to parse the list even if one entry can't be decoded is kind of separate – it makes it more resillient. Yes, we may end up with a missing product that way, and little way of figuring out why, but better than failing the whole download.

LMK if it would help to pair on this. I think it would involve a migration, so we'll need to co-ordinate that with FTS.

[joshheald]

See inline comment – just want to confirm whether we need a change for this or not.

[wpmobilebot]

Version 24.1 has now entered code-freeze, so the milestone of this PR has been updated to 24.2.