No blank backend role before adding a new one in Create User page

[nurSaadat]

Description

Perform a check on the list of backend roles whether it contains an empty value. If it does, display an error message and don't allow the user to create additional backend roles until the existing blank panels are filled with values.

Category

[Bug fix]

Why these changes are required?

Those changes help to make UX more intuitive and also prevent undesired user behaviour.

What is the old behavior before changes and new behavior after changes?

Before the changes, new empty backend roles were added every time the "Add another backend role" button was clicked.
After the change, it is checked whether there are any empty fields before adding a new one.

Issues Resolved

• Resolve: [BUG] Backend roles section of Create User page allows blank backend role #1307

Testing

• First case : the user tries to add a new backend role while the last text field is empty.
  
  

• Second case: the user tries to add a new backend role while any previously created text field is empty
  

Check List

• New functionality includes testing
• New functionality has been documented
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[RyanL1997]

Hi @nurSaadat, great job on fixing this! There are only couple problems we need to address before we merge this fix:

1. Modify/add unit test for this fix, but still it is nice to see you have tested it manually :) I believe the place for adding test case is here.
2. Fix the lint, and you can find out more details here.
3. I also left some comments on the code:

For your information, you can also use the pattern * Resolve: [issue_url] in your "Issues Resolved" section, so that the related issue will be automatically resolved once your fix gets merged.

[RyanL1997]

Should use ===, instead of ==.

[RyanL1997]

Unrelated change, please revert.

[stephen-crawford]

This may be a ./gradlew spotlessApply thing. If so, it is probably fine.

[RyanL1997]

Same with the above, should use !== instead of ==.

[stephen-crawford]

Hi @nurSaadat, great job fixing this! Thank you for your contribution. Can you confirm that you checked that the issue from before is fixed? I know frontend testing can be a pain, so I won't request that you write a whole cypress test, but I just want to check with you that it did seem to fix the issue.

Edit: @peternied has a good point so I rescind my comment. If you could add a test to confirm this is as expected that would be great.

[peternied]

I know frontend testing can be a pain, so I won't request that you write a whole cypress test, but I just want to check with you that it did seem to fix the issue.

@scrawfor99 I disagree, we should always have a tests associated with changes. However, if it is a unit test, snapshot, jest, or cypress should be evaluated by maintainers

[nurSaadat]

@peternied I don't quite get what is wrong with my tests, so they fail.

Given that I have changed the contents of backend-role-panel.tsx, so that EuiFormRow has additional props error, isInvalid, and EuiFieldText has isInvalid

<EuiFormRow
    label={arrayIndex === 0 ? 'Backend role' : ''}
    error={roleEmptyErrorMessage}
    isInvalid={arrayIndex === emptyRoleIndex && !isEmpty(roleEmptyErrorMessage)}
  >
    <EuiFieldText
      isInvalid={arrayIndex === emptyRoleIndex && !isEmpty(roleEmptyErrorMessage)}
      id={`backend-role-${arrayIndex}`}
      value={backendRole}
      onChange={(e) => {
        updateElementInArrayHandler(setBackendRoles, [arrayIndex])(e.target.value);
        setRoleEmptyErrorMessage('');
      }}
      placeholder="Type in backend role"
    />
  </EuiFormRow>
  
...

  <EuiButton
    id="backend-role-add-row"
    onClick={() => {
      if (state.indexOf('') !== -1) {
        setRoleEmptyErrorMessage('Type a backend role before adding a new one');
        setEmptyRoleIndex(state.indexOf(''));
      } else {
        setRoleEmptyErrorMessage('');
        appendElementToArray(setState, [], '');
      }
    }}
  >
    Add another backend role
  </EuiButton>

I don't understand why following tests fail:

1. add backend role when the previous role is blank. Expected behavior: call AppendElementToArray once. But now it calls function appendElementToArray twice.

    it('add backend role when the previous role is blank', () => {
      const component = shallow(<BackendRolePanel state={sampleState} setState={setState} />);
      component.find('#backend-role-add-row').simulate('click');
      expect(appendElementToArray).toBeCalledWith(setState, [], '');
      component.find('#backend-role-add-row').simulate('click');
      expect(appendElementToArray).not.toBeCalledWith(setState, [], '');
    });

2. add backend role when one of the previous roles is blank. Expected behavior: set EuiFormRow prop isInvalid to true and set error message as 'Type a backend role before adding a new one'. Now it says that the error message is an empty string.

    it('add backend role when one of the previous roles is blank', () => {
      const component = shallow(<BackendRolePanel state={sampleState} setState={setState} />);
      component.find('#backend-role-0').simulate('change', { target: { value: '' } });
      component.find('#backend-role-add-row').simulate('click');
      expect(component.find(EuiFormRow).at(0).prop('error')).toBe('Type a backend role before adding a new one');
      expect(component.find(EuiFormRow).at(0).prop('isInvalid')).toBe(true);
    });

My confidence with Jest is still building up. Could you please suggest me what I am doing wrong?

[peternied]

@nurSaadat Rather than a jest-based integration test, try a snapshot test. For this change a jest integration is overkill as they are complex to build and maintain.

There are a number of 'snapshot tests' that verify the rendering of a component's DOM, which I think might be a better fit for this change. There is a component permissions-tree that has a snapshot test that might be a good starting point, links [component], [test], [snapshot].

Please post an update to the pull request with your test(s) even if it does not work, this way others can provide feedback.

[nurSaadat]

Thank you @peternied for giving useful links!

TLDR: my generated snapshots are completely different from what I wanted them to be, and I can't see why that should be a case (currently trying to find a solution)

It looks like created snapshots are useless, since their content doesn't match what I expected. For example: (I am using the same two tests, that I planned before)

1. add backend role when the previous role is blank.

 it('add backend role when the previous role is blank', () => {
      const component = shallow(<BackendRolePanel state={sampleState} setState={setState} />);
      component.find('#backend-role-add-row').simulate('click');
      component.find('#backend-role-add-row').simulate('click');
      expect(component).toMatchSnapshot();
    });

generates only the component with the Add another backend role button:

exports[`User editing - backend role panel BackendRolePanel add backend role when the previous role is blank 1`] = `
<PanelWithHeader
  headerSubText="Backend roles are used to map users from external authentication systems, such as LDAP or SAML to OpenSearch security roles."
  headerText="Backend roles"
  helpLink="https://opensearch.org/docs/latest/security-plugin/access-control/index/"
  optional={true}
>
  <EuiSpacer />
  <EuiButton
    id="backend-role-add-row"
    onClick={[Function]}
  >
    Add another backend role
  </EuiButton>
</PanelWithHeader>
`;

While I expected also child component to be there and in it there should be last row with an error:

exports[`User editing - backend role panel BackendRolePanel add backend role when the previous role is blank 1`] = `
<PanelWithHeader
  headerSubText="Backend roles are used to map users from external authentication systems, such as LDAP or SAML to OpenSearch security roles."
  headerText="Backend roles"
  helpLink="https://opensearch.org/docs/latest/security-plugin/access-control/index/"
  optional={true}
>
  <EuiFlexGroup>
    <EuiFlexItem
      className="limit-width-input"
    >
      <EuiFormRow
        describedByIds={Array []}
        display="row"
        error=""
        fullWidth={false}
        hasChildLabel={true}
        hasEmptyLabelSpace={false}
        isInvalid={false}
        label="Backend role"
        labelType="label"
      >
        <EuiFieldText
          id="backend-role-0"
          isInvalid={false}
          onChange={[Function]}
          placeholder="Type in backend role"
          value="admin"
        />
      </EuiFormRow>
    </EuiFlexItem>
    <EuiFlexItem
      grow={false}
    >
      <EuiFormRow
        describedByIds={Array []}
        display="row"
        fullWidth={false}
        hasChildLabel={true}
        hasEmptyLabelSpace={true}
        labelType="label"
      >
        <EuiButton
          color="danger"
          id="backend-role-delete-0"
          onClick={[Function]}
        >
          Remove
        </EuiButton>
      </EuiFormRow>
    </EuiFlexItem>
  </EuiFlexGroup>
  <EuiFlexGroup>
    <EuiFlexItem
      className="limit-width-input"
    >
      <EuiFormRow
        describedByIds={Array []}
        display="row"
        error=""
        fullWidth={false}
        hasChildLabel={true}
        hasEmptyLabelSpace={false}
        isInvalid={false}
        label=""
        labelType="label"
      >
        <EuiFieldText
          id="backend-role-1"
          isInvalid={false}
          onChange={[Function]}
          placeholder="Type in backend role"
          value="HR"
        />
      </EuiFormRow>
    </EuiFlexItem>
    <EuiFlexItem
      grow={false}
    >
      <EuiFormRow
        describedByIds={Array []}
        display="row"
        fullWidth={false}
        hasChildLabel={true}
        hasEmptyLabelSpace={false}
        labelType="label"
      >
        <EuiButton
          color="danger"
          id="backend-role-delete-1"
          onClick={[Function]}
        >
          Remove
        </EuiButton>
      </EuiFormRow>
    </EuiFlexItem>
  </EuiFlexGroup>
  <EuiFlexGroup>
    <EuiFlexItem
      className="limit-width-input"
    >
      <EuiFormRow
        describedByIds={Array []}
        display="row"
        error="Type a backend role before adding a new one"
        fullWidth={false}
        hasChildLabel={true}
        hasEmptyLabelSpace={false}
        isInvalid={true}
        label=""
        labelType="label"
      >
        <EuiFieldText
          id="backend-role-1"
          isInvalid={true}
          onChange={[Function]}
          placeholder="Type in backend role"
          value=""
        />
      </EuiFormRow>
    </EuiFlexItem>
    <EuiFlexItem
      grow={false}
    >
      <EuiFormRow
        describedByIds={Array []}
        display="row"
        fullWidth={false}
        hasChildLabel={true}
        hasEmptyLabelSpace={false}
        labelType="label"
      >
        <EuiButton
          color="danger"
          id="backend-role-delete-1"
          onClick={[Function]}
        >
          Remove
        </EuiButton>
      </EuiFormRow>
    </EuiFlexItem>
  </EuiFlexGroup>
  <EuiSpacer />
  <EuiButton
    id="backend-role-add-row"
    onClick={[Function]}
  >
    Add another backend role
  </EuiButton>
</PanelWithHeader>
`;

2. add backend role when one of the previous roles is blank.

    it('add backend role when one of the previous roles is blank', () => {
      const component = shallow(<BackendRolePanel state={sampleState} setState={setState} />);
      component.find('#backend-role-0').simulate('change', { target: { value: '' } });
      component.find('#backend-role-add-row').simulate('click');
      expect(component).toMatchSnapshot();
    });

generates just a normal view with two roles and no changes:

exports[`User editing - backend role panel BackendRolePanel add backend role when one of the previous roles is blank 1`] = `
<PanelWithHeader
  headerSubText="Backend roles are used to map users from external authentication systems, such as LDAP or SAML to OpenSearch security roles."
  headerText="Backend roles"
  helpLink="https://opensearch.org/docs/latest/security-plugin/access-control/index/"
  optional={true}
>
  <EuiFlexGroup>
    <EuiFlexItem
      className="limit-width-input"
    >
      <EuiFormRow
        describedByIds={Array []}
        display="row"
        error=""
        fullWidth={false}
        hasChildLabel={true}
        hasEmptyLabelSpace={false}
        isInvalid={false}
        label="Backend role"
        labelType="label"
      >
        <EuiFieldText
          id="backend-role-0"
          isInvalid={false}
          onChange={[Function]}
          placeholder="Type in backend role"
          value="admin"
        />
      </EuiFormRow>
    </EuiFlexItem>
    <EuiFlexItem
      grow={false}
    >
      <EuiFormRow
        describedByIds={Array []}
        display="row"
        fullWidth={false}
        hasChildLabel={true}
        hasEmptyLabelSpace={true}
        labelType="label"
      >
        <EuiButton
          color="danger"
          id="backend-role-delete-0"
          onClick={[Function]}
        >
          Remove
        </EuiButton>
      </EuiFormRow>
    </EuiFlexItem>
  </EuiFlexGroup>
  <EuiFlexGroup>
    <EuiFlexItem
      className="limit-width-input"
    >
      <EuiFormRow
        describedByIds={Array []}
        display="row"
        error=""
        fullWidth={false}
        hasChildLabel={true}
        hasEmptyLabelSpace={false}
        isInvalid={false}
        label=""
        labelType="label"
      >
        <EuiFieldText
          id="backend-role-1"
          isInvalid={false}
          onChange={[Function]}
          placeholder="Type in backend role"
          value="HR"
        />
      </EuiFormRow>
    </EuiFlexItem>
    <EuiFlexItem
      grow={false}
    >
      <EuiFormRow
        describedByIds={Array []}
        display="row"
        fullWidth={false}
        hasChildLabel={true}
        hasEmptyLabelSpace={false}
        labelType="label"
      >
        <EuiButton
          color="danger"
          id="backend-role-delete-1"
          onClick={[Function]}
        >
          Remove
        </EuiButton>
      </EuiFormRow>
    </EuiFlexItem>
  </EuiFlexGroup>
  <EuiSpacer />
  <EuiButton
    id="backend-role-add-row"
    onClick={[Function]}
  >
    Add another backend role
  </EuiButton>
</PanelWithHeader>
`;

While I expected the first row to be empty and it to contain error message.

exports[`User editing - backend role panel BackendRolePanel add backend role when one of the previous roles is blank 1`] = `
<PanelWithHeader
  headerSubText="Backend roles are used to map users from external authentication systems, such as LDAP or SAML to OpenSearch security roles."
  headerText="Backend roles"
  helpLink="https://opensearch.org/docs/latest/security-plugin/access-control/index/"
  optional={true}
>
  <EuiFlexGroup>
    <EuiFlexItem
      className="limit-width-input"
    >
      <EuiFormRow
        describedByIds={Array []}
        display="row"
        error="Type a backend role before adding a new one"
        fullWidth={false}
        hasChildLabel={true}
        hasEmptyLabelSpace={false}
        isInvalid={true}
        label="Backend role"
        labelType="label"
      >
        <EuiFieldText
          id="backend-role-0"
          isInvalid={true}
          onChange={[Function]}
          placeholder="Type in backend role"
          value=""
        />
      </EuiFormRow>
    </EuiFlexItem>
    <EuiFlexItem
      grow={false}
    >
      <EuiFormRow
        describedByIds={Array []}
        display="row"
        fullWidth={false}
        hasChildLabel={true}
        hasEmptyLabelSpace={true}
        labelType="label"
      >
        <EuiButton
          color="danger"
          id="backend-role-delete-0"
          onClick={[Function]}
        >
          Remove
        </EuiButton>
      </EuiFormRow>
    </EuiFlexItem>
  </EuiFlexGroup>
  <EuiFlexGroup>
    <EuiFlexItem
      className="limit-width-input"
    >
      <EuiFormRow
        describedByIds={Array []}
        display="row"
        error=""
        fullWidth={false}
        hasChildLabel={true}
        hasEmptyLabelSpace={false}
        isInvalid={false}
        label=""
        labelType="label"
      >
        <EuiFieldText
          id="backend-role-1"
          isInvalid={false}
          onChange={[Function]}
          placeholder="Type in backend role"
          value="HR"
        />
      </EuiFormRow>
    </EuiFlexItem>
    <EuiFlexItem
      grow={false}
    >
      <EuiFormRow
        describedByIds={Array []}
        display="row"
        fullWidth={false}
        hasChildLabel={true}
        hasEmptyLabelSpace={false}
        labelType="label"
      >
        <EuiButton
          color="danger"
          id="backend-role-delete-1"
          onClick={[Function]}
        >
          Remove
        </EuiButton>
      </EuiFormRow>
    </EuiFlexItem>
  </EuiFlexGroup>
  <EuiSpacer />
  <EuiButton
    id="backend-role-add-row"
    onClick={[Function]}
  >
    Add another backend role
  </EuiButton>
</PanelWithHeader>
`;

[peternied]

@nurSaadat I am not going to be able to follow the code in your comment and provide you useful info feedback. Please add files to the pull request and annotation them with comments where you include the text of error output or unexpected behavior.

Pull requests can be messy while they are coming together, you can always iterate on them or include/remove files - thanks for digging in on this!

[nurSaadat]

@peternied thank you for your feedback!

I have included snapshots with my latest commit.

[peternied]

@nurSaadat Thanks for adding the tests, looks like the linters are failing that is preventing the tests from running.

Other than the linting issues, the new tests look good to me

[DarshitChanpura]

LGTM! Thank you @nurSaadat. Once the linter errors are fixed, this is good to go!

[cwperks]

Thank you for the contribution @nurSaadat ! This change looks good to me, I will approve this change once the CI is passing.

You can use node ../../scripts/eslint to automatically format the changes to satisfy the linter. Run this command from the the security-dashboards-plugin directory.

[cwperks]

🙌 - thank you for adding the input validation here :)