-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
enh: update desktop client unsupported version (403) error message #43281
Conversation
d5db7bc
to
6e513f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests must be fixed.
What about adding a link on the message to server/config/config.sample.php Lines 1105 to 1117 in ee6a62a
|
b9e4df9
to
7a10ed0
Compare
7a10ed0
to
494f87c
Compare
I don't know how to fix the tests. I tried to run it locally: I need help. |
@camilasan I was more thinking about a clickable link (if it can be done).
Otherwise, the full link is too much noise IMO :( |
Agreed with @solracsf that the link could be hidden. What do you think about adding the link to the text "version 3.0.0 or later"? Would that be possible :)
|
e071f1b
to
7670c30
Compare
done, please review @solracsf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me. 👍
Maybe just update the 1st post with an updated screenshot of the final version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good if it’s a clickable link like @solracsf suggested. :)
Another detail: In the settings it says "Unable to connect to Nextcloud" and in the notification "Network error" – both of these are different and neither really seem correct for the issue?
Maybe rather it should say "Connection issue" @camilasan?
I changed it here: nextcloud/desktop#6514 since the message reason may vary, I think 'Connection issue' is better. @jancborchardt I didn't realize your comment only referred to the strings used in the desktop client. So this PR on the server is actually done. |
7670c30
to
6f8918a
Compare
935cef4
to
f94ab7f
Compare
* The user should get a more friendly warning when their desktop client version is not supported anymore by the server. See #nextcloud/desktop/issues/6273 * Update BlockLegacyClientPluginTest to reflect the new 403 error message. Signed-off-by: Camila Ayres <[email protected]>
Signed-off-by: Ferdinand Thiessen <[email protected]>
f94ab7f
to
5fc715a
Compare
$customClientDesktopLink = htmlspecialchars($this->themingDefaults->getSyncClientUrl()); | ||
$minimumSupportedDesktopVersion = htmlspecialchars($minimumSupportedDesktopVersion); | ||
|
||
throw new \Sabre\DAV\Exception\Forbidden("This version of the client is unsupported. Upgrade to <a href=\"$customClientDesktopLink\">version $minimumSupportedDesktopVersion or later</a>."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, this should be a translated string 😿
Maybe in a follow-up PR.
The user should get a more friendly warning when their desktop client version is not supported anymore by the server. See #nextcloud/desktop/issues/6273
Before:
After:
Systray message
minimumSupportedDesktopVersion
is thrown desktop#6273Summary
TODO
Checklist