-
Notifications
You must be signed in to change notification settings - Fork 657
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 SDK] Enable downloads in the Connect SDK #9583
Conversation
Diffuse output:
APK
|
ae703b3
to
eeb1657
Compare
0f24247
to
4c9c1b9
Compare
7f250fc
to
e667558
Compare
} | ||
|
||
private fun showErrorToast() { | ||
MainScope().launch { |
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.
i'd pass in the main scope like you're doing with io Scope to make it more testable
context.getSystemService(Context.DOWNLOAD_SERVICE) as? DownloadManager, | ||
private val ioScope: CoroutineScope = CoroutineScope(Dispatchers.IO), | ||
// methods below exposed for mocking in tests | ||
private val getDownloadManagerRequest: (Uri) -> DownloadManager.Request = { uri -> |
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.
passing in all these lambdas feels a bit strange here. is there some kind of interface you can use here to abstract away the OS level functionality? that way you can just mock the interface.
This will likely make the code a lot easier to follow too
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.
Maybe abstracting away and creating components for the following? Then this service could call StripeDownloadManager and you could mock all of these for your tests
StripeDownloadManager:
- startDownload(url, ua, ...,) : DownloadMetadata
- getDownloadProgress(Id): DownloadStatus
DownloadMetadata:
- id
- fileName
ToastManager
- showToast(string)
val downloadId = downloadManager.enqueue(request) | ||
|
||
// Monitor the download progress and show a toast when done | ||
val query = DownloadManager.Query().setFilterById(downloadId) |
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.
this static method feels like it should be behind some kind of interface if we want this class to be testable
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.
I think the interface for the DownloadListener
can be cleaned up a bit here. It doesn't have to be what i'm suggesting, but I think it's worth taking a stab at. Let me know what you think
Good point, I think abstracting these behind an interface is a good idea. @seankenkeremath-stripe I've updated the code and I find it's more idiomatic and easier to read now, but still pretty lightweight - let me know what you think |
} | ||
|
||
private fun showErrorToast() { | ||
mainScope.launch { |
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.
you may actually just want to make the toast manager call mainScope so you don't need to worry about it here. it will never be called on any other thread anyway
/** | ||
* Returns a [DownloadManager.Request] for the given URI. | ||
*/ | ||
fun getDownloadRequest(uri: String): DownloadManager.Request |
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.
does this need to be part of the interface? sort of feels like an implementation detail
/** | ||
* Returns a best-guess file name for the given URL, content disposition, and MIME type. | ||
*/ | ||
fun getFileName(url: String, contentDisposition: String? = null, mimeType: String? = null): String |
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.
same with this one, with both of these it seems like the end result is just that you want the file name, and i'm wondering if you could just return that along with the ID when you start the DL request
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.
Looks good! A few non-blocking suggestions
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.
nice 👍
Summary
Adds a download listener so that when the user attempts to download a file via the webhook, it downloads in the background and shows a toast when it's done.
Note that we don't need storage permissions because the downloading is delegated to a system service that has higher permissions: https://developer.android.com/reference/android/app/DownloadManager.Request#setDestinationUri(android.net.Uri)
Motivation
Certain embedded components, such as the Payouts Component, provide the user the ability to download files.
Testing
Screenshots
demo-1731262880.mp4