[Connect] Enable file downloads #4086

mludowise-stripe · 2024-10-02T07:49:11Z

Summary

Enables file downloads from embedded components
When the file is downloaded, it's previewed in a QLPreviewController and allows the user to save the file or open in an app installed on their system.
Lowers the min supported OS version of the example app to 15.0, the same as the SDK
This enables us to test on iOS 15 devices.
Adds constraints to the example app Launchscreen so the text is centered on all device sizes

Motivation

https://jira.corp.stripe.com/browse/MXMOBILE-2485

Testing

Unit tests
Manual testing (see video)

RPReplay_Final1727924099.MP4

github-actions · 2024-10-02T08:05:30Z

🚨 New dead code detected in this PR:

ConnectWebView.swift: warning: Function 'showErrorAlert(for:)' is unused
ConnectWebView.swift: warning: Function 'download(decideDestinationUsing:suggestedFilename:)' is unused
ConnectWebView.swift: warning: Function 'download(didFailWithError:resumeData:)' is unused
ConnectWebView.swift: warning: Function 'downloadDidFinish()' is unused
ConnectWebView.swift: warning: Function 'download(_:decideDestinationUsing:suggestedFilename:)' is unused
ConnectWebView.swift: warning: Function 'download(_:didFailWithError:resumeData:)' is unused
ConnectWebView.swift: warning: Function 'downloadDidFinish(_:)' is unused

Please remove the dead code before merging.

If this is intentional, you can bypass this check by adding the label skip dead code check to this PR.

ℹ️ If this comment appears to be left in error, double check that the flagged code is actually used and/or make sure your branch is up-to-date with master.

mludowise-stripe · 2024-10-03T03:17:41Z

Dead code check flagged false-positive – all the list methods are delegate methods called from WebKit

mludowise-stripe · 2024-10-03T04:12:52Z

Example/StripeConnectExample/StripeConnect Example.xcodeproj/project.pbxproj

@@ -519,7 +519,7 @@
 				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
 				GCC_WARN_UNUSED_FUNCTION = YES;
 				GCC_WARN_UNUSED_VARIABLE = YES;
-				IPHONEOS_DEPLOYMENT_TARGET = 17.5;


Lets us test the SDK on iOS 15 devices

mludowise-stripe · 2024-10-03T04:13:11Z

Example/StripeConnectExample/StripeConnectExample/API/API.swift

@@ -26,7 +26,7 @@ struct API {
        guard let baseUrl = URL(string: baseURL) else {
            return .failure(.invalidURL)
        }
-        let url = baseUrl.appending(path: path)


appending(path: ) is only available on iOS 16

mludowise-stripe · 2024-10-03T04:16:49Z

Example/StripeConnectExample/StripeConnectExample/Base.lproj/LaunchScreen.storyboard

+                        <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>


Centers the launchscreen text with 20pt horizontal padding

mludowise-stripe · 2024-10-03T04:21:51Z

StripeConnect/StripeConnect/Source/Internal/StripeConnectConstants.swift

@@ -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)


We're not using allowedHosts for downloads because most of our downloads come from external hosts (e.g. S3)

mludowise-stripe · 2024-10-03T04:34:03Z

StripeConnect/StripeConnect/Source/Internal/Webview/ConnectWebView.swift

+    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)
+    }


Displays "There was an unexpected error -- try again in a few seconds"

mludowise-stripe · 2024-10-03T04:35:41Z

StripeConnect/StripeConnectTests/Internal/Webview/ConnectWebViewTests.swift

@@ -112,10 +120,11 @@ class ConnectWebViewTests: XCTestCase {
                                      windowFeatures: .init())

        XCTAssertNil(webView)
-        wait(for: [canOpenURLExpectation, openURLExpectation], timeout: 0.1)
+        wait(for: [canOpenURLExpectation, openURLExpectation], timeout: TestHelpers.defaultTimeout)


I assume this was always intended to use the default timeout? If not, I can change it back.

mludowise-stripe · 2024-10-03T04:37:25Z

StripeConnect/StripeConnectTests/Internal/Webview/ConnectWebViewTests.swift

+    /// `window.close()` should close the view
+    func testWindowCloseDismissesView() {


Added test coverage for existing functionalty

mludowise-stripe · 2024-10-03T04:46:05Z

StripeConnect/StripeConnect/Source/Internal/Webview/ConnectWebView.swift

+    /// File URL for a downloaded file
+    var downloadedFile: URL?
+


@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.

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.

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 :-)

nschris-stripe · 2024-10-04T11:59:59Z

StripeConnect/StripeConnect/Source/Internal/Webview/ConnectWebView.swift

+        // Display a preview of the file to the user
+        let previewController = QLPreviewController()
+        previewController.dataSource = self
+        previewController.modalPresentationStyle = .pageSheet
+        presentPopup(previewController)


Would we want to just present the share sheet directly here rather than having to fix potential bugs involving previewing certain file types?

We could – let me get feedback from the team what the preference is.

Looks like we can check if the file is previewable, and if it's not then directly open the share sheet.
https://developer.apple.com/documentation/quicklook/qlpreviewcontroller/canpreview(_:)

Really good call out – from the docs:

Discussion
If the system can’t display an item, but you still attempt to display it, a Quick Look preview controller displays a generic error. Always check whether you can display an item before choosing to do so.

I'm going to merge this but do a fast-follow PR to address this issue

https://jira.corp.stripe.com/browse/MXMOBILE-2827

mludowise-stripe added 5 commits October 2, 2024 15:15

Use system share sheet for download

e0fa6b2

min deploy target 15.0

ae114fd

preview view

f0ca18f

center launch screen

f217963

tests

e1fcaf5

mludowise-stripe force-pushed the mludowise/MXMOBILE-2485_file_downloads branch from dfe075c to e1fcaf5 Compare October 3, 2024 02:47

mludowise-stripe changed the title ~~[WIP] [Connect] Enable file downloads~~ [Connect] Enable file downloads + lower example app support to 15.0 Oct 3, 2024

mludowise-stripe added the skip dead code check label Oct 3, 2024

mludowise-stripe changed the title ~~[Connect] Enable file downloads + lower example app support to 15.0~~ [Connect] Enable file downloads + open in SafariVC for non-allowlisted redirects Oct 3, 2024

mludowise-stripe added 3 commits October 2, 2024 21:16

fix launchscreen constraints

12a07c5

comments

d50f44b

comments

a4cc8fe

mludowise-stripe commented Oct 3, 2024

View reviewed changes

mludowise-stripe assigned nschris-stripe Oct 3, 2024

mludowise-stripe requested review from kush-stripe and nschris-stripe October 3, 2024 04:40

mludowise-stripe marked this pull request as ready for review October 3, 2024 04:40

mludowise-stripe requested review from a team as code owners October 3, 2024 04:40

mludowise-stripe commented Oct 3, 2024

View reviewed changes

revert blocking non-allowlisted host responses

52915f5

mludowise-stripe changed the title ~~[Connect] Enable file downloads + open in SafariVC for non-allowlisted redirects~~ [Connect] Enable file downloads Oct 3, 2024

mludowise-stripe added 2 commits October 2, 2024 22:14

revert tests

8f651ab

fix

c41dfcc

nschris-stripe approved these changes Oct 4, 2024

View reviewed changes

mludowise-stripe merged commit 7608f9d into master Oct 4, 2024
6 checks passed

mludowise-stripe deleted the mludowise/MXMOBILE-2485_file_downloads branch October 4, 2024 19:04

mludowise-stripe mentioned this pull request Oct 4, 2024

[Connect] Don't preview downloads + better error handling #4099

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Connect] Enable file downloads #4086

[Connect] Enable file downloads #4086

mludowise-stripe commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

mludowise-stripe commented Oct 3, 2024 •

edited

Loading

mludowise-stripe Oct 3, 2024

mludowise-stripe Oct 3, 2024

mludowise-stripe Oct 3, 2024

mludowise-stripe Oct 3, 2024

mludowise-stripe Oct 3, 2024

mludowise-stripe Oct 3, 2024

mludowise-stripe Oct 3, 2024

mludowise-stripe Oct 3, 2024

mxl-stripe Oct 4, 2024

mludowise-stripe Oct 4, 2024

nschris-stripe Oct 4, 2024

mludowise-stripe Oct 4, 2024

mludowise-stripe Oct 4, 2024

mludowise-stripe Oct 4, 2024

mludowise-stripe Oct 4, 2024

		/// `window.close()` should close the view
		func testWindowCloseDismissesView() {

[Connect] Enable file downloads #4086

[Connect] Enable file downloads #4086

Conversation

mludowise-stripe commented Oct 2, 2024 • edited Loading

Summary

Motivation

Testing

github-actions bot commented Oct 2, 2024 • edited Loading

mludowise-stripe commented Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mludowise-stripe commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

mludowise-stripe commented Oct 3, 2024 •

edited

Loading