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

Fix analyses from partitions are considered in manage analyses view #2598

Draft
wants to merge 11 commits into
base: 2.x
Choose a base branch
from

Conversation

xispa
Copy link
Member

@xispa xispa commented Jul 29, 2024

Description of the issue/feature this PR addresses

This Pull Request comes as an answer to #2593 (comment) by ensuring that from "Manage analyses" view is only possible to add/remove analyses that belong to current sample, leaving analyses from partitions out of the equation. Likewise, analyses from parent sample are not being considered when managing analyses from a partition.

Current behavior before PR

Manage analyses view takes analyses from partitions into account

Desired behavior after PR is merged

Manage analyses view does not take analyses from partitions into account

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

@xispa xispa requested a review from ramonski July 29, 2024 09:21
@xispa xispa added the Bug 🐞 label Jul 29, 2024
@xispa xispa marked this pull request as draft July 29, 2024 09:37
@xispa xispa marked this pull request as ready for review July 29, 2024 10:32
@xispa xispa marked this pull request as draft July 29, 2024 14:10
@xispa xispa marked this pull request as ready for review November 2, 2024 16:58
@ramonski
Copy link
Contributor

I think there was a concern regarding this PR, but I don't see it in the comments here. Can you remember? I'll check it otherwise later.

@ramonski
Copy link
Contributor

Ah, probably that you cannot enter anymore results from the root sample, but need to go directly to the partition instead 🤔

@xispa
Copy link
Member Author

xispa commented Jan 31, 2025

Ah, probably that you cannot enter anymore results from the root sample, but need to go directly to the partition instead 🤔

It is possible to enter results from the root sample. See below:

Captura de 2025-01-31 20-08-13

I've been playing around and I've realized that if you have a given test (e.g. Ca) in a partition, you cannot longer add another test Ca in the root sample. You can select the checkbox in manage analysis, and press "Save" afterwards, but nothing happens.

@xispa
Copy link
Member Author

xispa commented Jan 31, 2025

The default behaviour before this PR when adding a service to a partition via "Manage Analyses" is that the system first checks for a counterpart analysis in the root sample. If found, it moves the analysis to the partition instead of creating a new one.

This is ok for when a partition is being created, but once created, user should be able to add analyses in the partition regardless of the root. Likewise, the addition of analyses in root should not entail changes in the partition. Since this "dependency" logic is encoded in the datamanager, is not possible to make "Manage analyses" work properly for root sample and partitions without breaking the partition creation functionality.

Therefore, I think that the logic for moving analyses from root to partitions should not reside in the datamanager, but explicitly handled on partition creation.

Will revisit and modify this PR and apply the changes in accordance to ensure system works as expected in both scenarios.

Related: #1505

@xispa xispa marked this pull request as draft January 31, 2025 23:12
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.

2 participants