Merge pull request #554 from 200ok-ch/fix/settings-persistance-3

fix: Remove previously stored bogus values from localStorage

changelog.org

@@ -4,6 +4,16 @@ All user visible changes to organice will be documented in this file.
 
 When there are updates to the changelog, you will be notified and see a 'gift' icon appear on the top right corner.
 
+* [2020-11-01 Sun]
+** Fixed
+   - organice has various settings that the user can configure. Before manual configuration, there organice loads sane defaults. Loading and persisting some of these defaults was buggy before.
+     - Loading and persisting of defaults works now.
+     - Previously saved wrong values are removed from =localStorage= to reduce future bug potential.
+     - Changing this is - strictly speaking - not visible to the end-user, so it wouldn't ordinarily show up in this changelog. However, since it makes changes to the already saved settings (in cleaning up old faulty values), it theoretically could introduce a bug in the settings. Hence, the change is added to the changelog.
+     - Related PRs:
+       - /~https://github.com/200ok-ch/organice/pull/552
+       - /~https://github.com/200ok-ch/organice/pull/553
+       - /~https://github.com/200ok-ch/organice/pull/554
 * [2020-10-25 Sun]
 ** Changed
    - The 'focus header' feature is renamed.

src/migrations/index.js

@@ -1,7 +1,9 @@
 import migrateAccessTokenToDropboxAccessToken from './migrate_access_token_to_dropbox_access_token';
 import migrateStoreInDropboxToStoreInSyncBackend from './migrate_store_in_dropbox_to_store_in_sync_backend';
+import migrateNonsenseValuesInLocalstorage from './migrate_nonsense_values_in_localstorage';
 
 export default () => {
   migrateAccessTokenToDropboxAccessToken();
   migrateStoreInDropboxToStoreInSyncBackend();
+  migrateNonsenseValuesInLocalstorage();
 };

src/migrations/migrate_nonsense_values_in_localstorage.js

@@ -0,0 +1,15 @@
+import { isLocalStorageAvailable } from '../util/settings_persister';
+
+export default () => {
+  if (!isLocalStorageAvailable) {
+    return;
+  }
+
+  Object.entries(localStorage).forEach(([k, v], _) => {
+    if (['null', 'undefined'].includes(v)) {
+      console.warn(`localStorage contains a bogus entry: '${k}': '${v}'`);
+      console.warn('Deleting the bogus entry.');
+      localStorage.removeItem(k);
+    }
+  });
+};

src/util/settings_persister.js

@@ -11,7 +11,9 @@ import generateId from '../lib/id_generator';
 export const isLocalStorageAvailable = () => {
   try {
     localStorage.setItem('test', 'test');
-    return localStorage.getItem('test') === 'test';
+    const localStorageRes = localStorage.getItem('test') === 'test';
+    localStorage.removeItem('test');
+    return localStorageRes && localStorage;
   } catch (e) {
     return false;
   }
@@ -139,6 +141,7 @@ export const persistableFields = [
     category: 'base',
     name: 'colorScheme',
     type: 'string',
+    default: 'Light',
     shouldStoreInConfig: true,
   },
   {
@@ -155,27 +158,29 @@ export const readOpennessState = () => {
   return !!opennessStateJSONString ? JSON.parse(opennessStateJSONString) : null;
 };
 
-const getFieldsToPersist = (state, fields) =>
-  fields
+const getFieldsToPersist = (state, fields) => {
+  return fields
     .filter((field) => !field.depreacted)
     .filter((field) => field.category === 'org')
     .map((field) => field.name)
     .map((field) => [field, state.org.present.get(field)])
     .concat(
       persistableFields
         .filter((field) => field.category !== 'org')
-        .map((field) =>
-          field.type === 'json'
+        .map((field) => {
+          return field.type === 'json'
             ? [
                 field.name,
                 JSON.stringify(state[field.category].get(field.name) || field.default || {}),
               ]
-            : [field.name, state[field.category].get(field.name)]
-        )
+            : [field.name, state[field.category].get(field.name) || field.default];
+        })
     );
+};
 
-const getConfigFileContents = (fieldsToPersist) =>
-  JSON.stringify(_.fromPairs(fieldsToPersist), null, 2);
+const getConfigFileContents = (fieldsToPersist) => {
+  return JSON.stringify(_.fromPairs(fieldsToPersist), null, 2);
+};
 
 export const applyCategorySettingsFromConfig = (state, config, category) => {
   persistableFields
@@ -258,7 +263,8 @@ export const readInitialState = () => {
     initialState.org.present = initialState.org.present.set('opennessState', fromJS(opennessState));
   }
 
-  // Cache the config file contents locally so we don't overwrite on initial page load.
+  // Cache the config file contents locally so we don't overwrite on
+  // initial page load.
   window.previousSettingsFileContents = getConfigFileContents(
     getFieldsToPersist(initialState, persistableFields)
   );
@@ -310,7 +316,9 @@ export const subscribeToChanges = (store) => {
 
       const fieldsToPersist = getFieldsToPersist(state, persistableFields);
 
-      fieldsToPersist.forEach(([name, value]) => localStorage.setItem(name, value));
+      fieldsToPersist.forEach(([name, value]) => {
+        if (name && value) localStorage.setItem(name, value);
+      });
 
       if (state.base.get('shouldStoreSettingsInSyncBackend')) {
         const settingsFileContents = getConfigFileContents(fieldsToPersist);

src/util/settings_persister.test.js

@@ -0,0 +1,24 @@
+import Store from '../store';
+import { readInitialState, subscribeToChanges } from './settings_persister';
+
+describe('Settings persister', () => {
+  let store;
+  beforeEach(() => {
+    const initialState = readInitialState();
+    store = Store(initialState);
+    subscribeToChanges(store)();
+  });
+
+  afterEach(() => {
+    localStorage.clear();
+  });
+
+  test('Do not persist nonsense like like "false" for settings without default', () => {
+    expect(localStorage.getItem('showClockDisplay')).not.toBe('false');
+    expect(localStorage.getItem('showClockDisplay')).toBe(null);
+  });
+
+  test('Does persist given default values, for example colorScheme', () => {
+    expect(localStorage.getItem('colorScheme')).toBe('Light');
+  });
+});