fix: LEAP-447: Fix Number tag validation #1649

Gondragos · 2023-12-21T23:37:37Z

PR fulfills these requirements

Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
Tests for the changes have been added/updated (for bug fixes/features)
Docs have been added/updated (for bug fixes/features)
Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

(check all that apply)

Product design
Frontend

Describe the reason for change

The change was necessary due to failures of the number validation in some cases that resulted in the ability to save invalid values.

What does this fix?

This fix introduces value validation for the Number tag, conducts a refactoring of the existing validation code, and implements a per-item mode validation.

Does this change affect performance?

Nope

Does this change affect security?

Nope

What alternative approaches were there?

N/A

What feature flags were used to cover this change?

N/A

Does this PR introduce a breaking change?

(check only one)

Yes, and covered entirely by feature flag(s)
Yes, and covered partially by feature flag(s)
No
Not sure (briefly explain the situation below)
At least it should not.

What level of testing was included in the change?

(check all that apply)

e2e
integration
unit

Which logical domain(s) does this change affect?

Number, DateTime, Validation, Required, perItem

Improved the validation logic in ClassificationBase.js, DateTime.js, and PerRegion.js. Provided distinct validation scenarios for classifications, including per region and per object, and streamlined the date validation in DateTime.js for better precision and simplicity.

The Number model was modified to improve validation logic. 'validateValue' function was added to cover more number validation cases such as minimum, maximum, and step checks. Furthermore, some parameter descriptions were adjusted for better clarity, namely the 'required' parameter definition.

Makes it be able to work in chain with other validations

A new feature flag controlled code block was introduced to enable the validation of per-item tags as part of the Classification Base and Per Item Mixin. If the feature flag FF_LSDV_4583 is active and self.peritem is true, the new `_validatePerItem` method is used to validate each region result.

This commit introduces functional tests for various configurations of the number control tag. It covers setting and loading of values, validation against min/max and steps, perRegion and perItem validation, and proper error display for each case.

codecov-commenter · 2023-12-22T01:15:58Z

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (88ba15c) 68.27% compared to head (0824f4f) 68.36%.

Files	Patch %	Lines
src/tags/control/Number.js	68.42%	12 Missing and 6 partials ⚠️
src/mixins/Required.js	89.74%	2 Missing and 2 partials ⚠️
src/mixins/PerItem.js	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1649      +/-   ##
==========================================
+ Coverage   68.27%   68.36%   +0.08%     
==========================================
  Files         443      443              
  Lines       28690    28735      +45     
  Branches     7633     7642       +9     
==========================================
+ Hits        19589    19644      +55     
+ Misses       7849     7841       -8     
+ Partials     1252     1250       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/mixins/PerItem.js

hlomzik

leaving only linting comments for now, but blocking PR to check the actual validation, change here is pretty big and important

hlomzik · 2023-12-26T17:46:50Z

src/mixins/PerItem.js

@@ -22,6 +22,26 @@ const PerItemMixin = types
    },
  }))
  .actions(self => ({
+    _validatePerItem() {


what's the idea behind underscore here? we use underscores rarely and for purely internal things, and those are called from other models using this mixin.

The idea was to prevent using this method outside of the bundle PerItemMixin <-> ClassificationBase. I'm not sure how to prevent it in better way.

src/tags/control/ClassificationBase.js

src/tags/control/Number.js

hlomzik · 2023-12-26T17:49:49Z

src/tags/control/ClassificationBase.js

+          return self._validatePerObject();
+        }
+      },
+      validateValue() {


comment on underscores: feels especially wrong to see those methods one by one with different styles

It's great 'cause validateValue should be overridden and _validatePerObject should not. It's good if you can feel that something here must to be taken in account.

add a comment that this method is the one to override in inheritors, not validate()

src/tags/control/Number.js

Co-authored-by: yyassi-heartex <[email protected]>

…s/label-studio-frontend into fb-leap-447/number-validation

hlomzik · 2024-01-30T00:43:34Z

src/tags/control/ClassificationBase.js

+          return self._validatePerObject();
+        }
+      },
+      validateValue() {


add a comment that this method is the one to override in inheritors, not validate()

Gondragos · 2024-01-30T15:25:49Z

/git merge master

Workflow run
Error: failed to execute "merge" command

Signed-off-by: Sergey <[email protected]>

# Conflicts: # tests/functional/package.json # tests/functional/yarn.lock

Signed-off-by: Sergey <[email protected]>

src/mixins/Required.js

Signed-off-by: Sergey <[email protected]>

Co-authored-by: hlomzik <[email protected]>

* refactor: LEAP-447: Refactor validation method across control tags Improved the validation logic in ClassificationBase.js, DateTime.js, and PerRegion.js. Provided distinct validation scenarios for classifications, including per region and per object, and streamlined the date validation in DateTime.js for better precision and simplicity. * fix: LEAP-447: Improve Number tag validation The Number model was modified to improve validation logic. 'validateValue' function was added to cover more number validation cases such as minimum, maximum, and step checks. Furthermore, some parameter descriptions were adjusted for better clarity, namely the 'required' parameter definition. * Improve validation method of Required mixin Makes it be able to work in chain with other validations * Add validation for per-item tags A new feature flag controlled code block was introduced to enable the validation of per-item tags as part of the Classification Base and Per Item Mixin. If the feature flag FF_LSDV_4583 is active and self.peritem is true, the new `_validatePerItem` method is used to validate each region result. * Update ls-test dependency in functional tests * Add functional tests for number control tag This commit introduces functional tests for various configurations of the number control tag. It covers setting and loading of values, validation against min/max and steps, perRegion and perItem validation, and proper error display for each case. * Fix linting issues * Fix the accidental deletion of a line of code * Refactor with simplifying Co-authored-by: yyassi-heartex <[email protected]> * Fix linting problems and texts * Update @heartexlabs/ls-test version Signed-off-by: Sergey <[email protected]> * Add comments about new methods and tricky changes Signed-off-by: Sergey <[email protected]> * Make validation tests more intuitive Signed-off-by: Sergey <[email protected]> * Remove unused import in number.cy Signed-off-by: Sergey <[email protected]> * Fix comments for validation methods Signed-off-by: Sergey <[email protected]> * Update @heartexlabs/ls-test package version Signed-off-by: Sergey <[email protected]> --------- Signed-off-by: Sergey <[email protected]> Co-authored-by: yyassi-heartex <[email protected]>

Gondragos added 6 commits December 21, 2023 21:53

Improve validation method of Required mixin

b81a02c

Makes it be able to work in chain with other validations

Update ls-test dependency in functional tests

6a4572a

github-actions bot added the fix label Dec 21, 2023

Gondragos mentioned this pull request Dec 21, 2023

feat: LEAP-447: Add helper methods for Modals, Number, ToolBar HumanSignal/ls-frontend-test#24

Merged

This was referenced Dec 21, 2023

fix: LEAP-447: Fix Number tag validation HumanSignal/label-studio#5220

Merged

fix: LEAP-447: Fix Number tag validation monorepo HumanSignal/label-studio#5221

Closed

Gondragos added 2 commits December 21, 2023 23:55

Fix linting issues

cfc64ea

Fix the accidental deletion of a line of code

0c489c8

yyassi-heartex reviewed Dec 22, 2023

View reviewed changes

src/mixins/PerItem.js Outdated Show resolved Hide resolved

hlomzik suggested changes Dec 26, 2023

View reviewed changes

hlomzik reviewed Dec 26, 2023

View reviewed changes

src/tags/control/Number.js Outdated Show resolved Hide resolved

src/tags/control/Number.js Show resolved Hide resolved

src/tags/control/Number.js Show resolved Hide resolved

Gondragos and others added 3 commits December 27, 2023 06:23

Refactor with simplifying

0824f4f

Co-authored-by: yyassi-heartex <[email protected]>

Fix linting problems and texts

d0e153c

Merge branch 'fb-leap-447/number-validation' of github.com:heartexlab…

91d50e3

…s/label-studio-frontend into fb-leap-447/number-validation

hlomzik approved these changes Jan 30, 2024

View reviewed changes

yyassi-heartex approved these changes Jan 30, 2024

View reviewed changes

robot-ci-heartex added the Confirmed label Jan 30, 2024

Gondragos added 6 commits January 30, 2024 15:32

Update @heartexlabs/ls-test version

9c711bc

Signed-off-by: Sergey <[email protected]>

Add comments about new methods and tricky changes

5bc493b

Signed-off-by: Sergey <[email protected]>

Make validation tests more intuitive

0c8f7bf

Signed-off-by: Sergey <[email protected]>

Remove unused import in number.cy

62d118c

Signed-off-by: Sergey <[email protected]>

Merge branch 'master' into fb-leap-447/number-validation

9b2a86a

# Conflicts: # tests/functional/package.json # tests/functional/yarn.lock

Fix comments for validation methods

e30decb

Signed-off-by: Sergey <[email protected]>

hlomzik approved these changes Jan 30, 2024

View reviewed changes

src/mixins/Required.js Outdated Show resolved Hide resolved

Update @heartexlabs/ls-test package version

2e7954e

Signed-off-by: Sergey <[email protected]>

Gondragos enabled auto-merge (squash) January 30, 2024 23:17

Update src/mixins/Required.js

f986d88

Co-authored-by: hlomzik <[email protected]>

Gondragos merged commit 524aa7d into master Jan 30, 2024
11 of 12 checks passed

Gondragos deleted the fb-leap-447/number-validation branch January 30, 2024 23:47

hlomzik mentioned this pull request Feb 22, 2024

fix: LEAP-724: Fix Textarea required + skipDuplicates #1705

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: LEAP-447: Fix Number tag validation #1649

fix: LEAP-447: Fix Number tag validation #1649

Gondragos commented Dec 21, 2023

codecov-commenter commented Dec 22, 2023 •

edited

Loading

hlomzik left a comment

hlomzik Dec 26, 2023

Gondragos Dec 27, 2023

hlomzik Dec 26, 2023

Gondragos Dec 27, 2023

hlomzik Jan 30, 2024

hlomzik Jan 30, 2024

Gondragos commented Jan 30, 2024 •

edited by robot-ci-heartex

Loading

fix: LEAP-447: Fix Number tag validation #1649

fix: LEAP-447: Fix Number tag validation #1649

Conversation

Gondragos commented Dec 21, 2023

PR fulfills these requirements

Change has impacts in these area(s)

Describe the reason for change

What does this fix?

Does this change affect performance?

Does this change affect security?

What alternative approaches were there?

What feature flags were used to cover this change?

Does this PR introduce a breaking change?

What level of testing was included in the change?

Which logical domain(s) does this change affect?

codecov-commenter commented Dec 22, 2023 • edited Loading

Codecov Report

hlomzik left a comment

Choose a reason for hiding this comment

hlomzik Dec 26, 2023

Choose a reason for hiding this comment

Gondragos Dec 27, 2023

Choose a reason for hiding this comment

hlomzik Dec 26, 2023

Choose a reason for hiding this comment

Gondragos Dec 27, 2023

Choose a reason for hiding this comment

hlomzik Jan 30, 2024

Choose a reason for hiding this comment

hlomzik Jan 30, 2024

Choose a reason for hiding this comment

Gondragos commented Jan 30, 2024 • edited by robot-ci-heartex Loading

codecov-commenter commented Dec 22, 2023 •

edited

Loading

Gondragos commented Jan 30, 2024 •

edited by robot-ci-heartex

Loading