Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server: Parse version from capabilities too #5691 #5698

Merged
merged 2 commits into from Apr 19, 2017
Merged

Conversation

guruz
Copy link
Contributor

@guruz guruz commented Apr 12, 2017

No description provided.

@guruz guruz added this to the 2.3.2 milestone Apr 12, 2017
@guruz
Copy link
Contributor Author

guruz commented Apr 12, 2017

I only parse in ConnectionValidator not in wizard.. is that a problem? I didn't see the version used earlier

@@ -119,6 +119,8 @@ void ConnectionValidator::slotStatusFound(const QUrl&url, const QVariantMap &inf
<< url << " with version "
<< CheckServerJob::versionString(info)
<< "(" << CheckServerJob::version(info) << ")";
// Newer servers don't disclose any version in status.php anymore
// https://github.com/owncloud/core/pull/27473/files

QString version = CheckServerJob::version(info);
_account->setServerVersion(version);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be guarded by a if (!version.isEmpty()) - I think we run this periodically and it means that we might reset the server version stored in the account to the empty string occasionally!

Copy link
Contributor

@ckamm ckamm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs guard against accidentally clearing out server version.

@ogoffart
Copy link
Contributor

We use the version info in the wizzard (httpcredspage) before connecting, and in the password dialog.
So we need to make sure that if no version is reported by status.php, then these behave as if the server is >= 10.0

QString serverVersion = caps.toMap()["core"].toMap()["status"].toMap()["version"].toString();
if (!serverVersion.isEmpty()) {
_account->setServerVersion(serverVersion);
qDebug() << "Version from capabilities instead of status.php:" << serverVersion;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this debug? the version will already be shown in the caps earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can try reading the capabilities, but to most people it will look like

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that adding debug output only make it looks more like that matrix screen, even for me. While people who care to see the server version will be able to parse it from other debug output already

@@ -477,7 +477,8 @@ bool CheckServerJob::finished()
if( status.contains("installed")
&& status.contains("version")
&& status.contains("versionstring") ) {

// Newer servers don't disclose anything in that empty string "version" anymore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So then we will show instanceNotFound and we can't connect. This needs to be changed as well!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The server still sends "version" and "versionstring", but it will be empty. I had to look this up too. Expand comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I wrote "empty string" and not "does not contain this specific key"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok, so it still has the version string, but it is empty?
Shoud we not make it works also if the version string is not there at all? (maybe future server remove it completely?)

@ckamm
Copy link
Contributor

ckamm commented Apr 19, 2017

@ogoffart Could you re-review? In particular you said "We use the version info in the wizzard (httpcredspage) before connecting, and in the password dialog." - but I only found it being used in httpcredspage!

@guruz guruz added the p2-high Escalation, on top of current planning, release blocker label Apr 19, 2017
&& status.contains("version")
&& status.contains("versionstring") ) {

if( status.contains("installed") ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is still there even when the version is not there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The presence of the other attributes is currently discussed at owncloud/core#27669

@ogoffart
Copy link
Contributor

👍 Looks good

@ckamm ckamm merged commit 5ac58d3 into 2.3 Apr 19, 2017
@guruz guruz deleted the version_from_capabilities branch July 31, 2017 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-high Escalation, on top of current planning, release blocker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants