diff --git a/src/mixins/PerItem.js b/src/mixins/PerItem.js index 3c9672585..118d1b8b2 100644 --- a/src/mixins/PerItem.js +++ b/src/mixins/PerItem.js @@ -22,6 +22,35 @@ const PerItemMixin = types }, })) .actions(self => ({ + /** + * Validates all values related to the current classification per item (Multi Items Segmentation). + * + * - This method should not be overridden. + * - It is used only in validate method of the ClassificationBase mixin. + * + * @returns {boolean} + * @private + */ + _validatePerItem() { + const objectTag = self.toNameTag; + + return self.annotation.regions + .every((reg) => { + const result = reg.results.find(s => s.from_name === self); + + if (!result?.hasValue) { + return true; + } + const value = result.mainValue; + const isValid = self.validateValue(value); + + if (!isValid) { + objectTag.setCurrentItem(reg.item_index); + return false; + } + return true; + }); + }, createPerItemResult() { self.createPerObjectResult({ item_index: self.toNameTag.currentItemIndex, diff --git a/src/mixins/PerRegion.js b/src/mixins/PerRegion.js index f3233ef23..4c7d6da66 100644 --- a/src/mixins/PerRegion.js +++ b/src/mixins/PerRegion.js @@ -55,6 +55,30 @@ const PerRegionMixin = types }, })) .actions(self => ({ + /** + * Validates all values related to the current classification per region. + * + * - This method should not be overridden. + * - It is used only in validate method of the ClassificationBase mixin. + * + * @returns {boolean} + * @private + */ + _validatePerRegion() { + const objectTag = self.toNameTag; + + for (const reg of objectTag.allRegs) { + const value = reg.results.find(s => s.from_name === self)?.mainValue; + const isValid = self.validateValue(value); + + if (!isValid) { + self.annotation.selectArea(reg); + return false; + } + } + + return true; + }, createPerRegionResult() { self.perRegionArea?.setValue(self); }, diff --git a/src/mixins/Required.js b/src/mixins/Required.js index 7aa276457..f5f0c1b19 100644 --- a/src/mixins/Required.js +++ b/src/mixins/Required.js @@ -6,71 +6,77 @@ const RequiredMixin = types required: types.optional(types.boolean, false), requiredmessage: types.maybeNull(types.string), }) - .actions(self => ({ - validate() { - if (!self.required) return true; + .actions(self => { + const Super = { + validate: self.validate, + }; - if (self.perregion) { + return { + validate() { + if (!Super.validate()) return false; + if (!self.required) return true; + + if (self.perregion) { // validating when choices labeling is done per region, // for example choice may be required to be selected for // every bbox - const objectTag = self.toNameTag; + const objectTag = self.toNameTag; - // if regions don't meet visibility conditions skip validation - for (const reg of objectTag.allRegs) { - const s = reg.results.find(s => s.from_name === self); + // if regions don't meet visibility conditions skip validation + for (const reg of objectTag.allRegs) { + const s = reg.results.find(s => s.from_name === self); - if (self.visiblewhen === 'region-selected') { - if (self.whentagname) { - const label = reg.labeling?.from_name?.name; + if (self.visiblewhen === 'region-selected') { + if (self.whentagname) { + const label = reg.labeling?.from_name?.name; - if (label && label !== self.whentagname) continue; + if (label && label !== self.whentagname) continue; + } } - } - if (self.whenlabelvalue && !reg.hasLabel(self.whenlabelvalue)) { - continue; - } + if (self.whenlabelvalue && !reg.hasLabel(self.whenlabelvalue)) { + continue; + } - if (!s?.hasValue) { - self.annotation.selectArea(reg); - self.requiredModal(); + if (!s?.hasValue) { + self.annotation.selectArea(reg); + self.requiredModal(); - return false; + return false; + } } - } - } else if (isFF(FF_LSDV_4583) && self.peritem) { + } else if (isFF(FF_LSDV_4583) && self.peritem) { // validating when choices labeling is done per item, - const objectTag = self.toNameTag; - const maxItemIndex = objectTag.maxItemIndex; - const existingResultsIndexes = self.annotation.regions - .reduce((existingResultsIndexes, reg) => { - const result = reg.results.find(s => s.from_name === self); + const objectTag = self.toNameTag; + const maxItemIndex = objectTag.maxItemIndex; + const existingResultsIndexes = self.annotation.regions + .reduce((existingResultsIndexes, reg) => { + const result = reg.results.find(s => s.from_name === self); - if (result?.hasValue) { - existingResultsIndexes.add(reg.item_index); - } - return existingResultsIndexes; - }, new Set()); + if (result?.hasValue) { + existingResultsIndexes.add(reg.item_index); + } + return existingResultsIndexes; + }, new Set()); - for (let idx = 0; idx <= maxItemIndex; idx++) { - if (!existingResultsIndexes.has(idx)) { - objectTag.setCurrentItem(idx); - self.requiredModal(); - return false; + for (let idx = 0; idx <= maxItemIndex; idx++) { + if (!existingResultsIndexes.has(idx)) { + objectTag.setCurrentItem(idx); + self.requiredModal(); + return false; + } } - } - } else { + } else { // validation when its classifying the whole object // isVisible can be undefined (so comparison is true) or boolean (so check for visibility) - if (!self.holdsState && self.isVisible !== false && getParent(self, 2)?.isVisible !== false) { - self.requiredModal(); - return false; + if (!self.holdsState && self.isVisible !== false && getParent(self, 2)?.isVisible !== false) { + self.requiredModal(); + return false; + } } - } - - return true; - }, - })); + return true; + }, + }; + }); export default RequiredMixin; diff --git a/src/tags/control/ClassificationBase.js b/src/tags/control/ClassificationBase.js index a62a7a6e6..9036db43e 100644 --- a/src/tags/control/ClassificationBase.js +++ b/src/tags/control/ClassificationBase.js @@ -1,4 +1,5 @@ import { types } from 'mobx-state-tree'; +import { FF_LSDV_4583, isFF } from '../../utils/feature-flags'; /** * This is a mixin for a control-tag that is a base of creating classification-like tags. @@ -40,6 +41,65 @@ const ClassificationBase = types.model('ClassificationBase', { }; }).actions(self => { return { + /** + * Validates the input based on certain conditions. + * + * Generally, this method does not need to be overridden. And you need to override the validateValue method instead. + * However, there are exceptions. For example, RequiredMixin, Choices, and + * Taxonomy have their own additional logic, for which a broader context is needed. + * In this case, the parent method call is added at the beginning or end + * of the method to maintain all functionality in a predictable manner. + * + * @returns {boolean} + */ + validate() { + if (self.perregion) { + return self._validatePerRegion(); + } else if (self.peritem && isFF(FF_LSDV_4583)) { + return self._validatePerItem(); + } else { + return self._validatePerObject(); + } + }, + /** + * Validates the value. + * + * Override to add your custom validation logic specific for the tag. + * Per-item, per-region and per-object validation will be applied automatically. + * + * @example + * SomeModel.actions(self => { + * const Super = { validateValue: self.validateValue }; + * + * return { + * validateValue(value) { + * if (!Super.validateValue(value)) return false; + * // your validation logic + * } + * // other actions + * } + * }); + * + * @param {*} value - The value to be validated. + * @returns {boolean} + * + */ + // eslint-disable-next-line @typescript-eslint/no-unused-vars + validateValue(value) { + return true; + }, + /** + * Validates all values related to the current classification per object. + * + * - This method should not be overridden. + * - It is used only in validate method of the ClassificationBase mixin. + * + * @returns {boolean} + * @private + */ + _validatePerObject() { + return self.validateValue(self.selectedValues()); + }, createPerObjectResult(areaValues = {}) { self.annotation.createResult( areaValues, diff --git a/src/tags/control/DateTime.js b/src/tags/control/DateTime.js index 8cfdda637..60131e37e 100644 --- a/src/tags/control/DateTime.js +++ b/src/tags/control/DateTime.js @@ -293,53 +293,30 @@ const Model = types }, })) .actions(self => { - const Super = { validate: self.validate }; + const Super = { validateValue: self.validateValue }; return { - validate() { - if (!Super.validate()) return false; + validateValue(value) { + if (!Super.validateValue(value)) return false; - const { min, max } = self; - - if (!min && !max) return true; - - function validateValue(value) { - const errors = []; - - if (!value) return true; + const errors = []; - let date = self.getISODate(value); + if (!value) return true; - if (self.only?.includes('year')) date = date.slice(0, 4); + let date = self.getISODate(value); - if (min && date < min) errors.push(`min date is ${min}`); - if (max && date > max) errors.push(`max date is ${max}`); + if (self.only?.includes('year')) date = date.slice(0, 4); - if (errors.length) { - InfoModal.warning(`Date "${date}" is not valid: ${errors.join(', ')}.`); - return false; - } - return true; - } - - // per-region results are not visible, so we have to check their values - if (self.perregion) { - const objectTag = self.toNameTag; - - for (const reg of objectTag.allRegs) { - const date = reg.results.find(s => s.from_name === self)?.mainValue; - const isValid = validateValue(date); + const { min, max } = self; - if (!isValid) { - self.annotation.selectArea(reg); - return false; - } - } + if (min && date < min) errors.push(`min date is ${min}`); + if (max && date > max) errors.push(`max date is ${max}`); - return true; - } else { - return validateValue(self.datetime); + if (errors.length) { + InfoModal.warning(`Date "${date}" is not valid: ${errors.join(', ')}.`); + return false; } + return true; }, }; }); diff --git a/src/tags/control/Number.js b/src/tags/control/Number.js index 13656929e..b1734886c 100644 --- a/src/tags/control/Number.js +++ b/src/tags/control/Number.js @@ -39,7 +39,7 @@ import { FF_LSDV_4583, isFF } from '../../utils/feature-flags'; * @param {number} [step=1] - Step for value increment/decrement * @param {number} [defaultValue] - Default number value; will be added automatically to result for required fields * @param {string} [hotkey] - Hotkey for increasing number value - * @param {boolean} [required=false] - Whether to require number validation + * @param {boolean} [required=false] - Whether number is required or not * @param {string} [requiredMessage] - Message to show if validation fails * @param {boolean} [perRegion] - Use this tag to classify specific regions instead of the whole object * @param {boolean} [perItem] - Use this tag to classify specific items inside the object instead of the whole object[^FF_LSDV_4583] @@ -72,80 +72,116 @@ const Model = types return isDefined(self.number); }, })) - .actions(self => ({ - getSelectedString() { - return self.number + ' star'; - }, + .actions(self => { + const Super = { validateValue: self.validateValue }; - needsUpdate() { - if (self.result) self.number = self.result.mainValue; - else self.number = null; - }, + return { + validateValue(value) { + if (!Super.validateValue(value)) return false; + if (!isDefined(value)) return true; - beforeSend() { - if (!isDefined(self.defaultvalue)) return; + const errors = []; - // let's fix only required perRegions - if (self.perregion && self.required) { - const object = self.toNameTag; + if (isDefined(self.min) && value < self.min) { + errors.push(`Value must be greater than or equal to ${self.min}`); + } + if (isDefined(self.max) && value > self.max) { + errors.push(`Value must be less than or equal to ${self.max}`); + } + if (isDefined(self.step)) { + const step = parseFloat(self.step); + const basis = isDefined(self.min) ? +self.min : 0; + const delta = (value - basis) % step; - for (const reg of object?.allRegs ?? []) { - // add result with default value to every region of related object without number yet - if (!reg.results.some(r => r.from_name === self)) { - reg.results.push({ - area: reg, - from_name: self, - to_name: object, - type: self.resultType, - value: { - [self.valueType]: +self.defaultvalue, - }, - }); + if (delta !== 0) { + errors.push(`The two nearest valid values are ${value - delta} and ${value - delta + step}`); } } - } else { + if (errors.length) { + InfoModal.warning(`Number "${value}" is not valid: ${errors.join(', ')}.`); + return false; + } + return true; + }, + getSelectedString() { + return self.number + ' star'; + }, + + needsUpdate() { + if (self.result) self.number = self.result.mainValue; + else self.number = null; + }, + + beforeSend() { + if (!isDefined(self.defaultvalue)) return; + + // let's fix only required perRegions + if (self.perregion && self.required) { + const object = self.toNameTag; + + for (const reg of object?.allRegs ?? []) { + // add result with default value to every region of related object without number yet + if (!reg.results.some(r => r.from_name === self)) { + reg.results.push({ + area: reg, + from_name: self, + to_name: object, + type: self.resultType, + value: { + [self.valueType]: +self.defaultvalue, + }, + }); + } + } + } else { // add defaultValue to results for top-level controls - if (!isDefined(self.number)) self.setNumber(+self.defaultvalue); - } - }, + if (!isDefined(self.number)) self.setNumber(+self.defaultvalue); + } + }, - unselectAll() {}, + unselectAll() {}, - setNumber(value) { - self.number = value; - self.updateResult(); - }, + setNumber(value) { + self.number = value; + self.updateResult(); + }, - onChange(e) { - const value = +e.target.value; + onChange(e) { + const value = +e.target.value; - if (!isNaN(value)) self.setNumber(value); - }, + if (!isNaN(value)) { + self.setNumber(value); + // without this line we can have `7` in model field while it's displayed as `007`. + // at least it is bad for testing cases + e.target.value = isDefined(self.number) ? self.number : ''; + } + }, - updateFromResult() { - this.needsUpdate(); - }, + updateFromResult() { + this.needsUpdate(); + }, - requiredModal() { - InfoModal.warning(self.requiredmessage || `Number "${self.name}" is required.`); - }, + requiredModal() { + InfoModal.warning(self.requiredmessage || `Number "${self.name}" is required.`); + }, - increaseValue() { - if (self.number >= Number(self.max)) { - self.setNumber(0); - } else { - if (self.number > 0) { - self.setNumber(self.number + 1); + increaseValue() { + if (self.number >= Number(self.max)) { + self.setNumber(0); } else { - self.setNumber(1); + if (self.number > 0) { + self.setNumber(self.number + 1); + } else { + self.setNumber(1); + } } - } - }, + }, - onHotKey() { - return self.increaseValue(); - }, - })); + onHotKey() { + return self.increaseValue(); + }, + }; + }); const NumberModel = types.compose('NumberModel', ControlBase, diff --git a/tests/functional/data/control_tags/number.ts b/tests/functional/data/control_tags/number.ts new file mode 100644 index 000000000..0c309cec3 --- /dev/null +++ b/tests/functional/data/control_tags/number.ts @@ -0,0 +1,75 @@ +export const simpleData = { + text: 'The Answer to the Ultimate Question of Life, the Universe, and Everything', +}; + +export const simpleMIGData = { + images: [ + 'https://data.heartex.net/open-images/train_0/mini/0030019819f25b28.jpg', + 'https://data.heartex.net/open-images/train_0/mini/00155094b7acc33b.jpg', + 'https://data.heartex.net/open-images/train_0/mini/00133643bbf063a9.jpg', + 'https://data.heartex.net/open-images/train_0/mini/0061ec6e9576b520.jpg', + ], +}; + +export const numberConfigMinMax = ` + + +`; + +export const numberPerRegionConfig = ` + + + + +`; + +export const numberPerItemConfig = ` + + +`; + +export const numberResult = [ + { + id: 'n2ldmNpSQI', + from_name: 'answer', + to_name: 'text', + type: 'number', + value: { + number: 42, + }, + }, +]; + +export const numberPerRegionResult = [ + { + 'value': { + 'start': 4, + 'end': 10, + 'text': 'Answer', + 'labels': [ + 'Word', + ], + }, + 'id': 'oAfls5zbRH', + 'from_name': 'label', + 'to_name': 'text', + 'type': 'labels', + 'origin': 'manual', + }, + { + 'value': { + 'start': 63, + 'end': 73, + 'text': 'Everything', + 'labels': [ + 'Word', + ], + }, + 'id': '6ogw2y5n_S', + 'from_name': 'label', + 'to_name': 'text', + 'type': 'labels', + 'origin': 'manual', + }, +]; diff --git a/tests/functional/package.json b/tests/functional/package.json index 18646b694..aa80aa99a 100644 --- a/tests/functional/package.json +++ b/tests/functional/package.json @@ -20,7 +20,7 @@ "cvg:summary": "nyc report --temp-dir=.nyc_output --reporter=text-summary --cwd=. --exclude-after-remap false" }, "dependencies": { - "@heartexlabs/ls-test": "heartexlabs/ls-frontend-test#9ff949fc19abaf124477dfd41c6f230b00665ab3" + "@heartexlabs/ls-test": "heartexlabs/ls-frontend-test#2619550d365dbfdb2b29fc07cf5ca70278a6928f" }, "devDependencies": { "ts-loader": "^9.4.2", diff --git a/tests/functional/specs/control_tags/number.cy.ts b/tests/functional/specs/control_tags/number.cy.ts new file mode 100644 index 000000000..9a58b036e --- /dev/null +++ b/tests/functional/specs/control_tags/number.cy.ts @@ -0,0 +1,149 @@ +import { + numberConfigMinMax, numberPerItemConfig, + numberPerRegionConfig, + numberPerRegionResult, + numberResult, + simpleData, simpleMIGData +} from 'data/control_tags/number'; +import { ImageView, LabelStudio, Modals, Number, Sidebar, ToolBar } from '@heartexlabs/ls-test/helpers/LSF'; +import { FF_LSDV_4583 } from '../../../../src/utils/feature-flags'; + +describe('Control Tags - Number', () => { + it('should set value', () => { + LabelStudio.params() + .config(numberConfigMinMax) + .data(simpleData) + .withResult([]) + .init(); + + LabelStudio.waitForObjectsReady(); + + Number.type('42'); + Number.hasValue('42'); + LabelStudio.serialize().then(result => { + expect(result[0].value.number).to.be.eq(42); + }); + }); + + it('should load value', () => { + LabelStudio.params() + .config(numberConfigMinMax) + .data(simpleData) + .withResult(numberResult) + .init(); + + LabelStudio.waitForObjectsReady(); + + Number.hasValue('42'); + }); + + it('should be limited by min/max', () => { + LabelStudio.params() + .config(numberConfigMinMax) + .data(simpleData) + .withResult([]) + .init(); + + LabelStudio.waitForObjectsReady(); + + Number.type('126{upArrow}{upArrow}{upArrow}'); + Number.hasValue('128'); + + Number.selectAll(); + Number.type('6{downArrow}{downArrow}{downArrow}{downArrow}{downArrow}'); + Number.hasValue('2'); + }); + it('should show errors for min validation', () => { + LabelStudio.params() + .config(numberConfigMinMax) + .data(simpleData) + .withResult([]) + .init(); + + LabelStudio.waitForObjectsReady(); + + Number.selectAll(); + Number.type('-100'); + ToolBar.clickSubmit(); + Modals.hasWarning('-100'); + Modals.hasWarning('Value must be greater than or equal to 2'); + }); + + it('should show errors for max validation', () => { + LabelStudio.params() + .config(numberConfigMinMax) + .data(simpleData) + .withResult([]) + .init(); + + LabelStudio.waitForObjectsReady(); + + Number.selectAll(); + Number.type('148'); + ToolBar.clickSubmit(); + Modals.hasWarning('148'); + Modals.hasWarning('Value must be less than or equal to 128'); + }); + + it('should show errors for step validation', () => { + LabelStudio.params() + .config(numberConfigMinMax) + .data(simpleData) + .withResult([]) + .init(); + + LabelStudio.waitForObjectsReady(); + + // Check step=2 + Number.selectAll(); + Number.type('43'); + ToolBar.clickSubmit(); + Modals.hasWarning('43'); + Modals.hasWarning('The two nearest valid values are 42 and 44.'); + }); + + it('should show errors for perRegion validation', () => { + LabelStudio.params() + .config(numberPerRegionConfig) + .data(simpleData) + .withResult(numberPerRegionResult) + .init(); + + LabelStudio.waitForObjectsReady(); + + Sidebar.toggleRegionSelection(0); + Number.type('43'); + + Sidebar.toggleRegionSelection(1); + Number.type('8'); + + + cy.log('43 is not a multiple of 2, which is set as a step in the configuration, so there should be a warning on submit'); + ToolBar.clickSubmit(); + Modals.hasWarning('43'); + }); + + it('should show errors for perItem validation', () => { + LabelStudio.addFeatureFlagsOnPageLoad({ + [FF_LSDV_4583]: true, + }); + + LabelStudio.params() + .config(numberPerItemConfig) + .data(simpleMIGData) + .withResult([]) + .init(); + + LabelStudio.waitForObjectsReady(); + + ImageView.paginationNextBtn.click(); + Number.type('3'); + ImageView.paginationNextBtn.click(); + + ToolBar.clickSubmit(); + Modals.hasWarning('3'); + Modals.closeWarning(); + // Check that we are at the right page that contains the error + Number.hasValue('3'); + }); +}); diff --git a/tests/functional/yarn.lock b/tests/functional/yarn.lock index 017549734..a001bcd03 100644 --- a/tests/functional/yarn.lock +++ b/tests/functional/yarn.lock @@ -264,9 +264,9 @@ resolved "https://registry.yarnpkg.com/@discoveryjs/json-ext/-/json-ext-0.5.7.tgz#1d572bfbbe14b7704e0ba0f39b74815b84870d70" integrity sha512-dBVuXR082gk3jsFp7Rd/JI4kytwGHecnCoTtXFb7DB6CNHp4rg5k1bhg0nWdLGLnOV71lmDzGQaLMy8iPLY0pw== -"@heartexlabs/ls-test@heartexlabs/ls-frontend-test#9ff949fc19abaf124477dfd41c6f230b00665ab3": +"@heartexlabs/ls-test@heartexlabs/ls-frontend-test#2619550d365dbfdb2b29fc07cf5ca70278a6928f": version "1.0.8" - resolved "https://codeload.github.com/heartexlabs/ls-frontend-test/tar.gz/9ff949fc19abaf124477dfd41c6f230b00665ab3" + resolved "https://codeload.github.com/heartexlabs/ls-frontend-test/tar.gz/2619550d365dbfdb2b29fc07cf5ca70278a6928f" dependencies: "@cypress/code-coverage" "^3.10.0" "@cypress/webpack-preprocessor" "^5.17.0" @@ -288,9 +288,9 @@ webpack-cli "^5.0.1" yargs "^17.7.1" -"@heartexlabs/ls-test@heartexlabs/ls-frontend-test#df8e7ea65179f5d143a4a299428116646f2236c4": +"@heartexlabs/ls-test@heartexlabs/ls-frontend-test#a96c5052e67ec807732508b03b8d858f5ef1f220": version "1.0.8" - resolved "https://codeload.github.com/heartexlabs/ls-frontend-test/tar.gz/df8e7ea65179f5d143a4a299428116646f2236c4" + resolved "https://codeload.github.com/heartexlabs/ls-frontend-test/tar.gz/a96c5052e67ec807732508b03b8d858f5ef1f220" dependencies: "@cypress/code-coverage" "^3.10.0" "@cypress/webpack-preprocessor" "^5.17.0"