From 948e8ee409a47c4b99802b291fc3d32ea903abb4 Mon Sep 17 00:00:00 2001 From: Hicham Boushaba Date: Wed, 11 Sep 2024 15:44:34 +0100 Subject: [PATCH 1/6] Fix issue of clickability of the custom fields content Before this, clicks were not registered when done on the field value text --- .../customfields/list/CustomFieldsScreen.kt | 52 +++++++++++-------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsScreen.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsScreen.kt index 5271960f845..3ee82bff0bd 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsScreen.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsScreen.kt @@ -41,6 +41,7 @@ import androidx.compose.ui.text.SpanStyle import androidx.compose.ui.text.UrlAnnotation import androidx.compose.ui.text.buildAnnotatedString import androidx.compose.ui.text.style.TextOverflow +import androidx.compose.ui.text.withStyle import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp import com.woocommerce.android.R @@ -182,29 +183,38 @@ private fun CustomFieldItem( ) Spacer(modifier = Modifier.height(4.dp)) - val text = buildAnnotatedString { - if (customField.contentType != CustomFieldContentType.TEXT) { - pushUrlAnnotation(UrlAnnotation(customField.value)) - pushStyle(SpanStyle(color = MaterialTheme.colors.primary)) - } - append(customField.valueStrippedHtml) - } - ClickableText( - text = text, - style = MaterialTheme.typography.body2.copy( - color = MaterialTheme.colors.onSurface - ), - maxLines = 2, - overflow = TextOverflow.Ellipsis, - onClick = { offset -> - text.getUrlAnnotations( - start = offset, - end = offset - ).firstOrNull()?.let { _ -> - onValueClicked(customField) + + if (customField.contentType != CustomFieldContentType.TEXT) { + val text = buildAnnotatedString { + withStyle(SpanStyle(color = MaterialTheme.colors.primary)) { + pushUrlAnnotation(UrlAnnotation(customField.value)) + append(customField.value) } } - ) + ClickableText( + text = text, + style = MaterialTheme.typography.body2.copy( + color = MaterialTheme.colors.onSurface + ), + maxLines = 2, + overflow = TextOverflow.Ellipsis, + onClick = { offset -> + text.getUrlAnnotations( + start = offset, + end = offset + ).firstOrNull()?.let { _ -> + onValueClicked(customField) + } + } + ) + } else { + Text( + text = customField.value, + maxLines = 2, + overflow = TextOverflow.Ellipsis, + style = MaterialTheme.typography.body2 + ) + } } Icon( From ad94ecd444e9e23c6ef5335effe266065096b0be Mon Sep 17 00:00:00 2001 From: Hicham Boushaba Date: Wed, 11 Sep 2024 16:08:52 +0100 Subject: [PATCH 2/6] Fix condition for checking for the insertion flow --- .../ui/customfields/editor/CustomFieldsEditorViewModel.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModel.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModel.kt index 7194bf2078a..6d97d48df2d 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModel.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModel.kt @@ -83,7 +83,7 @@ class CustomFieldsEditorViewModel @Inject constructor( fun onDoneClicked() { val value = requireNotNull(customFieldDraft.value) - if (storedValue == null) { + if (value.id == null) { // Check for duplicate keys before inserting the new custom field // For more context: pe5sF9-33t-p2#comment-3880 launch { From 205a6840930eddabeaaa9494f4423402c47c8af8 Mon Sep 17 00:00:00 2001 From: Hicham Boushaba Date: Wed, 11 Sep 2024 16:20:37 +0100 Subject: [PATCH 3/6] Separate keys for custom field creation and update --- .../editor/CustomFieldsEditorViewModel.kt | 27 ++++++++++--------- .../customfields/list/CustomFieldsFragment.kt | 9 +++---- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModel.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModel.kt index 6d97d48df2d..94e05219d85 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModel.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModel.kt @@ -27,6 +27,7 @@ class CustomFieldsEditorViewModel @Inject constructor( private val repository: CustomFieldsRepository ) : ScopedViewModel(savedStateHandle) { companion object { + const val CUSTOM_FIELD_CREATED_RESULT_KEY = "custom_field_created" const val CUSTOM_FIELD_UPDATED_RESULT_KEY = "custom_field_updated" const val CUSTOM_FIELD_DELETED_RESULT_KEY = "custom_field_deleted" } @@ -82,25 +83,25 @@ class CustomFieldsEditorViewModel @Inject constructor( } fun onDoneClicked() { - val value = requireNotNull(customFieldDraft.value) - if (value.id == null) { - // Check for duplicate keys before inserting the new custom field - // For more context: pe5sF9-33t-p2#comment-3880 - launch { + launch { + val value = requireNotNull(customFieldDraft.value) + if (value.id == null) { + // Check for duplicate keys before inserting the new custom field + // For more context: pe5sF9-33t-p2#comment-3880 val existingFields = repository.getDisplayableCustomFields(navArgs.parentItemId) if (existingFields.any { it.key == value.key }) { keyErrorMessage.value = UiString.UiStringRes(R.string.custom_fields_editor_key_error_duplicate) - } else { - triggerEvent( - MultiLiveEvent.Event.ExitWithResult(data = value, key = CUSTOM_FIELD_UPDATED_RESULT_KEY) - ) + return@launch } } - } else { - // When editing, we don't need to check for duplicate keys - triggerEvent( + + val event = if (storedValue == null) { + MultiLiveEvent.Event.ExitWithResult(data = value, key = CUSTOM_FIELD_CREATED_RESULT_KEY) + + } else { MultiLiveEvent.Event.ExitWithResult(data = value, key = CUSTOM_FIELD_UPDATED_RESULT_KEY) - ) + } + triggerEvent(event) } } diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsFragment.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsFragment.kt index f3a38d521f2..fa0ca769b4f 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsFragment.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsFragment.kt @@ -64,12 +64,11 @@ class CustomFieldsFragment : BaseFragment() { } private fun handleResults() { + handleResult(CustomFieldsEditorViewModel.CUSTOM_FIELD_CREATED_RESULT_KEY) { result -> + viewModel.onCustomFieldInserted(result) + } handleResult(CustomFieldsEditorViewModel.CUSTOM_FIELD_UPDATED_RESULT_KEY) { result -> - if (result.id == null) { - viewModel.onCustomFieldInserted(result) - } else { - viewModel.onCustomFieldUpdated(result) - } + viewModel.onCustomFieldUpdated(result) } handleResult(CustomFieldsEditorViewModel.CUSTOM_FIELD_DELETED_RESULT_KEY) { result -> viewModel.onCustomFieldDeleted(result) From 0c8557f2a5e07e147a6e2e1fd6c17ca494171d74 Mon Sep 17 00:00:00 2001 From: Hicham Boushaba Date: Wed, 11 Sep 2024 16:23:51 +0100 Subject: [PATCH 4/6] Make sure that editing a non-saved field doesn't insert a new separate one --- .../editor/CustomFieldsEditorViewModel.kt | 13 ++++++++++++- .../ui/customfields/list/CustomFieldsFragment.kt | 5 +++-- .../ui/customfields/list/CustomFieldsViewModel.kt | 9 +++++++-- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModel.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModel.kt index 94e05219d85..b54d9262749 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModel.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModel.kt @@ -1,5 +1,6 @@ package com.woocommerce.android.ui.customfields.editor +import android.os.Parcelable import androidx.annotation.StringRes import androidx.lifecycle.SavedStateHandle import androidx.lifecycle.asLiveData @@ -19,6 +20,7 @@ import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch +import kotlinx.parcelize.Parcelize import javax.inject.Inject @HiltViewModel @@ -99,7 +101,10 @@ class CustomFieldsEditorViewModel @Inject constructor( MultiLiveEvent.Event.ExitWithResult(data = value, key = CUSTOM_FIELD_CREATED_RESULT_KEY) } else { - MultiLiveEvent.Event.ExitWithResult(data = value, key = CUSTOM_FIELD_UPDATED_RESULT_KEY) + MultiLiveEvent.Event.ExitWithResult( + data = CustomFieldUpdateResult(oldKey = storedValue.key, updatedField = value), + key = CUSTOM_FIELD_UPDATED_RESULT_KEY + ) } triggerEvent(event) } @@ -159,4 +164,10 @@ class CustomFieldsEditorViewModel @Inject constructor( @StringRes val labelResource: Int, val content: String ) : MultiLiveEvent.Event() + + @Parcelize + data class CustomFieldUpdateResult( + val oldKey: String, + val updatedField: CustomFieldUiModel + ) : Parcelable } diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsFragment.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsFragment.kt index fa0ca769b4f..cb52225db4f 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsFragment.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsFragment.kt @@ -15,6 +15,7 @@ import com.woocommerce.android.ui.compose.composeView import com.woocommerce.android.ui.customfields.CustomFieldContentType import com.woocommerce.android.ui.customfields.CustomFieldUiModel import com.woocommerce.android.ui.customfields.editor.CustomFieldsEditorViewModel +import com.woocommerce.android.ui.customfields.editor.CustomFieldsEditorViewModel.CustomFieldUpdateResult import com.woocommerce.android.ui.main.AppBarStatus import com.woocommerce.android.util.ActivityUtils import com.woocommerce.android.util.ChromeCustomTabUtils @@ -67,8 +68,8 @@ class CustomFieldsFragment : BaseFragment() { handleResult(CustomFieldsEditorViewModel.CUSTOM_FIELD_CREATED_RESULT_KEY) { result -> viewModel.onCustomFieldInserted(result) } - handleResult(CustomFieldsEditorViewModel.CUSTOM_FIELD_UPDATED_RESULT_KEY) { result -> - viewModel.onCustomFieldUpdated(result) + handleResult(CustomFieldsEditorViewModel.CUSTOM_FIELD_UPDATED_RESULT_KEY) { result -> + viewModel.onCustomFieldUpdated(result.oldKey, result.updatedField) } handleResult(CustomFieldsEditorViewModel.CUSTOM_FIELD_DELETED_RESULT_KEY) { result -> viewModel.onCustomFieldDeleted(result) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsViewModel.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsViewModel.kt index 716832cb687..99abccc7c18 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsViewModel.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsViewModel.kt @@ -97,9 +97,14 @@ class CustomFieldsViewModel @Inject constructor( } } - fun onCustomFieldUpdated(result: CustomFieldUiModel) { + fun onCustomFieldUpdated(oldValueKey: String, result: CustomFieldUiModel) { pendingChanges.update { - it.copy(editedFields = it.editedFields.filterNot { field -> field.id == result.id } + result) + if (result.id == null) { + // We are updating a field that was just added and hasn't been saved yet + it.copy(insertedFields = it.insertedFields.filterNot { field -> field.key == oldValueKey } + result) + } else { + it.copy(editedFields = it.editedFields.filterNot { field -> field.id == result.id } + result) + } } } From 3385bfef67eb5caaa7964ecf4c433873717dad64 Mon Sep 17 00:00:00 2001 From: Hicham Boushaba Date: Wed, 11 Sep 2024 16:33:14 +0100 Subject: [PATCH 5/6] Add and fix unit tests --- .../editor/CustomFieldsEditorViewModelTest.kt | 27 +++++++++++++++++-- .../list/CustomFieldsViewModelTest.kt | 27 ++++++++++++++++--- 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModelTest.kt b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModelTest.kt index ed838f2a466..59b333a3da0 100644 --- a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModelTest.kt +++ b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModelTest.kt @@ -171,7 +171,7 @@ class CustomFieldsEditorViewModelTest : BaseUnitTest() { } @Test - fun `when done is clicked, then exit with result`() = testBlocking { + fun `given editing an existing field, when done is clicked, then exit with result`() = testBlocking { setup(editing = true) val events = viewModel.event.runAndCaptureValues { @@ -182,12 +182,35 @@ class CustomFieldsEditorViewModelTest : BaseUnitTest() { assertThat(events).isEqualTo( MultiLiveEvent.Event.ExitWithResult( - data = CustomFieldUiModel(id = CUSTOM_FIELD_ID, key = "new key", value = "new value"), + data = CustomFieldsEditorViewModel.CustomFieldUpdateResult( + CUSTOM_FIELD.key, + CustomFieldUiModel(id = CUSTOM_FIELD_ID, key = "new key", value = "new value") + ), key = CustomFieldsEditorViewModel.CUSTOM_FIELD_UPDATED_RESULT_KEY ) ) } + @Test + fun `given creating a new field, when done is clicked, then exit with result`() = testBlocking { + setup(editing = false) { + whenever(repository.getDisplayableCustomFields(PARENT_ITEM_ID)).thenReturn(emptyList()) + } + + val events = viewModel.event.runAndCaptureValues { + viewModel.onKeyChanged("key") + viewModel.onValueChanged("value") + viewModel.onDoneClicked() + }.last() + + assertThat(events).isEqualTo( + MultiLiveEvent.Event.ExitWithResult( + data = CustomFieldUiModel(key = "key", value = "value"), + key = CustomFieldsEditorViewModel.CUSTOM_FIELD_CREATED_RESULT_KEY + ) + ) + } + @Test fun `given adding a new field, when key is duplicate, then show error`() = testBlocking { setup(editing = false) { diff --git a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsViewModelTest.kt b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsViewModelTest.kt index 1c3b80850d9..6d8a5f1a120 100644 --- a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsViewModelTest.kt +++ b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsViewModelTest.kt @@ -217,7 +217,7 @@ class CustomFieldsViewModelTest : BaseUnitTest() { setup() val state = viewModel.state.runAndCaptureValues { - viewModel.onCustomFieldUpdated(customField) + viewModel.onCustomFieldUpdated(CUSTOM_FIELDS.first().key, customField) advanceUntilIdle() }.last() @@ -230,8 +230,8 @@ class CustomFieldsViewModelTest : BaseUnitTest() { setup() val state = viewModel.state.runAndCaptureValues { - viewModel.onCustomFieldUpdated(customField.copy(value = "new value")) - viewModel.onCustomFieldUpdated(customField.copy(value = "new value 2")) + viewModel.onCustomFieldUpdated(CUSTOM_FIELDS.first().key, customField.copy(value = "new value")) + viewModel.onCustomFieldUpdated(CUSTOM_FIELDS.first().key, customField.copy(value = "new value 2")) advanceUntilIdle() }.last() @@ -256,6 +256,25 @@ class CustomFieldsViewModelTest : BaseUnitTest() { assertThat(state.customFields.last().value).isEqualTo(customField.value) } + @Test + fun `when adding a custom field then updating it, then confirm the field is not duplicated`() = testBlocking { + val customField = CustomFieldUiModel( + key = "new key", + value = "new value" + ) + setup() + + val state = viewModel.state.runAndCaptureValues { + viewModel.onCustomFieldInserted(customField) + viewModel.onCustomFieldUpdated(customField.key, customField.copy(value = "new value 2")) + advanceUntilIdle() + }.last() + + assertThat(state.customFields).hasSize(CUSTOM_FIELDS.size + 1) + assertThat(state.customFields.last().key).isEqualTo(customField.key) + assertThat(state.customFields.last().value).isEqualTo("new value 2") + } + @Test fun `when deleting a custom field, then custom fields are refreshed`() = testBlocking { val customField = CustomFieldUiModel(CUSTOM_FIELDS.first()) @@ -325,7 +344,7 @@ class CustomFieldsViewModelTest : BaseUnitTest() { setup() - viewModel.onCustomFieldUpdated(updatedField) + viewModel.onCustomFieldUpdated(CUSTOM_FIELDS.first().key, updatedField) viewModel.onCustomFieldInserted(insertedField) viewModel.onSaveClicked() From 7de6ad072bd5010ce7386d01a60bb4b3f8461ca5 Mon Sep 17 00:00:00 2001 From: Hicham Boushaba Date: Wed, 11 Sep 2024 16:34:04 +0100 Subject: [PATCH 6/6] Remove blank lines --- .../ui/customfields/editor/CustomFieldsEditorViewModel.kt | 1 - .../android/ui/customfields/list/CustomFieldsScreen.kt | 1 - 2 files changed, 2 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModel.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModel.kt index b54d9262749..c2172cb9d29 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModel.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/editor/CustomFieldsEditorViewModel.kt @@ -99,7 +99,6 @@ class CustomFieldsEditorViewModel @Inject constructor( val event = if (storedValue == null) { MultiLiveEvent.Event.ExitWithResult(data = value, key = CUSTOM_FIELD_CREATED_RESULT_KEY) - } else { MultiLiveEvent.Event.ExitWithResult( data = CustomFieldUpdateResult(oldKey = storedValue.key, updatedField = value), diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsScreen.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsScreen.kt index 3ee82bff0bd..8bb09db20f7 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsScreen.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/customfields/list/CustomFieldsScreen.kt @@ -183,7 +183,6 @@ private fun CustomFieldItem( ) Spacer(modifier = Modifier.height(4.dp)) - if (customField.contentType != CustomFieldContentType.TEXT) { val text = buildAnnotatedString { withStyle(SpanStyle(color = MaterialTheme.colors.primary)) {