Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
21 changes: 14 additions & 7 deletions Modules/Sources/Networking/Remote/ProductsRemote.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ public protocol ProductsRemoteProtocol {
excludedProductIDs: [Int64]) async throws -> [Product]
func searchProducts(for siteID: Int64,
keyword: String,
searchFields: [ProductSearchField],
pageNumber: Int,
pageSize: Int,
stockStatus: ProductStockStatus?,
Expand Down Expand Up @@ -313,7 +314,11 @@ public final class ProductsRemote: Remote, ProductsRemoteProtocol {
parameters.updateValue(query, forKey: ParameterKey.searchNameOrSKU)

// Takes precedence over `search_name_or_sku` from WC 10.1+ and is combined with `search` value
parameters.updateValue([SearchField.name, SearchField.sku, SearchField.globalUniqueID], forKey: ParameterKey.searchFields)
parameters.updateValue([
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you've only updated the mapping here, but the documentation for this function seems to lag, as it still says it searches in "name, description, and short_description". Could you please update it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 1414442.

ProductSearchField.name.rawValue,
ProductSearchField.sku.rawValue,
ProductSearchField.globalUniqueID.rawValue
], forKey: ParameterKey.searchFields)

return try await makePagedPointOfSaleProductsRequest(
for: siteID,
Expand Down Expand Up @@ -429,6 +434,7 @@ public final class ProductsRemote: Remote, ProductsRemoteProtocol {
///
public func searchProducts(for siteID: Int64,
keyword: String,
searchFields: [ProductSearchField],
pageNumber: Int,
pageSize: Int,
stockStatus: ProductStockStatus? = nil,
Expand All @@ -447,10 +453,11 @@ public final class ProductsRemote: Remote, ProductsRemoteProtocol {
ParameterKey.exclude: stringOfExcludedProductIDs
].filter({ $0.value.isEmpty == false })

let parameters = [
let parameters: [String: Any] = [
ParameterKey.page: String(pageNumber),
ParameterKey.perPage: String(pageSize),
ParameterKey.search: keyword,
ParameterKey.searchFields: searchFields.map { $0.rawValue },
ParameterKey.exclude: stringOfExcludedProductIDs,
ParameterKey.contextKey: Default.context
].merging(filterParameters, uniquingKeysWith: { (first, _) in first })
Expand Down Expand Up @@ -774,12 +781,12 @@ public extension ProductsRemote {
static let productSegment = "product"
static let itemsSold = "items_sold"
}
}

private enum SearchField {
static let name = "name"
static let sku = "sku"
static let globalUniqueID = "global_unique_id"
}
public enum ProductSearchField: String {
case name
case sku
case globalUniqueID = "global_unique_id"
}

private extension ProductsRemote {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,14 @@
public enum ProductSearchFilter: String, Equatable, CaseIterable {
/// Search for all products based on the keyword.
case all
/// Search for products that match the name field.
case name
/// Search for products that match the SKU field.
case sku

/// Options for searching in product selector view.
/// `name` is omitted as it's used for bookable product filters only so far.
public static var productSelectorOptions: [ProductSearchFilter] {
[.all, .sku]
}
}
9 changes: 8 additions & 1 deletion Modules/Sources/Yosemite/Stores/ProductStore.swift
Original file line number Diff line number Diff line change
Expand Up @@ -235,9 +235,16 @@ private extension ProductStore {
do {
let products: [Product]
switch filter {
case .all:
case .all, .name:
let searchFields: [ProductSearchField] = {
if filter == .name {
Copy link
Contributor

Choose a reason for hiding this comment

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

This extra layer of if makes this a bit hard to read. I'd rather extract try await remote.searchProducts(for: siteID, .... to a helper functuion and do something like

switch filter {
case .all:
    products = try await searchByKeyword(searchFields: [])

case .name:
    products = try await searchByKeyword(searchFields: [.name])

case .sku:
    products = try await remote.searchProductsBySKU(
        for: siteID,
        keyword: keyword,
        pageNumber: pageNumber,
        pageSize: pageSize
    )
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 26476a9.

return [.name]
}
return []
}()
products = try await remote.searchProducts(for: siteID,
keyword: keyword,
searchFields: searchFields,
pageNumber: pageNumber,
pageSize: pageSize,
stockStatus: stockStatus,
Expand Down
22 changes: 22 additions & 0 deletions Modules/Tests/NetworkingTests/Remote/ProductsRemoteTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,7 @@ final class ProductsRemoteTests: XCTestCase {
// When
let products = try await remote.searchProducts(for: sampleSiteID,
keyword: "photo",
searchFields: [],
pageNumber: 0,
pageSize: 100)

Expand All @@ -475,6 +476,7 @@ final class ProductsRemoteTests: XCTestCase {
do {
_ = try await remote.searchProducts(for: sampleSiteID,
keyword: String(),
searchFields: [],
pageNumber: 0,
pageSize: 100)
XCTFail("Expected error to be thrown")
Expand All @@ -483,6 +485,26 @@ final class ProductsRemoteTests: XCTestCase {
}
}

/// Verifies that searchProducts with name search field includes the search_fields param in network request.
///
func test_searchProducts_with_name_search_field_includes_search_fields_param_in_network_request() async throws {
// Given
let remote = ProductsRemote(network: network)
network.simulateResponse(requestUrlSuffix: "products", filename: "products-search-photo")

// When
_ = try await remote.searchProducts(for: sampleSiteID,
keyword: "test",
searchFields: [.name],
pageNumber: 0,
pageSize: 100)

// Then
let queryParameters = try XCTUnwrap(network.queryParameters)
let expectedParam = "search_fields=[\"name\"]"
XCTAssertTrue(queryParameters.contains(expectedParam), "Expected to have param: \(expectedParam)")
}

// MARK: - Search Products by SKU

func test_searchProductsBySKU_properly_returns_parsed_products() async throws {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ extension MockProductsRemote: ProductsRemoteProtocol {

func searchProducts(for siteID: Int64,
keyword: String,
searchFields: [ProductSearchField],
pageNumber: Int,
pageSize: Int,
stockStatus: ProductStockStatus?,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ struct BookableProductListSyncable: ListSyncable {
ProductAction.searchProducts(
siteID: siteID,
keyword: keyword,
filter: .name,
pageNumber: pageNumber,
pageSize: pageSize,
productType: .booking,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ private extension ProductSelectorView {
.submitLabel(.done)
.accessibilityIdentifier("product-selector-search-bar")
Picker(selection: $viewModel.productSearchFilter, label: EmptyView()) {
ForEach(ProductSearchFilter.allCases, id: \.self) { option in Text(option.title) }
ForEach(ProductSearchFilter.productSelectorOptions, id: \.self) { option in Text(option.title) }
}
.if(geometry.size.width <= Constants.headerSearchRowWidth) { $0.pickerStyle(.menu) }
.if(geometry.size.width > Constants.headerSearchRowWidth) { $0.pickerStyle(.segmented) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,22 +200,27 @@ private extension ProductSearchUICommand {
}

extension ProductSearchFilter {
/// The title of the option on the picker of product selector view.
var title: String {
switch self {
case .all:
return NSLocalizedString("All Products", comment: "Title of the product search filter to search for all products.")
case .sku:
return NSLocalizedString("SKU", comment: "Title of the product search filter to search for products that match the SKU.")
case .name:
fatalError("This option is not supported on the product selector UI")
}
}

/// The value that is set in the analytics event property.
/// The value that is set in the analytics event property when selecting the option on the product selector view.
var analyticsValue: String {
switch self {
case .all:
return "all"
case .sku:
return "sku"
case .name:
fatalError("This option is not supported on the product selector UI")
}
}
}