Stash upload #2505
Stash upload #2505
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2505 +/- ##
=========================================
+ Coverage 2.69% 5.87% +3.17%
=========================================
Files 258 259 +1
Lines 12266 12366 +100
Branches 1108 1112 +4
=========================================
+ Hits 331 726 +395
+ Misses 11909 11581 -328
- Partials 26 59 +33
Continue to review full report at Codecov.
|
|
@whym Is the PR ready for review or is it still WIP? :) |
|
@maskaravivek It's ready, as far as I'm concerned. Sorry about the confusing commit summaries - they should be removed when the commits are squashed. |
|
Conflict resolved and tested again with betaDebug. |
|
@whym Can you add the test plan for the changes you made. I tried picking one image from the gallery and I was expecting to see the upload begin(based on logcat) but it doesn't seem to happen. |
|
Thank you for testing @maskaravivek. I tried uploading from camera last time, but when I tried it today it got stuck at "finishing uploading", and here was the log:
It sounds like a server side probem ("Could not connect to storage backend"), hopefully a temporary one, but it's also possible that I might have introduced bugs during when I resolved conflicts with recent changes in master (which were a bit complex). Did you get the same error? I tried prodDebug and upload attempts went through with this. Prod uses a different (and more stable) server. Before today's testing, I included latest master (again) using "git rebase master" into this branch. |
|
For me, the upload wasn't initiated at all. Let me try again today with both camera and gallery. |
|
I tested with two real devices I have (API 27 and API 23). In the end both worked, but I had some (non-reproducible) issues.
The tests were done for both the Camera and Gallery methods. |
|
Resolved conflicts with recent changes to master.
|
- split upload process and use stash - resolve filename conflict after upload not before - use NotificationManagerCompat; add notification tag; assign temporaty stash file name
|
Conflicts resolved again. (Updates for AndroidX #2594) It passed the same (manual) tests as above. |
|
I tested it once again and the upload worked for me. But I am not sure about the testing steps. This is what I did:
After these steps, the upload begins. Is this expected? From the description, it seems that a stashed upload should begin in the background as soon as I pick the image and meanwhile I can fine-tune the title and other details. So I was expecting upload to begin as soon as I landed on upload activity. @whym It would be great if you could briefly explain how to test the changes. It is sad that this PR has been waiting for so many days and you have already rebased the PR multiple times. |
That is my intention, but most of the plan will be done in subsequent PR(s) I am still preparing. It seems like I should have been clearer - the expected behavior for now is to be able to upload pictures as normally as before, nothing more. There is no noticeable change for users, including the timing to start uploading. At this time, I just wanted to make sure we can upload stably with the stash API first. I'm glad that it worked for you. The only change here is replacing the traditional upload API with two API calls - 1. API call to upload a file without a real title, 2. API call to give a name to the file. Both happen after the user finishes entering image details (for now). |
|
To think about it, the question is whether I should really ask for merging this, or I should combine it into the larger and more substantial change and resubmit. (This review allowed more tests than I did before, so I don't think it was wasted anyway.)
I initially preferred separate PRs (=1) since this is something new to a core functionality of the app. But maybe merging this partial solution is too confusing? |
|
I vote for 1.
It will allow the stash API to get proven in real life :-)
…On Mon, Mar 18, 2019 at 7:31 PM Yusuke Matsubara ***@***.***> wrote:
To think about it, the question is whether I should really ask for merging
this, or I should combine it into the larger and more substantial change
and resubmit. (This review allowed more tests than I did before, so I don't
think it was wasted anyway.)
1. Get this merged now and see if any unexpected issue with the stash
API arises while I work on the background upload for a next PR.
2. Abandon this and comeback later with a full background upload
implementation.
I initially preferred separate PRs (=1) since this is something new to a
core functionality of the app. But maybe merging this partial solution is
too confusing?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#2505 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGFBkuRkBuyGKizkBX4L3N0hn6vFUD9ks5vX2r0gaJpZM4bJg-7>
.
|
|
I am also in favor of merging it first. Am merging it so that alpha testers can report issues if any. |
Description (required)
Fixes #860 - Use stash to upload
This decouples (1) the uploading of the file content and (2) the fine-tuning of the file name. It's a first step before moving forward the idea of background uploading while the user entering the file name (this is possible because stash upload doesn't require a proper file name in the first part).
No UI changes included here, just changes in the backend part of the upload process. It seemed like a good idea to fix potential bugs before moving onto the next step (the forementioned background uploading).
Tests performed (required)
Tested ProdDebug on emulator with API level 24.
Screenshots showing what changed (optional - for UI changes)
No UI changes.