Migrate the hidden read receipts flag to new "send read receipts" option

cypress/e2e/settings/hidden-rr-migration.spec.ts

@@ -0,0 +1,90 @@
+/*
+Copyright 2022 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+/// <reference types="cypress" />
+
+import { SynapseInstance } from "../../plugins/synapsedocker";
+
+function seedLabs(synapse: SynapseInstance, labsVal: boolean | null) {
+    cy.initTestUser(synapse, "Sally", () => {
+        // seed labs flag
+        cy.window({ log: false }).then(win => {
+            if (typeof labsVal === "boolean") {
+                // stringify boolean
+                win.localStorage.setItem("mx_labs_feature_feature_hidden_read_receipts", `${labsVal}`);
+            }
+        });
+    });
+}
+
+function testForVal(settingVal: boolean | null) {
+    const testRoomName = "READ RECEIPTS";
+    cy.createRoom({ name: testRoomName }).as("roomId");
+    cy.all([cy.get<string>("@roomId")]).then(() => {
+        cy.viewRoomByName(testRoomName).then(() => {
+            // if we can see the room, then sync is working for us. It's time to see if the
+            // migration even ran.
+
+            cy.getSettingValue("sendReadReceipts", null, true).should("satisfy", (val) => {
+                if (typeof settingVal === "boolean") {
+                    return val === settingVal;
+                } else {
+                    return !val; // falsy - we don't actually care if it's undefined, null, or a literal false
+                }
+            });
+        });
+    });
+}
+
+describe("Hidden Read Receipts Setting Migration", () => {
+    // We run this as a full-blown end-to-end test to ensure it works in an integration
+    // sense. If we unit tested it, we'd be testing that the code works but not that the
+    // migration actually runs.
+    //
+    // Here, we get to test that not only the code works but also that it gets run. Most
+    // of our interactions are with the JS console as we're honestly just checking that
+    // things got set correctly.
+    //
+    // For a security-sensitive feature like hidden read receipts, it's absolutely vital
+    // that we migrate the setting appropriately.
+
+    let synapse: SynapseInstance;
+
+    beforeEach(() => {
+        cy.startSynapse("default").then(data => {
+            synapse = data;
+        });
+    });
+
+    afterEach(() => {
+        cy.stopSynapse(synapse);
+    });
+
+    it('should not migrate the lack of a labs flag', () => {
+        seedLabs(synapse, null);
+        testForVal(null);
+    });
+
+    it('should migrate labsHiddenRR=false as sendRR=true', () => {
+        seedLabs(synapse, false);
+        testForVal(true);
+    });
+
+    it('should migrate labsHiddenRR=true as sendRR=false', () => {
+        seedLabs(synapse, true);
+        testForVal(false);
+    });
+});

cypress/support/login.ts

@@ -35,13 +35,19 @@ declare global {
              * Generates a test user and instantiates an Element session with that user.
              * @param synapse the synapse returned by startSynapse
              * @param displayName the displayName to give the test user
+             * @param prelaunchFn optional function to run before the app is visited
              */
-            initTestUser(synapse: SynapseInstance, displayName: string): Chainable<UserCredentials>;
+            initTestUser(
+                synapse: SynapseInstance,
+                displayName: string,
+                prelaunchFn?: () => void,
+            ): Chainable<UserCredentials>;
         }
     }
 }
 
-Cypress.Commands.add("initTestUser", (synapse: SynapseInstance, displayName: string): Chainable<UserCredentials> => {
+// eslint-disable-next-line max-len
+Cypress.Commands.add("initTestUser", (synapse: SynapseInstance, displayName: string, prelaunchFn?: () => void): Chainable<UserCredentials> => {
     // XXX: work around Cypress not clearing IDB between tests
     cy.window({ log: false }).then(win => {
         win.indexedDB.databases().then(databases => {
@@ -87,6 +93,8 @@ Cypress.Commands.add("initTestUser", (synapse: SynapseInstance, displayName: str
             win.localStorage.setItem("mx_local_settings", '{"language":"en"}');
         });
 
+        prelaunchFn?.();
+
         return cy.visit("/").then(() => {
             // wait for the app to load
             return cy.get(".mx_MatrixChat", { timeout: 15000 });

cypress/support/settings.ts

@@ -96,7 +96,7 @@ declare global {
              * value.
              * @return {*} The value, or null if not found
              */
-            getSettingValue<T>(name: string, roomId?: string): Chainable<T>;
+            getSettingValue<T>(name: string, roomId?: string, excludeDefault?: boolean): Chainable<T>;
         }
     }
 }
@@ -116,9 +116,10 @@ Cypress.Commands.add("setSettingValue", (
     });
 });
 
-Cypress.Commands.add("getSettingValue", <T = any>(name: string, roomId?: string): Chainable<T> => {
+// eslint-disable-next-line max-len
+Cypress.Commands.add("getSettingValue", <T = any>(name: string, roomId?: string, excludeDefault?: boolean): Chainable<T> => {
     return cy.getSettingsStore().then((store: typeof SettingsStore) => {
-        return store.getValue(name, roomId);
+        return store.getValue(name, roomId, excludeDefault);
     });
 });
 

src/Lifecycle.ts

@@ -826,6 +826,9 @@ async function startMatrixClient(startSyncing = true): Promise<void> {
         await MatrixClientPeg.assign();
     }
 
+    // Run the migrations after the MatrixClientPeg has been assigned
+    SettingsStore.runMigrations();
+
     // This needs to be started after crypto is set up
     DeviceListener.sharedInstance().start();
     // Similarly, don't start sending presence updates until we've started

src/settings/SettingsStore.ts

@@ -35,6 +35,9 @@ import SettingsHandler from "./handlers/SettingsHandler";
 import { SettingUpdatedPayload } from "../dispatcher/payloads/SettingUpdatedPayload";
 import { Action } from "../dispatcher/actions";
 import PlatformSettingsHandler from "./handlers/PlatformSettingsHandler";
+import dispatcher from "../dispatcher/dispatcher";
+import { ActionPayload } from "../dispatcher/payloads";
+import { MatrixClientPeg } from "../MatrixClientPeg";
 
 const defaultWatchManager = new WatchManager();
 
@@ -565,6 +568,44 @@ export default class SettingsStore {
         return null;
     }
 
+    /**
+     * Runs or queues any setting migrations needed.
+     */
+    public static runMigrations() {
+        // Dev notes: to add your migration, just add a new `migrateMyFeature` function, call it, and
+        // add a comment to note when it can be removed.
+
+        SettingsStore.migrateHiddenReadReceipts(); // Can be removed after October 2022.
+    }
+
+    private static migrateHiddenReadReceipts() {
+        if (MatrixClientPeg.get().isGuest()) return; // not worth it
+
+        // We wait for the first sync to ensure that the user's existing account data has loaded, as otherwise
+        // getValue() for an account-level setting like sendReadReceipts will return `null`.
+        const disRef = dispatcher.register((payload: ActionPayload) => {
+            if (payload.action === "MatrixActions.sync") {
+                dispatcher.unregister(disRef);
+
+                const rrVal = SettingsStore.getValue("sendReadReceipts", null, true);
+                if (typeof rrVal !== "boolean") {
+                    // new setting isn't set - see if the labs flag was. We have to manually reach into the
+                    // handler for this because it isn't a setting anymore (`getValue` will yell at us).
+                    const handler = LEVEL_HANDLERS[SettingLevel.DEVICE] as DeviceSettingsHandler;
+                    const labsVal = handler.readFeature("feature_hidden_read_receipts");
+                    if (typeof labsVal === "boolean") {
+                        // Inverse of labs flag because negative->positive language switch in setting name
+                        const newVal = !labsVal;
+                        console.log(`Setting sendReadReceipts to ${newVal} because of previously-set labs flag`);
+
+                        // noinspection JSIgnoredPromiseFromCall
+                        SettingsStore.setValue("sendReadReceipts", null, SettingLevel.ACCOUNT, newVal);
+                    }
+                }
+            }
+        });
+    }
+
     /**
      * Debugging function for reading explicit setting values without going through the
      * complicated/biased functions in the SettingsStore. This will print information to

src/settings/handlers/DeviceSettingsHandler.ts

@@ -114,12 +114,16 @@ export default class DeviceSettingsHandler extends AbstractLocalStorageSettingsH
     // Note: features intentionally don't use the same key as settings to avoid conflicts
     // and to be backwards compatible.
 
-    private readFeature(featureName: string): boolean | null {
+    // public for access to migrations - not exposed from the SettingsHandler interface
+    public readFeature(featureName: string): boolean | null {
         if (MatrixClientPeg.get() && MatrixClientPeg.get().isGuest()) {
             // Guests should not have any labs features enabled.
             return false;
         }
 
+        // XXX: This turns they key names into `mx_labs_feature_feature_x` (double feature).
+        // This is because all feature names start with `feature_` as a matter of policy.
+        // Oh well.
         return this.getBoolean("mx_labs_feature_" + featureName);
     }