Result Value to not be fixed to integer #1957

Ruhanga · 2022-04-04T15:01:47Z

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.

ramonski

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.

ramonski · 2022-04-26T11:56:15Z

src/bika/lims/api/__init__.py

@@ -89,7 +89,7 @@

 _marker = object()

-UID_RX = re.compile("[a-z0-9]{32}$")
+UID_RX = re.compile("[a-zA-Z0-9\\-]{32,36}$")


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.

Ohhh ok. Maybe leaving this untouched and shifting this logic to section needing it in the validators.py might suffice.

ramonski · 2022-04-26T11:56:30Z

src/bika/lims/api/__init__.py

@@ -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):


Same here. Why 36?

This to allow for the uuid standard specified in Section 3 of RFC4122.

ramonski · 2022-04-26T12:01:49Z

src/bika/lims/content/abstractbaseanalysis.py

                    'ResultText': 25,},
-    subfield_maxlength={'ResultValue': 5,
+    subfield_maxlength={'ResultValue': 40,
                        'ResultText': 255,},


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

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.

ramonski · 2022-04-26T12:02:26Z

src/bika/lims/validators.py

@@ -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"))


Confusing, because when I type in a number with 37 digits it will fail as well

To resolve this the underlying field could enforce a max field length required.

ramonski · 2022-04-26T12:40:11Z

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.

Ruhanga · 2022-04-27T11:30:25Z

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

xispa · 2022-04-27T13:08:48Z

@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 uuid module is used), the values are stored in hex (and the uppercase criteria is superfluous in any case):

>>> 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 api.is_uid in senaite ecosystem gives us more than 40 matches. Some of these calls are done in functions that are widely used too... so... no chance at all for this change to go in.

Ruhanga · 2022-04-27T14:03:36Z

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 uid(options-value) ➙ Option Text(display text) maps from the UI which is not the case for the default Results Options configuration. Illustrating this with a snapshot.

It'd suffice if alpha-numeric values are supported and not being limited to numeric values.

rbuisson · 2022-04-27T15:03:49Z

Indeed it sounds limiting to consider that the Result Value can only be a integer. At least, allowing alphanumerical characters sounds fair.
Result value could be convention of coded values to be later interpreted in reports or in integrated systems, that is not as user friendly as the Display Value.

Result Value to not be fixed to integer

356a0c9

Ruhanga mentioned this pull request Apr 4, 2022

Creating Analysis Service result options with non numeric values fails/not accepted #1956

Open

ramonski requested changes Apr 26, 2022

View reviewed changes

ramonski closed this Apr 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Result Value to not be fixed to integer #1957

Result Value to not be fixed to integer #1957

Ruhanga commented Apr 4, 2022

ramonski left a comment

ramonski Apr 26, 2022

Ruhanga Apr 27, 2022

ramonski Apr 26, 2022

Ruhanga Apr 27, 2022

ramonski Apr 26, 2022

Ruhanga Apr 27, 2022

ramonski Apr 26, 2022

Ruhanga Apr 27, 2022

ramonski commented Apr 26, 2022

Ruhanga commented Apr 27, 2022 •

edited

Loading

xispa commented Apr 27, 2022

Ruhanga commented Apr 27, 2022

rbuisson commented Apr 27, 2022

Result Value to not be fixed to integer #1957

Result Value to not be fixed to integer #1957

Conversation

Ruhanga commented Apr 4, 2022

Description of the issue/feature this PR addresses

Current behavior before PR

Desired behavior after PR is merged

ramonski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonski commented Apr 26, 2022

Ruhanga commented Apr 27, 2022 • edited Loading

xispa commented Apr 27, 2022

Ruhanga commented Apr 27, 2022

rbuisson commented Apr 27, 2022

Ruhanga commented Apr 27, 2022 •

edited

Loading