Skip to content
This repository has been archived by the owner on Apr 18, 2024. It is now read-only.

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

Merged
merged 9 commits into from
Feb 22, 2024

Conversation

hlomzik
Copy link
Collaborator

@hlomzik hlomzik commented Feb 22, 2024

New validation system (#1649) started to invoke validateValue() from control tags, but Textarea had a different meaning for this method, validating only text just entered by user, so the format was different from the whole Textarea result and LSF was crashing. Also semantics is also different, so the method was simply renamed to not clash with new validation.

PR fulfills these requirements

  • Tests for the changes have been added/updated
  • Docs have been added/updated
  • 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

Describe the reason for change

LSF crashes on submit with Textarea tag with both required and skipDuplicates set to true.

What feature flags were used to cover this change?

skipDuplicates work only with fflag_feat_front_lsdv_4659_skipduplicates_060323_short ON

What alternative approaches were there?

To validate duplicated texts on submit as well, but that can block some existing annotations and should be done separately as a feature.

This change affects (describe how if yes)

  • Performance
  • Security
  • UX

Does this PR introduce a breaking change?

  • Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • It fixes breaking change

What level of testing was included in the change?

  • e2e (codecept)
  • integration (cypress)
  • unit (jest)

This method is used to validate text when it's added by user,
not to validate the entire value of Textarea result, it has different signature.
We could adjust it to validate duplicated texts on submit, but that's improvement
and we need just a fix right now.
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1a234f1) 68.79% compared to head (81dcb7c) 21.93%.
Report is 4 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #1705       +/-   ##
===========================================
- Coverage   68.79%   21.93%   -46.86%     
===========================================
  Files         443      441        -2     
  Lines       28793    28757       -36     
  Branches     7655     7527      -128     
===========================================
- Hits        19808     6308    -13500     
- Misses       7742    22449    +14707     
+ Partials     1243        0     -1243     

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

@hlomzik hlomzik enabled auto-merge (squash) February 22, 2024 11:54
@hlomzik hlomzik merged commit 313caaa into master Feb 22, 2024
12 of 13 checks passed
@hlomzik hlomzik deleted the fb-leap-724/required-textarea branch February 22, 2024 12:40
MasherJames pushed a commit to HelloPareto/label-studio-frontend that referenced this pull request Feb 29, 2024
* Adjust test to fail with required + skipDuplicates

* Fix test to really see the problem

* Run only this test for now

* Rename validateValue in Textarea to fix validation

This method is used to validate text when it's added by user,
not to validate the entire value of Textarea result, it has different signature.
We could adjust it to validate duplicated texts on submit, but that's improvement
and we need just a fix right now.

* Return all tests

* Unrelated doc for displayMode param + fix FF comment

* Fix linting and line endings in some tests

ocr + textarea.skip-duplicates

* Fix Textarea editing

* One more missed thing
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants