From c65a88fc9f6f7312b704e813f21030b140e6615a Mon Sep 17 00:00:00 2001
From: Paul Lessing <paul@paullessing.com>
Date: Tue, 28 Jun 2022 19:32:54 +0100
Subject: [PATCH 1/3] Fix: Changing security settings should not implicitly
 save allowedDomains

---
 app/scenes/Settings/Security.tsx | 94 ++++++++++++++++++--------------
 app/stores/AuthStore.ts          |  1 +
 2 files changed, 53 insertions(+), 42 deletions(-)

diff --git a/app/scenes/Settings/Security.tsx b/app/scenes/Settings/Security.tsx
index f978340d5..f56d2ab2d 100644
--- a/app/scenes/Settings/Security.tsx
+++ b/app/scenes/Settings/Security.tsx
@@ -36,9 +36,12 @@ function Security() {
     defaultUserRole: team.defaultUserRole,
     memberCollectionCreate: team.memberCollectionCreate,
     inviteRequired: team.inviteRequired,
-    allowedDomains: team.allowedDomains,
   });
 
+  const [allowedDomains, setAllowedDomains] = useState([
+    ...(team.allowedDomains ?? []),
+  ]);
+
   const authenticationMethods = team.signinMethods;
 
   const showSuccessMessage = React.useMemo(
@@ -59,9 +62,7 @@ function Security() {
         setData(newData);
         await auth.updateTeam(newData);
         showSuccessMessage();
-        setDomainsChanged(false);
       } catch (err) {
-        setDomainsChanged(true);
         showToast(err.message, {
           type: "error",
         });
@@ -77,6 +78,21 @@ function Security() {
     [data, saveData]
   );
 
+  const handleSaveDomains = React.useCallback(async () => {
+    try {
+      await auth.updateTeam({
+        allowedDomains,
+      });
+      showSuccessMessage();
+      setDomainsChanged(false);
+    } catch (err) {
+      setDomainsChanged(true);
+      showToast(err.message, {
+        type: "error",
+      });
+    }
+  }, [auth, allowedDomains, showSuccessMessage, showToast]);
+
   const handleDefaultRoleChange = React.useCallback(
     async (newDefaultRole: string) => {
       await saveData({ ...data, defaultUserRole: newDefaultRole });
@@ -123,31 +139,26 @@ function Security() {
   );
 
   const handleRemoveDomain = async (index: number) => {
-    const newData = {
-      ...data,
-    };
-    newData.allowedDomains && newData.allowedDomains.splice(index, 1);
+    const newDomains = allowedDomains.filter((_, i) => index !== i);
 
-    setData(newData);
+    setAllowedDomains(newDomains);
     setDomainsChanged(true);
   };
 
   const handleAddDomain = () => {
-    const newData = {
-      ...data,
-      allowedDomains: [...(data.allowedDomains || []), ""],
-    };
+    const newDomains = [...allowedDomains, ""];
 
-    setData(newData);
+    setAllowedDomains(newDomains);
+    setDomainsChanged(true);
   };
 
   const createOnDomainChangedHandler = (index: number) => (
     ev: React.ChangeEvent<HTMLInputElement>
   ) => {
-    const newData = { ...data };
+    const newDomains = allowedDomains.slice();
 
-    newData.allowedDomains![index] = ev.currentTarget.value;
-    setData(newData);
+    newDomains[index] = ev.currentTarget.value;
+    setAllowedDomains(newDomains);
     setDomainsChanged(true);
   };
 
@@ -269,35 +280,34 @@ function Security() {
           "The domains which should be allowed to create accounts. This applies to both SSO and Email logins. Changing this setting does not affect existing user accounts."
         )}
       >
-        {data.allowedDomains &&
-          data.allowedDomains.map((domain, index) => (
-            <Flex key={index} gap={4}>
-              <Input
-                key={index}
-                id={`allowedDomains${index}`}
-                value={domain}
-                autoFocus={!domain}
-                placeholder="example.com"
-                required
-                flex
-                onChange={createOnDomainChangedHandler(index)}
-              />
-              <Remove>
-                <Tooltip tooltip={t("Remove domain")} placement="top">
-                  <NudeButton onClick={() => handleRemoveDomain(index)}>
-                    <CloseIcon />
-                  </NudeButton>
-                </Tooltip>
-              </Remove>
-            </Flex>
-          ))}
+        {allowedDomains.map((domain, index) => (
+          <Flex key={index} gap={4}>
+            <Input
+              key={index}
+              id={`allowedDomains${index}`}
+              value={domain}
+              autoFocus={!domain}
+              placeholder="example.com"
+              required
+              flex
+              onChange={createOnDomainChangedHandler(index)}
+            />
+            <Remove>
+              <Tooltip tooltip={t("Remove domain")} placement="top">
+                <NudeButton onClick={() => handleRemoveDomain(index)}>
+                  <CloseIcon />
+                </NudeButton>
+              </Tooltip>
+            </Remove>
+          </Flex>
+        ))}
 
         <Flex justify="space-between" gap={4} style={{ flexWrap: "wrap" }}>
-          {!data.allowedDomains?.length ||
-          data.allowedDomains[data.allowedDomains.length - 1] !== "" ? (
+          {!allowedDomains.length ||
+          allowedDomains[allowedDomains.length - 1] !== "" ? (
             <Fade>
               <Button type="button" onClick={handleAddDomain} neutral>
-                {data.allowedDomains?.length ? (
+                {allowedDomains.length ? (
                   <Trans>Add another</Trans>
                 ) : (
                   <Trans>Add a domain</Trans>
@@ -312,7 +322,7 @@ function Security() {
             <Fade>
               <Button
                 type="button"
-                onClick={handleChange}
+                onClick={handleSaveDomains}
                 disabled={auth.isSaving}
               >
                 <Trans>Save changes</Trans>
diff --git a/app/stores/AuthStore.ts b/app/stores/AuthStore.ts
index da47ae944..fa2a20343 100644
--- a/app/stores/AuthStore.ts
+++ b/app/stores/AuthStore.ts
@@ -236,6 +236,7 @@ export default class AuthStore {
     collaborativeEditing?: boolean;
     defaultCollectionId?: string | null;
     subdomain?: string | null | undefined;
+    allowedDomains?: string[] | null | undefined;
   }) => {
     this.isSaving = true;
 

From 117421b4cba19ccde7a584f34bb0f08f702c7121 Mon Sep 17 00:00:00 2001
From: Paul Lessing <paul@paullessing.com>
Date: Wed, 29 Jun 2022 08:33:07 +0100
Subject: [PATCH 2/3] Feat: Only show save domains button if changes were made

The logic for this is that we show the button if either:
a) one or more new non-empty domains have been added, or
b) an existing domain was modified, even if the modification was then undone.

The reasoning for b) is as follows:
If a user adds a new domain row, makes changes, then removes the domain row, it is clear to the user that no changes have been made, and therefore the "save" button should not be visible.
However, as soon as the user makes any changes to an existing domain, they want to feel confident that they can hit save and ensure that whatever change they made is persisted; even if the change is identical to the current state, because they may not be able to recall accurately what the current state was. In those situations a user gets more confidence out of being able to hit save, than they would from being told by the system "you haven't made any changes".
---
 app/scenes/Settings/Security.tsx | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/app/scenes/Settings/Security.tsx b/app/scenes/Settings/Security.tsx
index f56d2ab2d..19b98ca3d 100644
--- a/app/scenes/Settings/Security.tsx
+++ b/app/scenes/Settings/Security.tsx
@@ -41,6 +41,16 @@ function Security() {
   const [allowedDomains, setAllowedDomains] = useState([
     ...(team.allowedDomains ?? []),
   ]);
+  const [lastKnownDomainCount, updateLastKnownDomainCount] = useState(
+    allowedDomains.length
+  );
+
+  const [existingDomainsTouched, setExistingDomainsTouched] = useState(false);
+
+  const showSaveDomainsButton = React.useCallback(() => {
+    const validDomains = allowedDomains.filter((value) => value !== "");
+    return existingDomainsTouched || validDomains.length > lastKnownDomainCount;
+  }, [existingDomainsTouched, allowedDomains, lastKnownDomainCount]);
 
   const authenticationMethods = team.signinMethods;
 
@@ -54,8 +64,6 @@ function Security() {
     [showToast, t]
   );
 
-  const [domainsChanged, setDomainsChanged] = useState(false);
-
   const saveData = React.useCallback(
     async (newData) => {
       try {
@@ -84,9 +92,9 @@ function Security() {
         allowedDomains,
       });
       showSuccessMessage();
-      setDomainsChanged(false);
+      setExistingDomainsTouched(false);
+      updateLastKnownDomainCount(allowedDomains.length);
     } catch (err) {
-      setDomainsChanged(true);
       showToast(err.message, {
         type: "error",
       });
@@ -142,14 +150,17 @@ function Security() {
     const newDomains = allowedDomains.filter((_, i) => index !== i);
 
     setAllowedDomains(newDomains);
-    setDomainsChanged(true);
+
+    const touchedExistingDomain = index < lastKnownDomainCount;
+    if (touchedExistingDomain) {
+      setExistingDomainsTouched(true);
+    }
   };
 
   const handleAddDomain = () => {
     const newDomains = [...allowedDomains, ""];
 
     setAllowedDomains(newDomains);
-    setDomainsChanged(true);
   };
 
   const createOnDomainChangedHandler = (index: number) => (
@@ -159,7 +170,11 @@ function Security() {
 
     newDomains[index] = ev.currentTarget.value;
     setAllowedDomains(newDomains);
-    setDomainsChanged(true);
+
+    const touchedExistingDomain = index < lastKnownDomainCount;
+    if (touchedExistingDomain) {
+      setExistingDomainsTouched(true);
+    }
   };
 
   return (
@@ -318,7 +333,7 @@ function Security() {
             <span />
           )}
 
-          {domainsChanged && (
+          {showSaveDomainsButton() && (
             <Fade>
               <Button
                 type="button"

From d8f125f413780f1df0ffb192f096d162385327e0 Mon Sep 17 00:00:00 2001
From: Paul Lessing <paul@paullessing.com>
Date: Thu, 7 Jul 2022 11:01:32 +0100
Subject: [PATCH 3/3] Fix: Move logic inline

---
 app/scenes/Settings/Security.tsx | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/app/scenes/Settings/Security.tsx b/app/scenes/Settings/Security.tsx
index 19b98ca3d..b4292fae3 100644
--- a/app/scenes/Settings/Security.tsx
+++ b/app/scenes/Settings/Security.tsx
@@ -47,11 +47,6 @@ function Security() {
 
   const [existingDomainsTouched, setExistingDomainsTouched] = useState(false);
 
-  const showSaveDomainsButton = React.useCallback(() => {
-    const validDomains = allowedDomains.filter((value) => value !== "");
-    return existingDomainsTouched || validDomains.length > lastKnownDomainCount;
-  }, [existingDomainsTouched, allowedDomains, lastKnownDomainCount]);
-
   const authenticationMethods = team.signinMethods;
 
   const showSuccessMessage = React.useMemo(
@@ -177,6 +172,8 @@ function Security() {
     }
   };
 
+  const excludeEmpty = (value: unknown) => value !== "";
+
   return (
     <Scene title={t("Security")} icon={<PadlockIcon color="currentColor" />}>
       <Heading>{t("Security")}</Heading>
@@ -333,7 +330,9 @@ function Security() {
             <span />
           )}
 
-          {showSaveDomainsButton() && (
+          {(existingDomainsTouched ||
+            allowedDomains.filter(excludeEmpty).length > // New domains were added
+              lastKnownDomainCount) && (
             <Fade>
               <Button
                 type="button"