Merge pull request #691 from lithictech/role-access-fixes

Role access fixes

adminapp/src/hooks/globalApiState.jsx

@@ -15,34 +15,62 @@ const loading = Symbol("globalApiStateLoading");
  * @param {object=} options
  * @param {function=} options.pick Called with the response, to pick the data for the state.
  *   Defaults to `(r) => r.data`.
- * @returns {*}
+ * @returns {*} The default value before the API call is resolved
+ *   (before it's made, while it's pending, if it's rejected),
+ *   or the picked value (like response.data) once the API call is resolved.
  */
 export function useGlobalApiState(apiGet, defaultValue, options) {
-  options = merge({ pick: (r) => r.data }, options);
-  const { state, setState } = React.useContext(GlobalApiStateContext);
+  options = merge({}, defaultOptions, options);
+  const { hasKey, getKey, setKey } = React.useContext(GlobalApiStateContext);
   const { enqueueErrorSnackbar } = useErrorSnackbar();
   const key = "" + apiGet;
   React.useEffect(() => {
-    if (has(state, key)) {
+    if (hasKey(key)) {
       return;
     }
     apiGet()
-      .then((r) => setState({ ...state, [key]: r }))
+      .then((r) => setKey(key, r))
       .catch(enqueueErrorSnackbar);
-    setState({ ...state, [key]: loading });
-  }, [enqueueErrorSnackbar, state, apiGet, setState, key, options]);
-  if (has(state, key)) {
-    const r = state[key];
+    setKey(key, loading);
+  }, [enqueueErrorSnackbar, apiGet, key, hasKey, setKey]);
+  if (hasKey(key)) {
+    const r = getKey(key);
     return r === loading ? defaultValue : options.pick(r);
   }
   return defaultValue;
 }
 
 export function GlobalApiStateProvider({ children }) {
-  const [state, setState] = React.useState({});
+  // Under the hood, we need to save the state into a ref,
+  // because we don't want to make multiple API calls for the same request.
+  // But it is very easy for children to be working with different views of the state
+  // (ie, when useGlobalApiState is used in multiple places in the tree)
+  // and trigger multiple api calls (because both components are adding keys when they render).
+  //
+  // Whenever we write to the ref, we also modify the state,
+  // which causes children to re-render, calling getKey and seeing the new state stored in the ref.
+  //
+  // If we just had the ref (no state), modifying the ref would not cause any re-renders
+  // and children would not see the new data.
+  const storage = React.useRef({});
+  const [dummyState, setDummyState] = React.useState({});
+  const hasKey = React.useCallback((k) => has(storage.current, k), [storage]);
+  const getKey = React.useCallback((k) => storage.current[k], [storage]);
+  const setKey = React.useCallback(
+    (k, v) => {
+      storage.current = { ...storage.current, [k]: v };
+      setDummyState({ ...dummyState, [k]: v });
+    },
+    [dummyState]
+  );
   return (
-    <GlobalApiStateContext.Provider value={{ state, setState }}>
+    <GlobalApiStateContext.Provider value={{ hasKey, getKey, setKey }}>
       {children}
     </GlobalApiStateContext.Provider>
   );
 }
+
+function pickData(r) {
+  return r.data;
+}
+const defaultOptions = { pick: pickData };

adminapp/src/pages/MemberForm.jsx

@@ -7,7 +7,6 @@ import { useGlobalApiState } from "../hooks/globalApiState";
 import useRoleAccess from "../hooks/useRoleAccess";
 import mergeAt from "../shared/mergeAt";
 import withoutAt from "../shared/withoutAt";
-import theme from "../theme";
 import AddIcon from "@mui/icons-material/Add";
 import AddCircleOutlineIcon from "@mui/icons-material/AddCircleOutline";
 import DeleteIcon from "@mui/icons-material/Delete";
@@ -112,7 +111,7 @@ function Roles({ roles, setRoles }) {
       <FormHelperText>
         If you remove special roles like "admin", you will be logged out of this account.
       </FormHelperText>
-      <Stack direction="row" spacing={1} sx={{ mt: theme.spacing(1) }}>
+      <Stack direction="row" gap={1} sx={{ marginTop: 1, flexWrap: "wrap" }}>
         {allRoles === null && <CircularProgress />}
         {allRoles?.map((r) => {
           const hasRole = hasRoleIds.has(r.id);
@@ -129,7 +128,7 @@ function Roles({ roles, setRoles }) {
             />
           );
         })}
-        {allRoles === [] && (
+        {allRoles && allRoles.length === 0 && (
           <Typography>
             * No roles available, ask developers for help if you see this
           </Typography>

db/migrations/052_new_roles.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+Sequel.migration do
+  up do
+    ["upload_files", "admin", "onboarding_manager", "admin_readonly"].each do |name|
+      from(:roles).
+        insert_conflict.
+        insert(name:)
+    end
+  end
+end