Support hidden version in status.php taking version from capabilities… by davivel · Pull Request #1936 · owncloud/android

davivel · 2017-04-07T12:59:42Z

… API as preferred.

Fixes #1931

davigonz · 2017-04-10T06:57:05Z

src/com/owncloud/android/authentication/AccountUtils.java

-            if (serverVersionStr != null) {
-                serverVersion = new OwnCloudVersion(serverVersionStr);
+            // capabilities are now the preferred source for version info
+            FileDataStorageManager fds = new FileDataStorageManager(


I would rename fds variable name to a more specific one like fileDataStorageManager. In this part of the code, you know exactly what it is, but if you use it in other part below, you won't know it.

davigonz · 2017-04-10T07:36:54Z

src/com/owncloud/android/authentication/AccountUtils.java

+                }
            }
        }
        return serverVersion;


This serverVersion could be returned as null, I guess this will be take into account in the method which calls it

Added @nullable to method to make it clearer to callers; they had it into account before, also.

davigonz · 2017-04-10T07:37:54Z

src/com/owncloud/android/operations/DetectAuthenticationMethodOperation.java

+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * <p>


Not important, but these <p> characters could be deleted

davigonz · 2017-04-10T07:39:10Z

src/com/owncloud/android/operations/GetServerInfoOperation.java

+ * @author David A. Velasco
+ * @author masensio
+ * Copyright (C) 2017 ownCloud GmbH.
+ * <p>


davigonz · 2017-04-10T07:51:04Z

src/com/owncloud/android/operations/RefreshFolderOperation.java

-        RemoteOperationResult  result = getCapabilities.execute(getStorageManager(), mContext);
-        if (!result.isSuccess()){
-            Log_OC.w(TAG, "Update Capabilities unsuccessfully");
+    private OwnCloudVersion updateCapabilities() {


I'm not sure if updateCapabilities would be a good name for this method since you are getting something, not setting it. Besides, this method could be also useful in other parts of the code, have you thinked about moving it to a generic capabilities class with several methods related to it?

Edition: I've just seen that SyncCapabilitiesOperation is getting and saving the capabilities. Method name LGTM but consider if moving it to a capabilities util class would worth it.

I renamed the method to 'syncCapabilities()', that is more accurate.

Reading the code of the method I really hope we never need to reuse this in any other place. First, it returns an OwnCloudVersion instance as a 'side effect' of syncing capabilities, instead of returning capabilities instance, so the name of the method is confusing. Second, it checks the AccountManager for the version as a fallback for servers that do not respond capabilities (or don't support them), code that I would prefer to get rid of because users just should update to servers with capabilities API instead of staying with elder ones - but I don't trust enough that if we forget that possibility it will not bite us back.

Besides, util classes, the lesser, the better. Static methods are perfect to trigger career conditions in multithreaded environments. Or to dump all kind of stuff in without thinking twice.

EDIT: renamed again so that returned value is not so confusing.

davivel · 2017-04-10T11:29:39Z

@davigonz , updated.

jesmrec · 2017-04-12T06:51:55Z

Pending on the server PR: owncloud/core#27473

davivel · 2017-04-17T06:53:00Z

@jesmrec , @davigonz , server PR was merged.

davivel · 2017-04-17T11:09:17Z

Trying to work around the weird block of repeated license/cla check. Will close the PR and create a new one.

davivel · 2017-04-17T11:14:10Z

This is stupid.

@DeepDiver1975 , please, take a look to repo configuration. We can't merge recent PRs. Same problem in the iOS repo.

jesmrec · 2017-04-17T11:28:45Z

QA Approved.

davivel · 2017-04-17T11:32:30Z

By the way: the wrong check says "licence/cla" while the correct one say "license/cla" : licence Vs license . I suspect has something to do with the block.

davivel · 2017-04-17T11:41:21Z

@DeepDiver1975 , I suspect that in the repository configuration the protection of master branch is requiring a test with context licence/cla instead of license/cla. That was probably changed recently, and is blocking the PRs.

Please, take a look when possible.

… API as preferred

davivel requested review from davigonz and jesmrec April 7, 2017 12:59

davivel mentioned this pull request Apr 7, 2017

Allow the usage of OwnCloudVersion instances for invalid server versi… owncloud/android-library#156

Merged

davigonz suggested changes Apr 10, 2017

View reviewed changes

davivel force-pushed the support_hidden_version_in_status_php branch 3 times, most recently from a41088b to d653999 Compare April 10, 2017 11:25

davigonz approved these changes Apr 11, 2017

View reviewed changes

davivel force-pushed the support_hidden_version_in_status_php branch from d653999 to 2d98698 Compare April 11, 2017 08:09

davivel force-pushed the support_hidden_version_in_status_php branch 2 times, most recently from b9d8694 to 7447cd1 Compare April 17, 2017 10:54

davivel added this to the 2.4.0 milestone Apr 17, 2017

davivel closed this Apr 17, 2017

davivel reopened this Apr 17, 2017

jesmrec added the Approved by qa label Apr 17, 2017

jesmrec approved these changes Apr 17, 2017

View reviewed changes

Support hidden version in status.php taking version from capabilities…

3765244

… API as preferred

davivel force-pushed the support_hidden_version_in_status_php branch from 7447cd1 to 3765244 Compare April 17, 2017 11:49

davivel merged commit d801018 into master Apr 17, 2017

davivel deleted the support_hidden_version_in_status_php branch April 17, 2017 12:27

Conversation

davivel commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davivel Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davivel commented Apr 10, 2017

Uh oh!

jesmrec commented Apr 12, 2017

Uh oh!

davivel commented Apr 17, 2017

Uh oh!

davivel commented Apr 17, 2017

Uh oh!

davivel commented Apr 17, 2017

Uh oh!

jesmrec commented Apr 17, 2017

Uh oh!

davivel commented Apr 17, 2017

Uh oh!

davivel commented Apr 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davivel commented Apr 7, 2017 •

edited

Loading

davivel Apr 10, 2017 •

edited

Loading