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

[MM-57409] Notify error sharing on iOS #8408

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[MM-57409] Notify error sharing on iOS #8408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7586206
ios share extension handle error
Willyfrog Dec 5, 2024
ec3f621
move info to preferences and cancel if we are over the timeout
Willyfrog Dec 11, 2024
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
85 changes: 83 additions & 2 deletions ios/Gekidou/Package.resolved
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the changes on this file?

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'll remove them, they were added by mistake

Original file line number Diff line number Diff line change
@@ -1,13 +1,94 @@
{
"object": {
"pins": [
{
"package": "Cryptor",
"repositoryURL": "https://github.com/Kitura/BlueCryptor.git",
"state": {
"branch": null,
"revision": "cec97c24b111351e70e448972a7d3fe68a756d6d",
"version": "2.0.2"
}
},
{
"package": "CryptorECC",
"repositoryURL": "https://github.com/Kitura/BlueECC.git",
"state": {
"branch": null,
"revision": "1485268a54f8135435a825a855e733f026fa6cc8",
"version": "1.2.201"
}
},
{
"package": "CryptorRSA",
"repositoryURL": "https://github.com/Kitura/BlueRSA.git",
"state": {
"branch": null,
"revision": "440f78db26d8bb073f29590f1c7bd31004da09ae",
"version": "1.0.201"
}
},
{
"package": "KituraContracts",
"repositoryURL": "https://github.com/Kitura/KituraContracts.git",
"state": {
"branch": null,
"revision": "8a4778c3aa7833e9e1af884e8819d436c237cd70",
"version": "1.2.201"
}
},
{
"package": "LoggerAPI",
"repositoryURL": "https://github.com/Kitura/LoggerAPI.git",
"state": {
"branch": null,
"revision": "e82d34eab3f0b05391082b11ea07d3b70d2f65bb",
"version": "1.9.200"
}
},
{
"package": "OpenGraph",
"repositoryURL": "https://github.com/satoshi-takano/OpenGraph.git",
"state": {
"branch": null,
"revision": "382972f1963580eeabafd88ad012e66576b4d213",
"version": "1.4.1"
}
},
{
"package": "Sentry",
"repositoryURL": "https://github.com/getsentry/sentry-cocoa.git",
"state": {
"branch": null,
"revision": "5575af93efb776414f243e93d6af9f6258dc539a",
"version": "8.36.0"
}
},
{
"package": "SQLite.swift",
"repositoryURL": "https://github.com/stephencelis/SQLite.swift.git",
"state": {
"branch": null,
"revision": "9af51e2edf491c0ea632e369a6566e09b65aa333",
"version": "0.13.0"
"revision": "a95fc6df17d108bd99210db5e8a9bac90fe984b8",
"version": "0.15.3"
}
},
{
"package": "SwiftJWT",
"repositoryURL": "https://github.com/Kitura/Swift-JWT.git",
"state": {
"branch": null,
"revision": "47c6384b6923e9bb1f214d2ba4bd52af39440588",
"version": "3.6.201"
}
},
{
"package": "swift-log",
"repositoryURL": "https://github.com/apple/swift-log.git",
"state": {
"branch": null,
"revision": "9cb486020ebf03bfa5b5df985387a14a98744537",
"version": "1.6.1"
}
}
]
Expand Down
24 changes: 23 additions & 1 deletion ios/Gekidou/Sources/Gekidou/Networking/ShareExtension+Delegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,14 @@ extension ShareExtension: URLSessionDataDelegate {
session.configuration.identifier ?? "no identifier",
error?.localizedDescription ?? "no error description available"
)

if let sessionID = session.configuration.identifier {
let uploadData = getUploadSessionData(id: sessionID)
self.notifyFailureNow(failID: uploadData?.failNotificationID)
} else {
_ = self.scheduleFailNotification(timeout: 1)
}

return
}

Expand Down Expand Up @@ -176,6 +184,14 @@ extension ShareExtension: URLSessionDataDelegate {

let total = data.totalFiles
let count = data.fileIds.count
if let timeout = data.failTimeout, timeout < Date() {
backgroundSession?.invalidateAndCancel()
os_log(
"Mattermost BackgroundSession: postMessageIfUploadFinished session %{public}@ timed out",
session.configuration.identifier ?? "no identifier"
)
return
}
Comment on lines +187 to +194
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be done after checking if the last file was actually uploaded? because if that is the case, this is cancelling the whole thing instead of just posting the message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! The reason is that we already told the user that the sharing failed due to a timeout. Since we can't control when this happens this could be a day later making things confusing. So we cancel the first opportunity we have


if count == total {
os_log(
Expand All @@ -189,10 +205,16 @@ extension ShareExtension: URLSessionDataDelegate {
self.postMessageForSession(withId: id)
self.urlSessionDidFinishEvents(forBackgroundURLSession: session)
}
os_log(
OSLogType.default,
"Mattermost BackgroundSession: we are good, removing the fail notification=%{public}@",
id
)
self.cancelFailNotification(failID: data.failNotificationID)
} else {
os_log(
OSLogType.default,
"Mattermost BackgroundSession: finish uploading files %{public}@ of %{public}@ for identifier=%{public}@",
"Mattermost BackgroundSession: still uploading files %{public}@ of %{public}@ for identifier=%{public}@",
"\(count)",
"\(total)",
id
Expand Down
17 changes: 13 additions & 4 deletions ios/Gekidou/Sources/Gekidou/Networking/ShareExtension+Post.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,19 @@ import Foundation
import os.log

extension ShareExtension {

public func uploadFiles(serverUrl: String, channelId: String, message: String,
files: [String], completionHandler: @escaping () -> Void) -> String? {
let id = "mattermost-share-upload-\(UUID().uuidString)"

let uuidString = self.scheduleFailNotification(timeout: SHARE_TIMEOUT)

createUploadSessionData(
id: id, serverUrl: serverUrl,
channelId: channelId, message: message,
files: files
files: files,
failNotificationID: uuidString,
failTimeout: Date().addingTimeInterval(SHARE_TIMEOUT)
)

guard let token = try? Keychain.default.getToken(for: serverUrl) else {return "Could not retrieve the session token from the KeyChain"}
Expand Down Expand Up @@ -56,6 +61,7 @@ extension ShareExtension {
filename,
id
)
return "There was an error when trying to upload. Please try again"
Copy link
Contributor

Choose a reason for hiding this comment

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

before the other two returns you added a notifyFailureNow call, but not here, why?

}
} else {
os_log(
Expand All @@ -64,6 +70,7 @@ extension ShareExtension {
filename,
id
)
notifyFailureNow(description: FILE_ERROR_MESSAGE, failID: uuidString)
return "The file \(filename) could not be processed for upload"
}
} else {
Expand All @@ -73,6 +80,7 @@ extension ShareExtension {
file,
id
)
notifyFailureNow(description: FILE_ERROR_MESSAGE, failID: uuidString)
return "File not found \(file)"
}
}
Expand All @@ -83,13 +91,14 @@ extension ShareExtension {
"Mattermost BackgroundSession: posting message for identifier=%{public}@ without files",
id
)
self.postMessageForSession(withId: id, completionHandler: completionHandler)
let cancelAndComplete = createCancelHandler(completionHandler: completionHandler, failNotificationID: uuidString)
self.postMessageForSession(withId: id, completionHandler: cancelAndComplete)
}

return nil
}

func postMessageForSession(withId id: String, completionHandler: (() -> Void)? = nil) {
func postMessageForSession(withId id: String, completionHandler: ((Bool) -> Void)? = nil) {
guard let data = getUploadSessionData(id: id)
else {
os_log(
Expand Down Expand Up @@ -127,7 +136,7 @@ extension ShareExtension {
"Mattermost BackgroundSession: postMessageForSession without files call completionHandler for identifier=%{public}@",
id
)
handler()
handler(error == nil)
}
})
}
Expand Down
89 changes: 84 additions & 5 deletions ios/Gekidou/Sources/Gekidou/Networking/ShareExtension.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import Foundation
import os.log
import UserNotifications

struct UploadSessionData {
var serverUrl: String?
Expand All @@ -15,6 +16,8 @@ struct UploadSessionData {
var fileIds: [String] = []
var message: String = ""
var totalFiles: Int = 0
var failNotificationID: String?
var failTimeout: Date?

func toDictionary() -> NSDictionary {
let data: NSDictionary = [
Expand All @@ -23,7 +26,9 @@ struct UploadSessionData {
"files": files,
"fileIds": fileIds,
"message": message,
"totalFiles": totalFiles
"totalFiles": totalFiles,
"failNotificationID": failNotificationID,
"failTimeout": failTimeout
]

return data
Expand All @@ -36,18 +41,27 @@ struct UploadSessionData {
let fileIds = dict["fileIds"] as! [String]
let message = dict["message"] as! String
let totalFiles = dict["totalFiles"] as! Int
let failNotificationID = dict["failNotificationID"] as! String
let failTimeout = dict["failTimeout"] as! Date

return UploadSessionData(
serverUrl: serverUrl,
channelId: channelId,
files: files,
fileIds: fileIds,
message: message,
totalFiles: totalFiles
totalFiles: totalFiles,
failNotificationID: failNotificationID,
failTimeout: failTimeout
)
}
}

// TODO: setup some configuration for allowing users to figure out the right ammount of time to wait.
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 should allow the user to set this, we should come up with a number that makes sense.. maybe an hour or so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's cool with me too. O just didn't feel entitled to make a decision on either option

let SHARE_TIMEOUT: Double = 120 // change to 300
let NETWORK_ERROR_MESSAGE: String = "Mattermost App couldn't acknowledge the message was posted due to network conditions, please review and try again"
let FILE_ERROR_MESSAGE: String = "The shared file couldn't be processed for sharing"
Comment on lines +62 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally this would be localized. You can check the code in the Share Extension around the Views on how is done with the use of NSLocalizedString or String.localizedString.


public class ShareExtension: NSObject {
public var backgroundSession: URLSession?
private var cacheURL: URL?
Expand Down Expand Up @@ -88,13 +102,15 @@ public class ShareExtension: NSObject {
)
}

func createUploadSessionData(id: String, serverUrl: String, channelId: String, message: String, files: [String]) {
func createUploadSessionData(id: String, serverUrl: String, channelId: String, message: String, files: [String], failNotificationID: String, failTimeout: Date) {
let data = UploadSessionData(
serverUrl: serverUrl,
channelId: channelId,
files: files,
message: message,
totalFiles: files.count
totalFiles: files.count,
failNotificationID: failNotificationID,
failTimeout: failTimeout
)

saveUploadSessionData(id: id, data: data)
Expand Down Expand Up @@ -127,7 +143,9 @@ public class ShareExtension: NSObject {
files: data.files,
fileIds: fileIds,
message: data.message,
totalFiles: data.totalFiles
totalFiles: data.totalFiles,
failNotificationID: data.failNotificationID,
failTimeout: data.failTimeout
)
saveUploadSessionData(id: id, data: newData)
}
Expand All @@ -146,4 +164,65 @@ public class ShareExtension: NSObject {
}
}
}

func scheduleFailNotification(timeout: Double = SHARE_TIMEOUT, description: String = NETWORK_ERROR_MESSAGE) -> String {
let failNotification = UNMutableNotificationContent()
failNotification.title = "Share content failed"
Copy link
Contributor

Choose a reason for hiding this comment

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

this should also be localized.

failNotification.body = description

let timeoutTrigger = UNTimeIntervalNotificationTrigger(timeInterval: timeout, repeats: false)

let uuidString = UUID().uuidString
let request = UNNotificationRequest(identifier: uuidString, content: failNotification, trigger: timeoutTrigger)

os_log(
OSLogType.default,
"Mattermost BackgroundSession: Created Fail notification with ID: %{uuidString}@",
uuidString
)

// Schedule the request with the system.
UNUserNotificationCenter.current().add(request)

return uuidString
}

func cancelFailNotification(failID: String? = nil) -> Void {
if let id = failID {
UNUserNotificationCenter.current().removePendingNotificationRequests(withIdentifiers: [id])
os_log(
OSLogType.default,
"Mattermost BackgroundSession: Cancelled fail notification %{id}@",
id
)
}
}

func notifyFailureNow(description: String = NETWORK_ERROR_MESSAGE, failID: String?) -> Void {
self.cancelFailNotification(failID: failID)
_ = self.scheduleFailNotification(timeout: 1, description: description)
}

func createCancelHandler(completionHandler: @escaping () -> Void, failNotificationID: String) -> (Bool) -> Void {
return { success in
if success {
self.cancelFailNotification()

// cancel any pending tasks in case of not getting connection. If this affects anything else, then we should track the uploads and cancel them individually
// Session is configured to wait for connection, but we don't know when that's going to happen, so we die early if we don't get a connection in a time that
// makes sense.
self.backgroundSession?.invalidateAndCancel()
completionHandler()
return
}

self.notifyFailureNow(failID: failNotificationID)
os_log(
OSLogType.default,
"Mattermost BackgroundSession: sending fail notification now"
)

completionHandler()
}
}
}