Alters 'No Internet Connection' error message. #181 #374
Conversation
|
@lpszBuffer, thanks for your PR! By analyzing the annotation information on this pull request, we identified @rullzer, @DeepDiver1975 and @LukasReschke to be potential reviewers |
|
Changing all the js and json files will have no effect, because this is overwritten by the stuff, that is in transifex. |
|
@LukasReschke I think we should change this check to https://nextcloud.com |
|
@lpszBuffer Thanks a lot for your contribution :) |
nickvergessen
added
2. developing
|
Setting back to developing for the URL change |
|
Altered CheckSetupController.isInternetConnectionWorking() to use the new URI you specified. |
| @@ -94,7 +94,7 @@ | |||
| if (xhr.status === 200 && data) { | |||
| if (!data.serverHasInternetConnection) { | |||
| messages.push({ | |||
| msg: t('core', 'This server has no working Internet connection. This means that some of the features like mounting external storage, notifications about updates or installation of third-party apps will not work. Accessing files remotely and sending of notification emails might not work, either. We suggest to enable Internet connection for this server if you want to have all features.'), | |||
| msg: t('core', 'This server has no working Internet connection: http://owncloud.org could not be reached. This means that some of the features like mounting external storage, notifications about updates or installation of third-party apps will not work. Accessing files remotely and sending of notification emails might not work, either. We suggest to enable Internet connection for this server if you want to have all features.'), | |||
There was a problem hiding this comment.
You forgot to change the URL in the error messages? 😉
|
I agree completely with also checking an external site that should have guaranteed uptime. I'll make those changes and have them committed by the end of the day. |
| @@ -92,14 +92,31 @@ private function isInternetConnectionWorking() { | |||
| return false; | |||
| } | |||
|
|
|||
| $siteArray = array('www.nextcloud.com', 'www.google.com', 'www.github.com'); | |||
There was a problem hiding this comment.
Just a nitpick but usually we'd prefer the new [] syntax and everything in a new line :)
|
This is very good stuff, @lpszBuffer. Just some small unit test adjustments are missing and this should be probably be good to go. As you can see at https://drone.weasel.rocks/nextcloud/server/431 the following tests do fail: Can I ask you to adjust those texts at https://github.com/nextcloud/server/blob/master/core/js/tests/specs/setupchecksSpec.js as well? Alternatively, you can add me as contributor to your repo and I can take care of that for you :-) And awesome with contributing back to us! Much appreciated 🚀 👯 ❤️ |
|
I'm happy to bring it in line with your project's standards and conventions. Also looks like I must have missed altering some unit tests somewhere if the CI drone failed. |
|
Oh. And lowercase "C" on "Nextcloud". Otherwise @jancborchardt gets a minor heart attack if he ever sees that error 😉 |
| @@ -129,7 +129,7 @@ | |||
| "Strong password" : "Contraseña mui bona", | |||
| "Your web server is not yet set up properly to allow file synchronization because the WebDAV interface seems to be broken." : "El to sirvidor web entá nun ta configuráu afayadizamente pa permitir la sincronización de ficheros porque la interfaz WebDAV paez tar rota.", | |||
| "Your web server is not set up properly to resolve \"{url}\". Further information can be found in our <a target=\"_blank\" rel=\"noreferrer\" href=\"{docLink}\">documentation</a>." : "El to sirvidor web nun ta configuráu correchamente pa resolver \"{url}\". Pues atopar más información en nuesa documentación <a target=\"_blank\" rel=\"noreferrer\" href=\"{docLink}\"></a>.", | |||
| "This server has no working Internet connection. This means that some of the features like mounting external storage, notifications about updates or installation of third-party apps will not work. Accessing files remotely and sending of notification emails might not work, either. We suggest to enable Internet connection for this server if you want to have all features." : "Esti sirvidor nun tien conexón a Internet. Esto significa que dalgunes de les carauterístiques nun van funcionar, como'l montaxe d'almacenamiento esternu, les notificaciones sobre anovamientos, la instalación d'aplicaciones de terceros, l'accesu a los ficheros de mou remotu o l'unviu de correos-e de notificación. Suxerimos habilitar una conexón a Internet nesti sirvidor pa esfrutar de toles funciones.", | |||
| "This server has no working Internet connection: http://owncloud.org could not be reached. This means that some of the features like mounting external storage, notifications about updates or installation of third-party apps will not work. Accessing files remotely and sending of notification emails might not work, either. We suggest to enable Internet connection for this server if you want to have all features." : "Esti sirvidor nun tien conexón a Internet. Esto significa que dalgunes de les carauterístiques nun van funcionar, como'l montaxe d'almacenamiento esternu, les notificaciones sobre anovamientos, la instalación d'aplicaciones de terceros, l'accesu a los ficheros de mou remotu o l'unviu de correos-e de notificación. Suxerimos habilitar una conexón a Internet nesti sirvidor pa esfrutar de toles funciones.", | |||
There was a problem hiding this comment.
Also still the old translations texts that only cover owncloud.org for the English version. :)
|
Thanks for taking so much time to look through this. It's become a little bit more involved at this point than changing a string literal for an error message. 😛 @MorrisJobke mentioned that altering the js and json files would be futile as they are overwritten by transifex. However, if these need to be updated again I'll take care of it. I'll change my array syntax, alter the language files if necessary, track down the unit tests that are still failing (I think I saw two of them), update their expected messages, and then I think we should be ready to pull. |
Considering that the translations wouldn't be updated by people then like forever. Ok, let's just remove all changes to the translation files also from the Git history :) |
|
Something like the following should do, make sure to have our repo configured as upstream first (https://help.github.com/articles/configuring-a-remote-for-a-fork/) Then you should be only left with a single commit. You can then force push that and the history here will be cleaner :-) |
|
@lpszBuffer Btw. if you're interested in joining the Nextcloud organization on GitHub let us know. That would allow you to assign labels and so on to tickets / PRs yourself and also create branches on the repo directly. Which makes it easier for others to cooperate on your PRs. So if you plan to contribute also a second, third, fourth and so on time. That may be a very good idea 😄 |
| @@ -192,7 +192,7 @@ describe('OC.SetupChecks tests', function() { | |||
| async.done(function( data, s, x ){ | |||
| expect(data).toEqual([ | |||
| { | |||
| msg: 'This server has no working Internet connection. This means that some of the features like mounting external storage, notifications about updates or installation of third-party apps will not work. Accessing files remotely and sending of notification emails might not work, either. We suggest to enable Internet connection for this server if you want to have all features.', | |||
| msg: 'This server has no working Internet connection: http://owncloud.org could not be reached. This means that some of the features like mounting external storage, notifications about updates or installation of third-party apps will not work. Accessing files remotely and sending of notification emails might not work, either. We suggest to enable Internet connection for this server if you want to have all features.', | |||
KittieCrash
force-pushed
the
master
branch
2 times, most recently
from
45e694a to
2e5e36b
Compare
|
The results from CI look like I've done something terrible. |
|
No not all all. It it not so bad. The thing is that there was way to much CI output. This has since been fixed on master. So if you rebase onto master (yet again, sorry). All should be fine! |
I restarted the CI... let's see what happens. |
|
All tests passed. Nice idea to check multiple servers, in case one server is temporarily unreachable. But I would not mention the three server in the error message explicitly. Why should we do gratis advertisement for Google and Github? I don't see any additional value for the user in mentioning the servers. |
|
The motivation behind listing the error messages was to maintain the initial intent of this issue - which was clarity in the 'No Internet Connection' error message. Though admittedly, it makes more sense to specifically say that the site was unreachable when we were only checking owncloud's site. In theory, we should not ever hit a point where Nextcloud, Google, and Github are all down. In light of this, please just let me know if your team would like the error message altered again, perhaps back to its original that simply stated there was no internet connection. |
|
I agree, we should use them as backup test endpoints but the user doesn't need to know that. Something like "multiple endpoints were tested" or "multiple endpoints could not be reached" would be better. |
|
|
@lpszBuffer Weird. I can't find this in your code, but it has failed for the same reason on Travis and Drone. I just retriggered them. Maybe it was just a cloning error. |
MorrisJobke
added
3. to review
|
Looks like it passed! 🎉 |
|
Awesome stuff! 👍 🚀 |
|
👍 |
Resolves #181.
Error message now reads:
'This server has no working Internet connection: http://owncloud.org could not be reached.'