From 96da1b763914d02e283478646e0147c515044ed5 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 9 Mar 2020 15:13:18 -0300 Subject: Show Meaningful Configuration Parsing Errors Closes #0006113 --- app/build.gradle | 9 ++ .../java/net/taler/merchantpos/MainViewModel.kt | 2 +- .../merchantpos/config/ConfigFetcherFragment.kt | 15 +-- .../net/taler/merchantpos/config/ConfigManager.kt | 54 +++++---- .../merchantpos/config/MerchantConfigFragment.kt | 27 +++-- .../java/net/taler/merchantpos/order/LiveOrder.kt | 2 +- .../net/taler/merchantpos/order/OrderManager.kt | 32 ++--- .../taler/merchantpos/order/OrderStateFragment.kt | 2 +- app/src/main/res/values/strings.xml | 11 +- .../taler/merchantpos/order/OrderManagerTest.kt | 135 +++++++++++++++++++++ 10 files changed, 228 insertions(+), 61 deletions(-) create mode 100644 app/src/test/java/net/taler/merchantpos/order/OrderManagerTest.kt diff --git a/app/build.gradle b/app/build.gradle index fd6c18c..594cab3 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -30,6 +30,12 @@ android { jvmTarget = "1.8" } + testOptions { + unitTests { + includeAndroidResources = true + } + } + lintOptions { abortOnError true ignoreWarnings false @@ -64,4 +70,7 @@ dependencies { // JSON parsing and serialization implementation "com.fasterxml.jackson.module:jackson-module-kotlin:2.10.2" + + testImplementation 'androidx.test.ext:junit:1.1.1' + testImplementation 'org.robolectric:robolectric:4.3.1' } diff --git a/app/src/main/java/net/taler/merchantpos/MainViewModel.kt b/app/src/main/java/net/taler/merchantpos/MainViewModel.kt index 3aa9f9f..c68688c 100644 --- a/app/src/main/java/net/taler/merchantpos/MainViewModel.kt +++ b/app/src/main/java/net/taler/merchantpos/MainViewModel.kt @@ -18,7 +18,7 @@ class MainViewModel(app: Application) : AndroidViewModel(app) { .configure(FAIL_ON_UNKNOWN_PROPERTIES, false) private val queue = Volley.newRequestQueue(app) - val orderManager = OrderManager(mapper) + val orderManager = OrderManager(app, mapper) val configManager = ConfigManager(app, viewModelScope, mapper, queue).apply { addConfigurationReceiver(orderManager) } diff --git a/app/src/main/java/net/taler/merchantpos/config/ConfigFetcherFragment.kt b/app/src/main/java/net/taler/merchantpos/config/ConfigFetcherFragment.kt index ccadb8b..b4a566a 100644 --- a/app/src/main/java/net/taler/merchantpos/config/ConfigFetcherFragment.kt +++ b/app/src/main/java/net/taler/merchantpos/config/ConfigFetcherFragment.kt @@ -32,17 +32,18 @@ class ConfigFetcherFragment : Fragment() { super.onActivityCreated(savedInstanceState) configManager.fetchConfig(configManager.config, false) configManager.configUpdateResult.observe(viewLifecycleOwner, Observer { result -> - when { - result == null -> return@Observer - result.error -> onNetworkError(result.authError) - else -> actionConfigFetcherToOrder().navigate(findNavController()) + when (result) { + null -> return@Observer + is ConfigUpdateResult.Error -> onNetworkError(result.msg) + is ConfigUpdateResult.Success -> { + actionConfigFetcherToOrder().navigate(findNavController()) + } } }) } - private fun onNetworkError(authError: Boolean) { - val res = if (authError) R.string.config_auth_error else R.string.config_error - Snackbar.make(view!!, res, LENGTH_SHORT).show() + private fun onNetworkError(msg: String) { + Snackbar.make(view!!, msg, LENGTH_SHORT).show() actionConfigFetcherToMerchantSettings().navigate(findNavController()) } diff --git a/app/src/main/java/net/taler/merchantpos/config/ConfigManager.kt b/app/src/main/java/net/taler/merchantpos/config/ConfigManager.kt index f8d5629..fd221f2 100644 --- a/app/src/main/java/net/taler/merchantpos/config/ConfigManager.kt +++ b/app/src/main/java/net/taler/merchantpos/config/ConfigManager.kt @@ -19,6 +19,7 @@ import com.fasterxml.jackson.module.kotlin.readValue import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch +import net.taler.merchantpos.R import org.json.JSONObject private const val SETTINGS_NAME = "taler-merchant-terminal" @@ -35,13 +36,13 @@ private val TAG = ConfigManager::class.java.simpleName interface ConfigurationReceiver { /** - * Returns true if the configuration was valid, false otherwise. + * Returns null if the configuration was valid, or a error string for user display otherwise. */ - suspend fun onConfigurationReceived(json: JSONObject, currency: String): Boolean + suspend fun onConfigurationReceived(json: JSONObject, currency: String): String? } class ConfigManager( - context: Context, + private val context: Context, private val scope: CoroutineScope, private val mapper: ObjectMapper, private val queue: RequestQueue @@ -86,12 +87,14 @@ class ConfigManager( queue.add(stringRequest) } + @UiThread private fun onConfigReceived(json: JSONObject, config: Config?) { val merchantConfig: MerchantConfig = try { mapper.readValue(json.getString("config")) } catch (e: Exception) { Log.e(TAG, "Error parsing merchant config", e) - mConfigUpdateResult.value = ConfigUpdateResult(null) + val msg = context.getString(R.string.config_error_malformed) + mConfigUpdateResult.value = ConfigUpdateResult.Error(msg) return } @@ -108,29 +111,27 @@ class ConfigManager( configJson: JSONObject, merchantConfig: MerchantConfig, json: JSONObject - ) = scope.launch(Dispatchers.Main) { + ) = scope.launch(Dispatchers.Default) { val currency = json.getString("currency") - var configValid = true - configurationReceivers.forEach { + for (receiver in configurationReceivers) { val result = try { - it.onConfigurationReceived(configJson, currency) + receiver.onConfigurationReceived(configJson, currency) } catch (e: Exception) { - Log.e(TAG, "Error handling configuration by ${it::class.java.simpleName}", e) - false + Log.e(TAG, "Error handling configuration by ${receiver::class.java.simpleName}", e) + context.getString(R.string.config_error_unknown) } - configValid = result && configValid - } - if (configValid) { - newConfig?.let { - config = it - saveConfig(it) + if (result != null) { // error + mConfigUpdateResult.postValue(ConfigUpdateResult.Error(result)) + return@launch } - this@ConfigManager.merchantConfig = merchantConfig.copy(currency = currency) - mConfigUpdateResult.value = ConfigUpdateResult(currency) - } else { - mConfigUpdateResult.value = ConfigUpdateResult(null) } + newConfig?.let { + config = it + saveConfig(it) + } + this@ConfigManager.merchantConfig = merchantConfig.copy(currency = currency) + mConfigUpdateResult.postValue(ConfigUpdateResult.Success(currency)) } fun forgetPassword() { @@ -147,13 +148,18 @@ class ConfigManager( .apply() } + @UiThread private fun onNetworkError(it: VolleyError?) { - val authError = it?.networkResponse?.statusCode == 401 - mConfigUpdateResult.value = ConfigUpdateResult(null, authError) + val msg = context.getString( + if (it?.networkResponse?.statusCode == 401) R.string.config_auth_error + else R.string.config_error_network + ) + mConfigUpdateResult.value = ConfigUpdateResult.Error(msg) } } -class ConfigUpdateResult(val currency: String?, val authError: Boolean = false) { - val error: Boolean = currency == null +sealed class ConfigUpdateResult { + data class Error(val msg: String) : ConfigUpdateResult() + data class Success(val currency: String) : ConfigUpdateResult() } diff --git a/app/src/main/java/net/taler/merchantpos/config/MerchantConfigFragment.kt b/app/src/main/java/net/taler/merchantpos/config/MerchantConfigFragment.kt index 8bbc70d..19b3ab0 100644 --- a/app/src/main/java/net/taler/merchantpos/config/MerchantConfigFragment.kt +++ b/app/src/main/java/net/taler/merchantpos/config/MerchantConfigFragment.kt @@ -54,12 +54,9 @@ class MerchantConfigFragment : Fragment() { ) configManager.fetchConfig(config, true, savePasswordCheckBox.isChecked) configManager.configUpdateResult.observe(viewLifecycleOwner, Observer { result -> - when { - result == null -> return@Observer - result.error -> onNetworkError(result.authError) - else -> onConfigReceived(result.currency!!) + if (onConfigUpdate(result)) { + configManager.configUpdateResult.removeObservers(viewLifecycleOwner) } - configManager.configUpdateResult.removeObservers(viewLifecycleOwner) }) } forgetPasswordButton.setOnClickListener { @@ -99,6 +96,21 @@ class MerchantConfigFragment : Fragment() { forgetPasswordButton.visibility = if (config.hasPassword()) VISIBLE else GONE } + /** + * Processes updated config and returns true, if observer can be removed. + */ + private fun onConfigUpdate(result: ConfigUpdateResult?) = when (result) { + null -> false + is ConfigUpdateResult.Error -> { + onError(result.msg) + true + } + is ConfigUpdateResult.Success -> { + onConfigReceived(result.currency) + true + } + } + private fun onConfigReceived(currency: String) { onResultReceived() updateView() @@ -106,10 +118,9 @@ class MerchantConfigFragment : Fragment() { actionSettingsToOrder().navigate(findNavController()) } - private fun onNetworkError(authError: Boolean) { + private fun onError(msg: String) { onResultReceived() - val res = if (authError) R.string.config_auth_error else R.string.config_error - Snackbar.make(view!!, res, LENGTH_LONG).show() + Snackbar.make(view!!, msg, LENGTH_LONG).show() } private fun onResultReceived() { diff --git a/app/src/main/java/net/taler/merchantpos/order/LiveOrder.kt b/app/src/main/java/net/taler/merchantpos/order/LiveOrder.kt index 206b046..c239f8d 100644 --- a/app/src/main/java/net/taler/merchantpos/order/LiveOrder.kt +++ b/app/src/main/java/net/taler/merchantpos/order/LiveOrder.kt @@ -32,7 +32,7 @@ internal class MutableLiveOrder( get() = productsByCategory.keys.map { it.id to it }.toMap() override val order: MutableLiveData = MutableLiveData(Order(id, availableCategories)) override val orderTotal: LiveData = Transformations.map(order) { it.total } - override val restartState = MutableLiveData(DISABLED) + override val restartState = MutableLiveData(DISABLED) private val selectedOrderLine = MutableLiveData() override val selectedProductKey: String? get() = selectedOrderLine.value?.id diff --git a/app/src/main/java/net/taler/merchantpos/order/OrderManager.kt b/app/src/main/java/net/taler/merchantpos/order/OrderManager.kt index ab561e2..b97219b 100644 --- a/app/src/main/java/net/taler/merchantpos/order/OrderManager.kt +++ b/app/src/main/java/net/taler/merchantpos/order/OrderManager.kt @@ -1,5 +1,6 @@ package net.taler.merchantpos.order +import android.content.Context import android.util.Log import androidx.annotation.UiThread import androidx.lifecycle.LiveData @@ -8,11 +9,15 @@ import androidx.lifecycle.Transformations.map import com.fasterxml.jackson.core.type.TypeReference import com.fasterxml.jackson.databind.ObjectMapper import net.taler.merchantpos.Amount.Companion.fromString +import net.taler.merchantpos.R import net.taler.merchantpos.config.ConfigurationReceiver import net.taler.merchantpos.order.RestartState.ENABLED import org.json.JSONObject -class OrderManager(private val mapper: ObjectMapper) : ConfigurationReceiver { +class OrderManager( + private val context: Context, + private val mapper: ObjectMapper +) : ConfigurationReceiver { companion object { val TAG = OrderManager::class.java.simpleName @@ -32,15 +37,14 @@ class OrderManager(private val mapper: ObjectMapper) : ConfigurationReceiver { private val mCategories = MutableLiveData>() internal val categories: LiveData> = mCategories - @Suppress("BlockingMethodInNonBlockingContext") // run on Dispatchers.Main - override suspend fun onConfigurationReceived(json: JSONObject, currency: String): Boolean { + override suspend fun onConfigurationReceived(json: JSONObject, currency: String): String? { // parse categories val categoriesStr = json.getJSONArray("categories").toString() val categoriesType = object : TypeReference>() {} val categories: List = mapper.readValue(categoriesStr, categoriesType) if (categories.isEmpty()) { Log.e(TAG, "No valid category found.") - return false + return context.getString(R.string.config_error_category) } // pre-select the first category categories[0].selected = true @@ -52,23 +56,21 @@ class OrderManager(private val mapper: ObjectMapper) : ConfigurationReceiver { // group products by categories productsByCategory.clear() - val seenIds = ArrayList() products.forEach { product -> val productCurrency = fromString(product.price).currency if (productCurrency != currency) { Log.e(TAG, "Product $product has currency $productCurrency, $currency expected") - return false + return context.getString( + R.string.config_error_currency, product.description, productCurrency, currency + ) } - if (seenIds.contains(product.id)) { - Log.e(TAG, "Product $product has duplicate product_id ${product.id}") - return false - } - seenIds.add(product.id) product.categories.forEach { categoryId -> val category = categories.find { it.id == categoryId } if (category == null) { Log.e(TAG, "Product $product has unknown category $categoryId") - return false + return context.getString( + R.string.config_error_product_category_id, product.description, categoryId + ) } if (productsByCategory.containsKey(category)) { productsByCategory[category]?.add(product) @@ -86,10 +88,8 @@ class OrderManager(private val mapper: ObjectMapper) : ConfigurationReceiver { orders[id] = MutableLiveOrder(id, productsByCategory) mCurrentOrderId.postValue(id) } - true - } else { - false - } + null // success, no error string + } else context.getString(R.string.config_error_product_zero) } @UiThread diff --git a/app/src/main/java/net/taler/merchantpos/order/OrderStateFragment.kt b/app/src/main/java/net/taler/merchantpos/order/OrderStateFragment.kt index 9a40577..3afb2cf 100644 --- a/app/src/main/java/net/taler/merchantpos/order/OrderStateFragment.kt +++ b/app/src/main/java/net/taler/merchantpos/order/OrderStateFragment.kt @@ -128,7 +128,7 @@ private class OrderAdapter : Adapter() { return oldItem.quantity == newItem.quantity } } - private val differ = AsyncListDiffer(this, itemCallback) + private val differ = AsyncListDiffer(this, itemCallback) override fun getItemCount() = differ.currentList.size diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 7ce1ecb..740a080 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -21,9 +21,14 @@ Username Password Fetch Configuration - Invalid URL - Invalid username or password - Error: Invalid Configuration + Error: Invalid username or password + Error: Could not connect to configuration server + Error: No valid product category found + Error: The configuration JSON is malformed + Error: Product %1$s has currency %2$s, but %3$s expected + Error: Product %1$s references unknown category ID %2$d + Error: No valid products found + Error: Invalid Configuration Fetching Configuration… Remember Password Forget diff --git a/app/src/test/java/net/taler/merchantpos/order/OrderManagerTest.kt b/app/src/test/java/net/taler/merchantpos/order/OrderManagerTest.kt new file mode 100644 index 0000000..1507b22 --- /dev/null +++ b/app/src/test/java/net/taler/merchantpos/order/OrderManagerTest.kt @@ -0,0 +1,135 @@ +package net.taler.merchantpos.order + +import android.app.Application +import androidx.test.core.app.ApplicationProvider.getApplicationContext +import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.fasterxml.jackson.databind.DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES +import com.fasterxml.jackson.databind.ObjectMapper +import com.fasterxml.jackson.module.kotlin.KotlinModule +import kotlinx.coroutines.runBlocking +import net.taler.merchantpos.R +import org.json.JSONObject +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test +import org.junit.runner.RunWith + +@RunWith(AndroidJUnit4::class) +class OrderManagerTest { + + private val mapper = ObjectMapper() + .registerModule(KotlinModule()) + .configure(FAIL_ON_UNKNOWN_PROPERTIES, false) + + private val app: Application = getApplicationContext() + private val orderManager = OrderManager(app, mapper) + + @Test + fun `config test missing categories`() = runBlocking { + val json = JSONObject( + """ + { "categories": [] } + """.trimIndent() + ) + val result = orderManager.onConfigurationReceived(json, "KUDOS") + assertEquals(app.getString(R.string.config_error_category), result) + } + + @Test + fun `config test currency mismatch`() = runBlocking { + val json = JSONObject( + """{ + "categories": [ + { + "id": 1, + "name": "Snacks" + } + ], + "products": [ + { + "product_id": "631361561", + "description": "Chips", + "price": "WRONGCURRENCY:1.00", + "categories": [ 1 ], + "delivery_location": "cafeteria" + } + ] + }""".trimIndent() + ) + val result = orderManager.onConfigurationReceived(json, "KUDOS") + val expectedStr = app.getString( + R.string.config_error_currency, "Chips", "WRONGCURRENCY", "KUDOS" + ) + assertEquals(expectedStr, result) + } + + @Test + fun `config test unknown category ID`() = runBlocking { + val json = JSONObject( + """{ + "categories": [ + { + "id": 1, + "name": "Snacks" + } + ], + "products": [ + { + "product_id": "631361561", + "description": "Chips", + "price": "KUDOS:1.00", + "categories": [ 2 ] + } + ] + }""".trimIndent() + ) + val result = orderManager.onConfigurationReceived(json, "KUDOS") + val expectedStr = app.getString( + R.string.config_error_product_category_id, "Chips", 2 + ) + assertEquals(expectedStr, result) + } + + @Test + fun `config test no products`() = runBlocking { + val json = JSONObject( + """{ + "categories": [ + { + "id": 1, + "name": "Snacks" + } + ], + "products": [] + }""".trimIndent() + ) + val result = orderManager.onConfigurationReceived(json, "KUDOS") + val expectedStr = app.getString(R.string.config_error_product_zero) + assertEquals(expectedStr, result) + } + + @Test + fun `config test valid config gets accepted`() = runBlocking { + val json = JSONObject( + """{ + "categories": [ + { + "id": 1, + "name": "Snacks" + } + ], + "products": [ + { + "product_id": "631361561", + "description": "Chips", + "price": "KUDOS:1.00", + "categories": [ 1 ] + } + ] + }""".trimIndent() + ) + val result = orderManager.onConfigurationReceived(json, "KUDOS") + assertNull(result) + } + +} -- cgit v1.2.3