-
-
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
Result Value to not be fixed to integer #1957
Conversation
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.
At least for the standard GUI approach this seems to me not like the right way to do, because no user would type in a UID as a result option. However, I'm not sure if I understood what your are trying to accomplish completely.
Therefore, at least for me it seems to make more sense in a private add-on instead of the core.
@@ -89,7 +89,7 @@ | |||
|
|||
_marker = object() | |||
|
|||
UID_RX = re.compile("[a-z0-9]{32}$") | |||
UID_RX = re.compile("[a-zA-Z0-9\\-]{32,36}$") |
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 seems to be wrong at this place, as long as we talk of UIDs in the sense how Plone/Senaite generates them. Changing this regular expression will lead to unexpected results/side-effects on other places where we rely on this.
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.
Ohhh ok. Maybe leaving this untouched and shifting this logic to section needing it in the validators.py might suffice.
@@ -1236,7 +1236,7 @@ def is_uid(uid, validate=False): | |||
return False | |||
if uid == '0': | |||
return True | |||
if len(uid) != 32: | |||
if not (len(uid) >= 32 and len(uid) <= 36): |
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.
Same here. Why 36?
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 to allow for the uuid standard specified in Section 3 of RFC4122.
'ResultText': 25,}, | ||
subfield_maxlength={'ResultValue': 5, | ||
subfield_maxlength={'ResultValue': 40, | ||
'ResultText': 255,}, |
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.
I agree that 5 might be too small, but at probably saves some horizontal screen space when an analysis has many interims defined as well.
Off topic: It would be great to have some JS that allows fields to grow according to their value
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.
Indeed @ramonski, and looks like the underlying html/js-template consumes these values under the hood which could be modified to do as you suggest.
@@ -642,7 +642,8 @@ class ResultOptionsValueValidator(object): | |||
def __call__(self, value, *args, **kwargs): | |||
# Result Value must be floatable | |||
if not api.is_floatable(value): | |||
return _t(_("Result Value must be a number")) | |||
if not api.is_uid(value): | |||
return _t(_("Result Value must be a number or valid UID")) |
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.
Confusing, because when I type in a number with 37 digits it will fail as well
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.
To resolve this the underlying field could enforce a max field length required.
After reviewing the code and discussion, we believe that this code fits better into a custom add-on than in the core. Therefore, we are closing it here. |
Hi @ramonski, @xispa thank you for the review. The use case requiring this change, which is integration oriented, is to address a requirement that would potentially meet the need of any one interested in integrating their systems with senaite like we do @ozone-his. This intuitively makes it necessary for collaborative effort towards improving senaite lims where applicable and other can benefit alike, than having variants of the product through custom add-ons. We are however open to guidance towards achieving our use-case with no/minimal consequences on senaite lims. cc: @rbuisson |
@Ruhanga this change request does not provide any apparent functionality to senaite. As @ramonski stated, I cannot see a user manually typing a [U]UID as a result option. And if this was the requirement, a reference widget or a selection list should be used instead. In any case, these changes are a ticking time-bomb. Please note that although the generation of UIDs at SENAITE adheres to RFC4122 (python's >>> import uuid
>>> uid = uuid.uuid4()
>>> uid
UUID('9a0b5564-288d-4aa7-8c47-341d2afdcbff')
>>> uid.hex
'9a0b5564288d4aa78c47341d2afdcbff'
>>> from bika.lims import api
>>> api.is_uid(uid)
False
>>> api.is_uid(uid.hex)
True
>>> senaite = app.senaite
>>> clients = senaite.clients
>>> clients.UID()
'149656918a214b5d9c30d04753f97a95'
>>> api.is_uid(clients.UID())
True and a search by |
Ohh, thanks @xispa for the prompt and detailed response. I might be missing something or be misunderstood. Part of the reason behind requesting for this change is that it's actually possible with interim fields to configure multiple results/options as It'd suffice if alpha-numeric values are supported and not being limited to numeric values. |
Indeed it sounds limiting to consider that the |
Description of the issue/feature this PR addresses
Linked issue: #1956
Current behavior before PR
Results Options widget does not accept non-numeric values for it's options.
Desired behavior after PR is merged
Results Options fields should accept non-numeric values(at-least valid uids) for it's options just like the the Interim Results fields do.
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.