From 89e00f797eaf2f832d019c562048ebdcabac7d2d Mon Sep 17 00:00:00 2001 From: Allan Lang Date: Mon, 11 Jul 2022 22:04:26 +0100 Subject: [PATCH] A liberal dose of Rollbar around upload (#67) --- README.md | 30 ++++++-- Tree Tracker.xcodeproj/project.pbxproj | 4 ++ Tree Tracker/Extensions/DataResponse.swift | 12 ++++ Tree Tracker/Info.plist | 2 +- .../Screens/Upload/UploadViewModel.swift | 12 ++++ Tree Tracker/Services/AlamofireApi.swift | 72 ++++++------------- Tree Tracker/Services/Api.swift | 5 -- Tree Tracker/Services/MockApi.swift | 22 ------ .../Services/RetryingRequestInterceptor.swift | 2 + Unit Tests/Info.plist | 2 +- 10 files changed, 78 insertions(+), 85 deletions(-) create mode 100644 Tree Tracker/Extensions/DataResponse.swift diff --git a/README.md b/README.md index f13f0e8..ca05ce4 100644 --- a/README.md +++ b/README.md @@ -2,15 +2,21 @@ App for taking pictures of trees and storing that on a remote server. Mainly used by people who plant trees so they don't have to manually type coordinates with pictures they took and then try to guess the site/species afterwards. ## Running the app from Xcode with Mock server -1. Make sure you have downloaded Xcode 12.2+ +1. Make sure you have downloaded Xcode 13.4+ 2. Open the project in Xcode (you'll notice the dependencies will start to fetch in the background). (In the meantime, Xcode will need to fetch dependencies for the project... 😴) -3. You'll most likely need to change bundle identifier of the project. Basically because the project is set to auto-sign, each person that wants to run this on the device would need to update the bundle to be a unique id not registered before. E.g. from `com.protect.earth.Tree-Tracker` to `com.mynickname.Tree-Tracker`. -4. Make sure you are running `Tree Tracker (Mock server)` scheme and hit run! -5. When running on a device, you'll also need to trust the certificate in Settings -> General -> Profiles, otherwise you'll see an error after installing the build and before running it. +3. The signing settings for the project are configured for our CICD build pipeline, and will not allow you to build and run the app on your own device. To fix this, simply enable automatic signing in XCode and update the bundle identifier to something unique to you. This will update the .xcodeproj file accordingly. **NOTE** _Changes to signing settings must not be checked in, as these will break the automated builds._ +4. Running the `Tree Tracker` scheme will use the main Airtable base you [configure in your secrets file](#config) and will make inserts to your base tables. Running the `Tree Tracker (Mock server` scheme will use hard-coded mock API responses and will not touch Airtable. +5. When running on a device, you'll also need to trust the certificate in _Settings -> General -> Profiles_, otherwise you'll see an error after installing the build and before running it. ## Using your own Airtable/Cloudinary server -Well, this is a bit complicated but still doable. +Well, this is a bit complicated but still doable. +Sign up for a free [Airtable](https://www.airtable.com) account, as you will need to provide the details of *2* Airtable bases - one +to support the execution of integration tests, and one for the app to use when in normal usage. + +For development purposes, the 2 bases +can actually be the same. If you are doing this, it is recommended to create two sets of tables in the same base, and use a prefix on +the table name. This can then be specified in the `TEST_AIRTABLE_TABLE_NAME_PREFIX` secret (see [later](#config)). ### Airtable tables Our current API type expects that you have 4 tables: @@ -49,7 +55,15 @@ Because Airtable doesn't support uploading images yet, we have to use an externa 2. Now create an [upload preset](https://cloudinary.com/console/settings/upload) (this will give you the Upload Preset name). 3. Keep the keys as you'd need to add them to Secrets.xcconfig later on. -### Additional project config +## Rollbar +We use [Rollbar](https://www.rollbar.com) for centralised logging of errors, to help us troubleshoot issues with the app during real world usage. +If you wish, you can sign up for a free Rollbar account, generate your own API token and provide it through `ROLLBAR_AUTH_TOKEN` to see telemetry +in Rollbar during development. This can be useful if you are specifically adding telemetry features, but otherwise is probably more complex than +just looking at the logs in XCode console. + +If you choose not to setup Rollbar, simply add a dummy value for `ROLLBAR_AUTH_TOKEN` and any Rollbar calls will silently fail. + +## Additional project config {#config} Now, to run the project, we'll need to generate Secrets file. This means you need to run first install [`pouch`](https://github.com/sunshinejr/pouch) (the easiest is using `brew install sunshinejr/formulae/pouch`). Now, you need to have these environment variables available. Have this at the end of the file (bash: most likely in `.bash_profile` or `.bashrc`, zsh: most likely `.zshenv` or `.zshrc`): ``` export AIRTABLE_API_KEY=yourKey123 @@ -60,6 +74,10 @@ export AIRTABLE_SUPERVISORS_TABLE_NAME=Supervisors export AIRTABLE_SITES_TABLE_NAME=Sites export CLOUDINARY_CLOUD_NAME=qqq2ek4mq export CLOUDINARY_UPLOAD_PRESET_NAME=iadfadff +export TEST_AIRTABLE_API_KEY=yourTestKey123 +export TEST_AIRTABLE_BASE_ID=appNiceTreeTest +export TEST_AIRTABLE_TABLE_NAME_PREFIX=test_ +export ROLLBAR_AUTH_TOKEN=yourRollbarToken ``` In the root folder, run `pouch`, which should generate a file at `./TreeTracker/Secrets.swift`. diff --git a/Tree Tracker.xcodeproj/project.pbxproj b/Tree Tracker.xcodeproj/project.pbxproj index af59528..50a8406 100644 --- a/Tree Tracker.xcodeproj/project.pbxproj +++ b/Tree Tracker.xcodeproj/project.pbxproj @@ -116,6 +116,7 @@ 9D5D5E2A284B635900F3AD3E /* AirtableSpeciesService.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9D5D5E29284B635900F3AD3E /* AirtableSpeciesService.swift */; }; 9D5D5E2C284B66BB00F3AD3E /* SupervisorService.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9D5D5E2B284B66BB00F3AD3E /* SupervisorService.swift */; }; 9D5D5E2E284B670400F3AD3E /* AirtableSupervisorService.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9D5D5E2D284B670400F3AD3E /* AirtableSupervisorService.swift */; }; + 9D5F06332878ADF000C8D4A6 /* DataResponse.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9D5F06322878ADF000C8D4A6 /* DataResponse.swift */; }; 9D79A5A7283AE03100F0F96C /* SiteService.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9D79A5A6283AE03100F0F96C /* SiteService.swift */; }; 9D79A5AA283AE27500F0F96C /* DataAccessError.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9D79A5A9283AE27500F0F96C /* DataAccessError.swift */; }; 9D79A5AC283AE32C00F0F96C /* AirtableSiteService.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9D79A5AB283AE32C00F0F96C /* AirtableSiteService.swift */; }; @@ -253,6 +254,7 @@ 9D5D5E29284B635900F3AD3E /* AirtableSpeciesService.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AirtableSpeciesService.swift; sourceTree = ""; }; 9D5D5E2B284B66BB00F3AD3E /* SupervisorService.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SupervisorService.swift; sourceTree = ""; }; 9D5D5E2D284B670400F3AD3E /* AirtableSupervisorService.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AirtableSupervisorService.swift; sourceTree = ""; }; + 9D5F06322878ADF000C8D4A6 /* DataResponse.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = DataResponse.swift; sourceTree = ""; }; 9D79A5A6283AE03100F0F96C /* SiteService.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SiteService.swift; sourceTree = ""; }; 9D79A5A9283AE27500F0F96C /* DataAccessError.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DataAccessError.swift; sourceTree = ""; }; 9D79A5AB283AE32C00F0F96C /* AirtableSiteService.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AirtableSiteService.swift; sourceTree = ""; }; @@ -379,6 +381,7 @@ 85792A7425B0A35100BFDA96 /* Extensions */ = { isa = PBXGroup; children = ( + 9D5F06322878ADF000C8D4A6 /* DataResponse.swift */, 85B839EB25B8661E0008E167 /* Collection.swift */, 85B83A3725B9D9C40008E167 /* Data.swift */, 851DAC1C262B4B0B0087E1D4 /* Date.swift */, @@ -804,6 +807,7 @@ 9DB29B562821C28400AAC73D /* SettingsController.swift in Sources */, 9D5D5E2E284B670400F3AD3E /* AirtableSupervisorService.swift in Sources */, 853ABD562596144900144B0D /* AppDelegate.swift in Sources */, + 9D5F06332878ADF000C8D4A6 /* DataResponse.swift in Sources */, 857BADA825B1FA93005D7D35 /* TreeDetailsViewController.swift in Sources */, 9D5D5E2C284B66BB00F3AD3E /* SupervisorService.swift in Sources */, 85B83A1C25B8AC650008E167 /* EditLocalTreeViewModel.swift in Sources */, diff --git a/Tree Tracker/Extensions/DataResponse.swift b/Tree Tracker/Extensions/DataResponse.swift new file mode 100644 index 0000000..fa98ffb --- /dev/null +++ b/Tree Tracker/Extensions/DataResponse.swift @@ -0,0 +1,12 @@ +import Foundation +import Alamofire + +extension DataResponse { + + func dataAsUTF8String() -> String { + guard let data = self.data else { return "" } + guard let utf8String = String.init( data: data, encoding: .utf8) else { return "" } + return utf8String + } + +} diff --git a/Tree Tracker/Info.plist b/Tree Tracker/Info.plist index 7b467df..0f7f991 100644 --- a/Tree Tracker/Info.plist +++ b/Tree Tracker/Info.plist @@ -17,7 +17,7 @@ CFBundlePackageType $(PRODUCT_BUNDLE_PACKAGE_TYPE) CFBundleShortVersionString - 0.8.1 + 0.8.2 CFBundleVersion $(CURRENT_PROJECT_VERSION) ITSAppUsesNonExemptEncryption diff --git a/Tree Tracker/Screens/Upload/UploadViewModel.swift b/Tree Tracker/Screens/Upload/UploadViewModel.swift index b792856..89833ed 100644 --- a/Tree Tracker/Screens/Upload/UploadViewModel.swift +++ b/Tree Tracker/Screens/Upload/UploadViewModel.swift @@ -1,5 +1,6 @@ import Foundation import Resolver +import RollbarNotifier protocol UploadNavigating: AnyObject { func triggerAddTreesFlow(completion: @escaping (Bool) -> Void) @@ -133,8 +134,10 @@ final class UploadViewModel: CollectionViewModel { logger.log(.upload, "Uploading images...") database.fetchLocalTrees { [weak self] trees in self?.logger.log(.upload, "Trees to upload: \(trees.count)") + Rollbar.infoMessage("Starting upload of trees", data: ["tree_count": trees.count], context: "UploadViewModel.uploadLocalTreesRecursively") guard let tree = trees.sorted(by: \.createDate, order: .descending).first else { + Rollbar.infoMessage("Trees upload complete") self?.logger.log(.upload, "No more items to upload - bailing.") self?.stopUploading() return @@ -150,6 +153,8 @@ final class UploadViewModel: CollectionViewModel { completion: { result in switch result { case let .success(airtableTree): + Rollbar.infoMessage("Successfully uploaded tree", data: ["id": airtableTree.id, + "md5": airtableTree.imageMd5 ?? ""]) self?.logger.log(.upload, "Successfully uploaded tree.") self?.database.save([airtableTree], sentFromThisDevice: true) self?.database.remove(tree: tree) { @@ -159,6 +164,13 @@ final class UploadViewModel: CollectionViewModel { case let .failure(error): self?.update(uploadProgress: 0.0, for: tree) self?.presentUploadButton(isUploading: false) + Rollbar.errorError(error, + data: ["supervisor": tree.supervisor, + "site": tree.site, + "coordinates": tree.coordinates ?? "", + "md5": tree.imageMd5 ?? "", + "phImageId": tree.phImageId], + context: "UploadViewModel.uploadLocalTreesRecursively") self?.logger.log(.upload, "Error when uploading a local tree: \(error)") } } diff --git a/Tree Tracker/Services/AlamofireApi.swift b/Tree Tracker/Services/AlamofireApi.swift index 823af7f..69c0202 100644 --- a/Tree Tracker/Services/AlamofireApi.swift +++ b/Tree Tracker/Services/AlamofireApi.swift @@ -1,6 +1,7 @@ import Foundation import Alamofire import class UIKit.UIImage +import RollbarNotifier fileprivate extension LogCategory { static var api = LogCategory(name: "Api") @@ -36,52 +37,6 @@ final class AlamofireApi: Api { maxRetries: Constants.Http.requestRetryLimit)) } - - func treesPlanted(offset: String?, completion: @escaping (Result, AFError>) -> Void) { - let request = session.request(Config.treesUrl, method: .get, parameters: ["offset": offset].compactMapValues { $0 }, encoding: URLEncoding.queryString, headers: Config.headers, interceptor: nil, requestModifier: nil) - - request.validate().responseDecodable(decoder: JSONDecoder._iso8601ms) { (response: DataResponse, AFError>) in - completion(response.result) - } - } - - func species(offset: String?, completion: @escaping (Result, AFError>) -> Void) { - let request = session.request(Config.speciesUrl, method: .get, parameters: ["offset": offset].compactMapValues { $0 }, encoding: URLEncoding.queryString, headers: Config.headers, interceptor: nil, requestModifier: nil) - - request.validate().responseDecodable(decoder: JSONDecoder._iso8601ms) { (response: DataResponse, AFError>) in - completion(response.result) - } - } - - func sites(offset: String?, completion: @escaping (Result, AFError>) -> Void) { - let request = session.request(Config.sitesUrl, method: .get, parameters: ["offset": offset].compactMapValues { $0 }, encoding: URLEncoding.queryString, headers: Config.headers, interceptor: nil, requestModifier: nil) - - request.validate().responseDecodable(decoder: JSONDecoder._iso8601ms) { (response: DataResponse, AFError>) in - completion(response.result) - } - } - - func supervisors(offset: String?, completion: @escaping (Result, AFError>) -> Void) { - let request = session.request(Config.supervisorsUrl, method: .get, parameters: ["offset": offset].compactMapValues { $0 }, encoding: URLEncoding.queryString, headers: Config.headers, interceptor: nil, requestModifier: nil) - - request.validate().responseDecodable(decoder: JSONDecoder._iso8601ms) { (response: DataResponse, AFError>) in - completion(response.result) - } - } - - func addSite(name: String, completion: @escaping (Result) -> Void) { - // build struct to represent target JSON body - let parameters: [String: [String: String]] = [ - "fields": ["Name": name] - ] - - // TODO: does specifying a nil interceptor here override the retrying interceptor we configure at session level? - let request = session.request(Config.sitesUrl, method: .post, parameters: parameters, encoder: JSONParameterEncoder.default, headers: Config.headers, interceptor: nil, requestModifier: nil) - - request.validate().responseDecodable(decoder: JSONDecoder._iso8601ms) { (response: DataResponse) in - completion(response.result) - } - } func upload(tree: LocalTree, progress: @escaping (Double) -> Void = { _ in }, completion: @escaping (Result) -> Void) -> Cancellable { let upload = ImageUpload(tree: tree, logger: logger) @@ -145,6 +100,13 @@ final class ImageUpload: Cancellable { newTree.imageMd5 = md5 self?.request = self?.upload(tree: newTree, imageUrl: url, session: session, completion: completion) case let .failure(error): + Rollbar.errorError(error, + data: ["md5": tree.imageMd5 ?? "", + "phImageId": tree.phImageId, + "coordinates": tree.coordinates ?? "", + "supervisor": tree.supervisor, + "site": tree.site], + context: "Fetching upload image for tree") completion(.failure(error)) } } @@ -155,6 +117,7 @@ final class ImageUpload: Cancellable { logger.log(.api, "Uploading image to Cloudinary...") guard let data = image.jpegData(compressionQuality: 0.8) else { logger.log(.api, "No pngData for the image, bailing") + Rollbar.errorMessage("No pngData for image, upload will be skipped") completion(.failure(.explicitlyCancelled)) return nil } @@ -175,7 +138,10 @@ final class ImageUpload: Cancellable { return request.validate().responseJSON { [weak self] response in switch response.result { case let .failure(error): - self?.logger.log(.api, "Error when uploading image: \(response.data.map { String.init(data: $0, encoding: .utf8) })") + Rollbar.errorError(error, + data: [:], + context: response.dataAsUTF8String()) + self?.logger.log(.api, "Error when uploading image: \(response.dataAsUTF8String())") completion(.failure(error)) case let .success(json as [String: Any]): let url = json["secure_url"] as? String @@ -186,7 +152,10 @@ final class ImageUpload: Cancellable { fallthrough } default: - self?.logger.log(.api, "Error when parsing json: \(response.data.map { String.init(data: $0, encoding: .utf8) })") + Rollbar.errorMessage("Error while parsing JSON", + data: [:], + context: response.dataAsUTF8String()) + self?.logger.log(.api, "Error when parsing json: \(response.dataAsUTF8String())") completion(.failure(.explicitlyCancelled)) } } @@ -194,7 +163,7 @@ final class ImageUpload: Cancellable { private func upload(tree: LocalTree, imageUrl: String, session: Session, completion: @escaping (Result) -> Void) -> Request? { let airtableTree = tree.toAirtableTree(imageUrl: imageUrl) - let request = session.request(AlamofireApi.Config.treesUrl, method: .post, parameters: airtableTree, encoder: JSONParameterEncoder(encoder: ._iso8601ms), headers: AlamofireApi.Config.headers, interceptor: nil, requestModifier: nil) + let request = session.request(AlamofireApi.Config.treesUrl, method: .post, parameters: airtableTree, encoder: JSONParameterEncoder(encoder: ._iso8601ms), headers: AlamofireApi.Config.headers) return request.validate().responseDecodable(decoder: JSONDecoder._iso8601ms) { [weak self] (response: DataResponse) in self?.progress?(1.0) @@ -204,7 +173,10 @@ final class ImageUpload: Cancellable { self?.logger.log(.api, "Tree uploaded!") completion(.success(tree)) case let .failure(error): - self?.logger.log(.api, "Error when creating Airtable record: \(response.data.map { String.init(data: $0, encoding: .utf8) })") + Rollbar.errorError(error, + data: [:], + context: response.dataAsUTF8String()) + self?.logger.log(.api, "Error when creating Airtable record: \(response.dataAsUTF8String())") completion(.failure(error)) } } diff --git a/Tree Tracker/Services/Api.swift b/Tree Tracker/Services/Api.swift index 053314a..6abcad0 100644 --- a/Tree Tracker/Services/Api.swift +++ b/Tree Tracker/Services/Api.swift @@ -3,11 +3,6 @@ import Alamofire import class UIKit.UIImage protocol Api { - func treesPlanted(offset: String?, completion: @escaping (Result, AFError>) -> Void) - func species(offset: String?, completion: @escaping (Result, AFError>) -> Void) - func sites(offset: String?, completion: @escaping (Result, AFError>) -> Void) - func supervisors(offset: String?, completion: @escaping (Result, AFError>) -> Void) func upload(tree: LocalTree, progress: @escaping (Double) -> Void, completion: @escaping (Result) -> Void) -> Cancellable func loadImage(url: String, completion: @escaping (UIImage?) -> Void) - func addSite(name: String, completion: @escaping (Result) -> Void) } diff --git a/Tree Tracker/Services/MockApi.swift b/Tree Tracker/Services/MockApi.swift index 978122b..73b518e 100644 --- a/Tree Tracker/Services/MockApi.swift +++ b/Tree Tracker/Services/MockApi.swift @@ -18,22 +18,6 @@ final class MockApi: Api { private(set) var supervisors: [AirtableSupervisor] = [.init(id: "1", name: "Josh Hopkins")] private var images = [UIImage.mockTree1, .mockTree2, .mockTree3] - func treesPlanted(offset: String?, completion: @escaping (Result, AFError>) -> Void) { - delayAndCompleteWithPossibleError(successResponse: Paginated(offset: offset, records: treesPlanted), completionToCall: completion) - } - - func species(offset: String?, completion: @escaping (Result, AFError>) -> Void) { - delayAndCompleteWithPossibleError(successResponse: Paginated(offset: offset, records: species), completionToCall: completion) - } - - func sites(offset: String?, completion: @escaping (Result, AFError>) -> Void) { - delayAndCompleteWithPossibleError(successResponse: Paginated(offset: offset, records: sites), completionToCall: completion) - } - - func supervisors(offset: String?, completion: @escaping (Result, AFError>) -> Void) { - delayAndCompleteWithPossibleError(successResponse: Paginated(offset: offset, records: supervisors), completionToCall: completion) - } - func upload(tree: LocalTree, progress: @escaping (Double) -> Void, completion: @escaping (Result) -> Void) -> Cancellable { var isCancelled = false @@ -68,12 +52,6 @@ final class MockApi: Api { } } - func addSite(name: String, completion: @escaping (Result) -> Void) { - delay { - completion(.success(self.sites[0])) - } - } - private func delay(completion: @escaping () -> Void) { let delay = TimeInterval.random(in: delayRange) diff --git a/Tree Tracker/Services/RetryingRequestInterceptor.swift b/Tree Tracker/Services/RetryingRequestInterceptor.swift index a4b89e5..94627f3 100644 --- a/Tree Tracker/Services/RetryingRequestInterceptor.swift +++ b/Tree Tracker/Services/RetryingRequestInterceptor.swift @@ -1,5 +1,6 @@ import Alamofire import Foundation +import RollbarNotifier class RetryingRequestInterceptor: RequestInterceptor { var maxRetries: Int = 5 @@ -14,6 +15,7 @@ class RetryingRequestInterceptor: RequestInterceptor { let response = request.task?.response as? HTTPURLResponse if let statusCode = response?.statusCode, (500...599).contains(statusCode), request.retryCount < maxRetries { + Rollbar.warningMessage("Retrying request with delay \(retryDelay)s due to status code \(statusCode). Retry \(request.retryCount) of \(maxRetries)") completion(.retryWithDelay(retryDelay)) } else { return completion(.doNotRetry) diff --git a/Unit Tests/Info.plist b/Unit Tests/Info.plist index 93c38de..157a32a 100644 --- a/Unit Tests/Info.plist +++ b/Unit Tests/Info.plist @@ -15,7 +15,7 @@ CFBundlePackageType $(PRODUCT_BUNDLE_PACKAGE_TYPE) CFBundleShortVersionString - 0.8.1 + 0.8.2 CFBundleVersion 1