Add Job and Bridge Creation Pages #483

NavyAdmiral · 2018-08-07T02:39:05Z

No description provided.

NavyAdmiral · 2018-08-07T03:30:38Z

@rupurt Wanted to test the container properly without shallow rendering, but I keep getting unknown node with tag 13 error.

rupurt · 2018-08-08T19:32:40Z

@NavyAdmiral I'm not exactly sure why that error is happening but doesn't occur when I remove the <BridgeForm /> component.

NavyAdmiral · 2018-08-10T19:19:22Z

@rupurt There was a bug with Enzyme and this has been reported both in Formik and Enzyme repos. issue
Enzyme 3.4.0 fixes it ✅

rupurt

@NavyAdmiral nice job on tracking down the Enzyme bug. I've investigated them before and it's quite frustrating.

rupurt · 2018-08-14T16:58:36Z

gui/src/containers/Jobs.js

          </Grid>
-          <Grid item xs={3}>
-            {renderSidebar(this.props)}
+          <Grid item>


I feel like this should only have a Create Job button on this page. How do you feel about moving the Create Bridge button to the bridges index page?

Also, how do you feel about aligning the button to the right of the grid?

Right of the grid? If I understand you correctly, I didn't like aligning it that way because Drawer slid over it when opened.

rupurt · 2018-08-14T17:00:33Z

gui/src/containers/Jobs.js

          </Grid>
+          <Grid item>


I think this needs the xs attribute with a column size.

rupurt · 2018-08-14T17:04:59Z

gui/src/api.js

@@ -73,4 +74,6 @@ export const getBridgeSpec = (name) => get(`/v2/bridge_types/${name}`)

 export const createSession = (data) => post(`/sessions`, data)

+export const createObject = (endpoint, data, shouldStringify) => post(`/${endpoint}`, data, shouldStringify)


I feel like by having to pass in the endpoint we are leaking concerns. Actions shouldn't need to know about things like a URL path.

How do you feel about creating wrapper functions called something like createBridgeType & createJobSpec that abstracts this away?

rupurt · 2018-08-14T17:06:57Z

gui/src/components/BridgeForm.js

+  handleSubmit (values, { props }) {
+    const formattedValues = JSON.parse(JSON.stringify(values).replace('confirmations', 'defaultConfirmations'))
+    formattedValues.defaultConfirmations = parseInt(formattedValues.defaultConfirmations) || 0
+    props.submitCreate('v2/bridge_types', formattedValues, true)


Instead of passing potentially invalid JSON to the endpoint how do you feel about parsing it in the browser and displaying an error instead?

@rupurt since confirmations is an optional field of type int as demanded by the api, if I don't manually parseInt the input, then it will make a post request with string which will fail. alternatively, the input field can be set as type='number', so that no manual parsing is required, but if it's left untouched (no input from user) then it won't default to 0, it will default to ''

rupurt · 2018-08-14T17:08:46Z

gui/src/components/Flash.js

@@ -20,17 +20,23 @@ const styles = (theme) => {
    error: {
      backgroundColor: theme.palette.error.dark,
      color: theme.palette.error.contrastText
+    },
+    warning: {


We should avoid using hex color codes outside of the theme. Can you add these to palette under something like warning?

https://github.com/smartcontractkit/chainlink/blob/master/gui/src/theme.js

rupurt · 2018-08-14T17:09:11Z

gui/src/components/Flash.js

    }
  })
 }

-const applyClass = ({base, success, error, classes, className}) => {
+const applyClass = ({base, success, error, warning, classes, className}) => {


rupurt · 2018-08-14T17:11:21Z

gui/src/connectors/redux/reducers/create.js

+
+export default (state = initialState, action = {}) => {
+  switch (action.type) {
+    case MATCH_ROUTE:


As this doesn't change any properties, how do you feel about returning initialState directly?

rupurt · 2018-08-14T17:12:07Z

gui/src/containers/Create.js

+    paddingBottom: theme.spacing.unit * 2
+  },
+  tabPadding: {
+    padding: 24


This should be relative to the theme's spacing

rupurt · 2018-08-14T17:14:37Z

gui/src/containers/Create.js

+    // Need to set int value because <Tabs/> component
+    // will not focus on tab with strings
+    if (this.props.match) {
+      switch (this.props.match.params.structure) {


How do you feel about using setState with a ternary? Something like:

const value = this.props.match.params.structure === 'job' ? 1 : 0 this.setState({ value: value })

@rupurt That will not work if we add Create Service Agreement in this tab container. Do you want to see it with a ternary in this PR though?

rupurt · 2018-08-14T17:16:23Z

gui/src/containers/Create.js

+    if (this.props.history) {
+      switch (value) {
+        case 0:
+          this.props.history.replace('/create/bridge')


I feel like these should be regular links so that a history entry is pushed onto the stack instead of replacing the entry on the stack. If we go down this route the back button doesn't work.

am I wrong to think .push should make it work?

NavyAdmiral · 2018-08-15T18:18:39Z

@rupurt addressed most of the comments; I'm not too happy with create button positionings

NavyAdmiral · 2018-08-15T18:42:34Z

@rupurt addressed most of the comments including the create button positioning. I think I liked both create job/create bridge buttons on the `/ || /jobs`` page more though

j16r · 2018-08-15T20:45:41Z

Just looking at the form, a couple of thoughts:

When I submitted apparently invalid JSON, the submit button was disabled, but there didn't appear to be a way to enable it again.
The submit button in the form isn't obvious until I move my mouse cursor over it, like the sign in form, the button should have a clear demarcation from its surroundings.
The JSON field should probably be a text area, rather than an input box.
The verbs around fields are unnecessary "e.g: Type Bridge Name", the noun should be sufficient, e.g: "Paste JSON" should be "Job Spec"
Not sure how I feel about combining all the forms into the one GUI with the tabs to select different functions, could be a bit limiting.

NavyAdmiral · 2018-08-15T20:57:03Z

@j16r in what way do you think combining all the forms could be limiting?

j16r · 2018-08-15T21:58:37Z

It limits the opportunities to take cues from the context in which it is launched, e.g: imagine that the form for creating a job is at the top of the jobs index. Then you can view example jobs and you can see when the job is added live as you add the job. If you can change the "Create Job" form to another form, then this context no longer makes sense.

NavyAdmiral · 2018-08-21T18:39:51Z

Will reopen a new PR as this one got messy.

NavyAdmiral force-pushed the Job-And-Bridge-Pages branch from 26c2438 to c4059af Compare August 7, 2018 02:40

NavyAdmiral force-pushed the Job-And-Bridge-Pages branch 4 times, most recently from c2c1957 to d6cd1dc Compare August 8, 2018 10:25

NavyAdmiral force-pushed the Job-And-Bridge-Pages branch 6 times, most recently from 0076650 to 2c39d17 Compare August 10, 2018 19:13

NavyAdmiral force-pushed the Job-And-Bridge-Pages branch from 0fba22f to 37d7199 Compare August 11, 2018 00:39

rupurt suggested changes Aug 14, 2018

View reviewed changes

NavyAdmiral force-pushed the Job-And-Bridge-Pages branch 7 times, most recently from 368767b to 8eee300 Compare August 15, 2018 08:02

NavyAdmiral force-pushed the Job-And-Bridge-Pages branch from 8eee300 to db02a43 Compare August 15, 2018 18:36

NavyAdmiral force-pushed the Job-And-Bridge-Pages branch from 2a30464 to db02a43 Compare August 15, 2018 20:01

rupurt and others added 27 commits August 21, 2018 22:29

Remove unused imports in truffle test helpers

7949081

Import support helpers into variable

b8c070e

Add circleci configuration

0d6b476

Only trap signals in ethereum_test if !CIRCLECI

f8cc93b

Bump builder image to 1.0.6

8035749

Only skip killing child processes for CircleCI

91bb900

Clean commit

a44e7b8

redux and flash

aa38ffb

handle network error

1fe210f

ephemeral errors

acd6302

handle Tabs better & improved navigation

55bc335

remove individual fetching

9e024e4

make post function configurable

d1f793f

make sure flash notif works as expected

0ccf0ea

dry forms

e054d81

readjust tests for reducer and container

9e155cc

clean and warning for expired session

f3a9208

bigger height for notifications

e3d522d

upgrade enzyme and enzyme adapter react

249a00f

add Create container test

bf3d4e0

Warn user on leaving filled and unsubmitted form

8f6766f

refactor notifications to be usable by both bridge and job

83ae901

suggestions

6c17207

lint

0b061d1

decompose and tweak

6ff4c8c

suggestions

4b1c0a7

use grid for both forms

96393e3

NavyAdmiral force-pushed the Job-And-Bridge-Pages branch from a22387d to 96393e3 Compare August 21, 2018 18:29

NavyAdmiral closed this Aug 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Job and Bridge Creation Pages #483

Add Job and Bridge Creation Pages #483

NavyAdmiral commented Aug 7, 2018

NavyAdmiral commented Aug 7, 2018

rupurt commented Aug 8, 2018 •

edited

Loading

NavyAdmiral commented Aug 10, 2018 •

edited

Loading

rupurt left a comment

rupurt Aug 14, 2018

NavyAdmiral Aug 14, 2018

rupurt Aug 14, 2018

rupurt Aug 14, 2018

rupurt Aug 14, 2018

NavyAdmiral Aug 14, 2018 •

edited

Loading

rupurt Aug 14, 2018

rupurt Aug 14, 2018

rupurt Aug 14, 2018 •

edited

Loading

rupurt Aug 14, 2018

rupurt Aug 14, 2018

NavyAdmiral Aug 14, 2018 •

edited

Loading

rupurt Aug 14, 2018

NavyAdmiral Aug 14, 2018

NavyAdmiral commented Aug 15, 2018

NavyAdmiral commented Aug 15, 2018

j16r commented Aug 15, 2018

NavyAdmiral commented Aug 15, 2018

j16r commented Aug 15, 2018

NavyAdmiral commented Aug 21, 2018

		@@ -73,4 +74,6 @@ export const getBridgeSpec = (name) => get(`/v2/bridge_types/${name}`)

		export const createSession = (data) => post(`/sessions`, data)

		export const createObject = (endpoint, data, shouldStringify) => post(`/${endpoint}`, data, shouldStringify)

Add Job and Bridge Creation Pages #483

Add Job and Bridge Creation Pages #483

Conversation

NavyAdmiral commented Aug 7, 2018

NavyAdmiral commented Aug 7, 2018

rupurt commented Aug 8, 2018 • edited Loading

NavyAdmiral commented Aug 10, 2018 • edited Loading

rupurt 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

NavyAdmiral Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rupurt Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NavyAdmiral Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NavyAdmiral commented Aug 15, 2018

NavyAdmiral commented Aug 15, 2018

j16r commented Aug 15, 2018

NavyAdmiral commented Aug 15, 2018

j16r commented Aug 15, 2018

NavyAdmiral commented Aug 21, 2018

rupurt commented Aug 8, 2018 •

edited

Loading

NavyAdmiral commented Aug 10, 2018 •

edited

Loading

NavyAdmiral Aug 14, 2018 •

edited

Loading

rupurt Aug 14, 2018 •

edited

Loading

NavyAdmiral Aug 14, 2018 •

edited

Loading