Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,21 @@ import Yosemite
///
struct ProductVariationGenerator {

/// Group a colection of attribute options.
/// Group a collection of attribute options.
/// EG: [Size: Large, Color: Black, Fabric: Cotton]
///
private struct Combination: Hashable {
private struct Combination: Hashable, Equatable {
let options: [Option]

static func == (lhs: Combination, rhs: Combination) -> Bool {
Set(lhs.options) == Set(rhs.options)
}
}

/// Represents an attribute option.
/// EG: Size: Large
///
private struct Option: Hashable {
private struct Option: Hashable, Equatable {
let attributeID: Int64
let attributeName: String
let value: String
Expand All @@ -33,7 +37,7 @@ struct ProductVariationGenerator {
/// Generates all posible combination for a product attributes.
///
private static func getCombinations(from product: Product) -> [Combination] {
// Iterates through attributes while eceiving the previous combinations list.
// Iterates through attributes while receiving the previous combinations list.
product.attributes.reduce([Combination(options: [])]) { combinations, attribute in
combinations.flatMap { combination in
// When receiving a previous combination list, we add each attribute to each previous combination util we finish with them.
Expand All @@ -49,17 +53,17 @@ struct ProductVariationGenerator {
private static func filterExistingCombinations(_ combinations: [Combination], existing variations: [ProductVariation]) -> [Combination] {
// Convert variations into combinations
let existingCombinations = variations.map { existingVariation in
let options = existingVariation.attributes.map { attibute in
Option(attributeID: attibute.id, attributeName: attibute.name, value: attibute.option)
let options = existingVariation.attributes.map { attribute in
Option(attributeID: attribute.id, attributeName: attribute.name, value: attribute.option)
}
return Combination(options: options)
}

// Filter existing combinations.
let existingSet = Set(existingCombinations)
return combinations.filter { combination in
!existingSet.contains(combination)
let unique = combinations.filter { combination in
!existingCombinations.contains(combination)
}
return unique
}

/// Convert the provided combinations into `[CreateProductVariation]` types that are consumed by our Yosemite stores.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,13 @@ private extension ProductVariationsViewController {
let bottomSheetPresenter = BottomSheetListSelectorPresenter(viewProperties: viewProperties, command: command)
bottomSheetPresenter.show(from: self, sourceView: topStackView)
}

/// Informs the merchant about errors that happen during the variation generation
///
private func presentGenerationError(_ error: ProductVariationsViewModel.GenerationError) {
let notice = Notice(title: error.errorTitle, message: error.errorDescription)
noticePresenter.enqueue(notice: notice)
}
}

// MARK: - Placeholders
Expand Down Expand Up @@ -695,10 +702,17 @@ extension ProductVariationsViewController: SyncingCoordinatorDelegate {
/// Generates all possible variations for the product attibutes.
///
private func generateAllVariations() {
viewModel.generateAllVariations(for: product)
viewModel.generateAllVariations(for: product) { [weak self] result in
guard let self else { return }
switch result {
case .success:
break
case .failure(let error):
self.presentGenerationError(error)
}
}
// TODO:
// - Show Loading Indicator
// - Alert if there are more than 100 variations to create
// - Hide Loading Indicator
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ final class ProductVariationsViewModel {

/// Generates all missing variations for a product. Up to 100 variations.
///
func generateAllVariations(for product: Product) {
func generateAllVariations(for product: Product, onCompletion: @escaping (Result<Void, GenerationError>) -> Void) {
let action = ProductVariationAction.synchronizeAllProductVariations(siteID: product.siteID, productID: product.productID) { result in
// TODO: Fetch this via a results controller
let existingVariations = ServiceLocator.storageManager.viewStorage.loadProductVariations(siteID: product.siteID, productID: product.productID)?
Expand All @@ -39,6 +39,13 @@ final class ProductVariationsViewModel {
let variationsToGenerate = ProductVariationGenerator.generateVariations(for: product, excluding: existingVariations)
print("Variations to Generate: \(variationsToGenerate.count)")

// Guard for 100 variation limit
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we guard for this overflow early? So we won't actually create a lot of objects before failing? We should be able to quickly count possible combinations by multiplying the amount of options for each attribute.

But I'm not sure if we can easily subtract a number of existing variations - there may be duplicated combinations. Can we subtract just total number of existing variations for pre-generation guard (for a worst case)? And if it <100, we can proceed to existing check to make additional verification of combinations.

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 don't think we will hit a performance problem here right now. The plan is to eventually ask core to generate variations so probably not worth spending extra time on performance unless we notice something significant!

guard variationsToGenerate.count <= 100 else {
return onCompletion(.failure(.tooManyVariations(variationCount: variationsToGenerate.count)))
}

onCompletion(.success(()))

}
stores.dispatch(action)

Expand Down Expand Up @@ -71,3 +78,29 @@ extension ProductVariationsViewModel {
product.attributesForVariations.isEmpty
}
}

extension ProductVariationsViewModel {
/// Type to represent known generation errors
///
enum GenerationError: LocalizedError, Equatable {
case tooManyVariations(variationCount: Int)

var errorTitle: String {
switch self {
case .tooManyVariations:
return NSLocalizedString("Generation limit exceeded", comment: "Error title for for when there are too many variations to generate.")
}
}

var errorDescription: String? {
switch self {
case .tooManyVariations(let variationCount):
let format = NSLocalizedString(
"Currently creation is supported for 100 variations maximum. Generating variations for this product would create %d variations.",
comment: "Error description for when there are too many variations to generate."
)
return String.localizedStringWithFormat(format, variationCount)
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -114,4 +114,38 @@ final class ProductVariationsViewModelTests: XCTestCase {
XCTAssertTrue(product.existsRemotely)
XCTAssertEqual(viewModel.formType, .readonly)
}

func test_trying_to_generate_more_than_100_variations_will_return_error() {
// Given
let product = Product.fake().copy(attributes: [
ProductAttribute.fake().copy(attributeID: 1, name: "Size", options: ["XS", "S", "M", "L", "XL"]),
ProductAttribute.fake().copy(attributeID: 2, name: "Color", options: ["Red", "Green", "Blue", "White", "Black"]),
ProductAttribute.fake().copy(attributeID: 3, name: "Fabric", options: ["Cotton", "Nylon", "Polyester", "Silk", "Linen"]),
])

let stores = MockStoresManager(sessionManager: SessionManager.makeForTesting())
stores.whenReceivingAction(ofType: ProductVariationAction.self) { action in
switch action {
case .synchronizeAllProductVariations(_, _, let onCompletion):
onCompletion(.success(()))
default:
break
}
}

let viewModel = ProductVariationsViewModel(stores: stores, formType: .edit)

// When
let error = waitFor { promise in
viewModel.generateAllVariations(for: product) { result in
if case let .failure(error) = result {
promise(error)
}
}
}

// Then
XCTAssertEqual(error, .tooManyVariations(variationCount: 125))

}
}