Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

Add support for batch file uploading #4 #5

Closed
wants to merge 2 commits into from

Conversation

akosel
Copy link
Contributor

@akosel akosel commented Sep 3, 2015

I noticed the open issue, and figured I would take a crack at it since I would like to have this functionality. Also haven't used angular in a little while, so figured I would brush up on the docs.

Thanks for putting together such a slick interface! Let me know if there is anything I can do to improve the PR.

Push to promises

Missing semicolon

A few more missing semicolons.
@zombiezen
Copy link
Owner

Hi Aaron! Thanks for the patch and sorry for the delay in response. Two things:

  1. I will need you to sign the Google CLA before I can incorporate your patch: https://cla.developers.google.com/
  2. From a technical side: I'd like to have a more friendly way of handling the retries in the case of multiple files. I'm imagining that there should only be one toast which says either: "Uploaded X files" on success, or "Uploaded X files, Y files failed [RETRY]" on failure. Otherwise, the user has to hunt down a bunch of toasts instead of just one.

@akosel
Copy link
Contributor Author

akosel commented Sep 16, 2015

Yes, I agree with your recommendation. We will need to gate the initial batch upload in the function passed to retriableAction. That will allow better control over the fail/ok messages, which now need to have more knowledge of whether the action failed completely, partially succeeded (which is new), or completely succeeded. That will allow multiple rounds of failures, terminating when the user gives up, or all original files are uploaded.

As an update, I'm on this. Should be done in the next couple days.

@akosel
Copy link
Contributor Author

akosel commented Sep 18, 2015

Ok, I put together a solution to retry handling. The solution manages the user messages via the response passed to the deferred's resolve/reject method. This allows the messages to provide information based on the number of files successfully/unsuccessfully uploaded. By clicking 'Retry' the user is then able to attempt to upload the files that failed (unless they all the toast to time out). I do have a couple thoughts/questions in regards to this:

First, the retriable action is intended to be idempotent, which is compromised if they can leave six files uploaded out of twelve total, say. Granted they can keep attempting to upload files until they give up, but there is no guarantee that all files will be uploaded. Technically, the same set of inputs could lead to a different output. We could enforce a strict requirement that all original files must be uploaded successfully to be stored, but that seems overly strict. Do we want to loosen the idempotence requirement on retriable action or add a new function that reflects the wider array of possibilities with batch operations.

A second thought: file sizes will vary, potentially significantly. It could be useful to have a simple heuristic that groups files within a batch together to handle this. For example, if we had ten ~4MB files, one 500MB, and one 1GB file, we could send the "Uploaded X files, Y files failed" message per tier, rather than for the entire group.

Final thought: I noticed that the $mdToast doesn't seem to be able to reach the fail state (unless I am missing something). It can timeout or it can be dismissed via user action, but I am not sure how it can fail. I found this issue which indicates that promise response is "ok" if the user clicks 'Retry' and true if it times out. I made a modification to reflect this (to keep the retriable action from continuously running until the success condition is met, regardless of timeouts). I wrote a comment in the code, but I am wondering if it makes sense to have a dialog here so the user is forced to act if the action fails. I can imagine a case where someone is uploading a large file and not paying attention to the page. They might miss the message.

I'm happy to help with all of the above items now, or in a later PR. I want to get your opinions before moving forward on them. I'm enjoying working on this!

@zombiezen
Copy link
Owner

Hi Aaron! (Sorry for delayed response; my day job's been busy.)

My thinking at the moment is that the toast is probably too limiting for exactly the reasons you mention (and doesn't show progress, as mentioned in #6). What would be ideal to have is an operations dialog, a la Google Drive or Cloud Console, that shows progress of all in-flight files.

What I'm seeing as the steps for this (ideally, each would be a separate pull for easier review):

  • Update the filedrop.Files.upload method to return a promise that notifies about progress events. This will probably need to use an XMLHttpRequest directly, since Angular's $http service doesn't appear to set it up.
  • Create a new filedrop.UploadManager service that maintains a list of all the file upload operations in progress.
  • Create the UI+controller that displays the information from filedrop.UploadManager.

Do you agree with this approach? Would you be willing to tackle this?

@akosel
Copy link
Contributor Author

akosel commented Sep 29, 2015

Hi Ross,

No worries on any delays! I agree with the approach. I am happy to work on this, but I will probably not have much of a chance to work on it until mid-October. If that's alright, then I'm on it!

@zombiezen
Copy link
Owner

SGTM. I might pick off some of the smaller parts in the meantime as I get time. No worries on timeframe. :)

@akosel
Copy link
Contributor Author

akosel commented Dec 1, 2015

I am going to close this one as I believe this functionality will fall out of the UploadManager quite nicely.

@akosel akosel closed this Dec 1, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants