Fix problems with key value input and empty entries (#19421)

js/apps/admin-ui/cypress/e2e/clients_test.spec.ts

@@ -542,7 +542,8 @@ describe("Clients test", () => {
     it("Should delete attribute from client role", () => {
       commonPage.tableUtils().clickRowItemLink(itemId);
       rolesTab.goToAttributesTab();
-      attributesTab.deleteAttribute(1);
+      attributesTab.deleteAttribute(0);
+      attributesTab.assertEmpty();
       commonPage
         .masthead()
         .checkNotificationMessage("The role has been saved", true);

js/apps/admin-ui/cypress/e2e/group_test.spec.ts

@@ -420,7 +420,8 @@ describe("Group test", () => {
     });
 
     it("Remove attribute", () => {
-      attributesTab.deleteAttribute(1).assertRowItemsEqualTo(1);
+      attributesTab.deleteAttribute(0);
+      attributesTab.assertEmpty();
       groupPage.assertNotificationGroupUpdated();
     });
 
@@ -429,7 +430,7 @@ describe("Group test", () => {
         .addAttribute("key", "value")
         .addAnAttributeButton()
         .revert()
-        .assertRowItemsEqualTo(1);
+        .assertEmpty();
     });
   });
 

js/apps/admin-ui/cypress/e2e/realm_roles_test.spec.ts

@@ -248,10 +248,10 @@ describe("Realm roles test", () => {
       listingPage.itemExist(editRoleName).goToItemDetails(editRoleName);
 
       createRealmRolePage.goToAttributesTab();
-      keyValue.fillKeyValue({ key: "one", value: "1" }).validateRows(2);
+      keyValue.fillKeyValue({ key: "one", value: "1" }).validateRows(1);
       keyValue.save();
       masthead.checkNotificationMessage("The role has been saved", true);
-      keyValue.validateRows(2);
+      keyValue.validateRows(1);
     });
 
     it("should add attribute multiple", () => {
@@ -259,17 +259,17 @@ describe("Realm roles test", () => {
 
       createRealmRolePage.goToAttributesTab();
       keyValue
-        .fillKeyValue({ key: "two", value: "2" }, 1)
-        .fillKeyValue({ key: "three", value: "3" }, 2)
+        .fillKeyValue({ key: "two", value: "2" })
+        .fillKeyValue({ key: "three", value: "3" })
         .save()
-        .validateRows(4);
+        .validateRows(3);
     });
 
     it("should delete attribute", () => {
       listingPage.itemExist(editRoleName).goToItemDetails(editRoleName);
       createRealmRolePage.goToAttributesTab();
 
-      keyValue.deleteRow(1).save().validateRows(3);
+      keyValue.deleteRow(1).save().validateRows(2);
     });
   });
 

js/apps/admin-ui/cypress/support/pages/admin-ui/manage/AttributesTab.ts

@@ -2,9 +2,10 @@ export default class AttributesTab {
   private saveAttributeBtn = "save-attributes";
   private addAttributeBtn = "attributes-add-row";
   private attributesTab = "attributes";
-  private attributeRow = "row";
-  private keyInput = (index: number) => `attributes[${index}].key`;
-  private valueInput = (index: number) => `attributes[${index}].value`;
+  private keyInput = "attributes-key";
+  private valueInput = "attributes-value";
+  private removeBtn = "attributes-remove";
+  private emptyState = "attributes-empty-state";
 
   public goToAttributesTab() {
     cy.findByTestId(this.attributesTab).click();
@@ -13,14 +14,15 @@ export default class AttributesTab {
   }
 
   public addAttribute(key: string, value: string) {
-    cy.findAllByTestId(this.attributeRow)
+    cy.findByTestId(this.addAttributeBtn).click();
+
+    cy.findAllByTestId(this.keyInput)
       .its("length")
-      .then((index) => {
-        cy.findByTestId(this.keyInput(index - 1)).type(key, { force: true });
-        cy.findByTestId(this.valueInput(index - 1)).type(value, {
-          force: true,
-        });
+      .then((length) => {
+        this.keyInputAt(length - 1).type(key, { force: true });
+        this.valueInputAt(length - 1).type(value, { force: true });
       });
+
     return this;
   }
 
@@ -35,7 +37,7 @@ export default class AttributesTab {
   }
 
   public deleteAttributeButton(row: number) {
-    cy.findByTestId(`attributes[${row - 1}].remove`).click({ force: true });
+    this.removeButtonAt(row).click({ force: true });
     return this;
   }
 
@@ -48,19 +50,27 @@ export default class AttributesTab {
     this.deleteAttributeButton(rowIndex);
     this.save();
 
-    cy.findAllByTestId(`attributes[${rowIndex - 1}].key`).should(
-      "have.value",
-      ""
-    );
-    cy.findAllByTestId(`attributes[${rowIndex - 1}].value`).should(
-      "have.value",
-      ""
-    );
     return this;
   }
 
+  public assertEmpty() {
+    cy.findByTestId(this.emptyState).should("exist");
+  }
+
   public assertRowItemsEqualTo(amount: number) {
-    cy.findAllByTestId("row").its("length").should("be.eq", amount);
+    cy.findAllByTestId(this.keyInput).its("length").should("be.eq", amount);
     return this;
   }
+
+  private keyInputAt(index: number) {
+    return cy.findAllByTestId(this.keyInput).eq(index);
+  }
+
+  private valueInputAt(index: number) {
+    return cy.findAllByTestId(this.valueInput).eq(index);
+  }
+
+  private removeButtonAt(index: number) {
+    return cy.findAllByTestId(this.removeBtn).eq(index);
+  }
 }

js/apps/admin-ui/cypress/support/pages/admin-ui/manage/KeyValueInput.ts

@@ -7,22 +7,26 @@ export default class KeyValueInput {
     this.name = name;
   }
 
-  fillKeyValue({ key, value }: KeyValueType, index = 0) {
-    cy.findByTestId(`${this.name}[${index}].key`).clear();
-    cy.findByTestId(`${this.name}[${index}].key`).type(key);
-    cy.findByTestId(`${this.name}[${index}].value`).clear();
-    cy.findByTestId(`${this.name}[${index}].value`).type(value);
+  fillKeyValue({ key, value }: KeyValueType) {
     cy.findByTestId(`${this.name}-add-row`).click();
+
+    cy.findAllByTestId(`${this.name}-key`)
+      .its("length")
+      .then((length) => {
+        this.keyInputAt(length - 1).type(key);
+        this.valueInputAt(length - 1).type(value);
+      });
+
     return this;
   }
 
   deleteRow(index: number) {
-    cy.findByTestId(`${this.name}[${index}].remove`).click();
+    cy.findAllByTestId(`${this.name}-remove`).eq(index).click();
     return this;
   }
 
   validateRows(numberOfRows: number) {
-    cy.findAllByTestId("row").should("have.length", numberOfRows);
+    cy.findAllByTestId(`${this.name}-key`).should("have.length", numberOfRows);
     return this;
   }
 
@@ -30,4 +34,12 @@ export default class KeyValueInput {
     cy.findByTestId("save-attributes").click();
     return this;
   }
+
+  keyInputAt(index: number) {
+    return cy.findAllByTestId(`${this.name}-key`).eq(index);
+  }
+
+  valueInputAt(index: number) {
+    return cy.findAllByTestId(`${this.name}-value`).eq(index);
+  }
 }

js/apps/admin-ui/cypress/support/pages/admin-ui/manage/LegacyKeyValueInput.ts

@@ -0,0 +1,33 @@
+import type { KeyValueType } from "../../../../../src/components/key-value-form/key-value-convert";
+
+export default class LegacyKeyValueInput {
+  private name: string;
+
+  constructor(name: string) {
+    this.name = name;
+  }
+
+  fillKeyValue({ key, value }: KeyValueType, index = 0) {
+    cy.findByTestId(`${this.name}.${index}.key`).clear();
+    cy.findByTestId(`${this.name}.${index}.key`).type(key);
+    cy.findByTestId(`${this.name}.${index}.value`).clear();
+    cy.findByTestId(`${this.name}.${index}.value`).type(value);
+    cy.findByTestId(`${this.name}-add-row`).click();
+    return this;
+  }
+
+  deleteRow(index: number) {
+    cy.findByTestId(`${this.name}.${index}.remove`).click();
+    return this;
+  }
+
+  validateRows(numberOfRows: number) {
+    cy.findAllByTestId("row").should("have.length", numberOfRows);
+    return this;
+  }
+
+  save() {
+    cy.findByTestId("save-attributes").click();
+    return this;
+  }
+}

js/apps/admin-ui/cypress/support/pages/admin-ui/manage/identity_providers/AddMapperPage.ts

@@ -1,4 +1,4 @@
-import KeyValueInput from "../KeyValueInput";
+import LegacyKeyValueInput from "../LegacyKeyValueInput";
 
 export default class AddMapperPage {
   private mappersTab = "mappers-tab";
@@ -23,8 +23,8 @@ export default class AddMapperPage {
   private mappersUrl = "/oidc/mappers";
   private regexAttributeValuesSwitch = "are.attribute.values.regex";
   private syncmodeSelectToggle = "#syncMode";
-  private attributesKeyInput = '[data-testid="config.attributes[0].key"]';
-  private attributesValueInput = '[data-testid="config.attributes[0].value"]';
+  private attributesKeyInput = '[data-testid="config.attributes.0.key"]';
+  private attributesValueInput = '[data-testid="config.attributes.0.value"]';
   private template = "template";
   private target = "#target";
 
@@ -392,7 +392,7 @@ export default class AddMapperPage {
 
     cy.findByTestId(this.idpMapperSelect).contains("Claim to Role").click();
 
-    const keyValue = new KeyValueInput("config.claims");
+    const keyValue = new LegacyKeyValueInput("config.claims");
 
     keyValue.fillKeyValue({ key: "key", value: "value" });
 

js/apps/admin-ui/cypress/support/pages/admin-ui/manage/realm_settings/UserProfile.ts

@@ -17,8 +17,9 @@ export default class UserProfile {
   private newAttributeRequiredFor = 'input[name="roles"]';
   private newAttributeRequiredWhen = 'input[name="requiredWhen"]';
   private newAttributeEmptyValidators = ".kc-emptyValidators";
-  private newAttributeAnnotationKey = 'input[name="annotations[0].key"]';
-  private newAttributeAnnotationValue = 'input[name="annotations[0].value"]';
+  private newAttributeAnnotationBtn = "annotations-add-row";
+  private newAttributeAnnotationKey = "annotations-key";
+  private newAttributeAnnotationValue = "annotations-value";
   private validatorRolesList = "#validator";
   private validatorsList = 'tbody [data-label="name"]';
   private saveNewAttributeBtn = "attribute-create";
@@ -106,8 +107,9 @@ export default class UserProfile {
     cy.get(this.newAttributeRequiredFor).first().check({ force: true });
     cy.get(this.newAttributeRequiredWhen).first().check();
     cy.get(this.newAttributeEmptyValidators).contains("No validators.");
-    cy.get(this.newAttributeAnnotationKey).type("test");
-    cy.get(this.newAttributeAnnotationValue).type("123");
+    cy.findByTestId(this.newAttributeAnnotationBtn).click();
+    cy.findByTestId(this.newAttributeAnnotationKey).type("test");
+    cy.findByTestId(this.newAttributeAnnotationValue).type("123");
     return this;
   }
 

js/apps/admin-ui/public/resources/en/common.json

@@ -172,10 +172,13 @@
     "years": "Years"
   },
   "attributes": "Attributes",
+  "missingAttributes": "No attributes have been defined yet. Click the below button to add attributes, key and value are required for a key pair.",
   "addAttribute": "Add an attribute",
   "removeAttribute": "Remove attribute",
   "keyPlaceholder": "Type a key",
   "valuePlaceholder": "Type a value",
+  "keyError": "A key must be provided.",
+  "valueError": "A value must be provided.",
   "credentials": "Credentials",
   "clientId": "Client ID",
   "clientName": "Name",

js/apps/admin-ui/src/clients/authorization/KeyBasedAttributeInput.tsx

@@ -103,14 +103,14 @@ const ValueInput = ({
     <Td>
       {resources || attributeValues?.length ? (
         <Controller
-          name={`${name}[${rowIndex}].value`}
+          name={`${name}.${rowIndex}.value`}
           defaultValue={[]}
           control={control}
           render={({ field }) => (
             <Select
               toggleId={`${attribute.id}-value`}
               className="kc-attribute-value-selectable"
-              name={`${name}[${rowIndex}].value`}
+              name={`${name}.${rowIndex}.value`}
               chipGroupProps={{
                 numChips: 1,
                 expandedText: t("common:hide"),
@@ -171,7 +171,7 @@ export const KeyBasedAttributeInput = ({
     }
   }, [fields]);
 
-  const watchLastValue = watch(`${name}[${fields.length - 1}].value`, "");
+  const watchLastValue = watch(`${name}.${fields.length - 1}.value`, "");
 
   return (
     <TableComposable
@@ -190,14 +190,14 @@ export const KeyBasedAttributeInput = ({
           <Tr key={attribute.id} data-testid="attribute-row">
             <Td>
               <Controller
-                name={`${name}[${rowIndex}].key`}
+                name={`${name}.${rowIndex}.key`}
                 defaultValue=""
                 control={control}
                 render={({ field }) => (
                   <Select
-                    toggleId={`${name}[${rowIndex}].key`}
+                    toggleId={`${name}.${rowIndex}.key`}
                     className="kc-attribute-key-selectable"
-                    name={`${name}[${rowIndex}].key`}
+                    name={`${name}.${rowIndex}.key`}
                     onToggle={(open) => toggleKeySelect(rowIndex, open)}
                     isOpen={isKeyOpenArray[rowIndex]}
                     variant={SelectVariant.typeahead}

js/apps/admin-ui/src/components/dynamic/MapComponent.tsx

@@ -84,11 +84,11 @@ export const MapComponent = ({ name, label, helpText }: ComponentProps) => {
           <Flex key={attribute.id} data-testid="row">
             <FlexItem grow={{ default: "grow" }}>
               <TextInput
-                name={`${fieldName}[${index}].key`}
+                name={`${fieldName}.${index}.key`}
                 placeholder={t("common:keyPlaceholder")}
                 aria-label={t("key")}
                 defaultValue={attribute.key}
-                data-testid={`${fieldName}[${index}].key`}
+                data-testid={`${fieldName}.${index}.key`}
                 onChange={(value) => updateKey(index, value)}
                 onBlur={() => update()}
               />
@@ -98,11 +98,11 @@ export const MapComponent = ({ name, label, helpText }: ComponentProps) => {
               spacer={{ default: "spacerNone" }}
             >
               <TextInput
-                name={`${fieldName}[${index}].value`}
+                name={`${fieldName}.${index}.value`}
                 placeholder={t("common:valuePlaceholder")}
                 aria-label={t("common:value")}
                 defaultValue={attribute.value}
-                data-testid={`${fieldName}[${index}].value`}
+                data-testid={`${fieldName}.${index}.value`}
                 onChange={(value) => updateValue(index, value)}
                 onBlur={() => update()}
               />
@@ -113,7 +113,7 @@ export const MapComponent = ({ name, label, helpText }: ComponentProps) => {
                 title={t("common:removeAttribute")}
                 isDisabled={map.length === 1}
                 onClick={() => remove(index)}
-                data-testid={`${fieldName}[${index}].remove`}
+                data-testid={`${fieldName}.${index}.remove`}
               >
                 <MinusCircleIcon />
               </Button>

js/apps/admin-ui/src/components/key-value-form/KeyValueInput.tsx

@@ -2,16 +2,20 @@ import {
   ActionList,
   ActionListItem,
   Button,
-  Flex,
-  FlexItem,
+  EmptyState,
+  EmptyStateBody,
+  Grid,
+  GridItem,
+  HelperText,
+  HelperTextItem,
+  InputGroup,
 } from "@patternfly/react-core";
 import { MinusCircleIcon, PlusCircleIcon } from "@patternfly/react-icons";
-import { useEffect } from "react";
-import { useFieldArray, useFormContext, useWatch } from "react-hook-form";
+import { Fragment } from "react";
+import { useFieldArray, useFormContext } from "react-hook-form";
 import { useTranslation } from "react-i18next";
 
 import { KeycloakTextInput } from "../keycloak-text-input/KeycloakTextInput";
-import { KeyValueType } from "./key-value-convert";
 
 type KeyValueInputProps = {
   name: string;
@@ -19,83 +23,82 @@ type KeyValueInputProps = {
 
 export const KeyValueInput = ({ name }: KeyValueInputProps) => {
   const { t } = useTranslation("common");
-  const { control, register } = useFormContext();
+  const {
+    control,
+    register,
+    formState: { errors },
+  } = useFormContext();
+
   const { fields, append, remove } = useFieldArray({
     control,
     name,
   });
 
-  const watchFields = useWatch({
-    control,
-    name,
-    defaultValue: [{ key: "", value: "" }],
-  });
+  const appendNew = () => append({ key: "", value: "" });
 
-  const isValid =
-    Array.isArray(watchFields) &&
-    watchFields.every(
-      ({ key, value }: KeyValueType) =>
-        // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
-        key?.trim().length !== 0 && value?.trim().length !== 0
-    );
-
-  useEffect(() => {
-    if (!fields.length) {
-      append({ key: "", value: "" }, { shouldFocus: false });
-    }
-  }, [fields]);
-
-  return (
+  return fields.length > 0 ? (
     <>
-      <Flex direction={{ default: "column" }}>
-        <Flex>
-          <FlexItem
-            grow={{ default: "grow" }}
-            spacer={{ default: "spacerNone" }}
-          >
-            <strong>{t("key")}</strong>
-          </FlexItem>
-          <FlexItem grow={{ default: "grow" }}>
-            <strong>{t("value")}</strong>
-          </FlexItem>
-        </Flex>
-        {fields.map((attribute, index) => (
-          <Flex key={attribute.id} data-testid="row">
-            <FlexItem grow={{ default: "grow" }}>
-              <KeycloakTextInput
-                placeholder={t("keyPlaceholder")}
-                aria-label={t("key")}
-                defaultValue=""
-                data-testid={`${name}[${index}].key`}
-                {...register(`${name}[${index}].key`)}
-              />
-            </FlexItem>
-            <FlexItem
-              grow={{ default: "grow" }}
-              spacer={{ default: "spacerNone" }}
-            >
-              <KeycloakTextInput
-                placeholder={t("valuePlaceholder")}
-                aria-label={t("value")}
-                defaultValue=""
-                data-testid={`${name}[${index}].value`}
-                {...register(`${name}[${index}].value`)}
-              />
-            </FlexItem>
-            <FlexItem>
-              <Button
-                variant="link"
-                title={t("removeAttribute")}
-                isDisabled={watchFields.length === 1}
-                onClick={() => remove(index)}
-                data-testid={`${name}[${index}].remove`}
-              >
-                <MinusCircleIcon />
-              </Button>
-            </FlexItem>
-          </Flex>
-        ))}
-      </Flex>
+      <Grid hasGutter>
+        <GridItem className="pf-c-form__label" span={6}>
+          <span className="pf-c-form__label-text">{t("key")}</span>
+        </GridItem>
+        <GridItem className="pf-c-form__label" span={6}>
+          <span className="pf-c-form__label-text">{t("value")}</span>
+        </GridItem>
+        {fields.map((attribute, index) => {
+          const keyError = !!(errors as any)[name]?.[index]?.key;
+          const valueError = !!(errors as any)[name]?.[index]?.value;
+
+          return (
+            <Fragment key={attribute.id}>
+              <GridItem span={6}>
+                <KeycloakTextInput
+                  placeholder={t("keyPlaceholder")}
+                  aria-label={t("key")}
+                  data-testid={`${name}-key`}
+                  {...register(`${name}.${index}.key`, { required: true })}
+                  validated={keyError ? "error" : "default"}
+                  isRequired
+                />
+                {keyError && (
+                  <HelperText>
+                    <HelperTextItem variant="error">
+                      {t("keyError")}
+                    </HelperTextItem>
+                  </HelperText>
+                )}
+              </GridItem>
+              <GridItem span={6}>
+                <InputGroup>
+                  <KeycloakTextInput
+                    placeholder={t("valuePlaceholder")}
+                    aria-label={t("value")}
+                    data-testid={`${name}-value`}
+                    {...register(`${name}.${index}.value`, { required: true })}
+                    validated={valueError ? "error" : "default"}
+                    isRequired
+                  />
+                  <Button
+                    variant="link"
+                    title={t("removeAttribute")}
+                    onClick={() => remove(index)}
+                    data-testid={`${name}-remove`}
+                  >
+                    <MinusCircleIcon />
+                  </Button>
+                </InputGroup>
+                {valueError && (
+                  <HelperText>
+                    <HelperTextItem variant="error">
+                      {t("valueError")}
+                    </HelperTextItem>
+                  </HelperText>
+                )}
+              </GridItem>
+            </Fragment>
+          );
+        })}
+      </Grid>
       <ActionList>
         <ActionListItem>
           <Button
@@ -103,13 +106,29 @@ export const KeyValueInput = ({ name }: KeyValueInputProps) => {
             className="pf-u-px-0 pf-u-mt-sm"
             variant="link"
             icon={<PlusCircleIcon />}
-            isDisabled={!isValid}
-            onClick={() => append({ key: "", value: "" })}
+            onClick={appendNew}
           >
             {t("addAttribute")}
           </Button>
         </ActionListItem>
       </ActionList>
     </>
+  ) : (
+    <EmptyState
+      data-testid={`${name}-empty-state`}
+      className="pf-u-p-0"
+      variant="xs"
+    >
+      <EmptyStateBody>{t("missingAttributes")}</EmptyStateBody>
+      <Button
+        data-testid={`${name}-add-row`}
+        variant="link"
+        icon={<PlusCircleIcon />}
+        isSmall
+        onClick={appendNew}
+      >
+        {t("addAttribute")}
+      </Button>
+    </EmptyState>
   );
 };

js/apps/admin-ui/src/components/key-value-form/key-value-convert.test.ts

@@ -41,18 +41,6 @@ describe("Tests the convert functions for attribute input", () => {
     expect(result).toEqual({ theKey: ["theValue"] });
   });
 
-  it("convert empty object to attributes", () => {
-    const given: {
-      [key: string]: string[];
-    } = {};
-
-    //when
-    const result = arrayToKeyValue(given);
-
-    //then
-    expect(result).toEqual([{ key: "", value: "" }]);
-  });
-
   it("convert object to attributes", () => {
     const given = { one: ["1"], two: ["2"] };
 
@@ -63,7 +51,6 @@ describe("Tests the convert functions for attribute input", () => {
     expect(result).toEqual([
       { key: "one", value: "1" },
       { key: "two", value: "2" },
-      { key: "", value: "" },
     ]);
   });
 

js/apps/admin-ui/src/components/key-value-form/key-value-convert.ts

@@ -22,5 +22,5 @@ export function arrayToKeyValue<T>(attributes: Record<string, string[]> = {}) {
     value.map<KeyValueType>((value) => ({ key, value }))
   );
 
-  return result.concat({ key: "", value: "" }) as PathValue<T, Path<T>>;
+  return result as PathValue<T, Path<T>>;
 }

js/apps/admin-ui/src/components/multi-line-input/MultiLineInput.tsx

@@ -85,7 +85,7 @@ export const MultiLineInput = ({
             <TextInput
               data-testid={name + index}
               onChange={(value) => updateValue(index, value)}
-              name={`${name}[${index}].value`}
+              name={`${name}.${index}.value`}
               value={value}
               isDisabled={isDisabled}
               {...rest}

js/apps/admin-ui/src/util.test.ts

@@ -26,10 +26,7 @@ describe("Tests the form convert util functions", () => {
     expect(values).toEqual({
       name: "client",
       other: { one: "1", two: "2" },
-      attributes: [
-        { key: "one", value: "1" },
-        { key: "", value: "" },
-      ],
+      attributes: [{ key: "one", value: "1" }],
     });
   });