Refactored Instant Upload #681
Conversation
… background upload preference has now been seperated from the InstantUpload preference. Instant Upload now uses the Photos Library instead of the Assets library for photo access. Instant upload last uploaded timestamp is now stored as an NSTimeInterval rather than a long.
Jon-Schneider
changed the title
|
Thanks a lot for your contribution! Alternatively you can add a comment here where you state that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/ |
|
@Jon-Schneider please, check again your github email address |
|
@nasli There may have been an error earlier. Both commits show as being from Jon-Schneider on my end. |
|
@DeepDiver1975 could you check what is wrong? Thank you! |
|
|
Clear cache button and log out account
|
@Jon-Schneider please rebase this branch |
… background upload preference has now been seperated from the InstantUpload preference. Instant Upload now uses the Photos Library instead of the Assets library for photo access. Instant upload last uploaded timestamp is now stored as an NSTimeInterval rather than a long.
|
@nasli Rebased. |
|
Thanks a lot for your contribution! Alternatively you can add a comment here where you state that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/ |
|
|
||
| #pragma mark - Utility | ||
|
|
||
| - (void) showPhotoLibraryAccessPermissionDeniedAlert { |
There was a problem hiding this comment.
I would suggest to instead of use this three methods for any alertview code use just one method like
- (void) showAlertView:(NSString*)string{
UIAlertView *alertView = [[UIAlertView alloc] initWithTitle:string message:@"" delegate:self cancelButtonTitle:NSLocalizedString(@"ok", nil) otherButtonTitles:nil, nil];
[alertView show];
}
and called
[self showAlertView:NSLocalizedString(@"access_photos_library_not_enabled", nil)];
This would make also more easier the refactor that we plan to do it in a near future to the new UIAlertController instead of the deprecated UIAlertView.
There was a problem hiding this comment.
Well, since you are using title and body use this method that you are not using
(void) showAlertViewWithTitle:(NSString *)title body:(NSString *)body{
instead of the other two
showLocationAccessAlwaysPermissionDeniedAlert
showLocationAccessAlwaysPermissionDeniedAlert
…be newer than to be uploaded to the current datetime. This has the effect of only uploading photos taken after Instant Upload is enabled
|
@nasli Both done. |
|
Great!! Nice work @Jon-Schneider!! Let's move this to ready to test @jesmrec |
|
@jesmrec I'm new here. What does testing involve? How can I help move the process along? |
|
@Jon-Schneider hi!! Today i will start with these tasks. The test plan will be uploaded to the QA repo (https://github.com/owncloud/QA) and i will link it to this PR to make easier the access. If bugs or problems are found during the process i will post or link them here, if you want to review them :) . All comments or review about the test plan will be always welcome!!! Thank you very much for your contribution. |
|
I have detected the following behaviour: 1.- Disable location services Actual Behaviour If you try again to enable "Background Instant Upload", the switch button is enabled without requesting to enable location services. If you change the view and then return to settings, the switch button is disabled. Expected behaviour If location services are disabled in device (or for ownCloud), the option "Background Instant Upload" can not be enabled in any case without requesting it to the user. |
|
@Jon-Schneider apart of the problem mentioned in my last post, it works really fine!! great job :) |
|
@jesmrec you are right, #681 (comment), |
|
I'll check it out. What should be happening if you hit the switch after location permissions have been denied is the delegate method backgroundInstantUploadPermissionLostOrDenied is called immediately and the switch is flipped back off. |
…n permission, could flip the 'Background Instant Upload' Settings option to true.
|
@Jon-Schneider right!! now working fine. That was the only problem, so this PR is already validated and ready to be merged. |
|
@nasli Am I allowed to merge this once approved by QA or does project staff prefer to do it? |
|
@Jon-Schneider in regular basis, if possible, we'd rather than the pr is merged by a different person to the one who launched the pr, so that we ensure that the pr has been reviewed, does this make sense? Right now, there is something else that we have to take into account, @jesmrec has just noticed one, important, extra test case to check: versions upgrade. He is currently checking it. |
|
Indeed, if it's ok for you we may ping you to review pr too |
|
If you wanted me to test migration in addition to @jesmrec, I did test version upgrade from latest to this PR during my own testing without issue. I haven't touched the data model since then, but if you find any edge cases let me know and I'll fix them. |
|
Checks about upgrading:
All OK. |
|
👍 Great!! Ready! |
|
@nasli Just so you know, this merge request was actually into clear_cache_button, not master, so clear_cache_button will need merged into master again. |
|
@Jon-Schneider Fixed the conflicts with master and waiting for travis 👍 |
|
@Jon-Schneider could you launch this PR into master branch? thanks! |
|
Travis failed, and since the branch clear cache is already in master an strange behaviour happens, and not detect all the conflicts. |
This Pull Request Does the Following:
TASKS
This closes #511
BUGS / IMPROVEMENTS: