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

Workaround for re-loading bug in profile editor: don't show profile form during save #4074

Merged
merged 5 commits into from
Feb 2, 2019

Conversation

DigitalDan1
Copy link
Contributor

This fixes a problem I noticed in the profile editor where it re-loads and re-draws every time there's an update from the server (due to socket.io emit calling the callback).

This was causing issues where the user could be making changes to their profile, then the data would re-load and re-draw, so they'd lose their changes and have to start again.

It was also causing the save button's event to be hooked up multiple times, which meant saving was taking longer and longer, and made it more likely that any further changes made by the user were lost. Users would have seen the user interface update whilst editing, replacing their values with a previous version (from whichever save came back last). This change (crudely) hides the form whilst the save is happening to make sure the user doesn't carry on making changes that will be lost when the re-draw happens.

This should fix #4056.

@PieterGit PieterGit added this to the 0.11.0 milestone Nov 16, 2018
@sulkaharo
Copy link
Member

So hey - I've never seen this behavior, so I'm wondering what the root cause for the reload is? Or at least - in our instance, I can edit the profiles and the page doesn't reload and I've never lost edits. Which browser are you testing on?

@DigitalDan1
Copy link
Contributor Author

Hey @sulkaharo, I've seen this on Windows 10 Chrome and Edge, and iPhone XS Max safari. When it happens there are regular "Client connected to server" logs in the console. I think it's more likely to happen when there's a lot of data being transferred, maybe a timeout is causing socket IO to reconnect?

Basically the 'connect' event is fired on the initial connection and any subsequent re-connection, so there's always a chance the callback could be called more than once.

It's easy enough to reproduce: just switch your network connection off then on again.

@PieterGit PieterGit changed the title Fix re-loading bug in profile editor Workaround for re-loading bug in profile editor: don't show profile form during save Feb 2, 2019
@PieterGit
Copy link
Contributor

PieterGit commented Feb 2, 2019

@DigitalDan1 thanks for this PR.
I was able to reproduce the fact that socket.io updates can revert changes to the profile.
I would classify hiding screen elements as a workaround and we should fix the root cause of this problem with Nightscout 0.12. I created #4252 for that.

Since this issue can cause problems for users I applied this, so that we can include it in Nightscout 0.11.

I tested this PR and it looks good to me. My profile is only hidden very briefly and I noticed the status saving profile at the top of the page.

@PieterGit PieterGit merged commit 81c38e5 into nightscout:dev Feb 2, 2019
@DigitalDan1
Copy link
Contributor Author

@PieterGit, sounds good, thanks.

Just want to capture a few comments here for anyone (including me) that may come back to work on a more elegant fix.

I agree the route cause should be fixed, but also think every page should gracefully handle a socket IO reconnect (I’m pretty sure the admin page has the same issue).

Hiding screen elements addresses a different problem; if the save takes a long time the user can carry on making changes that are lost when the save actually finishes. I’d usually show a dialog (like a Bootstrap Modal) whilst waiting for the save to return. Hiding the form is crude, but does the job for now.

@sulkaharo sulkaharo mentioned this pull request Feb 7, 2019
sulkaharo added a commit that referenced this pull request Feb 7, 2019
Draft release notes for upcoming 0.11 release (currently release candidate phase).

# Changes

Over 360 commits, 89 files changed, +8,428 / −6,569 lines of changes (full list of changes here: 
https://github.com/nightscout/cgm-remote-monitor/pull/4022/commits )

## New features
- Fully secure by default out of the box.  Unsecure access via http is not allowed anymore by default. This might force you to re-authenticate with your `API_SECRET` or token if you were using unsecure access. (@PieterGit )
- No outdated packages with vulnerabilities are being used anymore (@PieterGit ) 
- Add Week to Week report (@jpcunningh, #4123 ) 
- Add Loopalyzer report to analyse looping. Visualize your loop (@lixgbg, #3629 #4235 )
- Add predictions support to Day to Day report (@lixgbg, #3179 )
- Add cgm sensor stop to Careportal (@jpcunningh, #4060)

## Removed features
- remove `mqtt` module, because it had a security issue and was not used
- remove `sgvdata`  module, because it had a security issue, added a lot of complexity and wasn't needed (@PieterGit ). Replacement implementation for CSV and TSV export (@sulkaharo ).

## Improvements
- Fix MongoDB database insert handling. Log error on inserts and don't crash in case the MongoDB disk is full or MongoDB quota is reached (@sulkaharo and @jpcunningh)
- Upgrade packages to recent version, fixing all known security issues with dependencies (@PieterGit)
- Redirect redirect HTTP to HTTPS and implement HSTS (@jweismann, @PieterGit, #4044 and #4010	and #4253 )
- Technical improvement: Migrate from `uglify-js` to `terser-webpack-plugin` (@PieterGit)
- Streamlined Heroku deployment template with more descriptive text and more appropriate defaults for new users (@unsoluble, #4116 )

## Bug fixes
- Fix CGM voltageb battery warning level to match xDrip+ (@jpcunningh, #3954 )
- Fix daylight saving and reloading bug in profile editor, (@DigitalDan1, @Kywalh #4029 and #4074 )
- Reduce the amount of Profile Switch treatments being loaded to fix UI slowdown and Nightscout home screen losing AAPS data from >3 hours ago, (@sulkaharo, @vickster1, #4055 )
- Upgrade to [share2nightscout 0.2.0](https://github.com/nightscout/share2nightscout-bridge/releases/tag/0.2.0). Prevent Nightscout server crashes in case Dexcom server does not respond (@PieterGit, @veryfancy)
- Fix UI so pills are updated immediately after new data is loaded (@sulkaharo)
- Fixes to If-Modified-Since HTTP header handling for BG data (@sulkaharo)

## Documentation and language updates
- Language updates for Danish, Dutch, German, Hebrew, Norwegian, Russian 
- New languages: Japanese, Turkish
- Update Alexa documentation. Note that some Alexa improvements are postponed to Nightscout 0.12 because the Alexa plugin needs refactoring, see #4168 (comment)
- Update IFTTT maker-setup.md docs (@Dave9111, @unsoluble, #4206 )
- Updated various docs, including [CONTRIBUTING](https://github.com/nightscout/cgm-remote-monitor/blob/dev/CONTRIBUTING.md) documentation

# Upgrade notes
- We only allow Nightscout to start with a secure Node JS. 
  - Latest Node 8 LTS (8.15.0 or later) and Latest Node 10 LTS (10.15.1 or later) are recommended and supported. 
  - Latest Node version on Azure (currently 10.14.0) is tolerated, but not recommended
  - Other versions will not start 
- The [rawbg](https://github.com/nightscout/cgm-remote-monitor#rawbg-raw-bg) settings are converted to a single setting tri-state variable.
- We improved security and added several new environment variables such as [INSECURE_USE_HTTP and SECURE_HSTS_HEADER](https://github.com/nightscout/cgm-remote-monitor/#predefined-values-for-your-server-settings-optional)
  - Your site redirects to https by default. If you don't want that or use a Nginx or Apache proxy, set `INSECURE_USE_HTTP` to `true`.
  - We enabled [HTTP Strict Transport Security (HSTS)](https://en.wikipedia.org/wiki/HTTP_Strict_Transport_Security) headers by default, settings `SECURE_HSTS_HEADER` and `SECURE_HSTS_HEADER_*`

## Upgrade notes for Azure users

We recommend Azure users consider migrating their hosting to Heroku, as we've observed Heroku users have significantly less issues with having their sites work reliably. If you want to continue using Azure, change the following configuration variables in Azure before updating to the latest Nightscout version:
```
WEBSITE_NODE_DEFAULT_VERSION=10.14.1
SCM_COMMAND_IDLE_TIMEOUT=300
```

# Install instructions

Install instructions can be found: https://github.com/nightscout/cgm-remote-monitor/blob/master/README.md#install

# Contributors to this release

The release coordination for this release was done by @PieterGit 
We would like to thank the following people for their contribution (in alphabetical order):
@anderser, @apanasef, @balshor, @bewest, @blocklist_twitter, @CaroGo, @cascer1, @cluckj, @danamlewis, @Dave9111, @diabetlum, @herzogmedia, @janrpn, @jasoncalabrese, @jpcunningh, @jweismann, @kenstack, @Kywalh, @lixgbg, @LuminaryXion, @MilosKozak, @mitrei, @PaperT1D, @PieterGit, @unsoluble, @rarneson, @renegadeandy, @scottleibrand, @sulkaharo, @T-o-b-i-a-s, @tynbendad, @unsoluble, @veryfancy, @viq, @wootmasterslick

(if I forgot somebody, please respond)

# TODO

TODO: Translations, Languages with less than 80% will be removed in a future Nightscout version. Currently the following languages are at risk: 
中文(繁體) (zh_tw), Hrvatski (hr), Ελληνικά (el), 한국어 (ko)
See https://gitter.im/nightscout/public?at=5bef2f34de42d46bba766f66

TODO: Fix Codacy errors, https://app.codacy.com/app/Nightscout/cgm-remote-monitor/issues?bid=2452379&filters=W3siaWQiOiJDYXRlZ29yeSIsInZhbHVlcyI6WyJFcnJvciBQcm9uZSJdfV0=

TODO: test dev after all new features are merged for at least two weeks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants