Skip to content

Fix client reset on start #470

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

Merged
merged 16 commits into from
Oct 19, 2018
Merged

Conversation

shankari
Copy link
Contributor

Main change is to switch to native storage, consistent with https://github.com/e-mission/e-mission-phone/issues/443#issuecomment-423066729

Bonus fixes:

  • Move native code in other screens to after platform.ready
  • Clarify the prompted-auth example

I guess this has been working on production due to timing issues but it is not
a long-term fix. All native code should really be inside an ionicPlatform.ready
block and not in random places during file load. This includes all callbacks
Back in the old days, we tried to move all storage to local storage so that we
could read it without waiting for ionicPlatform.ready and startup quickly.
Well, it turned out that sometimes local storage was deleted on iOS, so we
ended up with the case where local intro_done was deleted, so we would reset the UI.
Which looked stupid and like a bug (https://github.com/e-mission/e-mission-phone/issues/443).

Re-writing to pull out the KV storage into a service, write primarily to native
with local as backup, and read from both and unify.

Everything was already promisified because we were reading consent from native
code, so I am not sure that this actually affects performance. And it is a heck
of a lot easier to understand.

Testing done:
- started app -> went to intro
- restarted app after restarting emulator -> went to current screen. For fixes to the current screen, see 0ecfdae
- cleared out intro_done and consent from local storage and restarted app -> went to current screen
- cleared all native storage, generated popups for intro_done and consent, then crashed due to e-mission/e-mission-transition-notify#18 On relaunch, launched properly without any popups. I declare that this is done.

```
[phonegap] [console.log] DEBUG:uc_stored_val = null ls_stored_val = {"intro_done":"2018-09-25T13:14:51-07:00"}
```
To indicate that you need to enter a prompt, not a token
so that we know when it happened and it is not just in a popup that people
might have ignored.
@krisma really really wanted it, but there is no evidence that anybody else
cares. And there's a bunch of random code sitting around in here making it
complex.  Removing it since we are cleaning up the base code anyway.
@shankari
Copy link
Contributor Author

shankari commented Sep 27, 2018

Dark theme is now gone for good

$ grep -r dark www/js
$ grep -r dark www/templates/
www/templates//common/place-list.html:<!--   <ion-nav-title class="dark-color">
www/templates//common/trip-list.html:<!--   <ion-nav-title class="dark-color">
www/templates//diary/list.html:        <div class="buttons dark-color" style="text-align:center; background-color: transparent !important;" id="toget">

We want to move away from angularLocalStorage, since local storage can be
deleted by the OS, at least on iOS, when the device is low on disk space.
This means that we can't store persistent data in local storage, only session
data.

In a09385b, fixed this for the startup code.
This is the first of a series of changes that will refactor other code code to
remove all instances.

In this change, we remove the module dependency if it is no longer used.
Simple changes!
In particular:
- metrics stores the user display preference for calories
- metrics stores the user BMI data to correct for calorie burn
- updatecheck stores the channel

Also change the control panel to read the data using promises instead of
directly from local storage.

This also involved minor changes (function -> field) that also required changes
to the HTML.

Also, add new clear methods to the storage function and call them from the
control panel.

Testing done:
- Navigated between dashboard and profile multiple times with no errors
- Set the user information in the dashboard - verified that it was visible in
  the profile
error is JSON, plugin expects string, so we can't deserialize properly
This is intended to return data that:
- we want to return inline (for easy porting)
- is not catastrophic if it is cleared out (e.g. walkthrough code), OR
- is used primarily for session storage so will not be cleared out (e.g. referral code)

We can replace this with promisified native storage in a future PR if needed
The code does copy the native value to local storage in the background,
so even if this is stripped out, it will work on retry.
goals code in general switches to native storage, but
the referral code alone, since it is not intended for long-term storage, uses
the new local-only storage.

Note that we can change the referral code to store a single object that
includes all referral objects, e.g.

{
    REFERRED_KEY:
    REFERRED_GROUP_ID:
    REFERRED_USER_ID:
    REFERRAL_NAVIGATION_KEY:
}
...

I had initially implemented this change, but the navigation key is modified
separately from the others, so we would need to deal with it correctly, and I
don't really want to re-test this code when nobody is actively using it.

Bonus fix: clean up the survey code that we had marked with a TODO
This was a bit tricky because of the way that the slider is hooked into the
variable.

If you read the data immediately without promisifying AND the load happens
before the store, then the slider is set to the loaded value and the store
retains the correct value.

HOWEVER, if you read the data through a promise, then you get the store first,
and without additional checks, the value is reset to the default.

The solution is to add additional checks to only store valid values.
@shankari
Copy link
Contributor Author

shankari commented Oct 2, 2018

Other modules also stored data in local storage. Moved almost all of them to native storage.
Exceptions:

  • referral, because that is essentially session storage and can be recreated by re-clicking the invite link
  • walkthrough, because the consequences if it occasionally gets deleted are pretty minor

I also tried to move out the walkthrough into its own module for clarity but that seems to subtly interfere with the timing, so moved it back.

@shankari
Copy link
Contributor Author

shankari commented Oct 2, 2018

Future improvements may include:

  • moving everything to native storage
  • really refactoring stuff properly into services and factories - e.g. the display code for metrics, the walkthrough code, etc
  • making sure that all startup code is bunched properly and invoked when the platform is ready instead of being strewn all over the place in the body of the controller.

@shankari shankari mentioned this pull request Oct 12, 2018
Split out the munging into its own function.
And then use the new functin to temporarily munge values before syncing local
-> remote
There are multiple reasons for this:
- If the user has not logged in, this generates an error
- The most common use case is for the user to set the channel and then log in
  In that case, this fails anyway.
- The channels typically set their own values in their code anyway

```
  $scope.login = function() {
    window.cordova.plugins.BEMJWTAuth.signIn().then(function(userEmail) {
      // ionicToast.show(message, position, stick, time);
      // $scope.next();
      ionicToast.show(userEmail, 'middle', false, 2500);
      CommHelper.registerUser(function(successResult) {
        $scope.finish();
      }, function(errorResult) {
        $scope.alertError('User registration error', errorResult);
        $scope.finish();
      });
    }, function(error) {
        $scope.alertError('Sign in error', error);
        $scope.finish();
    });
  };
```
Since we no longer update the channel on the server when we set the client
(2737600)
If you are returning `store_val`, make sure to define `store_val` and not `storage_val`.
Without this fix, we get the error

```
variable store_val not found
```
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.

1 participant