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

Cannot verify calculated analyses if retracted dependencies #439

Merged
merged 10 commits into from
Dec 1, 2017

Conversation

xispa
Copy link
Member

@xispa xispa commented Dec 1, 2017

Description of the issue/feature this PR addresses

To determine if a given analysis can be transitioned from to_be_verified to verified state, the system checks if this analysis has dependencies (analyses the analysis depends on to calculate its result). If these dependencies, in turn can be transitioned from to_be_verified to verified or the transition verify for these dependencies has already been performed, then the system returns True, so the target analysis can be transitioned to verified state. Otherwise, the system, returns False (the target analysis cannot be transitioned to verified state.

All these checks live in bika.lims.workflow.analysis.events and specially in bika.lims.workflow.analysis.guards, cause the guard is always checked (as per DC workflow default behavior) when using DC's workflow tool to obtain the available transitions for a given state and obect.

When returning the dependencies of a given analysis, the system was including analyses that were retracted (or rejected), and because from a retracted/rejected status there is no possibility to reach neither to_be_verified or verified state, the system was not allowing the verify transition to analyses that had dependencies with retracted or rejected states.

With this PR, the system returns the dependencies excluding retracted/rejected analyses by default. The same criteria has been applied to dependents (analyses that depends on the target analysis). Because both getDependents and getDependencies rely on getSiblings the logic for this fix has been delegated to getSiblings function. The code has been inspected carefully from a functional point of view if there was any place that retracted dependents/depencies were required.

Current behavior before PR

The user cannot verify analyses with retracted dependencies

Desired behavior after PR is merged

The user can verify analyses with retracted dependencies

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

@xispa xispa requested a review from ramonski December 1, 2017 11:29
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.

Some minor changes only, the rest is looking fine

if analysis.getRequestUID() != requestuid:
continue

if retracted == False and in_state(analysis, retracted_states):
Copy link
Contributor

Choose a reason for hiding this comment

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

better use if retracted is False and in_state(analysis, retracted_states) (compare bool with is)

for sibling in ans:
if sibling.UID() == self.UID():
continue
if retracted == False and in_state(sibling, retracted_states):
Copy link
Contributor

Choose a reason for hiding this comment

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

better use if retracted is False and in_state(sibling, retracted_states) (compare bool with is)

if not states:
return False
obj_state = getCurrentState(obj, stateflowid=stateflowid)
return obj_state in states
Copy link
Contributor

Choose a reason for hiding this comment

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

Please note that getCurrentState returns an empty string as default (return wf.getInfoFor(obj, stateflowid, ''))

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I take note, this change requires a new PR imo, because has nothing to do with in_state rather with getCurrentState.

@xispa xispa merged commit 3032ec7 into senaite:master Dec 1, 2017
@xispa xispa deleted the verify-calculated-analyses branch December 1, 2017 18:33
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