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

Commit

Permalink
fix: LEAP-447: Fix Number tag validation (#1649)
Browse files Browse the repository at this point in the history 
* 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]>
  • Loading branch information
@Gondragos @yyassi-heartex
Gondragos and yyassi-heartex authored Jan 30, 2024
1 parent 91c7b33 commit 524aa7d
Show file tree
Hide file tree
Showing 10 changed files with 503 additions and 147 deletions.
29 changes: 29 additions & 0 deletions src/mixins/PerItem.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
24 changes: 24 additions & 0 deletions src/mixins/PerRegion.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
},
Expand Down
100 changes: 53 additions & 47 deletions src/mixins/Required.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
60 changes: 60 additions & 0 deletions src/tags/control/ClassificationBase.js
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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,
Expand Down
51 changes: 14 additions & 37 deletions src/tags/control/DateTime.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
},
};
});
Expand Down
Loading

0 comments on commit 524aa7d

Please sign in to comment.