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

improve federation error messages #767

Merged
merged 1 commit into from
Aug 9, 2016
Merged

Conversation

schiessle
Copy link
Member

improve error message, in case a wrong URL gets added to the "add trusted server" input field #735

@schiessle schiessle added this to the Nextcloud 11.0 milestone Aug 8, 2016
@mention-bot
Copy link

@schiessle, thanks for your PR! By analyzing the annotation information on this pull request, we identified @DeepDiver1975, @MorrisJobke and @nickvergessen to be potential reviewers

@MorrisJobke MorrisJobke mentioned this pull request Aug 8, 2016
67 tasks
@LukasReschke
Copy link
Member


1) OCA\Federation\Tests\TrustedServersTest::testIsOwnCloudServerFail
Failed asserting that exception of type "\Exception" is thrown.

@LukasReschke LukasReschke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Aug 8, 2016
@schiessle schiessle added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 8, 2016
@schiessle schiessle force-pushed the federation-error-messages branch 2 times, most recently from 0bb2b93 to f3b1864 Compare August 8, 2016 16:22

}
} catch (\Exception $e) {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Could we somehow log the error message here? This would make it a lot easier to debug when needed (so debug log level is enough)

Copy link
Member Author

Choose a reason for hiding this comment

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

added, please review 😃

@MorrisJobke
Copy link
Member

👍

@@ -89,7 +89,7 @@ public function addServer($url) {
if ($result) {
return (int)$this->connection->lastInsertId('*PREFIX*'.$this->dbTable);
} else {
$message = 'Internal failure, Could not add ownCloud as trusted server: ' . $url;
$message = 'Internal failure, Could not add trusted server: ' . $url;
Copy link
Member

Choose a reason for hiding this comment

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

Could not add trusted server = The server is trusted but you were not able to add
Could not add **as** trusted server = The server could not be added because there is a problem with the trust

Copy link
Member Author

Choose a reason for hiding this comment

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

The server could not be added because there is a problem with the trust

There is no problem with the trust, the error happens most likely if you point to a URL which doesn't run a Nextcloud/ownCloud. So you could for example argue that if you add your webapage it is a page you trust, but well it is no Nextcloud so it can't be added. Anyway, this is just a log message to identify the code path which failed.

@MariusBluem
Copy link
Member

👍

@MariusBluem MariusBluem merged commit ba15687 into master Aug 9, 2016
@MariusBluem MariusBluem deleted the federation-error-messages branch August 9, 2016 12:28
@MariusBluem
Copy link
Member

Backport? @karlitschek

@karlitschek
Copy link
Member

perfect. please backport 👍 :-)

@MariusBluem
Copy link
Member

On it 💪

@schiessle
Copy link
Member Author

Thanks @Mar1u5 😃

@MariusBluem
Copy link
Member

#788 is the backport to stable10 ... Should it be also in stable9 😕

@MorrisJobke
Copy link
Member

@Mar1u5 When the backport PRs are opened the backport-request label could be removed. This makes it then easier to find not yet backported PRs ;)

@MariusBluem
Copy link
Member

When the backport PRs are opened the backport-request label could be removed. This makes it then easier to find not yet backported PRs ;)

I know, but I am not sure, wheter we also need this in stable9 and thats what I've asked above 😉

@schiessle
Copy link
Member Author

It is a small fix, I would also backport it to stable9

@MariusBluem
Copy link
Member

Okay... Thats something for you @schiessle ... I experience conflicts when I cherry-pick the commit to stable9 😁

@schiessle
Copy link
Member Author

ah yes, because almost all file name changed between 9 and 10 because of PSR4 compatibility... Let's keep it this way. This is also nothing urgent, just a error string.

GitHubUser4234 pushed a commit to GitHubUser4234/server that referenced this pull request Aug 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants