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

Add missing logs to ProductsFetcherSK2 #1780

Merged
merged 11 commits into from
Aug 1, 2022
Merged

Add missing logs to ProductsFetcherSK2 #1780

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
55a91d0
Add logs for parity with ProductsFetcherSK1
Jul 27, 2022
31b7005
add missing log about cached products
Jul 28, 2022
965d85a
Make products request strings storefront-agnostic
Jul 28, 2022
ac73aae
pull skproducts request string to storekit strings
Jul 28, 2022
7713332
Fix for lint, add two more missing logs for sk2, add comments for dis…
Jul 28, 2022
8eff1fd
lint fix
Jul 28, 2022
c7c6f99
Merge branch 'main' into sk2-products-fetcher-logs
Jul 28, 2022
20abf07
Merge branch 'main' into sk2-products-fetcher-logs
Aug 1, 2022
159a388
Update last couple skproduct-specific strings, remove duplicate logs
Aug 1, 2022
8109ee8
Fix casing
Aug 1, 2022
9fcbd95
quick string updates
Aug 1, 2022
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
5 changes: 0 additions & 5 deletions Sources/Logging/Strings/OfferingStrings.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ enum OfferingStrings {
case fetching_offerings_error(error: String)
case found_existing_product_request(identifiers: Set<String>)
case no_cached_offerings_fetching_from_network
case no_cached_requests_and_products_starting_skproduct_request(identifiers: Set<String>)
case offerings_stale_updated_from_network
case offerings_stale_updating_in_background
case offerings_stale_updating_in_foreground
Expand Down Expand Up @@ -62,10 +61,6 @@ extension OfferingStrings: CustomStringConvertible {
case .no_cached_offerings_fetching_from_network:
return "No cached Offerings, fetching from network"

case .no_cached_requests_and_products_starting_skproduct_request(let identifiers):
return "No existing requests and " +
"products not cached, starting SKProducts request for: \(identifiers)"

case .offerings_stale_updated_from_network:
return "Offerings updated from network."

Expand Down
22 changes: 14 additions & 8 deletions Sources/Logging/Strings/StoreKitStrings.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@ enum StoreKitStrings {

case skrequest_failed(error: Error)

case skproductsrequest_failed(error: Error)
case store_products_request_failed(error: Error)

case skproductsrequest_timed_out(after: Int)

case skproductsrequest_finished
case store_product_request_did_finish

case skproductsrequest_received_response
case store_product_request_received_response

case skunknown_payment_mode(String)

Expand All @@ -36,6 +36,8 @@ enum StoreKitStrings {

case sk1_discount_missing_locale

case no_cached_requests_and_products_starting_skproduct_request(identifiers: Set<String>)

}

extension StoreKitStrings: CustomStringConvertible {
Expand All @@ -46,20 +48,20 @@ extension StoreKitStrings: CustomStringConvertible {
case .skrequest_failed(let error):
return "SKRequest failed: \(error.localizedDescription)"

case .skproductsrequest_failed(let error):
return "SKProductsRequest failed! error: \(error.localizedDescription)"
case .store_products_request_failed(let error):
return "Store products request failed! error: \(error.localizedDescription)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
return "Store products request failed! error: \(error.localizedDescription)"
return "Store products request failed! Error: \(error.localizedDescription)"


case .skproductsrequest_timed_out(let afterTimeInSeconds):
return "SKProductsRequest took longer than \(afterTimeInSeconds) seconds, " +
"cancelling request and returning an empty set. This seems to be an App Store quirk. " +
"If this is happening to you consistently, you might want to try using a new Sandbox account. " +
"More information: https://rev.cat/skproductsrequest-hangs"

case .skproductsrequest_finished:
case .store_product_request_did_finish:
return "SKProductsRequest did finish"
Copy link
Contributor

Choose a reason for hiding this comment

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

This can change as well:

Suggested change
return "SKProductsRequest did finish"
return "Store product request did finish"


case .skproductsrequest_received_response:
return "SKProductsRequest request received response"
case .store_product_request_received_response:
return "Store products request request received response"

case let .skunknown_payment_mode(name):
return "Unrecognized PaymentMode: \(name)"
Expand All @@ -77,6 +79,10 @@ extension StoreKitStrings: CustomStringConvertible {
case .sk1_discount_missing_locale:
return "There is an issue with the App Store, this SKProductDiscount is missing a Locale - " +
"The current device Locale will be used instead."

case .no_cached_requests_and_products_starting_skproduct_request(let identifiers):
return "No existing requests and " +
"products not cached, starting SKProducts request for: \(identifiers)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, you're still using this in SK2, maybe change it to Store product request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was leaving this one in case we decided to cache requests... might be worth a different string until we decide to cache

Copy link
Contributor

Choose a reason for hiding this comment

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

We cache products, just not concurrent requests, which might be good enough?

}
}

Expand Down
13 changes: 9 additions & 4 deletions Sources/Purchasing/ProductsManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,10 @@ private extension ProductsManager {
_ = Task<Void, Never> {
do {
let products = try await self.sk2StoreProducts(withIdentifiers: identifiers)
Logger.debug(Strings.storeKit.store_product_request_did_finish)
completion(.success(Set(products)))
} catch {
Logger.debug(Strings.storeKit.store_products_request_failed(error: error))
completion(.failure(error))
}
}
Expand All @@ -119,9 +121,11 @@ private extension ProductsManager {
productsFetcherSK1.products(withIdentifiers: removedProductIdentifiers, completion: { result in
switch result {
case.success:
Copy link
Contributor

Choose a reason for hiding this comment

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

Hun looks like this is missing a space?

Suggested change
case.success:
case .success:

Logger.debug(Strings.storeKit.skproductsrequest_finished)
// this one is duplicate, though other in ProductsFetcherSK1 is rcSuccess
Copy link
Contributor Author

Choose a reason for hiding this comment

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

options i see are:
*. remove duplication -- need to look into hierarchy of logging, when would rcSuccess happen vs debug?

  • ensure sk2 has the same duplication

Copy link
Contributor

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've looked into these logs very much in detail, so this refactor is great 👍🏻

rcSuccess is just a success LogIntent, but still LogLevel.debug. I think that's good for these success cases.

Logger.debug(Strings.storeKit.store_product_request_did_finish)
case .failure(let error):
Logger.debug(Strings.storeKit.skproductsrequest_failed(error: error))
// this one is duplicate, though other in ProductsFetcherSK1 is appleError
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I'd say this one should be appleError too.

Logger.debug(Strings.storeKit.store_products_request_failed(error: error))
}
})
}
Expand All @@ -134,9 +138,10 @@ private extension ProductsManager {
if !removedProductIdentifiers.isEmpty {
do {
_ = try await self.productsFetcherSK2.products(identifiers: removedProductIdentifiers)
Logger.debug(Strings.storeKit.skproductsrequest_finished)

Logger.debug(Strings.storeKit.store_product_request_did_finish)
} catch {
Logger.debug(Strings.storeKit.skproductsrequest_failed(error: error))
Logger.debug(Strings.storeKit.store_products_request_failed(error: error))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: For consistency I'd keep the "finished" wording, or change this to did_fail as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, i'm lost, which finished wording?

Copy link
Contributor

Choose a reason for hiding this comment

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

store_products_request_failed vs store_product_request_did_finish vs store_product_request_received_response.
I would use "did {fail/finish/receive}" for all, or the past tense, for consistency.

Just mentioning that since you changed skproductsrequest_finished to store_product_request_did_finish

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh got it, i was looking for "finished" in that string you commented on. monday brain, thanks

}
}
}
Expand Down
8 changes: 4 additions & 4 deletions Sources/Purchasing/StoreKit1/ProductsFetcherSK1.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class ProductsFetcherSK1: NSObject {
}

Logger.debug(
Strings.offering.no_cached_requests_and_products_starting_skproduct_request(identifiers: identifiers)
Strings.storeKit.no_cached_requests_and_products_starting_skproduct_request(identifiers: identifiers)
)

self.completionHandlers[identifiers] = [completion]
Expand Down Expand Up @@ -119,7 +119,7 @@ extension ProductsFetcherSK1: SKProductsRequestDelegate {

func productsRequest(_ request: SKProductsRequest, didReceive response: SKProductsResponse) {
queue.async { [self] in
Logger.rcSuccess(Strings.storeKit.skproductsrequest_received_response)
Logger.rcSuccess(Strings.storeKit.store_product_request_received_response)
guard let productRequest = self.productsByRequests[request] else {
Logger.error("requested products not found for request: \(request)")
return
Expand All @@ -141,7 +141,7 @@ extension ProductsFetcherSK1: SKProductsRequestDelegate {
}

func requestDidFinish(_ request: SKRequest) {
Logger.rcSuccess(Strings.storeKit.skproductsrequest_finished)
Logger.rcSuccess(Strings.storeKit.store_product_request_did_finish)
self.cancelRequestToPreventTimeoutWarnings(request)
}

Expand All @@ -151,7 +151,7 @@ extension ProductsFetcherSK1: SKProductsRequestDelegate {
}

queue.async { [self] in
Logger.appleError(Strings.storeKit.skproductsrequest_failed(error: error))
Logger.appleError(Strings.storeKit.store_products_request_failed(error: error))

guard let productRequest = self.productsByRequests[request] else {
Logger.error(Strings.purchase.requested_products_not_found(request: request))
Expand Down
10 changes: 10 additions & 0 deletions Sources/Purchasing/StoreKit2/ProductsFetcherSK2.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,20 @@ actor ProductsFetcherSK2 {
func products(identifiers: Set<String>) async throws -> Set<SK2StoreProduct> {
do {
if let cachedProducts = await self.cachedProducts(withIdentifiers: identifiers) {
Logger.debug(
Strings.offering.products_already_cached(
identifiers: Set(cachedProducts.map { $0.productIdentifier})
)
)
return cachedProducts
}

Logger.debug(
Strings.storeKit.no_cached_requests_and_products_starting_skproduct_request(identifiers: identifiers)
)

let storeKitProducts = try await StoreKit.Product.products(for: identifiers)
Logger.rcSuccess(Strings.storeKit.store_product_request_received_response)
let sk2StoreProducts = Set(storeKitProducts.map { SK2StoreProduct(sk2Product: $0) })

await self.cache(products: sk2StoreProducts)
Expand Down