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

BadRequest error when changing Calculation on Analysis Service #598

Merged
merged 3 commits into from
Jan 23, 2018

Conversation

mikejmets
Copy link
Contributor

Description of the issue/feature this PR addresses

Linked issue: #597

Current behavior before PR

BadRequest Error thrown on AS listing

Desired behavior after PR is merged

No error on AS listing

--
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 Recursion broken in getCalculationDependants BadRequest error when changing Calculation on Analysis Service Jan 23, 2018
Copy link
Member

@xispa xispa left a comment

Choose a reason for hiding this comment

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

Well spotted!

@xispa xispa merged commit fda9f9b into senaite:master Jan 23, 2018
@ramonski
Copy link
Contributor

I think that change is not right, because setting a mutable default argument (in this case the list) is a bad idea. A new list is created once when the function is defined, and the same list is used in each successive call.

I would therefore recommend that we use the version from AR Add2 to calculate the dependencies: https://github.com/senaite/senaite.core/blob/master/bika/lims/browser/analysisrequest/add2.py#L519

@ramonski
Copy link
Contributor

Please see this article for details: http://docs.python-guide.org/en/latest/writing/gotchas

@mikejmets mikejmets deleted the lims-597-badrequest branch March 31, 2023 09:26
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.

3 participants