Fix samples are indicated as late when Turnaround Time is zero #2569
Conversation
There was a problem hiding this comment.
Many thanks @DanE417 for your suggestion. I think you can make it way simpler, please check the comments!
Also, please add an entry at the top of the changelog: https://github.com/senaite/senaite.core/blob/2.x/CHANGES.rst
src/senaite/core/tests/doctests/AnalysisTurnaroundTimeForZeroValues.rst
Outdated
Show resolved
Hide resolved
src/senaite/core/tests/doctests/AnalysisTurnaroundTimeForZeroValues.rst
Outdated
Show resolved
Hide resolved
src/senaite/core/tests/doctests/AnalysisTurnaroundTimeForZeroValues.rst
Outdated
Show resolved
Hide resolved
src/senaite/core/tests/doctests/AnalysisTurnaroundTimeForZeroValues.rst
Outdated
Show resolved
Hide resolved
xispa
changed the title
|
Hi @DanE417 , any news regarding this PR? |
There was a problem hiding this comment.
Thanks @DanE417 , everything looks ok now, but before we can merge you need to do the following:
- Make the tests pass (see comment below)
- Add an upgrade step to:
The upgrade step is necessary because existing samples and analyses will still be indicated as late in listings. Reason is that they use the value returned for getDueDate metadata to render the late icon. Likewise, the getDueDate index is used in searches and to sort them in listings too.
The issue only happens for services their TAT is 0 days, 0 hours and 0 minutes. Therefore, the upgrade step will be faster if you search only for analyses their service uid is one of those. You can use the getServiceUID index to search analyses by service uid.
src/senaite/core/tests/doctests/AnalysisTurnaroundTimeForZeroValues.rst
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This will take ages to complete cause you are
- iterating over all analyses to fully reindex all them
- iterating over all samples to fully reindex all them
As suggested before, please
- stick to those analyses that might be affected by this TAT issue only
- and only reindex the samples from those analyses that were affected
- do not do a full reindex (
obj.reindexObject()), but reindexgetDueDateindex only and update metadata. For this, use thecatalog_objectfunction instead, e.g.:analysis_catalog.reindexObject(analysis, idxs=['getDueDate'], update_metadata=1)
Also,
- Since this may take a lot of time to complete, is a good idea to log the progress of the upgrade (e.g. https://github.com/senaite/senaite.core/blob/2.x/src/senaite/core/upgrade/v02_06_000.py#L2436-L2439 )
- Since a lot of objects might be waken up, is also a good idea to flush every object from memory after reindex (e.g. https://github.com/senaite/senaite.core/blob/2.x/src/senaite/core/upgrade/v02_06_000.py#L2445)
| @@ -2065,6 +2065,18 @@ def migrate_sampletypes_to_dx(tool): | |||
|
|
|||
| logger.info("Convert SampleTypes to Dexterity [DONE]") | |||
|
|
|||
| @upgradestep(product, version) | |||
There was a problem hiding this comment.
Remove this upgradestep decorator from here, cause what this function reindex_getDueDate does is not likely to break other add-ons
| sample_uids = [] | ||
| for num, brain in enumerate(brains): | ||
| if num and num % 100 == 0: | ||
| logger.info("Reindexing getDueDate index from analyses catalog {0}/{1}" |
There was a problem hiding this comment.
PEP 8: E501 line too long (82 > 79 characters)
| logger.info("Reindexing getDueDate index from analyses catalog {0}/{1}" | ||
| .format(num, total)) | ||
| obj = api.get_object(brain) | ||
| if api.to_minutes(**obj.MaxTimeAllowed) == 0: |
There was a problem hiding this comment.
Never use the attribute directly, but the accessor instead:
max_time = obj.getMaxTimeAllowed()| .format(num, total)) | ||
| obj = api.get_object(brain) | ||
| if api.to_minutes(**obj.MaxTimeAllowed) == 0: | ||
| brain.reindexObject(obj, idxs=['getDueDate'], update_metadata=1) |
There was a problem hiding this comment.
you are calling reindexObject from brain instead. Do the following:
obj.reindexObject(idxs=["getDueDate"])update_metadata defaults to 1 already
| brain.reindexObject(obj, idxs=['getDueDate'], update_metadata=1) | ||
| sample_uid = obj.aq_parent.UID() | ||
| if sample_uid not in sample_uids: | ||
| sample_uids.append(sample_uid) |
There was a problem hiding this comment.
Better declare sample_uids as a set. So you can then skip the check and directly do the following:
sample_uids.add(sample_uid)| obj = api.get_object(brain) | ||
| if api.to_minutes(**obj.MaxTimeAllowed) == 0: | ||
| brain.reindexObject(obj, idxs=['getDueDate'], update_metadata=1) | ||
| sample_uid = obj.aq_parent.UID() |
There was a problem hiding this comment.
Note that reference analyses do not live inside AnalysisRequest objects, but inside ReferenceSample objects. Rely on IRequestAnalysis interface to skip those non-affected.
if not IRequestAnalysis.providedBy(obj):
continueMove this check to the top of the loop, before the reindexObject, cause non-request analyses are not affected by this issue. If you look carefully, getDueDate from ReferenceAnalysis returns the expiration date of the reference sample: https://github.com/senaite/senaite.core/blob/2.x/src/bika/lims/content/referenceanalysis.py#L90
There are the DuplicateAnalysis objects as well. So better you use the getRequest function from the analysis to get the sample the analysis belongs to:
sample = obj.getRequest()
sample_uid = api.get_uid(sample)| obj = api.get_object_by_uid(sample_uid) | ||
| obj.reindexObject(idxs=['getDueDate']) | ||
| obj._p_deactivate() | ||
| logger.info("Reindexing getDueDate index from samples catalog ...") |
There was a problem hiding this comment.
Remove this line, you already printed this log before
| sample_uids.append(sample_uid) | ||
| obj._p_deactivate() | ||
| logger.info("Reindexing getDueDate index from analyses catalog [DONE]") | ||
| logger.info("Reindexing getDueDate index from samples catalog ...") |
There was a problem hiding this comment.
A blank line before this line will improve the readability
Description of the issue/feature this PR addresses
Linked issue: #2568
Current behavior before PR
Samples are shown as late when having a Turnaround Time of zero.
Desired behavior after PR is merged
Samples are not shown as late when having a Turnaround Time of zero.
--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.