Conversation
…traints only - enhanced remove uploads with no accounts associated - use List<OCUpload> instead of OCUpload[] Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
|
master-Unit test failed, but no output was generated. Maybe a preliminary stage failed. |
|
stable-Unit test failed, but no output was generated. Maybe a preliminary stage failed. |
|
stable-Screenshot test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/14200-stable-Screenshot |
|
master-Screenshot test failed, but no output was generated. Maybe a preliminary stage failed. |
| } | ||
|
|
||
| public void setWhileChargingOnly(boolean whileChargingOnly) { | ||
| public OCUpload setWhileChargingOnly(boolean whileChargingOnly) { |
| return; | ||
| } | ||
|
|
||
| // always clean up |
There was a problem hiding this comment.
...except when there is no internet or power-saving
--> the following removal call could just be executed no matter the above (doesn't hurt the way it is now either)
There was a problem hiding this comment.
try { ... } finally { ... } can be used to deal with such situation.
|
looking good, just some minor comments |
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
This reverts commit ddf7176.
retry failed uploads only once Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/14258.apk |
Codacy392Lint
SpotBugs (new)
SpotBugs (master)
SpotBugs increased! |
| } | ||
|
|
||
| @Test | ||
| public void testConstraintsWithFailedUploads() { |
There was a problem hiding this comment.
This test is pretty intimidating by it's size. I'd consider splitting that into smaller units.
| @@ -988,61 +997,61 @@ public static void retryUpload(@NonNull Context context, @NonNull Account accoun | |||
| * Retry a subset of all the stored failed uploads. | |||
| * | |||
| * @param context Caller {@link Context} | |||
There was a problem hiding this comment.
Superficial param description - safe to remove maybe?
| * Get all uploads which where successfully completed. | ||
| */ | ||
| public OCUpload[] getFinishedUploads() { | ||
| public void removeUploadsWithExpiredUsers(List<Account> existingAccounts) { |
There was a problem hiding this comment.
Seems to be a new method. Would it be possible to use List<User> instead of Account?
| return; | ||
| } | ||
|
|
||
| // always clean up |
There was a problem hiding this comment.
try { ... } finally { ... } can be used to deal with such situation.
4 participants
first check constraints, then get failed uploads based on this constraints only
enhanced remove uploads with no accounts associated
use List instead of OCUpload[]
Signed-off-by: tobiasKaminsky tobias@kaminsky.me
Testing
Writing tests is very important. Please try to write some tests for your PR.
If you need help, please do not hesitate to ask in this PR for help.
unit tests
instrumented tests
UI tests