Skip to content

Fix: don't offer the contains conditions for string values#2042

Open
stevecassidy wants to merge 3 commits intomainfrom
fix/contains-condition-clarification
Open

Fix: don't offer the contains conditions for string values#2042
stevecassidy wants to merge 3 commits intomainfrom
fix/contains-condition-clarification

Conversation

@stevecassidy
Copy link
Copy Markdown
Contributor

Fix: don't offer the contains conditions for string values

Ticket

#1872

Description

There is some inconsistency with the definitions of 'contains' and 'does-not-contain' conditions
which are defined to operate on list values. In the condition editor, these are only offered for
use with non-list values.

In addition, the operators offered for other value types were not correct.

Proposed Changes

Effectively deprecates 'contains' and 'does-not-contain' by not offering them in the editor. They
are not useful for multi-select since we have 'contains-one-of' and 'contains all of' instead which make
more sense since you can select more than one option for 'value' in this case.

Refactor the selection of operators to getAllowedOperatorsForField and add options for number/date fields.

Adds new operators string-contains and string-does-not-contain for string valued fields and offer these
in the editor.

Instead of displaying the raw operator name, use the existing translations in allOperators to create a nicer
select drop-down.

How to Test

In the designer, add a condition to a field and see the updated operator menus for different field types.

Additional Information

The translations in allOperators should really go in the operator definitions themselves.

A better way of knowing what type a value is would be really useful here!

Checklist

  • I have confirmed all commits have been signed.
  • I have added JSDoc style comments to any new functions or classes.
  • Relevant documentation such as READMEs, guides, and class comments are updated.

Signed-off-by: Steve Cassidy <steve@fieldnote.au>
…g-contains instead

Signed-off-by: Steve Cassidy <steve@fieldnote.au>
…lect

Signed-off-by: Steve Cassidy <steve@fieldnote.au>
Copy link
Copy Markdown
Contributor

@PeterBaker0 PeterBaker0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving - as my only comment pertains to a likely existing bug/risk - just wanting you to critically assess the runtime safety of the current compile functions since we never assert anything about the data being of the string/array type before calling 'includes' (that I can see via quick inspection)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be an existing issue - but RecordValues type is just Record<string, any> - right? So can we assume that the value has the includes method - what if you craft a condition over a non string data type such as a JSON - wouldn't this cause a runtime crash?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants