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

System does not validate values from Results Options to be different #1594

Merged
merged 5 commits into from Jun 14, 2020

Conversation

xispa
Copy link
Member

@xispa xispa commented Jun 13, 2020

Description of the issue/feature this PR addresses

This Pull Request makes the Result Options validator (analysis service) to also check for duplicates in result texts and values

Linked issue: #1278

Current behavior before PR

User can set up Result Options for an analysis service with duplicated values and texts.

Desired behavior after PR is merged

User cannot set up Result Options for an analysis service with duplicated values and texts.

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

@xispa xispa added the Bug 🐞 label Jun 13, 2020
# Result values must be unique
value = api.to_float(value)
values = map(lambda ro: ro.get("ResultValue"), records)
values = filter(api.is_floatable, values)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line can be removed

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, actually not sure. Did you add that for backwards compatibility with non-floatable values? But shouldn't fail then the other validator? or does that one come first?🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

The validator is triggered sequentially for all values registered for the subfield ResultValue (param value). Note the first thing I do in the function is to check if the value is floatable. If not, it returns the validation error. By filtering here only floatable values, those that are not "valid" are not considered (they have been handled already or they will be handled later, I don't care). The outcome is that no additional/duplicate error messages are rised.

TL;DR below

This validator only applies to "ResultValue" subfield, that stores the 'real' value used internally when an item from the display list is selected on results entry. This is the value that is eventually stored in analysis' Result field.

The other validator is for the "ResultText" subfield, that not necessarily has to be a floatable value. Quite often is an string, since is the value "displayed" in the selection list on results entry.

Basically, when the Analysis Service form is submitted, the system calls the two validators for each ResultOptions record, one for ResultValue subfield and the other one for ResultText field (param value from the function).

ResultValue must be floatable cause in fact, is the value that will be stored as the result. Note that therefore, it might be used in calculations, even if the result was entered by means of a selection list

These two validators are now independent, but there were a single validator before. Having a single validator has the problem that if a value for a subfield is wrong (either ResultText or ResultValue), the error message will be displayed twice, cause the validator machinery triggers the function twice, one for the value of ResultValue subfield and another for the value of ResultText subfield. Since there was a single validator that was checking everything regardless of the value passed-in and the subfield, two validation failures were happening (and the err message was displayed twice).

@ramonski ramonski merged commit 8851718 into master Jun 14, 2020
@ramonski ramonski deleted the i1278 branch June 14, 2020 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants