fix(ui): ignore false change events in VariableForm #15317

drdelambre · 2019-10-04T19:57:01Z

If a user re-selects the same variable type when adding a variable, no action takes place

closes #15059 if a user re-selects the same variable type when adding a variable, no action takes place

desa

👍

drdelambre · 2019-10-04T21:41:34Z

waiting on clarification of acceptance criteria before merging

the original intent of the issue was to persist user data across type selection interfaces, not only guard against double clicks. this commit pushes all of the variable editor information down to redux to allow persistance outside of the view state until the user clicks "cancel" or "create" in the interface. more tests forthcoming

made more sense than all of the copy pasta that was being slung around in the previous commit. Tests should still live on the form.

desa · 2019-10-07T20:49:56Z

@drdelambre it's pretty common that people rebase and squash things into a single commit, but there's no hard requirements on that at the moment. Feel free to merge at your discretion.

drdelambre · 2019-10-07T21:02:03Z

i'm all about it! game plan is to use the big green "squash and merge" button and rewrite the commit log, unless otherwise instructed.

desa · 2019-10-07T21:06:33Z

works for me 👍

121watts

This looks awesome. Some nits but 👍

121watts · 2019-10-07T23:45:23Z

ui/src/types/arguments.ts

-  values: KeyValueMap
+interface Argument<T> {
+  type: VariableArgumentType
+  values: T


Love this use of generics.

121watts · 2019-10-07T23:49:34Z

ui/src/variables/components/VariableArgumentsEditor.tsx

      case 'query':
        return (
          <Form.Element label="Script">
            <Grid.Column>
              <div className="overlay-flux-editor">
                <FluxEditor
-                  script={args.values.query}
+                  script={(args as QueryArguments).values.query}


Generally a smell when I have to cast a type using as

felt the same way, seemed to come along with the use of the generics and the vartype = type1 | type2 | type3 definitions. typescript seems to be only able to resolve in one direction. any tips on mitigating this kind of polymorphism?

I have a solution, but it's in a whole branch. let's look at it together tomorrow?

121watts · 2019-10-07T23:49:58Z

ui/src/variables/components/VariableForm.test.tsx

@@ -0,0 +1,84 @@
+// Libraries
+import React from 'react'
+import {shallow} from 'enzyme'


Why not use react-testing-library?

....cause i forgot.
WILL DO!

121watts · 2019-10-07T23:51:27Z

ui/src/variables/components/VariableForm.test.tsx

+import VariableForm from 'src/variables/components/VariableForm'
+
+// Constants
+import {variables} from 'mocks/dummyData'


Let's decide on a convention on where to put dummy test data.

i need a rough overview of this tomorrow. running into weird conditions when trying to run the code in different environments.

121watts · 2019-10-07T23:51:56Z

ui/src/variables/components/VariableForm.test.tsx

+  })
+
+  describe('type selector', () => {
+    it('should not tell the the parrent if the type was not changed', () => {


Suggested change

it('should not tell the the parrent if the type was not changed', () => {

it('should not tell the the parent if the type was not changed', () => {

lol thanks for the catch

121watts · 2019-10-07T23:54:11Z

ui/src/variables/components/VariableFormContext.test.tsx

@@ -0,0 +1,45 @@
+// Libraries
+import React from 'react'
+import {shallow} from 'enzyme'


I thought you liked react-testing-library

121watts · 2019-10-07T23:55:12Z

ui/src/variables/components/VariableFormContext.tsx

+}
+
+const mstp = (state: AppState): StateProps => {
+  const variables = extractVariablesList(state),


Love all the selectors.

Silly nit but is the prefix extract adding value to these function names?

not at all! i was trying to follow the convention, but extractions/segmentation/whatever is a function of their include path, so it's a bit redundant.

ebb-tide · 2019-10-04T20:55:37Z

ui/src/variables/components/VariableForm.test.tsx

+// Libraries
+import React from 'react'
+import {shallow} from 'enzyme'
+
+// Components
+import VariableForm from 'src/variables/components/VariableForm'
+
+// Constants
+import {variables} from 'mocks/dummyData'
+
+const setup = (override?) => {
+  const props = {
+    variables,
+    initialScript: 'Hello There!',
+    onCreateVariable: jest.fn(),
+    onHideOverlay: jest.fn(),
+    ...override,
+  }
+
+  const wrapper = shallow<VariableForm>(<VariableForm {...props} />)
+
+  return {wrapper}
+}
+
+describe('VariableForm', () => {
+  describe('rendering', () => {
+    it('renders', () => {
+      const {wrapper} = setup()
+      expect(wrapper.exists()).toBe(true)
+      expect(wrapper).toMatchSnapshot()
+    })
+  })
+
+  describe('type selector', () => {
+    it('should not update state if the type was not changed', () => {
+      const {wrapper} = setup()
+
+      const defaultType = wrapper.state().args.type
+      const stateStub = jest.fn()
+
+      wrapper.instance().setState = stateStub
+
+      // this way of accessing the instance function
+      // bypasses typescript's private function check
+      wrapper.instance()['handleChangeType'](defaultType)
+      expect(stateStub.mock.calls.length).toBe(0)
+    })
+
+    it('should update state if the type was changed', () => {
+      const {wrapper} = setup()
+
+      const stateStub = jest.fn()
+
+      wrapper.instance().setState = stateStub
+
+      // this way of accessing the instance function
+      // bypasses typescript's private function check
+      wrapper.instance()['handleChangeType']('map')
+      expect(stateStub.mock.calls.length).toBe(1)
+    })
+  })
+})


😍😍😍

ebb-tide · 2019-10-08T00:22:55Z

ui/src/dataExplorer/components/SaveAsVariable.tsx

  mstp,
-  mdtp
+  () => {}


I'm getting a mapDispatchToProps() in Connect(withRouter(SaveAsVariable)) must return a plain object. Instead received undefined error in the console from this line. I think we have used this previously:

export default connect<StateProps, {}, OwnProps>( mstp, null )(Component)

ebb-tide · 2019-10-08T00:53:43Z

ui/src/variables/components/VariableArgumentsEditor.tsx

      case 'query':
        return (
          <Form.Element label="Script">
            <Grid.Column>
              <div className="overlay-flux-editor">
                <FluxEditor
-                  script={args.values.query}
+                  script={(args as QueryArguments).values.query}


I have a solution, but it's in a whole branch. let's look at it together tomorrow?

ebb-tide · 2019-10-08T01:06:59Z

ui/src/variables/actions/index.ts

+export const updateConstant = (arg: VariableArguments): SetConstant => ({
+  type: 'UPDATE_VARIABLE_EDITOR_CONSTANT',
+  payload: arg,
+})


(nit) One pattern that makes these just a little bit easier to read since you don't have to separately define types..

export type Actions = | ReturnType<typeof createAction> | ReturnType<typeof createOtherAction> const createAction = (status: RemoteDataState) => ({ type: 'CREATE_THIS_ACTION' as 'CREATE_THIS_ACTION' payload: {status}, })

is there some more examples in the code so that i can wrap my head around this?

ui/src/alerting/actions/checks.ts

updated the file to reflect the new pattern!

ebb-tide · 2019-10-08T01:18:54Z

ui/src/variables/reducers/index.ts

+export const initialEditorState = (): VariableEditorState => ({
+  name: '',
+  selected: 'query',
+


runaway newline! :)

…_15059

…alex_bug_15059

closes #15059 the issue is to persist user data across variable type selection interfaces within the variable editor. this commit pushes all of the variable editor information down to redux to allow persistence outside of the view state until the user clicks "cancel" or "create" in the interface.

add test for hydrateVars dashboard variable dropdown test: inspect values, not just array length add RTL test for variable dropdown changes lint fix: Disable saving threshold check if no threshold selected (#15348) * Prevent check saving if no thresholds * Add tests * Add changes to changelog * make optional props optional * use false instead of null for boolean changelog fix(ui): ignore false change events in VariableForm (#15317) closes #15059 the issue is to persist user data across variable type selection interfaces within the variable editor. this commit pushes all of the variable editor information down to redux to allow persistence outside of the view state until the user clicks "cancel" or "create" in the interface.

fix(ui): ignore false change events in VariableForm

2ae6a75

closes #15059 if a user re-selects the same variable type when adding a variable, no action takes place

drdelambre requested a review from a team October 4, 2019 19:57

Alex Boatwright added 2 commits October 4, 2019 13:08

fix(ui): updating some comments and linting

d69e99a

fix(ui): whitespace

4fd11c1

desa approved these changes Oct 4, 2019

View reviewed changes

Alex Boatwright added 2 commits October 5, 2019 15:41

fix(ui): moving the unified state fetching into its own component

69343cc

made more sense than all of the copy pasta that was being slung around in the previous commit. Tests should still live on the form.

Alex Boatwright added 2 commits October 7, 2019 14:49

fix(ui): added a test, but dont know if it helped

ffab2e2

fix(ui): typo

6d3d142

121watts approved these changes Oct 7, 2019

View reviewed changes

ebb-tide reviewed Oct 8, 2019

View reviewed changes

ebb-tide and others added 6 commits October 8, 2019 09:53

WIP

57afc4d

fix(ui): moved a test to RTL

2100402

Merge branch 'master' of github.com:influxdata/influxdb into alex_bug…

c51d3d2

…_15059

Merge branch 'alex_bug_15059' of github.com:influxdata/influxdb into …

b81193a

…alex_bug_15059

fix(ui): addressing PR comments

c3f68f7

fix(ui): forgot to remove error that shows up in console

11dc62f

drdelambre merged commit 99478fe into master Oct 8, 2019

drdelambre deleted the alex_bug_15059 branch October 8, 2019 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ui): ignore false change events in VariableForm #15317

fix(ui): ignore false change events in VariableForm #15317

drdelambre commented Oct 4, 2019

desa left a comment

drdelambre commented Oct 4, 2019

desa commented Oct 7, 2019

drdelambre commented Oct 7, 2019

desa commented Oct 7, 2019

121watts left a comment

121watts Oct 7, 2019

121watts Oct 7, 2019

drdelambre Oct 8, 2019

ebb-tide Oct 8, 2019

121watts Oct 7, 2019

drdelambre Oct 8, 2019

121watts Oct 7, 2019

drdelambre Oct 8, 2019

121watts Oct 7, 2019

drdelambre Oct 8, 2019

121watts Oct 7, 2019

121watts Oct 7, 2019

drdelambre Oct 8, 2019

ebb-tide Oct 4, 2019

ebb-tide Oct 8, 2019

ebb-tide Oct 8, 2019

ebb-tide Oct 8, 2019

drdelambre Oct 8, 2019

ebb-tide Oct 8, 2019

drdelambre Oct 8, 2019

ebb-tide Oct 8, 2019

	it('should not tell the the parrent if the type was not changed', () => {
	it('should not tell the the parent if the type was not changed', () => {

fix(ui): ignore false change events in VariableForm #15317

fix(ui): ignore false change events in VariableForm #15317

Conversation

drdelambre commented Oct 4, 2019

desa left a comment

Choose a reason for hiding this comment

drdelambre commented Oct 4, 2019

desa commented Oct 7, 2019

drdelambre commented Oct 7, 2019

desa commented Oct 7, 2019

121watts 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

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

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