Only update selected instance groups if the select button is clicked in the modal

__tests__/components/Lookup.test.jsx

@@ -3,34 +3,41 @@ import { mount } from 'enzyme';
 import { I18nProvider } from '@lingui/react';
 import Lookup from '../../src/components/Lookup';
 
-let mockData = [{ name: 'foo', id: 0, isChecked: false }];
+let mockData = [{ name: 'foo', id: 1, isChecked: false }];
 describe('<Lookup />', () => {
   test('initially renders succesfully', () => {
     mount(
       <Lookup
         lookup_header="Foo Bar"
-        lookupChange={() => { }}
+        onLookupSave={() => { }}
         data={mockData}
         selected={[]}
       />
     );
   });
-  test('calls "onLookup" when search icon is clicked', () => {
+  test('Opens modal when search icon is clicked', () => {
-    const spy = jest.spyOn(Lookup.prototype, 'onLookup');
+    const spy = jest.spyOn(Lookup.prototype, 'handleModalToggle');
+    const mockSelected = [{ name: 'foo', id: 1 }];
     const wrapper = mount(
       <I18nProvider>
         <Lookup
           lookup_header="Foo Bar"
-          lookupChange={() => { }}
+          onLookupSave={() => { }}
           data={mockData}
-          selected={[]}
+          selected={mockSelected}
         />
       </I18nProvider>
-    );
+    ).find('Lookup');
     expect(spy).not.toHaveBeenCalled();
+    expect(wrapper.state('lookupSelectedItems')).toEqual([]);
     const searchItem = wrapper.find('.pf-c-input-group__text#search');
     searchItem.first().simulate('click');
     expect(spy).toHaveBeenCalled();
+    expect(wrapper.state('lookupSelectedItems')).toEqual([{
+      id: 1,
+      name: 'foo'
+    }]);
+    expect(wrapper.state('isModalOpen')).toEqual(true);
   });
   test('calls "toggleSelected" when a user changes a checkbox', () => {
     const spy = jest.spyOn(Lookup.prototype, 'toggleSelected');
@@ -38,7 +45,7 @@ describe('<Lookup />', () => {
       <I18nProvider>
         <Lookup
           lookup_header="Foo Bar"
-          lookupChange={() => { }}
+          onLookupSave={() => { }}
           data={mockData}
           selected={[]}
         />
@@ -51,14 +58,15 @@ describe('<Lookup />', () => {
   });
   test('calls "toggleSelected" when remove icon is clicked', () => {
     const spy = jest.spyOn(Lookup.prototype, 'toggleSelected');
-    mockData = [{ name: 'foo', id: 0, isChecked: false }, { name: 'bar', id: 1, isChecked: true }];
+    mockData = [{ name: 'foo', id: 1, isChecked: false }, { name: 'bar', id: 2, isChecked: true }];
+    const mockSelected = [{ name: 'foo', id: 1 }, { name: 'bar', id: 2 }];
     const wrapper = mount(
       <I18nProvider>
         <Lookup
           lookup_header="Foo Bar"
-          lookupChange={() => { }}
+          onLookupSave={() => { }}
           data={mockData}
-          selected={[]}
+          selected={mockSelected}
         />
       </I18nProvider>
     );
@@ -73,7 +81,7 @@ describe('<Lookup />', () => {
       <I18nProvider>
         <Lookup
           lookup_header="Foo Bar"
-          lookupChange={() => { }}
+          onLookupSave={() => { }}
           data={mockData}
           selected={[]}
         />
@@ -83,4 +91,57 @@ describe('<Lookup />', () => {
     const pill = wrapper.find('span.awx-c-tag--pill');
     expect(pill).toHaveLength(0);
   });
+  test('toggleSelected successfully adds/removes row from lookupSelectedItems state', () => {
+    mockData = [{ name: 'foo', id: 1 }];
+    const wrapper = mount(
+      <I18nProvider>
+        <Lookup
+          lookup_header="Foo Bar"
+          onLookupSave={() => { }}
+          data={mockData}
+          selected={[]}
+        />
+      </I18nProvider>
+    ).find('Lookup');
+    wrapper.instance().toggleSelected({
+      id: 1,
+      name: 'foo'
+    });
+    expect(wrapper.state('lookupSelectedItems')).toEqual([{
+      id: 1,
+      name: 'foo'
+    }]);
+    wrapper.instance().toggleSelected({
+      id: 1,
+      name: 'foo'
+    });
+    expect(wrapper.state('lookupSelectedItems')).toEqual([]);
+  });
+  test('saveModal calls callback with selected items', () => {
+    mockData = [{ name: 'foo', id: 1 }];
+    const onLookupSaveFn = jest.fn();
+    const wrapper = mount(
+      <I18nProvider>
+        <Lookup
+          lookup_header="Foo Bar"
+          onLookupSave={onLookupSaveFn}
+          data={mockData}
+          selected={[]}
+        />
+      </I18nProvider>
+    ).find('Lookup');
+    wrapper.instance().toggleSelected({
+      id: 1,
+      name: 'foo'
+    });
+    expect(wrapper.state('lookupSelectedItems')).toEqual([{
+      id: 1,
+      name: 'foo'
+    }]);
+    wrapper.instance().saveModal();
+    expect(onLookupSaveFn).toHaveBeenCalledWith([{
+      id: 1,
+      name: 'foo'
+    }]);
+  });
 });

__tests__/pages/Organizations/screens/OrganizationAdd.test.jsx

@@ -93,6 +93,26 @@ describe('<OrganizationAdd />', () => {
     });
   });
 
+  test('updateSelectedInstanceGroups successfully sets selectedInstanceGroups state', () => {
+    const wrapper = mount(
+      <MemoryRouter>
+        <OrganizationAdd api={{}} />
+      </MemoryRouter>
+    ).find('OrganizationAdd');
+    wrapper.instance().updateSelectedInstanceGroups([
+      {
+        id: 1,
+        name: 'foo'
+      }
+    ]);
+    expect(wrapper.state('selectedInstanceGroups')).toEqual([
+      {
+        id: 1,
+        name: 'foo'
+      }
+    ]);
+  });
+
   test('onSelectChange successfully sets custom_virtualenv state', () => {
     const wrapper = mount(
       <MemoryRouter>
@@ -103,41 +123,6 @@ describe('<OrganizationAdd />', () => {
     expect(wrapper.state('custom_virtualenv')).toBe('foobar');
   });
 
-  test('onLookupChange successfully adds/removes row from selectedInstanceGroups state', () => {
-    const wrapper = mount(
-      <MemoryRouter>
-        <OrganizationAdd api={{}} />
-      </MemoryRouter>
-    ).find('OrganizationAdd');
-    wrapper.setState({ results: [{
-      id: 1,
-      name: 'foo'
-    }] });
-    wrapper.instance().onLookupChange({
-      id: 1,
-      name: 'foo'
-    });
-    expect(wrapper.state('results')).toEqual([{
-      id: 1,
-      name: 'foo',
-      isChecked: true
-    }]);
-    expect(wrapper.state('selectedInstanceGroups')).toEqual([{
-      id: 1,
-      name: 'foo'
-    }]);
-    wrapper.instance().onLookupChange({
-      id: 1,
-      name: 'foo'
-    });
-    expect(wrapper.state('results')).toEqual([{
-      id: 1,
-      name: 'foo',
-      isChecked: false
-    }]);
-    expect(wrapper.state('selectedInstanceGroups')).toEqual([]);
-  });
-
   test('onSubmit posts instance groups from selectedInstanceGroups', async () => {
     const createOrganizationFn = jest.fn().mockResolvedValue({
       data: {

src/components/Lookup/Lookup.jsx

@@ -9,7 +9,7 @@ import {
   ToolbarGroup,
 } from '@patternfly/react-core';
 import { I18n } from '@lingui/react';
-import { t } from '@lingui/macro';
+import { t, Trans } from '@lingui/macro';
 
 import CheckboxListItem from '../ListItem';
 
@@ -20,30 +20,51 @@ class Lookup extends React.Component {
     super(props);
     this.state = {
       isModalOpen: false,
+      lookupSelectedItems: []
     };
     this.handleModalToggle = this.handleModalToggle.bind(this);
-    this.onLookup = this.onLookup.bind(this);
     this.wrapTags = this.wrapTags.bind(this);
     this.toggleSelected = this.toggleSelected.bind(this);
-  }
+    this.saveModal = this.saveModal.bind(this);
-
-  onLookup () {
-    this.handleModalToggle();
   }
 
   toggleSelected (row) {
-    const { lookupChange } = this.props;
+    const { lookupSelectedItems } = this.state;
-    lookupChange(row);
+    const selectedIndex = lookupSelectedItems
+      .findIndex(selectedRow => selectedRow.id === row.id);
+    if (selectedIndex > -1) {
+      lookupSelectedItems.splice(selectedIndex, 1);
+      this.setState({ lookupSelectedItems });
+    } else {
+      this.setState(prevState => ({
+        lookupSelectedItems: [...prevState.lookupSelectedItems, row]
+      }));
+    }
   }
 
   handleModalToggle () {
+    const { isModalOpen } = this.state;
+    const { selected } = this.props;
+    // Resets the selected items from parent state whenever modal is opened
+    // This handles the case where the user closes/cancels the modal and
+    // opens it again
+    if (!isModalOpen) {
+      this.setState({ lookupSelectedItems: [...selected] });
+    }
     this.setState((prevState) => ({
       isModalOpen: !prevState.isModalOpen,
     }));
   }
 
+  saveModal () {
+    const { onLookupSave } = this.props;
+    const { lookupSelectedItems } = this.state;
+    onLookupSave(lookupSelectedItems);
+    this.handleModalToggle();
+  }
+
   wrapTags (tags) {
-    return tags.filter(tag => tag.isChecked).map((tag) => (
+    return tags.map(tag => (
       <span className="awx-c-tag--pill" key={tag.id}>
         {tag.name}
         <Button className="awx-c-icon--remove" id={tag.id} onClick={() => this.toggleSelected(tag)}>
@@ -54,14 +75,14 @@ class Lookup extends React.Component {
   }
 
   render () {
-    const { isModalOpen } = this.state;
+    const { isModalOpen, lookupSelectedItems } = this.state;
     const { data, lookupHeader, selected } = this.props;
     return (
       <div className="pf-c-input-group awx-lookup">
-        <Button className="pf-c-input-group__text" aria-label="search" id="search" onClick={this.onLookup}>
+        <Button className="pf-c-input-group__text" aria-label="search" id="search" onClick={this.handleModalToggle}>
           <SearchIcon />
         </Button>
-        <div className="pf-c-form-control">{this.wrapTags(data)}</div>
+        <div className="pf-c-form-control">{this.wrapTags(selected)}</div>
         <Modal
           className="awx-c-modal"
           title={`Select ${lookupHeader}`}
@@ -74,17 +95,17 @@ class Lookup extends React.Component {
                 key={i.id}
                 itemId={i.id}
                 name={i.name}
-                isSelected={i.isChecked}
+                isSelected={lookupSelectedItems.some(item => item.id === i.id)}
                 onSelect={() => this.toggleSelected(i)}
               />
             ))}
           </ul>
-          {selected.length > 0 && (
+          {lookupSelectedItems.length > 0 && (
             <I18n>
               {({ i18n }) => (
                 <SelectedList
                   label={i18n._(t`Selected`)}
-                  selected={selected}
+                  selected={lookupSelectedItems}
                   showOverflowAfter={5}
                   onRemove={this.toggleSelected}
                 />
@@ -94,10 +115,14 @@ class Lookup extends React.Component {
           <ActionGroup className="at-align-right">
             <Toolbar>
               <ToolbarGroup>
-                <Button className="at-C-SubmitButton" variant="primary" onClick={this.handleModalToggle}>Select</Button>
+                <Button className="at-C-SubmitButton" variant="primary" onClick={this.saveModal}>
+                  <Trans>Select</Trans>
+                </Button>
               </ToolbarGroup>
               <ToolbarGroup>
-                <Button className="at-C-CancelButton" variant="secondary" onClick={this.handleModalToggle}>Cancel</Button>
+                <Button className="at-C-CancelButton" variant="secondary" onClick={this.handleModalToggle}>
+                  <Trans>Cancel</Trans>
+                </Button>
               </ToolbarGroup>
             </Toolbar>
           </ActionGroup>

src/pages/Organizations/screens/OrganizationAdd.jsx

@@ -34,11 +34,11 @@ class OrganizationAdd extends React.Component {
 
     this.handleChange = this.handleChange.bind(this);
     this.onSelectChange = this.onSelectChange.bind(this);
-    this.onLookupChange = this.onLookupChange.bind(this);
     this.onSubmit = this.onSubmit.bind(this);
     this.resetForm = this.resetForm.bind(this);
     this.onSuccess = this.onSuccess.bind(this);
     this.onCancel = this.onCancel.bind(this);
+    this.updateSelectedInstanceGroups = this.updateSelectedInstanceGroups.bind(this);
   }
 
   state = {
@@ -65,30 +65,6 @@ class OrganizationAdd extends React.Component {
     this.setState({ custom_virtualenv: value });
   }
 
-  onLookupChange (row) {
-    const { results, selectedInstanceGroups } = this.state;
-    const selectedIndex = selectedInstanceGroups
-      .findIndex(instanceGroup => instanceGroup.id === row.id);
-    const resultsIndex = results.findIndex(instanceGroup => instanceGroup.id === row.id);
-    if (selectedIndex > -1) {
-      // Remove it from the list of selected instance groups
-      selectedInstanceGroups.splice(selectedIndex, 1);
-      if (resultsIndex > -1) {
-        results[resultsIndex].isChecked = false;
-      }
-      this.setState({ selectedInstanceGroups, results });
-    } else {
-      // Add it to the list of selected instance groups
-      if (resultsIndex > -1) {
-        results[resultsIndex].isChecked = true;
-      }
-      this.setState(prevState => ({
-        results,
-        selectedInstanceGroups: [...prevState.selectedInstanceGroups, row]
-      }));
-    }
-  }
-
   async onSubmit () {
     const { api } = this.props;
     const { name, description, custom_virtualenv } = this.state;
@@ -128,6 +104,10 @@ class OrganizationAdd extends React.Component {
     history.push(`/organizations/${id}`);
   }
 
+  updateSelectedInstanceGroups (selectedInstanceGroups) {
+    this.setState({ selectedInstanceGroups });
+  }
+
   handleChange (_, evt) {
     this.setState({ [evt.target.name]: evt.target.value });
   }
@@ -184,7 +164,7 @@ class OrganizationAdd extends React.Component {
                 <FormGroup label="Instance Groups" fieldId="simple-form-instance-groups">
                   <Lookup
                     lookupHeader="Instance Groups"
-                    lookupChange={this.onLookupChange}
+                    onLookupSave={this.updateSelectedInstanceGroups}
                     data={results}
                     selected={selectedInstanceGroups}
                   />