Skip to content
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

Added "Maximum holding time" setting to services and analyses #2624

Merged
merged 33 commits into from
Nov 19, 2024
Merged

Conversation

xispa
Copy link
Member

@xispa xispa commented Oct 8, 2024

Description of the issue/feature this PR addresses

This Pull Request allows the user to set the maximum holding time before it does no longer make sense to perform a given test. In the context of sample analysis, maximum holding time refers to the maximum duration a sample can be stored after collection before it must be analyzed to ensure reliable and accurate results. This period is established to prevent degradation, contamination, or chemical changes that could alter the sample's composition

For instance, testing for arboviral illnesses like Dengue, Zika, and Chikungunya might not make sense 5 days after the collection date. Likewise, identification of pathogenic microorganisms in a food sample won't make sense e.g. 2d after collection.

A new field "Maximum holding time" has been added to services and analyses:

Captura de 2024-10-31 17-01-56

On sample creation form, when the user selects a collection date that makes not possible to conduct a given analysis within the holding time, the service becomes not available for selection and an icon is displayed instead, along with an informative text:

Captura de 2024-10-08 14-12-17

Service is still available for selection from "Manage analyses" view though.

An alert icon is also displayed in analyses listing either when the result was captured past the holding time limit or when the holding time is about to expire:

Captura de 2024-10-10 10-58-46

--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.

@xispa xispa marked this pull request as ready for review October 10, 2024 09:19
@xispa xispa requested a review from ramonski October 10, 2024 09:19
@xispa xispa marked this pull request as draft October 10, 2024 09:20
@xispa xispa removed the request for review from ramonski October 10, 2024 09:20
@xispa xispa changed the title Analytical holding time functionality Added Maximum Holding Time setting to analyses Oct 31, 2024
@xispa xispa changed the title Added Maximum Holding Time setting to analyses Added "Maximum holding time" setting to analyses Oct 31, 2024
@xispa xispa changed the title Added "Maximum holding time" setting to analyses Added "Maximum holding time" setting to services and analyses Oct 31, 2024
@xispa xispa marked this pull request as ready for review October 31, 2024 16:07
@xispa xispa requested a review from ramonski October 31, 2024 16:07
Copy link
Contributor

@ramonski ramonski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool stuff!

I played around with the Date Sampled field after the sample was created with an Analysis contained that had a max holding time of 1 day set. After I changed to the date of today, I got the following traceback:

Traceback (innermost last):
  Module ZPublisher.WSGIPublisher, line 176, in transaction_pubevents
  Module ZPublisher.WSGIPublisher, line 385, in publish_module
  Module ZPublisher.WSGIPublisher, line 288, in publish
  Module ZPublisher.mapply, line 85, in mapply
  Module ZPublisher.WSGIPublisher, line 63, in call_object
  Module senaite.app.listing.view, line 246, in __call__
  Module senaite.app.listing.ajax, line 113, in handle_subpath
  Module senaite.core.decorators, line 40, in decorator
  Module senaite.app.listing.decorators, line 63, in wrapper
  Module senaite.app.listing.decorators, line 50, in wrapper
  Module senaite.app.listing.decorators, line 100, in wrapper
  Module senaite.app.listing.ajax, line 383, in ajax_folderitems
  Module senaite.app.listing.decorators, line 88, in wrapper
  Module senaite.app.listing.ajax, line 259, in get_folderitems
  Module bika.lims.browser.analyses.view, line 808, in folderitems
  Module senaite.app.listing.view, line 982, in folderitems
  Module bika.lims.browser.analyses.view, line 797, in folderitem
  Module bika.lims.browser.analyses.view, line 1758, in _folder_item_holding_time
TypeError: can't compare datetime.datetime to str

for brain in uid_catalog(UID=profiles):
profile = api.get_object(brain)
uids.extend(profile.getServiceUIDs() or [])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to have this removed. I see that this is already handled by the setProfiles method in the sample content.

Copy link
Contributor

@ramonski ramonski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it with profiles and templates and it worked perfectly.
The only issue seem to be when changing afterwards the Date sampled to today in the sample that has such an Analysis Service added

@xispa
Copy link
Member Author

xispa commented Nov 5, 2024

Cool stuff!

I played around with the Date Sampled field after the sample was created with an Analysis contained that had a max holding time of 1 day set. After I changed to the date of today, I got the following traceback:

Traceback (innermost last):
  Module ZPublisher.WSGIPublisher, line 176, in transaction_pubevents
  Module ZPublisher.WSGIPublisher, line 385, in publish_module
  Module ZPublisher.WSGIPublisher, line 288, in publish
  Module ZPublisher.mapply, line 85, in mapply
  Module ZPublisher.WSGIPublisher, line 63, in call_object
  Module senaite.app.listing.view, line 246, in __call__
  Module senaite.app.listing.ajax, line 113, in handle_subpath
  Module senaite.core.decorators, line 40, in decorator
  Module senaite.app.listing.decorators, line 63, in wrapper
  Module senaite.app.listing.decorators, line 50, in wrapper
  Module senaite.app.listing.decorators, line 100, in wrapper
  Module senaite.app.listing.ajax, line 383, in ajax_folderitems
  Module senaite.app.listing.decorators, line 88, in wrapper
  Module senaite.app.listing.ajax, line 259, in get_folderitems
  Module bika.lims.browser.analyses.view, line 808, in folderitems
  Module senaite.app.listing.view, line 982, in folderitems
  Module bika.lims.browser.analyses.view, line 797, in folderitem
  Module bika.lims.browser.analyses.view, line 1758, in _folder_item_holding_time
TypeError: can't compare datetime.datetime to str

Fixed with 48e7631

Note I use ANSIs to compare dates. Is useful for sorting and for preventing the infamous TypeError: can't compare offset-naive and offset-aware datetimes when comparing a TZ-naive datetime with a non-naive.

However, I now realized that this might lead to inconsistencies when the TZ of the collection date (DateSampled) is different from system's TZ.

To overccome this problem I am considering to add the param to_uc=False in to_ansi function, so it always tries to convert to Universal Time before formatting. This saves the developer from all dance re TZ conversion and naive cases before to_ansi is called. Or even better, I can use the parameter tz=None instead, so if tz is set, the system will try to convert to that TZ. Umm let me try this.

@xispa xispa marked this pull request as draft November 5, 2024 22:01
```
Traceback (innermost last):
  Module ZPublisher.WSGIPublisher, line 176, in transaction_pubevents
  Module ZPublisher.WSGIPublisher, line 385, in publish_module
  Module ZPublisher.WSGIPublisher, line 288, in publish
  Module ZPublisher.mapply, line 85, in mapply
  Module ZPublisher.WSGIPublisher, line 63, in call_object
  Module senaite.app.listing.view, line 246, in __call__
  Module senaite.app.listing.ajax, line 113, in handle_subpath
  Module senaite.core.decorators, line 40, in decorator
  Module senaite.app.listing.decorators, line 63, in wrapper
  Module senaite.app.listing.decorators, line 50, in wrapper
  Module senaite.app.listing.decorators, line 100, in wrapper
  Module senaite.app.listing.ajax, line 383, in ajax_folderitems
  Module senaite.app.listing.decorators, line 88, in wrapper
  Module senaite.app.listing.ajax, line 259, in get_folderitems
  Module bika.lims.browser.analyses.view, line 808, in folderitems
  Module senaite.app.listing.view, line 982, in folderitems
  Module bika.lims.browser.analyses.view, line 797, in folderitem
  Module bika.lims.browser.analyses.view, line 1737, in _folder_item_holding_time
  Module senaite.core.api.dtime, line 262, in to_ansi
  Module senaite.core.api.dtime, line 382, in to_zone
  Module pytz, line 188, in timezone
UnknownTimeZoneError: '+02'
```
@xispa xispa marked this pull request as ready for review November 6, 2024 16:46
@xispa
Copy link
Member Author

xispa commented Nov 6, 2024

Note I use ANSIs to compare dates. Is useful for sorting and for preventing the infamous TypeError: can't compare offset-naive and offset-aware datetimes when comparing a TZ-naive datetime with a non-naive.

However, I now realized that this might lead to inconsistencies when the TZ of the collection date (DateSampled) is different from system's TZ.

To overccome this problem I am considering to add the param to_uc=False in to_ansi function, so it always tries to convert to Universal Time before formatting. This saves the developer from all dance re TZ conversion and naive cases before to_ansi is called. Or even better, I can use the parameter tz=None instead, so if tz is set, the system will try to convert to that TZ. Umm let me try this.

Pheew... had to go through api.dtime and resolve some inconsistencies with TZ conversions. Hope it becomes cleaner now.

Copy link
Contributor

@ramonski ramonski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works like a charm, many thanks for this!

@ramonski ramonski merged commit 7be1e36 into 2.x Nov 19, 2024
2 checks passed
@ramonski ramonski deleted the hold-time branch November 19, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants