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.

Sign up for GitHub

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

Pass full error for use by Gutenberg #1459

Merged
merged 1 commit into from
Jan 28, 2020
Merged

Pass full error for use by Gutenberg #1459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions ...Test/java/org/wordpress/android/fluxc/release/ReleaseStack_ReactNativeWPAPIRequestTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.wordpress.android.fluxc.release

import kotlinx.coroutines.runBlocking
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Test
import org.wordpress.android.fluxc.example.BuildConfig
Expand Down Expand Up @@ -28,7 +29,17 @@ class ReleaseStack_ReactNativeWPAPIRequestTest : ReleaseStack_Base() {
val params = mapOf("context" to "view")
val response = runBlocking { reactNativeStore.performWPAPIRequest(url, params) }

val failureMessage = "Call failed with error: ${(response as? Error)?.error}"
assertTrue(failureMessage, response is Success)
val assertionMessage = "Call failed with error: ${(response as? Error)?.error}"
assertTrue(assertionMessage, response is Success)
}

@Test
fun testWPAPICallOnSelfHosted_fails() {
val url = BuildConfig.TEST_WPORG_URL_SH_WPAPI_SIMPLE + "/wp-json/wp/v2/an-invalid-extension/"
val response = runBlocking { reactNativeStore.performWPAPIRequest(url, emptyMap()) }

val assertionMessage = "Call should have failed with a 404, instead response was $response"
val actualStatusCode = (response as? Error)?.error?.volleyError?.networkResponse?.statusCode
assertEquals(assertionMessage, actualStatusCode, 404)
}
}
11 changes: 11 additions & 0 deletions ...Test/java/org/wordpress/android/fluxc/release/ReleaseStack_ReactNativeWPComRequestTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.wordpress.android.fluxc.release

import kotlinx.coroutines.runBlocking
import org.junit.Assert
import org.junit.Assert.assertTrue
import org.junit.Test
import org.wordpress.android.fluxc.store.ReactNativeFetchResponse.Success
Expand All @@ -26,4 +27,14 @@ class ReleaseStack_ReactNativeWPComRequestTest : ReleaseStack_WPComBase() {
val failureMessage = "Call failed with error: ${(response as? Error)?.error}"
assertTrue(failureMessage, response is Success)
}

@Test
fun testWpComCall_fails() {
val url = "https://public-api.wordpress.com/wp/v2/sites/${siteFromDb.siteId}/an-invalid-extension"
val response = runBlocking { reactNativeStore.performWPComRequest(url, emptyMap()) }

val assertionMessage = "Call should have failed with a 404, instead response was $response"
val actualStatusCode = (response as? Error)?.error?.volleyError?.networkResponse?.statusCode
Assert.assertEquals(assertionMessage, actualStatusCode, 404)
}
}
2 changes: 1 addition & 1 deletion example/src/main/java/org/wordpress/android/fluxc/example/ReactNativeFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class ReactNativeFragment : Fragment() {
prependToLog("$callType call succeeded")
AppLog.i(AppLog.T.API, "$callType call result: ${response.result}")
}
is Error -> prependToLog("$callType call failed: ${response.error}")
is Error -> prependToLog("$callType call failed: ${response.error.volleyError?.message}")
}
}
}
Expand Down
19 changes: 1 addition & 18 deletions fluxc/src/main/java/org/wordpress/android/fluxc/store/ReactNativeStore.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package org.wordpress.android.fluxc.store
import com.google.gson.JsonElement
import kotlinx.coroutines.withContext
import org.wordpress.android.fluxc.network.BaseRequest.BaseNetworkError
import org.wordpress.android.fluxc.network.rest.wpcom.WPComGsonRequest.WPComGsonNetworkError
import org.wordpress.android.fluxc.network.rest.wpapi.reactnative.ReactNativeWPAPIRestClient
import org.wordpress.android.fluxc.network.rest.wpcom.reactnative.ReactNativeWPComRestClient
import org.wordpress.android.fluxc.store.ReactNativeFetchResponse.Success
Expand Down Expand Up @@ -37,21 +36,5 @@ class ReactNativeStore

sealed class ReactNativeFetchResponse {
class Success(val result: JsonElement) : ReactNativeFetchResponse()
class Error(networkError: BaseNetworkError) : ReactNativeFetchResponse() {
val error: String

init {
val volleyError = networkError.volleyError?.message
val wpComError = (networkError as? WPComGsonNetworkError)?.apiError
val baseError = networkError.message
val errorType = networkError.type?.toString()
error = when {
volleyError?.isNotBlank() == true -> volleyError
wpComError?.isNotBlank() == true -> wpComError
baseError?.isNotBlank() == true -> baseError
errorType?.isNotBlank() == true -> errorType
else -> "Unknown ${networkError.javaClass.simpleName} Error"
}
}
}
class Error(val error: BaseNetworkError) : ReactNativeFetchResponse()
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, after looking into the WPAndroid part, since we're only using the error status code and the message, could we keep the logic above here and pass these two fields here? I'd rather not use this object in WPAndroid (if possible).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's certainly possible. Admittedly I was trying to do a bit of future-proofing with this change.

I wanted to avoid having to do FluxC changes every time we need to send more or different error information to React Native. Since this api will be used for a number of different calls by gutenberg (only one of which we've actually looked at so far), I think it's pretty safe to say that we'll probably end up needing to make some changes to the error information than we are now. The required error information is also heavily dependent on the web side of gutenberg, so it's pretty safe to say that in the future there will be changes on the web side that will require us to make changes to FluxC if it is only passing the currently-relevant parts of the error.

The fact that each time we want to change the error information we're sending back requires a change to FluxC felt a bit like a leak of the implementation details of gutenberg into FluxC. Instead, since for successful calls we're passing the raw response back to RN in the form of json (instead of trying to parse it and only passing back the specific things that I need), my thinking was that it made sense to take a similar approach with the error by just passing all the error information back from FluxC. Admittedly, to be perfectly consistent I would have converted the error to json like we do for the response and pass that back, but I didn't want to do that for obvious reasons.

Just to give a bit of flavor for where I'm coming from: the change to add the http error code to the error response we're sending back to React Native required changing around 10 lines in a single file on WiOS. Doing the same thing on Android required changing 14 files, coordinated across three different repositories. Admittedly, most of the Java changes are just boilerplate, but having to coordinate them between three repositories is still a fair bit of overhead for a relatively small change.

My hope with these PRs was to drastically reduce the number of changes needed whenever we wanted to change the error information sent back to react native. In particular, I believe the way that I have updated the handling in FluxC and WPAndroid in these PRs, it would only require a change to a single file in WPAndroid (ReactNativeRequestHandler) the next time we need to change the error response that is passed back to React Native, instead of the 14 files that needed to be changed this time.

With all that said (sorry this has run so long), you certainly have a much better feel for FluxC than I do, so if you still think we're better off changing this to only pass back the error message and http code from FluxC, just let me know and I'll gladly make the change. 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👋 @planarvoid ! Let me know if you'd like me to go ahead and make the change or if you're ok with my approach!

}