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

Compliance of Analyses from Samples with Specification #1506

Merged
merged 65 commits into from
Jan 31, 2020

Conversation

xispa
Copy link
Member

@xispa xispa commented Jan 21, 2020

Description of the issue/feature this PR addresses

Before this Pull Request, the results ranges from the Specification set in the Sample were stored in ResultsRanges field from Sample, rather than individually to Analyses. The manual addition of analyses by using "Manage analyses" view was storing the "custom" values directly to Sample's ResultsRange field making them available to analyses added and those that were already there. As a result, some shortcomings have been detected:

  • User can manually change results ranges manually for analyses (via "Manage Analyses" view inside Sample), but Specification won't change. Thus, this drives to an inconsistency: analyses might not longer be compliant with the Specification set at Sample level, but user is not warned about this. This Pull Request looks for compliance and if the results ranges set manually are not compliant with the Specification, a viewlet is displayed and an informative icon is displayed next to the analysis' specification column.

  • If an Specification object is edited (e.g. modification of a results range for a given service), this change is not propagated to Samples with that Specification assigned. This Pull Request displays a notification panel to the user when such case happens. If user re-assign the Specification, the results ranges for analyses will be reseted to latest version.

  • When a results range for a given analysis is changed by means of "Manage analyses" view inside Sample and the analysis belongs to a partition, the result range of the analysis does not change. This Pull Request makes results ranges to be stored at analysis level. Therefore, the changes will be applied to analyses from partitions too.

Additions

  • Specification non-compliant viewlet: displayed when the Sample contains analyses that are not compliant with the Specification initially set.

  • Sample results ranges out-of-date: displayed when results ranges from Specification initially set is are different from current Specification version (Specification history-aware kind of thing).

  • Warning icon next to specification column in analyses listing that is displayed when analysis' results range has been manually changed and does not match with the Sample's.

Other issues addressed

Added doctests

Testing guidelines

  1. Create a Sample with some analyses
  2. Assign a Specification to the samle
  3. Play around with manage analyses (change ranges, add new ones, etc.)
  4. Modify the Specification object (add a new result range or modify the range for a service)
  5. Go back to the Sample, re-assign the Specification and keep an eye to changes in results ranges
  6. Create partitions and do same play around you've done before, but with partitions and root sample at same time

Screenshots:

Captura de 2020-01-23 20-32-51

Captura de 2020-01-23 20-33-32

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

@xispa xispa changed the title Compliance of Analyses from Samples with Specifications Compliance of Analyses from Samples with Specification Jan 21, 2020
Comment on lines +57 to +71
def set(self, instance, value, **kwargs):
from bika.lims.content.analysisspec import ResultsRangeDict
if isinstance(value, ResultsRangeDict):
# Better store a built-in dict so it will always be available even
# if ResultsRangeDict is removed or changed
value = dict(value)

super(ResultRangeField, self).set(instance, value, **kwargs)

def get(self, instance, **kwargs):
from bika.lims.content.analysisspec import ResultsRangeDict
value = super(ResultRangeField, self).get(instance, **kwargs)
if value:
return ResultsRangeDict(dict(value.items()))
return {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the internal imports?

Copy link
Member Author

Choose a reason for hiding this comment

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

I got a circular dependency, but now you ask, I think I can get rid of it

Copy link
Member Author

Choose a reason for hiding this comment

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

xispa and others added 10 commits January 31, 2020 00:39
Traceback (innermost last):
  Module ZPublisher.Publish, line 138, in publish
  Module ZPublisher.mapply, line 77, in mapply
  Module ZPublisher.Publish, line 48, in call_object
  Module bika.lims.browser.analysisrequest.add2, line 66, in decorator
  Module bika.lims.browser.analysisrequest.add2, line 734, in __call__
  Module bika.lims.browser.analysisrequest.add2, line 1673, in ajax_submit
TypeError: create_analysisrequest() got an unexpected keyword argument
'specifications'
This change displays an icon in the header table next to the fields that
have the `primary_bound` set to `True`.
The aim is to preserve the result ranges manually set to analyses
and to the Sample itself when the Save button is pressed in the
Sample view while the user has not changed the value for Specification
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.

Another piece of great code melted into the core 👍

@ramonski ramonski merged commit ca85bce into master Jan 31, 2020
@ramonski ramonski deleted the specs-inconsistencies branch January 31, 2020 16:53
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