Type-aware where #109

jgellin-sf · 2020-12-22T18:02:50Z

What does this PR do?

This PR:

creates a utility to get a field type from SObject metadata
creates a validation framework for where input using a factory pattern
implements validation for boolean, currency, date, integer, floating point, and pickist types
implements normalization (quoting and escaping of string values
implements special case normalization for null input
shows an error message when validation of where input fails

This PR does not:

show an error message in a beautiful elegant manner
change the HTML input based on type

What issues does this PR fix or reference?

@W-8555377@

codecov · 2020-12-22T18:02:56Z

Codecov Report

Merging #109 (1723daf) into feature/where (2ebc1df) will decrease coverage by 0.39%.
The diff coverage is 92.22%.

@@                Coverage Diff                @@
##           feature/where     #109      +/-   ##
=================================================
- Coverage          94.85%   94.46%   -0.40%     
=================================================
  Files                 49       58       +9     
  Lines               1595     1878     +283     
  Branches             344      406      +62     
=================================================
+ Hits                1513     1774     +261     
- Misses                79      101      +22     
  Partials               3        3

Flag	Coverage Δ
language-server	`97.46% <ø> (ø)`
soql-builder-ui	`92.31% <83.06%> (-1.86%)`	⬇️
soql-model	`95.39% <98.83%> (+0.80%)`	⬆️

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

Impacted Files	Coverage Δ
...builder-ui/src/modules/querybuilder/where/where.ts	`100.00% <ø> (ø)`
...rybuilder/whereModifierGroup/whereModifierGroup.ts	`83.96% <75.29%> (-14.34%)`	⬇️
packages/soql-model/src/validators/validator.ts	`93.33% <93.33%> (ø)`
...oql-builder-ui/src/modules/querybuilder/app/app.ts	`87.85% <100.00%> (+0.11%)`	⬆️
.../src/modules/querybuilder/services/sobjectUtils.ts	`100.00% <100.00%> (ø)`
...-ui/src/modules/querybuilder/services/soqlUtils.ts	`95.95% <100.00%> (+0.72%)`	⬆️
packages/soql-model/src/messages/messages.ts	`100.00% <100.00%> (ø)`
packages/soql-model/src/model/model.ts	`100.00% <100.00%> (ø)`
...ages/soql-model/src/validators/booleanValidator.ts	`100.00% <100.00%> (ø)`
...ges/soql-model/src/validators/currencyValidator.ts	`100.00% <100.00%> (ø)`
... and 14 more

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 2ebc1df...1723daf. Read the comment docs.

dehru · 2020-12-22T18:47:31Z

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

+    });
+  }
+
+  get buttonData() {


This would be best to do in the @api. so add get/set to the buttonLabels and do it in the setter. Then just render buttonData. It's best to keep as much looping as possible outside of the render loop of LWC, since we don't know how often it attempts to re-render itself.

You could also apply the class names in a function called from the template.
class={getClassName(idx)}
This would allow the button to dynamically restyle whenever the selectedIndex is changed. Altering the selected index will trigger a re-render and calculate the new styles...the most important being which is selected.

Ok I put the className in the buttonData so everything needed to render the template is in that object. Then on selection I update the style. Since the buttonData is tracked LWC knows to re-render.

dehru · 2020-12-22T19:11:30Z

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

+  @api selectedIndex = '-1';
+
+  renderedCallback() {
+    const buttonElements = this.template.querySelectorAll('button');


Opposite of my last comment, this could be best to do in the render loop from html. You can call a function from the template with class={applyStyle(index)} in the template.

Would love to do this, but you can't have a call with arguments in the template.

Sorta kinda do this now with keeping the classlist in the buttondata.

Yeah. the limitations of LWC often catch me as a surprise coming from Angular / React's much richer template syntax.

dehru · 2020-12-22T19:25:15Z

packages/soql-builder-ui/src/modules/querybuilder/buttonGroup/buttonGroup.css

+  align-self: flex-start;
+}
+.btn {
+  height: 1.5em;


I generally like to use rem vs em personally. Because em is dynamic as compared to it's parent. So that can be funky as text up the dom possibly gets set to "larger" or "smaller", it affects what an em is on the button. Where as rem is based on the root font size, which is just easier to think about logically for me.

Normally, it's not a big issue, but i wanted to mention it from my experience.

I like that better. I made the change.

dehru · 2020-12-22T19:26:31Z

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

+    buttonGroup.addEventListener('selection__changed', handler);
+    document.body.appendChild(buttonGroup);
+
+    const buttonElements = buttonGroup.shadowRoot.querySelectorAll('button');


I think you can just do
const button0 = buttonGroup.shadowRoot.querySelectorAll('button')[0];

Thanks. Done.

dehru · 2020-12-22T19:29:35Z

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

+  }
+
+  @api get selection() {
+    return parseInt(this.selectedIndex);


How do you see this being used vs the event? I noticed you are using it in testing, however, outside of testing, i think the event will likely drive the app.

I agree. It's gone now.

ekapner · 2021-01-13T16:20:53Z

packages/soql-model/src/messages/messages.ts

  export const error_incompleteFrom = 'Incomplete FROM clause. The FROM clause requires an object.';
  export const error_incompleteLimit = 'Incomplete LIMIT clause. The LIMIT keyword must be followed by a number.';
+
+  export const error_fieldInput_boolean = 'Value must be TRUE or FALSE';


Does case matter here? Must be all caps?

Case does not actually matter, but all-caps is consistent with the way the documentation (https://developer.salesforce.com/docs/atlas.en-us.soql_sosl.meta/soql_sosl/sforce_api_calls_soql.htm) writes elements of SOQL syntax.

ekapner · 2021-01-13T16:21:40Z

packages/soql-model/src/messages/messages.ts

  export const error_incompleteLimit = 'Incomplete LIMIT clause. The LIMIT keyword must be followed by a number.';
+
+  export const error_fieldInput_boolean = 'Value must be TRUE or FALSE';
+  export const error_fieldInput_currency = 'Currency value is not valid';


Any guidelines we can add here?

There's a 3 letter currency code followed by the numeric value according to the ISO standard. So $100 US is USD100. I think the currency value can also be just a floating point number. So the most concise we can get is maybe...

"Value must be a number or a three-letter ISO currency code followed by a number"

I feel like including the three-letter ISO currency code makes this feel more complicated than necessary.

They are all a bit complicated - so let's just leave them as @jgellin-sf wrote them. Thanks for answering my questions in such detail.

ekapner · 2021-01-13T16:22:15Z

packages/soql-model/src/messages/messages.ts

+
+  export const error_fieldInput_boolean = 'Value must be TRUE or FALSE';
+  export const error_fieldInput_currency = 'Currency value is not valid';
+  export const error_fieldInput_date = 'Date value is not valid';


Can we provide valid date format or some other guidance? like: You must enter the date as YYYY-MM-DD (or something like that)

Date is tricky. Because you can write date as:

Date only: 2021-01-13

UTC date and time: 2021-01-13T12:00:00Z

Date and time with timezone offset: 2021-01-13T12:00:00-05:00

Any of the many range literals built into SOQL (TODAY, TOMORROW, NEXT_WEEK, LAST_YEAR, etc)

Any of the many parameterized date range literals build into SOQL (NEXT_N_DAYS:5, LAST_N_YEARS:20, etc.)
See https://developer.salesforce.com/docs/atlas.en-us.soql_sosl.meta/soql_sosl/sforce_api_calls_soql_select_dateformats.htm?search_text=currency

ekapner · 2021-01-13T16:23:21Z

packages/soql-model/src/messages/messages.ts

+  export const error_fieldInput_boolean = 'Value must be TRUE or FALSE';
+  export const error_fieldInput_currency = 'Currency value is not valid';
+  export const error_fieldInput_date = 'Date value is not valid';
+  export const error_fieldInput_float = 'Value must be numeric';


Does this mean "numbers only"? decimals?

Right. So numeric values, containing decimal or not, are fine.

packages/soql-model/src/messages/messages.ts

ekapner

A couple questions before I can finalize the validation messages

contraction Co-authored-by: Emily Kapner <ekapner@salesforce.com>

…ng into jg/typeAwareWhere

dehru · 2021-01-13T23:17:59Z

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

  modelService: ToolingModelService;
  messageService: IMessageService;
  theme = 'light';
+  _sobjectTypeUtils: SObjectTypeUtils;


Is there a reason to use a getter/setter here?

I don't think it's really necessary.

This is irrelevant following the pass sobject metadata rather than utils object commit

dehru · 2021-01-13T23:28:59Z

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

+    document.body.appendChild(modifierGroup);
+
+    const { criteriaInputEl } = getModifierElements();
+    criteriaInputEl.value = 'FALSE';


would this test pass if criteriaInputEl.value was lower case 'false'?

dehru · 2021-01-13T23:37:58Z

packages/soql-model/src/validators/integerValidator.ts

+
+export class IntegerValidator extends Validator {
+  public validate(input: string): ValidateResult {
+    const isValid = /^[+-]?[0-9]+$/.test(input.trim());


There's also Number.isInteger() or parseInt(). But i don't know if they are more inclusive than you want.

Those will return a integer for any string starting with an integer, so parseInt() will return 134 for " 134hello".

yeah, i guess that's a false positive.

dehru · 2021-01-14T00:03:27Z

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

+        type,
+        picklistValues
+      };
+


I notice we are checking 2 validations here, but only displaying 1 error message. In your experience with this, is it best to check the input validation for the field first, and if that passes, show check operator validation?

I don't have any special insight to the order of the validations. It felt right. 😂

dehru · 2021-01-14T00:06:41Z

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

  @api selectedOperator: string;
  @api isLoading = false;
  @api index;
+  @api sobjectTypeUtils: SObjectTypeUtils;


I'm thinking we should pass the metadata and then process it here instead of with app.ts. using a setter to process the data with sobjectutils

Made this change in pass sobject metadata rather than utils object commit

dehru · 2021-01-14T00:09:16Z

packages/soql-builder-ui/src/modules/querybuilder/services/sobjectUtils.ts

+import { Soql } from '@salesforce/soql-model';
+
+export class SObjectTypeUtils {
+  constructor(protected sobjectMetadata: any) {


To limit the looping/filtering in this utility, we could use a map so that the key is the field name ( possibly lowercased ). Something like this:

{ 'id' : { name: 'Id', type: 'id', picklistValues: [] }, 'name' : { name: 'Name', type: 'string', picklistValues: [] }, 'accountsource': { name: 'AccountSource', type: 'picklist', picklistValues: [{ value: 'apple' }, { value: 'banana' }, { value: 'cherry' }] }, 'annualrevenue': { name: 'AnnualRevenue', type: 'currency', picklistValues: [] }, 'billingaddress' : { name: 'BillingAddress', type: 'address', picklistValues: [] }, ... }

I updated this code in the Refactor sobject utils for easier map lookups commit.

dehru · 2021-01-14T00:13:56Z

This is amazingly cool work JG!

dehru

None of my comments are change requests. Just possible enhancements and/or thinking out loud. Address any you see fit.

We could even merge this so that I can attempt to beautify the message while you address any of the comments.

…ql-tooling into jg/typeAwareWhere

* 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)

* Utility to return field type from SObject metadata * type-aware input validation * boolean input initialization * integer and float input validation * Validation for picklists and dates * validation of currency literals

* 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>

generic button group

efa5313

Utility to return field type from SObject metadata

399e4cd

dehru reviewed Dec 22, 2020

View reviewed changes

Apply buttongroup style in data, not render

97f912c

dehru reviewed Dec 22, 2020

View reviewed changes

jgellin-sf added 13 commits December 22, 2020 14:33

refactor button group for minimal render looping

2d5b752

remove selection getter, em=>rem

0e7285b

separate label from value

8055c6a

fix buttongroup test

dbc9180

type-aware input fields

18be4df

type-aware input validation

47de468

boolean input initialization

ffa5f89

temporary test code

c1ab4fd

integer and float input validation

8c4bec5

Validation for picklists and dates

2fb0ba4

validation of currency literals

15cc689

Fix case statement fallthrough bug

723f84b

refactor validation

d5fd60c

jgellin-sf marked this pull request as ready for review January 13, 2021 14:52

jgellin-sf requested review from jonnyhork, mysticflute and praksb as code owners January 13, 2021 14:52

jgellin-sf requested a review from dehru January 13, 2021 14:52

ekapner reviewed Jan 13, 2021

View reviewed changes

jgellin-sf requested a review from ekapner January 13, 2021 16:21

ekapner reviewed Jan 13, 2021

View reviewed changes

packages/soql-model/src/messages/messages.ts Outdated Show resolved Hide resolved

ekapner reviewed Jan 13, 2021

View reviewed changes

jgellin-sf and others added 5 commits January 13, 2021 13:13

Update packages/soql-model/src/messages/messages.ts

863f742

contraction Co-authored-by: Emily Kapner <ekapner@salesforce.com>

remove unused code

88f15c1

Merge branch 'jg/typeAwareWhere' of github.com:forcedotcom/soql-tooli…

e0af62b

…ng into jg/typeAwareWhere

remove unused UI

aaa9405

changes in response to comments

68a6633

dehru reviewed Jan 13, 2021

View reviewed changes

dehru reviewed Jan 14, 2021

View reviewed changes

dehru self-requested a review January 14, 2021 00:16

dehru approved these changes Jan 14, 2021

View reviewed changes

jgellin-sf and others added 6 commits January 14, 2021 10:44

Refactor sobject utils for easier map lookups

7c1bc6d

pass sobject metadata rather than utils object

3196f3b

fix test failure

633f685

Include address in sobject field type table

8edb863

Merge branch 'feature/where' into jg/typeAwareWhere

15fefc4

Merge branch 'jg/typeAwareWhere' of https://github.com/forcedotcom/so…

1723daf

…ql-tooling into jg/typeAwareWhere

dehru merged commit 8602b7d into feature/where Jan 15, 2021

dehru deleted the jg/typeAwareWhere branch January 15, 2021 23:02

Type-aware where #109

Type-aware where #109

Uh oh!

Conversation

jgellin-sf commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

What issues does this PR fix or reference?

Uh oh!

codecov bot commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dehru Dec 22, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekapner Jan 13, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ekapner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jgellin-sf commented Dec 22, 2020 •

edited

Loading

codecov bot commented Dec 22, 2020 •

edited

Loading

dehru Dec 22, 2020 •

edited

Loading

ekapner Jan 13, 2021 •

edited

Loading

dehru Jan 14, 2021 •

edited

Loading