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 inactive services are added via profile on sample creation or edition #2623

Merged
merged 4 commits into from
Oct 5, 2024

Conversation

xispa
Copy link
Member

@xispa xispa commented Oct 3, 2024

Description of the issue/feature this PR addresses

This Pull Request fixes a side-effect of #2492 , so that inactivated services were no longer removed from existing panels on inactivation. Amongst other undesired effects, this was causing user was able to add inactive services on sample creation through the selection of analysis profiles with those services assigned.

Current behavior before PR

Inactive services are added on sample creation/edition when a profile with inactive services is selected

Desired behavior after PR is merged

Inactive services are added on sample creation/edition when a profile with inactive services is selected

--
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 Oct 3, 2024
@xispa xispa requested a review from ramonski October 3, 2024 16:36
Copy link
Contributor

@ramonski ramonski left a comment

Choose a reason for hiding this comment

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

Thanks for this fix and providing a test for it!
I just thought that it would be great to get an information displayed for the user to confirm the action before deactivation. But we'll do this in another PR.

@ramonski ramonski merged commit c35c16c into 2.x Oct 5, 2024
2 checks passed
@ramonski ramonski deleted the fix-inactive-service branch October 5, 2024 07:26
@xispa
Copy link
Member Author

xispa commented Oct 7, 2024

Thanks for this fix and providing a test for it! I just thought that it would be great to get an information displayed for the user to confirm the action before deactivation. But we'll do this in another PR.

I thought about this as well, but I think this is not how this whole thing should work. I think best would be to not update profiles on service inactivation, but make the system "active/inactive"-aware when required (e.g. when a profile is assigned on sample creation or edition). This could be partially achieved by changing the signature of the profile's getter to getServices(self, active_only=True) and revisit the setter to not remove inactive uids unless explicitly provided. This approach entails the following benefits:

  • no need to update/recatalog other objects via subscriber on service deactivation
  • service can be deactivated/reactivated and user won't need to manually reassign services to profiles later
  • the developer becomes responsible of the inclusion/exclusion of services depending on the use case
  • less code

Same principle would apply for SampleTemplate

This is for another PR though. I wanted this PR to only restore the functionality that was already there before #2492

@ramonski
Copy link
Contributor

ramonski commented Oct 7, 2024

That is a really great and smart idea!
We should exactly follow this approach to avoid the modification of references in the future.
Looking forward to get this implemented:)

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