Jhork/clear modifier field bug fix #168

jonnyhork · 2021-02-04T00:32:12Z

What does this PR do?

This PR will address a bug where the field of the last modifier group would not be cleared because the customSelect component would not update the _value with an empty array coming from the model.

I have also refactored a skipped test to work with the customSelect.

What issues does this PR fix or reference?

@W-8847700@

codecov · 2021-02-04T00:32:18Z

Codecov Report

Merging #168 (cb753a2) into develop (64d07e9) will increase coverage by 0.19%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #168      +/-   ##
===========================================
+ Coverage    94.44%   94.64%   +0.19%     
===========================================
  Files           64       64              
  Lines         2519     2519              
  Branches       647      646       -1     
===========================================
+ Hits          2379     2384       +5     
+ Misses         138      133       -5     
  Partials         2        2

Flag	Coverage Δ
language-server	`98.32% <ø> (ø)`
soql-builder-ui	`91.25% <100.00%> (+0.49%)`	⬆️
soql-model	`96.39% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../modules/querybuilder/customSelect/customSelect.ts	`91.95% <100.00%> (-0.08%)`	⬇️
...rybuilder/whereModifierGroup/whereModifierGroup.ts	`89.89% <100.00%> (+2.65%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64d07e9...cb753a2. Read the comment docs.

jgellin-sf

👍

jgellin-sf · 2021-02-04T15:20:13Z

I attempted to QA and there is a problem. The field value is tied to the condition model. When there is a validation problem the component is re-rendered to show the error, but the condition is not set. So on re-render the field is read from the condition (knock knock; nobody home) and the field input is reset. Here's a video:

Screen.Recording.2021-02-04.at.10.14.42.AM.mov

I think we need to decouple the custom select display value for the field from the condition model. I'll give that a whorl this morning.

jgellin-sf · 2021-02-04T15:36:57Z

I fixed the issue I described with the De-couple display field from condition model commit. However there is still another problem. If there is an incomplete condition (all modifiers do not have value OR validation fails so the condition is set), the inputs are not cleared.

Screen.Recording.2021-02-04.at.10.36.03.AM.mov

I think we need to explicitly clear the display values on X click. Back to work.

jgellin-sf · 2021-02-04T15:43:29Z

Ok one more commit Explicitly clear inputs on X click to fix that last issue. I think we need an independent QA on this one.

jonnyhork

How do we feel about testing to cover these sort of cases? I think we have the individual parts, clear modifiers, render errors, etc, each with a happy path. Do we want something that combines these conditions? Reset with and without errors.

I'm not sure if the issues we found here should have been caught by tests?

jonnyhork · 2021-02-04T16:08:52Z

packages/soql-builder-ui/src/modules/querybuilder/whereModifierGroup/whereModifierGroup.ts

-      return field !== this.getFieldName();
-    });
+    return this._currentFieldSelection ? [this._currentFieldSelection] : [];
  }


This is dead code, I forgot to remove this when we switched to the custom select

jonnyhork · 2021-02-04T16:28:32Z

One more thing: If there was no condition in the model, such as when the user is filling out the first modifier group and makes a field selection but then tries to clear it, _currentFieldSelection is redefined but the UI is not updated. I added @track to _currentFieldSelection to cover this.
See the track selected field to re-render if cleared commit

Screen.Recording.2021-02-04.at.9.22.25.AM.mov

dehru · 2021-02-04T17:07:05Z

packages/soql-builder-ui/src/modules/querybuilder/customSelect/customSelect.ts

  set selectedOptions(selectedOptions: string[]) {
    this._selectedOptions = selectedOptions || [];
-    if (selectedOptions && selectedOptions.length) {
+    if (selectedOptions) {


Now that the length is removed, this can be simplified to.

set selectedOptions(selectedOptions: string[]) { this._selectedOptions = this._value = selectedOptions || []; }

cool, fixed in the simplify syntax commit.

jonnyhork · 2021-02-04T17:54:27Z

I don't see any tests that actually click the clear modifier group X and see that it works to clear the values, only send and event. I'll work on that.

jonnyhork · 2021-02-04T21:04:46Z

packages/soql-builder-ui/src/modules/querybuilder/whereModifierGroup/whereModifierGroup.test.ts

  });

-  it.skip('updates inputs when condition model changes', () => {
+  it('clears all the values when the X is clicked', () => {


Do you think it is worth having a similar test but when there is an error state, like in
set error class on invalid operator input

Yes, i'm adding that now. It looks like the error state needs to be cleared as well in this case.

That'll be in a separate PR. QAing this now.

* Jhork: Support for Simple WHERE Expressions (#101) * move rule to css common (#108) * Preventing webview re-rendering when soql statement has not changed (#105) * Revert "Disable WHERE in model (#113)" * Type-aware where (#109) * Fix preview not updating when autosave is off - issue 120 (#125) * Recognize recoverable where errors (#116) * Operator and Criteria Error Tooltips (#122) * Support IN, NOT IN, INCLUDES, EXCLUDE operators (#123) * Add smarter completin on GROUP BY clause and WHERE literals (#126) * Feature/custom select (#148) * Jhork/custom select fields (#111) * Jhork/custom select from (#130) * Jhork/custom select order by (#134) * set the error overlay to be on top of all other form elements (#150) * prepare to publish 0 .0.30 (#151) * Feature/where (#154) * Jhork: Support for Simple WHERE Expressions (#101) * move rule to css common (#108) * Preventing webview re-rendering when soql statement has not changed (#105) * Revert "Disable WHERE in model (#113)" * Type-aware where (#109) * Recognize recoverable where errors (#116) * Operator and Criteria Error Tooltips (#122) * Support IN, NOT IN, INCLUDES, EXCLUDE operators (#123) * Revert "Feature/where (#154)" (#157) * Add support for line comments ( `//` ) at the top of SOQL files (#132) * Jhork/where custom select (#161) * Fix: 'undefined' in SOQL string (#162) * Clear tooltip when object changes, fix empty soql statement error blocker (#165) * Track to force render on condition change - fix rebase conflict (#166) * do not send event when user still adding input' (#169) * Jhork/clear modifier field bug fix (#168) * handle a option selection event (#178) * cler tooltip error when X is clicked (#180) * Jhork/workflow updates (#163) * Bump versions for release (#181) Co-authored-by: Jonny Hork <jhork@salesforce.com> Co-authored-by: dehru <599418+dehru@users.noreply.github.com> Co-authored-by: Dehru Cromer <dehru.cromer@salesforce.com> Co-authored-by: Fernando Dobladez <fernandodobladez@salesforce.com>

jonnyhork added 2 commits February 3, 2021 17:28

remove check for selectedConditions.length

6ca87af

adjust test for custom select component

4429d5c

jonnyhork requested review from dehru, jgellin-sf, mysticflute and praksb as code owners February 4, 2021 00:32

jgellin-sf previously approved these changes Feb 4, 2021

View reviewed changes

De-couple display field from condition model

6531055

jgellin-sf dismissed their stale review via 6531055 February 4, 2021 15:32

Explicitly clear inputs on X click

c7c143a

remove dead code

e1a63bd

jonnyhork commented Feb 4, 2021

View reviewed changes

track selected field to re-render if cleared

786f25f

dehru reviewed Feb 4, 2021

View reviewed changes

simplify syntax

ddf9b1d

check all values are cleared with X click

cb753a2

jonnyhork commented Feb 4, 2021

View reviewed changes

jonnyhork mentioned this pull request Feb 4, 2021

When modifying the Field in the filter clause it switches back silently to the previously picked field when editing the value #175

Closed

dehru self-requested a review February 5, 2021 16:55

dehru approved these changes Feb 5, 2021

View reviewed changes

jonnyhork merged commit cd462d8 into develop Feb 5, 2021

jonnyhork deleted the jhork/clear-modifier-field branch February 5, 2021 16:59

Jhork/clear modifier field bug fix #168

Jhork/clear modifier field bug fix #168

Uh oh!

Conversation

jonnyhork commented Feb 4, 2021

What does this PR do?

What issues does this PR fix or reference?

Uh oh!

codecov bot commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jgellin-sf left a comment

Choose a reason for hiding this comment

Uh oh!

jgellin-sf commented Feb 4, 2021

Uh oh!

jgellin-sf commented Feb 4, 2021

Uh oh!

jgellin-sf commented Feb 4, 2021

Uh oh!

jonnyhork left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonnyhork Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

jonnyhork commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dehru Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

jonnyhork Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

jonnyhork commented Feb 4, 2021

Uh oh!

jonnyhork Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

dehru Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

dehru Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Feb 4, 2021 •

edited

Loading

jonnyhork left a comment •

edited

Loading

jonnyhork commented Feb 4, 2021 •

edited

Loading