Custom Fields: Local editing logic #14029
Conversation
Contains logic for locally editing custom fields while keeping track of two main arrays, editedFields and addedFields.
Generated by 🚫 Danger |
|
|
There was a problem hiding this comment.
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.
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
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)
}
There was a problem hiding this comment.
Refactored in 98cec53
| func updateCombinedList() { | ||
| let editedList = originalCustomFields.map { field in | ||
| editedFields.first { $0.id == field.id } ?? CustomFieldUI(customField: field) | ||
| } | ||
| combinedList = editedList + addedFields | ||
| } |
There was a problem hiding this comment.
nit: indentation here looks wrong
There was a problem hiding this comment.
thanks, good catch
| } else { | ||
| // This case should not happen in normal flow | ||
| DDLogError("⛔️ Error: Editing a newly updated field that doesn't exist in combinedList") | ||
|
|
There was a problem hiding this comment.
nit: empty return that should be deleted
There was a problem hiding this comment.
Removed as part of refactor 98cec53
| 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks! Addressed in 0e29067
WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift
Show resolved
Hide resolved
Co-authored-by: Paolo Musolino <[email protected]>
|
Thanks for the review! Good for another round @pmusolino |
|
Version |
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:
This PR handles the local editing behavior, by adding these:
CustomFieldsListViewModelwhich handles the business logic.editedFieldsandaddedFieldsarrays to store the pending edit and addition. It also hascombinedListwhich is an array of the latest combined changes. These later can be used for making the API call.editField()andaddField()functions that can later be used when editing or adding a field.Not in this PR and will be added separately:
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: