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.

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

mludowise-stripe
Copy link
Collaborator

@mludowise-stripe mludowise-stripe commented Oct 2, 2024

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

Copy link

github-actions bot commented Oct 2, 2024

🚨 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 mludowise-stripe force-pushed the mludowise/MXMOBILE-2485_file_downloads branch from dfe075c to e1fcaf5 Compare October 3, 2024 02:47
@mludowise-stripe 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
Copy link
Collaborator Author

mludowise-stripe commented Oct 3, 2024

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

@mludowise-stripe 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
@@ -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;
Copy link
Collaborator Author

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

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

Choose a reason for hiding this comment

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

appending(path: ) is only available on iOS 16

Comment on lines +29 to +33
<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>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Centers the launchscreen text with 20pt horizontal padding

@@ -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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines +135 to +144
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)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@@ -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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Comment on lines +147 to +148
/// `window.close()` should close the view
func testWindowCloseDismissesView() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added test coverage for existing functionalty

Comment on lines +23 to +25
/// File URL for a downloaded file
var downloadedFile: URL?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mludowise-stripe mludowise-stripe changed the title [Connect] Enable file downloads + open in SafariVC for non-allowlisted redirects [Connect] Enable file downloads Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants