Merge branch 'develop' into issue/1713-image-upload-ux

Author: jaclync

Networking/Networking/Model/Media/Media.swift

@@ -1,8 +1,6 @@
 /// WordPress Site Media
-/// Note: this is an `NSObject` class instead of a struct because it has to conform to `WPMediaAsset` protocol defined in
-/// Objective-C.
 ///
-public class Media: NSObject, Decodable {
+public struct Media {
     public let mediaID: Int64
     public let date: Date    // gmt iso8601
     public let fileExtension: String
@@ -37,10 +35,12 @@ public class Media: NSObject, Decodable {
         self.height = height
         self.width = width
     }
+}
 
+extension Media: Decodable {
     /// Decodable Initializer.
     ///
-    required public convenience init(from decoder: Decoder) throws {
+    public init(from decoder: Decoder) throws {
         let container = try decoder.container(keyedBy: CodingKeys.self)
 
         let mediaID = try container.decode(Int64.self, forKey: .mediaID)

RELEASE-NOTES.txt

@@ -1,6 +1,7 @@
 4.1
 -----
 - Fix an intermittent crash when downloading Orders
+- The Photo Library permission alert shouldn't be prompted when opening the readonly product details or edit product for simple products, which is reproducible on iOS 11 or 12 devices. (The permission is only triggered when uploading images in Zendesk support or in debug builds with Products M2 enabled.)
 - [internal] Updated the empty search result views for Products and Orders. https://git.io/Jvdap
 
 

WooCommerce/Classes/Extensions/UINavigationController+Woo.swift

@@ -33,7 +33,7 @@ extension UINavigationController {
 
 // MARK: - Handle UINavigationBar's 'Back' button action
 //
-protocol  UINavigationBarBackButtonHandler {
+protocol UINavigationBarBackButtonHandler {
 
     /// Should block the 'Back' button action
     ///
@@ -49,6 +49,9 @@ extension UIViewController: UINavigationBarBackButtonHandler {
 }
 
 extension UINavigationController: UINavigationBarDelegate {
+
+    // This delegate method is not called on the simulator or device running iOS 13.4 from Xcode.
+    // You need to use a release build.
     public func navigationBar(_ navigationBar: UINavigationBar, shouldPop item: UINavigationItem) -> Bool {
         guard let items = navigationBar.items else {
             return false

WooCommerce/Classes/Tools/Currency/CurrencyFormatter.swift

@@ -14,12 +14,13 @@ public class CurrencyFormatter {
     /// Returns a decimal value from a given string.
     /// - Parameters:
     ///   - stringValue: the string received from the API
+    ///   - locale: the locale that the currency string is based on.
     ///
-    func convertToDecimal(from stringValue: String) -> NSDecimalNumber? {
+    func convertToDecimal(from stringValue: String, locale: Locale = .current) -> NSDecimalNumber? {
 
         // NSDecimalNumber use by default the local decimal separator to evaluate a decimal amount.
         // We substitute the current decimal separator with the locale decimal separator.
-        let localeDecimalSeparator = Locale.current.decimalSeparator ?? currencySettings.decimalSeparator
+        let localeDecimalSeparator = locale.decimalSeparator ?? currencySettings.decimalSeparator
         var newStringValue = stringValue.replacingOccurrences(of: ",", with: localeDecimalSeparator)
         newStringValue = newStringValue.replacingOccurrences(of: ".", with: localeDecimalSeparator)
 
@@ -28,7 +29,7 @@ public class CurrencyFormatter {
         let unit = currencySettings.symbol(from: currencyCode)
         newStringValue = newStringValue.replacingOccurrences(of: unit, with: "")
 
-        let decimalValue = NSDecimalNumber(string: newStringValue, locale: Locale.current)
+        let decimalValue = NSDecimalNumber(string: newStringValue, locale: locale)
 
         guard decimalValue != NSDecimalNumber.notANumber else {
             DDLogError("Error: string input is not a number: \(stringValue)")
@@ -72,21 +73,22 @@ public class CurrencyFormatter {
 
     /// Returns a string that displays the amount using all of the specified currency settings
     /// - Parameters:
-    ///     - amount: a formatted string, preferably converted using `localize(_:in:with:including:)`.
+    ///     - stringValue: a formatted string, preferably converted using `localize(_:in:with:including:)`.
     ///     - position: the currency position enum, either right, left, right_space, or left_space.
     ///     - symbol: the currency symbol as a string, to be used with the amount.
+    ///     - isNegative: whether the value is negative or not.
+    ///     - locale: the locale that is used to format the currency amount string.
     ///
-    func formatCurrency(using stringValue: String, at position: CurrencySettings.CurrencyPosition, with symbol: String, isNegative: Bool) -> String {
+    func formatCurrency(using amount: String,
+                        at position: CurrencySettings.CurrencyPosition,
+                        with symbol: String,
+                        isNegative: Bool,
+                        locale: Locale = .current) -> String {
         let space = "\u{00a0}" // unicode equivalent of  
         let negative = isNegative ? "-" : ""
 
         // We're relying on the phone's Locale to assist with language direction
-        let current = Locale.current as NSLocale
-        let languageCode = current.object(forKey: NSLocale.Key.languageCode) as? String
-
-        // Remove all occurences of the minus sign from the string amount.
-        // We want to position the minus sign manually.
-        let amount = stringValue.replacingOccurrences(of: "-", with: "")
+        let languageCode = locale.languageCode
 
         // Detect the language direction
         var languageDirection: Locale.LanguageDirection = .unknown
@@ -126,23 +128,25 @@ public class CurrencyFormatter {
     /// - Parameters:
     ///     - amount: a raw string representation of the amount, from the API, with no formatting applied. e.g. "19.87"
     ///     - currency: a 3-letter country code for currencies that are supported in the API. e.g. "USD"
+    ///     - locale: the locale that is used to format the currency amount string.
     ///
-    func formatAmount(_ stringAmount: String, with currency: String = CurrencySettings.shared.currencyCode.rawValue) -> String? {
-        guard let decimalAmount = convertToDecimal(from: stringAmount) else {
+    func formatAmount(_ amount: String, with currency: String? = nil, locale: Locale = .current) -> String? {
+        guard let decimalAmount = convertToDecimal(from: amount, locale: locale) else {
             return nil
         }
 
-        return formatAmount(decimalAmount, with: currency)
+        return formatAmount(decimalAmount, with: currency ?? currencySettings.currencyCode.rawValue, locale: locale)
     }
 
 
     /// Formats the provided `amount` param into a human readable value and applies the currency option
     /// settings for the given currency.
     ///
     /// - Parameters:
-    ///   - amount: a raw string representation of the amount, from the API, with no formatting applied. e.g. "19.87"
+    ///   - stringAmount: a raw string representation of the amount, from the API, with no formatting applied. e.g. "19.87"
     ///   - currency: a 3-letter country code for currencies that are supported in the API. e.g. "USD"
     ///   - roundSmallNumbers: if `true`, small numbers are rounded, if `false`, no rounding occurs (defaults to true)
+    ///   - locale: the locale that is used to format the currency amount string.
     /// - Returns: a formatted amount string
     ///
     /// For our purposes here, a "small number" is anything in-between -1000 and 1000 (exclusive).
@@ -167,17 +171,21 @@ public class CurrencyFormatter {
     ///  - 5800199.56 becomes "$5.8m"
     ///
     func formatHumanReadableAmount(_ stringAmount: String,
-                                   with currency: String = CurrencySettings.shared.currencyCode.rawValue,
-                                   roundSmallNumbers: Bool = true) -> String? {
-        guard let amount = convertToDecimal(from: stringAmount) else {
+                                   with currency: String? = nil,
+                                   roundSmallNumbers: Bool = true,
+                                   locale: Locale = .current) -> String? {
+        guard let amount = convertToDecimal(from: stringAmount, locale: locale) else {
+            assertionFailure("Cannot convert the amount \"\(stringAmount)\" to decimal value with locale \(locale.identifier)")
             return nil
         }
 
-        let humanReadableAmount = amount.humanReadableString(roundSmallNumbers: roundSmallNumbers)
+        let currency = currency ?? currencySettings.currencyCode.rawValue
+
+        let humanReadableAmount = amount.abs().humanReadableString(roundSmallNumbers: roundSmallNumbers)
         if humanReadableAmount == amount.stringValue, roundSmallNumbers == false {
             // The human readable version of amount is the same as the converted param value which means this is a "small"
             // number — format it normally *without* rounding.
-            return formatAmount(amount, with: currency)
+            return formatAmount(amount, with: currency, locale: locale)
         }
 
         // If we are here, the human readable version of the amount param is a "large" number *OR* a small number but rounding has been requested,
@@ -190,15 +198,18 @@ public class CurrencyFormatter {
         return formatCurrency(using: humanReadableAmount,
                               at: position,
                               with: symbol,
-                              isNegative: isNegative)
+                              isNegative: isNegative,
+                              locale: locale)
     }
 
     /// Applies currency option settings to the amount for the given currency.
     /// - Parameters:
     ///     - amount: a NSDecimalNumber representation of the amount, from the API, with no formatting applied. e.g. "19.87"
     ///     - currency: a 3-letter country code for currencies that are supported in the API. e.g. "USD"
+    ///     - locale: the locale that is used to format the currency amount string.
     ///
-    func formatAmount(_ decimalAmount: NSDecimalNumber, with currency: String = CurrencySettings.shared.currencyCode.rawValue) -> String? {
+    func formatAmount(_ amount: NSDecimalNumber, with currency: String? = nil, locale: Locale = .current) -> String? {
+        let currency = currency ?? currencySettings.currencyCode.rawValue
         // Get the currency code
         let code = CurrencySettings.CurrencyCode(rawValue: currency) ?? currencySettings.currencyCode
         // Grab the read-only currency options. These are set by the user in Site > Settings.
@@ -210,7 +221,7 @@ public class CurrencyFormatter {
 
         // Put all the pieces of user preferences on currency formatting together
         // and spit out a string that has the formatted amount.
-        let localized = localize(decimalAmount,
+        let localized = localize(amount,
                                  with: separator,
                                  in: numberOfDecimals,
                                  including: thousandSeparator)
@@ -224,7 +235,8 @@ public class CurrencyFormatter {
         let formattedAmount = formatCurrency(using: localizedAmount,
                                              at: position,
                                              with: symbol,
-                                             isNegative: decimalAmount.isNegative())
+                                             isNegative: amount.isNegative(),
+                                             locale: locale)
 
         return formattedAmount
     }

WooCommerce/Classes/Tools/ImageService/DefaultImageService.swift

@@ -14,11 +14,14 @@ struct DefaultImageService: ImageService {
     private let imageCache: ImageCache
 
     private var defaultOptions: KingfisherOptionsInfo {
-        return [.targetCache(imageCache), .downloader(imageDownloader)]
+        if let imageDownloader = imageDownloader as? Kingfisher.ImageDownloader {
+            return [.targetCache(imageCache), .downloader(imageDownloader)]
+        }
+        return [.targetCache(imageCache)]
     }
 
     init(imageCache: ImageCache = ImageCache.default,
-         imageDownloader: ImageDownloader = ImageDownloader.default) {
+         imageDownloader: ImageDownloader = Kingfisher.ImageDownloader.default) {
         self.imageCache = imageCache
         self.imageDownloader = imageDownloader
     }
@@ -35,21 +38,18 @@ struct DefaultImageService: ImageService {
         }
     }
 
-    func downloadImage(with url: URL, shouldCacheImage: Bool, completion: ImageDownloadCompletion?) {
-        imageDownloader.downloadImage(with: url,
-                                      options: nil) { result in
-                                        switch result {
-                                        case .success(let imageResult):
-                                            let image = imageResult.image
-
-                                            if shouldCacheImage {
-                                                self.imageCache.store(image, forKey: url.imageCacheKey)
-                                            }
+    func downloadImage(with url: URL, shouldCacheImage: Bool, completion: ImageDownloadCompletion?) -> ImageDownloadTask? {
+        return imageDownloader.downloadImage(with: url) { result in
+                                                switch result {
+                                                case .success(let image):
+                                                    if shouldCacheImage {
+                                                        self.imageCache.store(image, forKey: url.imageCacheKey)
+                                                    }
 
-                                            completion?(image, nil)
-                                        case .failure(let kingfisherError):
-                                            completion?(nil, .other(error: kingfisherError))
-                                        }
+                                                    completion?(image, nil)
+                                                case .failure(let kingfisherError):
+                                                    completion?(nil, .other(error: kingfisherError))
+                                                }
         }
     }
 

WooCommerce/Classes/Tools/ImageService/ImageDownloader.swift

@@ -0,0 +1,19 @@
+import UIKit
+
+/// Used for any activity or action that may be canceled.
+/// TODO: `Cancellable` is also available starting iOS 13, please delete when the project is iOS 13+.
+///
+protocol Cancellable {
+    func cancel()
+}
+
+/// A task that downloads an image asynchronously.
+///
+protocol ImageDownloadTask: Cancellable {}
+
+/// Performs tasks related to downloading an image.
+///
+protocol ImageDownloader {
+    func downloadImage(with url: URL,
+                       onCompletion: ((Result<UIImage, Error>) -> Void)?) -> ImageDownloadTask?
+}

WooCommerce/Classes/Tools/ImageService/ImageService.swift

@@ -19,7 +19,7 @@ protocol ImageService {
     ///   - url: url of the image.
     ///   - shouldCacheImage: whether the downloaded image should be stored in the cache for faster access in the future.
     ///   - completion: called when the image download completes.
-    func downloadImage(with url: URL, shouldCacheImage: Bool, completion: ImageDownloadCompletion?)
+    func downloadImage(with url: URL, shouldCacheImage: Bool, completion: ImageDownloadCompletion?) -> ImageDownloadTask?
 
     /// Downloads and caches an image for a `UIImageView` given a URL and a placeholder.
     /// - Parameters:

WooCommerce/Classes/Tools/ImageService/KingfisherImageDownloader+ImageDownloadable.swift

@@ -0,0 +1,16 @@
+import Kingfisher
+
+extension Kingfisher.DownloadTask: ImageDownloadTask {}
+
+extension Kingfisher.ImageDownloader: ImageDownloader {
+    func downloadImage(with url: URL, onCompletion: ((Result<UIImage, Error>) -> Void)?) -> ImageDownloadTask? {
+        return downloadImage(with: url, options: nil) { result in
+            switch result {
+            case .success(let imageResult):
+                onCompletion?(.success(imageResult.image))
+            case .failure(let kingfisherError):
+                onCompletion?(.failure(kingfisherError))
+            }
+        }
+    }
+}

WooCommerce/Classes/ViewRelated/Editor/AztecTextViewAttachmentHandler.swift

@@ -2,6 +2,13 @@ import Aztec
 
 /// Implements Aztec's `TextViewAttachmentDelegate` without media support.
 final class AztecTextViewAttachmentHandler: TextViewAttachmentDelegate {
+    private var activeImageTasks = [ImageDownloadTask]()
+
+    deinit {
+        activeImageTasks.forEach { $0.cancel() }
+        activeImageTasks.removeAll()
+    }
+
     func textView(_ textView: TextView,
                   attachment: NSTextAttachment,
                   imageAt url: URL,
@@ -17,13 +24,16 @@ final class AztecTextViewAttachmentHandler: TextViewAttachmentDelegate {
                 }
             }
 
-            imageService.downloadImage(with: url, shouldCacheImage: true) { image, error in
+            let task = imageService.downloadImage(with: url, shouldCacheImage: true) { image, error in
                 guard let image = image else {
                     failure()
                     return
                 }
                 success(image)
             }
+            if let task = task {
+                activeImageTasks.append(task)
+            }
         default:
             return
         }

WooCommerce/Classes/ViewRelated/Products/Cells/Product Images/Collection View Cells/ProductImageCollectionViewCell.swift

@@ -4,6 +4,8 @@ final class ProductImageCollectionViewCell: UICollectionViewCell {
 
     @IBOutlet weak var imageView: UIImageView!
 
+    var cancellableTask: Cancellable?
+
     override func awakeFromNib() {
         super.awakeFromNib()
         configureBackground()
@@ -16,6 +18,11 @@ final class ProductImageCollectionViewCell: UICollectionViewCell {
         // Border color is not automatically updated on trait collection changes and thus manually updated here.
         contentView.layer.borderColor = Colors.borderColor.cgColor
     }
+
+    override func prepareForReuse() {
+        cancellableTask?.cancel()
+        cancellableTask = nil
+    }
 }
 
 /// Private Methods