COMPASS 235 DV: Full range component #551

pzrq · 2016-10-31T13:49:02Z

No description provided.

pzrq · 2016-10-31T22:56:03Z

Still needs Decimal128 and probably could use some spot checks and playing with it.

rueckstiess · 2016-11-01T11:28:51Z

Pushed a small change to make the rule fit into a single line and make them look a bit nicer.

Couple of things I noticed from using the rule builder:

The ranges should not indicate an error before the user had the chance to enter something (i.e. validation should not happen initially).
Even if an invalid input is entered, I can still click "update" and the component sends something back to the server, e.g. an empty string. I don't know how we should handle this, perhaps disable the "Update" button?

I haven't had a chance yet to go through the full code, but I need to get some 💤.

I'm not too worried, we can always ship validation without this rule category and add it in a point release later, it's not a blocker for validation to go out.

imlucas · 2016-11-01T20:13:25Z

Currently failing a few tests.

  1) <RangeInput /> when rendering the default control has label `LOWER BOUND`:
     Error: Method “dive” is only meant to be run on a single node. 0 found instead.
      at ShallowWrapper.single (node_modules/enzyme/build/ShallowWrapper.js:1401:17)
      at ShallowWrapper.dive (node_modules/enzyme/build/ShallowWrapper.js:1477:21)
      at Context.it (test/validation.range.test.js:22:54)
  2) <RangeInput /> when rendering the default control has placeholder text of `enter lower bound`:
      AssertionError: expected 'lower bound' to equal 'enter lower bound'
      + expected - actual
      -lower bound
      +enter lower bound

      at Context.it (test/validation.range.test.js:28:37)
  3) <RangeInput /> when rendering an upperBound control has label `UPPER BOUND`:
     Error: Method “dive” is only meant to be run on a single node. 0 found instead.
      at ShallowWrapper.single (node_modules/enzyme/build/ShallowWrapper.js:1401:17)
      at ShallowWrapper.dive (node_modules/enzyme/build/ShallowWrapper.js:1477:21)
      at Context.it (test/validation.range.test.js:54:54)
  4) <RangeInput /> when rendering an upperBound control has placeholder text of `enter upper bound`:
      AssertionError: expected 'upper bound' to equal 'enter upper bound'
      + expected - actual
      -upper bound
      +enter upper bound

      at Context.it (test/validation.range.test.js:60:37)

Start by turning on the range component in the GUI again. Also unskip tests that only work properly when integrated due to the nature of the test API we have built.

Fixes warning with 'none' dropdown state: /Users/pzrq/Projects/compass/node_modules/fbjs/lib/warning.js:36 Warning: Failed prop type: Required prop `value` was not specified in `RangeInput`. in RangeInput (created by RuleCategoryRange) in RuleCategoryRange (created by Rule) in form (created by Form) in Form (created by Rule) in td (created by Rule) in tr (created by Rule) in Rule (created by RuleBuilder) in tbody (created by RuleBuilder) in table (created by Table) in Table (created by RuleBuilder) in div (created by Col) in Col (created by RuleBuilder) in div (created by Row) in Row (created by RuleBuilder) in div (created by Grid) in Grid (created by Editable) in div (created by Editable) in div (created by Editable) in div (created by Editable) in Editable (created by RuleBuilder) in RuleBuilder (created by Validation) in div (created by Grid) in Grid (created by Validation) in div (created by Validation) in Validation (created by ConnectedValidation) in StoreConnector (created by ConnectedValidation) in ConnectedValidationprintWarning @ /Users/pzrq/Projects/compass/node_modules/fbjs/lib/warning.js:36

https://facebook.github.io/react/docs/react-component.html#componentwillmount

parseFloat does not take a second argument, unlike parseInt :)

It can't possibly work with the combined range input unless you added multiple IDs somehow which would be a much bigger component refactoring.

So it appears to start in an invalid state if switching from a 'none' state.

It seems to confuse the component, leading to a bizarre cache issue. The best kind of bug fix - when deleting code makes things work better :)

react-bootstrap/react-bootstrap#1926

So I can reuse it shortly.

So we can put the common user error of mixing up the lower and upper bounds into the category of being reported by our Compass GUI, but still allow the user to proceed if they choose to.

parseFloat will truncate them later, but the user doesn't see it unless they switch to JSON view or reload the page...this is at least a little better in that they are warned, not sure what the behaviour should really be though. Not sure how to unit test this one as it's deep in the validation logic so it would be somewhere deep in Enzyme if it is testable.

rueckstiess · 2016-11-02T13:29:36Z

I think we got the form validation correct now. Here's a video.

Basic idea:

Inputs are not invalid initially
Once the user focuses an input field and then blurs it, we validate it and it may become invalid
When a field becomes invalid, it communicates this up the chain to its parent, which may then decide to also be invalid, etc.
Before final submit, we also run a validation pass down the chain to each component, to make sure that no fields are invalid

Just needed to drop the comboValidationState.

pzrq · 2016-11-03T00:56:56Z

@rueckstiess CANCEL button is now broken, looking into it now

pzrq · 2016-11-03T01:21:54Z

rueckstiess · 2016-11-06T14:00:20Z

@pzrq could you do another round of tests and try to break this latest version?

It should be pretty solid now, forms only get validated after the user focused and blurred a field.

pzrq · 2016-11-07T01:53:40Z

@rueckstiess Apart from CI tests and Decimal128 support regressing (and 0.1+0.2 but that's an edge case), this lgtm, i.e. the validation at least is working correctly now. But yeah overall still needs work.

pzrq · 2016-11-08T04:50:13Z

After discussion, we've decided to drop Decimal128 in the range view until users request it, but support it in the JSON view, see https://jira.mongodb.org/browse/COMPASS-294

That just leaves updating the TravisCI tests.

pzrq · 2016-11-08T05:32:16Z

👍 Woot, finally done.

* Add JSDoc of proposed coupling between RangeInput validationStates * COMPASS 235: DV - Full range component Start by turning on the range component in the GUI again. Also unskip tests that only work properly when integrated due to the nature of the test API we have built. * Add RuleCategoryRange unit and Rule integration tests * Add failing test for the empty range state * Update RangeInput.value to no longer be isRequired Fixes warning with 'none' dropdown state: /Users/pzrq/Projects/compass/node_modules/fbjs/lib/warning.js:36 Warning: Failed prop type: Required prop `value` was not specified in `RangeInput`. in RangeInput (created by RuleCategoryRange) in RuleCategoryRange (created by Rule) in form (created by Form) in Form (created by Rule) in td (created by Rule) in tr (created by Rule) in Rule (created by RuleBuilder) in tbody (created by RuleBuilder) in table (created by Table) in Table (created by RuleBuilder) in div (created by Col) in Col (created by RuleBuilder) in div (created by Row) in Row (created by RuleBuilder) in div (created by Grid) in Grid (created by Editable) in div (created by Editable) in div (created by Editable) in div (created by Editable) in Editable (created by RuleBuilder) in RuleBuilder (created by Validation) in div (created by Grid) in Grid (created by Validation) in div (created by Validation) in Validation (created by ConnectedValidation) in StoreConnector (created by ConnectedValidation) in ConnectedValidationprintWarning @ /Users/pzrq/Projects/compass/node_modules/fbjs/lib/warning.js:36 * Mostly working, but for some reason renders the stale value... * Refactor componentWillMount into the constructor https://facebook.github.io/react/docs/react-component.html#componentwillmount * Refactor onRangeInputBlur to the end of validate * Remove 10 from parseFloat parseFloat does not take a second argument, unlike parseInt :) * Drop unused RangeInput.onChange prop It can't possibly work with the combined range input unless you added multiple IDs somehow which would be a much bigger component refactoring. * Be clearer which store * Call validate after changing the dropdownOp value So it appears to start in an invalid state if switching from a 'none' state. * Remove componentWillReceiveProps It seems to confuse the component, leading to a bizarre cache issue. The best kind of bug fix - when deleting code makes things work better :) * ESlint * Fix race where validationState is not allowed to be empty string react-bootstrap/react-bootstrap#1926 * Finally, playing with the (unvalidated) component works correctly * Refactor into validateCombinedParams So I can reuse it shortly. * Finally get comboValidationState working So we can put the common user error of mixing up the lower and upper bounds into the category of being reported by our Compass GUI, but still allow the user to proceed if they choose to. * Add a regex to warn the user they cannot enter expressions like 10+5 parseFloat will truncate them later, but the user doesn't see it unless they switch to JSON view or reload the page...this is at least a little better in that they are warned, not sure what the behaviour should really be though. Not sure how to unit test this one as it's deep in the validation logic so it would be somewhere deep in Enzyme if it is testable. * Handle and test the 0 case correctly Thankfully the server already did, good thing I tried to record a demo video and check: > db.runCommand({collMod: 'validation', validator: {"age": {$gt:0, $lt:500}}}); { "ok" : 1 } > db.getCollectionInfos()[1].options.validator { "age" : { "$lt" : 500, "$gt" : 0 } } # Server also supports null which we don't because it gets mangled between my code and Thomas' code so meh until someone cares for it > db.runCommand({collMod: 'validation', validator: {"age": {$gt:null, $lt:500}}}); { "ok" : 1 } > db.getCollectionInfos()[1].options.validator { "age" : { "$gt" : null, "$lt" : 500 } } * Improve comments This particular state is no longer rejected by the Rule Builder GUI, just shown highlighted in red as something we highly doubt a user would intentionally create, for the reasons outlined in `npm run ci` like the range being empty or easily hard-code-able as an application-level constant. * Add better regex to handle exponential float forms * Fix translating regexes fail (the |, vertical bar ASCII code 0x7c should not be accepted) Though incoming hadron-type-checker should resolve this more completely (its DECIMAL_128_REGEX still accepts arbitrarily large exponents which it should not, but it's better than this one was). * Add some Decimal128 tests * Handle Decimal128 and drop clunky parseFloat uses * Be more explicit that combined ranges of Decimal128s are not validated * shrink range inputs, remove title labels. * 👕 fix linter issue. * don’t validate initially, fix tests. * form validation. such difficult. * [wip] up and downwards validation almost working * fixing some edge cases, updating tests. * Restore tests so accepts/rejects is clearer Just needed to drop the comboValidationState. * make rule id consistent (based on index of rule). * store cancel always needs to call _updateState() * range: disable bson type casting * cancelChanges no longer takes a boolean. * validation rewritten. only use single validate() method * use componentWillReceiveProps where props are forked. * force a redraw when cancel was pressed via key change * 👕 fixing eslint issues. * Drop HP_VERSION until COMPASS-294 * 💚 fix tests.

pzrq added the work in progress label Oct 31, 2016

pzrq force-pushed the COMPASS-235-params-to-query-etc branch 2 times, most recently from 081125b to 602a61a Compare November 1, 2016 01:07

pzrq added ready for review and removed work in progress labels Nov 1, 2016

imlucas self-assigned this Nov 1, 2016

imlucas added the work in progress label Nov 1, 2016

imlucas removed their assignment Nov 1, 2016

pzrq added 19 commits November 2, 2016 11:42

COMPASS 235: DV - Full range component

3d6c120

Start by turning on the range component in the GUI again. Also unskip tests that only work properly when integrated due to the nature of the test API we have built.

Add JSDoc of proposed coupling between RangeInput validationStates

f888ae6

Add RuleCategoryRange unit and Rule integration tests

1169102

Add failing test for the empty range state

c97bfa9

Mostly working, but for some reason renders the stale value...

0918163

Refactor componentWillMount into the constructor

4a96c33

https://facebook.github.io/react/docs/react-component.html#componentwillmount

Refactor onRangeInputBlur to the end of validate

3e1e8dc

Remove 10 from parseFloat

311ccf7

parseFloat does not take a second argument, unlike parseInt :)

Drop unused RangeInput.onChange prop

f14e53e

It can't possibly work with the combined range input unless you added multiple IDs somehow which would be a much bigger component refactoring.

Be clearer which store

d5efc05

Call validate after changing the dropdownOp value

fb90307

So it appears to start in an invalid state if switching from a 'none' state.

Remove componentWillReceiveProps

54d1a75

It seems to confuse the component, leading to a bizarre cache issue. The best kind of bug fix - when deleting code makes things work better :)

ESlint

53d73be

Fix race where validationState is not allowed to be empty string

3fc8115

react-bootstrap/react-bootstrap#1926

Finally, playing with the (unvalidated) component works correctly

6a306a2

Refactor into validateCombinedParams

67b9a81

So I can reuse it shortly.

Finally get comboValidationState working

e1f15b2

So we can put the common user error of mixing up the lower and upper bounds into the category of being reported by our Compass GUI, but still allow the user to proceed if they choose to.

Thomas Rueckstiess added 3 commits November 2, 2016 11:42

shrink range inputs, remove title labels.

f19bf05

👕 fix linter issue.

f038bf1

don’t validate initially, fix tests.

e6453c3

rueckstiess force-pushed the COMPASS-235-params-to-query-etc branch from f5dafa2 to e6453c3 Compare November 2, 2016 00:43

Thomas Rueckstiess added 3 commits November 2, 2016 17:09

form validation. such difficult.

c908d79

[wip] up and downwards validation almost working

a301d45

fixing some edge cases, updating tests.

a50f8f9

Restore tests so accepts/rejects is clearer

400d382

Just needed to drop the comboValidationState.

make rule id consistent (based on index of rule).

0d1054e

pzrq removed the ready for review label Nov 3, 2016

Thomas Rueckstiess added 7 commits November 3, 2016 22:41

store cancel always needs to call _updateState()

978538f

range: disable bson type casting

2a91175

cancelChanges no longer takes a boolean.

8214d33

validation rewritten. only use single validate() method

6f5e7d6

use componentWillReceiveProps where props are forked.

7d9cf64

force a redraw when cancel was pressed via key change

478057a

👕 fixing eslint issues.

20f734c

pzrq and others added 2 commits November 8, 2016 16:04

Drop HP_VERSION until COMPASS-294

106d496

💚 fix tests.

f9eb65b

rueckstiess merged commit 37d7c06 into master Nov 8, 2016

rueckstiess deleted the COMPASS-235-params-to-query-etc branch November 8, 2016 05:14

pzrq removed the work in progress label Nov 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

COMPASS 235 DV: Full range component #551

COMPASS 235 DV: Full range component #551

Uh oh!

pzrq commented Oct 31, 2016

Uh oh!

pzrq commented Oct 31, 2016

Uh oh!

rueckstiess commented Nov 1, 2016

Uh oh!

imlucas commented Nov 1, 2016

Uh oh!

rueckstiess commented Nov 2, 2016

Uh oh!

pzrq commented Nov 3, 2016

Uh oh!

pzrq commented Nov 3, 2016

Uh oh!

rueckstiess commented Nov 6, 2016

Uh oh!

pzrq commented Nov 7, 2016 •

edited

Loading

Uh oh!

pzrq commented Nov 8, 2016

Uh oh!

pzrq commented Nov 8, 2016

Uh oh!

Uh oh!

COMPASS 235 DV: Full range component #551

COMPASS 235 DV: Full range component #551

Uh oh!

Conversation

pzrq commented Oct 31, 2016

Uh oh!

pzrq commented Oct 31, 2016

Uh oh!

rueckstiess commented Nov 1, 2016

Uh oh!

imlucas commented Nov 1, 2016

Uh oh!

rueckstiess commented Nov 2, 2016

Uh oh!

pzrq commented Nov 3, 2016

Uh oh!

pzrq commented Nov 3, 2016

Uh oh!

rueckstiess commented Nov 6, 2016

Uh oh!

pzrq commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pzrq commented Nov 8, 2016

Uh oh!

pzrq commented Nov 8, 2016

Uh oh!

Uh oh!

pzrq commented Nov 7, 2016 •

edited

Loading