Merge pull request #2649 from objectcomputing/setting-option-values

mkimberlin · web-flow · commit 7cb33422a548 · 2024-10-21T13:38:27.000-05:00
Setting categories are not grouped properly
diff --git a/server/src/main/java/com/objectcomputing/checkins/services/settings/SettingOption.java b/server/src/main/java/com/objectcomputing/checkins/services/settings/SettingOption.java
@@ -16,28 +16,26 @@
 @JsonDeserialize(using = SettingOptionDeserializer.class)
 public enum SettingOption {
     LOGO_URL("The logo url", Category.THEME, Type.FILE),
-    PULSE_EMAIL_FREQUENCY("The Pulse Email Frequency (weekly, bi-weekly, monthly)", Category.CHECK_INS, Type.STRING);
+    PULSE_EMAIL_FREQUENCY("The Pulse Email Frequency", Category.CHECK_INS, Type.STRING, List.of("weekly", "bi-weekly", "monthly"));
 
     private final String description;
     private final Category category;
     private final Type type;
+    private final List<String> values;
 
     SettingOption(String description, Category category, Type type) {
         this.description = description;
         this.category = category;
         this.type = type;
+        this.values = List.of();
     }
 
-    public String getDescription() {
-        return description;
-    }
-
-    public Category getCategory() {
-        return category;
-    }
-
-    public Type getType() {
-        return type;
+    SettingOption(String description, Category category, Type type,
+                  List<String> values) {
+        this.description = description;
+        this.category = category;
+        this.type = type;
+        this.values = values;
     }
 
     public static List<SettingOption> getOptions(){
diff --git a/server/src/main/java/com/objectcomputing/checkins/services/settings/SettingOptionSerializer.java b/server/src/main/java/com/objectcomputing/checkins/services/settings/SettingOptionSerializer.java
@@ -5,6 +5,7 @@
 import com.fasterxml.jackson.databind.ser.std.StdSerializer;
 
 import java.io.IOException;
+import java.util.List;
 
 public class SettingOptionSerializer extends StdSerializer<SettingOption> {
 
@@ -28,6 +29,15 @@ public void serialize(
         generator.writeString(settingOption.getCategory().name());
         generator.writeFieldName("type");
         generator.writeString(settingOption.getType().name());
+        List<String> values = settingOption.getValues();
+        if (!values.isEmpty()) {
+          generator.writeFieldName("values");
+          generator.writeStartArray(values.size());
+          for(String value : values) {
+            generator.writeString(value);
+          }
+          generator.writeEndArray();
+        }
         generator.writeEndObject();
     }
-}
+}
diff --git a/server/src/main/java/com/objectcomputing/checkins/services/settings/SettingsController.java b/server/src/main/java/com/objectcomputing/checkins/services/settings/SettingsController.java
@@ -76,10 +76,8 @@ public SettingsResponseDTO findByName(@PathVariable @NotNull String name) {
     public List<SettingsResponseDTO> getOptions() {
         List<SettingOption> options = SettingOption.getOptions();
         return options.stream().map(option -> {
-                   // Default to an empty value and "invalid" UUID.
-                   // This can be used by the client to determine pre-existance.
                    String value = "";
-                   UUID uuid = new UUID(0, 0);
+                   UUID uuid = null;
                    try {
                        Setting s = settingsServices.findByName(option.name());
                        uuid = s.getId();
@@ -89,6 +87,7 @@ public List<SettingsResponseDTO> getOptions() {
                    return new SettingsResponseDTO(
                                   uuid, option.name(), option.getDescription(),
                                   option.getCategory(), option.getType(),
+                                  option.getValues(),
                                   value);
                }).toList();
     }
@@ -150,6 +149,7 @@ private SettingsResponseDTO fromEntity(Setting entity) {
         dto.setDescription(option.getDescription());
         dto.setCategory(option.getCategory());
         dto.setType(option.getType());
+        dto.setValues(option.getValues());
         dto.setValue(entity.getValue());
         return dto;
     }
diff --git a/server/src/main/java/com/objectcomputing/checkins/services/settings/SettingsResponseDTO.java b/server/src/main/java/com/objectcomputing/checkins/services/settings/SettingsResponseDTO.java
@@ -9,6 +9,7 @@
 import lombok.Setter;
 
 import java.util.UUID;
+import java.util.List;
 
 @Setter
 @Getter
@@ -36,6 +37,10 @@ public class SettingsResponseDTO {
     @Schema(description = "type of the setting")
     private SettingOption.Type type;
 
+    @NotNull
+    @Schema(description = "possible values for the setting")
+    private List<String> values;
+
     @NotBlank
     @Schema(description = "value of the setting")
     private String value;
diff --git a/web-ui/src/components/settings/types/number.jsx b/web-ui/src/components/settings/types/number.jsx
@@ -1,5 +1,11 @@
 import React from 'react';
-import { Input, Typography } from '@mui/material';
+import {
+  Select,
+  MenuItem,
+  ListItemText,
+  Input,
+  Typography
+} from '@mui/material';
 import { createLabelId } from '../../../helpers/strings.js';
 
 /**
@@ -13,7 +19,7 @@ import { createLabelId } from '../../../helpers/strings.js';
  * @param {function} props.handleChange - The callback function to handle value changes.
  * @returns {JSX.Element} - The rendered component.
  */
-const SettingsNumber = ({ name, description, value, handleChange }) => {
+const SettingsNumber = ({ name, description, values, value, handleChange }) => {
   const labelId = createLabelId(name);
 
   return (
@@ -24,13 +30,25 @@ const SettingsNumber = ({ name, description, value, handleChange }) => {
         </Typography>
       </label>
       {description && <p>{description}</p>}
+      {values && values.length > 0 ?
+      <Select
+        labelId={labelId}
+        value={value}
+        onChange={handleChange}
+      >
+        {values.map((option) => (
+          <MenuItem key={option} value={option}>
+            <ListItemText primary={option} />
+          </MenuItem>
+        ))}
+      </Select> :
       <Input
         id={labelId}
         className="settings-control"
         type="number"
         value={value}
         onChange={handleChange}
-      />
+      />}
     </div>
   );
 };
diff --git a/web-ui/src/components/settings/types/string.jsx b/web-ui/src/components/settings/types/string.jsx
@@ -1,5 +1,11 @@
 import React from 'react';
-import { Input, Typography } from '@mui/material';
+import {
+  Select,
+  MenuItem,
+  ListItemText,
+  Input,
+  Typography
+} from '@mui/material';
 import { createLabelId } from '../../../helpers/strings.js';
 
 /**
@@ -17,6 +23,7 @@ import { createLabelId } from '../../../helpers/strings.js';
 const SettingsString = ({
   name,
   description,
+  values,
   value,
   placeholder,
   handleChange
@@ -31,14 +38,26 @@ const SettingsString = ({
         </Typography>
       </label>
       {description && <p>{description}</p>}
+      {values && values.length > 0 ?
+      <Select
+        labelId={labelId}
+        value={value}
+        onChange={handleChange}
+      >
+        {values.map((option) => (
+          <MenuItem key={option} value={option}>
+            <ListItemText primary={option} />
+          </MenuItem>
+        ))}
+      </Select> :
       <Input
         id={labelId}
         className="settings-control"
         type="text"
         value={value}
         placeholder={placeholder ?? `Enter ${name}`}
         onChange={handleChange}
-      />
+      />}
     </div>
   );
 };
diff --git a/web-ui/src/pages/SettingsPage.jsx b/web-ui/src/pages/SettingsPage.jsx
@@ -44,20 +44,15 @@ const SettingsPage = () => {
                             (await getAllOptions()).payload.data : [];
 
       if (allOptions) {
-        // If the option has a valid UUID, then the setting already exists.
-        // This information is necessary to know since we must use POST to
-        // create new settings and PUT to modify existing settings.
-        for (let option of allOptions) {
-          option.exists = option?.id != '00000000-0000-0000-0000-000000000000';
-        }
+        // Sort the options by category, store them, and upate the state.
+        setSettingsControls(
+          allOptions.sort((l, r) => l.category.localeCompare(r.category)));
       }
-
-      // Sort the options by category, store them, and upate the state.
-      setSettingsControls(
-        allOptions.sort((l, r) => l.category.localeCompare(r.category)));
     };
-    fetchData();
-  }, []);
+    if (csrf) {
+      fetchData();
+    }
+  }, [state, csrf]);
 
   // For specific settings, add a handleFunction to the settings object.
   // Format should be handleSetting and then add it to the handlers object
@@ -126,12 +121,12 @@ const SettingsPage = () => {
   const save = async () => {
     let errors;
     let saved = 0;
-    for( let key of Object.keys(handlers)) {
+    for(let key of Object.keys(handlers)) {
       const setting = handlers[key].setting;
       // The settings controller does not allow blank values.
       if (setting && setting.value) {
         let res;
-        if (setting.exists) {
+        if (setting.id) {
           res = await putOption({ name: setting.name,
                                   value: setting.value }, csrf);
         } else {
@@ -183,12 +178,12 @@ const SettingsPage = () => {
 
   /** @type {Controls[]} */
   const updatedSettingsControls = addHandlersToSettings(settingsControls);
+  const categories = {};
 
   return (selectHasViewSettingsPermission(state) ||
           selectHasAdministerSettingsPermission(state)) ? (
     <div className="settings-page">
       {updatedSettingsControls.map((componentInfo, index) => {
-        const categories = {};
         const Component = componentMapping[componentInfo.type.toUpperCase()];
         const info = {...componentInfo, name: titleCase(componentInfo.name)};
         if (categories[info.category]) {
@@ -197,7 +192,8 @@ const SettingsPage = () => {
           categories[info.category] = true;
           return (
             <>
-            <Typography variant="h4"
+            <Typography data-testid={info.category}
+                        variant="h4"
                         sx={{textDecoration: 'underline'}}
                         display="inline">{titleCase(info.category)}</Typography>
             <Component key={index} {...info} />
diff --git a/web-ui/src/pages/SettingsPage.test.jsx b/web-ui/src/pages/SettingsPage.test.jsx
@@ -0,0 +1,77 @@
+import React from 'react';
+import SettingsPage from './SettingsPage';
+import { AppContextProvider } from '../context/AppContext';
+import { http, HttpResponse } from 'msw';
+import { setupServer } from 'msw/node';
+import { BrowserRouter } from 'react-router-dom';
+
+const initialState = {
+  state: {
+    csrf: 'csrf',
+    userProfile: {
+      name: 'Current User',
+      role: ['MEMBER'],
+      permissions: [{ permission: 'CAN_ADMINISTER_SETTINGS' }],
+    },
+    loading: {
+      teams: [],
+    },
+  }
+};
+
+const server = setupServer(
+  http.get('http://localhost:8080/services/settings/options', ({ request }) => {
+    return HttpResponse.json([
+      {
+        'name': 'STRING_SETTING',
+        'description': 'The description',
+        'category': 'THEME',
+        'type': 'STRING',
+        'value': 'The value',
+      },
+      {
+        'name': 'OTHER_SETTING',
+        'description': 'The description',
+        'category': 'THEME',
+        'type': 'NUMBER',
+        'value': '42',
+      },
+      {
+        'name': 'ANOTHER_SETTING',
+        'description': 'The description',
+        'category': 'INTEGRATIONS',
+        'type': 'BOOLEAN',
+        'value': 'false',
+      },
+    ]);
+  }),
+);
+
+beforeAll(() => server.listen());
+afterEach(() => server.resetHandlers());
+afterAll(() => server.close());
+
+describe('SettingsPage', () => {
+  it('renders correctly', async () => {
+    await waitForSnapshot(
+      // There are two settings with the THEME category.  If the page is
+      // rendered correctly, there will only be one THEME category heading.
+      'THEME',
+      <AppContextProvider value={initialState}>
+        <BrowserRouter>
+          <SettingsPage />
+        </BrowserRouter>
+      </AppContextProvider>
+    );
+  });
+
+  it('renders an error if user does not have appropriate permission', () => {
+    snapshot(
+      <AppContextProvider>
+        <BrowserRouter>
+          <SettingsPage />
+        </BrowserRouter>
+      </AppContextProvider>
+    );
+  });
+});
diff --git a/web-ui/src/pages/__snapshots__/SettingsPage.test.jsx.snap b/web-ui/src/pages/__snapshots__/SettingsPage.test.jsx.snap
