Fix updates on filtered order list #2973
Conversation
| enum class ListUpdateStrategy { | ||
| DEFAULT, | ||
| // Re-fetch the order list | ||
| REFRESH, |
There was a problem hiding this comment.
I followed the same naming that is used in the ListActionBuilder.
| }.copy(causeOfChange = WCOrderAction.UPDATE_ORDER_STATUS) | ||
| // Ensure the code gets executed even when the VM dies - eg. when the client app marks an order as | ||
| // completed and navigates to a different screen. | ||
| val remoteUpdateResult: OnOrderChanged = withContext(NonCancellable) { |
There was a problem hiding this comment.
This is now wrapped in NonCancellable to ensure that when we send the request we always wait and process the result.
There was a problem hiding this comment.
This article mentions some risks associated with using NonCancellable context, but I'm personally still leaning towards using it here over wrapping it all in an application context.
Considering this part of the code doesn't have access to the appContext, we'd need launch the coroutine in higher layers. However, this code is also inside a Flow, so we'd need to launch it in the layer which creates the flow => we'd be making more code uncancellable and risking other side-effects.
| ordersDaoDecorator.insertOrUpdateOrder( | ||
| remotePayload.order, | ||
| // Re-fetch the list to ensure the order is correctly placed even when a filter is applied | ||
| OrdersDaoDecorator.ListUpdateStrategy.REFRESH |
There was a problem hiding this comment.
We pass ListUpdateStrategy.REFRESH here to ensure the list of ids gets refetched from the remote to ensure the item disappears/re-appears in a list when filters on the Order List screen are applied.
| orderId, | ||
| site.localId(), | ||
| newStatus.statusKey, | ||
| OrdersDaoDecorator.ListUpdateStrategy.INVALIDATE |
There was a problem hiding this comment.
We pass Invalidate to ensure the updated status is immediately visible on the order list, we don't want to refetch here since the status isn't updated in the remote just yet (that's handled on line 626.
| orderBeforeUpdate == null -> UpdateOrderResult.INSERTED | ||
| // Ignore changes to the modifiedDate - it's updated even when there are no other changes | ||
| // and results in an infinite loop. | ||
| order.copy(dateModified = "") != orderBeforeUpdate.copy(dateModified = "") -> UpdateOrderResult.UPDATED |
There was a problem hiding this comment.
This code isn't great, but there is a bug in the API. When I invoke
https://public-api.wordpress.com/rest/v1.1/jetpack-blogs/154993204/rest-api/?path=%2Fwc%2Fv3%2Forders%2F%26_method%3Dget&json=true&query=%7B%22per_page%22%3A%221%22%2C%22include%22%3A%223180%22%2C%22_fields%22%3A%22billing%2Ccoupon_lines%2Ccurrency%2Corder_key%2Ccustomer_note%2Cdate_created_gmt%2Cdate_modified_gmt%2Cdate_paid_gmt%2Cdiscount_total%2Cfee_lines%2Ctax_lines%2Cid%2Cline_items%2Cnumber%2Cpayment_method%2Cpayment_method_title%2Cprices_include_tax%2Crefunds%2Cshipping%2Cshipping_lines%2Cshipping_total%2Cstatus%2Ctotal%2Ctotal_tax%2Cmeta_data%2Cpayment_url%2Cis_editable%2Cneeds_payment%2Cneeds_processing%2Cshipping_tax%22%7D&locale=en_US
the order data shouldn't change, it's GET request after all. However, date_paid, date_completed are sometime returned as null and sometime contain a value => the backend changes also date_modified which is what affects the app, since it relies on it to determine if the order needs to be re-fetched.
All this results in an infinite loop -> the app fetches an order -> it refetches the list -> the date modified is different so it re-fetches the order -> the order has a different date_modified again -> it refetches the list -> and this goes on forever.
There was a problem hiding this comment.
Internal report of the issue p7bje6-5LJ-p2
|
The review of this PR has been intentionally postponed last week as we prioritized testing other fixes and improvements. Having said that, it is on our radar. |
This PR fixes two bugs in the WooCommerce Android app.
Issue 1
When a filter is applied on the order list (eg. status: Pending Payment) and the user changes status of one the orders (eg. to Completed), the order remains in the list even though its status is not Pending Payment anymore.
Solution for Issue 1
This is fixed in the first three commits. The point of the fix is to re-fetch the list from the remote after the order status is updated in the remote. When the app optimistically updates the order in the local database, we just invalidate the list (reload it from database) instead of refetching it.
Issue 2
This issue was there before this PR but wasn't as apparent as it is now. When the user collects a payment, the app marks the order as completed locally (optimistic update) and send a request to the server to update it in the remote. However, when the app navigates away from the Payment Selection screen, all the coroutines are cancelled and the result of the request is never processed => the outcome of this is the app isn't notified about it, it doesn't update the database and doesn't refresh the order list. The order remains in the list as in issue 1.
Solution for Issue 2
Fixed in d63a924. I wrapped the request to the server and handling of the response into a NonCancellable coroutine context to ensure the result is handled.
Issue 3
When the user collects money using IPP, the order is re-fetched. However, the current implementation only invalidates the list (reloads it from database) instead of re-fetching it from the remote. This is essentially still Issue 1 in a different scenario.
Solution for Issue 3
Fixed in a665939. I updated the logic that takes care of inserting/updating the order - it checks whether the order has changed after it was refetched. If it has changed, the app refreshes the list instead of just invalidating it. It could refresh it no matter if the order has changed, but that would result in a lot of refreshes - moreover, the UI displays a loading indicator when the list is being refreshed so we don't want to refresh it more often than we need to.
To test
Test these changes in the WooCommerce Android app. More details on the corresponding PR.