-
-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix reference definition range validation #2452
Conversation
return "% Error must be between 0 and 100" | ||
return None | ||
|
||
validation.register(ReferenceValuesValidator()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This referencevalues_validator
is still called at https://github.com/senaite/senaite.core/blob/fix-referencedefinition-values-validation/src/bika/lims/content/referencesample.py#L173
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks for searching over the codebase once again! Fixed in b75d84e
Do you think it is worth to make a migration step for this? |
Good point!. Yes, I think is worth. I don't expect instances to have a lot of Reference Definitions, so the upgrade step would not take too much to complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note we can still have min > max values in reference definition when user inputs a negative percentage:
We have some options here:
- Move this code after line 234
- Do
s_err=abs(float(s_err))
before line 232. In such case, we might need to also do"error": str(s_err)
at line 236. - Rather consider negative percentage as a feature and leave it as it is
What do you think?
With c23508d we do the |
Description of the issue/feature this PR addresses
This PR fixes the field validation for the result, min and max values of the reference definition ranges.
Current behavior before PR
max
value or below themin
value afterwards.Desired behavior after PR is merged
--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.