refactor: [DC-324] decouple retrieving server settings and user info from connection validator routines (#12619)

* first pass for moving the FetchServerSettingsJob to AccountState

it leaks at the moment as there are issues with deleting the job on finished signal.

that needs to be fixed in the job by ensuring the "asyncUpdates" are finished before the finished signal is emitted.

alternately can call those extra operations from the AccountState but I think having it all in the job is simpler.

* this works but may have side effects

committing it for discussion with cohort

* works now but..

this is a very questionable impl. The core problem is not being able to simply delete the settings job, in tandem with questionable "need" to have that be null by the time the folders are enqueued.

will discuss next week and clean it up

* lots of cleanup

also refined the caps/user info/avatar/appprovider retrieval to only happen on start and re-auth.

needs a bit more testing then hoping it's done

* final cleanup and refinement

also renamed the very misleading FetchServerSettingsJob to FetchServerSettingsRunner.

by unanimous decision we just killed the previous caps related tests in ConnectionValidator since it is no longer in play there.

* added changelog

* grrrr

Author: modSpike

changelog/unreleased/12619.md

@@ -0,0 +1,5 @@
+Change: Eliminated excessive server checks
+
+The server capabilities, user settings, avatar and app providers are now updated only on application start and after the user has re-authenticated during a running session. 
+
+https://github.com/owncloud/client/pull/12619

src/gui/accountstate.cpp

@@ -16,7 +16,8 @@
 #include "application.h"
 #include "configfile.h"
 
-#include "fetchserversettings.h"
+// #include "fetchserversettings.h"
+#include "libsync/networkjobs/jsonjob.h"
 
 #include "libsync/creds/abstractcredentials.h"
 
@@ -56,6 +57,7 @@ AccountState::AccountState(Account *account)
     , _waitingForNewCredentials(false)
     , _connectionValidator(nullptr)
     , _maintenanceToConnectedDelay(1min + minutes(QRandomGenerator::global()->generate() % 4)) // 1-5min delay
+    , _needsServerSettingsRefresh(true)
 {
     qRegisterMetaType<AccountState *>("AccountState*");
 
@@ -187,29 +189,97 @@ void AccountState::setState(State state)
         }
     }
 
-    // might not have changed but the underlying _connectionErrors might have
+    // only do this once when the state actually changes from something to connected
+    // todo: need to investigate whether it's ever the case that we go from connected to connected.
+    // so far it looks to me as if this happens when the connection validator confirms all is still well?
     if (_state == Connected) {
-        QTimer::singleShot(0, this, [this, oldState] {
-            // ensure the connection validator is done
-            _queueGuard.unblock();
+        if (_needsServerSettingsRefresh) {
             // update capabilities and fetch relevant settings
-            _fetchCapabilitiesJob = new FetchServerSettingsJob(_account, this);
-            connect(_fetchCapabilitiesJob.get(), &FetchServerSettingsJob::finishedSignal, this, [oldState, this] {
-                // Lisa todo: I do not understand this logic at all - review it
-                if (oldState == Connected || _state == Connected) {
-                    _fetchCapabilitiesJob.clear();
-                    Q_EMIT isConnectedChanged();
-                }
-            });
-            _fetchCapabilitiesJob->start();
-        });
+            // in the code path we unblock the queue *after* the caps retrieval has succeeded
+            fetchServerSettings();
+        } else
+            _queueGuard.unblock();
     }
 
     if (oldState != _state) {
         Q_EMIT stateChanged(_state);
+        // the old->new state is confirmed to be different and one of them is connected state so
+        // isConnected did actually change
+        if (oldState == Connected || _state == Connected)
+            emit isConnectedChanged();
+    }
+}
+
+void AccountState::fetchServerSettings()
+{
+    Q_ASSERT(_fetchServerSettingsRunner == nullptr);
+    _fetchServerSettingsRunner = new FetchServerSettingsRunner(_account, this);
+
+    connect(_fetchServerSettingsRunner, &FetchServerSettingsRunner::finishedSignal, this, &AccountState::slotFetchServerSettingsResult);
+    _fetchServerSettingsRunner->start();
+}
+
+void AccountState::slotFetchServerSettingsResult(FetchServerSettingsRunner::Result result)
+{
+    Q_ASSERT(_state == Connected);
+
+    _connectionErrors.clear();
+
+    State newState = _state;
+
+    switch (result) {
+    case FetchServerSettingsRunner::Result::UnsupportedServer:
+        _connectionErrors.append(tr("The server is not supported by this client."));
+        newState = ConfigurationError;
+        break;
+    case FetchServerSettingsRunner::Result::InvalidCredentials:
+        slotInvalidCredentials();
+        break;
+    case FetchServerSettingsRunner::Result::TimeOut:
+        _connectionErrors.append(tr("Retrieving user settings and server capabilities timed out."));
+        // hmmm...do we need to retry in this case? I'm guessing yes but needs discussion
+        // actually no, we should not need to do it explicitly as the next round(s) of connection validator should
+        // hopefully resolve it
+        newState = NetworkError;
+        break;
+    case FetchServerSettingsRunner::Result::Undefined:
+        _connectionErrors.append(tr("Unable to retrieve user settings and server capabilities."));
+        newState = Disconnected;
+        break;
+    case FetchServerSettingsRunner::Result::Success:
+        break;
+    }
+
+    // this step is done, delete after this slot finishes else the self deleting jobs inside get munged up -> crash. TODO: evaluate whether there is any
+    // value to use the parenting mechanism for the AbstractNetworkJobs inside the FetchServerSettingsJob - I find it really questionable to parent
+    // self deleting objects but this needs deeper investigation.
+    _fetchServerSettingsRunner->deleteLater();
+
+    if (newState != Connected) {
+        setState(newState);
+        return;
     }
+
+    // these are both self deleting.
+    // they can finish whenever, everything else can carry on.
+    if (_account->capabilities().avatarsAvailable()) {
+        auto *avatarJob = new AvatarJob(_account, _account->davUser(), 128, nullptr);
+        connect(avatarJob, &AvatarJob::avatarPixmap, this, [this](const QPixmap &img) { _account->setAvatar(AvatarJob::makeCircularAvatar(img)); });
+        avatarJob->start();
+    }
+
+    if (_account->capabilities().appProviders().enabled) {
+        auto *jsonJob = new JsonJob(_account, _account->capabilities().appProviders().appsUrl, {}, "GET");
+        connect(jsonJob, &JsonJob::finishedSignal, this, [jsonJob, this] { _account->setAppProvider(AppProvider{jsonJob->data()}); });
+        jsonJob->start();
+    }
+
+    _needsServerSettingsRefresh = false;
+    _queueGuard.unblock();
+    emit isConnectedChanged();
 }
 
+
 bool AccountState::isSignedOut() const
 {
     return _state == SignedOut;
@@ -489,8 +559,6 @@ void AccountState::slotConnectionValidatorResult(ConnectionValidator::Status sta
         setState(Disconnected);
         break;
     case ConnectionValidator::ClientUnsupported:
-        [[fallthrough]];
-    case ConnectionValidator::ServerVersionMismatch:
         setState(ConfigurationError);
         break;
     case ConnectionValidator::StatusNotFound:
@@ -543,6 +611,7 @@ void AccountState::slotInvalidCredentials()
     qCInfo(lcAccountState) << "refreshing oauth failed";
     qCInfo(lcAccountState) << "asking user";
 
+    _needsServerSettingsRefresh = true;
     creds->askFromUser();
     setState(AskingCredentials);
 }
@@ -590,7 +659,12 @@ void AccountState::setSettingUp(bool settingUp)
 }
 bool AccountState::readyForSync() const
 {
-    return !_fetchCapabilitiesJob && isConnected();
+    // this is highly questionable.
+    // first, this explains why the folders aren't ever syncing after refactoring the fetchServerSettings job. Folder::canSync calls this and
+    // that is checked when trying to enqueue the folder
+    // net is that because we can't cleanly get rid of the fetshServerSettingsJob (yet) this always returns false! or at least it does
+    // on first folder load.
+    return !_needsServerSettingsRefresh && isConnected();
 }
 
 } // namespace OCC

src/gui/accountstate.h

@@ -19,6 +19,7 @@
 #include "gui/owncloudguilib.h"
 
 #include "connectionvalidator.h"
+#include "fetchserversettings.h"
 #include "jobqueue.h"
 
 #include "account.h"
@@ -38,7 +39,6 @@ namespace OCC {
 
 class QuotaInfo;
 class TlsErrorDialog;
-class FetchServerSettingsJob;
 
 /**
  * @brief Extra info about an ownCloud server account.
@@ -159,8 +159,8 @@ class OWNCLOUDGUI_EXPORT AccountState : public QObject
     void checkConnectivity(bool blockJobs = false);
 
 private:
-
     void setState(State state);
+    void fetchServerSettings();
 
 Q_SIGNALS:
     void stateChanged(State state);
@@ -169,6 +169,7 @@ class OWNCLOUDGUI_EXPORT AccountState : public QObject
 
 protected Q_SLOTS:
     void slotConnectionValidatorResult(ConnectionValidator::Status status, const QStringList &errors);
+    void slotFetchServerSettingsResult(OCC::FetchServerSettingsRunner::Result result);
     void slotInvalidCredentials();
     void slotCredentialsFetched();
 
@@ -217,7 +218,8 @@ protected Q_SLOTS:
 
     QuotaInfo *_quotaInfo = nullptr;
 
-    QPointer<FetchServerSettingsJob> _fetchCapabilitiesJob;
+    QPointer<FetchServerSettingsRunner> _fetchServerSettingsRunner;
+    bool _needsServerSettingsRefresh = false;
 };
 }
 

src/gui/connectionvalidator.cpp

@@ -19,7 +19,6 @@
 #include "libsync/creds/abstractcredentials.h"
 #include "libsync/networkjobs.h"
 #include "libsync/networkjobs/checkserverjobfactory.h"
-#include "libsync/theme.h"
 
 #include <QJsonObject>
 #include <QLoggingCategory>
@@ -284,38 +283,38 @@ void ConnectionValidator::slotAuthSuccess()
     }
 
     _errors.clear();
-    if (_mode != ConnectionValidator::ValidationMode::ValidateAuth) {
-        auto *fetchSetting = new FetchServerSettingsJob(_account, this);
-        const auto unsupportedServerError = [this] {
-            _errors.append({tr("The configured server for this client is too old."), tr("Please update to the latest server and restart the client.")});
-        };
-        connect(fetchSetting, &FetchServerSettingsJob::finishedSignal, this, [unsupportedServerError, this](FetchServerSettingsJob::Result result) {
-            switch (result) {
-            case FetchServerSettingsJob::Result::UnsupportedServer:
-                unsupportedServerError();
-                reportResult(ServerVersionMismatch);
-                break;
-            case FetchServerSettingsJob::Result::InvalidCredentials:
-                reportResult(CredentialsWrong);
-                break;
-            case FetchServerSettingsJob::Result::TimeOut:
-                reportResult(Timeout);
-                break;
-            case FetchServerSettingsJob::Result::Success:
-                if (_account->serverSupportLevel() == Account::ServerSupportLevel::Unknown) {
-                    unsupportedServerError();
-                }
-                reportResult(Connected);
-                break;
-            case FetchServerSettingsJob::Result::Undefined:
-                reportResult(Undefined);
-                break;
-            }
-        });
-
-        fetchSetting->start();
-        return;
-    }
+    /* if (_mode != ConnectionValidator::ValidationMode::ValidateAuth) {
+         auto *fetchSetting = new FetchServerSettingsJob(_account, this);
+         const auto unsupportedServerError = [this] {
+             _errors.append({tr("The configured server for this client is too old."), tr("Please update to the latest server and restart the client.")});
+         };
+         connect(fetchSetting, &FetchServerSettingsJob::finishedSignal, this, [unsupportedServerError, this](FetchServerSettingsJob::Result result) {
+             switch (result) {
+             case FetchServerSettingsJob::Result::UnsupportedServer:
+                 unsupportedServerError();
+                 reportResult(ServerVersionMismatch);
+                 break;
+             case FetchServerSettingsJob::Result::InvalidCredentials:
+                 reportResult(CredentialsWrong);
+                 break;
+             case FetchServerSettingsJob::Result::TimeOut:
+                 reportResult(Timeout);
+                 break;
+             case FetchServerSettingsJob::Result::Success:
+                 if (_account->serverSupportLevel() == Account::ServerSupportLevel::Unknown) {
+                     unsupportedServerError();
+                 }
+                 reportResult(Connected);
+                 break;
+             case FetchServerSettingsJob::Result::Undefined:
+                 reportResult(Undefined);
+                 break;
+             }
+         });
+
+         fetchSetting->start();
+         return;
+     }*/
     reportResult(Connected);
 }
 

src/gui/connectionvalidator.h

@@ -62,18 +62,9 @@ namespace OCC {
         |
         +-> slotAuthSuccess --+--> X (depending if coming from checkServerAndAuth or not)
                               |
-  +---------------------------+
-  |
-  +-> checkServerCapabilities
-        JsonApiJob (cloud/capabilities) -> slotCapabilitiesRecieved -+
-                                                                     |
-  +------------------------------------------------------------------+
-  |
-  +-> fetchUser -+
-                 |
-                 +-> AvatarJob
-                            |
-                            +-> slotAvatarImage --> reportResult()
+
+retrieving server capabilities, user settings, avatar and app providers (if enabled) has moved to account state as those
+checks should happen much less frequently!
 
     \endcode
  */
@@ -99,7 +90,7 @@ class OWNCLOUDGUI_EXPORT ConnectionValidator : public QObject
         Undefined,
         Connected,
         NotConfigured,
-        ServerVersionMismatch, // The server version is too old
+        //  ServerVersionMismatch, // The server version is too old
         CredentialsNotReady, // Credentials aren't ready
         CredentialsWrong, // AuthenticationRequiredError
         SslError, // SSL handshake error, certificate rejected by user?