Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
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: 2 additions & 0 deletions Experiments/Experiments/DefaultFeatureFlagService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ public struct DefaultFeatureFlagService: FeatureFlagService {
return buildConfig == .localDeveloper || buildConfig == .alpha
case .productsOnboarding:
return buildConfig == .localDeveloper || buildConfig == .alpha
case .productsPreview:
return buildConfig == .localDeveloper || buildConfig == .alpha
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could use the same .productsOnboarding feature flag right?
When the other deliverables are done, we can just take them out of the feature flag check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, updated in edf3702

case .simplifiedLoginFlowI1:
return buildConfig == .localDeveloper || buildConfig == .alpha
default:
Expand Down
5 changes: 4 additions & 1 deletion Experiments/Experiments/FeatureFlag.swift
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,12 @@ public enum FeatureFlag: Int {
///
case productsOnboarding

/// Products Onboarding subproject: Products preview.
///
case productsPreview

/// Temporary feature flag for the simplified login flow.
/// TODO: replace with A/B testing.
///
case simplifiedLoginFlowI1

}
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,34 @@ final class ProductFormViewController<ViewModel: ProductFormViewModelProtocol>:
saveProduct(status: .draft)
}

// MARK: Product preview action handling

@objc private func saveDraftAndDisplayProductPreview() {
guard viewModel.canSaveAsDraft() || viewModel.hasUnsavedChanges() else {
displayProductPreview()
return
}

saveProduct(status: .draft) { [weak self] result in
if result.isSuccess {
self?.displayProductPreview()
}
}
}

private func displayProductPreview() {
var permalink = URLComponents(string: product.permalink)
var updatedQueryItems = permalink?.queryItems ?? []
updatedQueryItems.append(.init(name: "preview", value: "true"))
permalink?.queryItems = updatedQueryItems
guard let url = permalink?.url else {
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this fail? if it can, should we show some kind of toast?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The source string, product.permalink is non-optional property. But all URL manipulation tools in iOS SDK return optional URL result.
So I consider this one is safe and never reachable.

}

// TODO: Show authenticated WebView
WebviewHelper.launch(url, with: self)
}

// MARK: Navigation actions

@objc func closeNavigationBarButtonTapped() {
Expand Down Expand Up @@ -716,14 +744,14 @@ private extension ProductFormViewController {
// MARK: Navigation actions
//
private extension ProductFormViewController {
func saveProduct(status: ProductStatus? = nil) {
func saveProduct(status: ProductStatus? = nil, onCompletion: @escaping (Result<Void, ProductUpdateError>) -> Void = { _ in }) {
let productStatus = status ?? product.status
let messageType = viewModel.saveMessageType(for: productStatus)
showSavingProgress(messageType)
saveProductRemotely(status: status)
saveProductRemotely(status: status, onCompletion: onCompletion)
}

func saveProductRemotely(status: ProductStatus?) {
func saveProductRemotely(status: ProductStatus?, onCompletion: @escaping (Result<Void, ProductUpdateError>) -> Void = { _ in }) {
viewModel.saveProductRemotely(status: status) { [weak self] result in
switch result {
case .failure(let error):
Expand All @@ -732,6 +760,7 @@ private extension ProductFormViewController {
// Dismisses the in-progress UI then presents the error alert.
self?.navigationController?.dismiss(animated: true) {
self?.displayError(error: error)
onCompletion(.failure(error))
}
case .success:
// Dismisses the in-progress UI, then presents the confirmation alert.
Expand All @@ -741,6 +770,7 @@ private extension ProductFormViewController {
// Show linked products promo banner after product save
(self?.viewModel as? ProductFormViewModel)?.isLinkedProductsPromoEnabled = true
self?.reloadLinkedPromoCellAnimated()
onCompletion(.success(()))
}
}
}
Expand Down Expand Up @@ -908,6 +938,8 @@ private extension ProductFormViewController {
// Create action buttons based on view model
let rightBarButtonItems: [UIBarButtonItem] = viewModel.actionButtons.reversed().map { buttonType in
switch buttonType {
case .preview:
return createPreviewBarButtonItem()
case .publish:
return createPublishBarButtonItem()
case .save:
Expand All @@ -934,6 +966,10 @@ private extension ProductFormViewController {
return UIBarButtonItem(title: Localization.saveTitle, style: .done, target: self, action: #selector(saveProductAndLogEvent))
}

func createPreviewBarButtonItem() -> UIBarButtonItem {
return UIBarButtonItem(title: Localization.previewTitle, style: .done, target: self, action: #selector(saveDraftAndDisplayProductPreview))
}

func createMoreOptionsBarButtonItem() -> UIBarButtonItem {
let moreButton = UIBarButtonItem(image: .moreImage,
style: .plain,
Expand Down Expand Up @@ -1535,6 +1571,7 @@ private extension ProductFormViewController {
private enum Localization {
static let publishTitle = NSLocalizedString("Publish", comment: "Action for creating a new product remotely with a published status")
static let saveTitle = NSLocalizedString("Save", comment: "Action for saving a Product remotely")
static let previewTitle = NSLocalizedString("Preview", comment: "Action for previewing draft Product changes in the webview")
static let groupedProductsViewTitle = NSLocalizedString("Grouped Products",
comment: "Navigation bar title for editing linked products for a grouped product")
static let unnamedProduct = NSLocalizedString("Unnamed product",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,12 @@ final class ProductFormViewModel: ProductFormViewModelProtocol {
}
}()

// Preview existing drafts or new products, that can be saved as a draft
if ServiceLocator.featureFlagService.isFeatureFlagEnabled(.productsPreview),
(canSaveAsDraft() || productModel.status == .draft) {
buttons.insert(.preview, at: 0)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we could move this logic to the switch above, or would things get more complicated? The switch is useful for us to not miss any case or combination.

Also, do you think this is worth unit testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic doesn't really fit in switch cases + it requires FeatureFlag check. If we add it to switch above, it'll explode into more cases and it'll be hard to understand when "preview" flow is applied.
I've updated it with comments in b6ea899, c1e812f.

Unit tests added in d69489c.


// Add more button if needed
if shouldShowMoreOptionsMenu() {
buttons.append(.more)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ enum ProductFormType {

/// The type of action that can be performed in the product.
enum ActionButtonType {
case preview
case publish
case save
case more
Expand Down