-
Notifications
You must be signed in to change notification settings - Fork 983
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.
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
[Connect] Enable file downloads #4086
Changes from all commits
e0fa6b2
ae114fd
f0ca18f
f217963
e1fcaf5
12a07c5
d50f44b
a4cc8fe
52915f5
8f651ab
c41dfcc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,7 +26,7 @@ struct API { | |
guard let baseUrl = URL(string: baseURL) else { | ||
return .failure(.invalidURL) | ||
} | ||
let url = baseUrl.appending(path: path) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
let url = baseUrl.appendingPathComponent(path) | ||
var request = URLRequest(url: url) | ||
request.httpMethod = method | ||
request.allHTTPHeaderFields = headers | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<document type="com.apple.InterfaceBuilder3.CocoaTouch.Storyboard.XIB" version="3.0" toolsVersion="32700.99.1234" targetRuntime="iOS.CocoaTouch" propertyAccessControl="none" useAutolayout="YES" launchScreen="YES" useTraitCollections="YES" useSafeAreas="YES" colorMatched="YES" initialViewController="01J-lp-oVM"> | ||
<document type="com.apple.InterfaceBuilder3.CocoaTouch.Storyboard.XIB" version="3.0" toolsVersion="23094" targetRuntime="iOS.CocoaTouch" propertyAccessControl="none" useAutolayout="YES" launchScreen="YES" useTraitCollections="YES" useSafeAreas="YES" colorMatched="YES" initialViewController="01J-lp-oVM"> | ||
<device id="retina6_12" orientation="portrait" appearance="light"/> | ||
<dependencies> | ||
<plugIn identifier="com.apple.InterfaceBuilder.IBCocoaTouchPlugin" version="22685"/> | ||
<deployment identifier="iOS"/> | ||
<plugIn identifier="com.apple.InterfaceBuilder.IBCocoaTouchPlugin" version="23084"/> | ||
<capability name="Safe area layout guides" minToolsVersion="9.0"/> | ||
<capability name="System colors in document resources" minToolsVersion="11.0"/> | ||
<capability name="documents saved in the Xcode 8 format" minToolsVersion="8.0"/> | ||
|
@@ -16,16 +17,20 @@ | |
<rect key="frame" x="0.0" y="0.0" width="393" height="852"/> | ||
<autoresizingMask key="autoresizingMask" widthSizable="YES" heightSizable="YES"/> | ||
<subviews> | ||
<label opaque="NO" userInteractionEnabled="NO" contentMode="left" horizontalHuggingPriority="251" verticalHuggingPriority="251" fixedFrame="YES" text="StripeConnect Example" textAlignment="natural" lineBreakMode="tailTruncation" baselineAdjustment="alignBaselines" adjustsFontSizeToFit="NO" translatesAutoresizingMaskIntoConstraints="NO" id="tpl-NI-fTA"> | ||
<rect key="frame" x="49" y="409" width="294" height="35"/> | ||
<autoresizingMask key="autoresizingMask" flexibleMaxX="YES" flexibleMaxY="YES"/> | ||
<label opaque="NO" userInteractionEnabled="NO" contentMode="left" horizontalHuggingPriority="251" verticalHuggingPriority="251" text="StripeConnect Example" textAlignment="center" lineBreakMode="tailTruncation" baselineAdjustment="alignBaselines" adjustsFontSizeToFit="NO" translatesAutoresizingMaskIntoConstraints="NO" id="tpl-NI-fTA"> | ||
<rect key="frame" x="20" y="408.66666666666669" width="353" height="35"/> | ||
<fontDescription key="fontDescription" type="system" pointSize="29"/> | ||
<nil key="textColor"/> | ||
<nil key="highlightedColor"/> | ||
</label> | ||
</subviews> | ||
<viewLayoutGuide key="safeArea" id="6Tk-OE-BBY"/> | ||
<color key="backgroundColor" systemColor="systemBackgroundColor"/> | ||
<constraints> | ||
<constraint firstItem="6Tk-OE-BBY" firstAttribute="trailing" secondItem="tpl-NI-fTA" secondAttribute="trailing" constant="20" id="C34-2d-4Rw"/> | ||
<constraint firstItem="tpl-NI-fTA" firstAttribute="leading" secondItem="6Tk-OE-BBY" secondAttribute="leading" constant="20" id="HH7-od-x32"/> | ||
<constraint firstItem="tpl-NI-fTA" firstAttribute="centerY" secondItem="Ze5-6b-2t3" secondAttribute="centerY" id="fIw-p1-lqC"/> | ||
</constraints> | ||
Comment on lines
+29
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Centers the launchscreen text with 20pt horizontal padding |
||
</view> | ||
</viewController> | ||
<placeholder placeholderIdentifier="IBFirstResponder" id="iYj-Kq-Ea1" userLabel="First Responder" sceneMemberID="firstResponder"/> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,6 @@ enum StripeConnectConstants { | |
/**' | ||
Pages or navigation requests matching any of these hosts will... | ||
- Automatically grant camera permissions | ||
- Accept downloads (TODO MXMOBILE-2485) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're not using allowedHosts for downloads because most of our downloads come from external hosts (e.g. S3) |
||
- Open popups in PopupWebViewController (instead of Safari) | ||
*/ | ||
static let allowedHosts: Set<String> = [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
// Created by Mel Ludowise on 5/3/24. | ||
// | ||
|
||
import QuickLook | ||
import SafariServices | ||
@_spi(STP) import StripeCore | ||
import WebKit | ||
|
@@ -14,11 +15,14 @@ import WebKit | |
- Camera access | ||
- Popup windows | ||
- Opening email links | ||
- Downloads TODO MXMOBILE-2485 | ||
- Downloads | ||
*/ | ||
@available(iOS 15, *) | ||
class ConnectWebView: WKWebView { | ||
|
||
/// File URL for a downloaded file | ||
var downloadedFile: URL? | ||
|
||
Comment on lines
+23
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mxl-stripe do we need to be robust to potentially handle multiple files that simultaneous download? The current design only stores one file at a time. If there's multiple downloads that occur simultaneously, we could get some unexpected behavior. If we need to, we could potentially handle multiple files, but it would take some effort. We'd have to effectively make a queue of downloaded files and wait for them to all finish before showing the preview. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should ever be triggering multiple downloads at the same time. Is it easy to detect if that is happening? I wonder if we could add a metric for that situation and monitor that it's zero. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we should be able to add a metric for this – I added a comment to MXMOBILE-2491 so I remember to do that when adding analytics :-) |
||
private var optionalPresentPopup: ((UIViewController) -> Void)? | ||
|
||
/// Closure to present a popup web view controller. | ||
|
@@ -40,15 +44,20 @@ class ConnectWebView: WKWebView { | |
/// The instance that will handle opening external urls | ||
let urlOpener: ApplicationURLOpener | ||
|
||
/// The file manager responsible for creating temporary file directories to store downloads | ||
let fileManager: FileManager | ||
|
||
/// The current version for the SDK | ||
let sdkVersion: String? | ||
|
||
init(frame: CGRect, | ||
configuration: WKWebViewConfiguration, | ||
// Only override for tests | ||
urlOpener: ApplicationURLOpener = UIApplication.shared, | ||
fileManager: FileManager = .default, | ||
sdkVersion: String? = StripeAPIConfiguration.STPSDKVersion) { | ||
self.urlOpener = urlOpener | ||
self.fileManager = fileManager | ||
self.sdkVersion = sdkVersion | ||
configuration.applicationNameForUserAgent = "- stripe-ios/\(sdkVersion ?? "")" | ||
super.init(frame: frame, configuration: configuration) | ||
|
@@ -101,6 +110,17 @@ private extension ConnectWebView { | |
func openOnSystem(url: URL) { | ||
urlOpener.openIfPossible(url) | ||
} | ||
|
||
func showErrorAlert(for error: Error) { | ||
// TODO: MXMOBILE-2491 Log analytic when receiving an eror | ||
debugPrint(error) | ||
|
||
let alert = UIAlertController( | ||
title: nil, | ||
message: NSError.stp_unexpectedErrorMessage(), | ||
preferredStyle: .alert) | ||
presentPopup(alert) | ||
} | ||
Comment on lines
+114
to
+123
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Displays "There was an unexpected error -- try again in a few seconds" |
||
} | ||
|
||
// MARK: - WKUIDelegate | ||
|
@@ -158,27 +178,128 @@ extension ConnectWebView: WKNavigationDelegate { | |
_ webView: WKWebView, | ||
decidePolicyFor navigationAction: WKNavigationAction | ||
) async -> WKNavigationActionPolicy { | ||
// TODO: MXMOBILE-2485 Handle downloads | ||
.allow | ||
/* | ||
`shouldPerformDownload` will be true if the request has MIME types | ||
or a `Content-Type` header indicating it's a download or it originated | ||
as a JS download. | ||
|
||
NOTE: We sometimes can't know if a request should be a download until | ||
after its response is received. Those cases are handled by | ||
`decidePolicyFor navigationResponse` below. | ||
*/ | ||
navigationAction.shouldPerformDownload ? .download : .allow | ||
} | ||
|
||
func webView( | ||
_ webView: WKWebView, | ||
decidePolicyFor navigationResponse: WKNavigationResponse | ||
) async -> WKNavigationResponsePolicy { | ||
// TODO: MXMOBILE-2485 Handle downloads | ||
.allow | ||
|
||
// Downloads will typically originate from a non-allow-listed host (e.g. S3) | ||
// so first check if the response is a download before evaluating the host | ||
|
||
// The response should be a download if its Content-Disposition is | ||
// shaped like `attachment; filename=payouts.csv` | ||
if navigationResponse.canShowMIMEType, | ||
let response = navigationResponse.response as? HTTPURLResponse, | ||
let contentDisposition = response.value(forHTTPHeaderField: "Content-Disposition"), | ||
contentDisposition | ||
.split(separator: ";") | ||
.map({ $0.trimmingCharacters(in: .whitespaces) }) | ||
.caseInsensitiveContains("attachment") { | ||
return .download | ||
} | ||
|
||
return .allow | ||
} | ||
|
||
func webView(_ webView: WKWebView, | ||
navigationAction: WKNavigationAction, | ||
didBecome download: WKDownload) { | ||
// TODO: MXMOBILE-2485 Handle downloads | ||
download.delegate = self | ||
} | ||
|
||
func webView(_ webView: WKWebView, | ||
navigationResponse: WKNavigationResponse, | ||
didBecome download: WKDownload) { | ||
// TODO: MXMOBILE-2485 Handle downloads | ||
download.delegate = self | ||
} | ||
} | ||
|
||
// MARK: - WKDownloadDelegate implementation | ||
|
||
@available(iOS 15, *) | ||
extension ConnectWebView { | ||
// This extension is an abstraction layer to implement `WKDownloadDelegate` | ||
// functionality and make it testable. There's no way to instantiate | ||
// `WKDownload` in tests without causing an EXC_BAD_ACCESS error. | ||
|
||
func download(decideDestinationUsing response: URLResponse, | ||
suggestedFilename: String) async -> URL? { | ||
// The temporary filename must be unique or the download will fail. | ||
// To ensure uniqueness, append a UUID to the directory path in case a | ||
// file with the same name was already downloaded from this app. | ||
let tempDir = fileManager | ||
.temporaryDirectory | ||
.appendingPathComponent(UUID().uuidString) | ||
do { | ||
try fileManager.createDirectory(at: tempDir, withIntermediateDirectories: true) | ||
} catch { | ||
showErrorAlert(for: error) | ||
return nil | ||
} | ||
|
||
downloadedFile = tempDir.appendingPathComponent(suggestedFilename) | ||
return downloadedFile | ||
} | ||
|
||
func download(didFailWithError error: any Error, | ||
resumeData: Data?) { | ||
showErrorAlert(for: error) | ||
} | ||
|
||
func downloadDidFinish() { | ||
|
||
// Display a preview of the file to the user | ||
let previewController = QLPreviewController() | ||
previewController.dataSource = self | ||
previewController.modalPresentationStyle = .pageSheet | ||
presentPopup(previewController) | ||
Comment on lines
+263
to
+267
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would we want to just present the share sheet directly here rather than having to fix potential bugs involving previewing certain file types? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could – let me get feedback from the team what the preference is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like we can check if the file is previewable, and if it's not then directly open the share sheet. Really good call out – from the docs:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to merge this but do a fast-follow PR to address this issue There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
} | ||
|
||
// MARK: - WKDownloadDelegate | ||
|
||
@available(iOS 15, *) | ||
extension ConnectWebView: WKDownloadDelegate { | ||
func download(_ download: WKDownload, | ||
decideDestinationUsing response: URLResponse, | ||
suggestedFilename: String) async -> URL? { | ||
await self.download(decideDestinationUsing: response, | ||
suggestedFilename: suggestedFilename) | ||
} | ||
|
||
func download(_ download: WKDownload, | ||
didFailWithError error: any Error, | ||
resumeData: Data?) { | ||
self.download(didFailWithError: error, resumeData: resumeData) | ||
} | ||
|
||
func downloadDidFinish(_ download: WKDownload) { | ||
self.downloadDidFinish() | ||
} | ||
} | ||
|
||
// MARK: - QLPreviewControllerDataSource | ||
|
||
@available(iOS 15, *) | ||
extension ConnectWebView: QLPreviewControllerDataSource { | ||
func numberOfPreviewItems(in controller: QLPreviewController) -> Int { | ||
downloadedFile == nil ? 0 : 1 | ||
} | ||
|
||
func previewController(_ controller: QLPreviewController, previewItemAt index: Int) -> any QLPreviewItem { | ||
// Okay to force-unwrap since numberOfPreviewItems returns 0 when downloadFile is nil | ||
downloadedFile! as QLPreviewItem | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets us test the SDK on iOS 15 devices