Migrate simple cases of get current account to get user #4853
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ezaquarii
force-pushed
the
migrate-simple-cases-of-get-current-account-to-get-user
branch
from
097addd to
e6fc540
Compare
|
IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/11661 |
ezaquarii
force-pushed
the
migrate-simple-cases-of-get-current-account-to-get-user
branch
2 times, most recently
from
0779a6b to
84b0849
Compare
ezaquarii
changed the title
|
I guess this needs to be rebased on master, as newUser PR is now in. |
ezaquarii
force-pushed
the
migrate-simple-cases-of-get-current-account-to-get-user
branch
from
84b0849 to
faa9d77
Compare
Codecov Report
@@ Coverage Diff @@
## master #4853 +/- ##
============================================
+ Coverage 17.62% 17.69% +0.06%
Complexity 3 3
============================================
Files 383 384 +1
Lines 32509 32528 +19
Branches 4587 4588 +1
============================================
+ Hits 5731 5757 +26
+ Misses 25857 25848 -9
- Partials 921 923 +2
|
ezaquarii
force-pushed
the
migrate-simple-cases-of-get-current-account-to-get-user
branch
from
522e79f to
33afcbc
Compare
ezaquarii
force-pushed
the
migrate-simple-cases-of-get-current-account-to-get-user
branch
3 times, most recently
from
c3e5f70 to
dc4f44a
Compare
|
Unit test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/11719 |
ezaquarii
force-pushed
the
migrate-simple-cases-of-get-current-account-to-get-user
branch
2 times, most recently
from
c2473b4 to
436b564
Compare
|
Unit test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/11725 |
| boolean isSearchSupported(@Nullable Account account); | ||
|
|
||
| @Deprecated |
There was a problem hiding this comment.
can you add here, what to use instead?
There was a problem hiding this comment.
JavaDoc updated.
| public OwnCloudClient create(User user) throws CreationException { | ||
| try { | ||
| return OwnCloudClientFactory.createOwnCloudClient(user.toPlatformAccount(), context); | ||
| } catch (OperationCanceledException|AuthenticatorException|IOException|AccountUtils.AccountNotFoundException e) { |
There was a problem hiding this comment.
as this is too long, please also line wrap it
There was a problem hiding this comment.
Long exceptions list reformatted into vertical.
| @@ -217,8 +214,8 @@ private Cursor searchForUsersOrGroups(Uri uri) { | |||
| Uri remoteBaseUri = new Uri.Builder().scheme(CONTENT).authority(DATA_REMOTE).build(); | |||
| Uri emailBaseUri = new Uri.Builder().scheme(CONTENT).authority(DATA_EMAIL).build(); | |||
|
|
|||
| FileDataStorageManager manager = new FileDataStorageManager(account, getContext().getContentResolver()); | |||
| boolean federatedShareAllowed = manager.getCapability(account.name).getFilesSharingFederationOutgoing() | |||
| FileDataStorageManager manager = new FileDataStorageManager(user.toPlatformAccount(), getContext().getContentResolver()); | |||
| Account account = accountManager.getCurrentAccount(); | ||
| GetActivityListTask getActivityListTask = new GetActivityListTask(account, accountManager, lastGiven, callback); | ||
| User user = accountManager.getUser(); | ||
| GetActivityListTask getActivityListTask = new GetActivityListTask(user.toPlatformAccount(), accountManager, lastGiven, callback); |
src/main/java/com/owncloud/android/ui/adapter/TrashbinListAdapter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/owncloud/android/ui/dialog/ChooseTemplateDialogFragment.java
Outdated
Show resolved
Hide resolved
src/main/java/com/owncloud/android/ui/fragment/ExtendedListFragment.java
Show resolved
Hide resolved
src/main/java/com/owncloud/android/ui/trashbin/RemoteTrashbinRepository.java
Outdated
Show resolved
Hide resolved
|
Nice! Again this is a great PR! Some very small issues and then we can merge. |
Migrate trivially convertible uses of getCurrentAccount() to new user model - getUser(). Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>
ezaquarii
force-pushed
the
migrate-simple-cases-of-get-current-account-to-get-user
branch
from
1c2ae46 to
a7eb714
Compare
|
@tobiasKaminsky Please bear in mind that #4884 is causing CI to fail sporadicaly. Just restart the build if this particular test fails. |
… [skip ci] Signed-off-by: nextcloud-android-bot <android@nextcloud.com>
|
Issues
======
+ Solved 3
- Added 3
Complexity increasing per file
==============================
- src/main/java/com/nextcloud/client/network/ClientFactoryImpl.java 2
- src/main/java/com/owncloud/android/ui/trashbin/RemoteTrashbinRepository.java 1
- src/main/java/com/owncloud/android/ui/activity/NotificationsActivity.java 2
Complexity decreasing per file
==============================
+ src/main/java/com/owncloud/android/providers/UsersAndGroupsSearchProvider.java -2
+ src/main/java/com/owncloud/android/ui/activities/data/files/FilesServiceApiImpl.java -3
See the complete overview on Codacy |
| String accountName = getIntent().getExtras().getString(NotificationJob.KEY_NOTIFICATION_ACCOUNT); | ||
| if(accountName != null && optionalUser.isPresent()) { | ||
| User user = optionalUser.get(); | ||
| if (user.getAccountName().equalsIgnoreCase(accountName)) { |
There was a problem hiding this comment.
| if (optionalUser.isPresent()) { | ||
| mLastDisplayedAccount = optionalUser.get().toPlatformAccount(); | ||
| } else { | ||
| mLastDisplayedAccount = null; |
There was a problem hiding this comment.
| Optional<User> optionalUser = getUser(); | ||
| if (optionalUser.isPresent() && accountName != null) { | ||
| User user = optionalUser.get(); | ||
| if (!accountName.equalsIgnoreCase(user.getAccountName())) { |
There was a problem hiding this comment.
|
IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/11734 |
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/11743.apk |
|
@tobiasKaminsky |
Codacy312Lint
SpotBugs (new)
SpotBugs (master)
|
AndyScherzinger
approved these changes
Nov 26, 2019
|
Ha finally greeeen :-) |
delete-merged-branch
bot
deleted the
migrate-simple-cases-of-get-current-account-to-get-user
branch
tobiasKaminsky
added a commit
that referenced
this pull request
Nov 27, 2019
e92daa2 Merge pull request #4853 from nextcloud/migrate-simple-cases-of-get-current-account-to-get-user 99d9a69 Merge pull request #4885 from nextcloud/disable_android_backup_qa 10960bf [tx-robot] updated from transifex 9f24b25 [tx-robot] updated from transifex b7876c4 disable GoogleAppIndexingWarning lint check 11687c6 Drone: update FindBugs results to reflect reduced error/warning count [skip ci] 80fb800 Merge commit 'a7eb7148fa0ceb42981366eb2ddcf0ff921e6a55' b91136c Disable android backup on the QA app a7eb714 Migrate simple cases of getCurrentAccount() to getUser() 6249a06 daily dev 20191123
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This branch is an attempt to migrate all trivial use cases of
UserAccountManager.getCurrentAccount()toUserAccountManager.getUser(), leveragingUser.toPlatformAccount()andUser.toOwnCloudAccount()APIs whenever refactoring becomes problematic or too risky.Since
UserAccountManageris injected in top-level component, such asActivities(top), this PR shall open path to migration of internal components, deeper in the object hierarchy (bottom).My initial plan to start from internal componentsand taking migration up to
Activity/Fragment/Servicelevel - botton-top approach - turned out to not be feasible, because various internal componetns require refactoring of the callers anyway (top) atd those refactorings spill horribly all over the codebase.I try to keep the risk of this change in reasonable bounds, but it will require complehensive application testing nevertheless.