React component for creating Nuage subnet #118

miha-plesko · 2018-07-27T12:13:01Z

With this commit we implement React component used to capture user inputs for creating CloudSubnet. Also, we add button to NetworkRouter details page to trigger the flow.

Video: http://x.k00.fr/subnetcreate
RFE BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1574972

Related PRs:

Implement CREATE operation for CloudSubnet #117 (already merged)
Expose CloudSubnet creation manageiq-api#440 (already merged)
Setup React component #121
Setup JavaScript tests for Nuage provider #122

(following ManageIQ/manageiq-providers-amazon#469)

/cc @Ladas @agrare

martinpovolny · 2018-08-03T12:22:17Z

app/javascript/components/create-nuage-cloud-subnet-form.jsx

+    super(props);
+    this.handleFormStateUpdate = this.handleFormStateUpdate.bind(this);
+    this.routerRef = 'ebca9d3b-53a4-4fd4-8268-196cb3fa6072'; // TODO: pass from Rails
+    this.emsId = 4;                                          // TODO: pass from Rails


generally you have ManageIQ.record.recordId with the id of the entity being displayed. If you need some other params, then the best way is to fetch them via API starting with the recordId.

Passing arguments through buttons from the toolbar will not help. Those are stored in a singleton evaluated at the first use of a toolbar (when the toolbar is required, well actually autoloaded).

Thanks, will apply Monday morning. We had a discussion with @Hyperkid123 and came to the same conclusion 👍 . BTW UI team did such a great job supporting React Components that I'm preparing a blogpost of it. Thank you so much for this.

miha-plesko · 2018-08-06T08:19:14Z

@martinpovolny @Hyperkid123 I organized files in 3 directories to make it more readable and reusable:

manageiq-providers-nuage/app/javascript/
├── components
│   └── create-nuage-cloud-subnet-form.jsx
├── forms
│   └── nuage-cloud-subnet-form.jsx
└── utils
    ├── api.js
    └── validators.js

Also, I was finally able to remove all the hard-coded stuff and obtain data from API hence I'm removing WIP tag on the PR. Kindly ask for your opinion, can we push it forward?

Hyperkid123

JS looks good.

The folder structure is for you to decide. Maybe i would add forms to under the components folder (because its a component also). But you should discuss this probably with your team.

Hyperkid123 · 2018-08-06T08:44:54Z

app/javascript/components/create-nuage-cloud-subnet-form.jsx

+    this.handleFormStateUpdate = this.handleFormStateUpdate.bind(this);
+    this.state = {
+      emsId: -1,
+      routerRef: -1,


I think i would prefer some bool value instead if this.

Hyperkid123 · 2018-08-06T08:53:17Z

app/javascript/forms/nuage-cloud-subnet-form.jsx

+                  <Field
+                    name="name"
+                    component={FinalFormField}
+                    label="Name"


Use i18n for all strings. name={__('Name')} should work. __ (two underscores) should be available on window.

Hyperkid123 · 2018-08-06T08:56:27Z

app/javascript/utils/api.js

+  resource: {...values, router_ref: routerRef},
+}).then(response => {
+  let res = response['results'][0];
+  window.add_flash(res.message, res.success ? 'success' : 'error');


I think it is safer to cycle through all result messages so you don't miss anything. response.forEach(item => window.add_flash...).

miha-plesko

Thanks for review @Hyperkid123, all what you suggest makes sense. I've applied and repushed. Do we also need to provide some kind of unit tests for it?

miha-plesko · 2018-08-06T09:13:20Z

app/javascript/components/create-nuage-cloud-subnet-form.jsx

+    this.handleFormStateUpdate = this.handleFormStateUpdate.bind(this);
+    this.state = {
+      emsId: -1,
+      routerRef: -1,


miha-plesko · 2018-08-06T09:15:19Z

app/javascript/forms/nuage-cloud-subnet-form.jsx

+                  <Field
+                    name="name"
+                    component={FinalFormField}
+                    label="Name"


miha-plesko · 2018-08-06T09:17:30Z

app/javascript/utils/api.js

+  resource: {...values, router_ref: routerRef},
+}).then(response => {
+  let res = response['results'][0];
+  window.add_flash(res.message, res.success ? 'success' : 'error');


Hyperkid123 · 2018-08-06T10:00:38Z

app/javascript/components/create-nuage-cloud-subnet-form.jsx

+    super(props);
+    this.handleFormStateUpdate = this.handleFormStateUpdate.bind(this);
+    this.state = {
+      emsId: false,


Ok now i don't like it either :D Now when i see it i would expect that those values will be bool in the future which is not true. I think it would be best just keep them undefined until you initialize them.

Hyperkid123 · 2018-08-06T10:03:14Z

app/javascript/components/create-nuage-cloud-subnet-form.jsx

+      payload: {
+        newRecord: true,
+        pristine: true,
+        addClicked: () => { createSubnet(this.state.values, this.state.emsId, this.state.routerRef); }


You don't need the curly brackets around createSubnet(...) function. You need them only for multi-line function body. Also you can drop the ;

Hyperkid123 · 2018-08-06T10:09:37Z

@miha-plesko yeah right i've completely forgot about tests. You will need to add additional config to enable testing.

I would recommend using JEST for JS testing and enzyme for React specific tests. You can check our config in react-ui-components (or in manageiq-ui-classic, you can pick), but they have got pretty good docs. And i can help you if you hit a wall or something.

miha-plesko · 2018-08-07T08:52:58Z

Okay so this PR has been split into three separate PRs:

Setup React component #121 - React config files
Setup JavaScript tests for Nuage provider #122 - javascript tests config files + Travis
this one - provides first actual component

The 1st and 2nd PRs have already been reviewed so I think we should merge them at this point so we can work further. I suggest either @Ladas or @agrare hits the button, should they agree, or @Hyperkid123 if you can suggest some guy that is more UI-related and has merge permissions, that's even better?

miha-plesko · 2018-08-07T08:54:31Z

Trying to find a word for such PRs. They are not "followup PRs" because we need to merge them upfront. Perhaps "followme PRs"?

Hyperkid123 · 2018-08-07T11:16:57Z

@miha-plesko i think merging should be done by someone in your team. But i have no problem doing reviews.

agrare · 2018-08-07T11:18:59Z

@Hyperkid123 if you approve I'll merge this

Hyperkid123 · 2018-08-07T11:20:57Z

@agrare ok i would suggest merge #121 and then #122 and this PR after that.

miha-plesko · 2018-08-08T14:44:14Z

Okay @Hyperkid123 I came up with jest/enzyme tests. Took me a while to figure how to handle promises but I think it eventually came out quite readable. Thanks to unit tests it turned out we didn't handle API error anywhere. Added it now.

Looking forward to your opinion, especially on unit tests!

Hyperkid123 · 2018-08-13T07:23:03Z

app/javascript/components/forms/nuage-cloud-subnet-form.jsx

+import { FinalFormField, FinalFormTextArea, FinalFormSelect } from '@manageiq/react-ui-components/dist/forms';
+import { ip4Validator, netmaskValidator } from '../../utils/validators'
+
+class NuageCloudSubnetForm extends Component {


When you are writing stateless components (without state or any lifecycle methods) it is generally better to write those components as functions.

Yeah my thinking was to have it all done the same way, but I agree, we can use function here to spare some LOC.

Hyperkid123 · 2018-08-13T07:25:21Z

app/javascript/utils/validators.js

+import { addValidator } from 'redux-form-validators';
+
+export const ip4Validator = addValidator({
+  defaultMessage: 'Must be IPv4 address',


I thin you should use i18n here too.

Hyperkid123 · 2018-08-13T07:28:31Z

spec/javascript/components/create-nuage-cloud-subnet-form.test.jsx

+  });
+});
+
+const renderComponent = () => shallowRedux(<CreateNuageCloudSubnetForm />);


I think u should use beforeEach (or beforeAll) functions that are available in jest. (see https://jestjs.io/docs/en/setup-teardown)

Um, but then I would always render the same component in same context, right? Here I want to render component in different context: once when fetchRouter succeeds, once when it fails etc. As far as I've been looking, it's not possible in it to influence the beforeEach in any way so I think I cannot let it know what context I want. Or did I miss something?

Hyperkid123 · 2018-08-13T07:29:52Z

spec/javascript/components/forms/nuage-cloud-subnet-form.test.jsx

+  });
+});
+
+const renderComponent = (loading = false) => shallowRedux(<NuageCloudSubnetForm updateFormState={jest.fn()} loading={loading} />);


Same here use the beforeEach to prepare common data or components for tests

So here are two its: first one renders component with loading=true and the other one with loading=false. If I do the rendering in beforeEach then it will render it in one way always, right?

Feels like I'm missing something, kindly ask for hint.

Hyperkid123 · 2018-08-13T07:34:47Z

spec/javascript/components/create-nuage-cloud-subnet-form.test.jsx

+      let component = renderComponent();
+      return fetchRouterMock().then(() => {
+        component.update();
+        expect(fetchRouterMock).toHaveBeenCalledWith(RECORD_ID);


I am not against globals for testing, but i don't think its a good idea to store test specific in them values. It should be immediately clear what is inside such values. I think this value should be set in tests.

Good point, done.

Hyperkid123 · 2018-08-13T07:36:34Z

spec/javascript/setup.js

+
+// Global variables that Components usually get from elsewhere.
+global.RECORD_ID = 123;
+global.ManageIQ = { record: { recordId: global.RECORD_ID } };


This is never used in tests.

Following your suggestion I now set this in beforeEach block in the test itself. But I find it more readable to do it like this:

// setup.js global.ManageIQ = { record: { recordId: -1 } };

// create-nuage-cloud-subnet-form.js beforeEach(() => { ManageIQ.record.recordId = 123; });

than to do it like this:

// create-nuage-cloud-subnet-form.js beforeEach(() => { global.ManageIQ = { record: { recordId: 123 } }; });

Please let me know if you disagree.

miha-plesko

Thanks for review @Hyperkid123 . I'm not sure if we can use beforeEach in all cases, please see below.

miha-plesko · 2018-08-13T07:49:42Z

app/javascript/components/forms/nuage-cloud-subnet-form.jsx

+import { FinalFormField, FinalFormTextArea, FinalFormSelect } from '@manageiq/react-ui-components/dist/forms';
+import { ip4Validator, netmaskValidator } from '../../utils/validators'
+
+class NuageCloudSubnetForm extends Component {


Yeah my thinking was to have it all done the same way, but I agree, we can use function here to spare some LOC.

miha-plesko · 2018-08-13T07:51:30Z

app/javascript/utils/validators.js

+import { addValidator } from 'redux-form-validators';
+
+export const ip4Validator = addValidator({
+  defaultMessage: 'Must be IPv4 address',


miha-plesko · 2018-08-13T08:05:55Z

spec/javascript/components/create-nuage-cloud-subnet-form.test.jsx

+      let component = renderComponent();
+      return fetchRouterMock().then(() => {
+        component.update();
+        expect(fetchRouterMock).toHaveBeenCalledWith(RECORD_ID);


Good point, done.

miha-plesko · 2018-08-13T08:11:32Z

spec/javascript/components/create-nuage-cloud-subnet-form.test.jsx

+  });
+});
+
+const renderComponent = () => shallowRedux(<CreateNuageCloudSubnetForm />);


Um, but then I would always render the same component in same context, right? Here I want to render component in different context: once when fetchRouter succeeds, once when it fails etc. As far as I've been looking, it's not possible in it to influence the beforeEach in any way so I think I cannot let it know what context I want. Or did I miss something?

miha-plesko · 2018-08-13T08:13:19Z

spec/javascript/components/forms/nuage-cloud-subnet-form.test.jsx

+  });
+});
+
+const renderComponent = (loading = false) => shallowRedux(<NuageCloudSubnetForm updateFormState={jest.fn()} loading={loading} />);


So here are two its: first one renders component with loading=true and the other one with loading=false. If I do the rendering in beforeEach then it will render it in one way always, right?

Feels like I'm missing something, kindly ask for hint.

miha-plesko · 2018-08-13T08:20:09Z

spec/javascript/setup.js

+
+// Global variables that Components usually get from elsewhere.
+global.RECORD_ID = 123;
+global.ManageIQ = { record: { recordId: global.RECORD_ID } };


Following your suggestion I now set this in beforeEach block in the test itself. But I find it more readable to do it like this:

// setup.js global.ManageIQ = { record: { recordId: -1 } };

// create-nuage-cloud-subnet-form.js beforeEach(() => { ManageIQ.record.recordId = 123; });

than to do it like this:

// create-nuage-cloud-subnet-form.js beforeEach(() => { global.ManageIQ = { record: { recordId: 123 } }; });

Please let me know if you disagree.

With this commit we implement react component used to capture user inputs for creating CloudSubnet. Also, we add button to NetworkRouter details page to trigger the flow. RFE BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1574972 Signed-off-by: Miha Pleško <[email protected]>

miha-plesko · 2018-08-13T08:45:29Z

Just pushed updated version with render auxilary functions defined in beforeAll blocks instead on the bottom of files.

Hyperkid123

Looks good

miq-bot · 2018-08-13T08:48:23Z

Checked commit miha-plesko@056dfef with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

miha-plesko · 2018-08-13T09:25:49Z

@agrare we have thumbsup from @Hyperkid123, hit the button at will.

React component for creating Nuage subnet

miq-bot added the wip label Jul 27, 2018

miha-plesko mentioned this pull request Jul 27, 2018

[DO NOT MERGE] Initial support for AnsibleRunner-driven CRUD #103

Closed

miha-plesko force-pushed the create-subnet-ui branch 2 times, most recently from 42200e9 to c192851 Compare July 30, 2018 14:12

miha-plesko mentioned this pull request Jul 31, 2018

Expose CloudSubnet creation ManageIQ/manageiq-api#440

Merged

miha-plesko force-pushed the create-subnet-ui branch 2 times, most recently from 0f1ee50 to eb2a14b Compare July 31, 2018 08:50

miha-plesko added the enhancement label Jul 31, 2018

miha-plesko assigned Ladas Jul 31, 2018

martinpovolny reviewed Aug 3, 2018

View reviewed changes

miha-plesko force-pushed the create-subnet-ui branch from eb2a14b to 111b4e3 Compare August 6, 2018 08:12

miha-plesko removed the wip label Aug 6, 2018

miha-plesko changed the title ~~[WIP] React component for creating Nuage subnet~~ React component for creating Nuage subnet Aug 6, 2018

Hyperkid123 reviewed Aug 6, 2018

View reviewed changes

miha-plesko force-pushed the create-subnet-ui branch from 111b4e3 to 7a1b6c5 Compare August 6, 2018 09:22

miha-plesko commented Aug 6, 2018

View reviewed changes

Hyperkid123 reviewed Aug 6, 2018

View reviewed changes

miha-plesko force-pushed the create-subnet-ui branch 2 times, most recently from 7e86dca to 7624292 Compare August 7, 2018 08:44

miha-plesko force-pushed the create-subnet-ui branch from 7624292 to 2e5a864 Compare August 8, 2018 14:39

Hyperkid123 reviewed Aug 13, 2018

View reviewed changes

miha-plesko force-pushed the create-subnet-ui branch from 2e5a864 to 359868a Compare August 13, 2018 08:22

miha-plesko commented Aug 13, 2018

View reviewed changes

miha-plesko force-pushed the create-subnet-ui branch from 359868a to 056dfef Compare August 13, 2018 08:43

Hyperkid123 approved these changes Aug 13, 2018

View reviewed changes

agrare approved these changes Aug 13, 2018

View reviewed changes

agrare merged commit 056dfef into ManageIQ:master Aug 13, 2018

agrare added a commit that referenced this pull request Aug 13, 2018

Merge pull request #118 from miha-plesko/create-subnet-ui

bd6b106

React component for creating Nuage subnet

agrare added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 13, 2018

miha-plesko deleted the create-subnet-ui branch August 31, 2018 06:15

miha-plesko mentioned this pull request Apr 17, 2019

React Component that triggers Provisioning Request ManageIQ/manageiq-providers-redfish#58

Merged

1 task

React component for creating Nuage subnet #118

React component for creating Nuage subnet #118

Conversation

miha-plesko commented Jul 27, 2018 • edited Loading

martinpovolny Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko commented Aug 6, 2018

Hyperkid123 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hyperkid123 Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hyperkid123 commented Aug 6, 2018

miha-plesko commented Aug 7, 2018

miha-plesko commented Aug 7, 2018

Hyperkid123 commented Aug 7, 2018 • edited Loading

agrare commented Aug 7, 2018

Hyperkid123 commented Aug 7, 2018

miha-plesko commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hyperkid123 Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hyperkid123 Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko commented Aug 13, 2018

Hyperkid123 left a comment

Choose a reason for hiding this comment

miq-bot commented Aug 13, 2018

miha-plesko commented Aug 13, 2018

miha-plesko commented Jul 27, 2018 •

edited

Loading

martinpovolny Aug 3, 2018 •

edited

Loading

Hyperkid123 left a comment •

edited

Loading

Hyperkid123 Aug 6, 2018 •

edited

Loading

Hyperkid123 commented Aug 7, 2018 •

edited

Loading

Hyperkid123 Aug 13, 2018 •

edited

Loading

Hyperkid123 Aug 13, 2018 •

edited

Loading