Fixed Price/Spec/Interim not set in AR Manage Analyses #593

ramonski · 2018-01-22T15:14:41Z

Description of the issue/feature this PR addresses

Linked issue: #544

Note for Reviewers:
Please step through the Commits for a better understanding

Current behavior before PR

Prices are not updated in "Manage Analyses"
Interim Fields of ASs are not set on update
Missing Spec Error when the AR was copied
JS error on "Manage Analyses" form

Desired behavior after PR is merged

Field set the all offered inputs on the Analyses

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

xispa · 2018-01-22T15:53:56Z

bika/lims/browser/fields/aranalysesfield.py

+
+        # Bail out if the AR is in a frozen state
+        ar_state = api.get_workflow_status_of(instance, state_var="review_state")
+        if ar_state not in ALLOWED_STATES:


inactive or cancelled states shouldn't be allowed. I would prefer to not give the responsibility to the developer to know about states unless strictly necessary. Thus, instead of having a var ALLOWED_STATES I suggest to use the functions isActive and wasTransitionPerformed from bika.lims.workflow to replace this condition by the following:

if not isActive(instance) or wasTransitionPerformed(instance, 'verify'): raise ValueError( ...

NB: I know, camelCase kind of functions, but no worries, bika.lims.workflowwill be revisited soon

Not sure if I missed now something, but inactive and cancelled would raise here a ValueError.

Regarding isActive there is a pendant in the API already:
https://github.com/senaite/senaite.core/blob/master/bika/lims/api.py#L834

Didn't know yet about the wasTransitionPerformed function...

Ahh, regarding inactive and cancelled, these are in another Workflow... Got it!

Re isActive I know there is a function for this in the api already, but I did notice this after isActive was there. But no worries, I have in mind to conciliate them soon

The reason of a wasTransitionPerformed is that verify transition (as well as others) is a "no-way-back" transition, so this code will always work also if an additional state is added later (e.g. "printed" after/before "published", etc.)

xispa · 2018-01-22T15:57:05Z

bika/lims/browser/fields/aranalysesfield.py

+        # Convert the items to a valid list of AnalysisServices
+        services = filter(None, map(self._to_service, items))
+        # Service UIDs
+        service_uids = map(api.get_uid, services)


Maybe it shouldn't be necessary, but in the original file, we were also removing empties:

service_uids = filter(None, service_uids)

It was introduced here ARAnalysesField setter doesn't accept Analysis objects #431. I don't recall about the reason (if any).

Line 159 removes empties, so 161 should never contain empty UIDs, unless the DB is broke;)

Hehe, maybe this was the reason of adding that check... It was bad, indeed.

xispa · 2018-01-22T16:06:08Z

bika/lims/browser/fields/aranalysesfield.py

+                continue
+
+            # Skip Analyses in frozen states
+            if self._is_frozen(analysis):


_is_frozen function is only used once, here. As per a previous comment, cancelled and inactive analyses should be considered as "frozen" states too. I'd replace this condition by the following:

if not isActive(instance) or wasTransitionPerformed(instance, 'verify'):

Or make this the logic inside _is_frozen and use it in L153 too

xispa · 2018-01-22T16:07:43Z

bika/lims/browser/fields/aranalysesfield.py

+                continue
+
+            # If it is assigned to a worksheet, unassign it before deletion.
+            if self._is_assigned_to_worksheet(analysis):


The following should give you the same result:

if getCurrentState(analysis) == 'assigned':

I prefer using small functions with mnemonic names to make the code more understandable and maintainable, even if it is used just once. I see them as small component blocks which I can reuse elsewhere and finally factor it out from here.

That's fine!

xispa · 2018-01-22T16:14:19Z

bika/lims/browser/fields/aranalysesfield.py

+        :param service: Analysis Service Object
+        """
+        service_interims = service.getInterimFields()
+        analysis.setInterimFields(service_interims)


The interim fields assigned to an AnalysisService are a copy of the interim fields of the calculation the service is associated to: when you create/edit an AnalysisService and assign a calculation, the system gets the interim fields from the Calculation and assigns them to the field with the same name of the Analysis Service. Although done with javascript (much better if it was done in python), this is fine, but what happens if you change the Calculation after it was previously assigned to an Analysis Service? The Analysis Service still has the interim fields from the calculation before it was changed. If I am not wrong, this is what the previous code was trying to overcome. I agree the previous approach was odd, but this scenario must be considered. I suggest to just update the interim fields from associated Analysis Services when a Calculation is updated.

And if you can add a doctest with this use-case, then it would be awesome!

The interim fields of the Analysis are only taken from the Analysis Service only, see here:

https://github.com/senaite/senaite.core/pull/593/files#diff-df7750f2a3730c33aaa0df4851641f7dR474

If the Interim Fields of the Calculation is changed, it reflects directly to each Analysis of the AS – not sure if dragons are sleeping here.

The code before did actually mesh the interims of both together in the Analysis, which is in my opinion wrong (if it would have set it, which it did not)

I am not sure the interim fields from AS get updated automatically when they are changed on his Calculation counterpart. If yes, then I totally agree with analysis.setInterimFields(service.getInterimFields()). Give me a couple of hours to double-check.

AS interims are currently not affected by the calculation Interims, see here in the doctest:

https://github.com/senaite/senaite.core/pull/593/files#diff-df7750f2a3730c33aaa0df4851641f7dR496

Therefore, I think putting them together on the Analysis doesn't make sense, but I've not analyzed the dependent code parts any further (If those merge the interims together or not)

xispa · 2018-01-22T20:07:23Z

bika/lims/tests/doctests/ARAnalysesField.rst

+
+    >>> analysisservice4.setInterimFields([interim2])
+    >>> map(lambda x: x["keyword"], analysisservice4.getInterimFields())
+    ['B']


If you want to omit the tricky workaround in ARAnalysisField regarding interim (something I agree), then this should be like follows:

>>> map(lambda x: x["keyword"], analysisservice4.getInterimFields()) ['A', 'B']

Remember that at AS level, you can override the default value for an Interim defined in the Calculation, so you could have an interim called "Ca" in calculation with a default value of 0.1 and with the attribute "Hidden" checked whilst you could have an interim with the same keyword ("Ca"), but with default value 0.5 and "Hidden=False". So, InterimFields set at AS level have always priority over the interims set at Calculation level.

If you add an interim to a Calculation, InterimFields field from all Analysis Services that have that Calculation assigned, must be updated accordingly. New interims that were not present in AnalysisService must be added accordingly (otherwise, the system will not be able to calculate the result). Those interims removed from Calculation must be removed from the AnalysisService. Those interims that were added directly in the Analysis Service that do not match with the interims set in Calcultion must be preserved.

You could argue that well, leaving this like this, the system still works. Yes, agree, but because we have to always take this priority thing with interims into consideration (so at some point, it works by accident). See this as an example:

https://github.com/senaite/senaite.core/blob/master/bika/lims/browser/calcs.py#L190-L205

First, we get the interim fields from the Calculation and just after mapping them, we override with the values from the analysis.... Wouldn't it be better to just get rid of calc.getInterimFields()and reduce complexity? The code would look much better and clean, with just only one loop.

xispa · 2018-01-22T20:08:52Z

bika/lims/tests/doctests/ARAnalysesField.rst

+The Analysis also inherits the Interim Fields of the Analysis Service:
+
+    >>> map(lambda x: x["keyword"], analysis.getInterimFields())
+    ['B']


But I don't want to rely on Calculation to get the interim fields from the analysis. So, I'd expect the following:

>>> map(lambda x: x["keyword"], analysis.getInterimFields()) ['A', 'B']

I always assume that analysis.getInterimFields()always return the interim fields that must be used. I don't care about what Calculation says on this regard, because interim fields from analysis always have priority over the ones that are defined in Calculation.

xispa · 2018-01-22T22:28:16Z

A reminder that the following things must be addressed in a further PR:

Analysis.getInterimFields() must not rely on Calculation, rather must always return all required interims, keeping in mind that those from Analysis Service have priority over those defined in the original Calculation.
Following the same reasoning, AnalysisService.getInterimFields()always returns the required interims and the logic to resolve inconsistencies amongst the interims assigned to the associated Calculation and those defined at AnalysisService are resolved in this same function.
The point above brings the opportunity to decouple Calculation from Analysis (removal of HistoryAwareReferenceField from https://github.com/senaite/senaite.core/blob/master/bika/lims/content/abstractroutineanalysis.py#L104) by adding a StringField where the formula from the Calculation is stored at Analysis creation time: for the calculation, only Analysis' interims + formula are required, so the link with Calculation can be dropped and this logic simplified.

See these comment threads for more detailed info:

ramonski added 7 commits January 21, 2018 17:37

Set price in ARAnalysesField + added Doctest

ec50076

Fixed: Error not set on AR copy

cd3ac7a

Fixed numeric field event

15a4a06

Refactored Field Code

cbc74ca

Set AS Interims to ANs on update

2d0af4b

Added Worksheet test

8080506

Removed wrong parameter

852b6e7

xispa added Bug 🐞 P4: Nice To Have labels Jan 22, 2018

Updated Changelog

3436f71

xispa requested changes Jan 22, 2018

View reviewed changes

ramonski added 3 commits January 22, 2018 18:10

Fixed doctest labels

34a9db3

Refactored WF checks

615dadb

Fixed docstrings

7b67744

xispa reviewed Jan 22, 2018

View reviewed changes

ramonski added 3 commits January 22, 2018 21:23

Handle Calculation Dependencies

648ec12

Set interims only on Analysis creation

f13b421

Comments only

c475c78

xispa approved these changes Jan 22, 2018

View reviewed changes

xispa merged commit bf97407 into master Jan 22, 2018

xispa deleted the i544 branch February 2, 2018 00:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Price/Spec/Interim not set in AR Manage Analyses #593

Fixed Price/Spec/Interim not set in AR Manage Analyses #593

ramonski commented Jan 22, 2018

xispa Jan 22, 2018

ramonski Jan 22, 2018

ramonski Jan 22, 2018

xispa Jan 22, 2018

xispa Jan 22, 2018

xispa Jan 22, 2018

ramonski Jan 22, 2018

xispa Jan 22, 2018

xispa Jan 22, 2018

xispa Jan 22, 2018

ramonski Jan 22, 2018

xispa Jan 22, 2018

xispa Jan 22, 2018 •

edited

Loading

xispa Jan 22, 2018

ramonski Jan 22, 2018 •

edited

Loading

xispa Jan 22, 2018

ramonski Jan 22, 2018

xispa Jan 22, 2018

xispa Jan 22, 2018

xispa Jan 22, 2018

xispa commented Jan 22, 2018

Fixed Price/Spec/Interim not set in AR Manage Analyses #593

Fixed Price/Spec/Interim not set in AR Manage Analyses #593

Conversation

ramonski commented Jan 22, 2018

Description of the issue/feature this PR addresses

Current behavior before PR

Desired behavior after PR is merged

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xispa Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonski Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xispa commented Jan 22, 2018

xispa Jan 22, 2018 •

edited

Loading

ramonski Jan 22, 2018 •

edited

Loading