Fix clock if authentication is enabled #4860

GermanBluefox · 2019-08-06T22:16:49Z

Add .npmignore file

Make authentication dialog bigger to suit for other languages.

sulkaharo · 2019-08-07T20:58:35Z

Not sure this needs a .npmignore? Also, see https://medium.com/@jdxcode/for-the-love-of-god-dont-use-npmignore-f93c08909d8d

unsoluble · 2019-08-08T07:33:25Z

There are tons of changes in here — can you elaborate please on exactly what functionality has been added and/or altered?

sulkaharo · 2019-08-08T07:40:02Z

lib/client/index.js

-      language.set('en');
+
+      // detect browser language
+      var lang = (navigator.language || navigator.userLanguage).toLowerCase();


For language, you should check the client preferences on the language picked in the UI in the main view. Nightscout explicitly does not use the browser language detection

Please see https://github.com/nightscout/cgm-remote-monitor/pull/4860/files#diff-5f3d9018a260bb33cee03e0c67fdf95bR81

sulkaharo · 2019-08-08T07:43:17Z

@unsoluble I read through the changes and everything except for the language auto-detection and the .npmignore look like they're good to go. This fixes the clock views not working on instances that don't allow API reads without authentication.

And get suitable language for the very first auth dialog (before the settings were read from server or user can select language in the configuration)

GermanBluefox · 2019-08-10T17:52:25Z

lib/client/index.js

-      language.set('en');
+
+      // detect browser language
+      var lang = Storages.localStorage.get('language') || (navigator.language || navigator.userLanguage).toLowerCase();


So I changed it. Please don't forget that for the VERY FIRST authentication dialog it is better to show it in the browser language, than in english.
E.g. my browser language is german and I would like to see the auth dialog in german.

GermanBluefox · 2019-08-10T17:54:32Z

Will it be merged or I should split it? (what will costs me time :) )

GermanBluefox · 2019-08-10T18:05:04Z

Not sure this needs a .npmignore? Also, see https://medium.com/@jdxcode/for-the-love-of-god-dont-use-npmignore-f93c08909d8d

The author forgot to write ".npmrc" into .npmignore and wrote the whole article about it on medium. Don't believe in everything what is written in internet. The authors there are just like you and me. :)

app.js

lib/client/clock-client.js

GermanBluefox · 2019-08-10T18:11:11Z

lib/client/clock-client.js


  // Insert the trend arrow.
-  $('#arrow').attr('src', '/images/' + rec.direction + '.svg');
+  $('#arrow').attr('src', '/images/' + (!rec.direction || rec.direction === 'NOT COMPUTABLE' ? 'NONE' : rec.direction) + '.svg');


If calculation cannot be done, so invalid icon was shown. This will fix the icon.

GermanBluefox · 2019-08-10T18:11:36Z

lib/client/clock-client.js


  // Generate and insert the clock.
-  let timeDivisor = (client.settings.timeFormat) ? client.settings.timeFormat : 12;
+  let timeDivisor = parseInt(client.settings.timeFormat ? client.settings.timeFormat : 12, 10);


Just number validation.

GermanBluefox · 2019-08-10T18:11:44Z

lib/client/clock-client.js

-  if (timeDivisor == 12) {
-    h = (h == 0) ? 12 : h; // In the case of 00:xx, change to 12:xx for 12h time
+  if (timeDivisor === 12) {
+    h = h || 12; // In the case of 00:xx, change to 12:xx for 12h time


This is not only formatting. For h == 4 this will be changed to h==12. Please restore old behaviour, perhaps with === instead of ==

fixed on my branch

GermanBluefox · 2019-08-10T18:12:00Z

lib/client/clock-client.js

  // defined in the template this is loaded into
  // eslint-disable-next-line no-undef
-  if (clockFace == 'clock-color') {
+  if (clockFace === 'clock-color') {


GermanBluefox · 2019-08-10T18:12:47Z

lib/hashauth.js

-      , height: 240
+        width: 400
+      , height: 270
+      , closeText: ''


Make an auth dialog a little bit bigger, because in other languages the text is longer and scroll bars appear.

GermanBluefox · 2019-08-10T18:13:24Z

lib/hashauth.js

      , buttons: [
        {
-          text: translate('Update')
+          id: 'requestauthenticationdialog-btn'


Search is better by ID than $(this).parent().find('button.ui-button-text-only')

GermanBluefox · 2019-08-10T18:14:10Z

lib/hashauth.js

      ]
      , open: function open ( ) {
-        $('#requestauthenticationdialog').keypress(function pressed (e) {
+        $('#apisecret').off('keyup').on('keyup' ,function pressed (e) {


Install handler only on INPUT element and not on the whole dialog.

GermanBluefox · 2019-08-10T18:14:53Z

lib/hashauth.js

      '<div id="requestauthenticationdialog" style="display:none" title="'+translate('Device authentication')+'">'+
        '<label for="apisecret">'+translate('Your API secret')+': </label>'+
-        '<input type="password" id="apisecret" size="20" />'+
+        '<input type="password" id="apisecret" size="20" style="width: 100%;"/>'+


make a secret input element to be a full width, because the secret could be longer than 20 chars.

GermanBluefox · 2019-08-10T18:15:02Z

lib/language.js


    language.languages.forEach(function (l) {
-      if (l.code == language.lang && l.speechCode) language.speechCode = l.speechCode;
+      if (l.code === language.lang && l.speechCode) language.speechCode = l.speechCode;


GermanBluefox · 2019-08-10T18:15:32Z

views/clockviews/shared.html

      <div id="trend">
        <div id="bgnow"></div>
-        <div id="arrowDiv"><img id="arrow" src=""/></div>
+        <div id="arrowDiv"><img id="arrow" src="" alt="arrow"/></div>


Just a HTML w3c requirement. Alt should be in every "img"

GermanBluefox · 2019-08-10T18:17:40Z

views/clockviews/shared.html

+    };
+    script.src = src;
+
+    document.head.appendChild(script); //or something of the likes


Load JSON with token or secret for the case that authentication is enabled.

There is still an open case if the user starts directly to work with clock view without calling the index.html. In this case no authentication dialog will be shown. But I don't know such a use case. In this case the user can use token.

unsoluble · 2019-08-10T19:26:03Z

Thanks for all the notes. Was really just wanting to know if any other functionality was affected, but appreciate the details. :)

PieterGit · 2019-08-20T14:29:20Z

views/clockviews/shared.html

-  <script src="/api/v1/status.js"></script>
-  <script src="<%= locals.bundle %>/js/bundle.clock.js?v=<%= locals.cachebuster %>"></script>
-
+  <script src="/<%= locals.bundle %>/js/bundle.clock.js?v=<%= locals.cachebuster %>"></script>


locals.bundle is /bundle by default, so I get error:

remove / in front of it to make it work

please cherry pick PieterGit@e143142

PieterGit · 2019-08-20T20:31:15Z

@GermanBluefox Thanks for your contributions. I tested this and did a code review of the changes. I don't understand how you have managed to test this ok, without removing the extra / because with the / it didn't work for me. I'm fine with merging this if you cherry pick or remove the /. I'm now able to use the clock and the implementation LGTM.

I'm sure you and @sulkaharo can find a solution on whether to keep or remove the .npmignore. I think I would advice not to merge it because I don't see the benefits of it. If the discussion about that single file is persistent, I would suggest to open an extra PR for that and merge this without .npmignore. You can just delete it from the PR, because we squash merge commits to dev when we approve them.

sulkaharo · 2019-08-22T18:57:02Z

Weird, I thought I posted a long piece here about the .npmignore. The main reason I'd prefer not having it is, given it's a file blacklist, the package maintainers will have to remember to consider the blacklist for every commit to the repository, while a whitelist approach would mean we detect something breaks if a mistake is made. As someone who's been looking after this repo for some years, I really want to reduce the maintenance effort as much as possible, so any new code that comes in should do everything possible to the least additional effort on maintenance.

(Related - we should talk about how / where / when / why to release nightscout as a npm package. If it's done, it should be done from the original repo using the Nightscout org.

PieterGit · 2019-08-22T19:00:54Z

merged with #4914 (without npmignore)

PieterGit · 2019-08-22T22:36:38Z

@sulkaharo Your reaction on npmignore is #4860 (comment) here. Based on the article I wrote #4860 (comment)

@GermanBluefox @sulkaharo agree to close this issue?

GermanBluefox · 2019-08-23T14:53:44Z

agree to close this issue?

It is Ok.

GermanBluefox added 8 commits August 7, 2019 00:09

Fix clock if authentication is enabled

3c25914

Add .npmignore file

Add time to clock view back

366d10c

Formatting

3a52cac

Select browser language for authentication dialog

d61e8f9

Make authentication dialog bigger to suit for other languages.

Update hashauth.js

050808d

Update hashauth.js

ccad081

Update hashauth.js

293ac55

Update hashauth.js

afbe6a3

sulkaharo reviewed Aug 8, 2019

View reviewed changes

ghost approved these changes Aug 10, 2019

View reviewed changes

Select language from localStorage

d361a72

And get suitable language for the very first auth dialog (before the settings were read from server or user can select language in the configuration)