Custom Fields: Local editing logic #14029

hafizrahman · 2024-09-24T15:11:47Z

Part of: #13493

Please do not merge until target is trunk

Description

The flow for editing/adding/deleting custom fields can be described as follows:

Each item in the Custom Fields List screen can open the Custom Field Editor screen to be edited. In the editor, merchants can change things and then tap "Done" when they're done editing. Doing so will return them to the list screen and the change will be shown.
Merchant can also delete a field from its Custom Field Editor screen. This removes the field from the list screen.
Custom Fields List also has an add button that opens the editor, then merchant can also tap "Done" to keep the change. This adds a field to the list screen.
All changes from edit/delete/add actions above are still pending, similar to making changes to various aspects of products in Product Details screen. The Custom Fields List has a "Save" button, which acts as the only way to do remote API call to save. It will save all existing changes at once, matching the behavior in core.

This PR handles the local editing behavior, by adding these:

CustomFieldsListViewModel which handles the business logic.
The viewmodel contains editedFields and addedFields arrays to store the pending edit and addition. It also has combinedList which is an array of the latest combined changes. These later can be used for making the API call.
The viewmodel has editField() and addField() functions that can later be used when editing or adding a field.
Unit tests for the viewmodel.

Not in this PR and will be added separately:

Handling for deletion.
UI functionality for editing or adding field.
UI functionality for remote saving

Steps to reproduce

The handling is not used anywhere in the app yet, so for now just check the code and ensure the unit tests also passes.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

Contains logic for locally editing custom fields while keeping track of two main arrays, editedFields and addedFields.

dangermattic · 2024-09-24T15:12:27Z

	1 Warning
⚠️	View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.

Generated by 🚫 Danger

wpmobilebot · 2024-09-24T15:21:26Z

📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS
	Build Number	pr14029-f280d61
	Version	20.5
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`f280d61`
	App Center Build	WooCommerce - Prototype Builds #11041

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

pmusolino

In general everything looks good @hafizrahman.

I just left some minor nits and two changes:

one related to an id that is not unique if nil.
more unit tests.

pmusolino · 2024-09-24T16:17:42Z

WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift

+        if newField.id == nil {
+            // If there's no id, it means we're now editing a newly added field.
+            if let addedIndex = addedFields.firstIndex(where: { $0.key == oldField.key }) {
+                if newField.key == oldField.key {
+                    // If the key hasn't changed, update the existing added field
+                    addedFields[addedIndex] = newField
+                } else {
+                    // If the key has changed, remove the old field and add the new one
+                    addedFields.remove(at: addedIndex)
+                    addedFields.append(newField)
+                }
+            } else {
+                // This case should not happen in normal flow
+                DDLogError("⛔️ Error: Editing a newly updated field that doesn't exist in combinedList")
+
+            }
+        } else {
+            // For when editing an already edited field
+            if let editedIndex = editedFields.firstIndex(where: { $0.id == oldField.id }) {
+                editedFields[editedIndex] = newField
+            } else {
+                // For the first time a field is edited
+                editedFields.append(newField)
+            }
+        }


nit: this logic can be even clearer if you split it into two different private methods.

Eg:

if newField.id == nil { handleNewlyAddedField(oldField: oldField, newField: newField) } else { handleEditedField(oldField: oldField, newField: newField) }

Refactored in 98cec53

pmusolino · 2024-09-24T16:20:27Z

WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift

+    func updateCombinedList() {
+            let editedList = originalCustomFields.map { field in
+                editedFields.first { $0.id == field.id } ?? CustomFieldUI(customField: field)
+            }
+            combinedList = editedList + addedFields
+        }


nit: indentation here looks wrong

2dde854

thanks, good catch

pmusolino · 2024-09-24T16:22:06Z

WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift

+            } else {
+                // This case should not happen in normal flow
+                DDLogError("⛔️ Error: Editing a newly updated field that doesn't exist in combinedList")
+


nit: empty return that should be deleted

Removed as part of refactor 98cec53

pmusolino · 2024-09-24T16:34:50Z

WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift

+    struct CustomFieldUI: Identifiable {
+        let key: String
+        let value: String
+        let id: Int64?
+
+        init(key: String, value: String, id: Int64? = nil) {
+            self.key = key
+            self.value = value
+            self.id = id
+        }
+
+        init(customField: CustomFieldViewModel) {
+            self.key = customField.title
+            self.value = customField.content
+            self.id = customField.id
+        }
+    }


This implementation can cause some issues in SwiftUI, which expects each item in a list to have a unique identifier to properly handle updates, deletions, and reordering.

If the id property is optional (Int64?) and there are two instances of CustomFieldUI with a nil id, both instances will not have a unique identifier.

To solve this problem, you can ensure that CustomFieldUI generates a unique identifier when id is nil. A common solution is to use an automatically generated UUID. Since a custom field can also have its own ID, I suggest adding an optional customFieldID. The id will become a unique UUID if customFieldID is nil, or it will be equal to the customFieldID if it is provided. This is just a proposal; you may have a better solution in mind.

Thanks! Addressed in 0e29067

WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift

Co-authored-by: Paolo Musolino <info@codeido.com>

hafizrahman · 2024-09-26T06:32:06Z

Thanks for the review! Good for another round @pmusolino

pmusolino

Changes looks good, thanks @hafizrahman !

wpmobilebot · 2024-09-27T11:00:06Z

Version 20.6 has now entered code-freeze, so the milestone of this PR has been updated to 20.7.

hafizrahman added 3 commits September 24, 2024 21:38

Add view model for custom fields list.

5d63ab4

Contains logic for locally editing custom fields while keeping track of two main arrays, editedFields and addedFields.

Update CustomFieldsListView to make use of CustomFieldsListViewModel

abb6f23

Add unit tests for CustomFieldsListViewModel

4ff4858

hafizrahman mentioned this pull request Sep 24, 2024

Custom Fields: Local editing handling #13898

Closed

3 tasks

hafizrahman marked this pull request as ready for review September 24, 2024 15:14

hafizrahman added this to the 20.6 milestone Sep 24, 2024

Add error handling for invalid index

f229605

hafizrahman requested a review from pmusolino September 24, 2024 15:25

hafizrahman added the type: task An internally driven task. label Sep 24, 2024

pmusolino requested changes Sep 24, 2024

View reviewed changes

pmusolino self-assigned this Sep 25, 2024

hafizrahman and others added 5 commits September 26, 2024 09:16

Refactor editing locally added or existing field to make it clearer.

98cec53

Fix indentation

2dde854

Update comments to make logic clearer.

66f425b

Add unit tests for adding duplicate key and editing invalid index

547e190

Co-authored-by: Paolo Musolino <info@codeido.com>

Add UUID for CustomFIeldUI and refactor previous id as fieldId.

0e29067

hafizrahman requested a review from pmusolino September 26, 2024 03:17

pmusolino approved these changes Sep 26, 2024

View reviewed changes

wpmobilebot modified the milestones: 20.6, 20.7 Sep 27, 2024

Update unit tests.

f280d61

hafizrahman mentioned this pull request Sep 28, 2024

Implement local Save functionality for existing Custom Fields #14061

Merged

4 tasks

Base automatically changed from feat/13493-refactor-aztec-make-more-generic to trunk September 30, 2024 08:23

hafizrahman merged commit 1168651 into trunk Sep 30, 2024
14 checks passed

hafizrahman deleted the feat/13493-custom-fields-local-editing branch September 30, 2024 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom Fields: Local editing logic #14029

Custom Fields: Local editing logic #14029

hafizrahman commented Sep 24, 2024 •

edited

Loading

dangermattic commented Sep 24, 2024 •

edited

Loading

wpmobilebot commented Sep 24, 2024 •

edited

Loading

pmusolino left a comment

pmusolino Sep 24, 2024

hafizrahman Sep 26, 2024

pmusolino Sep 24, 2024

hafizrahman Sep 26, 2024

pmusolino Sep 24, 2024

hafizrahman Sep 26, 2024

pmusolino Sep 24, 2024

hafizrahman Sep 26, 2024

hafizrahman commented Sep 26, 2024

pmusolino left a comment

wpmobilebot commented Sep 27, 2024

Custom Fields: Local editing logic #14029

Custom Fields: Local editing logic #14029

Conversation

hafizrahman commented Sep 24, 2024 • edited Loading

Description

Steps to reproduce

Reviewer (or Author, in the case of optional code reviews):

dangermattic commented Sep 24, 2024 • edited Loading

wpmobilebot commented Sep 24, 2024 • edited Loading

pmusolino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hafizrahman commented Sep 26, 2024

pmusolino left a comment

Choose a reason for hiding this comment

wpmobilebot commented Sep 27, 2024

hafizrahman commented Sep 24, 2024 •

edited

Loading

dangermattic commented Sep 24, 2024 •

edited

Loading

wpmobilebot commented Sep 24, 2024 •

edited

Loading