Refactored and fixed inconsistencies with Analysis Turnaround Time logic #1032
Conversation
Erliness is the remaining time *in minutes* for an analysis to be completed based on its maximum turn around time. The function was not returning the value in minutes.
It was using the date the sample partition was received instead.
bika/lims/browser/analyses/view.py
Outdated
| @@ -591,6 +591,8 @@ def _folder_item_duedate(self, analysis_brain, item): | |||
| # returns the date when the ReferenceSample expires. If the analysis is | |||
| # a duplicate, `getDueDate` returns the due date of the source analysis | |||
| due_date = analysis_brain.getDueDate | |||
| if not due_date: | |||
| return | |||
There was a problem hiding this comment.
better return None to be more explicit
| :return: the time in minutes that exceeds the maximum turnaround time | ||
| :rtype: int | ||
| """ | ||
| return -self.getEarliness() |
There was a problem hiding this comment.
Why - here? This does not match with the docstring
There was a problem hiding this comment.
getEarliness is the remaining time in minutes for the TAT of the analysis to be reached. If reached (the analysis is late), getEarliness returns the time in negative..... so getLateness becomes the negative of getEarliness
| if (resultdate and resultdate > duedate) \ | ||
| or (not resultdate and DateTime() > duedate): | ||
| for analysis in self.getAnalyses(full_objects=True, retracted=False): | ||
| if analysis.isLateAnalysis(): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
You could also do something like this:
any(filter(lambda an: an.isLateAnalysis(), self.getAnalyses(full_objects=True, retracted=False)))
There was a problem hiding this comment.
I first did something similar, but more bizarre:
ans = filter(lambda an: an.isLateAnalysis(), self.getAnalyses(full_objects=True, retracted=False))
return len(ans) > 0but later, I decided to use the for loop because there is no need to filter all analyses: I thought the function would be faster if I return immediately after the first late analysis is found.
bika/lims/api/__init__.py
Outdated
| @@ -1265,6 +1266,33 @@ def to_date(value, default=None): | |||
| return to_date(default) | |||
|
|
|||
|
|
|||
| def to_minutes(days=0, hours=0, minutes=0, seconds=0, milliseconds=0, | |||
| to_int=True): | |||
There was a problem hiding this comment.
I would leave in general the type conversion to the calling function and return only a string here.
Also that to_int does actually also a round which is not directly obvious to the caller
There was a problem hiding this comment.
Umm, don't like to do int(round(api.get_minutes(**kwargs))) everywhere. I suggest to change the param name from to_int to round_to_int
bika/lims/api/__init__.py
Outdated
| """ | ||
| minutes = to_minutes(days=days, hours=hours, minutes=minutes, | ||
| seconds=seconds, milliseconds=milliseconds) | ||
| delta = timedelta(minutes= int(round(minutes))) |
| return: a dictionary with the keys "days", "hours" and "minutes" | ||
| """ | ||
| tat = self.Schema().getField("MaxTimeAllowed").get(self) | ||
| return tat or self.bika_setup.getDefaultTurnaroundTime() |
There was a problem hiding this comment.
This does not allow to set a custom MaxTimeAllowed of 0. Better use return tat is not None or self.bika_setup.getDefaultTurnaroundTime()
There was a problem hiding this comment.
I think Turnaround Time of 0 does not make sense. I mean, I would not expect the labman to want an analysis to be resolved in less than a minute since the sample was received at the lab. Thus, I though this check was not necessary and I assumed that either if MaxTimeAllowed is 0 or None, the result is the same: return the system default
There was a problem hiding this comment.
Makes sense, but in code be better explicit to distinguish between None values (not set) and a user added int value w/o the assumption that it evaluates to False
There was a problem hiding this comment.
MaxTimeAllowed field is a DurationField, that inherits from RecordsField. It stores an array with a dictionary in position 0 with "days", "hours" and "minutes" as keys. So, getMaxTimeAllowed won't never return an int of 0, rather an empty array or an array with an empty dictionary in position 0. Thus, I think is better to keep this code as it is.
There was a problem hiding this comment.
That's true. I forgot about that these fields are record fields...
Description of the issue/feature this PR addresses
Superseds the following PRs:
Revisit, fixing and cleanup of functions and logic related with Analysis Turnaround Time.
--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.