Always allow interim fields to be added to Analysis Services

[ramonski]

Description of the issue/feature this PR addresses

Interim fields in Analysis Services are the only way, how to capture multiple results for one Analysis, e.g. 5 different areas in the sample like middle , right , left etc.

Linked issue: #751

Current behavior before PR

Interim fields are not visible when no calculation was selected

Desired behavior after PR is merged

Interim fields are always visible

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

[ramonski]

Welcome to the Jungle 🌴 🙈 🌴

This PR refactors the whole JS logic in Analysis Services.

The following functionality is given on the Analysis Service Edit View Analysis Tab:

• Toggling the checkbox Display a Detection Limit selector shows/hides the checkbox Allow Manual Detection Limit input. On de-select, the latter field is also deselected if it was selected.

The following functionality is given on the Analysis Service Edit View Container and Preservation Tab:

N.B.: Only the working code was ported (not too much)...

• Selecting a Sample Type in the records field copies copies the Required Volume value
• Selecting a Container in the records field copies the Preservation value and disables the field (no clue why?)
• Selecting a Default Container copies the Default Preservation value and disables the field (no clue why?)

The following functionality is given on the Analysis Service Edit View Methods Tab:

• Toggling the Instrument assignment is not required checkbox loads/unloads the Methods multi-select. When unchecked, only the None value is available in the Methods Multi-Select
• Selecting one or more Methods copies the Methods to the Default Method selection
• Toggling the Instrument assignment is allowed checkbox loads/unloads the Instruments multi-select. When unchecked, only the None value is available in the Instruments Multi-Select and the Default Instrument is set to None
• Selecting one or more Instruments copies the Instruments to the Default Instrument selection
• Toggling the Use the Default Calculation of Method auto-loads the default calculation of the method and set it to the Calculation select. When unchecked, all calculations are listed
• Selecting a Calculation loads its interim fields. Manual added interims (not part of the default calculation) are always preserved
• Selecting invalid/out-of-date instruments add an alert above the content
• All fields are loaded/filtered initially according to their initial values (please test this thoroughly)

Additional Notes

All methods that are available in the AnalysisServiceEditView JS Class can be accessed via the JS console via asv (Analysis Service View).

Example:

asv.is_instrument_assignment_allowed()
asv.get_methods()
asv.get_default_instrument()

[xispa]

First of all, thank you for dealing with this thing. This is one of the craziest and cumbersome js logic in core.

I've added some bits with comments and suggestions, feel free to decide if you want to apply them or not.

Apart from the code review, I've done functional testing for Analysis Service edit view and seems to work just fine. I've noticed the hide/display dance when method/instrument entry settings are changed are not performed as used to, but the lists are populated correctly. No worries is fine as it is. After playing a bit around, I created ARs, added results and did not have any problem. Anyhow, is quite difficult to test results entry view taking into consideration all possible options available for Analysis Services, so... you never know.

Haven't test the container/preservation logic tough... (in fact, not sure if that thing was working before).

I'd recommend to change the title "Calculation Interim Fields" by "Interim fields" ("Additional values" might be better - I always found "Interim" too cryptic).

Good exhausting work!.

[xispa]

Instrument content type has theinactive_workflow assigned, so I suggest to load only those with inactive_state='active'

[xispa]

sort_on='sortable_title would be nice too, as you did for Methods

[xispa]

I am thinking now what would happen if you have an Analysis Service with an instrument associated and someone inactivates the instrument afterwards... the Analysis Service will remain in inconsistent status. Would be nice to think about a guard for that transition that would return True only if the instrument is not assigned to any AS. Another option is to not filter by inactivate_state here, but in that case, the instrument should be added in the invalid_instruments dict on L#29

[ramonski]

Indeed, I added issue #829 to not forget about this

[xispa]

Would you mind to change this message by something like "Some of the selected instruments are out-of-date, with failed calibration tests or under maintenance".

Note that as per L#29:

`if instrument.Valid isnt "1"

Valid ComputedField delegates to isValid, that does the following:

    def isValid(self):
        """ Returns if the current instrument is not out for verification, calibration,
        out-of-date regards to its certificates and if the latest QC succeed
        """
        return self.isOutOfDate() is False \
            and self.isQCValid() is True \
            and self.getDisposeUntilNextCalibrationTest() is False \
            and self.isValidationInProgress() is False \
            and self.isCalibrationInProgress() is False

The conditions "DisposedUntilNextCalibrationTest", "ValidationInProgress" and "CalibrationInProgress" are temporary situations, so the suggested message would be more informative.

[xispa]

Same reasoning here about a guard for inactivate transition as explained for Instrument.

[ramonski]

This one was not used, so I removed it from the JS

[xispa]

Not critical, thinking loud. If the user has entered manual interims previously, all them are replaced by the ones that comes with the Calculation. Would be nice if system could keep the ones entered manually. A function like purge_interims could be in charge to remove only those interims that belong to the previous calculation (if any), and leave the rest. Then, using purge_interims together with a function add_interims (instead of set_interims) would do the job.

[ramonski]

Manual interims are always preserved, see set_interims.

However, I fixed a small issue, where "overwritten" interims, e.g. manual interims with the same keyword as the calculation interim, had different values, e.g. value or unit etc.
These interims are now merged on calculation change and preserved as "manual" interim

[xispa]

See my previous comment about keeping the interims manually set by using a purge_interims function or something.

[xispa]

Why we use /get_available_calculations here? you could use the @@API/read as you do below, for Methods (L#778) and Instruments (L#753). get_available_calculations is only used here, so as a bonus, you'll be able to remove the class bika.lims.browser.calcs.ajaxGetAvailableCalculations!

[xispa]

If you change this, remember that Calculation also uses inactive_workflow, so you should query only for those that are active.

[ramonski]

You are right. This method is a relict that remained from refactoring and nowhere used. I'm removing it...

[xispa]

This "Instrument assignment is not required"/"Manual entry of results" thing is totally weird. It shouldn't rely on the assignment/unassignment of methods, rather with instruments (if no instrument assigned, then there is no choice, the user must set the result manually)... but in turn, Method content type has the option "Manual entry" True/False, at the same time that can have none or multiple instruments associated.... so well, kind of an hydra we'll have to deal with in future.

[ramonski]

Hi Jordi, thanks for your review and feedback.
I implemented the changes that you suggested and fixed a bug as well.

Could you please functional check the following:

• Create 2 Calculations with the interim field A and B respectively
• Assign calculation A to AS (Interim A should be added automatically)
• Change the value or unit of Interim A in the AS and save
• Switch in the AS tab between the calculations A and B

Expected:

Interim A is always preserved on calculation change, but merged with the Calculation A's interim

[xispa]

I've done the functional test you mentioned regarding Calculations A + B and works as expected.

Nevertheless, I noticed that when you create an Analysis Service, the option "Use the Default Calculation of Method" is checked by default, so "None" option appears selected in the list for "Calculation" field. This is correct. If then I unselect "Use the Default Calculation of Method", the first available calculation appears selected in the list of calculations, but interims are not populated accordingly.

[ramonski]

Hi Jordi, good catch. I added this functionality.

[xispa]

A very good one!