Show Meaningful Configuration Parsing Errors

Closes #0006113

10 files changed, 228 insertions, 61 deletions

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<Order> = MutableLiveData(Order(id, availableCategories))
     override val orderTotal: LiveData<Double> = Transformations.map(order) { it.total }
-    override val restartState = MutableLiveData<RestartState>(DISABLED)
+    override val restartState = MutableLiveData(DISABLED)
     private val selectedOrderLine = MutableLiveData<ConfigProduct>()
     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<List<Category>>()
     internal val categories: LiveData<List<Category>> = 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<List<Category>>() {}
         val categories: List<Category> = 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<String>()
         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<OrderViewHolder>() {
             return oldItem.quantity == newItem.quantity
         }
     }
-    private val differ = AsyncListDiffer<ConfigProduct>(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 @@
     <string name="config_username">Username</string>
     <string name="config_password">Password</string>
     <string name="config_ok">Fetch Configuration</string>
-    <string name="config_malformed_url">Invalid URL</string>
-    <string name="config_auth_error">Invalid username or password</string>
-    <string name="config_error">Error: Invalid Configuration</string>
+    <string name="config_auth_error">Error: Invalid username or password</string>
+    <string name="config_error_network">Error: Could not connect to configuration server</string>
+    <string name="config_error_category">Error: No valid product category found</string>
+    <string name="config_error_malformed">Error: The configuration JSON is malformed</string>
+    <string name="config_error_currency">Error: Product %1$s has currency %2$s, but %3$s expected</string>
+    <string name="config_error_product_category_id">Error: Product %1$s references unknown category ID %2$d</string>
+    <string name="config_error_product_zero">Error: No valid products found</string>
+    <string name="config_error_unknown">Error: Invalid Configuration</string>
     <string name="config_fetching">Fetching Configuration…</string>
     <string name="config_save_password">Remember Password</string>
     <string name="config_forget_password">Forget</string>
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)
+    }
+
+}