Improved Messaging and Logging when fetching orders fails

[malinajirka]

Users brought to our attention that the app keeps displaying "Error fetching orders" without any helpful info in some cases.

Improved UX with better messaging I suggest that if the app encounters a parsing error or similar, we display something like "Unexpected format - possibly a plugin conflict. Please contact support.".

If the app isn't able to communicate with the server, I suggest we slightly improve the message to something like "Error Fetching Orders. Please check your internet connection and try again.".

Better Logging Improved internal logging might help HEs and devs understand the issue better. Users will profit from a more specific instructions on what to do from the HEs. We could even consider recording events into Sentry - not issues with client-server communication as those could swamp Sentry, but parsing errors are worth considering.

Note: All these are just suggestions from the top of my head. The ultimate goal is to improve the experience when users are seeing "Error fetching orders" error. Perhaps, there is a bug in the app and this error is shown even when it shouldn't or we use this generic message even for cases for which we can provide a more specific action users can take.

[peril-woocommerce[bot]]

	Fails
:no_entry_sign:	Please add a feature label to this issue. e.g. 'feature: stats'

Generated by :no_entry_sign: dangerJS

[malinajirka]

This might be potentially very relevant - it seems it was implemented on iOS but never on Android. ( 🎩 hat tip @rachelmcr).

[jd-alexander]

Upon my investigation, I found this functionality that seems similar to what you are requesting https://github.com/woocommerce/woocommerce-android/pull/9301

I am analyzing it to see if it covers all the necessary use cases.

[jd-alexander]

From a logging standpoint, we can follow iOS' implementation of sending the error data when the order list fails https://github.com/woocommerce/woocommerce-ios/issues/9084

[jd-alexander]

Investigation

Based on my exploration and architectural review of the OrderStore in FluxC, I've identified a set of errors that occur specifically with individual order operations such as update, add, delete, etc. For the order list, our focus is on operations that retrieve a group of orders. In FluxC, I've noticed that there are error types not bound to the list error types. Before implementing a solution, I want to ensure alignment with the expected goals of this task. A high-level solution would be to log the underlying order error that isn't yet mapped, leading to the identification of specific recurring errors.

For further context, the current list error types I've encountered are:

enum class ListErrorType {
GENERIC_ERROR,
PERMISSION_ERROR,
PARSE_ERROR
}

In contrast, the Order errors are:

enum class OrderErrorType {
INVALID_PARAM,
INVALID_ID,
ORDER_STATUS_NOT_FOUND,
PLUGIN_NOT_ACTIVE,
INVALID_RESPONSE,
GENERIC_ERROR,
PARSE_ERROR,
EMPTY_BILLING_EMAIL;
}

In the code below FluxC isn't propagating the underlying errors. Therefore, this is where the improvement lies to improve the UI layer.

Here are examples of how errors are currently handled:

private fun handleFetchOrderListCompleted(payload: FetchOrderListResponsePayload) {
// ... existing code ...
mDispatcher.dispatch(ListActionBuilder.newFetchedListItemsAction(FetchedListItemsPayload(
// ... existing code ...
error = payload.error?.let { fetchError ->
// TODO: Use the actual error type
ListError(type = ListErrorType.GENERIC_ERROR, message = fetchError.message)
}
)))
}

@Suppress("SpreadOperator")
private fun handleFetchOrderByIdsCompleted(payload: FetchOrdersByIdsResponsePayload) {
    coroutineEngine.launch(API, this, "handleFetchOrderByIdsCompleted") {
        // ... [Other code in the function]

        if (payload.isError) {
            val errorType = if (payload.error.type == PARSE_ERROR) {
                ListErrorType.PARSE_ERROR
            } else {
                ListErrorType.GENERIC_ERROR
            }

            mDispatcher.dispatch(
                ListActionBuilder.newListDataFailureAction(
                    OnListDataFailure(listTypeIdentifier).apply {
                        error = ListError(
                            errorType,
                            payload.error.message
                        )
                    }
                )
            )
        }

        // ... [Rest of the function]
    }
}

If we look at the UI layer code in the OrderListViewModel below we will realize that the PARSE_ERROR seems to cover for the PLUGIN_NOT_ACTIVE error based on current app messaging, indicating a more optimal solution that ensures the ListErrorType and OrderErrorType are in sync.

So if a parse error takes place the app shows:

<string name="orderlist_parsing_error_message">This could be related to a conflict with a plugin. Please try again later or reach out to us and we\'ll be happy to assist you!</string>

But I believe that if a plugin error takes place it may show the generic error based on the FluxC code.

pagedListWrapper.listError
.filter { !dismissListErrors }
.filterNotNull()
.observe(this) { error ->
if (error.type == ListStore.ListErrorType.PARSE_ERROR) {
viewState = viewState.copy(
isErrorFetchingDataBannerVisible = true,
isSimplePaymentsAndOrderCreationFeedbackVisible = false
)
} else {
triggerEvent(ShowErrorSnack(R.string.orderlist_error_fetch_generic))
}
}

Next Steps

I want to make changes to FluxC to send back all the relevant error types that are possible. Therefore, if anyone has any feedback about all the list error types that are possible that would be helpful. We could always try to see how we could log this error from FluxC in the app so this would involve mapping all order errors to list errors and then removing them once they aren't occurring frequently. Not the most optimal approach but it is a start. I would love to hear your thoughts.

[malinajirka]

Great investigation @jd-alexander!

In the code below FluxC isn't propagating the underlying errors.

Nice catch 👏

indicating a more optimal solution that ensures the ListErrorType and OrderErrorType are in sync.

⚠️ ListStore and ListErrors are generic types - can be used for any list, not just Order List. So we probably don't want to align them 1:1. Moreover, keep in mind that the ListStore component is used in the WordPress and Jetpack apps (for post list), so any changes we make need to be compatible with these apps or we need to update these apps as well.

But I believe that if a plugin error takes place it may show the generic error based on the FluxC code.

I'm not sure what you mean by 'plugin error' in this context. Do you mean a parse error which is caused by a conflicting plugin - aka a 3rd party plugin modifies the API response and the app fails to parse the updated response?

I want to make changes to FluxC to send back all the relevant error types that are possible. Therefore, if anyone has any feedback about all the list error types that are possible that would be helpful.

One thing I believe we might want to consider is updating this piece of code to return parse_error and generic_error + log the original type of the error. This could give use 95% of the desired behavior with a little effort. This code is specific to Order List - so it won't affect the WordPress/Jetpack apps. Moreover, if we won't do anything on the UI with the other error types, propagating them won't give us any real benefits -> propagating them might get quite tricky to implement, since as I mentioned ListStore and ListErrorType are generic types which need to remain type-independent.

Making this modification and intercepting/mocking responses from the API with something unparsable could allow us easily test if the change results in the correct UI being shown + correct error being logged.

Wdyt?

P.S. The list store component is quite complex, but it essentially first fetches a list of order ids (ids of all orders in the database) => then when the app shows a recycler view with lets say first 20 items => the list store component fetches details of these 20 orders by sending a batch of ids to the API asking for the specific set of orders. So there are two different API calls which can result in a parse error => 'fetch all order ids' and 'fetch order details for the provided set of ids'.

[jd-alexander]

Just for some additional context there is analytics being used to track order list failures https://github.com/woocommerce/woocommerce-android/pull/9172

[malinajirka]

Merged in https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/2951 and the Better Error Messages project implemented by Team Eagle - pe5pgL-4zy-p2.