Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Custom fields: Entry point logic in Product Details #14068

Open
wants to merge 21 commits into
base: trunk
Choose a base branch
from
Open

Custom fields: Entry point logic in Product Details #14068

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
a56dcf9
add "show custom fields" functionality on simple products.
hafizrahman Sep 28, 2024
3999821
Update logic for showing the Custom Fields option in Product details.
hafizrahman Sep 28, 2024
1092e0f
Move Custom Fields to the end for simple product actions.
hafizrahman Sep 29, 2024
61836a2
Add Custom Fields option to all other type of products.
hafizrahman Sep 29, 2024
72b669f
Add logic to hide custom fields option when creating new product.
hafizrahman Sep 29, 2024
9b6ffbc
Update custom fields option icon.
hafizrahman Sep 29, 2024
6fdc39d
Add visibility unit tests for custom fields option.
hafizrahman Sep 29, 2024
cd64bbc
Refactor to pass FeatureFlagService to the factory so that it's testa…
hafizrahman Sep 29, 2024
7076bde
Update visibility tests to include feature flag
hafizrahman Sep 29, 2024
f6f1c33
Refactor add new product case checking by using formType instead.
hafizrahman Sep 29, 2024
d6dcaf3
Move custom fields test about creation to ProductCreationTests
hafizrahman Sep 29, 2024
c1d5a44
Update unit tests with extra checks.
hafizrahman Sep 30, 2024
cfd6c47
Various design updates to make the navigation change to/from custom f…
hafizrahman Oct 1, 2024
1803039
Update test names
hafizrahman Oct 1, 2024
a8639e4
For custom fields visibility tests, loop through the entire product t…
hafizrahman Oct 1, 2024
722f978
Update requirement to only show custom fields row in product details …
hafizrahman Oct 1, 2024
3b45405
Add product details readonly tests related to custom fields row.
hafizrahman Oct 1, 2024
aa0ccd5
Fix strings for custom fields row title and subtitle
hafizrahman Oct 3, 2024
d513b54
Keep animation on push case.
hafizrahman Oct 3, 2024
fa038ce
Move CaseIterable conformance into unit test instead.
hafizrahman Oct 3, 2024
fbcabcd
Merge branch 'trunk' into feat/14067-show-custom-fields-product-details
hafizrahman Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion WooCommerce/Classes/Extensions/UIImage+Woo.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -984,7 +984,7 @@ extension UIImage {
/// Custom Fields Icon
///
static var customFieldsImage: UIImage {
return UIImage.gridicon(.alignLeft, size: CGSize(width: 24, height: 24))
return UIImage.gridicon(.grid, size: CGSize(width: 24, height: 24))
}

/// Print Icon
Expand Down
13 changes: 13 additions & 0 deletions ...wRelated/Products/Edit Product/BottomSheetListSelector/ProductFormBottomSheetAction.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ enum ProductFormBottomSheetAction {
case editLinkedProducts
case editReviews
case editDownloadableFiles
case editCustomFields

init?(productFormAction: ProductFormEditAction) {
switch productFormAction {
Expand All @@ -33,6 +34,8 @@ enum ProductFormBottomSheetAction {
self = .editReviews
case .downloadableFiles:
self = .editDownloadableFiles
case .customFields:
self = .editCustomFields
default:
return nil
}
Expand Down Expand Up @@ -69,6 +72,11 @@ extension ProductFormBottomSheetAction {
case .editDownloadableFiles:
return NSLocalizedString("Downloadable files",
comment: "Title of the product form bottom sheet action for editing downloadable files.")
case .editCustomFields:
return NSLocalizedString("productFormBottomSheetAction.customFieldsTitle",
value: "Custom Fields",
comment: "Title of the product form bottom sheet action for custom fields")

}
}

Expand Down Expand Up @@ -101,6 +109,11 @@ extension ProductFormBottomSheetAction {
case .editDownloadableFiles:
return NSLocalizedString("Include downloadable files with purchases",
comment: "Subtitle of the product form bottom sheet action for editing downloadable files.")

case .editCustomFields:
return NSLocalizedString("productFormBottomSheetAction.customFieldsSubtitle",
value: "View and edit custom fields",
comment: "Subtitle of the product form bottom sheet action for custom fields")
}
}
}
47 changes: 30 additions & 17 deletions WooCommerce/Classes/ViewRelated/Products/Edit Product/ProductFormActionsFactory.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,34 +64,40 @@ struct ProductFormActionsFactory: ProductFormActionsFactoryProtocol {
private let variationsPrice: VariationsPrice

private let stores: StoresManager
private let featureFlagService: FeatureFlagService

private let isLinkedProductsPromoEnabled: Bool
private let linkedProductsPromoCampaign = LinkedProductsPromoCampaign()
private var linkedProductsPromoViewModel: FeatureAnnouncementCardViewModel {
.init(analytics: ServiceLocator.analytics,
configuration: linkedProductsPromoCampaign.configuration)
}
private let isCustomFieldsEnabled: Bool

private var isCustomFieldsEnabled: Bool {
/// There's a technical API limitation where product ID is required to save custom fields,
/// thus it needs to be disabled during new product creation
featureFlagService.isFeatureFlagEnabled(.viewEditCustomFieldsInProductsAndOrders)
&& formType != .add
itsmeichigo marked this conversation as resolved.
Show resolved Hide resolved
}

// TODO: Remove default parameter
init(product: EditableProductModel,
formType: ProductFormType,
canPromoteWithBlaze: Bool = false,
addOnsFeatureEnabled: Bool = true,
isLinkedProductsPromoEnabled: Bool = false,
isCustomFieldsEnabled: Bool =
ServiceLocator.featureFlagService.isFeatureFlagEnabled(.viewEditCustomFieldsInProductsAndOrders),
variationsPrice: VariationsPrice = .unknown,
stores: StoresManager = ServiceLocator.stores) {
stores: StoresManager = ServiceLocator.stores,
featureFlagService: FeatureFlagService = ServiceLocator.featureFlagService) {
self.product = product
self.formType = formType
self.canPromoteWithBlaze = canPromoteWithBlaze
self.editable = formType != .readonly
self.addOnsFeatureEnabled = addOnsFeatureEnabled
self.variationsPrice = variationsPrice
self.isLinkedProductsPromoEnabled = isLinkedProductsPromoEnabled
self.isCustomFieldsEnabled = isCustomFieldsEnabled
self.stores = stores
self.featureFlagService = featureFlagService
}

/// Returns an array of actions that are visible in the product form primary section.
Expand Down Expand Up @@ -172,7 +178,6 @@ private extension ProductFormActionsFactory {

let actions: [ProductFormEditAction?] = [
.priceSettings(editable: editable, hideSeparator: false),
isCustomFieldsEnabled ? .customFields: nil,
shouldShowReviewsRow ? .reviews: nil,
shouldShowShippingSettingsRow ? .shippingSettings(editable: editable): nil,
.inventorySettings(editable: canEditInventorySettingsRow),
Expand All @@ -183,7 +188,8 @@ private extension ProductFormActionsFactory {
.downloadableFiles(editable: editable),
.shortDescription(editable: editable),
.linkedProducts(editable: editable),
.productType(editable: canEditProductType)
.productType(editable: canEditProductType),
isCustomFieldsEnabled ? .customFields: nil,
]
return actions.compactMap { $0 }
}
Expand All @@ -206,7 +212,8 @@ private extension ProductFormActionsFactory {
.tags(editable: editable),
.shortDescription(editable: editable),
.linkedProducts(editable: editable),
.productType(editable: canEditProductType)
.productType(editable: canEditProductType),
isCustomFieldsEnabled ? .customFields: nil
]
return actions.compactMap { $0 }
}
Expand All @@ -227,7 +234,8 @@ private extension ProductFormActionsFactory {
.tags(editable: editable),
.shortDescription(editable: editable),
.linkedProducts(editable: editable),
.productType(editable: canEditProductType)
.productType(editable: canEditProductType),
isCustomFieldsEnabled ? .customFields: nil
]
return actions.compactMap { $0 }
}
Expand Down Expand Up @@ -257,7 +265,8 @@ private extension ProductFormActionsFactory {
.tags(editable: editable),
.shortDescription(editable: editable),
.linkedProducts(editable: editable),
.productType(editable: canEditProductType)
.productType(editable: canEditProductType),
isCustomFieldsEnabled ? .customFields: nil
]
return actions.compactMap { $0 }
}
Expand All @@ -279,7 +288,8 @@ private extension ProductFormActionsFactory {
.tags(editable: editable),
.shortDescription(editable: editable),
.linkedProducts(editable: editable),
.productType(editable: false)
.productType(editable: false),
isCustomFieldsEnabled ? .customFields: nil
]
return actions.compactMap { $0 }
}
Expand All @@ -301,7 +311,8 @@ private extension ProductFormActionsFactory {
.tags(editable: editable),
.shortDescription(editable: editable),
.linkedProducts(editable: editable),
.productType(editable: false)
.productType(editable: false),
isCustomFieldsEnabled ? .customFields: nil
]
return actions.compactMap { $0 }
}
Expand All @@ -327,7 +338,8 @@ private extension ProductFormActionsFactory {
.downloadableFiles(editable: editable),
.shortDescription(editable: editable),
.linkedProducts(editable: editable),
.productType(editable: canEditProductType)
.productType(editable: canEditProductType),
isCustomFieldsEnabled ? .customFields: nil
]
return actions.compactMap { $0 }
}
Expand Down Expand Up @@ -359,7 +371,8 @@ private extension ProductFormActionsFactory {
.tags(editable: editable),
.shortDescription(editable: editable),
.linkedProducts(editable: editable),
.productType(editable: canEditProductType)
.productType(editable: canEditProductType),
isCustomFieldsEnabled ? .customFields: nil
]
}()

Expand All @@ -381,7 +394,8 @@ private extension ProductFormActionsFactory {
.tags(editable: editable),
.shortDescription(editable: editable),
.linkedProducts(editable: editable),
.productType(editable: false)
.productType(editable: false),
isCustomFieldsEnabled ? .customFields: nil
]
return actions.compactMap { $0 }
}
Expand All @@ -398,8 +412,7 @@ private extension ProductFormActionsFactory {
// The price settings action is always visible in the settings section.
return true
case .customFields:
// The custom fields action is always visible in the settings section.
return true
return product.product.customFields.isNotEmpty
case .subscriptionFreeTrial:
// The Free trial row is always visible in the settings section for a subscription product.
return true
Expand Down
34 changes: 32 additions & 2 deletions WooCommerce/Classes/ViewRelated/Products/Edit Product/ProductFormViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import Photos
import UIKit
import WordPressUI
import Yosemite
import SwiftUI
import protocol Storage.StorageManagerType

/// The entry UI for adding/editing a Product.
Expand Down Expand Up @@ -434,8 +435,7 @@ final class ProductFormViewController<ViewModel: ProductFormViewModelProtocol>:
eventLogger.logPriceSettingsTapped()
editPriceSettings()
case .customFields:
// TODO-13493: add tap handling.
return
showCustomFields()
case .reviews:
ServiceLocator.analytics.track(.productDetailViewReviewsTapped)
showReviews()
Expand Down Expand Up @@ -935,6 +935,8 @@ private extension ProductFormViewController {
case .editDownloadableFiles:
ServiceLocator.analytics.track(.productDetailViewDownloadableFilesTapped)
self?.showDownloadableFiles()
case .editCustomFields:
self?.showCustomFields()
}
}
}
Expand Down Expand Up @@ -1458,6 +1460,34 @@ private extension ProductFormViewController {
}
}

// MARK: Action - Show Custom Fields
private extension ProductFormViewController {
func showCustomFields() {
guard let product = product as? EditableProductModel else {
return
}

let customFields = product.product.customFields.map {
CustomFieldViewModel(metadata: $0)
}

let customFieldsView = UIHostingController(
rootView: CustomFieldsListView(
isEditable: true,
viewModel: CustomFieldsListViewModel(customFields: customFields),
onBackButtonTapped: { [weak self] in
// Restore the hidden navigation bar
self?.navigationController?.setNavigationBarHidden(false, animated: true)
})
)

// Hide the navigation bar as `CustomFieldsListView` will create its own toolbar.
navigationController?.setNavigationBarHidden(true, animated: true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The animation causes unwanted jiggling between transitions, so setting animated to false might help.

For a smoother experience, please consider adding a separate type CustomFieldsListHostingController and set the navigation items for the Save and Add buttons in that controller if it's editable. That way you can reuse the back button and avoid having a thinner chevron icon on this screen compared to the product form screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Paolo also raised something similar here #14059 (review) and I'll quote my reply:

I agree with that, though, and in hindsight it would've been more consistent to copy Blaze's method of creating navigation coordinator (BlazeCampaignCreationCoordinator) and use UIKit navigation still. While I do find that the experience of setting up navigation using SwiftUI's NavigationStack to be more intuitive and concise, it's also unfortunate that connecting it with UIKit's navigation isn't as smooth as possible.

An improvement here would be to refactor the navigation to use coordinator. Given what's already set up, this would be a decently sized task, so let's keep this for now. In the meanwhile I'll add a bit more things like turning off animation and making the back icon bolder, to make the back navigation look less odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added cfd6c47 to:

  • remove animations
  • make the back button thicker (good catch on this! This is also done in other parts of the code that uses the chevron)

Copy link
Contributor Author

@hafizrahman hafizrahman Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something I'm not able to fix is that the placement of the back arrow has an extra leading padding in the list view, compared to the back button on the product details view or custom fields editor view:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-10-01.at.12.23.03.mp4

I tried a few things:

  • adding negative padding: this works but make the target area smaller and taps don't register on the left side
  • using .topBarLeading instead of .cancellationAction as placement
  • using ToolbarItemGroup

But none of them fixes the problem. It's odd because the back button generated automatically by SwiftUI (in the editor view) has the correct placement similar to UIKit's back button (in the product details view). It's just the the back button created with Toolbar that has a different leading padding. This feels like a SwiftUI bug to me, and as it's a bit odd but not breaking functionality, I think we can keep it like that for now.

navigationController?.pushViewController(customFieldsView, animated: true)

}
}

// MARK: Action - Show Product Reviews Settings
//
private extension ProductFormViewController {
Expand Down
7 changes: 6 additions & 1 deletion WooCommerce/WooCommerceTests/Mocks/MockFeatureFlagService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ struct MockFeatureFlagService: FeatureFlagService {
private let blazeEvergreenCampaigns: Bool
private let blazeCampaignObjective: Bool
private let revampedShippingLabelCreation: Bool
private let viewEditCustomFieldsInProductsAndOrders: Bool

init(isInboxOn: Bool = false,
isShowInboxCTAEnabled: Bool = false,
Expand All @@ -40,7 +41,8 @@ struct MockFeatureFlagService: FeatureFlagService {
googleAdsCampaignCreationOnWebView: Bool = false,
blazeEvergreenCampaigns: Bool = false,
blazeCampaignObjective: Bool = false,
revampedShippingLabelCreation: Bool = false) {
revampedShippingLabelCreation: Bool = false,
viewEditCustomFieldsInProductsAndOrders: Bool = false) {
self.isInboxOn = isInboxOn
self.isShowInboxCTAEnabled = isShowInboxCTAEnabled
self.isUpdateOrderOptimisticallyOn = isUpdateOrderOptimisticallyOn
Expand All @@ -60,6 +62,7 @@ struct MockFeatureFlagService: FeatureFlagService {
self.blazeEvergreenCampaigns = blazeEvergreenCampaigns
self.blazeCampaignObjective = blazeCampaignObjective
self.revampedShippingLabelCreation = revampedShippingLabelCreation
self.viewEditCustomFieldsInProductsAndOrders = viewEditCustomFieldsInProductsAndOrders
}

func isFeatureFlagEnabled(_ featureFlag: FeatureFlag) -> Bool {
Expand Down Expand Up @@ -102,6 +105,8 @@ struct MockFeatureFlagService: FeatureFlagService {
return blazeCampaignObjective
case .revampedShippingLabelCreation:
return revampedShippingLabelCreation
case .viewEditCustomFieldsInProductsAndOrders:
return viewEditCustomFieldsInProductsAndOrders
default:
return false
}
Expand Down
22 changes: 20 additions & 2 deletions ...ViewRelated/Products/Actions Factory/ProductFormActionsFactory+ProductCreationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,22 @@ final class ProductFormActionsFactory_ProductCreationTests: XCTestCase {
XCTAssertTrue(actions.contains(.productType(editable: false)))
}
}

func test_givenNewProductCreation_whenCreatingActions_thenCustomFieldsRowIsInvisible() {
// Given
let product = Product.fake()
let model = EditableProductModel(product: product)

// When
let featureFlagService = MockFeatureFlagService(viewEditCustomFieldsInProductsAndOrders: true)
let actions = Fixtures.actionsFactory(product: model, formType: .add, featureFlagService: featureFlagService).settingsSectionActions()
let bottomSheetActions = Fixtures.actionsFactory(product: model, formType: .add, featureFlagService: featureFlagService).bottomSheetActions()

// Then
XCTAssertFalse(actions.contains(.customFields))
XCTAssertFalse(bottomSheetActions.contains(.editCustomFields))

}
}

private extension ProductFormActionsFactory_ProductCreationTests {
Expand All @@ -64,12 +80,14 @@ private extension ProductFormActionsFactory_ProductCreationTests {
formType: ProductFormType,
addOnsFeatureEnabled: Bool = false,
isLinkedProductsPromoEnabled: Bool = false,
variationsPrice: ProductFormActionsFactory.VariationsPrice = .unknown) -> ProductFormActionsFactory {
variationsPrice: ProductFormActionsFactory.VariationsPrice = .unknown,
featureFlagService: MockFeatureFlagService = MockFeatureFlagService()) -> ProductFormActionsFactory {
ProductFormActionsFactory(product: product,
formType: formType,
addOnsFeatureEnabled: addOnsFeatureEnabled,
isLinkedProductsPromoEnabled: isLinkedProductsPromoEnabled,
variationsPrice: variationsPrice)
variationsPrice: variationsPrice,
featureFlagService: featureFlagService)
}
}
}
32 changes: 32 additions & 0 deletions ...ests/ViewRelated/Products/Actions Factory/ProductFormActionsFactory+VisibilityTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,35 @@ final class ProductFormActionsFactory_VisibilityTests: XCTestCase {
// Assert
XCTAssertFalse(factory.settingsSectionActions().contains(.downloadableFiles(editable: true)))
}

// MARK: - Custom fields

func test_givenExistingProductAndCustomFields_whenCreatingActions_thenCustomFieldsRowIsVisible() {
itsmeichigo marked this conversation as resolved.
Show resolved Hide resolved
// Arrange
let model = EditableProductModel(product: Fixtures.productWithCustomFields)

// Act
let featureFlagService = MockFeatureFlagService(viewEditCustomFieldsInProductsAndOrders: true)
let factory = ProductFormActionsFactory(product: model, formType: .edit, featureFlagService: featureFlagService)
itsmeichigo marked this conversation as resolved.
Show resolved Hide resolved

// Assert
XCTAssertTrue(factory.settingsSectionActions().contains(.customFields))
}


func test_givenExistingProductAndEmptyCustomFields_whenCreatingActions_thenCustomFieldsRowIsInvisible() {
// Arrange
let model = EditableProductModel(product: Fixtures.productWithNoCustomFields)

// Act
let featureFlagService = MockFeatureFlagService(viewEditCustomFieldsInProductsAndOrders: true)
let factory = ProductFormActionsFactory(product: model, formType: .edit, featureFlagService: featureFlagService)

// Assert
XCTAssertFalse(factory.settingsSectionActions().contains(.customFields))
XCTAssertTrue(factory.bottomSheetActions().contains(.editCustomFields))

}
}

private extension ProductFormActionsFactory_VisibilityTests {
Expand Down Expand Up @@ -194,5 +223,8 @@ private extension ProductFormActionsFactory_VisibilityTests {
static let downloadableProduct = Product.fake().copy(productTypeKey: ProductType.simple.rawValue, downloadable: true)
static let nonDownloadableProduct = downloadableProduct.copy(downloadable: false)

// Custom fields
static let productWithCustomFields = Product.fake().copy(customFields: [MetaData(metadataID: 1, key: "test", value: "value")])
static let productWithNoCustomFields = Product.fake().copy(customFields: [])
}
}
1 change: 0 additions & 1 deletion ...ooCommerceTests/ViewRelated/Products/Actions Factory/ProductFormActionsFactoryTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1002,7 +1002,6 @@ private extension ProductFormActionsFactoryTests {
formType: formType,
addOnsFeatureEnabled: addOnsFeatureEnabled,
isLinkedProductsPromoEnabled: isLinkedProductsPromoEnabled,
isCustomFieldsEnabled: isCustomFieldsEnabled,
variationsPrice: variationsPrice)
}
}
Expand Down
Loading