Skip to content
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

Woo: Part one for orders/batch endpoint #3119

Merged
merged 8 commits into from
Dec 13, 2024
Merged

Conversation

hafizrahman
Copy link
Contributor

@hafizrahman hafizrahman commented Dec 12, 2024

Part of woocommerce/woocommerce-android#13115

Working on adding the endpoint for /wc/v3/orders/batch but in parts. Documentation in pe5sF9-3kb-p2

In this first part I'm focusing on the API response class and making sure it's correct first.

Actual API call and store functions, etc, will be added in a later PR.

Discussion:

According to the documentation https://woocommerce.github.io/woocommerce-rest-api-docs/?shell#batch-update-orders

The endpoint will return an object of three fields: "create", "update", and "delete". This is because the batch order action can handle all three actions in one go.

However, for our current purpose, we only want to deal with "update" batch action. This is why BatchOrderApiResponse now only has an "update" field.

Furthermore, the "update" field returns an array that contains either (and can be a mix of):

  • success case, where it will return a complete Order object
  • failure case, where it will return an object with "id" and "error" fields.

Please check orders-batch.json for an example of this.

Because of these two differences, I created OrderResponse to support either Success or Error.

I think this architecture makes sense. Later on the handling for the response can check whether it's of type OrderResponse.Success or OrderResponse.Error, and act accordingly (e.g: for updating successful case in the DB, or displaying success/error state). But, let me know if there's a way to improve this.

To test:

  1. Ensure testDeserializeBatchOrderResponse() passes.

Copy link
Member

@hichamboushaba hichamboushaba left a comment

Choose a reason for hiding this comment

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

Nice work @hafizrahman 👏, I left some comments, but nothing is blocking, so I'm pre-approving.

Comment on lines 152 to 154
.registerTypeAdapter(
BatchOrderApiResponse.OrderResponse::class.java,
BatchOrderApiResponse.OrderResponseDeserializer())
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: Instead of having to register the type adapter manually, we can use the @TypeAdapter annotation directly, this is something that recent (relatively 😄) versions of Gson added, this patch would show how it can be used:

Index: plugins/woocommerce/src/main/kotlin/org/wordpress/android/fluxc/network/rest/wpcom/wc/order/BatchOrderApiResponse.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/plugins/woocommerce/src/main/kotlin/org/wordpress/android/fluxc/network/rest/wpcom/wc/order/BatchOrderApiResponse.kt b/plugins/woocommerce/src/main/kotlin/org/wordpress/android/fluxc/network/rest/wpcom/wc/order/BatchOrderApiResponse.kt
--- a/plugins/woocommerce/src/main/kotlin/org/wordpress/android/fluxc/network/rest/wpcom/wc/order/BatchOrderApiResponse.kt	(revision 3c423f4b9646625298b72fbc52b981b47a7484c6)
+++ b/plugins/woocommerce/src/main/kotlin/org/wordpress/android/fluxc/network/rest/wpcom/wc/order/BatchOrderApiResponse.kt	(date 1734005033319)
@@ -3,12 +3,14 @@
 import com.google.gson.JsonDeserializationContext
 import com.google.gson.JsonDeserializer
 import com.google.gson.JsonElement
+import com.google.gson.annotations.JsonAdapter
 import java.lang.reflect.Type
 import org.wordpress.android.fluxc.network.Response
 
 data class BatchOrderApiResponse(
     val update: List<OrderResponse>
 ) : Response {
+    @JsonAdapter(OrderResponseDeserializer::class)
     sealed class OrderResponse {
         data class Success(
             val order: OrderDto
@@ -30,7 +32,7 @@
         val status: Int
     )
 
-    class OrderResponseDeserializer : JsonDeserializer<OrderResponse> {
+    private class OrderResponseDeserializer : JsonDeserializer<OrderResponse> {
         override fun deserialize(
             json: JsonElement,
             typeOfT: Type,
Index: example/src/test/java/org/wordpress/android/fluxc/wc/order/OrderEntityTest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/example/src/test/java/org/wordpress/android/fluxc/wc/order/OrderEntityTest.kt b/example/src/test/java/org/wordpress/android/fluxc/wc/order/OrderEntityTest.kt
--- a/example/src/test/java/org/wordpress/android/fluxc/wc/order/OrderEntityTest.kt	(revision 3c423f4b9646625298b72fbc52b981b47a7484c6)
+++ b/example/src/test/java/org/wordpress/android/fluxc/wc/order/OrderEntityTest.kt	(date 1734004691032)
@@ -149,9 +149,6 @@
     @Test
     fun testDeserializeBatchOrderResponse() {
         val testGson = GsonBuilder()
-            .registerTypeAdapter(
-                BatchOrderApiResponse.OrderResponse::class.java,
-                BatchOrderApiResponse.OrderResponseDeserializer())
             .create()
 
         val batchOrderJson = UnitTestUtils.getStringFromResourceFile(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for mentioning that! Applied in 254cf5c

import java.lang.reflect.Type
import org.wordpress.android.fluxc.network.Response

data class BatchOrderApiResponse(
Copy link
Member

Choose a reason for hiding this comment

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

minor, I would make this internal, I know we haven't been doing it often in the past, but I think it's still something we should consider for new features, as it would reduce leaking the networking models to the upper layers.

Copy link
Contributor Author

@hafizrahman hafizrahman Dec 13, 2024

Choose a reason for hiding this comment

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

This makes sense. However when I do so after the last commit, 6e2cdef, the unit test is unable to see this class, even though the package location is now the same. Still see the same issue after adding @VisibleForTesting, too.

Not sure yet why this happens, do you know? I'll merge this PR for now to continue work and add this change later.

Copy link
Member

Choose a reason for hiding this comment

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

This is because the test is defined in another module (example), I think it would be better to move the test to the same module plugins/woocommerce

@@ -143,4 +145,31 @@ class OrderEntityTest {
assertEquals("Flat Rate Shipping", shippingLinesList[0].methodTitle)
assertEquals("Local Pickup Shipping", shippingLinesList[1].methodTitle)
}

@Test
fun testDeserializeBatchOrderResponse() {
Copy link
Member

Choose a reason for hiding this comment

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

Minor, I think this test would better be in a separate file, OrderEntityTest is originally about OrderEntity logic, which doesn't apply here, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, moved in 6e2cdef 👍🏼

@hafizrahman hafizrahman merged commit e32b77f into trunk Dec 13, 2024
14 checks passed
@hafizrahman hafizrahman deleted the woo/orders-batch-endpoint branch December 13, 2024 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants